Skip to content
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

Evaluate requests contains not listed context "variables" #233

Closed
ko1 opened this issue Dec 2, 2021 · 4 comments
Closed

Evaluate requests contains not listed context "variables" #233

ko1 opened this issue Dec 2, 2021 · 4 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug clarification Protocol clarification
Milestone

Comments

@ko1
Copy link
Contributor

ko1 commented Dec 2, 2021

I'm using VSCode 1.62.3.

On the https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Evaluate it only lists 4 contexts:

context?: 'watch' | 'repl' | 'hover' | 'clipboard' | string

but I got variables context when copying a value from a variable pane, without supportsClipboardContext.

image

(copy from the variable o)

Received request:

[VSCode->DA] {"command":"evaluate","arguments":{"expression":"#<Object:0x00007f039108c910>","frameId":1,"context":"variables"},"type":"request","seq":15}
  • Question1: Is it intentional not to list the variable context?
  • Question2: Why not passes variable name in expression if it means to get the result of "variable"? On Ruby language, sometimes the representation of the value becomes "#<Object:...> like above and it is not evaluate-able expression (and this is why I stop to support supportsClipboardContext).
@weinand weinand self-assigned this Dec 14, 2021
@weinand weinand added the bug Issue identified by VS Code Team member as probable bug label Dec 14, 2021
@weinand weinand added this to the March 2022 milestone Mar 1, 2022
@weinand
Copy link
Contributor

weinand commented Mar 8, 2022

@ko1 thanks for the request.

Answer1: not including the variables context value in DAP was "somewhat intentional" but is still an omission we should fix. Here is the full story:

The variables context value is used whenever the evaluate request is used from a "variables" context, e.g. a view showing variables. Since clients typically populate a "variables view" by using the variables request, there are not many situations where an evaluate request is actually used from the "variables view": VS Code was using an evaluate(context: variables) request" only for the "Copy Value" command available for variables. When we added the "Copy Value" command we noticed that a context value variables is not really that helpful when implementing a debug adapter (DA). That's the reason why we introduced the clipboard context which eliminated the last reason for having a variables context.
However, because DAP needs to be backward compatible, we had to protect the clipboard context behind the supportsClipboardContext capability which makes it possible that a DA still sees the unspecified variables context value...

I propose to add the variables context to DAP with the following description:

  /**
   * The context in which the evaluate request is run.
   * Values: 
   * 'variables': evaluate is run in a variables view.
   * 'watch': evaluate is run in a watch.
   * 'repl': evaluate is run from REPL console.
   * 'hover': evaluate is run from a data hover.
   * 'clipboard': evaluate is run to generate the value that will be stored in
   * the clipboard. The attribute is only honored by a debug adapter if the capability
   * 'supportsClipboardContext' is true.
   * etc.
   */
  context?: 'variables' | 'watch' | 'repl' | 'hover' | 'clipboard' | string;

Answer2:
The Variable.evaluateName property determines what gets passed to the evaluate request. If the property exists it is passed as the expression argument. Otherwise the variable's name will be passed .
I suggest that you always set Variable.evaluateName to something that evaluates to the variable's value.

@puremourning
Copy link
Contributor

I propose to add the variables context to DAP with the following description:

Except, now this is not backward compatible... adding a value without adding a capability (the exact reason the new 'clipboard' value was protected by a capability!). Wouldn't it be better if clients currently sending this value just ... stop sending it as there's no legitimate reason for servers to be interpreting that undocumented out of protocol value?

@weinand
Copy link
Contributor

weinand commented Mar 8, 2022

Since context is of type string, I do not see the "backward compatible" argument: any DA has to deal with arbitrary strings anyway. Clients do not have to do anything for adopting this "change".

Adding variables just documents what existing client(s) do anyway.
We cannot ask existing clients to stop using variables because that might be breaking.
We cannot add a supportsVariablesContext for an already used variables context value because old clients will continue to pass the variables context value without checking the capability.

@weinand
Copy link
Contributor

weinand commented Mar 10, 2022

The final description for the context property is:

		/** The context in which the evaluate request is used.
			Values:
			'variables': evaluate is called from a variables view context.
			'watch': evaluate is called from a watch view context.
			'repl': evaluate is called from a REPL context.
			'hover': evaluate is called to generate the debug hover contents.
			This value should only be used if the capability 'supportsEvaluateForHovers' is true.
			'clipboard': evaluate is called to generate clipboard contents.
			This value should only be used if the capability 'supportsClipboardContext' is true.
			etc.
		*/
		context?: 'variables' | 'watch' | 'repl' | 'hover' | 'clipboard' | string;

@weinand weinand closed this as completed Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug clarification Protocol clarification
Projects
None yet
Development

No branches or pull requests

3 participants