Watch: allow to set value of expressions

[yannickowow]

Related to : #45621 ?

Hi,
I create this request to implement setExpression in VS Code. Since it already supports well "evaluateName", it would be easy to support this in "Watch" view.
In my mind, if debuggee supports setExpression, you could set a value by double-clicking in current one, or by a "Set Expression..." action.
Regards.

[isidorn]

If I understand this correctly: this is a feature request to add a "Set Value" context menu action to the Watch view.
I do not really see a user scenario needd for this. Though I do not mind having this open so we gather more user feedback, thus assigning to backlog

[yannickowow]

Yes, it is correct.
For some debuggers, it can make sense if your expression is, for example, an address or a register which can't be shown in a Variables pane.
GDB for example supports set <variable> <expression>, variable can be:

• A register (example: $pc)
• An array (example: t)
• An address(example *0x400000)
  (For reference: https://ftp.gnu.org/old-gnu/Manuals/gdb/html_node/gdb_111.html#SEC116)

[weinand]

I've quickly checked what VS is doing:

VS knows whether an expression (column "name") is an "lvalue" and can be assigned to. In the screenshot from above the context menu's "Edit Value" command is disabled on the second row because the expression port*port is not an "lvalue".

In VS Code we do not know whether an expression is an "lvalue", so using setExpression might fail if the expression is not an lvalue.
Today VS Code watches use DAP's evaluate request which works for expressions (including assignment expressions if the language and the underlying DAP implementation supports this). This means that in theory a variable's value can be set by an expression like port = 123. However, this does not really work in practice because the watch expressions are evaluated after every step and port = 123 will effectively pin the value of port to 123.

So I suggest that we should try to introduce a "Set Value" context menu command that uses the setExpression DAP request on the watch expression to try to set the value. If the expression is not an "lvalue" we expect setExpression to return an error that we present to the user.

/cc @gregg-miskelly

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

Edit: C# supports setExpression

[weinand]

@isidorn thanks for prototyping this!

1. IMO we should not show the "Set Value" command if the DA does not implement it. It would be confusing for a user to understand why "Set Value" is disabled. When coming from VS that means that the expression is not assignable. But here it only means the DA does not yet support this.
2. For child variables of watches let's continue to use setVariable for now. The discussion about setVariable vs. setExpression takes place in Make VS Code use DAP's "setExpression" request #124679.

Postponing this work is perfectly fine because for June it was only planned to investigate this feature.

[yannickowow]

As a reference, here is how Eclipse does to determine if an expression is editable (using GDB)

84-var-show-attributes var4
84^done,attr="editable"

If "attributes" contains editable, Eclipse GUI allows to set expression.

Here, for example, expressions 2, 3, and expression's child of &i are editable

Might we need to return a isEditable boolean in evaluateRequest response ?

[isidorn]

Great, thanks for feedback!
@weinand it would be cool if mock debug supported set expression, to make it easier to try this out end to end (without the need to install C#)

[weinand]

@isidorn sure, I will do this!

[gregg-miskelly]

@weinand In case it isn't obvious, VS knows if something is an l-value or not by looking at the readOnly attribute in VariablePresentationHint.

[weinand]

@isidorn I have reimplemented variables in Mock Debug. See https://github.com/microsoft/vscode-mock-debug/tree/main/sampleWorkspace#variables for details.
Both setVariable and setExpression are implemented but only setVariable is enabled through the capability. But I was not yet able to make VS Code call setVariable....

[isidorn]

Same as #124679 (comment)
Will be merged later today, thus closing this

[weinand]

@isidorn I've finished the implementation of setExpression for Mock Debug and published a new version 0.46.2 to the Marketplace. In addition I'm now setting the supportsSetVariablecapability to false, so that only setExpression is used in all cases.

Here is a demo:

I 'm seeing one issue: when setting a new value for a watch, the watch isn't updated in the WATCHES view but everywhere else. I'm returning the new value from the setExpression request (similar to the setVariable request). So there is no need re-evaluate the expression.

And we should consider to respect the readOnly attribute in VariablePresentationHint and disable "Set Value" if it is true. See Gregg's comment above.

[isidorn]

@weinand good catches. Thanks!

The watches setExpression issue I have handled via 19eb720 however it seems like mockDebug is not returning body for the setExpression response. And that seems to be the reason why this is now not working. Can you double check if the body object is properly returned?

Set Value is now disabled for readOnly variables via ba5054e

[weinand]

@isidorn I've verified that I'm returning a correct body from setExpression:

And this body is correctly received in VS Code:

[isidorn]

@weinand thanks, I have tackled this via c00e89d
So I think we are now good.

[weinand]

BTW, error handling works nicely: