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

internal/lsp: Provide (opt-in) custom semantic tokens & modifier #833

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Mar 16, 2022

Depends on hashicorp/terraform-schema#99


This aims to close the loop for hashicorp/vscode-terraform#958 but more importantly better support custom themes and accurately report token types and modifiers for those clients which support those, further enabling the wider ecosystem of themes.

@radeksimko radeksimko force-pushed the f-custom-tokens branch 4 times, most recently from 9e7d58e to 3b7899a Compare March 17, 2022 11:04
@radeksimko radeksimko marked this pull request as ready for review March 17, 2022 11:04
@radeksimko radeksimko requested a review from a team March 17, 2022 11:05
This was just lost in the reshuffle of previous commits.
@dbanck
Copy link
Member

dbanck commented Mar 18, 2022

I assumed the server would always report all supported semantic tokens and modifiers in the InitializeResult and clients should ignore server capabilities they don’t understand. So why do we filter tokens by client capabilities first? Or am I missing something?

@radeksimko
Copy link
Member Author

radeksimko commented Mar 18, 2022

There's not a great level of detail in the spec on how the capability negotiation should work between the client and the server, so I'm mostly working with my assumptions and the rest of the protocol spec.

Firstly the spec says the following https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#semanticTokensLegend

export interface SemanticTokensLegend {
	/**
	 * The token types a server uses.
	 */
	tokenTypes: string[];

	/**
	 * The token modifiers a server uses.
	 */
	tokenModifiers: string[];
}

server uses to me implies that server should only send the types and modifiers it actually uses, not all those it supports. I may be reading too much into it though and it may not matter in practice. 🤷🏻

For modifiers filtering most likely doesn't matter (the server could probably assign all of them), but for token types it does. There's only one token type the server can associate with a particular range and so it has to make a decision what token type that is. It cannot assign both e.g. hcl-blockType and type.

In theory client's ordering could indicate preferences within the list which the server could use to decide which token type to use. The spec also doesn't go into great detail about the ordering of token types or modifiers, so I'm just (safely) assuming that there's no meaning to the order.

A secondary (less important) point is that by filtering what's actually supported on the server side we can reduce the amount of data sent back to the client. Perhaps not very significant but it feels like a relatively cheap performance improvement to make. 🤷🏻‍♂️

@radeksimko
Copy link
Member Author

One question to ask is why would the client be sending its own capabilities at all if the server ignores it? I'm assuming the server is expected to do something with that information.

@radeksimko
Copy link
Member Author

(sorry for the thoughts scattered across multiple comments)

I know VSCode has a mechanism for registering fallbacks for token types and modifiers and allows mapping to TM scopes - so it may feel like we're duplicating some work, but I don't think we can or should assume this is how other clients work. Also there can be different mappings between the extension versions, so we can not and should not rely on the extension to provide these fallbacks.

@dbanck
Copy link
Member

dbanck commented Mar 18, 2022

I think you're right. Thanks for the detailed explanation. Since the list of token strings is used to build the token bit representation, sending unsupported tokens here doesn't make much sense.

One question to ask is why would the client be sending its own capabilities at all if the server ignores it? I'm assuming the server is expected to do something with that information.

I assumed at initialization both parties just list what features they are supporting. And inside the different handlers, the server would have to account for the client capabilities and wise-versa. But for semantic tokens, this doesn't make sense.

@radeksimko radeksimko merged commit 75c3c27 into main Mar 21, 2022
@radeksimko radeksimko deleted the f-custom-tokens branch March 21, 2022 08:42
@radeksimko radeksimko added this to the v0.27.0 milestone Apr 6, 2022
@github-actions
Copy link

This functionality has been released in v0.27.0 of the language server.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request textDocument/semanticTokens Semantic syntax highlighting workspace/semanticTokens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants