Display parents' members on GetProperties. (#68907)

* Prepared tests with the same members as in Console App.

* Adding parents' members to match the expected result in tests.

* Increasing the timeout because some EvaluateOnCallFrameTests tests take up to 63 sec.

* Reverting timeout editions.

* Applied refactoring suggestions from @radical and @thaystg.

* Added new text cases with addnotation which fields are displayed incorrectly and should be fixed.

* More detailed error message for count mismatch.

* Reorganising tests with order: grouping similar members.

* Fixing override/new problem.

* Fixing test to reflect ConsoleApp behavior.

* Wrong name - it should be tested with the new class: 2.

* Applied @radical's suggestion about backingField.

* @radical's changes - refactor + one new test case.

* Checking Getter vals; AssertEqual(cnt) not needed.

* New test cases, some to be fixed in the follow-up issue.

* Added @radical's suggestions.

* Different DataTimes for each property.

Author: ilonatommy

src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs

@@ -291,10 +291,10 @@ JArray GetHiddenElement()
             }
         }
 
-        public static async Task<Dictionary<string, JObject>> GetNonAutomaticPropertyValues(
+        public static async Task<Dictionary<string, JObject>> ExpandPropertyValues(
             MonoSDBHelper sdbHelper,
             int typeId,
-            string containerTypeName,
+            string typeName,
             ArraySegment<byte> getterParamsBuffer,
             bool isAutoExpandable,
             DotnetObjectId objectId,
@@ -311,6 +311,7 @@ public static async Task<Dictionary<string, JObject>> GetNonAutomaticPropertyVal
             var nProperties = retDebuggerCmdReader.ReadInt32();
             var typeInfo = await sdbHelper.GetTypeInfo(typeId, token);
             var typePropertiesBrowsableInfo = typeInfo?.Info?.DebuggerBrowsableProperties;
+            var parentSuffix = typeName.Split('.')[^1];
 
             GetMembersResult ret = new();
             for (int i = 0; i < nProperties; i++)
@@ -326,82 +327,139 @@ public static async Task<Dictionary<string, JObject>> GetNonAutomaticPropertyVal
                     continue;
 
                 MethodInfoWithDebugInformation getterInfo = await sdbHelper.GetMethodInfo(getMethodId, token);
-                MethodAttributes getterAttrs = getterInfo?.Info.Attributes ?? MethodAttributes.Public;
-                getterAttrs &= MethodAttributes.MemberAccessMask;
+                MethodAttributes getterAttrs = getterInfo.Info.Attributes;
+                MethodAttributes getterMemberAccessAttrs = getterAttrs & MethodAttributes.MemberAccessMask;
 
                 typePropertiesBrowsableInfo.TryGetValue(propName, out DebuggerBrowsableState? state);
 
-                if (allMembers.TryGetValue(propName, out JObject backingField))
+                // handle parents' members:
+                if (!allMembers.TryGetValue(propName, out JObject existingMember))
                 {
-                    if (backingField["__isBackingField"]?.Value<bool>() == true)
-                    {
-                        // Update backingField's access with the one from the property getter
-                        backingField["__section"] = getterAttrs switch
-                        {
-                            MethodAttributes.Private => "private",
-                            MethodAttributes.Public => "result",
-                            _ => "internal"
-                        };
-                        backingField["__state"] = state?.ToString();
-
-                        if (state is not null)
-                        {
-                            string namePrefix = GetNamePrefixForValues(propName, containerTypeName, isOwn, state);
-
-                            string backingFieldTypeName = backingField["value"]?["className"]?.Value<string>();
-                            var expanded = await GetExpandedMemberValues(
-                                sdbHelper, backingFieldTypeName, namePrefix, backingField, state, includeStatic, token);
-                            backingField.Remove();
-                            allMembers.Remove(propName);
-                            foreach (JObject evalue in expanded)
-                                allMembers[evalue["name"].Value<string>()] = evalue;
-                        }
-                    }
+                    // new member
+                    await AddProperty(getMethodId, state, propName, getterMemberAccessAttrs);
+                    continue;
+                }
+
+                bool isExistingMemberABackingField = existingMember["__isBackingField"]?.Value<bool>() == true;
+                if (isOwn && !isExistingMemberABackingField)
+                {
+                    // repeated propname on the same type! cannot happen
+                    throw new Exception($"Internal Error: should not happen. propName: {propName}. Existing all members: {string.Join(",", allMembers.Keys)}");
+                }
 
-                    // derived type already had a member of this name
+                bool isExistingMemberABackingFieldOwnedByThisType = isExistingMemberABackingField && existingMember["__owner"]?.Value<string>() == typeName;
+                if (isExistingMemberABackingField && (isOwn || isExistingMemberABackingFieldOwnedByThisType))
+                {
+                    // this is the property corresponding to the backing field in *this* type
+                    // `isOwn` would mean that this is the first type that we are looking at
+                    await UpdateBackingFieldWithPropertyAttributes(existingMember, propName, getterMemberAccessAttrs, state);
                     continue;
                 }
-                else
+
+                var overriddenOrHiddenPropName = $"{propName} ({parentSuffix})";
+                MethodAttributes vtableLayout = getterAttrs & MethodAttributes.VtableLayoutMask;
+                bool wasOverriddenByDerivedType = (vtableLayout & MethodAttributes.NewSlot) == MethodAttributes.NewSlot;
+                if (wasOverriddenByDerivedType)
+                {
+                    /*
+                     * property was overridden by a derived type member. We want to show
+                     * only the overridden members. So, remove the backing field
+                     * for this auto-property that was added, with the type name suffix
+                     *
+                     * Two cases:
+                     * 1. auto-prop in base, overridden by auto-prop in derived
+                     * 2. auto-prop in base, overridden by prop in derived
+                     *
+                     *    And in both cases we want to remove the backing field for the auto-prop for
+                     *      *this* base type
+                     */
+                    allMembers.Remove(overriddenOrHiddenPropName);
+                    continue;
+                }
+
+                /*
+                 * property was *hidden* by a derived type member. In this case, we
+                 * want to show *both* the members
+                 */
+
+                JObject backingFieldForHiddenProp = allMembers.GetValueOrDefault(overriddenOrHiddenPropName);
+                if (backingFieldForHiddenProp is null || backingFieldForHiddenProp["__isBackingField"]?.Value<bool>() != true)
                 {
-                    string returnTypeName = await sdbHelper.GetReturnType(getMethodId, token);
-                    JObject propRet = null;
-                    if (isAutoExpandable || (state is DebuggerBrowsableState.RootHidden && IsACollectionType(returnTypeName)))
+                    // hiding with a non-auto property, so nothing to adjust
+                    // add the new property
+                    await AddProperty(getMethodId, state, overriddenOrHiddenPropName, getterMemberAccessAttrs);
+                    continue;
+                }
+
+                await UpdateBackingFieldWithPropertyAttributes(backingFieldForHiddenProp, overriddenOrHiddenPropName, getterMemberAccessAttrs, state);
+            }
+
+            return allMembers;
+
+            async Task UpdateBackingFieldWithPropertyAttributes(JObject backingField, string autoPropName, MethodAttributes getterMemberAccessAttrs, DebuggerBrowsableState? state)
+            {
+                backingField["__section"] = getterMemberAccessAttrs switch
+                {
+                    MethodAttributes.Private => "private",
+                    MethodAttributes.Public => "result",
+                    _ => "internal"
+                };
+                backingField["__state"] = state?.ToString();
+
+                if (state is null)
+                    return;
+
+                string namePrefix = GetNamePrefixForValues(autoPropName, typeName, isOwn, state);
+                string backingPropTypeName = backingField["value"]?["className"]?.Value<string>();
+                var expanded = await GetExpandedMemberValues(
+                    sdbHelper, backingPropTypeName, namePrefix, backingField, state, includeStatic, token);
+                backingField.Remove();
+                allMembers.Remove(autoPropName);
+                foreach (JObject evalue in expanded)
+                    allMembers[evalue["name"].Value<string>()] = evalue;
+            }
+
+            async Task AddProperty(int getMethodId, DebuggerBrowsableState? state, string propNameWithSufix, MethodAttributes getterAttrs)
+            {
+                string returnTypeName = await sdbHelper.GetReturnType(getMethodId, token);
+                JObject propRet = null;
+                if (isAutoExpandable || (state is DebuggerBrowsableState.RootHidden && IsACollectionType(returnTypeName)))
+                {
+                    try
                     {
-                        try
-                        {
-                            propRet = await sdbHelper.InvokeMethod(getterParamsBuffer, getMethodId, token, name: propName);
-                        }
-                        catch (Exception)
-                        {
-                            continue;
-                        }
+                        propRet = await sdbHelper.InvokeMethod(getterParamsBuffer, getMethodId, token, name: propNameWithSufix);
                     }
-                    else
-                        propRet = GetNotAutoExpandableObject(getMethodId, propName);
-
-                    propRet["isOwn"] = isOwn;
-                    propRet["__section"] = getterAttrs switch
+                    catch (Exception)
                     {
-                        MethodAttributes.Private => "private",
-                        MethodAttributes.Public => "result",
-                        _ => "internal"
-                    };
-                    propRet["__state"] = state?.ToString();
-
-                    string namePrefix = GetNamePrefixForValues(propName, containerTypeName, isOwn, state);
-                    var expandedMembers = await GetExpandedMemberValues(
-                        sdbHelper, returnTypeName, namePrefix, propRet, state, includeStatic, token);
-                    foreach (var member in expandedMembers)
+                        propRet = GetNotAutoExpandableObject(getMethodId, propNameWithSufix);
+                    }
+                }
+                else
+                {
+                    propRet = GetNotAutoExpandableObject(getMethodId, propNameWithSufix);
+                }
+
+                propRet["isOwn"] = isOwn;
+                propRet["__section"] = getterAttrs switch
+                {
+                    MethodAttributes.Private => "private",
+                    MethodAttributes.Public => "result",
+                    _ => "internal"
+                };
+                propRet["__state"] = state?.ToString();
+
+                string namePrefix = GetNamePrefixForValues(propNameWithSufix, typeName, isOwn, state);
+                var expandedMembers = await GetExpandedMemberValues(
+                    sdbHelper, returnTypeName, namePrefix, propRet, state, includeStatic, token);
+                foreach (var member in expandedMembers)
+                {
+                    var key = member["name"]?.Value<string>();
+                    if (key != null)
                     {
-                        var key = member["name"]?.Value<string>();
-                        if (key != null)
-                        {
-                            allMembers.TryAdd(key, member as JObject);
-                        }
+                        allMembers.TryAdd(key, member as JObject);
                     }
                 }
             }
-            return allMembers;
 
             JObject GetNotAutoExpandableObject(int methodId, string propertyName)
             {
@@ -488,14 +546,14 @@ public static async Task<GetMembersResult> GetObjectMemberValues(
                         foreach (var f in allFields)
                             f["__hidden"] = true;
                     }
-                    AddOnlyNewValuesByNameTo(allFields, allMembers, isOwn);
+                    AddOnlyNewFieldValuesByNameTo(allFields, allMembers, typeName, isOwn);
                 }
 
                 // skip loading properties if not necessary
                 if (!getCommandType.HasFlag(GetObjectCommandOptions.WithProperties))
                     return GetMembersResult.FromValues(allMembers.Values, sortByAccessLevel);
 
-                allMembers = await GetNonAutomaticPropertyValues(
+                allMembers = await ExpandPropertyValues(
                     sdbHelper,
                     typeId,
                     typeName,
@@ -518,15 +576,29 @@ public static async Task<GetMembersResult> GetObjectMemberValues(
             }
             return GetMembersResult.FromValues(allMembers.Values, sortByAccessLevel);
 
-            static void AddOnlyNewValuesByNameTo(JArray namedValues, IDictionary<string, JObject> valuesDict, bool isOwn)
+            static void AddOnlyNewFieldValuesByNameTo(JArray namedValues, IDictionary<string, JObject> valuesDict, string typeName, bool isOwn)
             {
                 foreach (var item in namedValues)
                 {
-                    var key = item["name"]?.Value<string>();
-                    if (key != null)
+                    var name = item["name"]?.Value<string>();
+                    if (name == null)
+                        continue;
+
+                    if (valuesDict.TryAdd(name, item as JObject))
                     {
-                        valuesDict.TryAdd(key, item as JObject);
+                        // new member
+                        if (item["__isBackingField"]?.Value<bool>() == true)
+                            item["__owner"] = typeName;
+                        continue;
                     }
+
+                    if (isOwn)
+                        throw new Exception($"Internal Error: found an existing member on own type. item: {item}, typeName: {typeName}");
+
+                    var parentSuffix = typeName.Split('.')[^1];
+                    var parentMemberName = $"{name} ({parentSuffix})";
+                    valuesDict.Add(parentMemberName, item as JObject);
+                    item["name"] = parentMemberName;
                 }
             }
         }

src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs

@@ -270,7 +270,7 @@ public async Task ExpandPropertyValues(MonoSDBHelper sdbHelper, bool splitMember
                 // isParent:
                 if (i != 0) typeId = getParentsReader.ReadInt32();
 
-                allMembers = await MemberObjectsExplorer.GetNonAutomaticPropertyValues(
+                allMembers = await MemberObjectsExplorer.ExpandPropertyValues(
                     sdbHelper,
                     typeId,
                     className,

src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs

@@ -732,7 +732,20 @@ internal async Task CheckProps(JToken actual, object exp_o, string label, int nu
             if (!skip_num_fields_check)
             {
                 num_fields = num_fields < 0 ? exp.Values<JToken>().Count() : num_fields;
-                Assert.True(num_fields == actual.Count(), $"[{label}] Number of fields don't match, Expected: {num_fields}, Actual: {actual.Count()}");
+                var expected_str = string.Join(", ",
+                    exp.Children()
+                    .Select(e => e is JProperty jprop ? jprop.Name : e["name"]?.Value<string>())
+                    .Where(e => !string.IsNullOrEmpty(e))
+                    .OrderBy(e => e));
+
+                var actual_str = string.Join(", ",
+                    actual.Children()
+                    .Select(e => e["name"]?.Value<string>())
+                    .Where(e => !string.IsNullOrEmpty(e))
+                    .OrderBy(e => e));
+                Assert.True(num_fields == actual.Count(), $"[{label}] Number of fields don't match, Expected: {num_fields}, Actual: {actual.Count()}.{Environment.NewLine}"
+                    + $"  Expected: {expected_str}{Environment.NewLine}"
+                    + $"  Actual:   {actual_str}");
             }
 
             foreach (var kvp in exp)
@@ -756,8 +769,6 @@ internal async Task CheckProps(JToken actual, object exp_o, string label, int nu
                 else if (exp_val["__custom_type"] != null && exp_val["__custom_type"]?.Value<string>() == "getter")
                 {
                     // hack: for getters, actual won't have a .value
-                    // are we doing it on purpose? Why? CHECK if properties are displayed in Browser/VS, if not revert the value field here
-                    // we should be leaving properties, not their backing fields
                     await CheckCustomType(actual_obj, exp_val, $"{label}#{exp_name}");
                 }
                 else