Add support for textDocument/inlineValues

[DanTup]

Based on the fact that VS Code added it to vscode.languages and not DAP, my understanding is that this is intended to be provided by a language server (which already has access to parsed ASTs).

This is intended to mirror the equivalent VS Code feature:

https://github.com/microsoft/vscode/blob/c980ea2307905fd1efeee38915bf1e5ccdff1754/src/vs/vscode.d.ts#L2677-L2852

The only deliberate difference is removal of the DAP frame ID since it doesn't seem useful to provide to a language server (although if I've misunderstood that, it's easy to add back in).

I did not include a request to go the other way (like codeLens/refresh) because my assumption is that the client would re-request this when the content changes (and values would not change if the file does not), however if that's not true and we should add a server-to-client inlineValues/refresh request, let me know and I'll add.

@dbaeumer

[lnicola]

the client would re-request this when the content changes (and values would not change if the file does not)

This might be implied by the above, but changes to one file can affect the inline values in another, so the client needs to do this for all the visible documents.

[DanTup]

changes to one file can affect the inline values in another

In that case, I think there should probably be a refresh call (from server->client), the same as CodeLens. If it was intended for the client to request for all visible docs, I'd expect CodeLens would've been the same (and I think it's less clear that this should be done).

I'll add it in - it can always be removed if deemed unncessary.

[lnicola]

Yeah, I guess it can work like semantic tokens where the server can ask the client to refresh them.

The case I was thinking of is that the user edits a function in one module and you want to update the types/hints on the callers.

[mfussenegger]

How is this supposed to fit into the current language server protocol without any concept of application execution lifecycle? Something like "stopped" which is referenced (The document range where execution has stopped.) doesn't exist so far.

[puremourning]

I’m struggling to see what this is for. It seems more targeted at debuggers than language servers. Apologies if I’ve completely missed the point!

[puremourning]

What does ‘look up’ mean? Look up where?

[puremourning]

What is the ‘extracted expression’

[puremourning]

Why a request rather than a notification? Does the server really need a response?

[puremourning]

I did not understand this at all. What ‘execution’ is happening here?

[puremourning]

How would the expression be evaluated? It seems like a lot of this api relies on an execution context like a debugger? I could see this working with DAP but seems out of place in LSP

[DanTup]

How is this supposed to fit into the current language server protocol without any concept of application execution lifecycle? Something like "stopped" which is referenced

My understanding/assumption is that when the editor pauses in the debugger, it would send this request to the language server to get these ranges. With it, it would include the current execution point as an optimisation, since the language server can compute only the ranges up until this point.

note: I'm not proposing anything new here, this is simple adding an existing VS Code API to LSP.

I’m struggling to see what this is for. It seems more targeted at debuggers than language servers. Apologies if I’ve completely missed the point!

I think it is a little confusing because this is a feature for debugging that involves the language server. My understanding is that the goal is to allow the language server (which already has parsed ASTs for a language) to provide regions in a document and their expressions, such that the editors debugger could evaluate those expressions and show inline values as the debugger steps through them.

The original VS Code description is here:

microsoft/vscode#105690

https://code.visualstudio.com/updates/v1_56#_inline-values-by-default-for-some-languages

"With a new debugger extension API, it is now possible for language extensions to provide correct inline value support and we are enabling the Improved inline values feature by default."

So let's say you had the code:

print(foo);

This request allows the language server to return a range around foo that is one of the types described above (text, expression, variable). If it was text then the literal text in the InlineValue would be displayed. If an expression, the editor would evaluate it (via the debug adapter), and if a variable, the editor would look it up from the variables list provided by the debugger.

[puremourning]

ok thanks I think I get it. I would suggest including your explanation and example in the protocol docs. I think saying what an API is for is just as important as the actual messages and types.

Regarding the ‘variable’ and ‘expression’ types, IIRC DAP requires a frame ID to make a variables request or evaluate an expression. How is the LSP client (or DAP client) expected to divine this? I guess that’s not LSP scope and DAP client just has to guess (eg use ‘current’).

I’m a little confused about the refresh operation now. If you’re debugging, then either there is some custom ‘edit and continue’ cycle or changes to the code wouldn’t be effective in the debuggee ? What’s the expected real world case for this?

[mickaelistria]

Here are the 2 main stories for inline values: parameter names and debug value. For the 1st one, the range would be the whole parameter and the inline value would be displayed before, for the later one, the range would be an expression, and the value would be displayed after.
I think the specification needs to explicit this before/after decision.

I also am worried about the idea of creating subtypes for values or expressions. What do they do that couldn't be covered with plain text? I'd rather see a generic enough type that can cover the main use-cases and leave door open to build new use-cases with it than a restricted spec focused on very specific use-cases.

IMO, the refresh notification should be titled needsRefresh or invalidate maybe, to highlight the fact that it's actually not doing any refresh.

[puremourning]

Fwiw DAP already has the invalidate notification for a couple of things like locals etc

[DanTup]

Here are the 2 main stories for inline values: parameter names and debug value

I think Inlay Hints is intended for the first (#956) and this is entirely for debugging. I do think there's a lot of overlap - especially with each one being made fairly generic, but that seems to the the route VS Code has taken (it's already shipped Inline Values and has Inlay Hints as a proposed API) so it makes sense to follow (they have more information on the intended uses than I).

I also am worried about the idea of creating subtypes for values or expressions. What do they do that couldn't be covered with plain text?

Plain text is literals. The others are dynamic values that will be fetched via the debug adapter. Again, this isn't something I've come up with - this is simple a mapping from the VS Code API. Given any LSP implementation would need to map onto available APIs in an editor I don't think it's feasible to really change how this works.

IMO, the refresh notification should be titled needsRefresh or invalidate maybe, to highlight the fact that it's actually not doing any refresh.

I copied the existing CodeLens Refresh request for consistency. It's easily changed, but I don't know if it makes sense to be different to that?

[dbaeumer]

@weinand can you comment on whether inline values will be part of the DAP and why it ended up in the vscode.languages namespace?

[dbaeumer]

Slow response was cause by OOV.

[weinand]

The "inline values" support is new VS Code extension API and not part of the debug adapter protocol (and it never will be). See https://code.visualstudio.com/updates/v1_54#_inline-value-provider-api

The reason for this not being part of DAP, is the observation that DAP already has already mechanisms for getting the value of variables (e.g. the "evaluate" and "variables" requests). The real problem is on the language side: only the "language" knows, what are the interesting variables and where they are located in the source. A debug adapter neither knows what to show and where the variables are located. It only knows the values.

Having extension API like the InlineValuesProvider makes it possible to access both the language smartness (e.g. LSP) and the debugger (DAP) in one place.

BTW, we had the discussion already for the "debug hover" extension API and came to the same conclusion.

[dbaeumer]

Thanks Andre and thanks for the API pointer. Now that I understand how that works it makes sense to have this in LSP since VS Code will do the handshake with the DAP. I was worried that a LS needs to need to talk to a debugger.

[weinand]

@dbaeumer exactly, by using this approach we want to avoid that LSP or DAP have to know each other.

[dbaeumer]

@DanTup would you be willing to add this to vscode-languageserver-node repository in the way we did before, including the MD you provided here. Although this is already final in VS Code we should add this as proposed to LSP to allow other to give feedback as well.

[DanTup]

@dbaeumer yep! Do you have any comments on the other comments/review comments above before I do (for example using a refresh request, etc)?

I'm OOO next week so might not get to this before I return, but I am happy to implement it to see it supported :-) Thanks!

[dbaeumer]

I am not sure if we need a refresh. I am not able to come up with a case where this is necessary since this API is only called while debugging an application.

I think most comments come from the confusion about how this API is supposed to work. It is not meant to provide any real values. They will come from the debugger. It is only used to given the debugger either a name or an expression to evaluate inside the debugged application using DAP protocol.

[weinand]

Yes, from the LSP side a refresh would only make sense if some of the previously provided names or source ranges or expressions are no longer valid because the user has modified the source or some translation process has changed. But this is a typical hot-code-replacement case where VS Code has to refetch the set of variables and their values anyway. So I don't think that a "refresh" is needed/useful.

[weinand]

BTW, Java and PowerShell are not relying on LSP to implement inline values, because for these languages "language smartness" and "debugger" are basically one thing and they do not need a neutral protocol for getting data across.
For TS/LS the TS team and @connor4312 are working on a tsserver plugin for providing the inline values information (see microsoft/TypeScript#43449)

[DanTup]

Yes, from the LSP side a refresh would only make sense if some of the previously provided names or source ranges or expressions are no longer valid because the user has modified the source or some translation process has changed. But this is a typical hot-code-replacement case where VS Code has to refetch the set of variables and their values anyway.

Is it possible you could be modifying a different file to the one that you're debugging that would change an evaluated expression shown where you are debugging? Or will modifying/saving cause all to be re-evaluated? (If so and all clients are expected to do that, should it be noted in the spec?).

Out of interest, how many ranges is it expected a language might return? For example in the code:

var a = b ? c.d : e.f;

Would it be expected that there is just a, or a, b, c.d and e.f? I can see arguments for both ways (the latter could be very busy if the UI is bad, but it would also be really nice when debugging to see all of those values without having to hover over things).

[weinand]

@DanTup
today VS Code will call InlineValuesProvider again if it thinks that some inline values needs to be refreshed. E.g. when a variable was modified by the user in the VARIABLES view of the debugger UI.

VS Code makes no assumptions about the number of items returned from an InlineValuesProvider. The implementation is free to decide. But if there are too many, VS Code might drop some or might use a different presentation style.

[DanTup]

Sorry for the delay! I've opened an initial PR for an implementation of this in vscode-languageserver-node here:

microsoft/vscode-languageserver-node#806

[dbaeumer]

I have merged the implementation into vscode-languageserver-node. Lets see how it goes.

@puremourning your question should be answered here: #1318 (comment)

[KamasamaK]

This was added to the spec in e31b4a2

[DanTup]

Great! Seems like this PR is not required then. Thanks!