Loosen property name collision detection involving hidden properties (#36936) (#37105)v5.0.0-preview.5.20278.1

* Loosen property name collision detection involving hidden properties

* Delay ignored prop cache creation; add more tests

* Clarify comments

2 files changed, 146 insertions, 27 deletions

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
index f0adf741298..7c24c672dca 100644
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
@@ -101,6 +101,9 @@ namespace System.Text.Json
                                 ? StringComparer.OrdinalIgnoreCase
                                 : StringComparer.Ordinal);
 
+                        HashSet<string>? ignoredProperties = null;
+
+                        // We start from the most derived type and ascend to the base type.
                         for (Type? currentType = type; currentType != null; currentType = currentType.BaseType)
                         {
                             foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly))
@@ -111,44 +114,55 @@ namespace System.Text.Json
                                     continue;
                                 }
 
-                                if (IsNonPublicProperty(propertyInfo))
-                                {
-                                    if (JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(propertyInfo) != null)
-                                    {
-                                        ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType);
-                                    }
-
-                                    // Non-public properties should not be included for (de)serialization.
-                                    continue;
-                                }
-
-                                // For now we only support public getters\setters
+                                // For now we only support public properties (i.e. setter and/or getter is public).
                                 if (propertyInfo.GetMethod?.IsPublic == true ||
                                     propertyInfo.SetMethod?.IsPublic == true)
                                 {
                                     JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, currentType, options);
                                     Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null);
 
-                                    // If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception.
+                                    string propertyName = propertyInfo.Name;
+
+                                    // The JsonPropertyNameAttribute or naming policy resulted in a collision.
                                     if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo))
                                     {
                                         JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString];
 
-                                        if (other.ShouldDeserialize == false && other.ShouldSerialize == false)
+                                        if (other.IsIgnored)
                                         {
-                                            // Overwrite the one just added since it has [JsonIgnore].
+                                            // Overwrite previously cached property since it has [JsonIgnore].
                                             cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo;
                                         }
-                                        else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name &&
-                                            (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true))
+                                        else if (
+                                            // Does the current property have `JsonIgnoreAttribute`?
+                                            !jsonPropertyInfo.IsIgnored &&
+                                            // Is the current property hidden by the previously cached property
+                                            // (with `new` keyword, or by overriding)?
+                                            other.PropertyInfo!.Name != propertyName &&
+                                            // Was a property with the same CLR name was ignored? That property hid the current property,
+                                            // thus, if it was ignored, the current property should be ignored too.
+                                            ignoredProperties?.Contains(propertyName) != true)
                                         {
-                                            // Check for name equality is required to determine when a new slot is used for the member.
-                                            // Therefore, if names are not the same, there is conflict due to the name policy or attributes.
+                                            // We throw if we have two public properties that have the same JSON property name, and neither have been ignored.
                                             ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo);
                                         }
-                                        // else ignore jsonPropertyInfo since it has [JsonIgnore] or it's hidden by a new slot.
+                                        // Ignore the current property.
+                                    }
+
+                                    if (jsonPropertyInfo.IsIgnored)
+                                    {
+                                        (ignoredProperties ??= new HashSet<string>()).Add(propertyName);
                                     }
                                 }
+                                else
+                                {
+                                    if (JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(propertyInfo) != null)
+                                    {
+                                        ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType);
+                                    }
+
+                                    // Non-public properties should not be included for (de)serialization.
+                                }
                             }
                         }
 
@@ -211,13 +225,6 @@ namespace System.Text.Json
             }
         }
 
-        private static bool IsNonPublicProperty(PropertyInfo propertyInfo)
-        {
-            MethodInfo? getMethod = propertyInfo.GetMethod;
-            MethodInfo? setMethod = propertyInfo.SetMethod;
-            return !((getMethod != null && getMethod.IsPublic) || (setMethod != null && setMethod.IsPublic));
-        }
-
         private void InitializeConstructorParameters(Dictionary<string, JsonPropertyInfo> propertyCache, ConstructorInfo constructorInfo)
         {
             ParameterInfo[] parameters = constructorInfo!.GetParameters();
diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
index 2cc311764cb..69ce86512cc 100644
--- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
@@ -327,6 +327,39 @@ namespace System.Text.Json.Serialization.Tests
                 () => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows>(json, options));
         }
 
+        [Fact]
+        public static void HiddenPropertiesIgnored_WhenOverridesIgnored_AndPropertyNameConflicts()
+        {
+            string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride());
+            Assert.Equal(@"{""MyProp"":false}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName());
+            Assert.Equal(@"{""MyProp"":null}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty());
+            Assert.Equal(@"{""MyProp"":false}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty_And_ConflictingPropertyName());
+            Assert.Equal(@"{""MyProp"":null}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType());
+            Assert.Equal(@"{""MyProp"":false}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName());
+            Assert.Equal(@"{""MyProp"":null}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_WithIgnoredOverride());
+            Assert.Equal(@"{""MyProp"":false}", serialized);
+
+            serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_ConflictingPropertyName());
+            Assert.Equal(@"{""MyProp"":null}", serialized);
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName()));
+
+            serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride());
+            Assert.Equal(@"{""MyProp"":null}", serialized);
+        }
+
         public class ClassWithInternalProperty
         {
             internal string MyString { get; set; } = "DefaultValue";
@@ -474,6 +507,85 @@ namespace System.Text.Json.Serialization.Tests
             public new decimal MyNumeric { get; set; } = 1.5M;
         }
 
+        private class Class_With_VirtualProperty
+        {
+            public virtual bool MyProp { get; set; }
+        }
+
+        private class DerivedClass_With_IgnoredOverride : Class_With_VirtualProperty
+        {
+            [JsonIgnore]
+            public override bool MyProp { get; set; }
+        }
+
+        private class DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName : Class_With_VirtualProperty
+        {
+            [JsonPropertyName("MyProp")]
+            public string MyString { get; set; }
+
+            [JsonIgnore]
+            public override bool MyProp { get; set; }
+        }
+
+        private class Class_With_Property
+        {
+            public bool MyProp { get; set; }
+        }
+
+        private class DerivedClass_With_NewProperty : Class_With_Property
+        {
+            [JsonIgnore]
+            public new bool MyProp { get; set; }
+        }
+
+        private class DerivedClass_With_NewProperty_And_ConflictingPropertyName : Class_With_Property
+        {
+            [JsonPropertyName("MyProp")]
+            public string MyString { get; set; }
+
+            [JsonIgnore]
+            public new bool MyProp { get; set; }
+        }
+
+        private class DerivedClass_WithNewProperty_Of_DifferentType : Class_With_Property
+        {
+            [JsonIgnore]
+            public new int MyProp { get; set; }
+        }
+
+        private class DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName : Class_With_Property
+        {
+            [JsonPropertyName("MyProp")]
+            public string MyString { get; set; }
+
+            [JsonIgnore]
+            public new int MyProp { get; set; }
+        }
+
+        private class DerivedClass_WithIgnoredOverride : Class_With_VirtualProperty
+        {
+            [JsonIgnore]
+            public override bool MyProp { get; set; }
+        }
+
+        private class FurtherDerivedClass_With_ConflictingPropertyName : DerivedClass_WithIgnoredOverride
+        {
+            [JsonPropertyName("MyProp")]
+            public string MyString { get; set; }
+        }
+
+        private class DerivedClass_WithConflictingPropertyName : Class_With_VirtualProperty
+        {
+            [JsonPropertyName("MyProp")]
+            public string MyString { get; set; }
+        }
+
+        private class FurtherDerivedClass_With_IgnoredOverride : DerivedClass_WithConflictingPropertyName
+        {
+            [JsonIgnore]
+            public override bool MyProp { get; set; }
+        }
+
         [Fact]
         public static void NoSetter()
         {