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] Removed internalProperties group. #75904

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

ilonatommy
Copy link
Member

Fixes #75778.

Motivation: #75778 (comment).

It fixes a bug from net 7, so will need backporting.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Debugger-mono labels Sep 20, 2022
@ilonatommy ilonatommy added this to the 7.0.0 milestone Sep 20, 2022
@ilonatommy ilonatommy requested a review from thaystg as a code owner September 20, 2022 16:20
@ilonatommy ilonatommy self-assigned this Sep 20, 2022
@ilonatommy ilonatommy requested a review from radical as a code owner September 20, 2022 16:20
@ghost
Copy link

ghost commented Sep 20, 2022

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

Issue Details

Fixes #75778.

Motivation: #75778 (comment).

It fixes a bug from net 7, so will need backporting.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: 7.0.0

@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 20, 2022

/backport to release/7.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3091753776

@thaystg
Copy link
Member

thaystg commented Sep 20, 2022

If the tests are passing, LGTM.

@ilonatommy
Copy link
Member Author

/backport to release/7.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/7.0-rc2: https://github.com/dotnet/runtime/actions/runs/3091794256

@radical
Copy link
Member

radical commented Sep 21, 2022

@ilonatommy just curious, is VS using the privateProperties part?

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.

Changes look fine to me. I just have a few questions:

  • How is VS surfacing the public/private/protected members? Is it reading anything other than "result"?
  • We should also test this same thing with protected members being overridden by a derived class. And the base/derived split should be tested for being in the user code, and being base (!user code) -> derived(user code)

@lewing lewing merged commit 104c6e1 into dotnet:main Sep 21, 2022
@radical
Copy link
Member

radical commented Sep 21, 2022

Also, it would be a good idea to remove all the __* fields that we add to the json for our consumption, before sending them out from the proxy. The tests shouldn't depend on any of those.

@ilonatommy
Copy link
Member Author

@ilonatommy just curious, is VS using the privateProperties part?

It is using it in the meaning: they are displayed. However, the difference between them and result is not stressed in VS - in Browser it is. To visualise, here are two screenshots from the same class in VS and Browser:
image

image

@ilonatommy
Copy link
Member Author

Also, it would be a good idea to remove all the __* fields that we add to the json for our consumption, before sending them out from the proxy. The tests shouldn't depend on any of those.

I agree and Thays already mentioned this idea as well. The PR with it will be out soon.

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
4 participants