From 8ac112de5f7fd65bc6d28471edfe96fe89ab2cd7 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 15 Aug 2023 18:12:06 +0100 Subject: [PATCH 1/2] Fix support for non-public constructors using JsonIncludeAttribute --- .../DefaultJsonTypeInfoResolver.Helpers.cs | 20 +++++++++++++- .../Serialization/Metadata/MemberAccessor.cs | 5 ++-- .../ReflectionEmitCachingMemberAccessor.cs | 6 ++--- .../Metadata/ReflectionEmitMemberAccessor.cs | 13 ++++----- .../Metadata/ReflectionMemberAccessor.cs | 27 +++++++------------ .../ConstructorTests.AttributePresence.cs | 18 +++++++++++++ .../TestClasses/TestClasses.Constructor.cs | 27 +++++++++++++++++++ .../Serialization/ConstructorTests.cs | 6 +++++ 8 files changed, 92 insertions(+), 30 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs index 7ec9db9adc773..6649472aac650 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs +++ b/src/libraries/System.Text.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? createObject = MemberAccessor.CreateConstructor(typeInfo.Type); + Func? createObject = DetermineCreateObjectDelegate(type, converter); typeInfo.SetCreateObjectIfCompatible(createObject); typeInfo.CreateObjectForExtensionDataProperty = createObject; @@ -411,5 +411,23 @@ internal static void DeterminePropertyAccessors(JsonPropertyInfo jsonPrope break; } } + + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] + [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] + private static Func? 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 index 326ab657b8899..ff6c442fa488c 100644 --- 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? CreateConstructor( - [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type classType); + public abstract Func? CreateParameterlessConstructor( + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type, + ConstructorInfo? constructorInfo); public abstract Func CreateParameterizedConstructor(ConstructorInfo constructor); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs index a243f4be4d86a..e30f87d76da9f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs +++ b/src/libraries/System.Text.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()); - public override Func? CreateConstructor([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type classType) - => s_cache.GetOrAdd((nameof(CreateConstructor), classType, null), + public override Func? 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 CreateFieldGetter(FieldInfo fieldInfo) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs index 6e05e5c8057ac..6b4c816d25118 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.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? CreateConstructor( - [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type) + public override Func? 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,7 +39,7 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor ILGenerator generator = dynamicMethod.GetILGenerator(); - if (realMethod == null) + if (constructorInfo is null) { LocalBuilder local = generator.DeclareLocal(type); @@ -49,7 +50,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/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs index 606ad6aba79c2..8627a24f3f492 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs +++ b/src/libraries/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? CreateConstructor( - [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type type) + public override Func? 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 CreateParameterizedConstructor(ConstructorInfo constructor) diff --git a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.AttributePresence.cs b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.AttributePresence.cs index 681d495b8c752..f54692a5c59a4 100644 --- a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.AttributePresence.cs +++ b/src/libraries/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(() => 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 index 285c5b7893473..f12cf89e41de5 100644 --- 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/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs index b2a5fd465da76..d1769f491104d 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs +++ b/src/libraries/System.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))] From 133bba6064964e98062d8ea44dd5499f2601518a Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 15 Aug 2023 18:32:37 +0100 Subject: [PATCH 2/2] Address feedback. --- .../Metadata/DefaultJsonTypeInfoResolver.Helpers.cs | 1 + .../Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs index 6649472aac650..8ee9f3db0283b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs @@ -427,6 +427,7 @@ internal static void DeterminePropertyAccessors(JsonPropertyInfo jsonPrope // 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/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs index 6b4c816d25118..4b0b426bdaa9e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs @@ -41,6 +41,8 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor if (constructorInfo is null) { + Debug.Assert(type.IsValueType); + LocalBuilder local = generator.DeclareLocal(type); generator.Emit(OpCodes.Ldloca_S, local);