Skip to content

Add Variant serdeProxy support #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
WebFreak001 wants to merge 1 commit into from
Closed

Add Variant serdeProxy support #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 55 additions & 26 deletions source/mir/deser/package.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import mir.small_array;
import mir.small_string;
import mir.utility: _expect;
import std.traits: ForeachType, hasUDA, Unqual, isSomeChar, EnumMembers, TemplateArgsOf, getUDAs;
import mir.ion.conv : toProxy;

private alias AliasSeq(T...) = T;

Expand Down Expand Up @@ -380,7 +381,7 @@ template deserializeValue(string[] symbolTable)
Temporal proxy;
if (auto exception = impl(params, proxy))
return exception;
auto temporal = to!(serdeDeserializationMemberType!(T, member))(move(proxy));
auto temporal = toProxy!(serdeDeserializationMemberType!(T, member))(move(proxy));
static if (hasTransform)
transform(temporal);
__traits(getMember, value, member) = move(temporal);
Expand Down Expand Up @@ -442,6 +443,7 @@ template deserializeValue(string[] symbolTable)
if (!isFirstOrderSerdeType!T)
{with(params){
import mir.algebraic: isVariant, isNullable;
import mir.serde: ContainsProxied;
Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Contains!(staticMap!(serdeGetFinalProxy, Types))?

Copy link
Copy Markdown
Contributor Author

@WebFreak001 WebFreak001 Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, didn't think of that, but probably a lot simpler

import mir.internal.meta: Contains;
import mir.ndslice.slice: Slice, SliceKind;
import mir.rc.array: RCArray, RCI;
Expand Down Expand Up @@ -749,7 +751,6 @@ template deserializeValue(string[] symbolTable)
return error.ionException;
if (symbolId >= table.length)
return IonErrorCode.symbolIdIsTooLargeForTheCurrentSymbolTable.ionException;
import mir.conv: to;
auto elemParams = params.withData(elem);
serdeGetProxy!T temporal;
if (auto exception = impl(elemParams, temporal))
Expand All @@ -774,7 +775,6 @@ template deserializeValue(string[] symbolTable)
{
if (error)
return error.ionException;
import mir.conv: to;
auto elemParams = params.withData(elem);
serdeGetProxy!T temporal;
if (auto exception = impl(elemParams, temporal))
Expand All @@ -788,7 +788,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = impl(params, temporal))
return exception;

value = to!T(move(temporal));
value = toProxy!T(move(temporal));
}
static if(__traits(hasMember, T, "serdeFinalize"))
{
Expand Down Expand Up @@ -876,7 +876,26 @@ template deserializeValue(string[] symbolTable)
}

alias Types = T.AllowedTypes;
alias contains = Contains!Types;
alias contains = ContainsProxied!Types;

pragma(inline, true) void setValue(T)(T new_value)
Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shouldn't be a common logic. What is your use case?

Copy link
Copy Markdown
Contributor Author

@WebFreak001 WebFreak001 Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is only within the isVariant branch, it's the logic to set a variant to a value that might be using a proxy to a proxy-supported value.

This function is the core functionality of this PR and makes the added example pass.

Copy link
Copy Markdown
Member

@9il 9il Jun 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a too advanced feature for the Dlang's small community. It may make other features like better @nogc support via @serdeScoped harder to implement. Please describe what is your use case. I wonder if we can find another way to support it.

Copy link
Copy Markdown
Contributor Author

@WebFreak001 WebFreak001 Jun 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the problem is that this is a local function with scope? I can also make it static and use a ref parameter to take in the value to change.

The function body should have no problem with @nogc if using @serdeProxyCast. The branch that may potentially allocate is also only compiled in if there is a variant that has types which are proxies to primitives / otherwise unsupported value types of the variant.

Copy link
Copy Markdown
Member

@9il 9il Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I mean that there is unlikely should be any function of any kind. The idea itself is good. However, it may block some other code evolution paths. So, could we replace it with minimal hardcoded if else conditions for a minimal amount of types? What types do you need to support?

{
alias realContains = Contains!Types;
static if (realContains!T)
value = new_value;
else
{
static foreach (Type; Types)
{
static if (is(serdeGetFinalProxy!Type == T))
{
Type ret = new_value.toProxy!Type;
value = ret;
return; // kind of unsafe: first one wins, but compiler will warn with multiple returns anyway
}
}
}
}

static if (getAlgebraicAnnotationsOfVariant!T.length)
{
Expand All @@ -901,7 +920,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(annotatedParams, object))
return exception;
import core.lifetime: move;
value = move(object);
setValue(move(object));
return null;
}
}
Expand Down Expand Up @@ -936,7 +955,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(annotatedParams, object))
return exception;
import core.lifetime: move;
value = move(object);
setValue(move(object));
return null;
}
}
Expand All @@ -951,7 +970,7 @@ template deserializeValue(string[] symbolTable)
// TODO: check that descriptor.type correspond underlaying type
if (data.descriptor.L == 0xF)
{
value = IonNull(data.descriptor.type);
setValue(IonNull(data.descriptor.type));
return retNull;
}
}
Expand All @@ -961,7 +980,7 @@ template deserializeValue(string[] symbolTable)
// TODO: check that descriptor.type correspond underlaying type
if (data.descriptor.L == 0xF)
{
value = null;
setValue(null);
return retNull;
}
}
Expand All @@ -970,7 +989,7 @@ template deserializeValue(string[] symbolTable)
T.AllowedTypes[1] payload;
if (auto exception = deserializeValue(params, payload))
return exception;
value = payload;
setValue(payload);
return retNull;
}
else
Expand All @@ -980,7 +999,7 @@ template deserializeValue(string[] symbolTable)
// {
// case IonTypeCode.null_:
// {
// value = null;
// setValue(null);
// return retNull;
// }
// }
Expand All @@ -992,7 +1011,7 @@ template deserializeValue(string[] symbolTable)
bool boolean;
if (auto errorCode = data.get!bool(boolean))
return errorCode.ionException;
value = boolean;
setValue(boolean);
return retNull;
}
}
Expand All @@ -1005,7 +1024,7 @@ template deserializeValue(string[] symbolTable)
string str;
if (auto exception = deserializeValue(params, str))
return exception;
value = str;
setValue(str);
return retNull;
}
}
Expand All @@ -1018,27 +1037,37 @@ template deserializeValue(string[] symbolTable)
Filter!(isSmallString, Types)[$ - 1] str; // pick the largest one
if (auto exception = deserializeValue(params, str))
return exception;
value = str;
setValue(str);
return retNull;
}
}

