Proposal: Update JsonSerializer.Deserializer (and friends) so the return type is not automatically nullable

[richardszalay]

While I appreciate that there are at least two parts to this, specifically "whether the team agrees with the idea" and "how feasible it is to change in terms of public API changes", this discussion is intended to be on the former.

The JsonSerializer.Deserialize<T>() family of methods all return T? rather than T. This necessitates adding additional ceremony (either ! or a throw) in realistically at least 99% percent of call sites because the behaviour of the target API is usually well defined, if not owned by the same party.

As far as I understand, the justification/argument for the API in its present state is that the JSON being deserialized could simply be the value null (being valid JSON)

Assuming my understanding is correct, I'd like to offer two counterarguments:

1. The value could be anything. On top of being null, it could be another object shape, a JSON-encoded string, or a number. All are valid JSON and none are currently protected against via the deserialization method signatures.
2. The methods are generic, and C# has long-since supported a nullable annotation for reference types. If the caller wants to mark the type as nullable, the tools are available to them.

[huoyaoyuan]

#100144 (comment)

2. The methods are generic, and C# has long-since supported a nullable annotation for reference types. If the caller wants to mark the type as nullable, the tools are available to them.

No, it's not available. Reference nullability at non-signature are totally erased.

[richardszalay]

#100144 (comment)

2. The methods are generic, and C# has long-since supported a nullable annotation for reference types. If the caller wants to mark the type as nullable, the tools are available to them.

No, it's not available. Reference nullability at non-signature are totally erased.

That's true, but that only prevents the framework enforcing for you.

I meant that if the caller marks the type as nullable, they have to handle it being null. But that's no different to them handling the response being the wrong shape, having additional properties, etc.

[huoyaoyuan]

Removing the support of top-level null is a breaking change. There has to be some configuration or alternative entry.

[richardszalay]

No doubt, but that's why I started this discussion with:

While I appreciate that there are at least two parts to this, specifically "whether the team agrees with the idea" and "how feasible it is to change in terms of public API changes", this discussion is intended to be on the former.

[huoyaoyuan]

@eiriktsarpalis any plan or consideration about this topic?

[eiriktsarpalis]

The value could be anything. On top of being null, it could be another object shape, a JSON-encoded string, or a number. All are valid JSON and none are currently protected against via the deserialization method signatures.

There is an importance difference though. If you try to pass a JSON string or number to the deserializer of an object it will throw an exception. If you pass JSON null it will successfully deserialize to null.

While I appreciate that there are at least two parts to this, specifically "whether the team agrees with the idea" and "how feasible it is to change in terms of public API changes", this discussion is intended to be on the former.

There has been debate about whether we could expose a setting disallowing null for specific reference types. Even if that were added though, because it is a run-time setting it would have no bearing on the nullability annotation of the deserialization methods.

[richardszalay]

There is an importance difference though. If you try to pass a JSON string or number to the deserializer of an object it will throw an exception. If you pass JSON null it will successfully deserialize to null.

That's true, but it would hardly be the only scenario that the APIs allow through. The current limitations of RespectNullableAnnotations comes to mind, where an absent JSON property will result in the property having a null value despite the compiler telling you it won't.

Applying the same logic (and being unnecessarily facetious), it technically should be forcing all reference properties on the target type to be nullable since any of those could also end up being null.

There has been debate about whether we could expose a setting disallowing null for specific reference types. Even if that were added though, because it is a run-time setting it would have no bearing on the nullability annotation of the deserialization methods.

Honestly, this more than anything probably makes any other discussion irrelevant. It sounds like it just not a change that can happen unless the team created a completely separate JSON deserialization API.

I appreciate you humouring me!

[eiriktsarpalis]

The current limitations of RespectNullableAnnotations comes to mind, where an absent JSON property will result in the property having a null value despite the compiler telling you it won't.

Not to nitpick, but RespectNullableAnnotations only checks the validity of incoming JSON data but doesn't provide any guarantees about the integrity of the unmarshalled class. If a non-nullable property ends up with a null value it usually means that a nullability warning has been suppressed at the property definition. In other words, the serializer behaves in the same way as if an instance of the class was created manually with that property not being explicitly set.

Applying the same logic (and being unnecessarily facetious), it technically should be forcing all reference properties on the target type to be nullable since any of those could also end up being null.

That is literally the default behavior of the serializer today, although obviously this has no impact on compile-time nullability annotations given that it's a run-time library.

It sounds like it just not a change that can happen unless the team created a completely separate JSON deserialization API.

To be clear, this is not a matter of breaking change that a new API would be able to solve. We are generally flexible when it comes to altering nullability annotations. It's simply a matter of the deserialize method being generic and the T could sometimes represent a type that's configured as non-nullable but other times represents one that isn't. A non-nullable return type is making the assertion that null will not be returned under any circumstance, and if we can't uphold that then it must be made nullable.

[eiriktsarpalis]

If the caller wants to mark the type as nullable, the tools are available to them.

I need to clarify that nullable reference types is a compile-time feature that mostly gets erased when it comes to IL and the runtime. From the perspective of the serializer the calls Deserialize<string?>() and Deserialize<string>() are indistinguishable. Here's a sharplab example demonstrating this.

[richardszalay]

It's simply a matter of the deserialize method being generic and the T could sometimes represent a type that's configured as non-nullable but other times represents one that isn't.

I get it, I do. I guess I just disagree with the design choice given how infrequently nulls come up as JSON responses vs how often I have to add a ! to Deserialize<T>. I would personally prefer it that if I expected null, I should use T?, but I may not be the majority here.

I appreciate your time!

[eiriktsarpalis]

I guess I just disagree with the design choice given how infrequently nulls come up as JSON responses vs how often I have to add a ! to Deserialize.

That's not a design choice on the part of STJ though, it's just a matter of how nullability annotations work in general. Namely, it's not something chosen arbitrarily on the basis of frequency, but rather it is a consequence of a type being provably not null. The language team deliberately chose ! as a syntactically lightweight suppression mechanism in acknowledgement of the fact that as a proof system non-nullable reference types is neither sound nor complete. Users are expected to use ! from time to time, but they also assume the risk of null reference exceptions occurring further down the line because of it.

I would personally prefer it that if I expected null, I should use T?, but I may not be the majority here.

See my earlier comment on IL limitations not making such an approach feasible.