Fix up some nullability annotations to remove unnecessary null-forgiving operations (!)

[ahsonkhan]

Leftover from #2259 (comment)

[ahsonkhan]

With this, once #32090 is merged, we can remove the ! in classes like:
JsonIEnumerableDefaultConverter.cs
JsonDictionaryDefaultConverter.cs
etc.

Everywhere we do:
value = default!;

[ahsonkhan]

@steveharter - can you please take another quick look.

[stephentoub]

NotNull doesn't make sense here. Did you mean DisallowNull? And just so I understand, it's invalid to write nulls but read can return nulls?

[ahsonkhan]

And just so I understand, it's invalid to write nulls but read can return nulls?

Yes. Write won't receive null but Read can return null.

The intent is the JsonSerializer will never pass in null T values to the JsonConverter<T>.Write method and hence the implementer of that method doesn't need to do a null check on it.

They can certainly continue to write null JSON literals in the method itself or do whatever else. If an nullable object has the null value, the serializer writes null for you (and doesn't call the JsonConverter).

On the read side, the implementer should honor the nullability of the T. If T is nullable, the implementer of JsonConverter<T>.Read can certainly return null if they wish. If T is non-nullable (like string, or value type), it doesn't make sense for them to do so.

For types where null doesn't make sense (i.e. non-nullable valuetypes), the JsonSerializer can and will pass in a Utf8JsonReader with the null TokenType if the payload we are processing has the null JSON literal.
For types where null is allowed (nullable/non-nullable reference types), the JsonSerializer will not pass in a Utf8JsonReader in that state and will eagerly set the object to null up front, without calling the JsonConverter.

#nullable enable

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Xunit;

namespace System.Text.Json.Serialization.Tests
{
    public static partial class PropertyNameTests
    {
        [Fact]
        public static void QuickTest()
        {
            var opts = new JsonSerializerOptions
            {
                Converters =
                {
                    new MyStringConverter(),
                    new MyNullableStringConverter(),
                    new MyNullableIntConverter(),
                    new MyIntConverter()
                }
            };

            string json = "{\"foo\": null, \"bar\": null, \"baz\": null,\"nullBaz\": null}";

            MyClass output = JsonSerializer.Deserialize<MyClass>(json, opts)!;
            Assert.Null(output.foo);
            Assert.Null(output.bar);
            Assert.Equal(0, output.baz);
            Assert.Null(output.nullBaz);
        }

        public class MyClass
        {
#pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.
            public string foo { get; set; }
#pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.

            public string? bar { get; set; }

            public int baz { get; set; }

            public int? nullBaz { get; set; }
        }

        public class MyStringConverter : JsonConverter<string>
        {
            public override string Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                Debug.Assert(reader.TokenType != JsonTokenType.Null);

                return reader.GetString()!;
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] string value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyNullableStringConverter : JsonConverter<string?>
        {
            public override string? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                Debug.Assert(reader.TokenType != JsonTokenType.Null);

                return reader.GetString();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] string? value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyIntConverter : JsonConverter<int>
        {
            public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                    return default;
                return reader.GetInt32();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] int value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }

        public class MyNullableIntConverter : JsonConverter<int?>
        {
            public override int? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                    return null;
                return reader.GetInt32();
            }

            public override void Write(Utf8JsonWriter writer, [DisallowNull] int? value, JsonSerializerOptions options)
            {
                throw new NotImplementedException();
            }
        }
    }
}

[stephentoub]

DisallowNull?

[stephentoub]

Is this comment necessary? The purpose of nullable reference types is you shouldn't need a comment like this, given the signature says "out string" rather than "out string?"

[ahsonkhan]

I added it along with the Debug.Assert to make it explicit that referenceId shouldn't be null even if TryGetOrAddReferenceOnSerialize accidentally set referenceId to null!. Otherwise, it's a bug in the implementation detail.

I am fine removing the comment though. Should I remove the Debug.Assert as well?

[stephentoub]

Should I remove the Debug.Assert as well?

The comment seemed superfluous. I'm fine with the Debug.Assert if you think it adds value.

[steveharter]

I am not sure we want to always disallow null. We may have "fast path" code flows for internal converters that pass null. Also, since this may become a public API there may be cases where someone wants to pass null.

[ahsonkhan]

This API is already public. Having the JsonSerializer pass in null or having the user expect/handle null as the input to the Write method would be a behavioral breaking change. We can change the annotation if/when we do that.

Also, even if the JsonSerializer internally passes null for some reason, we should annotate the public surface area with the intent of the API and what the user can expect when they implement the abstract class JsonConverter<T>. What I recall from previous/offline discussions was that we never pass in null to the Write method so DisallowNull makes sense.

Btw, I added if (value == null) throw; to all the Write methods within our test converters to validate that we never pass null.

[steveharter]

Yes I see this is actually on the pubic type.

[steveharter]

Note that when we make "HandleNullValue" property protected (it is internal now), Write() will get nulls passed in. At that time, I believe should remove the [DisallowNull] here

[steveharter]

This may become protected (instead of just internal) and I don't think we want to say this shouldn't be null.

Also what's the difference between DisallowNull and [MaybeNullWhen(false)]?

[ahsonkhan]

DisallowNull is for inputs, MaybeNullWhen(false) is for outputs. In this case this is stating that regardless of the nullability of the generic, the OnTryRead might set it to null when it returns false.

Even if we make it protected (and expose it), the nullability is showcasing the intent of the API. We are doing T value = default! in a bunch of places.

We can revisit the annotation if the implementation/design changes as part of exposing it. This change makes sense to me as it accurately represents what the API is doing so the caller knows what to do with the out parameter.

[steveharter]

Was the motivation here only to remove the ! in referenceId! or also other reasons (perf?)

[ahsonkhan]

Yes, that and perf (avoiding unnecessary/duplicate checks on the enum).

[ghost]

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ahsonkhan]

Failures are unrelated: #13769 (comment)