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] Implement support for null-conditional operators in simple expressions #69307

Merged
merged 16 commits into from
Jun 8, 2022

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented May 13, 2022

Fixes #69133.

  • this PR enables support for expressions like ?., and !. in debugger watch in simple cases of member access.
  • TODO: the same for array access, and method calls.

Changes:

  • Added passing "forDebuggerDisplayAttribute" when calling RuntimeGetObjectMembers internally in ResolveAsInstanceMember because we need to get all the values from "forDebuggerProxyAttribute". Without it, the members we were extracting were shadowed by proxy and we were skipping some, e.g. Count when extracting members of a Collections.Generic.List. Now we can evaluate list.Count without throwing.
  • Removed the mechanism at the end of ResolveAsInstanceMember that checked if resolvedObject is null and prevented from evaluation of its members when it was true. This caused throwing an exception even when user used null safety operator. Now, evaluation of objectNull?.member results in returning null instead of throwing while objectNull.member throws as expected.

@ghost
Copy link

ghost commented May 13, 2022

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

Issue Details

Fixes #69133.

Changes:

  • Passing "forDebuggerDisplayAttribute" when calling RuntimeGetObjectMembers internally in ResolveAsInstanceMember because we need to get all the values from "forDebuggerProxyAttribute". Without it, the members we were extracting were shadowed by proxy and we were skipping some, e.g. Count when extracting members of a Collections.Generic.List. Now we can evaluate list.Count without throwing.
  • Removed the mechanism at the end of ResolveAsInstanceMember that checked if resolvedObject is null and prevented from evaluation of its members when it was true. This caused throwing an exception even when user used null safety operator. Now, evaluation of objectNull?.member results in returning null instead of throwing while objectNull.member throws as expected.
  • Cut short resolving member access to primitive objects to keep NegativeTestsInInstanceMethod behavior. Now, when we realize in Resolve that retObject is a primitive and expression has more than one part, we return null to let CompileAndRunTheExpression resolve it.
Author: ilonatommy
Assignees: ilonatommy
Labels:

area-Debugger-mono

Milestone: -

@ilonatommy ilonatommy added the arch-wasm WebAssembly architecture label May 13, 2022
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partial comments

@ilonatommy ilonatommy changed the title [wasm][debugger] Added null safety on EvaluateOnFrame [wasm][debugger] Enabled null-conditional operator on EvaluateOnFrame May 19, 2022
@ilonatommy
Copy link
Member Author

ilonatommy commented May 19, 2022

/azp run runtime-staging

@azure-pipelines
Copy link

Command 'runtime-staging' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented May 20, 2022

I think for this, if the expression is not a simple a, or a.b.c, then we should compile it with roslyn, so that we don't have to explicitly handle ?., ?[], !., ![], .. etc.

@ilonatommy
Copy link
Member Author

ilonatommy commented May 21, 2022

I think for this, if the expression is not a simple a, or a.b.c, then we should compile it with roslyn, so that we don't have to explicitly handle ?., ?[], !., ![], .. etc.

You are right, resolving e.g. tc?.memberList[0] will fail with this PR. However, evaluation of expressions in our CompileAndRunTheExpression function where we use Roslyn is tricky because SyntaxNode does not work as expected every time. In the quoted expression it recognizes correctly tc?.memberList[0] as ConditionalAccessExpressionSyntax but treats memberList as identifier instead of MemberAccessExpressionSyntax. Support of null-condition operator in complex expressions requires a lot of changes to the logic, if not a complex refactor of the existing mechanism (I mean: removing the separation between Resolve and CompileAndRunTheExpression and merging them into one process). I saved them in separate issues (#69640).
Good news is that ! operator can be handled in this PR, it does not change anything in the evaluation process, so we can remove it from the expression and evaluate as if it was not there.

@ilonatommy
Copy link
Member Author

/azp run runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy
Copy link
Member Author

Test failure in staging is not connected with the changes.

@ilonatommy ilonatommy requested a review from radical June 2, 2022 16:13
@ilonatommy
Copy link
Member Author

/azp run runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@ilonatommy ilonatommy changed the title [wasm][debugger] Enabled null-conditional operator on EvaluateOnFrame [wasm][debugger] Implement support for null-conditional operators in simple expressions Jun 8, 2022
@ilonatommy ilonatommy merged commit 2c110b6 into dotnet:main Jun 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 8, 2022
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.

[wasm][debugger] Evaluate list?.Count
2 participants