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

[dallmair]

trafficstars

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.

[zejji]

Just to echo @dallmair's comment above, we were also investigating using the System.Text.Json source generator in a new minimal API, together with the following configuration to specify that the generated code should be used to serialize/deserialize all requests and responses:

builder.Services.Configure<JsonOptions>(options =>
{
    options.SerializerOptions.AddContext<MyJsonContext>();
});

Since we are making use of record types for all requests and responses, we immediately ran into error "SYSLIB1037 The type '<typename>' defines init-only properties, deserialization of which is currently not supported in source generation mode."

Obviously we've had to shelve using the source generator and go back to standard reflection-based (de)serialization, but I just wanted to reinforce the point that it is reasonable to expect that the source generator would exhibit the exact same semantics as the standard (non-generated) deserializer.

[eiriktsarpalis]

it is reasonable to expect that the source generator would exhibit the exact same semantics as the standard (non-generated) deserializer.

This is our goal as well, though please keep in mind that it is not always possible to achieve 100% convergence. For instance, JsonIncludeAttribute will likely never be supported in sourcegen since it requires private reflection to work.

We are currently working on a document meant to highlight existing semantic differences between sourcegen and the reflection-based serializer: https://github.com/dotnet/docs/issues/26040

[jharjung]

I'm in the same spot as zejji - I have an asp.net web api that makes use of immutable record types, so I'm getting SYSLIB1037 warnings telling me my types can't be deserialized.

My initial reaction to this warning was: huh? shouldn't deserialization be calling the constructor? And if I ignore the warning and give it a go, sure enough, it works just fine. Seems like the analyzer is off here, and should only be emitting that warning for init-only properties that aren't parameters of the deserialization constructor.

As such, I'd have to disable this warning altogether (eg, <NoWarn>SYSLIB1037</NoWarn>), which will also silence legitimate warnings about non-constructor init-only properties. That feels dangerous, so unfortunately I think I will also have to shelve the source generator for the time being.

I'd also like to comment on layomia's suggestion:

Folks can use the source generator for improved serialization performance, and use the pre-existing deserialization methods that work using reflection.

This seems like a non-starter for asp.net use cases. We don't get separate JsonSerializerOptions for serialization and deserialization. We either call options.SerializerOptions.AddContext or we don't, and the context we pass either includes the type or it doesn't.

I'd imagine a generated deserialize handler would have no problem with init-only properties, as it would use a perfectly normal new Foo(...) { Prop = ... } expression that could set those init-only properties during construction. For metadata-only mode, I'd be in favor of a reflection fallback for init-only properties.

• I disagree that it's a hack. If it's such a terrible awful idea unto itself, then why was it deemed acceptable behavior for the reflection-based deserializer? Not implying any expectation here about feature parity between the two serializers, just pointing out that there's precedent for using reflection to set an init-only property.
• I disagree that it defeats the purpose of the source generator. That would be true for deserialization of a type that consisted only of init-only properties that aren't constructor parameters. For a type that uses mostly constructor parameters or mostly mutable properties, and just has one init-only property, you're back to reflection for the entire type. You miss out on all the benefits of the source generator for the constructor (deserialization), the non-init-only property setters (deserialization), and the property getters (serialization) - which you would have gotten if there were a reflection fallback for that one init-only property. That actually defeats the purpose of the source generator, since it just can't be used at all.

I could live with the reflection fallback requiring an explicit opt-in. But I'm not of the opinion that it's necessary. If I'm a novice, I don't really understand what all this means anyway, so that opt-in flag is just one more arcane confusing thing for me to trip over. If I'm an expert, then I think by giving my type an init-only property, I've already opted in - how else do I think a deserializer in metadata mode is supposed to set that property?

[333fred]

@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.

I was totally expecting this to be the mechanism for getting around this, and found myself very surprised when JsonConstructor didn't get rid of this warning.

I do not think we would ever add any enforcement around it to the runtime. It would be a hard to justify breaking change.

The lack of runtime enforcement is actually an intended feature here. We specifically designed init properties to be friendly for reflection-based JSON deserialization here.

[MeepTech]

I'm using source generator because I have no other option to, So I'm stuck with my project at the moment. I need to be able to access the default serialization logic for a class from within the JsonConverter of that class, and so far this is the only way I've found to be able to do that, however all of my classes use init only setters as they're meant to be immutable.

