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

@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.

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?

@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.

3. 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.

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?

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

3. 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

@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".

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

3. 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.

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.

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!

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

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