address feedback

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/CastingConverter.cs

@@ -32,43 +32,39 @@ internal CastingConverter(JsonConverter<SourceType> sourceConverter) : base(init
         }
 
         public override TargetType? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
-            => ConvertToTarget(_sourceConverter.Read(ref reader, typeToConvert, options));
+            => Cast<SourceType, TargetType>(_sourceConverter.Read(ref reader, typeToConvert, options));
 
         public override void Write(Utf8JsonWriter writer, TargetType value, JsonSerializerOptions options)
-            => _sourceConverter.Write(writer, ConvertToSource(value), options);
+            => _sourceConverter.Write(writer, Cast<TargetType, SourceType>(value), options);
 
         internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out TargetType? value)
         {
             bool ret = _sourceConverter.OnTryRead(ref reader, typeToConvert, options, ref state, out SourceType? sourceValue);
-            value = ConvertToTarget(sourceValue);
+            value = Cast<SourceType, TargetType>(sourceValue);
             return ret;
         }
 
         internal override bool OnTryWrite(Utf8JsonWriter writer, TargetType value, JsonSerializerOptions options, ref WriteStack state)
-            => _sourceConverter.OnTryWrite(writer, ConvertToSource(value), options, ref state);
+            => _sourceConverter.OnTryWrite(writer, Cast<TargetType, SourceType>(value), options, ref state);
 
         public override TargetType ReadAsPropertyName(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
-            => ConvertToTarget(_sourceConverter.ReadAsPropertyName(ref reader, typeToConvert, options));
+            => Cast<SourceType, TargetType>(_sourceConverter.ReadAsPropertyName(ref reader, typeToConvert, options));
 
         internal override TargetType ReadAsPropertyNameCore(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
-            => ConvertToTarget(_sourceConverter.ReadAsPropertyNameCore(ref reader, typeToConvert, options));
+            => Cast<SourceType, TargetType>(_sourceConverter.ReadAsPropertyNameCore(ref reader, typeToConvert, options));
 
         public override void WriteAsPropertyName(Utf8JsonWriter writer, TargetType value, JsonSerializerOptions options)
-            => _sourceConverter.WriteAsPropertyName(writer, ConvertToSource(value), options);
+            => _sourceConverter.WriteAsPropertyName(writer, Cast<TargetType, SourceType>(value), options);
 
         internal override void WriteAsPropertyNameCore(Utf8JsonWriter writer, TargetType value, JsonSerializerOptions options, bool isWritingExtensionDataProperty)
-            => _sourceConverter.WriteAsPropertyNameCore(writer, ConvertToSource(value), options, isWritingExtensionDataProperty);
+            => _sourceConverter.WriteAsPropertyNameCore(writer, Cast<TargetType, SourceType>(value), options, isWritingExtensionDataProperty);
 
         internal override TargetType ReadNumberWithCustomHandling(ref Utf8JsonReader reader, JsonNumberHandling handling, JsonSerializerOptions options)
-            => ConvertToTarget(_sourceConverter.ReadNumberWithCustomHandling(ref reader, handling, options));
+            => Cast<SourceType, TargetType>(_sourceConverter.ReadNumberWithCustomHandling(ref reader, handling, options));
 
         internal override void WriteNumberWithCustomHandling(Utf8JsonWriter writer, TargetType value, JsonNumberHandling handling)
-            => _sourceConverter.WriteNumberWithCustomHandling(writer, ConvertToSource(value), handling);
+            => _sourceConverter.WriteNumberWithCustomHandling(writer, Cast<TargetType, SourceType>(value), handling);
 
-        private static SourceType ConvertToSource(TargetType? targetValue)
-            => (SourceType)(object?)targetValue!;
-
-        private static TargetType ConvertToTarget(SourceType? sourceValue)
-            => (TargetType)(object?)sourceValue!;
+        private static TTarget Cast<TSource, TTarget>(TSource? source) => (TTarget)(object?)source!;
     }
 }

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

