System.Text.Json deserialized readonly struct wrong

[manfred-brands]

Description

JsonSerializer doesn't deserialize readonly structs with a non-default constructor properly. Nor does it give any error.

Reproduction Steps

using System;
using System.Text.Json;

namespace JsonReadOnlyStruct;

public static class Program
{
    public static void Main()
    {
        Angle angle = new(Math.PI);
        Console.WriteLine($"Angle: {angle}");

        string json = JsonSerializer.Serialize(angle);
        Console.WriteLine($"JSON: {json}");

        Angle roundtrip = JsonSerializer.Deserialize<Angle>(json);

        Console.WriteLine($"Angle: {roundtrip}");
    }
}

public readonly struct Angle
{
    public Angle(double radians) => Radians = radians;

    public double Radians { get; }

    public override string ToString() => Radians.ToString();
}

Expected behavior

Angle: 3.141592653589793
JSON: {"Radians":3.141592653589793}
Angle: 3.141592653589793

Actual behavior

Angle: 3.141592653589793
JSON: {"Radians":3.141592653589793}
Angle: 0

No exception generated!

Regression?

No response

Known Workarounds

IFF one has access to the actual class, you can put a JsonConstructor attribute on the one and only constructor.
or convert the class to a record. Which will be created with an init property which is settable.
The latter allows weird code like: angle = angle with { Radians = -Math.PI };

If one has no access to the actual class, or that class cannot have a dependency on System.Text.Json all that is left is to make your own converter.

Configuration

dotnet 7.0.200 with System.Text.Json 7.0.2

Other information

In the class ReflectionExtensions.TryGetDeserializationConstructor this is explicitly coded:

            // Structs will use default constructor if attribute isn't used.
            if (useDefaultCtorInAnnotatedStructs && type.IsValueType && ctorWithAttribute == null)
            {
                deserializationCtor = null;
                return true;
            }

            deserializationCtor = ctorWithAttribute ?? publicParameterlessCtor ?? lonePublicCtor;

Not sure about the logic behind this. If there is no parameterized constructor fine, but if there is one it should be used.
This code was added to support for deserializing with parameterized ctors (#56354) 2 years ago.

The flags _useDefaultConstructorInUnannotatedStructs is only set for F# records.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

JsonSerializer doesn't deserialize readonly structs with a non-default constructor properly. Nor does it give any error.

Reproduction Steps

using System;
using System.Text.Json;

namespace JsonReadOnlyStruct;

public static class Program
{
    public static void Main()
    {
        Angle angle = new(Math.PI);
        Console.WriteLine($"Angle: {angle}");

        string json = JsonSerializer.Serialize(angle);
        Console.WriteLine($"JSON: {json}");

        Angle roundtrip = JsonSerializer.Deserialize<Angle>(json);

        Console.WriteLine($"Angle: {roundtrip}");
    }
}

public readonly struct Angle
{
    public Angle(double radians) => Radians = radians;

    public double Radians { get; }

    public override string ToString() => Radians.ToString();
}

Expected behavior

Angle: 3.141592653589793
JSON: {"Radians":3.141592653589793}
Angle: 3.141592653589793

Actual behavior

Angle: 3.141592653589793
JSON: {"Radians":3.141592653589793}
Angle: 0

No exception generated!

Regression?

No response

Known Workarounds

IFF one has access to the actual class, you can put a JsonConstructor attribute on the one and only constructor.
or convert the class to a record. Which will be created with an init property which is settable.
The latter allows weird code like: angle = angle with { Radians = -Math.PI };

If one has no access to the actual class, or that class cannot have a dependency on System.Text.Json all that is left is to make your own converter.

Configuration

dotnet 7.0.200 with System.Text.Json 7.0.2

Other information

In the class ReflectionExtensions.TryGetDeserializationConstructor this is explicitly coded:

            // Structs will use default constructor if attribute isn't used.
            if (useDefaultCtorInAnnotatedStructs && type.IsValueType && ctorWithAttribute == null)
            {
                deserializationCtor = null;
                return true;
            }

            deserializationCtor = ctorWithAttribute ?? publicParameterlessCtor ?? lonePublicCtor;

Not sure about the logic behind this. If there is no parameterized constructor fine, but if there is one it should be used.
This code was added to support for deserializing with parameterized ctors (#56354) 2 years ago.

The flags _useDefaultConstructorInUnannotatedStructs is only set for F# records.

Author:	manfred-brands
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[b-a-roberts]

Obviously this also affects the use of a code generated serializer context such as
[JsonSerializable(typeof(Angle))] internal partial class AngleJsonSerializerContext : JsonSerializerContext { }

[krwq]

To fix your problem use:

    [JsonConstructor]
    public Angle(double radians) => Radians = radians;

The reason this is not working is that structs always have parameterless ctor regardless if you define it or not. Parameterless ctor always win fight for ctors for deserializer. Since Radians property is readonly it is not being deserialized.

@eiriktsarpalis I was trying to debug this using UnmappedMemberHandling but it didn't find any problems while it actually did ignore deserialization of member. Is that a bug?

[manfred-brands]

@krwq The class is defined in an external library and also used by other teams that still use Newtonsoft. Therefore the attribute option is not possible.
Constructor determination could be smarter, looking at the properties in JSON first to find one that fits best instead of requiring an attribute.

[eiriktsarpalis]

Duplicate of #59309. There's no workaround unfortunately other than adding an explicit annotation on the type. It should be possible to work around this in the future once #71944 is implemented.

I was trying to debug this using UnmappedMemberHandling but it didn't find any problems while it actually did ignore deserialization of member. Is that a bug?

Not sure I follow. Could you create a separate issue with a repro please?

[manfred-brands]

I didn't find that issue because it is closed, but not resolved. As @jnm2 said there, there should at least be an exception that there is more than one constructor.
The default constructor is explicitly skipped in the called method to get the constructors. If that was not done, there would have been an exception about multiple constructors.

[eiriktsarpalis]

I didn't find that issue because it is closed, but not resolved. As @jnm2 said there, there should at least be an exception that there is more than one constructor.

How would throwing an exception address your issue? It would also be a breaking change for users that do depend on the default constructor being picked up.

[manfred-brands]

That exception would at least indicate the reason as opposed to it simply filling zeroes.

For situations where there is only the default constructor there is no issue with my indicated change. It would still be found.

[manfred-brands]

The reason for closing #59309 was:

STJ parameterized ctor support is consistent within classes and structs respectively

It is however not as for classes with more than one constructor we get an exception. For structs we get bad behaviour and no error.

[jnm2]

Failing sooner and more noticeably is always preferred, knowing that this is a bug either way.

[eiriktsarpalis]

As mentioned, this would be a breaking change so it's not a change we could entertain.

It is not the case that the current behaviour is "bad behaviour": mutable structs can depend on it and rely on property setters to be populated. What's more, starting with C# 10 it is actually possible to create user-defined default constructors in structs, so it is no longer the case that we can treat those as trivial.

[b-a-roberts]

Silently failing at run-time is pretty bad, even if you don't consider it a bug. Fortunately we have extensive unit tests which picked this up, otherwise it could have slipped though. Not ideal.