Proposal: add a document location to the evaluate request for the 'hover' variant.

[walter-erquinigo]

Problem:

An existing problem is that the 'hover' variant of the evaluate request doesn't contain enough information to distinguish between shadowed variables. Consider the following example:

std::string x = "foo"                 # line A
if(condition) {
  std::string x = "bar"               # line B
  do_something(x);                    # <- currently stopped here
}

If a hover evaluate request for the x in line B is issued, then the debugger will be able to show its correct value ("bar") because that x corresponds to the current frame. However, if the hover happens for the x in line A, the debugger will return "bar" instead of "foo", just because it doesn't know which x the request is for.

Proposed solution

Include an optional Location attribute, similar to the one used by LSP, in EvaluateArguments for hover.

The debugger might be able to easily distinguish shadowed variables using this information, as it knows the location of the declarations of each variable.

[connor4312]

I'm in favor of this. Not all DA's can make use of it but it should be trivial for clients to provide for DA's that can. The DAP way would be adding more information to the evaluate request arguments:

interface EvaluateArguments {
  // ...

  /**
   * The contextual line where the expression should be evaluated. In the 'hover'
   * context, this may be set to the start of the expression being hovered.
   */
  line?: number;

  /**
   * The contextual column where the expression should be evaluated. This should only
   * be provided if `line` is also provided.
   * 
   * It is measured in UTF-16 code units and the client capability
   * `columnsStartAt1` determines whether it is 0- or 1-based.
   */
  column?: number;

  /**
   * The contextual source in which the `line` is found.
   */
  source?: Source;

somewhat related: #343

[walter-erquinigo]

Awesome!
And yes, the proposed addition seems simple and comprehensive enough.

[gregg-miskelly]

If you are going to provide a line and column, shouldn't you probably a source file as well?

[connor4312]

For the hover case it can be applied with a frameId, but I think that makes sense 🙂 Edited the proposal

[puremourning]

this may be set to the start of the expression being hovered.

this is ambiguous ‘may’.

But also, I would prefer to send the actual cursor/mouse position and DA works out the most appropriate expression; this is how LSP hover works.

[daveleroy]

But also, I would prefer to send the actual cursor/mouse position and DA works out the most appropriate expression; this is how LSP hover works.

I don't think most debuggers would be able to find an expression like that they aren't working with the source files like LSP is.

[puremourning]

The same is true of editors.

[daveleroy]

Editors can do it with syntax highlighting, LSP, etc they generally have some understanding of the source files they are working with but the same cannot be said about most debug adapters.

[mfussenegger]

I don't think most debuggers would be able to find an expression like that they aren't working with the source files like LSP is.

Editors can do it with syntax highlighting, LSP, etc they generally have some understanding of the source files they are working with but the same cannot be said about most debug adapters.

If the debuggers don't have the info, then how will adding line and column numbers to the request help them disambiguate shadowed variables?

[connor4312]

DA works out the most appropriate expression; this is how LSP hover works.

This is actually territory of LSP already https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlineValueEvaluatableExpression

In many cases (e.g. arbitrary languages compiled to JS or arbitrary languages being debugged under gdb/lldb) the debugger cannot be expected to have an understanding of the syntax tree of the source language.

If the debuggers don't have the info, then how will adding line and column numbers to the request help them disambiguate shadowed variables?

Independent of syntax, a DA is able to map between locations in the source code and locations in the program that's being executed -- this is needed in order to set breakpoints for example. Whether this is enough to disambiguate shadowed variables is implementation-dependent. I know I could do this for the JS debugger and it sounds like @walter-erquinigo expects to be able to do this for C++ as well.

this is ambiguous ‘may’.

Perhaps 'should' is better -- it's not a "must" because the property is optional

[walter-erquinigo]

At least in the case of my debugger, LLDB, I already support the inline variables feature (e.g. https://marketplace.visualstudio.com/items?itemName=TylerLeonhardt.vscode-inline-values-powershell), which means I already know the location of all variables and their references in the debugger, including shadowed variable disambiguation.
If this proposal gets accepted and line, column, and source get added, doing the evaluate request would be very feasible for me.