microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
11.23k stars 798 forks source link

Support semantic highlighting #18

Closed hackwaly closed 3 years ago

hackwaly commented 8 years ago

Like WebStorm and VS does: eg. Symbol is a type or parameter or namespace or unresolved ...

Textmate based grammars are hard to do this. Since we did support Diagnostics, why not support semantic highlighting?

kaby76 commented 4 years ago

I have created a proposed.semanticTokens.md gist ..

This looks good from my perspective (VS2019/Antlr). It's similar to what I've implemented via custom messaging and ITaggers's in VS2019. It'll be an easy change for me to switch the code over to the proposed semantic highlighting API if and when the LSP APIs are updated (with emphasis on the if).

This may be a stupid request, but I would like to see the purpose of LSP semantic highlighting elucidated to the point where I, a moron, can understand. When I first implemented my LSP server, I was a little surprised that the text in the VS2019 textview was completely monochrome. After all, a document symbols request assigns a type ("variable", "class", "method", etc) to a range of characters throughout the document. It could have been used for colorization of the text where the client defines apriori the display style for the type. Since it's clear the information in the semantic highlighting call is a superset of the information in a document symbols call, one could ask why are they are separate calls. What I'm looking for is a short description of what the client is going to use it for, something like this:

LSP semantic highlighting is for noting to the client a list of type and range of characters in the text a static display style in a text view.

LSP ‘textDocument/documentSymbol’ is for noting to the client a list of type and range of characters in the text whether the client can allow “go to def”, “find all refs”, … (???) for that text in a text view. (Note that there are separate API calls "go to def", "find all refs" to provide further information.)

LSP ‘textDocument/documentHighlight’ is for noting to the client a list of type and range of characters in the text so that the client can apply a dynamic highlighting style to the text, i.e., when you click on a symbol in the text.

I have still no idea what LSP ‘textDocument/documentColor’ is for, but now obviously not for “semantic highlighting”.

You can wordsmith this to whatever you feel, but my point is that it's absent from the LSP spec at each call. I imagine the response will be "Oh, we can't do that because it boxes in the versatility of LSP.' But, the fact that someone would ask "deprecating documenthighlight and renaming it to expressionhighlight, occurrencehighlight, or something different" is an indication of something wrong.

Respectfully, Ken Domino

ghost commented 4 years ago

If I may give a concrete idea for SemanticTokensEdit:

Why not something like edit: {rangeStart: 0, rangeLen: 3, property: "line", valueShift: 1} to move tokens 0,1,2 down by one line, instead of: edit: { start: 0, deleteCount: 1, data: [3] } as given in the spec. valueShift would be relative(!) value applied to the given "line" property. There could be an alternative value field applying an absolute value that also allows strings to e.g. set the "tokenType" in bulk.

Wouldn't this cover most practically relevant edits in a really short JSON string still?

we have decided to avoid allocating an object for each token and we represent tokens from a file as an array of integers

As a note to this, while for storage of the file as a whole this format might make sense that doesn't necessarily make it a good match for edit transfers.

Edit: I made a separate ticket for this now: https://github.com/microsoft/language-server-protocol/issues/979

HighCommander4 commented 4 years ago

LSP ‘textDocument/documentSymbol’ is for noting to the client a list of type and range of characters in the text whether the client can allow “go to def”, “find all refs”, … (???) for that text in a text view. (Note that there are separate API calls "go to def", "find all refs" to provide further information.)

[...]

I have still no idea what LSP ‘textDocument/documentColor’ is for, but now obviously not for “semantic highlighting”.

My understanding is:

I do agree that the purpose of requests isn't always clear from the spec, and I've sometimes had to look at example usages in clients or servers to understand the intent. Perhaps illustrative examples of intended editor features could be provided in the spec, without constraining what the requests can be used for (as they are just examples).

kjeremy commented 4 years ago

I do agree that the purpose of requests isn't always clear from the spec, and I've sometimes had to look at example usages in clients or servers to understand the intent. Perhaps illustrative examples of intended editor features could be provided in the spec, without constraining what the requests can be used for (as they are just examples).

This is as close as I've found for something like that: https://code.visualstudio.com/api/language-extensions/programmatic-language-features

kaby76 commented 4 years ago

I do agree that the purpose of requests isn't always clear from the spec, and I've sometimes had to look at example usages in clients or servers to understand the intent. Perhaps illustrative examples of intended editor features could be provided in the spec, without constraining what the requests can be used for (as they are just examples).

This is as close as I've found for something like that: https://code.visualstudio.com/api/language-extensions/programmatic-language-features

Folks, maybe the spec should have definitions of the UI terms along with the purpose of the API method, starting with "semantic highlighting"? I never understood what this "hierarchy" was all about in LSP for document symbols, so I just return a SymbolInformation[]. But I now read that it's for "outlining". Is that as in the "outlining" controls that somehow magically appeared in the left margin of a textview, e.g., the squares with + or -, with a vertical line that extends down, that show and hide code when toggled even though the spec says I specified a FLAT hierarchy using SymbolInformation[]? Or, maybe it's "outline" as in a tree displayed in the Solution Explorer? I'm taking this on blind faith that "semantic highlighting" is going to colorize my text. I hope you see my point.

dbaeumer commented 4 years ago

Wow, lots of comments :-).

@rcjsuen THANKS a lot for writing up the markdown. I will have a look beginning of next week. We are in endgame right now.

rcjsuen commented 4 years ago

Clients should declare a) if they support additive/merging support of semantic tokens on top of whatever internal grammar it has already and b) if they support discarding the grammar from A and replacing it completely with the semantic tokens information from the server.

IMHO 2B should just be mandatory for clients that support the new semantic highlighting protocol. (2A can be optional, of course.) I don't see why it would be difficult for anyone to implement, and not having it can obviously mess up the result as seen in microsoft/vscode-languageserver-node#570 .

On the topic of grammars vs semantic highlighting, I've encountered another issue in VS Code that's somewhat related to this topic due to how it chooses to trigger code completion (https://github.com/microsoft/vscode/issues/95924#issuecomment-620181834).

dbaeumer commented 4 years ago

I commented in #979 about the format and why we choose it. And there was a lot of measurements going on to end up with that format.

Here a short recap of why we ended up there:

  1. encoding the delta should be easy (no knowledge of what is delta encoded). In the current implementation the delta is simply the delta of two number arrays. In a previous implementation we ask to create a real semantic delta for tokens but the first tester ask to implement it (TypeScript) basically failed since it got to complex.

  2. delta encoding is used to save space on the wire and to reduce object allocation on the client.

  3. simple edits (new characters on the line, new lines) should result in a small delta.

  4. & 3. resulted in encoding a line relative to the previous line.

Additional thinks I read from the comments we should add:

        /**
         * Capabilities specific to the `textDocument/semanticTokens`
         *
         * @since 3.16.0 - Proposed state
         */
        semanticTokens?: {

                        // .....

            /**
             * Whether the client has its own tokenizer (e.g. TextMate)
             */
            tokenizer?: {
                /**
                 * Which strategy the client uses if both client and server tokenize.
                 */
                strategy: 'merge' | 'replace';

                /**
                 * If strategy is `merge` which token wins if both client and server
                 * provide a token for a range.
                 */
                conflict?: 'clientWins' | 'serverWins';
            }
                }

I am not sure if we would still need mergeRequired. What is the use case of letting the server decide this on every response.

ghost commented 4 years ago

@dbaeumer but wouldn't that complexity only exist when needing arbitrary document deltas? However, doesn't all of that go away when only looking at what the user can reasonably change? (insert of X tokens at a single point, deletion of X tokens at a single point) To me it seems like sending the changes resulting from that is actually fairly easy, unless I'm missing something. What's the scenario where this is so complex?

Sorry if I'm dragging this out unnecessarily, but I would really be interested in understanding what exactly you tested and why, if my questions aren't too much trouble. I understand arbitrary diff'ing is hard, but I just don't get why this would ever be needed here

Edit: I'm also just noticing, if the encoding helps people write a generic differ why not suggest it as internal conversion for those who need help with this? That still seems a bit odd why it'd need to be in the protocol just for that reason

dbaeumer commented 4 years ago

Regarding

I'm also just noticing, if the encoding helps people write a generic differ why not suggest it as internal conversion for those who need help with this?

We discussed this but decided against it. We think that it is easier to understand if the delta encoding happens on the request level. It gives more flexibility. What we do look in parallel is to support a different content encoding on the wire (message pack versus JSON).

Changing code at one place can change semantic tokens at many place in the same file even in other files. So for implementors (from the feedback) it seemed the easiest to always travers the full file (or a predefined given range) and create the semantic tokens for it. We think the easiest diffing is then happening afterwards using the old and new result, which BTW is optional in the protocol. If memory is an issue a builder can be used to compute the diff on the fly.

I also want to point out that we worked on this for months now being on our iteration plan since November 2019 (https://github.com/microsoft/vscode/issues/83930) with quite some feedback and discussions form LSP and VS Code implementers.

puremourning commented 4 years ago

I also want to point out that we worked on this for months now being on our iteration plan since November 2019 (microsoft/vscode#83930) with quite some feedback and discussions form LSP and VS Code implementers.

Where was this discussion held ? I would have thought this forum (the LSP repo) PRs/Issues to be the obvious place to have the discussion.... which i guess is what we're doing. I'd be surprised if anyone participating in this discussion here is not a LS or LSP client implementor.

We need to remember that there is more than 1 client of LSP, VSCode is not the only one that people care about.

ghost commented 4 years ago

and discussions form LSP and VS Code implementers.

Were all of them using JavaScript or TypeScript? I can understand it is easier to encode it like this for these languages where JSON objects are kind of "native", but I think all others this is likely a nuisance rather than a help, especially in languages like C++/Rust/Go/... with fast native types that work differently.

I also think the protocol shouldn't be trying to somehow teach me an internal format, it sounds like all of this should go into the appendix for those people who need help with implementing this efficiently. I think the protocol should focus on clarity on what happens with each protocol event (which I think the encoding right now works highly against) as long as this doesn't impact transfer speed too much (which from the sort of edits/shifts that will happen with real world typing input I don't think will be a problem, but that is why I asked for numbers and what exact scenarios you tested!). Beyond that, the protocol should IMHO leave the implementation details to the authors.

Summed up, to me it just feels a mix of purposes for me, things that don't belong together, maybe even a hack to make it easier for one group of people (JS/TS users) while sacrificing universality and ease of understanding of others. I don't think that is the right way to go, personally. But I'll happily bow if the majority disagrees with me, I just wanted to put my voice out there.

(FWIW I implemented an LSP server in the past and I plan to implement one again soon. So I also count myself as an implementer)

Edit: just as an additional note:

We think the easiest diffing is then happening afterwards using the old and new result, which BTW is optional in the protocol.

My assumption is that supporting partial edits and according events is not optional in practice for large files if any acceptable performance is to be reached. I think most LSP server-side implementers want to support that use case. So I don't think arguing that people can just not implement this protocol part is useful here, personally.

dbaeumer commented 4 years ago

@puremourning sorry for that. Because of all the feedback we got in other issue I somehow assumed that this is know more broadly. Is there anything in the current proposal that makes it hard or impossible for you to implement that client side?

puremourning commented 4 years ago

@dbaeumer well, in the case of vim and/or neovim (i guess) the whole text-properties thing means that highlighting applied moves with the text and doesn't require being constantly updated. so a lot of this delta stuff might actually be worse for client performance (as it might be redundant).

But if i'm completely honest, it requires a lot more thinking than i've actually done on it. I would say that the protocol in its current form doesn't appear to lend itself to be easily implemented in any client for Vim (or vim-a-likes). But that's more of a gut feeling than a technical analysis.

My original comment was intended to understand the reasoning, which you've explained. I would tend to agree in a philosophical sense that designing a protocol around what's good for one particular client and it's particular quirks (such as garbage collection) is probably not the ideal situation, but i also completely understand why that would be, and can see that you've applied some practical engineering which makes sense.

I would be interested in hearing the thoughts of @chemzqm and @prabirshrestha (who i understand are the authors other prominent vim clients). In particular i understand that @prabirshrestha may have already implemented this proposal

ghost commented 4 years ago

so a lot of this delta stuff might actually be worse for client performance (as it might be redundant).

That is a curious point, and I also wondered if it might be easiest to just send insert-token-at events - even my simplified proposal still implies manual explicit range shifts, albeit without the encoding. It's indeed a bit weird the protocol currently assumes token ids need to be shifted around in larger ranges in the first place. But it's hard to judge the actual perf details without trying it out (and sadly my code base isn't ready right now, that is quite unfortunate)

kjeremy commented 4 years ago

FWIW in rust-analyzer we do not support token edits, we just support full document and range and that works well enough.

ghost commented 4 years ago

@kjeremy interesting, but wouldn't that force you to resend the entire token range after any insert in the middle of the document fully with all the token attributes etc? That sounds like it might develop into a problem when typing at the very top of a long file, unless the editor/LSP client is smart enough to discard these areas quietly and refetch them only for the viewport and then gradually in the background for the invisible further down areas. But if the assumption is LSP clients do that, why do the token edits/deltas even exist?

dbaeumer commented 4 years ago

@puremourning I am a little bit lost now since the proposal basically boils down to more or less the same as in the early PR you linked. At end end a server reports a token type (class function) and set of token modifiers (const, async, ...) for ranges in a document. Are you saying that this concept can't be applied to VIM?

ghost commented 4 years ago

I don't want to dominate this discussion, and hoped there would be more input other than mine up to now (for any viepwoint really). But since nobody else jumped in, I've had time to reflect:

So it seems to me like right now, there are two ways to get good performance on a large file: 1. implement ranges server-side, and hope the LSP client will be smart enough to not instantly requery the entire file after an insert but rather in some gradual way (e.g. viewport-based + otherwise bit by bit refetching), 2. implement semantic edits server-side, and hope the LSP client will use it for really minimal deltas. I assume approach two gives even better performance, but not significantly so.

Therefore, this feels slightly duplicate to me. This is why I think it is problematic:

  1. Somebody writing a client could not implement smart ranges handling (or not use ranges at all) and could expect token edits deltas to be supported server-side if any good performance is desired,

  2. And somebody writing a server could skip token edits given they might not provide a huge boost in something like C++, where the integer encoding might not be a good fit to internals, and assume the client will be smart with ranges if needed.

Now both did "their part" to facilitate good performance, and it's still bad in the end result. That sounds undesirable to me, and it does also confuse me why both approaches are even needed for such a similar activity.

To solve this, I think one of the solutions should be marked as "mandatory when expecting reasonable performance with a common implementation," while the other marked as "icing on the cake" and maybe even moved out into some super-optional extension. I don't really care which one is preferred, but it might make sense to decide that first. Because IMHO the moved out one doesn't really need to be that understandable anyway, if people can just reasonably skip it.

I would however suggest that SemanticTokenEdits feels like a very specific approach that seems to only mainly benefit implementors that use the actual JSON objects for storage, so basically scripting languages that heavily use more generic key->value maps and generic list similar to JSON's object and array. That probably does not apply to the majority of implementors, so it does feel more alien to me than the ranges. But that is obviously just my personal opinion. I read things wrong here, after rereading it is obvious to me the "more alien" encoding is used both for ranges and token edits. So from that angle it doesn't really matter which method is the preferred one, but as noted below, not making deltas ever desync could be way more bug prone than simple ranges.

My apologies if I misunderstood how ranges and token edits work and I drew wrong conclusions, in that case please disregard my entire comment. (Will happily go back and edit all of this to strikethrough if that turns out to be the case)

axelson commented 4 years ago

I'm in agreement with @etc0de that with the current proposal could easily run into performance problems with certain client/server combos. I'm not sure that the textDocument/semanticTokens/edits request is worth the implementation effort, it seems very likely to not be fully adopted by all servers, and it seems potentially error-prone to generate and maintain (e.g. it could get out of sync easily).

Another issue that maybe I'm not understanding, but it seems like only giving theme creators a single set of tokens, with some modifiers applied might be limiting. I would expect that an approach with more nesting would be more useful because the theme could be used across more languages and more easily customized by end-user's. An example of the nesting that I'm talking about is: "string.quoted.double.interpolated.elixir" which lets the theme/user distinguish between strings with single quotes and those with double quotes.

Also is it possible to add module to the list of default token types? It would be helpful for languages that don't have classes.

ghost commented 4 years ago

Also is it possible to add module to the list of default token types?

I think the idea is that the token types don't match the exact keyword name but the "spirit" of what the item is. E.g. class would then be used for any sort of multi-instantiable base type (like a prototype object in javascipt, or I assume a module in elixir if that behaves similarly?). Or if a module is just a grouping of functions under a name without an instantiated OOP/object metaphor, namespace would probably be a good match. Or is the module you think of not similar to any of these two?

Edit: also please note you can derive names. So you can send a module token type that is derived from (= colored like) a class type.

axelson commented 4 years ago

Yes, in Elixir a module is most similar to a class (a grouping of functions, but without any state). Are there any examples of how deriving token types works? I didn't see an example of that in the proposal.

ghost commented 4 years ago

Documentation is still a bit poor at the moment, but fwiw here is the relevant note of the token types not being fixed but extensible e.g. via client capabilities: https://github.com/microsoft/vscode-languageserver-node/blob/e5d7ad881b1a51b486e3f0e4aa0fbc25dad2be58/protocol/src/protocol.semanticTokens.proposed.ts#L12 and I think the idea from a LSP server side is to specify new tokens in SemanticTokensLegend in the "tokenTypes" key offered by the server, although I'm not sure how exactly to specify what base token type each derives from.

@dbaeumer would you happen to have a small JSON example of how specifying a new server-specific "module" token type that maps to a "class" base type in the protocol would look like?

ghost commented 4 years ago

Fwiw, I just reread the specs and realized the delta integer encoding is also used for ranges, or initially sent tokens in whatever case, not just the delta edits. I was mostly stumped because I didn't understand why bother for short deltas with this complicated encoding, but for longer initial data ranges I have to agree that the data savings might be worth it.

So with that, I am really only left wondering why there is both ranges and delta edits (since the latter seem barely worth as a separate almost-doing-the-same-job thing) and I'm not really opposed to the short integer encoding as a whole anymore, apart from the weird deltaLines. I can see why streaming larger ranges this might be worth it in integer encoding, as ugly as it looks like. Sorry that it took me so long to understand that part correctly.

I think deltaLines & the whole delta edit thing might be worth to just not have in there. I don't think having otherwise the short integer encoding but absolute line numbers would increase the initial send a lot, and it seems to me the delta edits are way more complicated than resending of plain ranges with possibly little gain.

kjeremy commented 4 years ago

@etc0de while this doesn't answer your question the encoding stuff clicked for me when I saw this implementation: https://github.com/microsoft/vscode-languageserver-node/blob/50a56c01328699a3357cfb3726e8e2a05f67f30d/server/src/semanticTokens.proposed.ts#L45-L149

The implementation we use (full document and range only) is here: https://github.com/rust-analyzer/rust-analyzer/blob/b65d6471ca809b567b6045f987ae1bdee2849423/crates/rust-analyzer/src/semantic_tokens.rs#L88-L128

dbaeumer commented 4 years ago

Sorry for the long silence but I was heads down with implementation stuff in LSP.

I think there is still confusion about the idea behind all of this. So I will try to summarize it:

Everything else are optimizations to either speed up semantic coloring in the UI or to minimize the payload on the wire.

We left the combination (range, delta encoded) out since a range based result is usually small, only requested once and therefore the saving is minimal.

To make things more clear I propose the following renames:

textdocument/semanticTokens/full: request to get all semantic tokens of a text document. textdocument/semanticTokens/range: request to get all semantic tokens of a specific range. textdocument/semanticTokens/full/delta: to get the delta for a full document. textdocument/semanticTokens/range/delta: we could spec it but I guess given the above most servers will not bother implementing this.

I am also willing to spec a format using absolute positions in case a server doesn't want to implement the delta requests. However my fear is that server will start with this not thinking about the payload size and afterwards complain about things being slow. But in any case I will foresee different formats which the client can announce in case we want to extend this in the future.

So my question is: is there a big desire for a absolute position format. The relative encoded format can easily be converted into a absolute one. It is simply adding numbers :-) while iterating over the array of numbers.

ghost commented 4 years ago

So my question is: is there a big desire for a absolute position format.

I think using absolute numbers only makes sense if the delta variant is just removed. If at all the protocol needs less choices, not more. A client could even request less than the viewport on a large screen to gradually update even the visible parts, so honestly I'm a bit surprised the delta format is that important for performance especially with the already shortened integer encoding. And apart from the delta's problems with being hard to understand, and duplicating what ranges do and risking client/server combinations having poor performance, I agree with the previous note that implementing it without ever desyncing might also be hard and result in buggy servers. Is it really worth it?

However a client still needs to get the semantic tokens for the full file to avoid color flickering when scrolling and to be able to color the minimap.

This could also be done in gradual range chunks in the background which would also bring down payload, and while not completely eliminating any possibility of flickering it would likely remove it in many cases. It doesn't seem to be necessarily a given that delta is needed for this, if that was the reason it was added. And a minimap updating with a slight delay doesn't seem like the end of the world to me, but that's just my opinion, obviously

puremourning commented 4 years ago

@dbaeumer thanks for the (first?) clear explanation about how this all works :)

i think the 'delta encoding within one array of ints' is probably fine on reflection; as you say it's trivial to implement (though it still feels like a strange optimisation to me - do you have numbers on the payload differences for real documents from your testing ?).

On a somewhat different point, what was the motivation for the "delta" request to be client-to-server rather than a server-to-client notification (e.g in response to document change notifications) ? What is the expected client behaviour ... to send a delta request after each document change ?

kjeremy commented 4 years ago

To make things more clear I propose the following renames:

textdocument/semanticTokens/full: request to get all semantic tokens of a text document. textdocument/semanticTokens/range: request to get all semantic tokens of a specific range. textdocument/semanticTokens/full/delta: to get the delta for a full document. textdocument/semanticTokens/range/delta: we could spec it but I guess given the above most servers will not bother implementing this.

While not opposed to the renames (except that it means I have to change some code :)) they seem to go against the typical convention.

axelson commented 4 years ago

I am also willing to spec a format using absolute positions in case a server doesn't want to implement the delta requests. However my fear is that server will start with this not thinking about the payload size and afterwards complain about things being slow. But in any case I will foresee different formats which the client can announce in case we want to extend this in the future.

I don't think we need absolute positions. I think the integer-based encoding makes sense for minimizing the payload on the wire, and since all the needed information is available in a single request which it should be relatively straightforward to implement.

dbaeumer commented 4 years ago

@kjeremy can you elaborate why you think this. I am open for a better proposal. So far we didn't have the problem of a full and range request of the same type.

@puremourning LSP whenever possible uses a pull model (they only exception are diagnostics) driven by the client. The reasons are as follows:

What is the expected client behaviour ... to send a delta request after each document change ?

No the client will pull for the delta assuming that the server will provide a delta request.

puremourning commented 4 years ago

Thanks again. Makes sense for the client to determine which buffers it wants tokens for, but...

What is the expected client behaviour ... to send a delta request after each document change ?

No the client will pull for the delta assuming that the server will provide a delta request.

Sorry i didn't follow this... When would you expect the client to poll (pull) the textdocument/semanticTokens/full/delta ? Is the expectation that clients just do it periodically ? Or after all changes in the text document (e.g. at same time as didChange notification?)

If i understood correctly, the general flow would be:

FWIW - What I was expecting is something more like "register for tokens for file" (returning a snapshot of the full file tokens, followed by updates) until "unregister for tokens for file". So it's "Pull" as a query snapshot+updates paradigm with i guess a "cancel" mechanism. Feels more real-time that way to me, rather than the client polling for updates.

I realise that's a fairly meaningful departure in approach, and there probably isn't precedent for it in the existing protocol, but this is a new feature, and i think the success of this feature will entirely depend on the snappiness/promptness of UI updates. Many of us will have experienced unacceptable lag/"highlight popping" in existing semantic highlighting systems, so i'd certainly be curious which approach yields better feel in various clients. Hard to know without implementing both of course, and nobody wants that! :)

dbaeumer commented 4 years ago

@puremourning regarding your questions:

Does the client also send the textdocument/semanticTokens/full/delta alongside each didChange notification ?

Yes, but may be with a less intense frequency. We are also looking into adding batching support to make this more performant for servers (https://github.com/microsoft/language-server-protocol/issues/988)

FWIW - What I was expecting is something more like "register for tokens for file" (returning a snapshot of the full file tokens, followed by updates) until "unregister for tokens for file". So it's "Pull" as a query snapshot+updates paradigm with i guess a "cancel" mechanism. Feels more real-time that way to me, rather than the client polling for updates.

The problem with this approach is that the server doesn't know when to stop pushing. If the file is not visible anymore there is no need for the server to re-compute semantic tokens. Please note that the open/close calls are no indication for visibility. A document can be visible in the UI without the client every sending a open event. Open/close events are a ownership indication.

puremourning commented 4 years ago

The problem with this approach is that the server doesn't know when to stop pushing.

I was suggesting that the client "un-registered" for the updates with another message, e.g."

ghost commented 4 years ago

I don't think we need absolute positions. I think the integer-based encoding makes sense

Just to explain this again since apparently multiple people were confused, my related suggestion was to use absolute positions only and otherwise stick with integer coding, but scrap the delta stuff entirely. I agree offering absolute position as some sort of option is kind of pointless since all my suggestions aim to reduce the current protocol complexity, not increase it further.

On the same note I think the push model might make sense to make the delta stuff work better, but I think it might still be better to just remove it entirely and just use ranges only.

kjeremy commented 4 years ago

@kjeremy can you elaborate why you think this. I am open for a better proposal. So far we didn't have the problem of a full and range request of the same type.

Only that this would be the first case with three or more levels of specification textDocument -> semanticTokens -> full/range. It's just something I view as inconsistent (or maybe just new?) but I don't have a better proposal.

rcjsuen commented 4 years ago

@kjeremy can you elaborate why you think this. I am open for a better proposal. So far we didn't have the problem of a full and range request of the same type.

@dbaeumer @kjeremy What about the formatting requests?

matklad commented 4 years ago

Just want to report that we at rust-analyzer are pretty happy with the wire format. Relative positions and edits make total sense (albeit only after "In addition we specified that data structure in a way that computing the delta is done on a number array without any knowledge of the data's semantic" comment), I don't think we can improve this further, short of swapping JSON for protobuf.

ghost commented 4 years ago

For what it's worth, I am happy to be ignored if the conclusion is I'm the minority thinking the delta edits mostly just double the ranges and are more trouble than worth adding. It is at the end of the day more like an annoyance, not a total showstopper. However, I think a note saying which of ranges and deltas should be preferred by both sides when expecting good perf to avoid the "performance deadlock" I described above would be very useful if both mechanisms are kept.

Pyromuffin commented 4 years ago

Maybe I missed it, but what is the plan for the situation referenced

for a server to effectively compute these notification it needs to have a basic understanding of the UI model which I think we should never sync to the server. Consider the case that a change if file A changes semantic coloring in file B. Should a server always send an event for file B even if the file is not visible in an editor. It might be expensive to find out if B is affected.

Currently using the vscode proposed features, when editing a header file, i get stale semantic tokens in any file that includes it. I'm not sure if I should do something on the server to tell the client to re-request tokens for a file or if vscode should be re-requesting tokens regularly. The client doesn't have any knowledge of the actual semantics of the language, so i wouldn't expect it to know when a file needs to be refreshed.

dbaeumer commented 4 years ago

Regarding https://github.com/microsoft/language-server-protocol/issues/18#issuecomment-635986774

I agree, but today I would make the textDocument/formatting/range textDocument/formatting/full and textDocument/formatting/onType.

Especially around semantic tokens I think it does even make more sense to have an extra level since the requests are not independent of each other.

dbaeumer commented 4 years ago

Regarding https://github.com/microsoft/language-server-protocol/issues/18#issuecomment-635861405

We really try to stay away from syncing UI state over to the server or to ask to implement a subscribe / unsubscribe model. Major reason is that this that this gets complicated to implement in a remote scenario or if we make servers multi tenant. To allow for this evolution path we decided to whenever possible use a pull model over the push model since a client can easily recover from network failures. In the semantic token case if the network goes down the client and simply ask for all semantic tokens again.

dbaeumer commented 4 years ago

Regarding using only ranges in https://github.com/microsoft/language-server-protocol/issues/18#issuecomment-635866688

If it is about one request with a range that can spawn the whole document then a server has no way to indicate if it implements a special range request or not.

Typically the major amount of time spent to compute semantic tokens is to run the type binding phase in the compiler. If not optimized (e.g. by using a type pull model) libraries usually compute the type bindings for the whole file. This being said a range request is very likely equal expensive (minus the payload on the wire) than computing the tokens for a whole file. Having to end point allows the server to tell the client that it only supports whole documents and no range since there will actually be no win.

dbaeumer commented 4 years ago

Regarding https://github.com/microsoft/language-server-protocol/issues/18#issuecomment-643693626

The idea is that the client pulls for semantic tokens for all visible files. The one not having the focus should be pulled with less frequency. Assuming that the server implements the delta mechanism a pull for a file that has no semantic color changes will not produce any additional payload. This model was chosen to not sync any UI state. Otherwise the server might do an expensive impact analysis about which files need to refresh semantic tokens although non of them are visible in the UI.

dbaeumer commented 4 years ago

I did a pass over the proposed implementation to allign it with suggestion from this issue:

ghost commented 4 years ago

Having to end point allows the server to tell the client that it only supports whole documents and no range since there will actually be no win.

Sorry, I have trouble reading that. Can you try to reword it? However, here is my response to what I think I understood:

Just my thought process cleared up: Clients could range query in roughly viewport-sized chunks (or even smaller) and gradually like that for the entire file, with a higher priority if the user actively scrolls or types somewhere. While this has high bandwidth cost in the long run, it should be pretty cheap both computationally and in data sent in any given small time frame. So where's the significant win in delta'ing?

Response to what I think you wrote: If your argument was that servers would recompute the entire file for any range request making it as slow as fetching the entire document, why couldn't servers just look at the modified timestamp and/or edit events sent and otherwise cache it? It seems like that would be required for deltas as well to be efficient, after all the server needs to somehow be able to tell that the document updated to compute a delta too (if it doesn't want to recompute the entire thing all over again and again even when nothing changed). So I don't really get your argument here

dbaeumer commented 4 years ago

From our experience colorizing a range can be as expensive as coloring the whole file. This is due to the fact that to colorize the file the type bindings need to be resolved. Some tools have implemented a pull model for that (e.g. TypeScript, which is faster on range) but other don't (e.g. Java). The ones that don't are better off to always compute the color information for the whole file than being as for the range first and then for the whole file. These servers can not provide a range request which would ask the client to always ask for the whole file.

Regarding caching: we should not make caching a prerequisite. It should be up to servers to decide when to cache. We even investigate into batching to make it easier for servers to bring project states up to date, run design time builds or type binding phases (https://github.com/microsoft/language-server-protocol/issues/988).

For the delta: this is designed in a way that the server doesn't need to keep an AST or resolved bindings. It only needs to keep the last number array which is a lot smaller. And to detect that nothing has changed the server can use the version numbers send with document notifications.

As you can see with the new proposed protocol there is now a format and capabilities to indicate what requests the client will sent.

ghost commented 4 years ago

For the delta: this is designed in a way that the server doesn't need to keep an AST or resolved bindings. It only needs to keep the last number array which is a lot smaller

Sorry, I have trouble following this: @ AST: this is never needed to be kept for caching, neither for ranges nor deltas. What needs to be kept is the tokens in BOTH cases (for the delta to make a diff, for ranges to resend a cached range), so no difference here. (And the server could use your "number array" internally for both, so no difference here either.) Tracking changes on disk with version numbers and document notifications also is the same with diff and ranges, again no difference. So I'm really still quite baffled why delta is supposedly so useful. I get the benefits on transfer savings of course, I'm just not sure that alone is worth it. But I'm repeating myself, I should stop here.

Edt: to clarify the above into a more concrete suggestion, I feel like all the gains you attribute to deltas can be achieved similarly by writing "LSP servers should keep the tokens cached in an efficient internal format to make multiple range queries on an unchanged document fast." While I guess the delta format is a way of somehow forcing server writers to put this in, that seems to me like an unnecessarily over-engineered workaround to avoid just writing this suggested sentence into the spec. Although obviously the bandwidth gains remain, I don't want to explain those away.

Edit 2: and I deeply apologize for repeating myself so much. I am trying my best to make my points understandable to hopefully avoid this going on forever

dbaeumer commented 4 years ago

I agree that the server can keep whatever it feels best to produce the delta. This must not necessarily be the number[].

As I already outline a couple of times the delta is optional. A server will still function correctly if it is not providing it. We don't force servers to implement the delta. But in the light of more and more servers being used in a remote scenario having the delta is a useful addition. And with the latest changes I did you can even implement another format with another set of requests.

kjeremy commented 4 years ago

And to detect that nothing has changed the server can use the version numbers send with document notifications.

Is this really true? I don't think this is reliable in a language that supports imports.