@@ -71,12 +71,7 @@ internal virtual void ReadElementAndSetProperty(
 
         internal abstract JsonParameterInfo CreateJsonParameterInfo();
 
-        internal virtual JsonConverter<TargetType> CreateCastingConverter<TargetType>()
-        {
-            // This can only happen for factory converters and we should always expand factory here.
-            ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertMultipleTypes(typeof(TargetType), this);
-            return null!;
-        }
+        internal abstract JsonConverter<TTarget> CreateCastingConverter<TTarget>();
 
         internal abstract Type? ElementType { get; }
 

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

@@ -133,5 +133,11 @@ internal sealed override void WriteAsPropertyNameCoreAsObject(
 
             throw new InvalidOperationException();
         }
+
+        internal sealed override JsonConverter<TTarget> CreateCastingConverter<TTarget>()
+        {
+            ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertMultipleTypes(typeof(TTarget), this);
+            return null!;
+        }
     }
 }

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

@@ -23,6 +23,8 @@ protected internal JsonConverter()
 
         internal JsonConverter(bool initialize)
         {
+            // Initialize uses abstract members, in order for them to be initialized correctly
+            // without throwing we need to delay call to Initialize
             if (initialize)
             {
                 Initialize();
@@ -77,9 +79,9 @@ internal sealed override JsonParameterInfo CreateJsonParameterInfo()
             return new JsonParameterInfo<T>();
         }
 
-        internal sealed override JsonConverter<TargetType> CreateCastingConverter<TargetType>()
+        internal sealed override JsonConverter<TTarget> CreateCastingConverter<TTarget>()
         {
-            return new CastingConverter<TargetType, T>(this);
+            return new CastingConverter<TTarget, T>(this);
         }
 
         internal override Type? KeyType => null;

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

@@ -27,7 +27,7 @@ public abstract class JsonPropertyInfo
         /// - for reflection we store the original value since we need it in order to construct typed JsonPropertyInfo
         /// - for source gen it remains null, we will initialize it only if someone used resolver to remove CustomConverter
         /// </summary>
-        internal JsonConverter? NonCustomConverter { get; set; }
+        internal JsonConverter? DefaultConverterForType { get; set; }
 
         /// <summary>
         /// Converter after applying CustomConverter (i.e. JsonConverterAttribute)
@@ -153,7 +153,7 @@ internal virtual void Configure(JsonTypeInfo typeInfo)
                 return;
             }
 
-            SetEffectiveConverter();
+            DetermineEffectiveConverter();
             ConverterStrategy = EffectiveConverter.ConverterStrategy;
 
             if (IsForTypeInfo)
@@ -173,7 +173,7 @@ internal virtual void Configure(JsonTypeInfo typeInfo)
             }
         }
 
-        internal abstract void SetEffectiveConverter();
+        internal abstract void DetermineEffectiveConverter();
 
         internal void GetPolicies()
         {
@@ -372,7 +372,7 @@ internal void DetermineNumberHandlingForProperty()
 
             if (numberHandlingIsApplicable)
             {
-                // Priority 1: Get handling from attribute on property/field, it's parent class type or property type.
+                // Priority 1: Get handling from attribute on property/field, its parent class type or property type.
                 JsonNumberHandling? handling = NumberHandling ?? DeclaringTypeNumberHandling ?? JsonTypeInfo.NumberHandling;
 
                 // Priority 2: Get handling from JsonSerializerOptions instance.
@@ -466,7 +466,7 @@ internal abstract void Initialize(
             JsonIgnoreCondition? ignoreCondition,
             JsonSerializerOptions options,
             JsonTypeInfo? jsonTypeInfo = null,
-            bool isCustomProperty = false);
+            bool isUserDefinedProperty = false);
 
         internal bool IgnoreDefaultValuesOnRead { get; private set; }
         internal bool IgnoreDefaultValuesOnWrite { get; private set; }
@@ -483,7 +483,7 @@ internal abstract void Initialize(
 
         /// <summary>
         /// The name of the property.
-        /// It's either the actual CLR property name,
+        /// It is either the actual .NET property name,
         /// the value specified in JsonPropertyNameAttribute,
         /// or the value returned from PropertyNamingPolicy.
         /// </summary>

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

@@ -92,7 +92,7 @@ private protected override void SetSetter(Delegate? setter)
 
         internal override object? DefaultValue => default(T);
 
-        internal JsonConverter<T> TypedEffectiveConverter { get; set; } = null!;
+        internal JsonConverter<T> TypedEffectiveConverter { get; private set; } = null!;
 
         internal override void Initialize(
             Type declaringType,
@@ -104,7 +104,7 @@ internal override void Initialize(
             JsonIgnoreCondition? ignoreCondition,
             JsonSerializerOptions options,
             JsonTypeInfo? jsonTypeInfo = null,
-            bool isCustomProperty = false)
+            bool isUserDefinedProperty = false)
         {
             Debug.Assert(converter != null);
 
@@ -117,7 +117,7 @@ internal override void Initialize(
                 JsonTypeInfo = jsonTypeInfo;
             }
 
-            NonCustomConverter = converter;
+            DefaultConverterForType = converter;
             Options = options;
             DeclaringType = declaringType;
             MemberInfo = memberInfo;
@@ -178,7 +178,7 @@ internal override void Initialize(
 
                 GetPolicies();
             }
-            else if (!isCustomProperty)
+            else if (!isUserDefinedProperty)
             {
                 IsForTypeInfo = true;
             }
@@ -210,12 +210,12 @@ internal void InitializeForSourceGen(JsonSerializerOptions options, JsonProperty
                 name = options.PropertyNamingPolicy.ConvertName(ClrName);
             }
 
+            // Compat: We need to do validation before we assign Name so that we get InvalidOperationException rather than ArgumentNullException
             if (name == null)
             {
                 ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameNull(DeclaringType, this);
             }
 
-            // Compat: We need to do validation before we assign this so that we get InvalidOperationException rather than ArgumentNullException
             Name = name;
 
             SrcGen_IsPublic = propertyInfo.IsPublic;
@@ -232,7 +232,7 @@ internal void InitializeForSourceGen(JsonSerializerOptions options, JsonProperty
             CustomConverter = typedCustomConverter;
 
             JsonConverter<T>? typedNonCustomConverter = propertyTypeInfo?.Converter as JsonConverter<T>;
-            NonCustomConverter = typedNonCustomConverter;
+            DefaultConverterForType = typedNonCustomConverter;
 
             IsIgnored = propertyInfo.IgnoreCondition == JsonIgnoreCondition.Always;
             if (!IsIgnored)
@@ -263,7 +263,7 @@ internal override void Configure(JsonTypeInfo typeInfo)
             }
         }
 
-        internal override void SetEffectiveConverter()
+        internal override void DetermineEffectiveConverter()
         {
             JsonConverter? customConverter = CustomConverter;
             if (customConverter != null)
@@ -272,7 +272,7 @@ internal override void SetEffectiveConverter()
                 JsonSerializerOptions.CheckConverterNullabilityIsSameAsPropertyType(customConverter, PropertyType);
             }
 
-            JsonConverter effectiveConverter = customConverter ?? NonCustomConverter ?? Options.GetConverterFromTypeInfo(PropertyType);
+            JsonConverter effectiveConverter = customConverter ?? DefaultConverterForType ?? Options.GetConverterFromTypeInfo(PropertyType);
             if (effectiveConverter.TypeToConvert == PropertyType)
             {
                 EffectiveConverter = effectiveConverter;
@@ -359,14 +359,11 @@ value is not null &&
                 }
             }
 
-            if (ShouldSerialize != null)
+            if (ShouldSerialize?.Invoke(obj, value) == false)
             {
-                if (!ShouldSerialize(obj, value))
-                {
-                    // We return true here.
-                    // False means that there is not enough data.
-                    return true;
-                }
+                // We return true here.
+                // False means that there is not enough data.
+                return true;
             }
 
             if (value == null)
@@ -412,14 +409,11 @@ internal override bool GetMemberAndWriteJsonExtensionData(object obj, ref WriteS
             bool success;
             T value = Get!(obj);
 
-            if (ShouldSerialize != null)
+            if (ShouldSerialize?.Invoke(obj, value) == false)
             {
-                if (!ShouldSerialize(obj, value))
-                {
-                    // We return true here.
-                    // False means that there is not enough data.
-                    return true;
-                }
+                // We return true here.
+                // False means that there is not enough data.
+                return true;
             }
 
             if (value == null)

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

@@ -61,7 +61,7 @@ internal static JsonPropertyInfo CreateProperty(
             JsonIgnoreCondition? ignoreCondition = null,
             JsonTypeInfo? jsonTypeInfo = null,
             JsonConverter? customConverter = null,
-            bool isCustomProperty = false)
+            bool isUserDefinedProperty = false)
         {
             // Create the JsonPropertyInfo instance.
             JsonPropertyInfo jsonPropertyInfo = converter.CreateJsonPropertyInfo();
@@ -76,7 +76,7 @@ internal static JsonPropertyInfo CreateProperty(
                 ignoreCondition,
                 options,
                 jsonTypeInfo,
-                isCustomProperty: isCustomProperty);
+                isUserDefinedProperty: isUserDefinedProperty);
 
             jsonPropertyInfo.CustomConverter = customConverter;
 

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

@@ -199,7 +199,7 @@ public JsonConverter Converter
             // while NonCustomConverter always contains final converter.
             // This property can be used before JsonTypeInfo is configured (especially in SourceGen case)
             // therefore it's safer to return NonCustomConverter rather than EffectiveConverter.
-            => PropertyInfoForTypeInfo.NonCustomConverter!;
+            => PropertyInfoForTypeInfo.DefaultConverterForType!;
 
         /// <summary>
         /// Determines the kind of contract metadata current JsonTypeInfo instance is customizing
@@ -285,7 +285,7 @@ private protected void CheckMutable()
             }
         }
 
