Allow to use Variable's evaluateName in setVariable request

[hyangah]

Variable in DAP has evaluateName which is used when evaluateRequest is made. This is particularly useful when accessing nested fields in a complex data structure.

t := struct { I int } { I: 123 }

For example, Go debug adapter represents t's field I as a Variable like the following

{"variables":[{"name":"I","value":"1","presentationHint":{},"evaluateName":"t.I","variablesReference":0}

That allows us compact and comprehensible display of the struct variable while still allowing us to get the correct syntax to locate the nested field.

I noticed however, setVariable request uses name of Variable, not evaluateName.
https://github.com/microsoft/vscode/blob/5207b984029a2fa8eb4133f594218d25f5325e29/src/vs/workbench/contrib/debug/common/debugModel.ts#L248

[<- from client]{"seq":15,"type":"request","command":"setVariable","arguments":{"variablesReference":1002,"name":"I","value":"2","format":{}}}

We can make the debug adapter to recompute the evaluateName by looking up data with "variablesReference" and "name" info. But, it's sad that the debug adapter has already provided the fully qualified name to vscode but has to recompute it.

I'd argue using evaluateName here is reasonable given that - it tells us how to access the variable for read, and so it should be good for write too.

If it's too late to change setVariable request's name to be evaluateName, is it possible to add an optional evaluateName to the setVariable request?

--
Originally filed as microsoft/vscode#120774
to see if this requires a spec change or changing vscode's debug code to reuse existing name for different purpose is sufficient.

[weinand]

This request makes a lot of sense.

But allowing to use an evaluateName value for the name property does not work because it is not backward compatible: a debug adapter that tries to identify a variable by the variablesReference/name pair will likely fail if name starts to contain a path.

So I suggest to add an optional property evaluateName to the setVariables request:

  /**
   * Optional evaluatable name of this variable which can be used in an
   * 'evaluate' request to set the variable's value.
   */
  evaluateName?: string;

I don't think that we need any new capabilities for this.

@connor4312 @isidorn what do you think?

[isidorn]

I agree this request makes perfect sense and in order to not break anybody a new optional field should be added.
So I am 100% aligned with the proposal.

[gregg-miskelly]

Maybe I am missing something, but couldn't a client already do this if the DA supports SetExpression? Would a new capability make more sense? Or maybe just a DA should say that it supports SetExpression and that it does NOT support SetVariable?

[weinand]

@gregg-miskelly many thanks for reminding us about the setExpression request. It just dropped from my radar because VS Code does not make use of it...

Yes, setExpression is exactly the appropriate request to use when an "l-value" (aka evaluateName) is available.
setExpression uses an "l-value" expression and a frameId to denote the location where to store a value, whereas setVariable uses a variablesReference and a name.

Both requests are controlled by capabilities: supportsSetVariable and supportsSetExpression.

To provide better guidance when to use setExpression instead of setVariable we could clarify the spec with this proposal:

If a debug adapter supports both setVariable and setExpression, a client can freely decide which one to use. A reasonable strategy is to let the client use setExpression if the variable provides a evaluateName and setVariable otherwise.

@gregg-miskelly @connor4312 @isidorn what do you think?

[isidorn]

I was also not aware of the setExpression request, sorry about that.
The new proposal makes sense, that it is up to the client to the decide. However VS Code would follow the recommendation and use setExpression when it can.

[connor4312]

Huh, interesting. I'm curious about the difference between them. setVariable's value simply states it's the name of the value, where setExpression states that value is an expression with access to the scope. However, at least both Python and js-debug ignore this and allow expressions in its setVariable.value, which technically breaks the specification. And there seems to be no official way to set a variable using variables reference with an expression, since setExpression requires an evaluateName.

Should the protocol be refined in some way to avoid this?

[hyangah]

Go debug adapter also allows expressions as setVariable.value which violates the specification. However, the backend debugger doesn't distinguish a simple value from an expression as r-value, so following the spec strictly needs us to do more work on the debug adapter side and I don't see the value.

If vscode supports setExpression, will it be accessible through the 'Set Value' context menu entry?

[weinand]

Thanks for the feedback.

I suggest that we keep the value semantics discussion separate from the original feature request discussed here because we cannot change the semantics of existing DAP requests "en passent". setVariable and setExpression exist for quite some time and my clarification proposal from above does not change anything with respect to the semantics of the value property.

I've created #195 for the value semantics of setVariable.

So I'm asking again:
is the original feature request covered by the clarification proposal from above?
@hyangah @gregg-miskelly @connor4312 what do you think?

[connor4312]

The clarification sounds good to me.

[gregg-miskelly]

A reasonable strategy is to let the client use setExpression if the variable provides a evaluateName and setVariable otherwise.

Both the clr and cppvsdbg debug adapters support all three of these things. In our case, it is more efficient to use setVariable then setExpression -- setExpression requires the debug adapter to reevaluate the expression, while setVariable can use our internal variable objects. setVariable is also nicer because historically evaluateName calculation tends to be a buggy area, so it is nice to avoid it in places where it isn't required. This is why I was originally suggesting that the client should prefer setExpression only when setVariable isn't implemented. That said, I don't feel super strongly about it.

[isidorn]

@weinand Once we settle on this proposal feel free to create an issue for me on the VS Code side so I update how VS Code client behaves.

[weinand]

@gregg-miskelly I've changed the wording of my proposal to make it more clear that it only makes sense for clients to use setExpression if an evaluatable expression is available for a variable via the evaluateName property:

Modified proposal:

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

With this your debug adapter could still control the client's use of setExpression by either providing the evaluateName property or omitting it, correct?

[weinand]

@isidorn I've created microsoft/vscode#124679 for tracking the adoption in VS Code.

[gregg-miskelly]

@Weinard

With this your debug adapter could still control the client's use of setExpression by either providing the evaluateName property or omitting it, correct?

No, because evaluateName is required for other features such as Add To Watch.

[weinand]

@gregg-miskelly since your DA implements both setVariable and setExpression I wonder in what situation you would expect the client to call setExpression?

[gregg-miskelly]

setExpression is used to set the value of a top level item.

[weinand]

VS Code doesn't use setExpression for top level variables. We are using setVariable in that case and pass the Scope as a variable container.

[gregg-miskelly]

Sorry, top level items in the watch window.