Bug: Serialization of readonly nullable custom struct fails

[dbolin]

Trying to serialize a class containing a readonly field of a custom struct type throws an exception: 'The binary operator Equal is not defined for the types 'System.Nullable`1[A.Test]' and 'System.Nullable`1[A.Test]'.'

Config:

            var config = new SerializerConfig { DefaultTargets = TargetMember.AllFields, PreserveReferences = false };
            config.Advanced.ReadonlyFieldHandling = ReadonlyFieldHandling.ForcedOverwrite;
            config.Advanced.SkipCompilerGeneratedFields = false;
            config.OnConfigNewType = tc => tc.TypeConstruction = TypeConstruction.ByUninitialized();

Types:

    public class Wrapper
    {
        public readonly Test? NullableStruct;

        public Wrapper(Test? nullableStruct)
        {
            NullableStruct = nullableStruct;
        }
    }

    public struct Test
    {
        public decimal Value;
    }

[dbolin]

If the struct contains a string field instead, the following error also occurs: System.ArgumentException: 'Expression of type 'A.CustomProperty' cannot be used for parameter of type 'System.Object' of method 'Void SetValue(System.Object, System.Object)' (Parameter 'arg0')'

[rikimaru0345]

There's a comparison there to avoid using SetValue. I've looked into this, but I'm not sure how I can solve this without hurting performance. There are a few solutions I have in mind but most of them would require more time to implement and test than I currently have :/ I'll post updates here when I think of something.

Potential solutions:

• generating a custom Equals method for nullable types
• unsafe code to overwrite struct (probably not portable)
• not doing a compare and always using SetValue ?

[dbolin]

This also happens for a non nullable field as well. Seems like the simplest solution would be to check if the equality operator exists when generating the code, and if not, always overwrite the field.

[rikimaru0345]

Wait, it happens for every readonly field? I'll check it out. If that doesn't uncover a different root issue I'll do just that (check and then just SetValue if there's no way to compare).

[dbolin]

I just meant the nullable part of the field declaration doesn't appear to matter - any user defined struct without an equality operator will cause the problem.

[rikimaru0345]

Hey I looked into all of this some more and I'm working on a solution.

Simply omitting the check (for structs) would work fine, but I'm also investigating if it could be worth it to generate comparison functions for structs.

[rikimaru0345]

any user defined struct without an equality operator will cause the problem.

The last two commits should solve this by implementing a custom equality check. I'll add more unit tests for this soon to finalize this. 😄 👍

[Mrnikbobjeff]

I have looked into the changes you made, perhaps I might have some insight on struct equality as I have worked on Xamarin and WPF improvements to structs and know about the internals of struct equality. Currently, iff all fields/properties of a value type are memcomparable and there is no padding between them the equals operation is simply a memcmp and is fast pathed. If however the struct contains a primitive with ambiguous equality (e.g. float/double as 0.0==-0.0) a memberwise equality check is used. You also might be able to hotpath it if the struct has these properties. Otherwise, iff the struct has padding or contains these types another short path is taken if the struct implements IEquatable, as I see you introduced in your recent commits. This optimization is also present in collections such as dictionary or hashset. Only if both of these checks fail a memberwise equality check is performed which is extremely slow compared to the other paths. You also might be able to compare structs which implement IEquatable via a generic function with a struct, IEquatable constraint as the boxing operation can be ellided for these cases :) thank you for your work on this library by the way, I am a huge fan of it :)

[Mrnikbobjeff]

Also, as a bit of trivia: nullable structs are the only type I know of which succeed in equality and getHashCode calls but fail when calling gettype if the actual value of the nullable is null, which makes it hard to reason about them in generic functions

[rikimaru0345]

Hey, thanks for the message! :D

Yes, I have experimented with IEquatable, but passing by ref vs passing by value is an even bigger deal when it comes to performance than what I imagined.

For now I opted to ignore IEquatable and just compare the fields directly. I know that's not a very good solution because it ignores custom equality functions.

But then again I sort of have to avoid any custom logic as well, because the check is intended to determine whether or not I have to overwrite a readonly value type Ina serialization! That means I will have to keep ignored/excluded fields in mind as well.

In fact, there might even be unexpected scenarios when it comes to recycling. (passing an existing object by ref to Deserialize). Stuff like recycling objects and ceras' ability to forcefully overwrite readonly fields/structs are used regularly for performance reasons, but they also make it very hard to reason about the usual semantics of many things (readonly, object identity for reference objects,...)

To be honest, I'm not quite sure what all of this means for the handling of those things in Ceras and for the implementation I will end up using.

Solving this particular issue #64 on its own would be very easy. But working on it has showed me just how many things there are to consider... how many edge cases there are that could end up becoming a big problem...

The easy way out would be to just say "the language doesn't allow for that, so I'll just remove all related functionality and require everyone to provide serialization constructors :P".

But that wouldn't help anyone and most certainly wouldn't be "comfortable" which, after all, is one of Ceras main goals!

[Jmerk523]

Correct me if I'm wrong, but I think this thread is missing the root cause of the exception: System.ArgumentException: 'Expression of type 'A.CustomProperty' cannot be used for parameter of type 'System.Object' of method 'Void SetValue(System.Object, System.Object)' (Parameter 'arg0')'

The problem is that the method SetValue is being passed a first parameter (the target of the SetValue call) of type ref T where T : struct, but SetValue expects a parameter of type System.Object. In EmitReadonlyWriteBack, the expression tree passes refValueArg directly to the Expression.Call, resulting in a failed cast from ref struct to object. The solution is to box the struct, then call SetValue, and finally unbox the struct and assign it back to the value parameter.

This boxing and unboxing is obviously best avoided (the equality check helps with this), but it's necessary to be able to use SetValue properly on a struct.

[rikimaru0345]

Yea, that would be the obvious solution. I was trying to come up with more creative ways to avoid that overhead. Lots of things actually: different ways to generate code (didn't help), directly calling internal reflection methods to avoid overhead (reduced portability too much), somehow overlapping a dynamically constructed struct with explicit field offsets (way too complicated and didn't help with performance).

I think simply accepting the performance of a SetValue really is the only thing that can be done here...

I'll go with that once I have some time to continue the work on v5.

[rikimaru0345]

Alright, fixed in: https://github.com/rikimaru0345/Ceras/commit/39ecdab2e1eef8b2c01343a3ae998ce8e460d084

I'm currently working on improving how Ceras handles constructors. That should solve this problem in a much better way.

I plan to get this done soon:

• Auto detect more (all!) viable ctors; more than just using the parameterless ctor!
• Smarter inference of any "constructor-parameter-to-member" mappings
• Order constructors by how well their parameters cover the members (prefer a ctor with more parameters over one with fewer)

The unit test here emulates those new features for now: https://github.com/rikimaru0345/Ceras/blob/39ecdab2e1eef8b2c01343a3ae998ce8e460d084/tests/Ceras.Test/GitHub/Issue64_ReadonlyStructs.cs#L145

Please let me know if it works for you :) @Jmerk523 @Mrnikbobjeff @dbolin

[dbolin]

Yep, this works with the current master.

[rikimaru0345]

Thanks for reporting back! 😄

[Mrnikbobjeff]

Just out of curiosity, will this ever not call the constructor for structs? All structs have to have a public parameterless constructor, thus removing the need for Formattierservices for structs doesn't it? It works for me as well, just wanted to be sure about that fact!

[rikimaru0345]

All structs have to have a public parameterless constructor,

It's the other way around. Structs are not allowed to have a public parameterless constructor.

It would imply that you could force some custom default state, which of course doesn't work.

What you are thinking of is default(...) and that just means "all fields are null".

Thus, there is no constructor for Ceras to call in the first place.

[Mrnikbobjeff]

No I really meant the implicit struct constructor, but I assumed that the default inbuilt struct creation is a constructor and present in the GetConstructors call. I just validated that this is not the case. I believe the terminology is explicit parameterless constructor, they are forbidden for structs. While thinking about this I realised that makes no sense to have them present in the return value of the call :). default simply will call this parameterless constructor