-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wasm][debugger] Support indexing of valuetype scheme #92637
Conversation
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsFollow-up for #92630. After merging this PR we will be able to fix multidimensional indexing of valuetypes.
|
src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Outdated
Show resolved
Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Outdated
Show resolved
Hide resolved
throw new InvalidOperationException($"Type '{rootObject?["className"]?.Value<string>()}' cannot be indexed."); | ||
var type = rootObject?["type"]?.Value<string>(); | ||
|
||
// ToDo: optimize the loop by choosing the right method at once without trying out them all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is needed, or missing to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your idea here is the better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason why it might be not possible:
- If the parameter is required (as opposed to default) then we don't have information about its type. Even though we know what types were passed to the method, we cannot tell if this method will work well with them till we ask the runtime to
CMD_VM_INVOKE_METHOD
and it will throw / return a result. - Sometimes we even are not sure what type was passed to the method, see [wasm][debugger] Evaluate methods with short arguments #93057.
It would be good to try if something changed but please, not in this PR. I will see if I can make it in a separate one.
Edit:
Trying to create the new PR I found a good example for situation 1).
void foo(Class1 c);
void foo(Class2 c);
When we ask for GetMethodIdsByName
for "foo" we will get both methods' IDs. We can check both method parameter types by refection but the information we will get is ElementType.Class
which does not tell us enough.
|
||
public double this[int key1, double key2] => key1 + key2; | ||
public string this[char key1, string key2, string key3] => $"{key1}-{key2}-{key3}"; | ||
public Dictionary<string, bool> indexedByStr = new Dictionary<string, bool>() { { "1", true }, { "111", false }, { "true", true} }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: be careful with changing existing test code, as it can be easy to break tests even when we think we changed the line numbers correctly, especially if they end up affecting line numbers used for breakpoints for instance. Such a change might not break a test immediately, but it might now be stopping on the incorrect line opening a potential for the test not doing the intended checks.
Follow-up for #92630.
Changes in tests:
TestEvaluate
class intoCommonCollections
,ClassWithIndexers
andStructWithIndexers
to avoid code duplication.f
was an ambiguous name, now we have c for class and s for struct.Changes in logic:
LiteralExpressionSyntax
arguments for compatibility - this revealed a small overlooking inCheckParameterCompatibility
switch case - when the method expectsElementType.Boolean
we cannot send string or char type as a param, it's not compatible.get_Items
method (InvokeGetItemOnJObject
).