Prevent runtime prop metadata retrieval when [JsonIgnore] is used (#6…

…0024)

* Prevent runtime prop metadata retrieval when [JsonIgnore] is used

* Address feedback

* Re-add ctor param default handling tests

* Remove new concurrent dictionary for generic method holders

src/libraries/System.Text.Json/Common/ReflectionExtensions.cs

@@ -310,5 +310,18 @@ public static bool TryGetDeserializationConstructor(
             deserializationCtor = ctorWithAttribute ?? publicParameterlessCtor ?? lonePublicCtor;
             return true;
         }
+
+        public static object? GetDefaultValue(this ParameterInfo parameterInfo)
+        {
+            object? defaultValue = parameterInfo.DefaultValue;
+
+            // DBNull.Value is sometimes used as the default value (returned by reflection) of nullable params in place of null.
+            if (defaultValue == DBNull.Value && parameterInfo.ParameterType != typeof(DBNull))
+            {
+                return null;
+            }
+
+            return defaultValue;
+        }
     }
 }

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

@@ -804,14 +804,19 @@ private string GenerateCtorParamMetadataInitFunc(TypeGenerationSpec typeGenerati
                 {
                     ParameterInfo reflectionInfo = parameters[i].ParameterInfo;
 
+                    string parameterTypeRef = reflectionInfo.ParameterType.GetCompilableName();
+
+                    object? defaultValue = reflectionInfo.GetDefaultValue();
+                    string defaultValueAsStr = GetParamDefaultValueAsString(defaultValue, parameterTypeRef);
+
                     sb.Append(@$"
     {InfoVarName} = new()
     {{
         Name = ""{reflectionInfo.Name!}"",
-        ParameterType = typeof({reflectionInfo.ParameterType.GetCompilableName()}),
+        ParameterType = typeof({parameterTypeRef}),
         Position = {reflectionInfo.Position},
         HasDefaultValue = {ToCSharpKeyword(reflectionInfo.HasDefaultValue)},
-        DefaultValue = {GetParamDefaultValueAsString(reflectionInfo.DefaultValue)}
+        DefaultValue = {defaultValueAsStr}
     }};
     {parametersVarName}[{i}] = {InfoVarName};
 ");
@@ -1313,12 +1318,12 @@ private static string GetNumberHandlingAsStr(JsonNumberHandling? numberHandling)
 
         private static string ToCSharpKeyword(bool value) => value.ToString().ToLowerInvariant();
 
-        private static string GetParamDefaultValueAsString(object? value)
+        private static string GetParamDefaultValueAsString(object? value, string objectTypeAsStr)
         {
             switch (value)
             {
                 case null:
-                    return "null";
+                    return $"default({objectTypeAsStr})";
                 case bool boolVal:
                     return ToCSharpKeyword(boolVal);
                 default:

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

@@ -26,6 +26,7 @@ private sealed class Parser
             private const string SystemTextJsonNamespace = "System.Text.Json";
             private const string JsonConverterAttributeFullName = "System.Text.Json.Serialization.JsonConverterAttribute";
             private const string JsonConverterFactoryFullName = "System.Text.Json.Serialization.JsonConverterFactory";
+            private const string JsonConverterOfTFullName = "System.Text.Json.Serialization.JsonConverter`1";
             private const string JsonArrayFullName = "System.Text.Json.Nodes.JsonArray";
             private const string JsonElementFullName = "System.Text.Json.JsonElement";
             private const string JsonExtensionDataAttributeFullName = "System.Text.Json.Serialization.JsonExtensionDataAttribute";
@@ -101,6 +102,9 @@ private sealed class Parser
             private readonly Type? _dateOnlyType;
             private readonly Type? _timeOnlyType;
 
+            // Needed for converter validation
+            private readonly Type _jsonConverterOfTType;
+
             private readonly HashSet<Type> _numberTypes = new();
             private readonly HashSet<Type> _knownTypes = new();
             private readonly HashSet<Type> _knownUnsupportedTypes = new();
@@ -215,6 +219,8 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
                 _dateOnlyType = _metadataLoadContext.Resolve(DateOnlyFullName);
                 _timeOnlyType = _metadataLoadContext.Resolve(TimeOnlyFullName);
 
+                _jsonConverterOfTType = _metadataLoadContext.Resolve(JsonConverterOfTFullName);
+
                 PopulateKnownTypes();
             }
 
@@ -224,8 +230,12 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
                 INamedTypeSymbol jsonSerializerContextSymbol = compilation.GetBestTypeByMetadataName(JsonSerializerContextFullName);
                 INamedTypeSymbol jsonSerializableAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSerializerAttributeFullName);
                 INamedTypeSymbol jsonSourceGenerationOptionsAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSourceGenerationOptionsAttributeFullName);
+                INamedTypeSymbol jsonConverterOfTAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonConverterOfTFullName);
 
-                if (jsonSerializerContextSymbol == null || jsonSerializableAttributeSymbol == null || jsonSourceGenerationOptionsAttributeSymbol == null)
+                if (jsonSerializerContextSymbol == null ||
+                    jsonSerializableAttributeSymbol == null ||
+                    jsonSourceGenerationOptionsAttributeSymbol == null ||
+                    jsonConverterOfTAttributeSymbol == null)
                 {
                     return null;
                 }
@@ -1354,7 +1364,14 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor)
                     return null;
                 }
 
-                if (converterType.GetCompatibleBaseClass(JsonConverterFactoryFullName) != null)
+                // Validated when creating the source generation spec.
+                Debug.Assert(_jsonConverterOfTType != null);
+
+                if (converterType.GetCompatibleGenericBaseClass(_jsonConverterOfTType) != null)
+                {
+                    return $"new {converterType.GetCompilableName()}()";
+                }
+                else if (converterType.GetCompatibleBaseClass(JsonConverterFactoryFullName) != null)
                 {
                     hasFactoryConverter = true;
 
@@ -1368,7 +1385,7 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor)
                     }
                 }
 
-                return $"new {converterType.GetCompilableName()}()";
+                return null;
             }
 
             private static string DetermineRuntimePropName(string clrPropName, string? jsonPropName, JsonKnownNamingPolicy namingPolicy)

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs

@@ -67,10 +67,7 @@ protected sealed override void InitializeConstructorArgumentCaches(ref ReadStack
             {
                 JsonParameterInfo? parameterInfo = cache[i].Value;
                 Debug.Assert(parameterInfo != null);
-
-                arguments[parameterInfo.ClrInfo.Position] = parameterInfo.ShouldDeserialize
-                    ? parameterInfo.DefaultValue
-                    : parameterInfo.ClrDefaultValue;
+                arguments[parameterInfo.ClrInfo.Position] = parameterInfo.DefaultValue;
             }
 
             state.Current.CtorArgumentState!.Arguments = arguments;

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

@@ -600,9 +600,7 @@ internal JsonTypeInfo GetOrAddClass(Type type)
         {
             _haveTypesBeenCreated = true;
 
-            // todo: for performance and reduced instances, consider using the converters and JsonTypeInfo from s_defaultOptions by cloning (or reference directly if no changes).
-            // https://github.com/dotnet/runtime/issues/32357
-            if (!_classes.TryGetValue(type, out JsonTypeInfo? result))
+            if (!TryGetClass(type, out JsonTypeInfo? result))
             {
                 result = _classes.GetOrAdd(type, GetClassFromContextOrCreate(type));
             }
@@ -644,6 +642,20 @@ internal JsonTypeInfo GetOrAddClassForRootType(Type type)
             return jsonTypeInfo;
         }
 
+        internal bool TryGetClass(Type type, [NotNullWhen(true)] out JsonTypeInfo? jsonTypeInfo)
+        {
+            // todo: for performance and reduced instances, consider using the converters and JsonTypeInfo from s_defaultOptions by cloning (or reference directly if no changes).
+            // https://github.com/dotnet/runtime/issues/32357
+            if (!_classes.TryGetValue(type, out JsonTypeInfo? result))
+            {
+                jsonTypeInfo = null;
+                return false;
+            }
+
+            jsonTypeInfo = result;
+            return true;
+        }
+
         internal bool TypeIsCached(Type type)
         {
             return _classes.ContainsKey(type);

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Text.Json.Reflection;
@@ -12,17 +13,33 @@ namespace System.Text.Json.Serialization.Metadata
     /// </summary>
     internal abstract class GenericMethodHolder
     {
+        /// <summary>
+        /// Returns the default value for the specified type.
+        /// </summary>
+        public abstract object? DefaultValue { get; }
+
         /// <summary>
         /// Returns true if <param name="value"/> contains only default values.
         /// </summary>
         public abstract bool IsDefaultValue(object value);
+
+        /// <summary>
+        /// Creates a holder instance representing a type.
+        /// </summary>
+        public static GenericMethodHolder CreateHolder(Type type)
+        {
+            Type holderType = typeof(GenericMethodHolder<>).MakeGenericType(type);
+            return (GenericMethodHolder)Activator.CreateInstance(holderType)!;
+        }
     }
 
     /// <summary>
     /// Generic methods for {T}.
     /// </summary>
     internal sealed class GenericMethodHolder<T> : GenericMethodHolder
     {
+        public override object? DefaultValue => default(T);
+
         public override bool IsDefaultValue(object value)
         {
             // For performance, we only want to call this method for non-nullable value types.

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs

@@ -17,8 +17,6 @@ internal abstract class JsonParameterInfo
 
         private protected bool MatchingPropertyCanBeNull { get; private set; }
 
-        internal abstract object? ClrDefaultValue { get; }
-
         // The default value of the parameter. This is `DefaultValue` of the `ParameterInfo`, if specified, or the CLR `default` for the `ParameterType`.
         public object? DefaultValue { get; private protected set; }
 
@@ -74,18 +72,49 @@ public virtual void Initialize(JsonParameterInfoValues parameterInfo, JsonProper
             MatchingPropertyCanBeNull = matchingProperty.PropertyTypeCanBeNull;
         }
 
-        // Create a parameter that is ignored at run time. It uses the same type (typeof(sbyte)) to help
-        // prevent issues with unsupported types and helps ensure we don't accidently (de)serialize it.
-        public static JsonParameterInfo CreateIgnoredParameterPlaceholder(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty)
+        /// <summary>
+        /// Create a parameter that is ignored at run time. It uses the same type (typeof(sbyte)) to help
+        /// prevent issues with unsupported types and helps ensure we don't accidently (de)serialize it.
+        /// </summary>
+        public static JsonParameterInfo CreateIgnoredParameterPlaceholder(
+            JsonParameterInfoValues parameterInfo,
+            JsonPropertyInfo matchingProperty,
+            bool sourceGenMode)
         {
-            JsonParameterInfo jsonParameterInfo = matchingProperty.ConverterBase.CreateJsonParameterInfo();
+            JsonParameterInfo jsonParameterInfo = new JsonParameterInfo<sbyte>();
             jsonParameterInfo.ClrInfo = parameterInfo;
             jsonParameterInfo.RuntimePropertyType = matchingProperty.RuntimePropertyType!;
             jsonParameterInfo.NameAsUtf8Bytes = matchingProperty.NameAsUtf8Bytes!;
-            jsonParameterInfo.InitializeDefaultValue(matchingProperty);
+
+            // TODO: https://github.com/dotnet/runtime/issues/60082.
+            // Default value initialization for params mapping to ignored properties doesn't
+            // account for the default value of optional parameters. This should be fixed.
+
+            if (sourceGenMode)
+            {
+                // The <T> value in the matching JsonPropertyInfo<T> instance matches the parameter type.
+                jsonParameterInfo.DefaultValue = matchingProperty.DefaultValue;
+            }
+            else
+            {
+                // The <T> value in the created JsonPropertyInfo<T> instance (sbyte)
+                // doesn't match the parameter type, use reflection to get the default value.
+                Type parameterType = parameterInfo.ParameterType;
+
+                GenericMethodHolder holder;
+                if (matchingProperty.Options.TryGetClass(parameterType, out JsonTypeInfo? typeInfo))
+                {
+                    holder = typeInfo.GenericMethods;
+                }
+                else
+                {
+                    holder = GenericMethodHolder.CreateHolder(parameterInfo.ParameterType);
+                }
+
+                jsonParameterInfo.DefaultValue = holder.DefaultValue;
+            }
+
             return jsonParameterInfo;
         }
-
-        protected abstract void InitializeDefaultValue(JsonPropertyInfo matchingProperty);
     }
 }

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfoOfT.cs

@@ -11,8 +11,6 @@ namespace System.Text.Json.Serialization.Metadata
     /// </summary>
     internal sealed class JsonParameterInfo<T> : JsonParameterInfo
     {
-        internal override object? ClrDefaultValue => default(T);
-
         public T TypedDefaultValue { get; private set; } = default!;
 
         public override void Initialize(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty, JsonSerializerOptions options)
@@ -21,7 +19,7 @@ public override void Initialize(JsonParameterInfoValues parameterInfo, JsonPrope
             InitializeDefaultValue(matchingProperty);
         }
 
-        protected override void InitializeDefaultValue(JsonPropertyInfo matchingProperty)
+        private void InitializeDefaultValue(JsonPropertyInfo matchingProperty)
         {
             Debug.Assert(ClrInfo.ParameterType == matchingProperty.DeclaredPropertyType);
 

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs

@@ -42,16 +42,14 @@ internal static JsonPropertyInfo GetPropertyPlaceholder()
 
         // Create a property that is ignored at run-time.
         internal static JsonPropertyInfo CreateIgnoredPropertyPlaceholder(
-            JsonConverter converter,
             MemberInfo memberInfo,
             Type memberType,
             bool isVirtual,
             JsonSerializerOptions options)
         {
-            JsonPropertyInfo jsonPropertyInfo = converter.CreateJsonPropertyInfo();
+            JsonPropertyInfo jsonPropertyInfo = new JsonPropertyInfo<sbyte>();
 
             jsonPropertyInfo.Options = options;
-            jsonPropertyInfo.ConverterBase = converter;
             jsonPropertyInfo.MemberInfo = memberInfo;
             jsonPropertyInfo.IsIgnored = true;
             jsonPropertyInfo.DeclaredPropertyType = memberType;
@@ -525,5 +523,10 @@ internal JsonTypeInfo RuntimeTypeInfo
         internal string? ClrName { get; set; }
 
         internal bool IsVirtual { get; set; }
+
+        /// <summary>
+        /// Default value used for parameterized ctor invocation.
+        /// </summary>
+        internal abstract object? DefaultValue { get; }
     }
 }

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

@@ -28,8 +28,11 @@ internal sealed class JsonPropertyInfo<T> : JsonPropertyInfo
         private bool _propertyTypeEqualsTypeToConvert;
 
         internal Func<object, T>? Get { get; set; }
+
         internal Action<object, T>? Set { get; set; }
 
+        internal override object? DefaultValue => default(T);
+
         public JsonConverter<T> Converter { get; internal set; } = null!;
 
         internal override void Initialize(

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs

@@ -58,19 +58,19 @@ internal static JsonPropertyInfo AddProperty(
             JsonNumberHandling? parentTypeNumberHandling,
             JsonSerializerOptions options)
         {
+            JsonIgnoreCondition? ignoreCondition = JsonPropertyInfo.GetAttribute<JsonIgnoreAttribute>(memberInfo)?.Condition;
+            if (ignoreCondition == JsonIgnoreCondition.Always)
+            {
+                return JsonPropertyInfo.CreateIgnoredPropertyPlaceholder(memberInfo, memberType, isVirtual, options);
+            }
+
             JsonConverter converter = GetConverter(
                 memberType,
                 parentClassType,
                 memberInfo,
                 out Type runtimeType,
                 options);
 
-            JsonIgnoreCondition? ignoreCondition = JsonPropertyInfo.GetAttribute<JsonIgnoreAttribute>(memberInfo)?.Condition;
-            if (ignoreCondition == JsonIgnoreCondition.Always)
-            {
-                return JsonPropertyInfo.CreateIgnoredPropertyPlaceholder(converter, memberInfo, memberType, isVirtual, options);
-            }
-
             return CreateProperty(
                 declaredPropertyType: memberType,
                 runtimePropertyType: runtimeType,
@@ -646,7 +646,7 @@ internal void InitializeParameterCache()
                 return;
             }
 
-            InitializeConstructorParameters(array);
+            InitializeConstructorParameters(array, sourceGenMode: true);
             Debug.Assert(ParameterCache != null);
         }
     }