[wasm][debugger] Fix DebuggerTypeProxy for structs and for classes with multiple constructors

[ilonatommy]

Fixes #68390.
As a result of the discussion here: #75958 (comment) I separated the test fix into this PR. EvaluateBrowsableRootHidden when run on main is failing, so the quoted PR changes are not connected with the problem.
The problem: DebuggerProxy was working only in a specific case - when the proxy class had only one constructor with 1 argument or the constructor we intended to use was declared as the first one.
Fix:

• we are able to serve multiple constructors and recognise the proper one by number of arguments and their type.
• we support DebuggerProxy for value types (not only classes).

Connected issues:
#77315
#77768

[ghost]

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

---

As a result of the discussion here: 14a2be2#r995534127 I separated the test fix into this PR. EvaluateBrowsableRootHidden when run on main is failing, so the quoted PR changes are not connected with the problem.

Author:	ilonatommy
Assignees:	ilonatommy
Labels:	area-Debugger-mono
Milestone:	-

[radical]

What's the failure with the test? Is this related to #76876 ?

[radical]

background:

• List<T> allocates a private array (_items) of Capacity size. The size for that can be obtained from Capacity, or _items.Length.
• It also has a private int _size, which has the number of actual elements in the list (== list.Count). And this changes when you add/remove elements to the list.
• At some threshold, _items array is reallocated to have a bigger size, and the elements are copied over.

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs

Lines 25 to 26 in 51e3afb

	internal T[] _items; // Do not rename (binary serialization)
	internal int _size; // Do not rename (binary serialization)

Based on that:

• maxSize is really the size of the internal array
• _items here makes sense, since that is a fixed private field in List<T> (specifically in List<T> though)
• Also, the type has [DebuggerTypeProxy(typeof(ICollectionDebugView<>))] which has:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ICollectionDebugView.cs

Lines 19 to 28 in 51e3afb

	[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)]
	public T[] Items
	{
	get
	{
	T[] items = new T[_collection.Count];
	_collection.CopyTo(items, 0);
	return items;
	}
	}

.. which returns an array of size == list.Count .

• So, is this using the type proxy, or are we reading from the actual object? If it's from the former then Items, and for the latter _items would be relevant. Feel free to correct me, I might be missing something.

[ilonatommy]

I don't see any errors in the above. I only cannot understand what mechanism makes the decision if to use Items or _items. I don't think that it's too important but if we knew the rule it would help in simplifying the code. I think I will add some comments to the current code

[radical]

Oh, found the comment - #75958 (comment) .

[ilonatommy]

Actually it's strange. I reverted the changes on #75958 that were fixing this failure and it's passing the CI. It might be something platform-related then because on my machine they are failing after the revert.

[radical]

I tried to look at this, and found:

1. GetValuesFromDebuggerProxyAttribute is used only for object types, but it uses ValueCreator.TryGetValueTypeById:
   

   runtime/src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs

   Lines 2121 to 2128 in 10d342c

   	if (ValueCreator.TryGetValueTypeById(objectId, out var valueType))
   	{
   	//FIXME: Issue #68390
   	//ctorArgsWriter.Write((byte)0); //not used but needed
   	//ctorArgsWriter.Write(0); //not used but needed
   	//ctorArgsWriter.Write((int)1); // num args
   	//ctorArgsWriter.Write(valueType.Buffer);
   	return null;

• Even though the objectId is not for valuetypes, it can still find a value type with the same int id, and then end up incorrectly returning null. Instead, the value type part should be removed from this method completely. And maybe change the name to GetValuesFromDebuggerProxyAttributeForObject.
• This is the reason why sometimes the DebuggerProxyType for List<T> would get ignored, and you would get _items instead.

2. The way we are checking the values for List<> type is incorrect:
   

   runtime/src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs

   Lines 1181 to 1184 in 10d342c

   	var (refList, _) = await EvaluateOnCallFrame(id, "testPropertiesNone.list");
   	var refListProp = await GetProperties(refList["objectId"]?.Value<string>());
   	var list = refListProp.First(v => v["name"]?.Value<string>() is "Items" or "_items");
   	var refListElementsProp = await GetProperties(list["value"]["objectId"]?.Value<string>());

• refListProp should have the root-hidden contents of the list, IIUC based on [wasm][debugger] RootHidden is not working correctly in DebuggerTypeProxy #76876 .

• So, it should not be using line 1183, and line 1184

• This reveals that RootHidden is not working for properties in types that are used as DebuggerProxyType (ICollectionDebugView<> used here on List<>).

• In FindDebuggerProxyConstructorIdFor, it doesn't find the correct set of .ctor methods because it is missing BindingFlags.DeclaredOnly, so it returns the object..ctor also

• Hopefully, we should be able to get rid of hardcoded _items, and Items check with these changes.

About the the corresponding test:

1. it is hacking around the returned result to pick out the values, and check them
2. it is checking the properties of the container object with the properties from the individual members, but this is self-referential. The test should instead check the returned results, with the expected output hardcoded in the tests. For example:

                JObject[] expectedArrayElements = new[]
                {
                    TNumber(11),
                    TNumber(22)
                };
                JObject[] expectedListRootHiddenElements = new[]
                {
                    TNumber(1),
                    TNumber(2)
                };

                //in Console App names are in []
                //adding variable name to make elements unique
                for (int i = 0; i < expectedArrayElements.Length; i ++)
                {
                    var actualValObj = GetAndAssertObjectWithName(testRootHiddenProps, $"arrayRootHidden[{i}]");
                    await CheckValue(actualValObj["value"], expectedArrayElements[i], "x");
                }
                await CheckProps(refArrayProp, expectedArrayElements, "arrayRootHidden");

                for (int i = 0; i < expectedListRootHiddenElements.Length; i ++)
                {
                    var actualValObj = GetAndAssertObjectWithName(testRootHiddenProps, $"listRootHidden[{i}]");
                    await CheckValue(actualValObj["value"], expectedListRootHiddenElements[i], "listRootHidden");
                }
                await CheckProps(refListProp, expectedListRootHiddenElements, "listRootHidden");

This is the incomplete patch that I was able to come up with.

[radical]

In what cases(example code) would we be executing this block?

[ilonatommy]

All cases in DebuggerTests.EvaluateOnCallFrameTests.EvaluateBrowsableRootHidden because

Error Message:
   [] Could not find variable 'listRootHidden[0]'

This PR fixes only the problem with Items vs _items (now it's always Items). But it does not fix the fact that they need to get expanded manually - rootHidden info does not exist on them and cannot be easily extracted. The issue where we address it is this one: #76876.

[radical]

Is my understanding here correct?:

1. List<T> has [DebuggerTypeProxy(typeof(ICollectionDebugView<>))], so we look at the proxy
2. the proxy has Items property
3. Items has [DebuggerBrowsable(DebuggerBrowsableState.RootHidden)], which isn't working yet ([wasm][debugger] RootHidden is not working correctly in DebuggerTypeProxy #76876)
4. So, we are expanding Items here to work specifically for List<T>?

If so, then is this only to get EvaluateBrowsableRootHidden passing?
Also, does #76876 still happen after the DebuggerTypeProxy fix here?

[radical]

I looked at it, and (4) is true. The test for listRootHidden should be disabled for now, and a comment mentioning the issue. And we can remove this incorrect hack.
And in a follow up PR, the root hidden issue can be fixed.

[ilonatommy]

I would say it's a misunderstanding caused by me not deleting the dependency on non-fixed values in the tests properly. Manual expansion was not done on an object decorated with RootHidden but on testPropertiesNone. The else block is not to make the tests pass, it is necessary till RootHidden on Items will be received correctly by Proxy. It was the test that was not 100% logical.
I think now it will make more sense after the last commit.

Yes, the issue is still here. Proxy is not receiving any DebuggerAttributes assigned to Items, so it does not know it's of rootHidden type.

[ilonatommy]

I looked at it, and (4) is true. The test for listRootHidden should be disabled for now, and a comment mentioning the issue. And we can remove this incorrect hack. And in a follow up PR, the root hidden issue can be fixed.

Sorry, this comment did not display for me when I was investigating. I don't agree. This will break mechanism for all Collections that are in rootHidden objects. The else block can be removed when we have the fix already, so that we won't be changing the behavior from working to not working.

[radical]

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

Lines 149 to 161 in 8064bfe

	else
	{
	// a collection - expose elements to be of array scheme
	var memberNamedItems = members
	.Where(m => m["name"]?.Value<string>() == "Items" || m["name"]?.Value<string>() == "_items")
	.FirstOrDefault();
	if (memberNamedItems is not null &&
	(DotnetObjectId.TryParse(memberNamedItems["value"]?["objectId"]?.Value<string>(), out DotnetObjectId itemsObjectId)) &&
	itemsObjectId.Scheme == "array")
	{
	rootObjectId = itemsObjectId;
	}
	}

This code is very explicitly looking for a Items member which is an array. So, the else arm is affecting only the very specific types that have such a member, which is the case for List<T>. The proxy, or the debugger agent aren't populating it as an extra thing.

I agree that we shouldn't break the behavior in this PR. But it is incorrect. We want to avoid depending on magic like that, especially when proper mechanisms are available to get the information - DebuggerTypeProxy in this case.

The rootHidden issue is in addition to this, and that's fine to fix separately.

[radical]

The order matters, so we shouldn't adjust the results to match the test. The test types seem to have slightly different ordering of the fields/properties, which is causing the difference that you see. Making that consistent would allow not having to sort here.

I made these changes to the tests https://gist.github.com/radical/208598ae72b6310224dc531e3b66eff8 . Removing the sorting actually reveals that in case of root hidden, the members don't get added in the correct order. For example, you get arrayRootHidden[0] as the first member, but arrayRootHidden[1] is not the next one (it was the last in my run).

In general, we want to test the result as-is, as much as possible. And want to avoid adjusting the tests to match incorrect behavior. In case of incorrect behavior, we can have correct tests, but disabled with a corresponding issue.

But since we already have overlapping issues, you can either disable some tests, or patch them up as needed, to merge this PR. And in follow ups fix things to do it correctly.. thank you for being patient with it ;) Make sure to have some issue(s) tracking the TODOs.

[thaystg]

LGTM.