Fix support for non-public default constructors using JsonIncludeAttr…

…ibute (#90612) * Fix support for non-public constructors using JsonIncludeAttribute * Address feedback.
dotnet · Aug 16, 2023 · 227e882 · 227e882
1 parent c7cb08c
commit 227e882
Show file tree

Hide file tree

Showing 8 changed files with 95 additions and 30 deletions.
diff --git a/...t.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs b/...t.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs
@@ -49,7 +49,7 @@ private static JsonTypeInfo CreateTypeInfoCore(Type type, JsonConverter converte
             typeInfo.PopulatePolymorphismMetadata();
             typeInfo.MapInterfaceTypesToCallbacks();
 
-            Func<object>? createObject = MemberAccessor.CreateConstructor(typeInfo.Type);
+            Func<object>? createObject = DetermineCreateObjectDelegate(type, converter);
             typeInfo.SetCreateObjectIfCompatible(createObject);
             typeInfo.CreateObjectForExtensionDataProperty = createObject;
 
@@ -411,5 +411,24 @@ internal static void DeterminePropertyAccessors<T>(JsonPropertyInfo<T> jsonPrope
                     break;
             }
         }
+
+        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+        [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
+        private static Func<object>? DetermineCreateObjectDelegate(Type type, JsonConverter converter)
+        {
+            ConstructorInfo? defaultCtor = null;
+
+            if (converter.ConstructorInfo != null && !converter.ConstructorIsParameterized)
+            {
+                // A parameterless constructor has been resolved by the converter
+                // (e.g. it might be a non-public ctor with JsonConverterAttribute).
+                defaultCtor = converter.ConstructorInfo;
+            }
+
+            // Fall back to resolving any public constructors on the type.
+            defaultCtor ??= type.GetConstructor(BindingFlags.Public | BindingFlags.Instance, binder: null, Type.EmptyTypes, modifiers: null);
+
+            return MemberAccessor.CreateParameterlessConstructor(type, defaultCtor);
+        }
     }
 }
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/MemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/MemberAccessor.cs
@@ -9,8 +9,9 @@ namespace System.Text.Json.Serialization.Metadata
 {
     internal abstract class MemberAccessor
     {
-        public abstract Func<object>? CreateConstructor(
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type classType);
+        public abstract Func<object>? CreateParameterlessConstructor(
+             [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type,
+             ConstructorInfo? constructorInfo);
 
         public abstract Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor);
 

diff --git a/...t.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs b/...t.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs
@@ -21,12 +21,12 @@ internal sealed partial class ReflectionEmitCachingMemberAccessor : MemberAccess
             => s_cache.GetOrAdd((nameof(CreateAddMethodDelegate), typeof(TCollection), null),
                 static (_) => s_sourceAccessor.CreateAddMethodDelegate<TCollection>());
 
-        public override Func<object>? CreateConstructor([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type classType)
-            => s_cache.GetOrAdd((nameof(CreateConstructor), classType, null),
+        public override Func<object>? CreateParameterlessConstructor([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type, ConstructorInfo? ctorInfo)
+            => s_cache.GetOrAdd((nameof(CreateParameterlessConstructor), type, ctorInfo),
                 [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2077:UnrecognizedReflectionPattern",
                     Justification = "Cannot apply DynamicallyAccessedMembersAttribute to tuple properties.")]
 #pragma warning disable IL2077 // The suppression doesn't work for the trim analyzer: https://github.com/dotnet/roslyn/issues/59746
-                static (key) => s_sourceAccessor.CreateConstructor(key.declaringType));
+                static (key) => s_sourceAccessor.CreateParameterlessConstructor(key.declaringType, (ConstructorInfo?)key.member));
 #pragma warning restore IL2077
 
         public override Func<object, TProperty> CreateFieldGetter<TProperty>(FieldInfo fieldInfo)

diff --git a/...tem.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs b/...tem.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs
@@ -13,18 +13,19 @@ namespace System.Text.Json.Serialization.Metadata
     [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
     internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
     {
-        public override Func<object>? CreateConstructor(
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type)
+        public override Func<object>? CreateParameterlessConstructor(
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type,
+            ConstructorInfo? constructorInfo)
         {
             Debug.Assert(type != null);
-            ConstructorInfo? realMethod = type.GetConstructor(BindingFlags.Public | BindingFlags.Instance, binder: null, Type.EmptyTypes, modifiers: null);
+            Debug.Assert(constructorInfo is null || constructorInfo.GetParameters().Length == 0);
 
             if (type.IsAbstract)
             {
                 return null;
             }
 
-            if (realMethod == null && !type.IsValueType)
+            if (constructorInfo is null && !type.IsValueType)
             {
                 return null;
             }
@@ -38,8 +39,10 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
 
             ILGenerator generator = dynamicMethod.GetILGenerator();
 
-            if (realMethod == null)
+            if (constructorInfo is null)
             {
+                Debug.Assert(type.IsValueType);
+
                 LocalBuilder local = generator.DeclareLocal(type);
 
                 generator.Emit(OpCodes.Ldloca_S, local);
@@ -49,7 +52,7 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
             }
             else
             {
-                generator.Emit(OpCodes.Newobj, realMethod);
+                generator.Emit(OpCodes.Newobj, constructorInfo);
                 if (type.IsValueType)
                 {
                     // Since C# 10 it's now possible to have parameterless constructors in structs

diff --git a/.../System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs b/.../System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs
@@ -10,35 +10,26 @@ namespace System.Text.Json.Serialization.Metadata
 {
     internal sealed class ReflectionMemberAccessor : MemberAccessor
     {
-        private sealed class ConstructorContext
-        {
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]
-            private readonly Type _type;
-
-            public ConstructorContext([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type)
-                => _type = type;
-
-            public object? CreateInstance()
-                => Activator.CreateInstance(_type, nonPublic: false);
-        }
-
-        public override Func<object>? CreateConstructor(
-            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type)
+        public override Func<object>? CreateParameterlessConstructor(
+            [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type,
+            ConstructorInfo? ctorInfo)
         {
             Debug.Assert(type != null);
-            ConstructorInfo? realMethod = type.GetConstructor(BindingFlags.Public | BindingFlags.Instance, binder: null, Type.EmptyTypes, modifiers: null);
+            Debug.Assert(ctorInfo is null || ctorInfo.GetParameters().Length == 0);
 
             if (type.IsAbstract)
             {
                 return null;
             }
 
-            if (realMethod == null && !type.IsValueType)
+            if (ctorInfo is null)
             {
-                return null;
+                return type.IsValueType
+                    ? () => Activator.CreateInstance(type, nonPublic: false)!
+                    : null;
             }
 
-            return new ConstructorContext(type).CreateInstance!;
+            return () => ctorInfo.Invoke(null);
         }
 
         public override Func<object[], T> CreateParameterizedConstructor<T>(ConstructorInfo constructor)

diff --git a/...ries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.AttributePresence.cs b/...ries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.AttributePresence.cs
@@ -39,6 +39,24 @@ public async Task NonPublicCtors_WithJsonConstructorAttribute_WorksAsExpected(Ty
             }
         }
 
+        [Theory]
+        [InlineData(typeof(PrivateParameterlessCtor_WithAttribute), false)]
+        [InlineData(typeof(InternalParameterlessCtor_WithAttribute), true)]
+        [InlineData(typeof(ProtectedParameterlessCtor_WithAttribute), false)]
+        public async Task NonPublicParameterlessCtors_WithJsonConstructorAttribute_WorksAsExpected(Type type, bool isAccessibleBySourceGen)
+        {
+            if (!Serializer.IsSourceGeneratedSerializer || isAccessibleBySourceGen)
+            {
+                object? result = await Serializer.DeserializeWrapper("{}", type);
+                Assert.IsType(type, result);
+            }
+            else
+            {
+                NotSupportedException ex = await Assert.ThrowsAsync<NotSupportedException>(() => Serializer.DeserializeWrapper("{}", type));
+                Assert.Contains("JsonConstructorAttribute", ex.ToString());
+            }
+        }
+
         [Fact]
         public async Task SinglePublicParameterizedCtor_SingleParameterlessCtor_NoAttribute_Supported_UseParameterlessCtor()
         {

diff --git a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs
@@ -74,6 +74,33 @@ public class ProtectedParameterizedCtor_WithAttribute
         protected ProtectedParameterizedCtor_WithAttribute(int x) => X = x;
     }
 
+    public class PrivateParameterlessCtor_WithAttribute
+    {
+        public int X { get; }
+
+        [JsonConstructor]
+        private PrivateParameterlessCtor_WithAttribute()
+            => X = 42;
+    }
+
+    public class ProtectedParameterlessCtor_WithAttribute
+    {
+        public int X { get; }
+
+        [JsonConstructor]
+        protected ProtectedParameterlessCtor_WithAttribute()
+            => X = 42;
+    }
+
+    public class InternalParameterlessCtor_WithAttribute
+    {
+        public int X { get; }
+
+        [JsonConstructor]
+        internal InternalParameterlessCtor_WithAttribute()
+            => X = 42;
+    }
+
     public class PrivateParameterlessCtor_InternalParameterizedCtor_WithMultipleAttributes
     {
         [JsonConstructor]

diff --git a/...Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/...Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs
@@ -41,6 +41,9 @@ protected ConstructorTests_Metadata(JsonSerializerWrapper stringWrapper)
         [JsonSerializable(typeof(PrivateParameterizedCtor_WithAttribute))]
         [JsonSerializable(typeof(InternalParameterizedCtor_WithAttribute))]
         [JsonSerializable(typeof(ProtectedParameterizedCtor_WithAttribute))]
+        [JsonSerializable(typeof(PrivateParameterlessCtor_WithAttribute))]
+        [JsonSerializable(typeof(InternalParameterlessCtor_WithAttribute))]
+        [JsonSerializable(typeof(ProtectedParameterlessCtor_WithAttribute))]
         [JsonSerializable(typeof(SinglePublicParameterizedCtor))]
         [JsonSerializable(typeof(SingleParameterlessCtor_MultiplePublicParameterizedCtor))]
         [JsonSerializable(typeof(SingleParameterlessCtor_MultiplePublicParameterizedCtor_Struct))]
@@ -186,6 +189,9 @@ public ConstructorTests_Default(JsonSerializerWrapper jsonSerializer) : base(jso
         [JsonSerializable(typeof(PrivateParameterizedCtor_WithAttribute))]
         [JsonSerializable(typeof(InternalParameterizedCtor_WithAttribute))]
         [JsonSerializable(typeof(ProtectedParameterizedCtor_WithAttribute))]
+        [JsonSerializable(typeof(PrivateParameterlessCtor_WithAttribute))]
+        [JsonSerializable(typeof(InternalParameterlessCtor_WithAttribute))]
+        [JsonSerializable(typeof(ProtectedParameterlessCtor_WithAttribute))]
         [JsonSerializable(typeof(SinglePublicParameterizedCtor))]
         [JsonSerializable(typeof(SingleParameterlessCtor_MultiplePublicParameterizedCtor))]
         [JsonSerializable(typeof(SingleParameterlessCtor_MultiplePublicParameterizedCtor_Struct))]