Make VS Code use DAP's "setExpression" request

[weinand]

In microsoft/debug-adapter-protocol#188 we've discussed how to simplify the implementation of DAP's setVariable request in debug adapters. The proposal asked for passing an additional "l-value expression" to setVariable which can be easily used in an "evaluate"-based implementation.

One finding of this discussion was that DAP has already a setExpression request which is basically a variant of setVariable that takes a "l-value expression".

Based on that finding we came to the conclusion that only the following spec clarification is needed:

If a debug adapter implements both setVariable and setExpression, a client will only use setExpression if the variable has an evaluateName property.

A consequence of this is that VS Code will start to use setExpression if a variable has the evaluateName property and the debug adapter supports the setExpression request.

[isidorn]

This makes good sense. Let me assign to June, so we change this next milestone once we finalise the DAP clarification.

[weinand]

@isidorn the proposed solution might not be ideal for @gregg-miskelly. Please see the discussion starting with microsoft/debug-adapter-protocol#188 (comment)

[isidorn]

Thanks for the pointer, I see that it does not work for @gregg-miskelly
Should we move this to On-Deck until we land on a solution

[weinand]

I wonder, why VS Code is not using setExpression for watches.

[isidorn]

@weinand we are not using setExpression for anything on the VS Code side. We simply have not adopted it.
So I think the reason is historical, we first added setVariable and we use that when chaning the value of the Variable in the VARIABLES view.

For the Watch expression, we do not allow to set expression from the Watch view. Only to edit, which would re-evaluate it.

So it seems like there is a missing piece that VS Code starts using setExpression for watches

[weinand]

@isidorn yes, I understand why we have setVariable. But setVariable only works if there is a variable container. In the VARIABLEs view the top most container is the Scope.

But in the WATCHES view there is no scope (hence no container) and we are not using setVariable but evaluate.
If we would allow to change watches, we could use setExpression.

[yannickowow]

Hi
If you do your last sentence, it would close #115063

[isidorn]

Yeah good point, that feature request is what is missing for this scenario it seems.

[isidorn]

I started investigating into this and created this PR which proves it is possible #127162

Some open questions:

1. If the debugger does not support setExpression should we show the action as disabled or not in the menu
2. For variables that are children of watch expressions should we send the setVariable or setExpression

I am not aware of a debug adapter that supports the setExpression so I was not able to test this end to end right now. Also I am not super happy with the code thus I would prefer to not do this end of milestone. Thus moving to August since in July I am on vacation. If somebody is passionate about this feel free to continue the work on my PR.

[yannickowow]

Great ! Thanks a lot.
About your mentioned points, my feedbacks would be:

• setExpression should not be available in the Menu (like VS Code does currently for Data Breakpoints, Set Variable and so on)
• Here, can we just send setExpression if evaluateName is defined, setVariable instead ?

[gregg-miskelly]

I am not aware of a debug adapter that supports the setExpression so I was not able to test this end to end right now.

In case this is useful to you: the C# (coreclr) and Windows C++ (cppvsdbg) debug adapters support setExpression.

[weinand]

@isidorn thanks for investigating the use of setExpression for the Watches view.
Since we have #115063 for this investigation, it would be great if you could move your comment to that item. Then we can keep this item for the original (broader) discussion.

[isidorn]

@weinand I have moved my comment to that issue. If you would like to keep this discussion cleaner feel free to delete my comments.

[gregg-miskelly]

For the broader issue of DAs that would like to avoid needing to support setVariable to update the value of a child, I would like to repeat a suggestion I had earlier: a DA could indicate that it always wants to edit using evaluateName by:

• Providing evaluateName for any editable property
• Returning true for supportsSetExpression
• Returning false/null for supportsSetVariable.

[weinand]

@gregg-miskelly your suggestion makes sense and I will update the DAP doc as needed.

@isidorn we should support behavior.

[isidorn]

@gregg-miskelly this suggestion makes good sense, I have implemented it via f1b1e3f

As suggested above:

1. If the extension does not support setExpression, then we will not show the Set Value action in the context menu
2. We send the setVariable for child element in the WATCH view (unless in the case that @gregg-miskelly pointed out)

I plan to merge this later today, so this will be in Insiders next week. I would really appreciate that you try it out and provide feedback. I have only tested it with Mock debug so far. I will also create a test plan item.

[isidorn]

@connor4312 is it possible for js-debug to support setExpression this milestone? That would make it easier to test this out in the endgame. Let me know if you would like me to create an issue in js-debug. Thanks!

[isidorn]

Created this test plan item #130170
In case there are some cases that I did not cover feel free to comment on the test plan item. Thanks

[connor4312]

Thanks, I'm adopting this in js-debug. One point of feedback is that I think the "watch" and "variables" view should be automatically invalidated after the set completes, to match the behavior Set Value in the "variables" view.

[connor4312]

Oh, I was behind on Insiders. The watch view is refreshed, but should the variables view similarly be refreshed? When evaluating an expression I think all debuggers will want this since it's probably not trivial to figure out what variables might have changed as a result of the evaluation of an arbitrary expression.

[isidorn]

@connor4312 the variables view should also get refreshed. If that is not the case let me know and I can investigate. We basically refresh all views, since we never know how a change affect some other variables / watch expressions.

And awesome that you are adopting it, let me know how it goes.

[connor4312]

Nevermind, it was a caching error in my own code, works well :)

[connor4312]

Hm, actually this is (also) caching in V8. Even after changing the variable, getting the stackframe "object" properties still returns the old value. I don't think I can work around this very efficiently 😕

[isidorn]

@connor4312 it is a long standing issue I believe, since I think we were also hitting this a couple of years ago...