Skip to content
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 by value type schema #92334

Closed
wants to merge 15 commits into from

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Sep 20, 2023

Fixes partially #77536.

  • Added test for indexing by enum and struct
  • Enum indexing needed a fix. When indexing, SyntaxNode returns IdentifierNameSyntax value that is nested like this:
{
  "type": "object",
  "value": {
    "type": "object",
    "value": {
      "type": "number",
      "value": 0,
      "description": "0",
      "className": "Int32"
    },
    "description": "High",
    "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
    "objectId": "dotnet:valuetype:10",
    "isValueType": true,
    "isEnum": true
  },
  "description": "High",
  "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
  "objectId": "dotnet:valuetype:12",
  "isValueType": true,
  "isEnum": true
}

while in other scenarios, the nesting in enums do not occur and values look e.g. like this:

{
  "type": "object",
  "value": {
    "type": "number",
    "value": 0,
    "description": "0",
    "className": "Int32"
  },
  "description": "High",
  "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
  "objectId": "dotnet:valuetype:10",
  "isValueType": true,
  "isEnum": true
}

Because we want ConvertJSToCSharpLocalVariableAssignment to handle both cases, try-catch expression was added (checking type with "is" keyword does not work here as we are casting and exception happens in the cast).

In GetVariableDefinitions we handle enum case separately as a workaround to the following:
In the original version of code, we would produce variable definition that looks like this: DebuggerTests.castToEnumTypeThatIsDefinedInExternalAssembly enumIdx = (DebuggerTests.castToEnumTypeThatIsDefinedInExternalAssembly) 0;
Error produced when we use this approach implies that we are missing the assembly with Enum type definition passed as reference to the script: The type or namespace name 'DebuggerTests' could not be found (are you missing a using directive or an assembly reference?)",
Adding pdbs that are stored in DebugStore.assemblies to the Roslyn's script did not affect the error message, so I used the workaround of passing numeric value of enum instead.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Debugger-mono labels Sep 20, 2023
@ilonatommy ilonatommy self-assigned this Sep 20, 2023
@ghost
Copy link

ghost commented Sep 20, 2023

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

Issue Details
  • Added test for indexing by enum and struct
  • Enum indexing needed a fix. When indexing, SyntaxNode returns value that is nested like this:
{
  "type": "object",
  "value": {
    "type": "object",
    "value": {
      "type": "number",
      "value": 0,
      "description": "0",
      "className": "Int32"
    },
    "description": "High",
    "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
    "objectId": "dotnet:valuetype:10",
    "isValueType": true,
    "isEnum": true
  },
  "description": "High",
  "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
  "objectId": "dotnet:valuetype:12",
  "isValueType": true,
  "isEnum": true
}

while in other scenarios, the nesting in enums do not occur and values look e.g. like this:

{
  "type": "object",
  "value": {
    "type": "number",
    "value": 0,
    "description": "0",
    "className": "Int32"
  },
  "description": "High",
  "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.EnumIndexer",
  "objectId": "dotnet:valuetype:10",
  "isValueType": true,
  "isEnum": true
}

Because we want ConvertJSToCSharpLocalVariableAssignment to handle both cases, try-catch expression was added (checking type with "is" keyword does not work here as we are casting and exception happens in the cast).

In GetVariableDefinitions we handle enum case separately as a workaround to the following:
In the original version of code, we would produce variable definition that looks like this: DebuggerTests.castToEnumTypeThatIsDefinedInExternalAssembly enumIdx = (DebuggerTests.castToEnumTypeThatIsDefinedInExternalAssembly) 0;
Error produced when we use this approach implies that we are missing the assembly with Enum type definition passed as reference to the script: The type or namespace name 'DebuggerTests' could not be found (are you missing a using directive or an assembly reference?)",
Adding pdbs that are stored in DebugStore.assemblies to the Roslyn's script did not affect the error message, so I used the workaround of passing numeric value of enum instead.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@radical
Copy link
Member

radical commented Sep 20, 2023

... while in other scenarios, the nesting in enums do not occur and values look ...

What are the cases when these two separate forms are emitted? Why not just one?

@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 21, 2023

Failures after merge with main:
The order of declaring indexing methods in TestEvaluate started to matter which should not happen. @thaystg, I think I will need some help here.

Reason:
When we are checking method argument types in GetParametersInfo:

methodSignature.ParameterTypes[paramInx]

all types: class Indexer, enum EnumIndexer and struct StructIndexer are decoded as "object", even though ElementType defines ValueType and we would expect the two latter to return ValueType:
.
As a consequence, when we evaluate indexing by struct: f[StructIndexer] and use CheckParametersCompatibility
private static bool CheckParametersCompatibility(ElementType? paramTypeCode, JObject value)

to check compatibility of method get_Items(Indexer), where indexObject is:

{
  "type": "object",
  "value": null,
  "description": "DebuggerTests.EvaluateLocalsWithIndexingTests.StructIndexer",
  "className": "DebuggerTests.EvaluateLocalsWithIndexingTests.StructIndexer",
  "objectId": "dotnet:valuetype:13",
  "isValueType": true
}

we are not able to detect type incompatibility (struct cannot be passed to get_Index with Indexer argument).
While similar case of passing struct into a method get_Index[EnumIndexer] fails on the debugger side, the above one passes, returning incorrect response:

{
   "type": "number",
   "value": 1.0,
   "description": "1",
   "className": "Object"
 }

instead of:

{
   "type": "boolean",
   "value": true,
   "description": "true"
 }

@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 21, 2023

What are the cases when these two separate forms are emitted? Why not just one?

Nested result:

  • Evaluated expression: f[enumIdx] or enumIdx or enumIdx.ToString()
  • How do we get this value? On VisitInternal we discover one IdentifierNameSyntax named enumIdx. We get its value on this line:
    IList<JObject> identifierValues = await Resolve(replacer.identifiers, resolver, ResolveIdentifier, token);

    by calling ResolveIdentifier -> Resolve(string, CancellationToken) -> ResolveAsLocalOrThisMember and retrieving the value from scopeCache.Locals. It is already nested.

Non-nested:

  • Evaluated expression: f[enumIdx] or enumIdx or enumIdx.ToString()
  • If we revert try-catch changes we can see the non-nested version that is a result of the changes in GetVariableDefinitions enum case from this PR. We are entering ConvertJSToCSharpLocalVariableAssignment from GetVariableDefinitions passing definition.Obj["value"] as a parameter (removing one nested level).

When we fix the passed argument, we can revert try-catch (and we stay with nested version everywhere).

@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 21, 2023

What are the cases when these two separate forms are emitted? Why not just one?

Update to this question after reverting the try-catch. Test that fails: EvaluateMethodsOnPrimitiveTypesReturningObjects.
Expression: test.propString.Split('_', 3, System.StringSplitOptions.TrimEntries), the problematic value is System.StringSplitOptions.TrimEntries.

Reason:
We are not nesting the results of ResolveStaticMembersInStaticTypes:

{
  "type": "object",
  "value": {
    "type": "number",
    "value": 2,
    "description": "2",
    "className": "Int32"
  },
  "description": "TrimEntries",
  "className": "System.StringSplitOptions",
  "objectId": "dotnet:valuetype:1",
  "isValueType": true,
  "isEnum": true
}

We could change the static members resolution behavior to be nested but it seems wrong and artificial. I think it will be better to go back to 2-version handling instead, @radical.

@ghost
Copy link

ghost commented Oct 26, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Debugger-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants