Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the analysis server for semantic highlighting #2202

Closed
simolus3 opened this issue Jan 7, 2020 · 15 comments
Closed

Use the analysis server for semantic highlighting #2202

simolus3 opened this issue Jan 7, 2020 · 15 comments
Assignees
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@simolus3
Copy link
Contributor

simolus3 commented Jan 7, 2020

On the insiders channel, VS Code now provides apis for semantic token highlighting. This would allow Dart Code to enhance the static grammar-based highlighter with highlighting information from the analyzer.

In particular, the extension could use it to:

  • be more accurate at semantic highlighting (members starting with an uppercase, named get/set, complex string literals, ...)
  • highlight non-Dart files via analysis server plugins

To my knowledge, LSP doesn't support syntax highlighting (yet). So I think this would have to be implemented using the DAS protocol and a contributor for now. I've started some work on this and I hope I can open a PR next week.

Current issues

@DanTup DanTup added this to the On Deck milestone Jan 7, 2020
@DanTup DanTup added blocked on vs code / lsp / dap Requires a change in VS Code to progress in editor Relates to code editing or language features is enhancement labels Jan 7, 2020
@DanTup DanTup modified the milestones: On Deck, v3.9.0 Jan 8, 2020
@DanTup DanTup modified the milestones: v3.9.0, On Deck Mar 11, 2020
@DanTup DanTup added the in lsp/analysis server Something to be fixed in the Dart analysis server label Jun 22, 2020
@DanTup DanTup modified the milestones: On Deck, Backlog Jul 2, 2020
@DanTup
Copy link
Member

DanTup commented Aug 27, 2020

@simolus3 I've uploaded a build of Dart-Code that uses the preview LSP client that includes semantic tokens:

https://github.com/Dart-Code/Dart-Code/releases/tag/v3.14.0-dev.semantic-tokens

You can download the vsix and install using the Extensions: Install from VSIX command in VS Code.

I have (temporarily) disabled the built-in syntax for that version to aid testing. This means files will be uncoloured until the server provides highlights (though when we ship for real, the original Dart syntax will be included to avoid a delay in providing some colours while the server is doing initial analysis etc.).

An easy test that it's working in Dart files is to create a variable starting with an uppercase letter - this was incorrectly coloured as a type in the original grammar.

Screenshot 2020-08-27 at 14 07 22

The latest server code is here:

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

It fixes multiline tokens, but not yet overlapping ones. For Dart, I might create a custom highlight_computer for LSP since the mapping is quite complicated (esp. for multiline), though for plugins we'd either need to continue to map, or introduce an LSP-specific tokens API for plugins (we don't currently have any LSP-specific plugin APIs, but I suspect we'll want to start at some point).

I'm undecided about when to start trying to land things in the SDK repo yet. I was planning to wait until the LSP spec is final, but I may consider putting it behind a flag or something to avoid building up a huge review.

@simolus3
Copy link
Contributor Author

Thanks! I can see that this works for Dart files, but I'm having no luck with my plugin. Is there a way to view the LSP communication between the extension and the server? Highlights generated by my plugin show up in the instrumentation log, but I'm not seeing anything in the editor.
It might just be that I just generate invalid tokens, but seeing the LSP messages would help to diagnose this.

but not yet overlapping ones.

What I did in my original PR was to split tokens when they overlap or nest. So if I had tokens like this:

"This is a $string literal"
\-------------------------/       string
           \-----/                variable

I'd split them into

"This is a $string literal"
\---------/        \------/       string
           \-----/                variable

They still display as intended without overlapping. Basically, I sort regions by ascending offset and then by descending length. I can then iterate through them, and keep track of the current active span. If the next token intersects with that span, I split the original token around the new one. As far as I remember, this worked pretty well in the client back then. Maybe the server can do this transformation too? I'm happy to port it to the server if you think it's necessary. But yeah, for Dart it might be more efficient to write another highlights computer that doesn't generate overlapping tokens at all.

@DanTup
Copy link
Member

DanTup commented Aug 27, 2020

Is there a way to view the LSP communication between the extension and the server?

You can use the Dart: Capture Logs to capture the traffic (or enable the dart.analysisServerLogfile setting), but what's in the instrumenation log should be the same thing - it's just logged by the server on the way out.

What I did in my original PR was to split tokens when they overlap or nest.

