Cysharp / MemoryPack

Zero encoding extreme performance binary serializer for C# and Unity.
MIT License
3.01k stars 182 forks source link

Deserialization logic assumes `null` default for some value types in v1.20.X #261

Closed alexyakunin closed 4 months ago

alexyakunin commented 4 months ago

We have the following type:

[StructLayout(LayoutKind.Auto)]
[DataContract, MemoryPackable(GenerateType.VersionTolerant)]
[Newtonsoft.Json.JsonObject(Newtonsoft.Json.MemberSerialization.OptOut)]
[method: Newtonsoft.Json.JsonConstructor]
public readonly partial struct ImmutableOptionSet(
    ImmutableDictionary<Symbol, object>? items
    ) : IServiceProvider, IEquatable<ImmutableOptionSet>
{
    [JsonConstructor, MemoryPackConstructor]
    public ImmutableOptionSet(Dictionary<string, NewtonsoftJsonSerialized<object>>? jsonCompatibleItems)
        : this(jsonCompatibleItems?.ToImmutableDictionary(
            p => (Symbol) p.Key,
            p => p.Value.Value))
    { }

    // ...
}

And when it's used in this type:

[DataContract, MemoryPackable(GenerateType.VersionTolerant)]
// ReSharper disable once InconsistentNaming
public partial record AuthBackend_SetupSession(
    [property: DataMember, MemoryPackOrder(0)] Session Session,
    [property: DataMember, MemoryPackOrder(1)] string IPAddress = "",
    [property: DataMember, MemoryPackOrder(2)] string UserAgent = "",
    [property: DataMember, MemoryPackOrder(3)] ImmutableOptionSet Options = default
) : ISessionCommand<SessionInfo>, IBackendCommand, INotLogged;

the following code is generated for deserialization, which triggers compile-time error:

    [global::MemoryPack.Internal.Preserve]
    static void IMemoryPackable<AuthBackend_SetupSession>.Deserialize(ref MemoryPackReader reader, scoped ref AuthBackend_SetupSession? value)
    {

        if (!reader.TryReadObjectHeader(out var count))
        {
            value = default!;
            goto END;
        }

        Span<int> deltas = stackalloc int[count];
        for (int i = 0; i < count; i++)
        {
            deltas[i] = reader.ReadVarIntInt32();
        }

        global::ActualLab.Fusion.Session __Session;
        string __IPAddress;
        string __UserAgent;
        global::ActualLab.Collections.ImmutableOptionSet __Options;

        var readCount = 4;
        if (count == 4)
        {
            if (value == null)
            {
                if (deltas[0] == 0) { __Session = default; } else __Session = reader.ReadPackable<global::ActualLab.Fusion.Session>();
                if (deltas[1] == 0) { __IPAddress = default; } else __IPAddress = reader.ReadString();
                if (deltas[2] == 0) { __UserAgent = default; } else __UserAgent = reader.ReadString();
                if (deltas[3] == 0) { __Options = default; } else __Options = reader.ReadPackable<global::ActualLab.Collections.ImmutableOptionSet>();

                goto NEW;
            }
            else
            {
                __Session = value.@Session;
                __IPAddress = value.@IPAddress;
                __UserAgent = value.@UserAgent;
                __Options = value.@Options;

                if (deltas[0] != 0) reader.ReadPackable(ref __Session);
                if (deltas[1] != 0) __IPAddress = reader.ReadString();
                if (deltas[2] != 0) __UserAgent = reader.ReadString();
                if (deltas[3] != 0) reader.ReadPackable(ref __Options);

                goto SET;
            }

        }
        // else if (count > 4)
        // {
            // MemoryPackSerializationException.ThrowInvalidPropertyCount(typeof(AuthBackend_SetupSession), 4, count);
            // goto READ_END;
        // }
        else
        {
            if (value == null)
            {
               __Session = default!;
               __IPAddress = "";
               __UserAgent = "";
               __Options = null; // Here
// ...
alexyakunin commented 4 months ago

Which renders v1.20 unusable :)

alexyakunin commented 4 months ago

For the note, changing type definition to this fixes the issue:

[DataContract, MemoryPackable(GenerateType.VersionTolerant)]
[method: JsonConstructor, Newtonsoft.Json.JsonConstructor, MemoryPackConstructor]
// ReSharper disable once InconsistentNaming
public partial record AuthBackend_SetupSession(
    [property: DataMember, MemoryPackOrder(0)] Session Session,
    [property: DataMember, MemoryPackOrder(1)] string IPAddress,
    [property: DataMember, MemoryPackOrder(2)] string UserAgent,
    [property: DataMember, MemoryPackOrder(3)] ImmutableOptionSet Options
) : ISessionCommand<SessionInfo>, IBackendCommand, INotLogged
{
    public AuthBackend_SetupSession(Session session, string ipAddress = "", string userAgent = "")
        : this(session, ipAddress, userAgent, default) { }
}

I.e. it seems it doesn't deal well with default values for [MemoryPackConstructor] arguments.

hadashiA commented 4 months ago

Hello, Thanks for the detailed report. We have released the fixed version. I hope you will check it out. Thanks. https://github.com/Cysharp/MemoryPack/releases/tag/1.20.4