-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] First cut at semantic token provider #19108
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
[ty] First cut at semantic token provider #19108
Conversation
|
CodSpeed WallTime Performance ReportMerging #19108 will not alter performanceComparing Summary
|
carljm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'd prefer for Micha or Dhruv (who know the LSP and our plans in that area better) to review this; just one initial comment.
dhruvmanila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, thank you for doing this!
I've been reviewing this for an hour and I need to take a lunch break but here are my initial comments. I'll look at the remaining parts after the lunch break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, my review is finally done, this is an awesome PR!
The test cases are extremely thorough, thank you for writing them!
I think the only required change would be to make sure that we convert the ty location values back to the LSP values in the common function for the semantic tokens handler. And, around the visitor implementation where some of the calls that might lead to duplicate tokens could be removed.
A lot of my review comments could be considered as suggestions so feel free to either apply them or discard them. It's totally fine if those are done as follow-up. I can also own them as follow-up to this PR if you prefer.
…None` means "entire document".
…er than `Option<SemanticTokens>`.
…g `tokens` as a public field.
|
@dhruvmanila, thanks for the thorough and insightful code review comments! I think I've addressed them all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thank you for taking this on and addressing all the review comments!
|
This can be done in a sepreate PR, but it would be great to add semantic highlighting to the playground too. It should be "as easy as" implementing the necessary provider and registering them here: ruff/playground/ty/src/Editor/Editor.tsx Lines 143 to 180 in a663796
|
|
I'll leave it up to you if this PR should close astral-sh/ty#260 |
…d the multilineTokenSupport capability of the client.
|
This PR implements a basic semantic token provider for ty's language server. This allows for more accurate semantic highlighting / coloring within editors that support this LSP functionality.
Here are screen shots that show how code appears in VS Code using the "rainbow" theme both before and after this change.
The token types and modifier tags in this implementation largely mirror those used in Microsoft's default language server for Python.
The implementation supports two LSP interfaces. The first provides semantic tokens for an entire document, and the second returns semantic tokens for a requested range within a document.
The PR includes unit tests. It also includes comments that document known limitations and areas for future improvements.