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

Methods on primitives 65707 #66654

Merged
merged 11 commits into from
May 13, 2022

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Mar 15, 2022

Fixes #65707 and
fixes #65670.
If a result is an array, it enables to get its properties.

Before: evaluation of methods on a primitive value in debugger window did not work, threw "cannot evaluate".
Now: we are getting a proper reply for most of cases with exceptions listed below. To achieve it, we are replacing variables in evaluated script, which means we are appending at the beginning a list of used variables in a form:

type valueName = value;
...
user'sScriptWhereValueNamesAreUsed()

and running to script to get the result that we want to return to the user.

Does not fix:
ToList in: someString.Split(".", 2, StringSplitOptions.None).ToList()
int_arr**.Length** (#66823)
myList + "Asd #65707 (comment)

Changes:

  • Minor refactoring of naming convention EvaluateExpression -> ExpressionEvaluator and FindVariableNMethodCall -> ExpressionSyntaxReplacer, FindVariableNMethodCall.methodCall -> ExpressionSyntaxReplacer.methodCalls.
  • Differentiation of schemes in "object" when replacing variables in script. Before we were using the same way for replacing all variables of "object" type in evaluated script, which resulted in errors for valueTypes.
  • Refactoring: merging ResolveMemberAccessExpressions and ResolveIdentifiers into Resolve that iterates through a collection and resolves accessExpression or identifier by a function accepting a single element. It's neater.
  • If a method call on a primitive type is detected in ResolveMethodCalls we are no longer using the regular procedure of asking the runtime to evaluate it because it would throw an exception - runtime has no means of evaluating objects that have no objectId. Instead in ReplaceMethodCall, we replace the values used in the script and append them at the beginning to evaluate the expression on the debugger side.
  • Supporting boolean return type. Before we were treating boolean as a complex object after evaluating an expression . We granted it "object" type in ConvertCSharpToJSType. Thus, we had to use TObject in tests when checking the value from a function that returned bool. It was confusing so bool scenario was added in ConvertCSharpToJSType and tests were adjusted accordingly.
  • Adjusted negative tests because some error messages got changed slightly as the flow changed.
  • Differentiation between Enum and Non-Enum in ValueTypeClass.ToJObject(). Enum has its value in the first field, even though it does not have fields in the same sense typical ValueType, e.g. structure has. To get the value of Non-Enum we need to additionally ask runtime for this info, so we keep this value null till we need it. We have to save Enum's value there because we will need it during the script's variables replacement procedure.
  • Saving the full className in FindStaticMemberInType (full = with namespace) because we need it for Enums in ConvertJSToCSharpLocalVariableAssignment to construct valueRet correctly. If we cut the namespace the script will throw during compilation because script will not be able to cast integer to Enum - it will not recognise the Enum's name as correct.

@ghost
Copy link

ghost commented Mar 15, 2022

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

Issue Details

Fixes #65707. Does not fix:
ToList in: someString.Split(".", 2, StringSplitOptions.None).ToList()
int_arr.Length
myList + "Asd #65707 (comment)

Author: ilonatommy
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@ilonatommy ilonatommy removed the request for review from marek-safar March 15, 2022 13:43
@ilonatommy ilonatommy added the arch-wasm WebAssembly architecture label Mar 15, 2022
@ghost
Copy link

ghost commented Mar 15, 2022

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

Issue Details

Fixes #65707.
If a result is an array, it enables to get its properties.

Does not fix:
ToList in: someString.Split(".", 2, StringSplitOptions.None).ToList()
int_arr.Length
myList + "Asd #65707 (comment)

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

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.

In the current state, this is very hard to read, mainly due to the EvaluationExpression.cs being split. I suggest either moving the split into a separate PR, or redo the commits, so the first one is just the split, with no other changes. And then the other commits on top of that. That would allow seeing what exactly is being changed.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 16, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 21, 2022
@ilonatommy ilonatommy requested a review from radical March 22, 2022 08:06
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 24, 2022
@ilonatommy ilonatommy requested a review from radical March 24, 2022 16:45
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 25, 2022
@ilonatommy ilonatommy requested a review from radical March 25, 2022 14:43
@ilonatommy ilonatommy force-pushed the methods-on-primitives-65707-2 branch from 0188f0a to 6e3021f Compare April 8, 2022 09:08
@ilonatommy ilonatommy force-pushed the methods-on-primitives-65707-2 branch from f319658 to 5567e88 Compare May 6, 2022 10:13
@radical
Copy link
Member

radical commented May 10, 2022

Please expand the PR description to explain what is being done in this PR, and why.

@ilonatommy ilonatommy requested a review from thaystg May 10, 2022 10:14
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 a few small suggestions, this looks good to me!

@radical
Copy link
Member

radical commented May 13, 2022

Some cases to fix in future PRs:

  • localEnum.toString(), and test.propEnum.ToString()
  • this.ParmToTestObjException.MyMethod() - this one fails with:
 Script used to compile it:

string this_ParmToTestObjException_fee35 = "System.Exception: error2"; return (this_ParmToTestObjException_fee35.MyMethod() );

(2,43): error CS1061: 'string' does not contain a definition for 'MyMethod' and no accessible extension method 'MyMethod' accepting a first argument of type 'string' could be found (are you missing a u    sing directive or an assembly reference?)

.. but it should fail because ParmToTestObjException threw an exception.

@ilonatommy ilonatommy merged commit d395c6b into dotnet:main May 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 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
3 participants