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

Implements setExpression request. #612

Merged
merged 1 commit into from Mar 12, 2018

Conversation

Projects
None yet
3 participants
@changsi-an
Copy link
Member

changsi-an commented Mar 9, 2018

setExpression is emitted by Visual Studio to change a value of a watch variable. Such request is out of the definition of VSCode debug protocol.

In this PR we implement only for Chrome. It is what we have verified working. Other debug adapter might need a different approach to update the value.

We want to release this change for Chrome first, because after we switch to PZ, we will have a regression of functionality unless we implement this code change. So we do want to release this piece asap.

I will spend more time to investigate whether the same approach of updating an expression will work the same for Edge. If it can work in the same way, I will take care of moving this code to vscode-chrome-debug-core as well.

@changsi-an changsi-an requested a review from roblourens Mar 9, 2018

capabilities.supportsRestartRequest = true;
capabilities.supportsSetExpression = true;

This comment has been minimized.

@changsi-an

changsi-an Mar 9, 2018

Member

setExpression request will not be issued by PZ untill we turn the configuration on like this.

@changsi-an changsi-an force-pushed the changsi-an:master branch from e83fb8a to 9df45eb Mar 9, 2018

@auchenberg

This comment has been minimized.

Copy link
Contributor

auchenberg commented Mar 9, 2018

I'm I understanding this correct: supportsSetExpression an extension to the VS Code Debug protocol provided by PZ, and is VS specific?

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 9, 2018

@auchenberg That is correct.

And if the DA is not connected to PZ, it won't receive a setExpression request regardless of what supportsSetExpression option is set to.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 9, 2018

Is there a feature request open for this on the DA protocol?

What happens if I edit an expression like 1+2?

Why is it not relevant to Node?

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 10, 2018

@roblourens Thanks for these questions.

  1. setExpression is a very specific thing to VS. Neither Chrome Dev Tools nor VS Code allows changing the value of an expression. Based on this, I don't feel that we have to add this to the general DA protocol definition. Please let me know if you have concerns.

  2. For 1+2 case, PZ is going to show a message box which shows why the value can't be edited. The error message is taken from the response of the setExpression call. If our handler in the DA hits an exception, it will be automatically converted to such a error response. Unfortunately, this code in chrome-core trivially converts that message to "not available". Preferably we should report variable.value (which is the reason reported by the target) to the end user. Also, this code attaches a callstack to the error message returned to PZ. This callstack is not useful, and too verbose. Because the Error object is constructed here, the callstack will be generated based on that code spot. So I intend to remove a callstack from being reported to the end user. (showing an callstack of the DA is not helpful for the end user to diagnose their issues anyway)

That being said, I will do another PR in chrome-core to fix those issues. The current PR should cover everything I need to do in vscode-chrome-debug. Please help share your opinion on what the right approach should be.

  1. It is relevant to Node. But we are not in a rush to fix it for Node now. Node PZ has been in this state for a while. And the reason we need to fix for Chrome is because Chrome has just shifted from using WebKit debugger(legacy) to PZ. To keep feature parity with what WebKit debugger does, we need this change now. I will follow up on testing whether the same approach would work for Node and Edge, then decide if we need to move this code to chrome-core.
@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 10, 2018

  1. I don't think the DA protocol should have one-off client-specific definitions but if VS can do this for JS, it's probably relevant for other runtimes too. C#? Please open an issue at least on that repo.

Would you have the flexibility to just send an evaluate request from PZ, implementing it on that end the same way it's implemented here?

  1. You could work around it by setting context to repl, I think

  2. Why didn't we have to keep parity with the previous node debugger?

If we go with this, I don't see why it wouldn't work for node, and I would think this code should live in -core

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 12, 2018

@roblourens

  1. I can open an issue on the VS Code side.

I don't have the flexibility to send an evaluate request. I think that PZ is doing the right thing. There is no guarantee that doing a "p = v" will work for every debug target. Some debug target might require getting the variable reference first, and change value using that reference. It also depends on whether the returned information from evaluate() is adequate for what the caller expects. If not, setExpression(p, v) has to do more work to get the extra information. In other words, the semantic of setExpression(p, v) does not necessarily equal to evaluating "p = v".

  1. What does "setting context to repl" mean, can you make an example?

  2. It is not clear to me whether in the previous node debugger, changing a watch variable works. If not, there is nothing to keep parity with. Actually, NTVS scenario is not my team's responsibility. So there is some coordination needed before an action should be taken. Moving this code to -core is my goal too. Can we make it as a two-step work? After this PR, I will start look into whether node and edge debug adapter need this feature.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 12, 2018

Sure I understand.

What does "setting context to repl" mean, can you make an example?

I just mean that when you call this.evaluate, you can set the 'context' param on the args object to 'repl', which I think will cause it to return the actual error.

Implements setExpression request.
setExpression is emitted by Visual Studio to change a value of a watch variable. Such request is out of the definition of VSCode debug protocol.

In this PR we implement only for Chrome. It is what we have verified working. Other debug adapter might need a different approach to update the value.

@changsi-an changsi-an force-pushed the changsi-an:master branch from 9df45eb to a7f08dd Mar 12, 2018

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 12, 2018

Setting "repl" works, but I still see a useless callstack appended to the error description. That's probably something only fixable in the -core code base (which I will do next).

And the VS Code issue regarding setExpression request is opened here: Microsoft/vscode#45621

PTAL. Thanks!

@roblourens roblourens merged commit 844604f into Microsoft:master Mar 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 12, 2018

Ok, I will merge this to unblock you but we can keep the discussion going.

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 12, 2018

Thanks Rob. I will keep you in the loop of the follow-up works.

@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment