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 passing negative/positive numbers to methods. #92754

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

ilonatommy
Copy link
Member

Passing negative number generates different ExpressionSyntax type than passing a positive number. Because we classify method arguments by casting them to Syntax types, we were not handling negative case at all (throwing Unable to evaluate method 'GetDefaultAndRequiredParam'. Unable to write into binary writer, not recognized expression type: PrefixUnaryExpressionSyntax").
Same with positive numbers, passing +2 should behave the same as passing 2.
SyntaxKind has a lot of possible values (https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.csharp.syntaxkind) and we probably are not handling all of them yet. Logging this into #65864.

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

ghost commented Sep 28, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Passing negative number generates different ExpressionSyntax type than passing a positive number. Because we classify method arguments by casting them to Syntax types, we were not handling negative case at all (throwing Unable to evaluate method 'GetDefaultAndRequiredParam'. Unable to write into binary writer, not recognized expression type: PrefixUnaryExpressionSyntax").
Same with positive numbers, passing +2 should behave the same as passing 2.
SyntaxKind has a lot of possible values (https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.csharp.syntaxkind) and we probably are not handling all of them yet. Logging this into #65864.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@ilonatommy ilonatommy changed the title [wasm][debugger] Support passing negative numbers to methods. [wasm][debugger] Support passing negative/positive numbers to methods. Sep 28, 2023
@@ -499,6 +499,8 @@ public async Task EvaluateLocalObjectFromAssemblyNotRelatedButLoaded()
("test.GetDefaultAndRequiredParam(3, 2)", TNumber(5)),
("test.GetDefaultAndRequiredParam(3, +2)", TNumber(5)),
("test.GetDefaultAndRequiredParam(3, -2)", TNumber(1)),
("test.GetDefaultAndRequiredParam(-123l, -1.1f)", TNumber("-124.1", isDecimal: true)), // long, float
("test.GetDefaultAndRequiredParam(-0.23)", TNumber("-32768.23", isDecimal: true)), // double, short
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the short argument.

Copy link
Member Author

@ilonatommy ilonatommy Oct 5, 2023

Choose a reason for hiding this comment

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

Short is optional param of value -32768, the test is checking for optional params to be used correctly and I wanted to keep this convention for this case.

Copy link
Member

Choose a reason for hiding this comment

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

                    ("test.GetDefaultAndRequiredParam(3, +2)", TNumber(5)),
                    ("test.GetDefaultAndRequiredParam(3, -2)", TNumber(1)),
                    ("test.GetDefaultAndRequiredParam(-123l, -1.1f)", TNumber("-124.1", isDecimal: true)),
  • These 3 are not testing optional parameters.
  • Also, for the cases that call GetDefaultAndRequiredParam - add comments mentioning what optional type is being tested.

Do the changes in this PR also affect passing a negative number as an argument to a method?

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.

Other than the feedback on the test, LGTM 👍

@ilonatommy ilonatommy merged commit 52aca8c into dotnet:main Oct 5, 2023
15 checks passed
ilonatommy added a commit that referenced this pull request Oct 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 4, 2023
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