System.Text.Json source generator fails to deserialize init-only & required properties

[dallmair]

Description

When using the System.Text.Json source generator, init-only properties are not deserialized.

Repro code:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace JsonSerialization
{
	public static class Program
	{
		public static void Main()
		{
			const string json = @"{ ""InitOnly"": "".NET"", ""Settable"": ""Rocks"" }";

			var tagLine1 = JsonSerializer.Deserialize<TagLine>(json);
			Console.WriteLine(tagLine1.InitOnly ?? "tagLine1.InitOnly is null");
			Console.WriteLine(tagLine1.Settable ?? "tagLine1.Settable is null");

			Console.WriteLine();

			var tagLine2 = JsonSerializer.Deserialize<TagLine>(json, TagLineJsonContext.Default.TagLine);
			Console.WriteLine(tagLine2.InitOnly ?? "tagLine2.InitOnly is null");
			Console.WriteLine(tagLine2.Settable ?? "tagLine2.Settable is null");
		}
	}

	public sealed class TagLine
	{
		public string InitOnly { get; init; }   // This is always null when deserialized with TagLineJsonContext
		public string Settable { get; set; }
	}

	[JsonSerializable(typeof(TagLine), GenerationMode = JsonSourceGenerationMode.Metadata)]
	public partial class TagLineJsonContext : JsonSerializerContext
	{
	}
}

Expected output:

.NET
Rocks

.NET
Rocks

Actual output:

.NET
Rocks

tagLine2.InitOnly is null
Rocks

Configuration

• .NET 5.0.9 with System.Text.Json version 6.0.0-preview.7.21377.19
• Windows 10.0.19043.1052 x64

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When using the System.Text.Json source generator, init-only properties are not deserialized.

Repro code:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace JsonSerialization
{
	public static class Program
	{
		public static void Main()
		{
			const string json = @"{ ""InitOnly"": "".NET"", ""Settable"": ""Rocks"" }";

			var tagLine1 = JsonSerializer.Deserialize<TagLine>(json);
			Console.WriteLine(tagLine1.InitOnly ?? "tagLine1.InitOnly is null");
			Console.WriteLine(tagLine1.Settable ?? "tagLine1.Settable is null");

			Console.WriteLine();

			var tagLine2 = JsonSerializer.Deserialize<TagLine>(json, TagLineJsonContext.Default.TagLine);
			Console.WriteLine(tagLine2.InitOnly ?? "tagLine2.InitOnly is null");
			Console.WriteLine(tagLine2.Settable ?? "tagLine2.Settable is null");
		}
	}

	public sealed class TagLine
	{
		public string InitOnly { get; init; }   // This is always null when deserialized with TagLineJsonContext
		public string Settable { get; set; }
	}

	[JsonSerializable(typeof(TagLine), GenerationMode = JsonSourceGenerationMode.Metadata)]
	public partial class TagLineJsonContext : JsonSerializerContext
	{
	}
}

Expected output:

.NET
Rocks

.NET
Rocks

Actual output:

.NET
Rocks

tagLine2.InitOnly is null
Rocks

Configuration

• .NET 5.0.9 with System.Text.Json version 6.0.0-preview.7.21377.19
• Windows 10.0.19043.1052 x64

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

[eiriktsarpalis]

Can reproduce. This is by-design, and a limitation of how metadata-based deserialization works. In source generated code, JsonPropertyInfo metadata instances are constructed as follows:

            properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
                getter: static (obj) => ((global::JsonSerialization.TagLine)obj).Settable,
                setter: static (obj, value) => ((global::JsonSerialization.TagLine)obj).Settable = value!,
...);

Since the setter lambda is invalid in the case of init-only properties, the generator will generate a null setter instead.

@layomia given that fast-path deserialization has not been implemented yet, do you think it might make sense to include a reflection-based workaround? For example:

            MethodInfo setterMethod = typeof(TagLine).GetProperty("Settable", bindingFlags).GetSetMethod(true);
            properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
                getter: static (obj) => ((global::JsonSerialization.TagLine)obj).Settable,
                setter: static (obj, value) => setterMethod.Invoke(obj, new [] { value }),
...);

[layomia]

Yes this is known, for the reason Eirik mentioned - we cannot reference an init-only setter in generated code outside of object initialization/construction.

do you think it might make sense to include a reflection-based workaround? For example:

So far we have avoided the use of runtime reflection in the source-gen programming model, as a first-class goal of the feature. Having a reflection workaround seems like something users should explicitly opt-into via a feature/new API. I don't think the use of init-only setters should implicitly opt users into runtime reflection.

given that fast-path deserialization has not been implemented yet

I believe the same issues would apply in the generated code for fast-path deserialization, unless I'm missing something.

[eiriktsarpalis]

do you think it might make sense to include a reflection-based workaround? For example:

So far we have avoided the use of runtime reflection in the source-gen programming model, as a first-class goal of the feature. Having a reflection workaround seems like something users should explicitly opt-into via a feature/new API. I don't think the use of init-only setters should implicitly opt users into runtime reflection.

This makes sense in principle, however the end result is we're blocking users with DTOs using init-only properties. The reflection suggested here should be linker-safe and have no issue other than being slightly slower compared to equivalent properties with setters. I don't think think this should be opt-in behavior, getting the correct serialization behavior should not require additional configuration.

given that fast-path deserialization has not been implemented yet

I believe the same issues would apply in the generated code for fast-path deserialization, unless I'm missing something.

A fast-path deserializer should be capable of initializing properties at construction path.

cc @steveharter @ericstj @eerhardt for more thoughts.

[dallmair]

As a long-time .NET dev I'd like to provide another perspective, or rather, my expectations.