Is there a workaround in non-source generation for getting the default deserialization and serialization functionality for a type that has JsonConverter?

For example, this is something like what I want to do in the current Link JsonConverter:

      protected override void Write(Utf8JsonWriter writer, Type typeToConvert, Link value) {
        if(value.Types.SequenceEqual(DefaultTypeNames)
          && value.MediaType == DefaultMediaType
          && value.OtherValues == null
        ) {
          // if most fields are empty and default, serialize to a single string
          writer.WriteStringValue(value.Href);
        }
        else {
          // use default generated logic:
          JsonSerializer.Serialize(writer, value, typeof(Link), SerializeOnlyLinkOnlyContext.Default.Options);
        }
      }

[cybertyche]

I just did a deep dive on C# 10/.Net 6 features for my team this week and ran into this problem.

Specifically, I was trying to demonstrate this feature but also show off the new record and record struct features, and in their shorthand forms these two features are incompatible. It seems a real shame, sadly, because it would be really, REALLY nice in our codebase to be able to do the following:

public record MyClass(...);

And then be able to use source generation to enable fast serialization/deserialization. Right now, because we have a great deal of code for JSON contracts that look like:

public class MyClass { public string? Field1 { get; set; } public int Field1 { get; set; } }

It would be wonderful to be able to move these to just using the shorthand record notation, which would also allow us to remove the nullable notation on some fields. In the meantime, I'm recommending to them to simply swap the class keyword for record, which gives us some of the benefit, but this really feels like a missed opportunity.

[jeffhandley]

I'm updating this feature's milestone to Future. We understand that handling init-only properties will be very useful, but we want to set expectations appropriately that it is not likely to make it into .NET 7.

Please refer to #63762 to see our revised set of System.Text.Json work planned for .NET 7. We are also doing a lot of incremental refactoring during .NET 7 that makes it easier for us to add features like this one and ensure developers have a more consistent experience when using System.Text.Json #63918. The list of PRs in System.Text.Json illustrates the progress we've been making on that effort.

[JohnGalt1717]

@jeffhandley Just to chime in on the importance of this: Right now, Records are the recommended way of doing DTOs in .NET. If you use the recommended way of doing DTOs in .NET then you can't use the source generator. That's a major issue because your recommendations are in direct conflict and makes Json source generators largely useless unless you open yourself up to mutable DTOs which is exactly what DTOs are not.

I'd strongly suggest that this gets prioritized for .NET 7 to be able to deserialize records properly otherwise this feature is really just a dustbin feature that doesn't get used and wastes dev's time and makes us annoyed that a good feature that significantly improves perf can't be used in the obvious use case. I think the comments makes that obvious.

At the very least this needs to be right at the top of the documentation for .NET 6+ until it's fixed "If you're using records or init only properties, don't bother with this functionality it doesn't work." Of course that will generate a ton of complaints from people that get annoyed with the obviousness of the only real use case for these not working, but at least you're being honest and direct and preventing people from wasting their time.

[jeffhandley]

Thank you for the valuable input, @JohnGalt1717. I appreciate each aspect of your recommendations here.

[cybertyche]

@jeffhandley @JohnGalt1717 I just want to second the sense of urgency here. My team is actively trying to do two things simultaneously: follow guidance for best practices on newest frameworks, and optimize performance in a setting where microseconds matter. Not having this feature means that we are having to do some uncomfortable half-measures throughout our code. In particular, we're trying to use:

• Records for better code brevity in code contracts
• Null check enabling
• Generated serializers

With generated serializers unable to cope with init members, our use of records is as noted earlier just using standard classes with the class keyword replaced by record. It loses most of the benefit of using records, and means all properties have public setters, which in turn makes null checking incredibly grumpy. The resulting code is awkward and unnatural at best, confusing and kind of ugly at worst. It's enough that it makes members of my team uncomfortable, PR reviews painful, and denies us the best parts of each of the features we are trying to use.

The feature of generated serializers in its current form being unable to play well with records, which are arguably the biggest tentpole feature of contract coding alongside generated serializers, is a substantial gap and makes the story about why to use them really, really hard to sell. Records and generated serializers together are a consistent story about code simplification and speed around JSON contracts. Having them essentially pitted against each other for adoption seems a situation we want to remedy as soon as possible.