static if (contains!long)
enum containsIntegral = contains!short
|| contains!ushort
|| contains!int
|| contains!uint
|| contains!long
|| contains!ulong;
Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be a separate MR with tests.

enum containsFloating = contains!float
|| contains!double
|| contains!real;

static if (containsIntegral)
{
case IonTypeCode.nInt:
case IonTypeCode.uInt:
{
long number;
if (auto exception = deserializeValue_(data, number))
return exception;
value = number;
setValue(number);
return retNull;
}
}

static if (contains!double)
static if (containsFloating)
{
static if (!contains!long)
static if (!containsIntegral)
{
case IonTypeCode.nInt:
case IonTypeCode.uInt:
Expand All @@ -1049,7 +1078,7 @@ template deserializeValue(string[] symbolTable)
double number;
if (auto exception = deserializeValue_(data, number))
return exception;
value = number;
setValue(number);
return retNull;
}
}
Expand All @@ -1061,7 +1090,7 @@ template deserializeValue(string[] symbolTable)
Timestamp timestamp;
if (auto error = data.trustedGet!IonTimestamp.get(timestamp))
return error.ionException;
value = timestamp;
setValue(timestamp);
return retNull;
}
}
Expand All @@ -1071,7 +1100,7 @@ template deserializeValue(string[] symbolTable)
case IonTypeCode.blob:
{
auto blob = data.trustedGet!Blob;
value = Blob(blob.data.dup);
setValue(Blob(blob.data.dup));
return retNull;
}
}
Expand All @@ -1081,7 +1110,7 @@ template deserializeValue(string[] symbolTable)
case IonTypeCode.clob:
{
auto clob = data.trustedGet!Clob;
value = Clob(clob.data.dup);
setValue(Clob(clob.data.dup));
return retNull;
}
}
Expand All @@ -1096,7 +1125,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(params, array))
return exception;
import core.lifetime: move;
value = move(array);
setValue(move(array));
return retNull;
}
}
Expand Down Expand Up @@ -1164,7 +1193,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(params, object))
return exception;
import core.lifetime: move;
value = move(object);
setValue(move(object));
return retNull;
}
}
Expand All @@ -1186,7 +1215,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(params, object))
return exception;
import core.lifetime: move;
value = move(object);
setValue(move(object));
return retNull;
}
}
Expand All @@ -1202,7 +1231,7 @@ template deserializeValue(string[] symbolTable)
if (auto exception = deserializeValue(params, object))
return exception;
import core.lifetime: move;
value = move(object);
setValue(move(object));
return retNull;
}
}
Expand Down
32 changes: 32 additions & 0 deletions source/mir/ion/conv.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,3 +562,35 @@ version(mir_ion_test) unittest
])
text.text2ion.ion2msgpack.msgpack2ion.ion2text.should == text;
}

