Serializing and deserializing List<T> returns base type if T isn't abstract/interface

[Measurity]

Code for testing:

abstract record Entity(string Name);
record WorldEntity(string Name) : Entity(Name);
record PlaceholderGroupWorldEntity(string Name) : WorldEntity(Name);
record BrokenCellEntities(List<WorldEntity> Entities) : Packet;
record WorkingCellEntities(List<Entity> Entities) : Packet;
abstract record Packet;
record Wrapper(Packet Instance);

internal class Program
{
    private static readonly string DesktopFile = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Desktop), "test.json");
    private static Wrapper Read() => BinaryConverter.Deserialize<Wrapper>(File.ReadAllBytes(DesktopFile));
    private static void Write<T>(T instance) where T : Packet => File.WriteAllBytes(DesktopFile, BinaryConverter.Serialize(new Wrapper(instance)));

    public static void Main(string[] args)
    {
        BinaryConverter.RegisterUnion<Packet>(typeof(BrokenCellEntities), typeof(WorkingCellEntities));
        BinaryConverter.RegisterUnion<Entity>(typeof(PlaceholderGroupWorldEntity), typeof(WorldEntity));
        PlaceholderGroupWorldEntity e = new("Placeholder Entity");
        
        // Broken
        Write(new BrokenCellEntities(new List<WorldEntity>() { e }));
        Console.WriteLine((Read().Instance as BrokenCellEntities).Entities.First());
        
        // Workaround
        Write(new WorkingCellEntities(new List<Entity>() { e }));
        Console.WriteLine((Read().Instance as WorkingCellEntities).Entities.First());
    }
}

Which outputs:

WorldEntity { Name = Placeholder Entity } <-- broken
PlaceholderGroupWorldEntity { Name = Placeholder Entity } <-- workaround

See https://github.com/SubnauticaNitrox/BinaryPack/blob/master/src/BinaryPack/Serialization/Processors/Collections/ListProcessor%7BT%7D.cs#L60

[SpaceMonkeyy86]

This is correct behavior. BinaryPack expects WorldEntity items when you deserialize a list of WorldEntities because it is not abstract and there is no union registered for the type. However, PlaceholderGroupWorldEntity is in the union for Entity, so BinaryPack can match against it when expecting any Entity.

Keep in mind, it will try to match the first type on the list, so if you flip the order of WorldEntity and PlaceholderGroupWorldEntity in the second union, the workaround will fail as well.

BinaryPack currently does not support unions on concrete types, so types inheriting from concrete types are difficult to work with. You could fix this by making WorldEntity abstract (AbstractWorldEntity, WorldEntityBase, etc.) and adding a second, concrete WorldEntity type that extends from it. Then register the concrete type and PlaceholderGroupWorldEntity in a union on the abstract type.

[Measurity]

What you have outlined is what was discovered when working on the Entity spawning system PR (currently open on Nitrox). I raised this issue because it's not clear that List loses information when you serialize and deserialize it with BinaryPack. Compared to NewtonsoftJson where this is possible by changing its settings.

I see this as having two possible solutions. Either we change BinaryPack to support this case or warn for it. Or we write an analyzer in Nitrox to warn. Do you agree with this assessment and if so, which would you choose?

[SpaceMonkeyy86]

I think writing an analyzer is a good idea. It should warn if a packet inherits from a concrete packet type.