Use underlying type converter for nullable type (#84208)

pedrobsaila · eiriktsarpalis · web-flow · commit 2a1b52a1b691 · 2023-05-31T12:29:27.000+01:00
* Use underlying type converter for nullable type

* verify that we use underlying type converter for nullable types

* rollback a little change

* Update src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs

* Update src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs

* Use consistent indentation.

* Fix runtime support for nullable properties with custom converters.

---------

Co-authored-by: Eirik Tsarpalis &lt;eirik.tsarpalis@gmail.com&gt;
diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
@@ -33,6 +33,7 @@ private sealed partial class Emitter
             private const string PropInitMethodNameSuffix = "PropInit";
             private const string TryGetTypeInfoForRuntimeCustomConverterMethodName = "TryGetTypeInfoForRuntimeCustomConverter";
             private const string ExpandConverterMethodName = "ExpandConverter";
+            private const string GetConverterForNullablePropertyMethodName = "GetConverterForNullableProperty";
             private const string SerializeHandlerPropName = "SerializeHandler";
             private const string OptionsLocalVariableName = "options";
             private const string ValueVarName = "value";
@@ -79,6 +80,11 @@ private sealed partial class Emitter
             /// </summary>
             private readonly Dictionary<string, string> _propertyNames = new();
 
+            /// <summary>
+            /// Indicates that the type graph contains a nullable property with a design-time custom converter declaration.
+            /// </summary>
+            private bool _emitGetConverterForNullablePropertyMethod;
+
             /// <summary>
             /// The SourceText emit implementation filled by the individual Roslyn versions.
             /// </summary>
@@ -88,6 +94,7 @@ public void Emit(ContextGenerationSpec contextGenerationSpec)
             {
                 Debug.Assert(_typeIndex.Count == 0);
                 Debug.Assert(_propertyNames.Count == 0);
+                Debug.Assert(!_emitGetConverterForNullablePropertyMethod);
 
                 foreach (TypeGenerationSpec spec in contextGenerationSpec.GeneratedTypes)
                 {
@@ -106,14 +113,15 @@ public void Emit(ContextGenerationSpec contextGenerationSpec)
                 string contextName = contextGenerationSpec.ContextType.Name;
 
                 // Add root context implementation.
-                AddSource($"{contextName}.g.cs", GetRootJsonContextImplementation(contextGenerationSpec));
+                AddSource($"{contextName}.g.cs", GetRootJsonContextImplementation(contextGenerationSpec, _emitGetConverterForNullablePropertyMethod));
 
                 // Add GetJsonTypeInfo override implementation.
                 AddSource($"{contextName}.GetJsonTypeInfo.g.cs", GetGetTypeInfoImplementation(contextGenerationSpec));
 
                 // Add property name initialization.
                 AddSource($"{contextName}.PropertyNames.g.cs", GetPropertyNameInitialization(contextGenerationSpec));
 
+                _emitGetConverterForNullablePropertyMethod = false;
                 _propertyNames.Clear();
                 _typeIndex.Clear();
             }
@@ -539,7 +547,7 @@ private SourceText GenerateForObject(ContextGenerationSpec contextSpec, TypeGene
                 return CompleteSourceFileAndReturnText(writer);
             }
 
-            private static void GeneratePropMetadataInitFunc(SourceWriter writer, string propInitMethodName, TypeGenerationSpec typeGenerationSpec)
+            private void GeneratePropMetadataInitFunc(SourceWriter writer, string propInitMethodName, TypeGenerationSpec typeGenerationSpec)
             {
                 Debug.Assert(typeGenerationSpec.PropertyGenSpecs != null);
                 ImmutableEquatableArray<PropertyGenerationSpec> properties = typeGenerationSpec.PropertyGenSpecs;
@@ -585,9 +593,15 @@ private static void GeneratePropMetadataInitFunc(SourceWriter writer, string pro
                         ? $"{JsonIgnoreConditionTypeRef}.{property.DefaultIgnoreCondition.Value}"
                         : "null";
 
-                    string converterInstantiationExpr = property.ConverterType != null
-                        ? $"({JsonConverterTypeRef}<{propertyTypeFQN}>){ExpandConverterMethodName}(typeof({propertyTypeFQN}), new {property.ConverterType.FullyQualifiedName}(), {OptionsLocalVariableName})"
-                        : "null";
+                    string? converterInstantiationExpr = null;
+                    if (property.ConverterType != null)
+                    {
+                        TypeRef? nullableUnderlyingType = _typeIndex[property.PropertyType].NullableUnderlyingType;
+                        _emitGetConverterForNullablePropertyMethod |= nullableUnderlyingType != null;
+                        converterInstantiationExpr = nullableUnderlyingType != null
+                            ? $"{GetConverterForNullablePropertyMethodName}<{nullableUnderlyingType.FullyQualifiedName}>(new {property.ConverterType.FullyQualifiedName}(), {OptionsLocalVariableName})"
+                            : $"({JsonConverterTypeRef}<{propertyTypeFQN}>){ExpandConverterMethodName}(typeof({propertyTypeFQN}), new {property.ConverterType.FullyQualifiedName}(), {OptionsLocalVariableName})";
+                    }
 
                     writer.WriteLine($$"""
                         var {{InfoVarName}}{{i}} = new {{JsonPropertyInfoValuesTypeRef}}<{{propertyTypeFQN}}>()
@@ -596,7 +610,7 @@ private static void GeneratePropMetadataInitFunc(SourceWriter writer, string pro
                             IsPublic = {{FormatBool(property.IsPublic)}},
                             IsVirtual = {{FormatBool(property.IsVirtual)}},
                             DeclaringType = typeof({{property.DeclaringType.FullyQualifiedName}}),
-                            Converter = {{converterInstantiationExpr}},
+                            Converter = {{converterInstantiationExpr ?? "null"}},
                             Getter = {{getterValue}},
                             Setter = {{setterValue}},
                             IgnoreCondition = {{ignoreConditionNamedArg}},
@@ -1007,7 +1021,7 @@ private static void GenerateTypeInfoProperty(SourceWriter writer, TypeGeneration
                     """);
             }
 
-            private static SourceText GetRootJsonContextImplementation(ContextGenerationSpec contextSpec)
+            private static SourceText GetRootJsonContextImplementation(ContextGenerationSpec contextSpec, bool emitGetConverterForNullablePropertyMethod)
             {
                 string contextTypeRef = contextSpec.ContextType.FullyQualifiedName;
                 string contextTypeName = contextSpec.ContextType.Name;
@@ -1048,7 +1062,7 @@ private static SourceText GetRootJsonContextImplementation(ContextGenerationSpec
 
                 writer.WriteLine();
 
-                GenerateConverterHelpers(writer);
+                GenerateConverterHelpers(writer, emitGetConverterForNullablePropertyMethod);
 
                 return CompleteSourceFileAndReturnText(writer);
             }
@@ -1082,7 +1096,7 @@ private static void GetLogicForDefaultSerializerOptionsInit(ContextGenerationSpe
                     """);
             }
 
-            private static void GenerateConverterHelpers(SourceWriter writer)
+            private static void GenerateConverterHelpers(SourceWriter writer, bool emitGetConverterForNullablePropertyMethod)
             {
                 // The generic type parameter could capture type parameters from containing types,
                 // so use a name that is unlikely to be used.
@@ -1109,15 +1123,20 @@ private static void GenerateConverterHelpers(SourceWriter writer)
                             {{JsonConverterTypeRef}}? converter = options.Converters[i];
                             if (converter?.CanConvert(type) == true)
                             {
-                                return {{ExpandConverterMethodName}}(type, converter, options);
+                                return {{ExpandConverterMethodName}}(type, converter, options, validateCanConvert: false);
                             }
                         }
 
                         return null;
                     }
 
-                    private static {{JsonConverterTypeRef}} {{ExpandConverterMethodName}}({{TypeTypeRef}} type, {{JsonConverterTypeRef}} converter, {{JsonSerializerOptionsTypeRef}} options)
+                    private static {{JsonConverterTypeRef}} {{ExpandConverterMethodName}}({{TypeTypeRef}} type, {{JsonConverterTypeRef}} converter, {{JsonSerializerOptionsTypeRef}} options, bool validateCanConvert = true)
                     {
+                        if (validateCanConvert && !converter.CanConvert(type))
+                        {
+                            throw new {{InvalidOperationExceptionTypeRef}}(string.Format("{{ExceptionMessages.IncompatibleConverterType}}", converter.GetType(), type));
+                        }
+                    
                         if (converter is {{JsonConverterFactoryTypeRef}} factory)
                         {
                             converter = factory.CreateConverter(type, options);
@@ -1126,15 +1145,29 @@ private static void GenerateConverterHelpers(SourceWriter writer)
                                 throw new {{InvalidOperationExceptionTypeRef}}(string.Format("{{ExceptionMessages.InvalidJsonConverterFactoryOutput}}", factory.GetType()));
                             }
                         }
-
-                        if (!converter.CanConvert(type))
-                        {
-                            throw new {{InvalidOperationExceptionTypeRef}}(string.Format("{{ExceptionMessages.IncompatibleConverterType}}", converter.GetType(), type));
-                        }
                     
                         return converter;
                     }
                     """);
+
+                if (emitGetConverterForNullablePropertyMethod)
+                {
+                    writer.WriteLine($$"""
+
+                        private static {{JsonConverterTypeRef}}<{{TypeParameter}}?> {{GetConverterForNullablePropertyMethodName}}<{{TypeParameter}}>({{JsonConverterTypeRef}} converter, {{JsonSerializerOptionsTypeRef}} options)
+                            where {{TypeParameter}} : struct
+                        {
+                            if (converter.CanConvert(typeof({{TypeParameter}}?)))
+                            {
+                                return ({{JsonConverterTypeRef}}<{{TypeParameter}}?>){{ExpandConverterMethodName}}(typeof({{TypeParameter}}?), converter, options, validateCanConvert: false);
+                            }
+                    
+                            converter = {{ExpandConverterMethodName}}(typeof({{TypeParameter}}), converter, options);
+                            {{JsonTypeInfoTypeRef}}<{{TypeParameter}}> typeInfo = {{JsonMetadataServicesTypeRef}}.{{CreateValueInfoMethodName}}<{{TypeParameter}}>(options, converter);
+                            return {{JsonMetadataServicesTypeRef}}.GetNullableConverter<{{TypeParameter}}>(typeInfo);
+                        }
+                        """);
+                }
             }
 
             private static SourceText GetGetTypeInfoImplementation(ContextGenerationSpec contextSpec)
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/ContextClasses.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/ContextClasses.cs
@@ -49,6 +49,8 @@ public interface ITestContext
         public JsonTypeInfo<StructWithCustomConverterProperty> StructWithCustomConverterProperty { get; }
         public JsonTypeInfo<ClassWithCustomConverterFactoryProperty> ClassWithCustomConverterFactoryProperty { get; }
         public JsonTypeInfo<StructWithCustomConverterFactoryProperty> StructWithCustomConverterFactoryProperty { get; }
+        public JsonTypeInfo<ClassWithCustomConverterNullableProperty> ClassWithCustomConverterNullableProperty { get; }
+        public JsonTypeInfo<ClassWithCustomConverterFactoryNullableProperty> ClassWithCustomConverterFactoryNullableProperty { get; }
         public JsonTypeInfo<ClassWithBadCustomConverter> ClassWithBadCustomConverter { get; }
         public JsonTypeInfo<StructWithBadCustomConverter> StructWithBadCustomConverter { get; }
         public JsonTypeInfo<PersonStruct?> NullablePersonStruct { get; }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MetadataAndSerializationContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MetadataAndSerializationContextTests.cs
@@ -45,6 +45,8 @@ namespace System.Text.Json.SourceGeneration.Tests
     [JsonSerializable(typeof(StructWithCustomConverterProperty))]
     [JsonSerializable(typeof(ClassWithCustomConverterFactoryProperty))]
     [JsonSerializable(typeof(StructWithCustomConverterFactoryProperty))]
+    [JsonSerializable(typeof(ClassWithCustomConverterNullableProperty))]
+    [JsonSerializable(typeof(ClassWithCustomConverterFactoryNullableProperty))]
     [JsonSerializable(typeof(ClassWithBadCustomConverter))]
     [JsonSerializable(typeof(StructWithBadCustomConverter))]
     [JsonSerializable(typeof(PersonStruct?))]
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MetadataContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MetadataContextTests.cs
@@ -44,6 +44,8 @@ namespace System.Text.Json.SourceGeneration.Tests
     [JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
     [JsonSerializable(typeof(ClassWithCustomConverterFactoryProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
     [JsonSerializable(typeof(StructWithCustomConverterFactoryProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
+    [JsonSerializable(typeof(ClassWithCustomConverterNullableProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
+    [JsonSerializable(typeof(ClassWithCustomConverterFactoryNullableProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
     [JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
     [JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
     [JsonSerializable(typeof(PersonStruct?), GenerationMode = JsonSourceGenerationMode.Metadata)]
@@ -144,6 +146,8 @@ public override void EnsureFastPathGeneratedAsExpected()
     [JsonSerializable(typeof(StructWithCustomConverterProperty))]
     [JsonSerializable(typeof(ClassWithCustomConverterFactoryProperty))]
     [JsonSerializable(typeof(StructWithCustomConverterFactoryProperty))]
+    [JsonSerializable(typeof(ClassWithCustomConverterNullableProperty))]
+    [JsonSerializable(typeof(ClassWithCustomConverterFactoryNullableProperty))]
     [JsonSerializable(typeof(ClassWithBadCustomConverter))]
     [JsonSerializable(typeof(StructWithBadCustomConverter))]
     [JsonSerializable(typeof(PersonStruct?))]
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MixedModeContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MixedModeContextTests.cs
@@ -45,6 +45,8 @@ namespace System.Text.Json.SourceGeneration.Tests
     [JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
     [JsonSerializable(typeof(ClassWithCustomConverterFactoryProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
     [JsonSerializable(typeof(StructWithCustomConverterFactoryProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
+    [JsonSerializable(typeof(ClassWithCustomConverterNullableProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
+    [JsonSerializable(typeof(ClassWithCustomConverterFactoryNullableProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
     [JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
     [JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
     [JsonSerializable(typeof(PersonStruct?), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs
@@ -6,7 +6,6 @@
 using System.Linq;
 using System.Text.Json.Serialization;
 using System.Text.Json.Serialization.Metadata;
-using System.Text.Json.Serialization.Tests;
 using Xunit;
 
 namespace System.Text.Json.SourceGeneration.Tests
@@ -257,6 +256,60 @@ public virtual void RoundtripWithCustomConverterProperty_Class()
             Assert.Equal(42, obj.Property.Value);
         }
 
+        [Fact]
+        public virtual void RoundTripWithCustomConverterNullableProperty()
+        {
+            const string Json = "{\"TimeSpan\":42}";
+
+            var obj = new ClassWithCustomConverterNullableProperty
+            {
+                TimeSpan = TimeSpan.FromSeconds(42)
+            };
+
+            // Types with properties in custom converters do not support fast path serialization.
+            Assert.True(DefaultContext.ClassWithCustomConverterNullableProperty.SerializeHandler is null);
+
+            if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization)
+            {
+                Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterNullableProperty));
+            }
+            else
+            {
+                string json = JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterNullableProperty);
+                Assert.Equal(Json, json);
+
+                obj = JsonSerializer.Deserialize(Json, DefaultContext.ClassWithCustomConverterNullableProperty);
+                Assert.Equal(42, obj.TimeSpan.Value.TotalSeconds);
+            }
+        }
+
+        [Fact]
+        public virtual void RoundTripWithCustomConverterFactoryNullableProperty()
+        {
+            const string Json = "{\"MyEnum\":\"Two\"}";
+
+            var obj = new ClassWithCustomConverterFactoryNullableProperty
+            {
+                MyEnum = SourceGenSampleEnum.Two
+            };
+
+            // Types with properties in custom converters do not support fast path serialization.
+            Assert.True(DefaultContext.ClassWithCustomConverterFactoryNullableProperty.SerializeHandler is null);
+
+            if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization)
+            {
+                Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterFactoryNullableProperty));
+            }
+            else
+            {
+                string json = JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterFactoryNullableProperty);
+                Assert.Equal(Json, json);
+
+                obj = JsonSerializer.Deserialize(Json, DefaultContext.ClassWithCustomConverterFactoryNullableProperty);
+                Assert.Equal(SourceGenSampleEnum.Two, obj.MyEnum.Value);
+            }
+        }
+
         [Fact]
         public virtual void RoundtripWithCustomConverterProperty_Struct()
         {
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/SerializationContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/SerializationContextTests.cs
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.CustomConverters.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.CustomConverters.cs
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