To toProxy(To, From)(auto ref scope return From v)
Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, place the logic where it is needed.

{
import std.traits : hasUDA, isImplicitlyConvertible;
import mir.conv : to;
import mir.deser : serdeProxyCast;
static if (__traits(compiles, hasUDA!(To, serdeProxyCast)) && hasUDA!(To, serdeProxyCast))
return cast(To)v;
else static if (isImplicitlyConvertible!(From, To))
{
To ret = v;
return ret;
}
else
return to!To(v);
}

To proxyTo(To, From)(auto ref scope return From v)
Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not introduce a separate function for that logic.

Copy link
Copy Markdown
Member

@9il 9il Jun 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, place the logic where it is needed.

Copy link
Copy Markdown
Contributor Author

@WebFreak001 WebFreak001 Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be ok to put them in the ser/deser packages? the same code is reused multiple times, that's why I made functions for the a little more complicated/configurable assignment logic.

Having that in a function means there is just a single place to change to control how proxies are assigned.

{
import std.traits : hasUDA, isImplicitlyConvertible;
import mir.conv : to;
import mir.deser : serdeProxyCast;
static if (__traits(compiles, hasUDA!(From, serdeProxyCast)) && hasUDA!(From, serdeProxyCast))
return cast(To)v;
else static if (isImplicitlyConvertible!(From, To))
{
To ret = v;
return ret;
}
else
return to!To(v);
}
23 changes: 23 additions & 0 deletions source/mir/ion/examples.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,3 +1404,26 @@ unittest
assert((cast(Foo)6).serializeText == "6", Foo.b.serializeText);
assert("6".deserializeText!Foo == 6);
}

///
version(mir_ion_test)
unittest
{
import mir.deser.json;
import mir.algebraic_alias.json;
import mir.algebraic: Variant, Nullable;

@serdeProxy!int
struct Aliased
{
int x;
alias x this;
}

struct S
{
@serdeOptional Variant!(void, Aliased) data;
}

S s = `{"data":4}`.deserializeJson!S;
}
21 changes: 6 additions & 15 deletions source/mir/ser/package.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import mir.ion.type_code;
import mir.reflection;
import std.meta;
import std.traits;
import mir.ion.conv : proxyTo;

public import mir.serde;

Expand Down Expand Up @@ -644,24 +645,14 @@ private template serializeWithProxy(Proxy)
static if (is(Proxy == const(char)[]) || is(Proxy == string) || is(Proxy == char[]))
{
import mir.format: stringBuf, print, getData;
static if (__traits(compiles, serializeValue(serializer, stringBuf() << value << getData)))
serializeValue(serializer, stringBuf() << value << getData);
static if (__traits(compiles, serializeValue(serializer, stringBuf() << proxyTo!Proxy(value) << getData)))
serializeValue(serializer, stringBuf() << proxyTo!Proxy(value) << getData);
else
{
serializeValue(serializer, to!Proxy(value));
}
serializeValue(serializer, proxyTo!Proxy(value));
}
else
{
static if (isImplicitlyConvertible!(V, Proxy))
{
Proxy proxy = value;
serializeValue(serializer, proxy);
}
else
{
serializeValue(serializer, to!Proxy(value));
}
serializeValue(serializer, proxyTo!Proxy(value));
}
}
}
Expand Down Expand Up @@ -776,7 +767,7 @@ void serializeValue(S, V)(scope ref S serializer, auto ref V value)
static if (__traits(compiles, value.serialize(serializer)) || !hasUDA!(V, serdeProxy))
value.serialize(serializer);
else
serializeValue(serializer, to!(serdeGetProxy!V)(value));
serializeValue(serializer, proxyTo!(serdeGetProxy!V)(value));

}
else
Expand Down