-        private volatile bool _isConfigured;
+        private protected volatile bool _isConfigured;
         private readonly object _configureLock = new object();
 
         internal void EnsureConfigured()
@@ -450,7 +450,7 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name)
                 isVirtual: false,
                 converter: converter,
                 options: Options,
-                isCustomProperty: true);
+                isUserDefinedProperty: true);
 
             propertyInfo.Name = name;
 

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

@@ -83,7 +83,7 @@ public Action<Utf8JsonWriter, T>? SerializeHandler
             }
             private protected set
             {
-                CheckMutable();
+                Debug.Assert(!_isConfigured, "We should not mutate configured JsonTypeInfo");
                 _serialize = value;
                 HasSerialize = value != null;
             }

src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs

@@ -101,7 +101,6 @@ public static void SupportsPositionalRecords()
         [Fact]
         public static void CombiningContexts_ResolveJsonTypeInfo()
         {
-            // Basic smoke test establishing combination of JsonSerializerContext classes.
             IJsonTypeInfoResolver combined = JsonTypeInfoResolver.Combine(NestedContext.Default, PersonJsonContext.Default);
             var options = new JsonSerializerOptions { TypeInfoResolver = combined };
 
@@ -142,7 +141,6 @@ public static void CombiningContexts_ResolveJsonTypeInfo_DifferentCasing()
         [MemberData(nameof(GetCombiningContextsData))]
         public static void CombiningContexts_Serialization<T>(T value, string expectedJson)
         {
-            // Basic smoke test establishing combination of JsonSerializerContext classes.
             IJsonTypeInfoResolver combined = JsonTypeInfoResolver.Combine(NestedContext.Default, PersonJsonContext.Default);
             var options = new JsonSerializerOptions { TypeInfoResolver = combined };
 

src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverMultiContextTests.cs

@@ -34,7 +34,10 @@ public async Task TypeInfoWithNullCreateObjectFailsDeserialization()
             o.TypeInfoResolver = resolver;
 
             string json = """{"StringProperty":"test"}""";
+            await TestMultiContextDeserialization<Poco>(json, new Poco() { StringProperty = "test" });
             await TestMultiContextDeserialization<Poco>(json, options: o, expectedExceptionType: typeof(NotSupportedException));
+
+            Assert.Throws<InvalidOperationException>(() => resolver.Modifiers.Add(ti => { }));
         }
 
         private class Poco