Add a STJ feature flag recognizing nullable reference type annotations on properties

[eiriktsarpalis]

Background and motivation

STJ should be able to recognize non-nullable reference types in property and constructor parameter annotations and fail serialization or deserialization if the run-time values are found to violate nullability. Recognizing annotations is valuable from the perspective of JSON schema extraction, since the current nullable-oblivious semantics result in more permissive schemas than what users might desire.

Enabling this would be a breaking change, so it needs to be turned on via an opt-in flag. The proposal intentionally makes this a property rather than a process-wide feature switch so that individual components can use the semantics appropriate to them.

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool ResolveNullableReferenceTypes { get; set; }
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool ResolveNullableReferenceTypes { get; set; }
}

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonPropertyInfo
{
    // Allow `null` values that might be returned by the getter.
    // Defaults to false if the global flag has been enabled 
    // and the property is annotated as non-nullable 
    // (e.g. via type annotation or `NotNull` or `MaybeNull` attributes)
    public bool AllowNullWrites { get; set; }

    // Allow `null` values to be passed to the setter
    // Defaults to false if the global flag has been enabled
    // and the property is annotated as non-nullable
    // (e.g. via type annotation or `DisallowNull` or `AllowNull` attributes)
    // either on the property itself or the associated constructor parameter.
    public bool AllowNullReads { get; set; }
}

API usage

// Default semantics
JsonSerializer.Deserialize<Person>("""{"Name":null, "Address":null}"""); // Success

// Nullable resolution enabled
var options = new JsonSerializerOptions { ResolveNullableReferenceTypes = true };
JsonSerializer.Deserialize<Person>("""{"Name":"John", "Address": null}"""); // Success
JsonSerializer.Deserialize<Person>("""{"Name":null, "Address": "22 Acacia Avenue"}"""); // JsonException

record Person(string Name, string? Address);

Alternative designs

This proposal is very closely related to #100075. We should consider a design that unifies both behaviors under a single feature switch.

Risks

Because of restrictions on how NRT's are implemented and the architecture used by JsonSerializer (contracts keyed on System.Type which is nullable-agnostic), it is currently not possible to determine nullability annotations for root-level types, generic properties or collection elements. For example, it is not possible to distinguish between Person and Person? or List<Person> and List<Person?> as root-level values. This could lead to user confusion so we should try to document this concern as clearly as possible.

[TylerBrinkley]

Those JsonPropertyInfo AllowNullWrites and AllowNullReads properties should default to false not true when the global flag has been enabled and the property is annotated as non-nullable, correct?

[bachratyg]

Just bikeshedding here but the originally proposed EnforceNonNullableReferenceTypes feels more intuitive than ResolveNullableReferenceTypes.

[eiriktsarpalis]

Those JsonPropertyInfo AllowNullWrites and AllowNullReads properties should default to false not true when the global flag has been enabled and the property is annotated as non-nullable, correct?

That is my expectation.

[TylerBrinkley]

Sorry, it's just the comments for the properties indicate the opposite.

[eiriktsarpalis]

Ah right, the comments predate the most recent naming change. Updated.

[eiriktsarpalis]

Just bikeshedding here but the originally proposed EnforceNonNullableReferenceTypes feels more intuitive than ResolveNullableReferenceTypes.

You mean using "non-nullable" instead of "nullable" in the name? Even though "non-nullable reference types" is technically more precise vis-a-vis the language semantics when the feature is disabled, calling it "nullable reference types" isn't wrong either and in practice the two terms are used interchangeably. The latter tends to win because it's shorter. These names are far from final, I'm sure they will be adequately bikeshed during API review.

[jozkee]

I think using "non-nullable" wording limits the scope, "nullable reference types" implies that all suffixes are honored. It also guards against suffixes that could be added in the future.

I'm also not sold on using "Resolve". If only ? will be honored, there's only one thing to resolve, that is, "enforce non-nullable" or "treat as requred" hence I think the previous name was better.

[TheRealAyCe]

Any possibility of specifying parsing behaviour for a missing string property, so that instead of an exception we can also just get an empty string? Maybe if the type is "simple"/built-in, try to generate a default value by invoking the empty constructor, if possible?

Optionally, of course.

This allows for "default" behaviour when null is not expected without having to declare all properties beforehand. Useful for settings files whose properties could change in the future.

[TylerBrinkley]

Ah right, the comments predate the most recent naming change. Updated.

The comments still say it defaults to true

[eiriktsarpalis]

Any possibility of specifying parsing behaviour for a missing string property, so that instead of an exception we can also just get an empty string?

No, that's out of scope for this feature. It specifically controls what happens when a given property exists in the JSON and is deserializing to null.

[eiriktsarpalis]

I think using non-nullable limits the scope, "nullable reference types" implies that all suffixes are honored

The scope of the feature is to accurately reflect all possible nullability states that can exist on a property (as modelled in, say, the NullabilityInfo class), so I wouldn't constrain it to just touching types that are explicitly non-nullable. There is a spectrum of configurations that can achieved using the NotNull, MaybeNull, AllowNull and DisallowNull attributes. In that sense, I would think that we could claim that the feature is about reference type nullability in general.

Perhaps some confusion might stem from the fact that this feature can only get nullability info from properties and constructor parameters, and is not meant to be able to fully reason about NRT's (type-level or generic parameter annotations, or suppressions appended to a JsonSerializer callsite). If that's the case we could try to disambiguate by calling it something something more specific like ResolveMemberNullabilityAnnotations.

[TheRealAyCe]

Any possibility of specifying parsing behaviour for a missing string property, so that instead of an exception we can also just get an empty string?

No, that's out of scope for this feature. It specifically controls what happens when a given property exists in the JSON and is deserializing to null.

And I just remembered that this scenario should already be covered by providing default values for the type you're deserializing, right?

[bachratyg]

You mean using "non-nullable" instead of "nullable" in the name?

It's less the non, more the resolve. To me resolving means something like a name table lookup or what JsonTypeResolver does. Enforce describes the behavior better: disabled/false means more lax, enabled/true means more strict behavior. After this the "non" just reads better: it's the non-nullness that is enforced.

It could also be an enum e.g. NullableReferenceTypeHandling.Strict or Lax or something similar leaving the door open for more fine grained options. Arguably it's a bit more chatty but aligns better with existing behavior flags on JsonSerializerOptions like JsonNumberHandling.

[huoyaoyuan]

Talking about nullability, is there any idea to deal with top-level nulls more fluently? I know that json literal null is valid, but it's just not realistic for real-world payloads. I want a Deserialize overload that returns non-null top-level object and throws for literal null.

[eiriktsarpalis]

It's something we could consider adding as a setting or attribute annotation on individual types, but it would be independent of nullable reference type annotations (IOW, it's not possible to distinguish between Deserialize<Person>() and Deserialize<Person?>() calls currently).

[Fazer01]

Following this as I've opened up another discussion where this issue was mentioned (marked answer from @huoyaoyuan)

#101420

[bartonjs]

Video

• AllowNullReads/Writes changed to DisallowNullReads/Writes to better match usage expectations and an existing [DisallowNull] attribute.
• ResolveNullableReferenceTypes => IgnoreNullableAnnotations
• Probably wants an AppContext switch to change the default. e.g. System.Text.Json.JsonSerializer.IgnoreNullableAnnotationsDefault

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool IgnoreNullableAnnotations { get; set; } = false;
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public bool IgnoreNullableAnnotations { get; set; } = false;
}

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonPropertyInfo
{
    public bool DisallowNullWrites { get; set; }

    public bool DisallowNullReads { get; set; }
}

[dotnet-policy-service]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

[KennethHoff]

Given the following scenario:

var person1 = JsonSerializer.Deserialize<Person>("{Age: 25}");
var person2 = JsonSerializer.Deserialize<Person>("{FirstName: null, LastName: null}");

class Person
{
	public string FirstName { get; set; }
	public string LastName { get; set; }
}

If I understood correctly, this will result in the following:

person1: {
	FirstName: null,
	LastName: null,
}

person2: throws 

Which, to me, does not feel like a "pit of success".

I think IgnoreNullableAnnotations = false should implicitly also mean mark all non-nullable reference types as IsRequired, because otherwise this doesn't really add anything; The property is still just as nullable as it used to, it's just that it's less likely to happen - You still have to handle it, so what did we gain, except presumably more overhead in the serializer?

You could argue that "Well, your type annotations were wrong" - and I would agree - but this proposal does not change anything in that regard; There are two main things you can do to fix this;

1. Mark the properties as required (public required string FirstName { get; set; }
2. Mark the properties as nullable (public string? FirstName { get; set; }

Both of these works just as well today as it will after this proposal, so I fail to see what we gain from this.

---

Assuming I understood incorrectly - and what this proposal.. proposes.. is to mark NRTs as required - then I think that should be reflected in the names. Something like MarkNonNullableReferenceTypesAsRequired, or DoNotMarkNonNullableReferenceTypesAsRequired (which, I believe, would still be a shorter name than the NRT-related configuration in ASP Model Binding :P)

[bachratyg]

There are two main things you can do to fix this;

1. Mark the properties as required (public required string FirstName { get; set; }
2. Mark the properties as nullable (public string? FirstName { get; set; }

3. set a default e.g. public string FirstName { get; set; } = "John";

In this case person2 would still throw, person1 would be { FirstName: "John", ...}

Lang team says nullability and requiredness are orthogonal concerns. OpenAPI Schema does the same (see https://swagger.io/docs/specification/data-models/data-types/ sections Null and Required Properties). STJ should follow suit.

[eiriktsarpalis]

Having worked on the implementation (via #102499), me and @jozkee have come to the conclusion that making this behavior the default is going to create substantial disruption (the large number of failing unit tests providing a strong signal that this is going to be the case). We have decided to instead make this an opt-in feature, which correspondingly should influence a change to the APIs:

public partial class JsonSerializerOptions
{
    public class bool RespectRequiredConstructorParameters { get; set; } = false;
-   public class IgnoreNullableAnnotations { get; set; } = true;
+   public class RespectNullableAnnotations { get; set; } = false;
}

public partial class JsonPropertyInfo
{
    public Func<object, object?> Get { get; set; }
-   public bool DisallowNullWrites { get; set; }
+   public bool IsGetNonNullable { get; set; }

    public Action<object, object?> Set { get; set; }
-   public bool DisallowNullReads { get; set; }
+   public bool IsSetNonNullable { get; set; }
}

public partial class JsonParameterInfoValues
{
-    public bool DisallowNullReads { get; init; }
+    public bool IsNonNullable { get; init; }
}

The name change from DisallowNullWrites to IsGetNonNullable conveys a slight change in semantics: the property now always reflects underlying nullability metadata, but whether the serializer acts on that metadata is governed by the global RespectNullableAnnotations flag.

[neon-sunset]

Thank you, this looks promising! I assume the middle ground of source generator looking at nullability settings in the project was not an option? e.g. having <WarningsAsErrors>nullable</WarningsAsError> making the nullability behavior strict?

[eiriktsarpalis]

It wouldn't completely avoid the potential for breaking changes, nullable warnings are far from perfect and they can always be suppressed inside a codebase.

[terrajobst]

Video

• We should use non-negated properties (i.e. remove the Non infix)

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    // Existing
    // public class bool RespectRequiredConstructorParameters { get; set; } = false;
    public class RespectNullableAnnotations { get; set; } = false;
}

public partial class JsonPropertyInfo
{
    // Existing
    // public Func<object, object?> Get { get; set; }
    public bool IsGetNullable { get; set; }

    // Existing
    // public Action<object, object?> Set { get; set; }
    public bool IsSetNullable { get; set; }
}

public partial class JsonParameterInfoValues
{
    public bool IsNullable { get; init; }
}