When serializing arrays and dictionaries, null values are being passed to the custom JsonConverter<T>.Write method

[ahsonkhan]

While reviewing the nullability analysis PR: #32474, found this issue.

This issue got introduced recently in #2259 and regresses the 3.1 behavior.

[Fact]
public static void WritePocoArray()
{
    var input = new MyPoco[] { null, new MyPoco { Foo = "foo" } };

    // On 5.0/master, this fails with InvalidOperationException since the MyPocoConverter.Write gets called with null.
    string json = JsonSerializer.Serialize(input, new JsonSerializerOptions { Converters = { new MyPocoConverter() } });

    // On 3.1, correctly outputs the expected value:
    Assert.Equal("[null,{\"Foo\":\"foo\"}]", json);
}

public class MyPoco
{
    public string Foo { get; set; }
}

public class MyPocoConverter : JsonConverter<MyPoco>
{
    public override MyPoco Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }

    public override void Write(Utf8JsonWriter writer, [DisallowNull] MyPoco value, JsonSerializerOptions options)
    {
        // Debug.Assert(value != null);
        if (value == null)
        {
            throw new InvalidOperationException("The custom converter should never get called with null value.");
        }

        writer.WriteStartObject();
        writer.WriteString(nameof(value.Foo), value.Foo);
        writer.WriteEndObject();
    }
}

System.Text.Json.Serialization.Tests.ArrayTests.WritePocoArray [FAIL]
        System.InvalidOperationException : The custom converter should never get called with null value.
        Stack Trace:
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\tests\Serialization\Array.WriteTests.cs(50,0): at System.Text.Json.Serialization.Tests.ArrayTests.MyPocoConverter.Write(Utf8JsonWriter writer, MyPoco value, JsonSerializerOptions options)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(251,0): at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Collection\ArrayConverter.cs(59,0): at System.Text.Json.Serialization.Converters.ArrayConverter`2.OnWriteResume(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options, WriteStack& state)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\Converters\Collection\IEnumerableDefaultConverter.cs(274,0): at System.Text.Json.Serialization.Converters.IEnumerableDefaultConverter`2.OnTryWrite(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options, WriteStack& state)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(267,0): at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonConverterOfT.cs(60,0): at System.Text.Json.Serialization.JsonConverter`1.TryWriteAsObject(Utf8JsonWriter writer, Object value, JsonSerializerOptions options, WriteStack& state)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Write.cs(24,0): at System.Text.Json.JsonSerializer.WriteCore(Utf8JsonWriter writer, Object value, JsonSerializerOptions options, WriteStack& state, JsonConverter jsonConverter)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Write.Helpers.cs(105,0): at System.Text.Json.JsonSerializer.WriteCore(Utf8JsonWriter writer, Object value, Type type, JsonSerializerOptions options)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Write.Helpers.cs(82,0): at System.Text.Json.JsonSerializer.WriteCore(PooledByteBufferWriter output, Object value, Type type, JsonSerializerOptions options)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Write.Helpers.cs(56,0): at System.Text.Json.JsonSerializer.WriteCoreString(Object value, Type type, JsonSerializerOptions options)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Write.String.cs(21,0): at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
          E:\GitHub\Fork\runtime\src\libraries\System.Text.Json\tests\Serialization\Array.WriteTests.cs(27,0): at System.Text.Json.Serialization.Tests.ArrayTests.WritePocoArray()

On 3.1:
https://github.com/dotnet/corefx/blob/29a6367ee4311da8ce984cfbd358d3214779fe59/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoNotNullable.cs#L115-L122

Current master (5.0):

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/ArrayConverter.cs

Lines 58 to 59 in 5d8f924

	TElement element = array[index];
	if (!elementConverter.TryWrite(writer, element, options, ref state))

I believe this needs to special-case/handle null:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Lines 63 to 68 in 5d8f924

	// Provide a default implementation for value converters.
	internal virtual bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref WriteStack state)
	{
	Write(writer, value, options);
	return true;
	}

Also, we have a test gap here for null elements in collections and enumerables (both with custom converter and default converter).

cc @jozkee, @layomia

[ahsonkhan]

Re-opening. Please clean up the TODO comments in source that link to this issue and update nullability (these comments were introduced in #32474).

For example:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Lines 70 to 75 in 385b4d4

	internal virtual bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref WriteStack state)
	{
	// TODO: https://github.com/dotnet/runtime/issues/32523
	Write(writer, value!, options);
	return true;
	}

I am going to assume all the test cases/examples are now covered :)