Change null handling in custom converters for backwards compat (dotne…

…t#40859)

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

@@ -21,9 +21,21 @@ protected internal JsonConverter()
             // In the future, this will be check for !IsSealed (and excluding value types).
             CanBePolymorphic = TypeToConvert == JsonClassInfo.ObjectType;
             IsValueType = TypeToConvert.IsValueType;
-            HandleNull = IsValueType;
             CanBeNull = !IsValueType || Nullable.GetUnderlyingType(TypeToConvert) != null;
             IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;
+
+            if (HandleNull)
+            {
+                HandleNullOnRead = true;
+                HandleNullOnWrite = true;
+            }
+
+            // For the HandleNull == false case, either:
+            // 1) The default values are assigned in this type's virtual HandleNull property
+            // or
+            // 2) A converter overroad HandleNull and returned false so HandleNullOnRead and HandleNullOnWrite
+            // will be their default values of false.
+
             CanUseDirectReadOrWrite = !CanBePolymorphic && IsInternalConverter && ClassType == ClassType.Value;
         }
 
@@ -61,7 +73,34 @@ internal override sealed JsonParameterInfo CreateJsonParameterInfo()
         /// <remarks>
         /// The default value is <see langword="true"/> for converters for value types, and <see langword="false"/> for converters for reference types.
         /// </remarks>
-        public virtual bool HandleNull { get; }
+        public virtual bool HandleNull
+        {
+            get
+            {
+                // HandleNull is only called by the framework once during initialization and any
+                // subsequent calls elsewhere would just re-initialize to the same values (we don't
+                // track a "hasInitialized" flag since that isn't necessary).
+
+                // If the type doesn't support null, allow the converter a chance to modify.
+                // These semantics are backwards compatible with 3.0.
+                HandleNullOnRead = !CanBeNull;
+
+                // The framework handles null automatically on writes.
+                HandleNullOnWrite = false;
+
+                return false;
+            }
+        }
+
+        /// <summary>
+        /// Does the converter want to be called when reading null tokens.
+        /// </summary>
+        internal bool HandleNullOnRead { get; private set; }
+
+        /// <summary>
+        /// Does the converter want to be called for null values.
+        /// </summary>
+        internal bool HandleNullOnWrite { get; private set; }
 
         /// <summary>
         /// Can <see langword="null"/> be assigned to <see cref="TypeToConvert"/>?
@@ -109,7 +148,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
                 Debug.Assert(!state.IsContinuation);
 
                 // For perf and converter simplicity, handle null here instead of forwarding to the converter.
-                if (reader.TokenType == JsonTokenType.Null && !HandleNull)
+                if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead)
                 {
                     if (!CanBeNull)
                     {
@@ -184,7 +223,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
             // For performance, only perform validation on internal converters on debug builds.
             if (IsInternalConverter)
             {
-                if (reader.TokenType == JsonTokenType.Null && !HandleNull && !wasContinuation)
+                if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead && !wasContinuation)
                 {
                     if (!CanBeNull)
                     {
@@ -206,7 +245,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
                 if (!wasContinuation)
                 {
                     // For perf and converter simplicity, handle null here instead of forwarding to the converter.
-                    if (reader.TokenType == JsonTokenType.Null && !HandleNull)
+                    if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead)
                     {
                         if (!CanBeNull)
                         {
@@ -267,7 +306,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
             {
                 if (value == null)
                 {
-                    if (!HandleNull)
+                    if (!HandleNullOnWrite)
                     {
                         writer.WriteNullValue();
                     }
@@ -303,9 +342,9 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
                     }
                 }
             }
-            else if (value == null && !HandleNull)
+            else if (value == null && !HandleNullOnWrite)
             {
-                // We do not pass null values to converters unless HandleNull is true. Null values for properties were
+                // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were
                 // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here.
                 writer.WriteNullValue();
                 return true;

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs

@@ -135,7 +135,7 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf
             {
                 Debug.Assert(Converter.CanBeNull);
 
-                if (Converter.HandleNull)
+                if (Converter.HandleNullOnWrite)
                 {
                     // No object, collection, or re-entrancy converter handles null.
                     Debug.Assert(Converter.ClassType == ClassType.Value);
@@ -194,7 +194,7 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U
             bool success;
 
             bool isNullToken = reader.TokenType == JsonTokenType.Null;
-            if (isNullToken && !Converter.HandleNull && !state.IsContinuation)
+            if (isNullToken && !Converter.HandleNullOnRead && !state.IsContinuation)
             {
                 if (!Converter.CanBeNull)
                 {
@@ -242,7 +242,7 @@ public override bool ReadJsonAsObject(ref ReadStack state, ref Utf8JsonReader re
         {
             bool success;
             bool isNullToken = reader.TokenType == JsonTokenType.Null;
-            if (isNullToken && !Converter.HandleNull && !state.IsContinuation)
+            if (isNullToken && !Converter.HandleNullOnRead && !state.IsContinuation)
             {
                 if (!Converter.CanBeNull)
                 {

src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.HandleNull.cs

@@ -172,15 +172,17 @@ public static void NullableValueTypeConverter_NoOverride()
             Assert.Null(val);
             Assert.Equal("null", JsonSerializer.Serialize(val));
 
-            // Per null handling default value for value types (true), converter handles null.
+            // For compat, deserialize does not call converter for null token unless the type doesn't support
+            // null or HandleNull is overridden and returns 'true'.
+            // For compat, serialize does not call converter for null unless null is a valid value and HandleNull is true.
             var options = new JsonSerializerOptions();
             options.Converters.Add(new NullableInt32NullConverter_SpecialCaseNull());
 
             val = JsonSerializer.Deserialize<int?>("null", options);
-            Assert.Equal(-1, val);
+            Assert.Null(val);
 
             val = null;
-            Assert.Equal("-1", JsonSerializer.Serialize(val, options));
+            Assert.Equal("null", JsonSerializer.Serialize(val, options));
         }
 
         private class NullableInt32NullConverter_SpecialCaseNull : JsonConverter<int?>

src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.NullableTypes.cs

@@ -161,32 +161,35 @@ public override void Write(Utf8JsonWriter writer, int? value, JsonSerializerOpti
         }
 
         [Fact]
-        public static void NullableConverterIsPassedNull()
+        public static void NullableConverterIsNotPassedNull()
         {
+            // For compat, deserialize does not call converter for null token unless the type doesn't support
+            // null or HandleNull is overridden and returns 'true'.
+            // For compat, serialize does not call converter for null unless null is a valid value and HandleNull is true.
+
             var options = new JsonSerializerOptions();
             options.Converters.Add(new NullIntTo42Converter());
 
             {
                 int? myInt = JsonSerializer.Deserialize<int?>("null", options);
-                Assert.True(myInt.HasValue);
-                Assert.Equal(42, myInt.Value);
+                Assert.Null(myInt);
             }
 
             {
                 string json = JsonSerializer.Serialize<int?>(null, options);
-                Assert.Equal("42", json);
+                Assert.Equal("null", json);
             }
 
             {
                 int?[] ints = JsonSerializer.Deserialize<int?[]>("[null, null]", options);
                 Assert.Equal(2, ints.Length);
-                Assert.Equal(42, ints[0]);
-                Assert.Equal(42, ints[1]);
+                Assert.Null(ints[0]);
+                Assert.Null(ints[1]);
             }
 
             {
                 string json = JsonSerializer.Serialize<int?[]>(new int?[] { null, null }, options);
-                Assert.Equal("[42,42]", json);
+                Assert.Equal("[null,null]", json);
             }
         }