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.

[tromey]

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

What should an adapter do if the request comes with both a source and a frameId but they conflict -- i.e., the source names some unrelated file? Perhaps the text could say this is invalid or say which to prefer.

[puremourning]

If debuggers can take a line and column and use that to ‘disambiguate’ a shadowed variable then they can use it to determine a variable at that location. What am I missing?

…

On Wednesday, February 28, 2024, Tom Tromey ***@***.***> wrote: For the hover case it can be applied with a frameId, but I think that makes sense 🙂 Edited the proposal What should an adapter do if the request comes with both a source and a frameId but they conflict -- i.e., the source names some unrelated file? Perhaps the text could say this is invalid or say which to prefer. — Reply to this email directly, view it on GitHub <#465 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQYGDWOD6VOUNUQN52SQODYV5XPXAVCNFSM6AAAAABDVTY4ZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRZGU3DGNJTGE> . You are receiving this because you commented.Message ID: ***@***.***>

[walter-erquinigo]

If debuggers can take a line and column and use that to ‘disambiguate’ a shadowed variable then they can use it to determine a variable at that location.

It depends. Not always the debugger has that information available. For C++ there are different levels of debug info depending on how each file was compiled, so in some cases fine-grained information is available, but in other cases it isn't, even for the same binary. For example, the debugger might know that a variable called x exists, but it might have no idea of its source location. And in other cases, it might know exactly where this variable was declared.

Given this, it would be very beneficial to include the source path along with the line and column, in which case the debugger could even ask an LSP as a fallback mechanism for obtaining the necessary information to disambiguate shadowed variables.

[connor4312]

If debuggers can take a line and column and use that to ‘disambiguate’ a
shadowed variable then they can use it to determine a variable at that
location.

It's also more useful when dealing with expressions. A client might request to evaluate an expression like foo.bar rather than a single variable at a location.

E.g. in chrome devtools since it's convenient :)

What should an adapter do if the request comes with both a source and a frameId but they conflict -- i.e., the source names some unrelated file? Perhaps the text could say this is invalid or say which to prefer.

Yea, in most cases this would be ignored if different from the stackframe, but I could see it being useful if a function from another file was inlined around the current stackframe.

[daveleroy]

What am I missing?

You can hover over any expression not just variables definitions

[tromey]

Yea, in most cases this would be ignored if different from the stackframe, but I could see it being useful if a function from another file was inlined around the current stackframe.

I think it would be good if the spec spelled this out -- that if the frame and the source location disagree, the source location is the one that should be ignored.

In gdb at least, inlining is represented as separate frames. So, the situation you mention should never occur there.

Also, I noticed the patch had an unrelated change to supportsDataBreakpointBytes and I was wondering if that one should be reverted or if it was slated for inclusion eventually anyway.

[connor4312]

I noticed the patch had an unrelated change to supportsDataBreakpointBytes and I was wondering if that one should be reverted or if it was slated for inclusion eventually anyway.

That was a change made earlier in the spec, we just hadn't run the markdown generator for it (JSON is the point of truth)

that if the frame and the source location disagree, the source location is the one that should be ignored.

I would say that an adapter should make a best-effort to evaluate the expression at the location presented when possible, but otherwise it could fall back to evaluating it at the current stackframe, as if there was no location data present.