When I tripped over this yesterday, we had started a new project at work and written a bunch of types we want to deserialize. As I remembered the relatively recent blog post regarding the System.Text.Json source generator, I wanted to give it a try and started with the metadata approach. Init-only properties are a perfect match for our data structure, so I quickly checked the docs and saw they are supported, thus I used them. Then, when deserialization didn't yield the expected result, I thought that I had done something wrong and it took me quite a while to pinpoint the problem and build the repro.

Thanks to your analysis and explanations, I do now see the underlying technical reasons for the current implementation.

I think there is another option to be discussed, though. Basically, I'm thinking of the System.Text.Json source generator as a performance optimization. Therefore, I do expect it to exhibit the exact same semantics as the standard (non-generated) deserializer. Of course, there are a lot of cases where optimizations achieve better performance exactly because they only support a subset of features, and that's fine. However, in these cases the optimized code only has two options from my pov:

1. Fall back to a less optimized solution like @eiriktsarpalis suggested. It might not yield the same speedups, but at least it works. At the minimum, it must achieve the same performance that the non-generated approach has.
2. Fail with an exception, telling the developer that and in the best case why the optimization is unavailable in the respective situation. Such as:

            properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
                getter: static (obj) => ((global::JsonSerialization.TagLine)obj).Settable,
                setter: static (obj, value) => throw new NotSupportedException("You can only have init-only properties OR source generators, but not both. Choose wisely."),
...);

The currently implemented option (different behavior) would be ok if it would be a totally revamped API (e.g. version 2 with lots of other breaking changes), but in the case of a performance optimization it's a violation of the principle of least surprise and I'd say subpar.

[ithline]

I encountered this issue as well.
Could this be solved similarly to collections, where all elemrnts are gathered to a list and then converted to the spicifc collection type? In this case properties would be stored inside a dictionary-like structure and assigned inside a factory delegate.

[layomia]

I agree that the current behavior of failing silently is not good, however the reflection-based approach feels like too much of a hack. For 6.0 I propose we throw NotSupportedException with a clear message in the setter code, and see if we can land on a better solution in the future. Folks can use the source generator for improved serialization performance, and use the pre-existing deserialization methods that work using reflection.

[eerhardt]

@jkotas @jcouv - do you have any thoughts here? Is relying on reflection to set an init property a good long-term decision?

Reading https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/init, it says:

The init accessor makes immutable objects more flexible by allowing the caller to mutate the members during the act of construction. That means the object's immutable properties can participate in object initializers

Should we consider "deserialization" a "construction phase" of the object? Is the runtime guaranteed to allow reflection to set init properties long-term?

[jkotas]

Is relying on reflection to set an init property a good long-term decision?

It defeats the point of the source generators.

Is the runtime guaranteed to allow reflection to set init properties long-term?

The init properties are Roslyn created and enforced contract. I do not think we would ever add any enforcement around it to the runtime. It would be a hard to justify breaking change.

I think the right long-term solution is to encourage the full source generation mode. The metadata-only source generation mode is neither here or there, and this issue shows its limitations. I expect that we will hit more issues like this one over time.

[eiriktsarpalis]

We're addressing the issue in 6.0 by emitting warnings and runtime exceptions. We will be working on a proper fix for 7.0.0.

[eiriktsarpalis]

@eerhardt has suggested that this can be solved without reflection by extending the JsonConstructor mechanism so that init-only properties are incorporated as constructor parameters.

[eiriktsarpalis]

Related to #66003

[Kumima]

@t-schreibs presumably though disabling trimming would fix the issue for you? Alternatively, you might consider using positional records with source gen, since deserialization will bind to constructor parameters instead of the unsupported init-only properties.

This one can only solve the problem of init-only, but not for required property. Will required property also be available for this issue?

[JohnGalt1717]

It looks like the PR for this was merged. Is this in .NET 7 or delayed to .NET 8?

Also looks like the required PR was merged. Is that also in .NET 7 or delayed to .NET 8?

[0xced]

Yes, it's part of .NET 7.

Here's how to figure it out.

1. In the associated pull request (Improving STJ source generator support for record types  #68064) follow the merge commit (e2a4bb0):
   

2. At the bottom of the commit message, you'll see all tags that the commit belongs to:
   

[JohnGalt1717]

Well I saw that, but then I tried to use it on a public readonly record struct with init only properties which should work if it's in there using the RTM of 7.0 and it gave me the same warning.

[eiriktsarpalis]

#68064 only concerns improvements for positional record parameters. The problem hasn't been addressed fully yet, which is why this issue is still open.

[kelson-ball-agilysys]

I appear to be getting this warning as a false positive now. Is that expected? Can I safely disregard the warning?

I thought at first that might be because I was targeting net6.0 from the 7.0.100 SDK, but I also get the warning+working deserialization when targeting 6.0 from SDK 6.0.201.

Incidentally the warning goes away (and deserialization continues to work) on 6.0 when using a record struct rather than a record class.

Code to reproduce apparent false-positive warning, targeting net6.0

var data = System.Text.Json.JsonSerializer.Deserialize("{\"Value\": 3}", Json.Default.Data);
Console.WriteLine(data!.Value); // gets 3, despite SYSLIB1037 warning

public record class Data(int Value);

[System.Text.Json.Serialization.JsonSerializable(typeof(Data))]
public partial class Json : System.Text.Json.Serialization.JsonSerializerContext { }

[eiriktsarpalis]

This warning should only occur using a 6.0 version of the SDK. If you force a 7.0 version of the sdk (e.g. by setting the sdk version in your global.json file) the warning should go away, even if your project is targeting net6.0.

[wzchua]

Would this issue also cover the source generator generating uncompilable code for a class with a required property and a parameterless constructor?