[eiriktsarpalis]

I would recommend upvoting the original post, since we use that signal to drive prioritization in the backlog.

[jharjung]

Would it be possible to prioritize a smaller fix to the analyzer that just silences the SYSLIB1037 warning for init-only properties that are also parameters of the deserialization constructor? This warning is inaccurate, as source generator deserialization does work exactly as it should for such properties.

Making the source generator logic work well with init-only properties in general is important. But doing only the above would enable the vast majority of use cases involving record types, while still giving a legitimate warning for non-constructor init-only props that the source generator can't currently handle.

[eiriktsarpalis]

Would it be possible to prioritize a smaller fix to the analyzer that just silences the SYSLIB1037 warning for init-only properties that are also parameters of the deserialization constructor? This warning is inaccurate, as source generator deserialization does work exactly as it should for such properties.

I wouldn't agree that the warning is inaccurate -- serialization really only works with fast-path serialization. In any case, it is still appropriate to surface a warning since we need to signal to users that the source generator has no way of roundtripping a value. If the warning is pervasive, it can always be disabled at the project level.

[jharjung]

I don't think that's right.

When a property happens to have an init-only setter, but is also included in the constructor chosen for deserialization, then deserialization will not use the init-only setter - it will pass the value in via the constructor. The fast path isn't disrupted, and the values DO round trip.

This is exactly the pattern that arises from desugaring record types into classes. The positional parameters become constructor parameters, and also happen to get init-only property setters.

This is why the source generator actually does work just fine with record types, if you ignore the SYSLIB1037 warning and your record types don't have any non-positional init-only properties. But disabling the warning at the project level is a bad idea, because then you also won't get the legitimate warnings for those non-positional init-only properties that aren't constructor parameters and do break the fast path.

To give an example, this class will work just fine with the source generator and shouldn't produce the SYSLIB1037 warning:

public class Foo
{
  public string Bar { get; init; } // <-- analyzer produces SYSLIB1037 because it found an init-only property setter

  public Foo(string Bar) // <-- but it's a constructor parameter! deserialization isn't going to use the property setter
  {
    this.Bar = Bar;
  }
}

And that class is of course the desugaring of: public record Foo(string Bar)

[eiriktsarpalis]

I was going to write a comment about the init-only setter being redundant in your Foo example, but then I saw your remark of it being the codegen for the record type. I'd support a PR that improves the analyzer for that particular case but we likely won't be able to dedicate cycles for such a change ourselves.

[jharjung]

It looks like a simple enough fix, around line 1031 in gen/JsonSourceGenerator.Parser.cs.

The current code is:

if (!hasInitOnlyProperties && spec.CanUseSetter && spec.IsInitOnlySetter)
{
  _sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InitOnlyPropertyDeserializationNotSupported, memberLocation, new string[] { type.Name }));
  hasInitOnlyProperties = true;
}

We have easy access to the paramGenSpecArray that tells us whether the init-only property is a constructor parameter or not. So it should be as simple as:

if (!hasInitOnlyProperties && spec.CanUseSetter && spec.IsInitOnlySetter && !IsConstructorParameter(spec, paramGenSpecArray))
{
  _sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InitOnlyPropertyDeserializationNotSupported, memberLocation, new string[] { type.Name }));
  hasInitOnlyProperties = true;
}

private static bool IsConstructorParameter(PropertyGenerationSpec propSpec, ParameterGenerationSpec[]? paramGenSpecArray) =>
  paramGenSpecArray != null && paramGenSpecArray.Any(paramSpec => propSpec.ClrName == paramSpec.ParameterInfo.Name);

I don't know if it needs any optimization beyond that. eg, a hashed or sorted collection to hold all the ParameterInfo.Name values so we can ask ctorParams.Contains(propSpec.ClrName) instead of the linear search we get with paramGenSpecArray.Any. That would be trivial to do, but may or may not give a win, depending on what scenario you're optimizing for.

[eiriktsarpalis]

@jharjung would be interested in contributing a PR?

[jharjung]

Sure, I’ll see what I can do. Will need to clone the repo and get a build going and such so it might be a few days.

[jharjung]

@eiriktsarpalis The PR is opened: https://github.com/dotnet/runtime/pull/68064