Add an optional source and line+column range for a Variable

[gfszr]

A Scope structures contain optional fields describing the source range in which the scope lies in - this is useful for presenting various per-frame/per-scope information overlaying over source code. This request proposes the same for Variable stuctures as well to describe the location where the variable is defined:

{
  /**
   * The source for this variable's definition. 
   */
  source?: Source;

  /**
   * The start line of the range where the variable is defined. 
   */
  line?: number;

  /**
   * Start position of the range where the variable is defined. It is measured in UTF-16
   * code units and the client capability `columnsStartAt1` determines whether
   * it is 0- or 1-based.
   */
  column?: number;

  /**
   * The end line of the range where this variable is defined. 
   */
  endLine?: number;

  /**
   * End position of the range where this variable is defined. It is measured in UTF-16
   * code units and the client capability `columnsStartAt1` determines whether
   * it is 0- or 1-based.
   */
  endColumn?: number
}

Using the above optional information for a variable, a UI can, for instance, overlay its value next to its definition, similar to IntelliJ IDEs' Inline Debugger feature:

[roblourens]

And similar to vscode's "inline values" feature 😁

Is the point of this request to enable that feature? Or improve it?

[gfszr]

Both improving it (Right now, it isn't as good as IntelliJ's one), and allowing usage on editors which do not have strong language-parsing features (Which is the main reason for this request). As this might be either very easy / harder by a debugger to fill this information, it is marked optional.

[weinand]

A debugger typically has no access to an AST (or related symbol information) and therefore doesn't know the source range of a variable declaration. So adding "line+column" information to DAP does not make a lot of sense if most debug extensions could not provide that information. For that reason we have decided not to include detailed source range information in DAP (only exception are scope ranges).

Instead VS Code offers debug related extension API ("providers") for inline values (as shown above in your screenshot) and debug hovers which can be implemented by language extensions. Typical language extensions have access to the AST and can easily determine the source range of language constructs.

[mickaelistria]

A debugger typically has no access to an AST (or related symbol information) and therefore doesn't know the source range of a variable declaration. So adding "line+column" information to DAP does not make a lot of sense if most debug extensions could not provide that information. For that reason we have decided not to include detailed source range information in DAP (only exception are scope ranges).

OK.

Typical language extensions have access to the AST and can easily determine the source range of language constructs.

That is less and less true nowadays with more and more tools being built on a combination of a Language Server and a Debug Adapter. The AST is not accessible to the IDE, nor to the Debug Adapter.
Do I get it right that ideally, the LSP should provide some evaluatableRange: Position -> Range function to allow resolving a position (provided by the IDE eg when hovering or for inlay hints), and that could then be passed to the Debug Adapter?

[weinand]

@mickaelistria yes, LSP should provide some "evaluatableRange: Position -> Range" function.

But I'm not sure that there is a need to pass that information to the Debug Adapter. I think it is more an extension API (of the client) that could use the info from the LSP and combine it with some DAP functionality.

E.g. VS Code uses that approach for the debug hover and for debug inline values: an extension API hook exists that can be implemented by using an LSP, and the DAP evaluate request is used to retrieve the value.
(Since VS Code knows nothing about LSP, LSP cannot be used generically. This is different from DAP which is known by VS Code)

---

You said:

... more and more tools being built on a combination of a Language Server and a Debug Adapter.

I'm aware of the Java extension which combines LSP and DA. What other languages are using this approach?

[mickaelistria]

I'm aware of the Java extension which combines LSP and DA. What other languages are using this approach?

In Eclipse IDE land, we have LSP+DAP enabled for JavaScript, TypeScript, HTML, C# at least. But the issue may also surface without DAP and using other debuggers (hence I agree LSP is the right place to get the expression to evaluate)

[mickaelistria]

@weinand I opened microsoft/language-server-protocol#1596 , your feedback on that proposal would be welcome.

[connor4312]

Reviving this with the merge of #372

I think this makes sense to do. Even when a DA doesn't know about the AST, and Andre says, it can be the case the the locations of certain identifiers as known. For example, V8 gives [[FunctionLocation]] information about all functions in the runtime.

Adapting @vogelsgesang's original proposal with some tweaks to make it fit with our existing methods:

/**
 * A reference to a location in a source file.
 */
interface SourceLocation {
  /**
   * The source for this location.
   */
  source: Source;

  /**
   * The start line of the location. 
   */
  line: number;

  /**
   * Start position of the location. It is measured in UTF-16 code units and
   * the client capability `columnsStartAt1` determines whether
   * it is 0- or 1-based.
   */
  column: number;

  /**
   * The end line of the range. 
   */
  endLine?: number;

  /**
   * End position of the range. It is measured in UTF-16 code units and the
   * client capability `columnsStartAt1` determines whether it is 0- or 1-based.
   */
  endColumn?: number
}

Then a new field on many types:

interface Variable {
  /**
   * The location where this variable is defined.
   */
  location?: SourceLocation;

  // ...
}

interface EvaluateResponse {
  /**
   * The location of definition of the evaluate result, if the result is
   * a variable reference.
   */
  location?: SourceLocation;

  // ...
}

interface OutputEvent {
  body: {
    /**
     * The location of definition of the logged data, if it was derived from
     * a variable.
     */
    location?: SourceLocation;
  
    // ...
  }
}

I don't think we need this on SetExpressionResponse / SetVariableResponse since those two calls won't change the location where the variable is declared. For output, the event can reference variables but itself is not a variable, so I don't think we need that there either.

[gregg-miskelly]

Is the idea here to provide a location for where the variable is defined? Or the value that the variable points at? From the original description I was thinking the former, but from [[FunctionLocation]] comment I thought maybe @connor4312 was referring to the later?

For what it's worth - the debug adapters that I own couldn't provide the former, but they could provide the later for function pointers/delegates, and I would be interested in supporting a "Go To Source" feature from the expression evaluation windows.

[vogelsgesang]

Is the idea here to provide a location for where the variable is defined? Or the value that the variable points at?

I agree with this point. I think we should support both a "declaration location" and a "value location". E.g., for the C++ snippet

void hello() { std::cout << "hello";}
using FuncPtrType = (void*)();
FuncPtrType myFuncPtr = hello;

the variable myFuncPtr would have a "declaration location" in line 3. The "value location" (i.e. the place where its current value was defined) would be in line 1.

As a user, I would find both information useful in the "Variables" view in VS Code. And at least for C++, the DWARF debug info could actually provide both locations.

Hence, I would propose that the interface should support both locations. E.g., something similar to

interface Variable {
  /**
   * The location where this variable is defined.
   */
  declarationLocation?: SourceLocation;
  /**
   * The location where the current value of this variable points to.
   * E.g. for C++/Rust/... function pointers, this should point to the function body of the currently defined.
   */
  valueLocation?: SourceLocation;

  // ...
}

interface EvaluateResponse {
  /**
   * The location where the current value of this variable points to.
   * E.g. for C++/Rust/... function pointers, this should point to the function body of the currently defined.
   */
  valueLocation?: SourceLocation;

  // ...
}

Afaict, there is no use case for declarationLocation inside EvaluateResponse, or am I missing something? Similar for OutputEvent

[connor4312]

Sure, I think that's sensible. I think EvaluateResponse can be left as-is, but we should similarly update setexpression/setvalue

interface Variable {
	/**
	 * The location where this variable is defined.
	 */
	declarationLocation?: SourceLocation;

	/**
	 * The location where this variable's value is defined. For example, if the variable points to a
	 * function, this should be set to the location of that function.
	 */
	valueLocation?: SourceLocation;

	// ...
}

interface EvaluateResponse {
	/**
	 * The location where the value returned from the `evaluate` request is defined. For example, if
	 * a function pointer is returned, this should be set to the location of that function.
	 */
	location?: SourceLocation;

	// ...
}

interface SetExpressionResponse/SetVariableResponse {
	body: {
		/**
		 * The location where this variable's new value is defined. For example, if the variable points to a
		 * function, this should be set to the location of that function.
		 */
		valueLocation?: SourceLocation;

		// ...
	}
}



interface OutputEvent {
	body: {
		/**
		 * The location of definition of the logged value.  For example, if a function pointer is
		 * logged, this should be set to the location of that function.
		 */
		location?: SourceLocation;
	
		// ...
	}
}

[gregg-miskelly]

I am assuming that many clients will not always use declarationLocation (ex: useful if they support in-editor value display, but probably not useful in a 'locals' type window) and that for some debug adapters, this information isn't cheap to obtain. So we might also want a field on the request to indicate if declarationLocation is wanted.

[connor4312]

for some debug adapters, this information isn't cheap to obtain.

If this is the case for some implementors, we should probably have the lookup deferred to a separate, explicit request from clients. I'm curious if we have any know adapters where this is the case.

[vogelsgesang]

declarationLocation [...] probably not useful in a 'locals' type window

I would find a "go to declaration" action action in the right-click menu on local variables useful. In particular when I expand the this pointer of a member function, and navigate to one of the member variables, going to the definition of that member variable is useful

have the lookup deferred to a separate, explicit request from clients

👍 sounds like a good solution, in case this is actually a problem for some adapters

[vogelsgesang]

I just noticed, that lldb actually already has a custom extension to expose the declarationLocation, see https://github.com/llvm/llvm-project/blob/main/lldb/tools/lldb-dap/JSONUtils.cpp#L1149. This was added in llvm/llvm-project#74865, the pull request also contains some reasoning / use cases

[gregg-miskelly]

If this is the case for some implementors, we should probably have the lookup deferred to a separate, explicit request from clients. I'm curious if we have any know adapters where this is the case.

The debug adapters that my team owns wouldn't be able to easily obtain the declaration location, but we could potentially work with a source file parser (probably the language service) to go from the information we do have to the declaration information.

[connor4312]

Sounds good, I think that's convincing. Likewise for JS/TS I could probably determine the declaration location of locals by extending some of the smartness I do for scopes today, but it would not be free.

In that case I would introduce a new type of reference, with a lifetime shared with variable references.

interface Variable {
	/**
	 * A reference that allows the client to request the location where the variable is declared.
	 * This should be present only if the adapter is likely to be able to resolve the location.
	 * 
	 * This reference shares the same lifetime as the `variablesReference`.
	 * See 'Lifetime of Object References' in the Overview section for details.
	 */
	declarationLocationReference?: number;

	/**
	 * A reference that allows the client to request the location where the variable's value is declared.
	 * This should be present only if the adapter is likely to be able to resolve the location.
	 * 
	 * This reference shares the same lifetime as the `variablesReference`.
	 * See 'Lifetime of Object References' in the Overview section for details.
	 */
	valueLocationReference?: number;

	// ...
}

interface EvaluateResponse {
	/**
	 * A reference that allows the client to request the location where the returned value is declared.
	 * For example, if a function pointer is returned, the adapter may be able to look up the function's location.
	 * This should be present only if the adapter is likely to be able to resolve the location.
	 * 
	 * This reference shares the same lifetime as the `variablesReference`.
	 * See 'Lifetime of Object References' in the Overview section for details.
	 */
	locationReference?: number;

	// ...
}

interface SetExpressionResponse/SetVariableResponse {
	body: {
		/**
		 * A reference that allows the client to request the location where the variable's new value is declared.
		 * This should be present only if the adapter is likely to be able to resolve the location.
		 * 
		 * This reference shares the same lifetime as the `variablesReference`.
		 * See 'Lifetime of Object References' in the Overview section for details.
		 */
		valueLocationReference?: number;

		// ...
	}
}

interface OutputEvent {
	body: {
		/**
		 * A reference that allows the client to request the location where the logged value is declared.
		 * For example, if a function pointer is logged, the adapter may be able to look up the function's location.
		 * This should be present only if the adapter is likely to be able to resolve the location.
		 * 
		 * This reference shares the same lifetime as the `variablesReference`.
		 * See 'Lifetime of Object References' in the Overview section for details.
		 */
		locationReference?: number;
	
		// ...
	}
}

// Reference lookup:

interface LocationReference {
	body: {
		/**
		 * Location reference to look up.
		 */
		locationReference: number;

		// ...
	}
}

interface LocationReferenceResponse {
	// same as what would have been in 'SourceLocation'
}

[vogelsgesang]

In that case I would introduce a new type of reference, with a lifetime shared with variable references.

Do we even need new reference ids? Or could we reuse the existing variablesReference? Variable, EvaluateResponse, SetExpressionResponse, SetVariableResponse and OutputEvent all already have a variablesReference.

Repurposing the variablesReference id, it would be sufficient to only introduce a new location lookup functionality, without changing any of the other types:

// Location lookup:
interface LocationLookup {
	body: {
		/**
		 * Location reference to look up.
		 */
		variablesReference: number;

		// ...
	}
}

interface LocationLookupResponse {
	/**
	 * The location where this variable is defined.
	 */
	declarationLocation?: SourceLocation;
	/**
	 * The location where the current value of this variable points to.
	 * E.g. for C++/Rust/... function pointers, this should point to the function body of the currently defined.
	 */
	valueLocation?: SourceLocation;
}

[gregg-miskelly]

Or could we reuse the existing variablesReference?

No, because variableReference is used by the protocol to indicate a variable supports expansion. We could add an evaluateResponseReference (see #134) and flags to indicate if the resource supports declaration/value locations

[vogelsgesang]

Makes sense, thanks for clarifying