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

improvement: Remove semantic tokens fallback #5985

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

jkciesluk
Copy link
Member

Previously, for identifiers with no symbol found, we've added fallback semantic tokens based on simple heuristic.
This was causing some problems for not-compiling projects, probably best to remove it and leave syntax highlighting to an editor.

@@ -171,7 +171,7 @@ class SemanticTokensLspSuite extends BaseLspSuite("SemanticTokens") {
"self-type",
"""|<<package>>/*keyword*/ <<a>>/*namespace*/
|
|<<object>>/*keyword*/ <<Abc>>/*class*/ { <<self>>/*variable,readonly*/: <<Any>>/*class*/ <<=>>>/*operator*/
|<<object>>/*keyword*/ <<Abc>>/*class*/ { self: Any <<=>>>/*operator*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like Any should be marked as class here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like no features work on Any (actually its not specific to Any, but I haven't yet figured the pattern) in self type, including hover and document highlight, because there is no symbol at this node in the AST.
I think its best to work on it in a follow up, since its not specifically related to semantic highlighting. I would still be coloured as class by the editor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work with the self type in line 192, so I though it's a strictly Any/AnyRef type of situation. Anyway let's do it as a follow up .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in #6006

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkciesluk jkciesluk merged commit 1f3e9f2 into scalameta:main Jan 10, 2024
@jkciesluk jkciesluk deleted the rem-sem-fallback branch January 10, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants