[experimental] Semantic token highlighting - #2203

Nice! I'll try it out and review it properly soon, but at a glance the code and screenshots look great! Have you noticed any issues (like bad colouring while typing due to latency) when using it?

We might need to come up with a better way to wait for the highlights - currently the code waits a while (which could maybe simplified something like this) which works for when the file is first opened, but I think when typing it's likely to just get "the last highlights" so the offsets might end up incorrect (we might need to evict the existing cached highlights on-change or similar).

I wonder if there's a way we can conditionally disable the built-in syntax so we can ship behind the flag initially (it's not ideal testing if both are active when it's enabled). If not, we might want to ship a custom vsix build and call it out in a release for people to test out, then ship it always-on (removing the old one) from a future release.

Thanks!

Have you noticed any issues (like bad colouring while typing due to latency) when using it

So far it appears like VS Code offsets the existing tokens if it doesn't get a response after an edit. I'll also try to use it for a longer duration after I finished the remaining mappings and see if everything feels natural.

We might need to come up with a better way to wait for the highlights

Ah cool, I didn't know about waitFor. I had the idea of making the DasFileTracker expose promises (or Events?) instead of a synchronous method. That way we could resolve pending promises as soon as analyzer notifications come in, we wouldn't have to wait for a fixed duration (of course we'd still need to respect cancellation tokens). What do you think?

There are some more complicated mappings that I still need to do. For instance, the analyzer sometimes reports multiline tokens (e.g. for multiline-comments and probably for '''-string literals as well). We'd have to split those tokens at line endings, otherwise VS Code won't accept them.
Sometimes tokens are also nested: 'hello\nworld' would report a string literal token from 0 to 16, and a "valid string escape" region from char 5 to 7. We'd have to transform that to a string literal, followed by the valid string escape, followed by another string literal. I'll see if I can come up with something clever here.

I wonder if there's a way we can conditionally disable the built-in syntax so we can ship behind the flag initially

I don't have a lot of experience with VS Code plugin development so I'm not 100% sure. But it appears like grammars have to be set in the package.json and can't be registered dynamically.

Sorry for the delay in getting back to this, I've not been around as much recently.

That way we could resolve pending promises as soon as analyzer notifications come in, we wouldn't have to wait for a fixed duration (of course we'd still need to respect cancellation tokens). What do you think?

There's a onDidChangeSemanticTokens event that the provider can raise, so I think we could just subscribe to the analyzer event and fire that, which will cause VS Code to re-request tokens (this is how some things like the flutter outline tree notify for changes).

We'd have to split those tokens at line endings, otherwise VS Code won't accept them.

If this isn't documented, it may be worth raising with VS Code to check it's not just a bug (and if it's not, that it's explicitly documented!).

it appears like grammars have to be set in the package.json and can't be registered dynamically.

Yeah, though recently I was reminded that if we were to drop this, then opening a file while the analyzer is doing its initial scan could result in no tokens for a while (this won't affect your language since it'll be blank anyway, but it would seem like a regression for Dart). I think it probably makes sense to keep the original grammar as a fallback and for immediate colouring for Dart and then have the provider improve it (assuming it layers on top nicely).

I haven't reviewed the code that's here yet, so let me know if it's in a reasonable state for me to do so, or if you're likely to make any significant changes. We could also create a branch to land it into while it's still being worked on to avoid having to do everything in one PR if that makes sense.

Thanks, I didn't know about onDidChangeSemanticTokens. I'll integrate that and check the line-ending issue.

I haven't reviewed the code that's here yet, so let me know if it's in a reasonable state for me to do so

I don't think there will be any significant changes, but I want to clean some things up and write more tests for the token transformations if they turn out to be necessary. It shouldn't take long and I'll do that over the weekend, so it's probably best to not properly review this until next week.

I took another look - the documentation does point out that tokens can't be multiline, so it looks like we need to split them.
I still have to do a transformation to remove nested highlight regions. For instance, if we had something like ///comment [link] more, the analysis server would report the whole thing as a single comment (offset 0, length 22) with a documentation region in between (offset 11, length 6). For [link] to be a different token, we need to split that to a comment token from 0..10, followed by a documentation token from 11..16 and another comment from 17..21. I've asked here to check if this transformation is really necessary on our side.

@DanTup I'm wondering if it makes sense to drop this and instead wait for LSP-based semantic highlighting? It looks like highlighting will be standardized with the next LSP release.

I'm not super familiar with the analysis server, but I could then try to make it support highlighting over LSP. I guess that's the best long-term approach either way. So if this will be obsolete in a couple of months and we don't have to do all the token transformations in here, it might be better to just wait and then use the LSP directly?

@simolus3 sorry for the delay, I'd been hoping to get back to this sooner.

The latest release of the extension does let you enable LSP mode, so if this API will be added to the LSP protocol soon, then I think that might be a good idea to avoid duplicated work. I do want to switch to LSP by default as soon as I can, but it's going to need plugin support. I have been looking at in the background - I've got the first request working when using the AngularDart plugin this week, though I have some other things that take priority in the short-term.

If you're interested in having a go there, I'll aim to tidy up my work so far and push it somewhere.

but it's going to need plugin support

Just to be clear: Each individual plugin will need to support LSP as well? I was hoping that the analysis server could act as a translating proxy here, similar to what it's doing with the Dart-specific protocol.
Are there any public resources describing how plugin communication will look like with LSP? I'm maintaining my own plugin, and obviously want it to continue working with LSP.

If you're interested in having a go there

I would love to take at current LSP work on the server side - especially if plugins need changes as well. Hopefully most of this can be implemented in package:analyzer_plugin rather than the individual plugins. I'm happy to add support for more requests if all plugins can benefit from that.

Either way, I'll close this PR and wait for LSP to standardize semantic highlighting.

Just to be clear: Each individual plugin will need to support LSP as well?

No sorry, I described it badly :) I'm testing with the AngularDart plugin, and the LSP server is translating LSP requests back into DAS-protocol requests and sending them to plugins (and then converting the plugin responses to LSP responses).

So far I've implemented only the definition request (as a proof-of-concept that plugins can work), but I'll also need to look at how notifications will work (things like errors). I'm hoping they're also quite straight forward (just translate them to LSP equivs), though I'm not sure how they're handled if multiple plugins (or plugin + base functionality) provide data for the same files (to avoid them overwriting each others errors) so I'll need to do a little investigation.

My initial change is here (though it needs much tidying up):

https://dart-review.googlesource.com/c/sdk/+/142378

It's possible in future plugins may want to deal with LSP (eg. to support LSP things that the server does not outside of LSP), but for now I think the translation will work fine.

Thanks for clearing that up, the DAS-protocol to LSP response translation definitely makes sense.
I could assist by implementing other plugin functionality as dependent CLs if that helps.

Contributions would definitely be appreciated! It's not my top priority right now so I'm progressing it mainly when blocked on other things. I've updated the change above that addresses some of the comments and adds a test for the definition request using a mock plugin.

I think the main thing that would prevent this working with your plugin in VS Code if you enabled LSP preview (and ran the server from source, using that branch) would be that the initialization handshake doesn't include file extensions from plugins (if the plugins are initialised by the time this happens, it probably won't be too difficult to collect there interestingFiles).

If you do decide to have a go at implementing bits, let me know what you're working on so I can avoid overlapping - and if I can be of any assistance, do shout (I'm usually responsive in discord when I'm around).

It appeared that because of token errors we cannot setup the color highlighting for variables (check the topic above)

@subzero911 this is unfortunately a known issue. There is a plan to move to semantic highlighting provided by the language server (which is what this PR was progress towards - #2202). There are many other issues with the TextMate grammar (for ex. #311 and #2056).

The Dart extension is going to move to the LSP protocol, so implementing it in the current version is less worthwhile because the work would be lost and must be done again in LSP.

Semantic highlighting in LSP is still work-in-progress (microsoft/language-server-protocol#18) but once it's finalised there, work can start on adding it to the LSP server. Then it would be available for VS Code themes when using the LSP mode (which can be opted in to using the dart.previewLsp setting until it's also ready to enable by default).

Hope this makes sense!

@simolus3 FYI (in case you thought I'd forgotten 😄) - since this API looks fairly stable now, I've made a start on implementing this in the LSP server (though nothing will land until LSP v3.16 is finalised):

https://dart-review.googlesource.com/c/sdk/+/158393/

It has some basic tests for Dart (none for plugins, though it does merge the results so they might "just work") though I haven't got as far as testing it for real in a client yet!

This is awesome, thanks for keeping me updated! Do you know how I can checkout CL branches to try this out? I imagine the push-based model of server plugins might cause some issues with the pull-based LSP model, especially for services with low latency like highlights. I'm happy to try this out, maybe it's not too bad.

Also, you mentioned splitting tokens at line endings as a todo. Do you think we might have to split nested tokens too? I think it might become an issue if we highlight nested string literals for instance. I'm happy to port the approach from this PR over if you think it's necessary.

Do you know how I can checkout CL branches to try this out?

I don't know for Gerrit, but I pushed the branch to GH if that helps (you can use dart.analyzerPath to run the server from source, but you'll need to make sure you're on a recent build (eg. nightly)):

https://github.com/DanTup/sdk/tree/lsp-3-16-semantic-tokens

(also be ware server startup will be slower from source)

Also, you mentioned splitting tokens at line endings as a todo. Do you think we might have to split nested tokens too?

I'm not sure - I couldn't find anything about that in the LSP docs and because the character deltas appear to be from the start of the previous token (not the end), I thought maybe they're intended to be supported (though I haven't got as far as any that overlap yet).

(FWIW I'm out of the office next week, so this might not progress much more - and I may be less responsive than normal - until I'm back).

No problem! I'll try to run your version with my plugins sometime next week then :)

And yeah, I meant overlaps in general when I mentioned nested tokens. Judging by microsoft/vscode#86415 (comment) we might need that transformation in the server, but since it mostly highlights atomic identifiers and keywords there's little chance of that actually happening.

I couldn't get this to work, not even for Dart. I'm using your SDK fork, the current version of the extension from git and VS Code insiders. In the server diagnostics, I see that server capabilities contain a semanticTokensProvider but the client capabilities don't mention semantic tokens. That probably indicates a misconfiguration on my end, do you see anything obvious that I'm missing?

{
    "dart.additionalAnalyzerFileExtensions": ["moor"],
    "dart.analyzerDiagnosticsPort": 9876,
    "dart.analyzerInstrumentationLogFile": "/tmp/analyzer_instrumentation.txt",
    "dart.sdkPath": "/path/to/current/dev/sdk",
    "dart.analyzerPath": "/your/dart/fork/pkg/analysis_server/bin/server.dart",
    "dart.previewLsp": true,
    "editor.semanticHighlighting.enabled": true,
    "dart.analyzeAngularTemplates": true,
}

@simolus3 I hadn't tried in VS Code yet, only tests in the server.

I just had a look, and the node lsp client with semantic token support hasn't been released yet. I can't figure out how to run it from source, however this tag was made a few hours ago, so I think once that version is published to npm, I can make a build of VS Code that uses that(preview) version to do some testing there.

No worries! I thought semantic highlighting already worked in VS Code, if that's not the case I can also wait a little longer :)

It does work in VS Code, however for LSP there's a client library that needs to also be updated (that's a shared library maintained by MS that translates between the VS Code APIs and LSP). Until the LSP spec is finalised, that won't ship in the release version, but we can use the pre-release builds once they're made (I did look, but couldn't figure out how to reference it from source).

That might happen today based on that tag, so if it's before I finish (or otherwise, probably tomorrow), I'll try to make it all work and build a version of Dart-Code we can use for testing.

I've opened microsoft/language-server-protocol#1077 asking for some explicit info about nested/intersecting tokens, as the comment you linked says the characterDelta is from the end of the previous token, but the LSP spec says it's from the start.