runtime: Nullability of JsonConverter.Write(T value) not correct

Since the consumer scenario where someone calls the converter directly (Eirik’s example) may NullReference, that is a valid reason not to add the nullable annotation. When forwarding to a converter in that example, HandleNull should be called first to determine if it should call the converter or just write null for it, but since we can’t enforce that pattern, erroring on the side of safety seems better even though it goes against the mainstream nullability practices.

Also, not obvious, but if we don’t annotate nullable on the base class JsonConverter<T>.Write(...) it does allow a derived converter to add the nullable if it overrides AllowNull and returns true, so nullability should be (or can be made) correct for consumers if calling a concrete converter class which should be the common case.

FWIW the canonical example of overriding HandleNull and returning true is a custom String converter that writes a null string reference an empty JSON string (instead of writing null or ignoring it). In any case, these scenarios should be rare.

To enumerate the options:

1. Leave as-is; the parameter is considered not nullable. May prevent NullReferences, but nullability annotations go against mainstream guidance.
2. Change to nullable; consumers that call “normal” converters may assume null is handled, causing NullReference.
3. (suggested) Do not annotate the value on the base class JsonConverter<T>. Although no nullability attributes are written to IL for JsonConverter<T>.Write(), currently for Roslyn this appears is going to be the same as (1) where ! may be needed when calling the base class in order to prevent a warning\error. See sample below.

In all cases, we should also update the existing xml doc for Write() to clear up the semantics and mention HandleNull.

#nullable enable

using System;

namespace ConsoleApp5
{
    class Program
    {
        static void Main(string[] args)
        {
            // Use a converter that is annotated with nullable:
            NullableConverterString nullable = new();
            nullable.Write(null);
            Converter<string?> baseNullable = nullable;
            baseNullable.Write(null);
            Converter<string> baseNullable2 = nullable!;
            baseNullable2.Write(null); // error, but should work?

            // Use a converter that is annotated with non-nullable:
            NonNullableConverterString nonnullable = new();
            nonnullable.Write(null); // error, as expected
            Converter<string> baseNonNullable = nonnullable;
            baseNonNullable.Write(null); // error, but should work?

            // Use a converter that is not annotated:
            NotAnnotatedConverterString notAnnotated = new();
            notAnnotated.Write(null);
            Converter<string> notAnnotatedBase = notAnnotated;
            notAnnotatedBase.Write(null); // error, but should work?
        }
    }

    public class Converter<T>
    {
        public virtual void Write(
#nullable disable
        T
#nullable enable
            value)
        {
            if (true)
            {
                Console.WriteLine(value?.ToString());
            }
        }
    }

    public class NullableConverterString : Converter<string?>
    {
        public override void Write(string? value)
        {
            base.Write(value);
        }
    }

    public class NonNullableConverterString : Converter<string>
    {
        public override void Write(string value)
        {
            base.Write(value);
            base.Write(null); // error, but should work?
        }
    }

#nullable disable
    public class NotAnnotatedConverterString : Converter<string>
    {
        public override void Write(string value)
        {
            base.Write(value);
            base.Write(null);
        }
    }
#nullable enable
}

Switching between non-nullable and nullable-oblivious in the base method of your code sample did not produce a different set of warnings.

Ah, indeed. Here’s an illustration of the behavior. I believe the new reflection APIs would express the difference, but the editor isn’t presenting the oblivious state.

My understanding is that we want to always annotate for nullability even for the rare cases. E.g. Activator.CreateInstance() may return a null object in rare cases so the return value is nullable.

What’s the risk of being incorrect here?

Since this is on an abstract base class, there are both author and consumer implications:

• For an author, which overrides HandleNull to return true, the annotation is not correct.
• For an author, which does not override HandleNull (which is the common case), the annotation is not accurate since value cannot be null.
• For a consumer that calls a custom converter’s Write() method directly, the annotation may not be correct if the converter supports returning null. The consumer should check HandleNull before calling the converter.

@steveharter We have been treating nullable annotation mistakes as bugs; we don’t need to go through API Review. Instead, we are simply aggregating the changes made in documentation by adding them to dotnet/docs#21202.