I did push fixes for overlapping tokens - but I realise now I didn't handle this properly. The examples I had (like imports and annotations) had nested tokens that filled the whole of the "outer" token. I hadn't gotten to string interpolation yet, I guess that would've turned up that I did it badly. I'm going to restructure how it works a little, as the current multi-line handling is really messy and I have some ideas to simplify it.

I do think we should try to support mapping the existing regions, whether we have a new computer for Dart and/or a new API for plugins, for compatibility.

@simolus3
Copy link
Contributor Author

Just tried to type around a bit and suddenly it worked 🎉

grafik

I was afraid that latency and the conflict between the notification and pull-based model might cause problems, but I didn't notice anything so far. The inconsistencies are probably because of my plugin, so this looks really awesome so far.

@DanTup
Copy link
Member

DanTup commented Sep 2, 2020

Neat!

I've started tidying the server implementation up a little (DanTup/sdk@ef362ea), though nothing is merged yet (I'm not sure of the timeline of the next LSP spec version - it has quite a lot of open issues on GH suggesting it might not be soon so I'm unsure about trying to land the spec updates in the SDK just yet).

I'd like to add range support (this would allow the editor to get tokens faster for the visible code rather than need the whole file), though I don't know if it would end up in us doing more work (eg. if we're re-computing the entire file for each range request) so I need to do a little testing.

@DanTup DanTup modified the milestones: v3.15.0, On Deck Sep 21, 2020
@DanTup
Copy link
Member

DanTup commented Oct 14, 2020

@simolus3 still haven't forgotten this, but it's still unfinalized in LSP so I've been holding off merging or doing any more. There are also a few things I've asked about, like supporting multiline/nested tokens that haven't really been resolved yet.

@DanTup
Copy link
Member

DanTup commented Dec 10, 2020

@simolus3 LSP v3.16 is due to be finalised imminently (hurrah!). I have started preparing to land changes in the server:

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

Once LSP 3.16 is final (and the next version of VS Code) I'll make a new preview build of Dart-Code with the new client that will work with the server changes.

I've included a basic plugin test in that CL, though if there are cases you think it doesn't cover, please let me know (or feel free to contribute changes).

@simolus3
Copy link
Contributor Author

Awesome, thanks for letting me know! I took a look at your CL and I think your test cases cover what I'm interested in.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 14, 2020
Fixes Dart-Code/Dart-Code#2202 + many more.

Change-Id: Ia2e8ec2415836a77618821144699971e97b4fc5c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175722
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@DanTup
Copy link
Member

DanTup commented Dec 15, 2020

Fixed by dart-lang/sdk@cb2ede5.

In order to get the fix you'll need Dart-Code v3.18.0 (a preview release should be available later today/tomorrow), be using LSP Preview, and have a Dart SDK from after yesterday (the most recent nightly works).

@simolus3 once the Dart-Code preview is available later, please give this a try with your plugin and let me know if you hit any issues. Thanks!

semantic_tokens

@DanTup DanTup closed this as completed Dec 15, 2020
@DanTup
Copy link
Member

DanTup commented Dec 15, 2020

@ericdallo
Copy link

Cool @DanTup, I'll test it with lsp-dart since already have support for semantic highlighting, thank you!

@DanTup
Copy link
Member

DanTup commented Dec 16, 2020

FYI in case you see this - there was a bug in where overlapping tokens might not be processed correctly (due to an unstable sort) that could result in some tokens being dropped (the blue imports):

Screenshot 2020-12-16 at 16 50 25

I've got a fix at https://dart-review.googlesource.com/c/sdk/+/176446/ though it won't be in the nightly until tomorrows (assuming I get it landed today).

@DanTup
Copy link
Member

DanTup commented Dec 20, 2020

FYI - I've disabled Semantic Tokens in the server while I get a fix for a race condition in the code that handles splitting multiline tokens. Hoping to get a fix soon.

@DanTup DanTup reopened this Dec 20, 2020
@DanTup
Copy link
Member

DanTup commented Dec 22, 2020

Due to the issue above, I'm reverting the change to support LSP v3.16 from the upcoming release and will instead aim to ship it in the next one. You can still use the preview build for it (and I can build. anew preview build after the release that re-enables it), though you'll want a version of the SDK with all the fixes to avoid the issue mentioned above.

@DanTup DanTup modified the milestones: v3.18.0, v3.19.0 Dec 22, 2020
@DanTup
Copy link
Member

DanTup commented Jan 19, 2021

The issues in the server were fixed and landed, and the Dart-Code change to re-enabled them landed in a11cba2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants