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

Improve highlighting of primitive type declarations #27

Open
radeksimko opened this issue Mar 7, 2022 · 2 comments
Open

Improve highlighting of primitive type declarations #27

radeksimko opened this issue Mar 7, 2022 · 2 comments
Labels
enhancement New feature or request syntax

Comments

@radeksimko
Copy link
Member

Problem Statement

This is to follow up on hashicorp/terraform-ls#827 which introduces semantic highlighting of type declarations.

Type declarations are highlighted by the server as keyword.

Our grammar currently highlights them as storage.type which seems to result in a different association in the default VSCode themes.

https://github.com/hashicorp/vscode-terraform/blob/c8b0154b8d2cc4cd67b4e68065ad6b85f8e133a8/syntaxes/terraform.tmGrammar.json#L49-L53

Grammar (only) Highlighting

Screenshot 2022-03-07 at 12 26 20

Semantic Highlighting

Screenshot 2022-03-07 at 12 27 53

Expected User Experience

Grammar and semantic highlighting are more aligned, i.e. type declarations are highlighted as keywords in the grammar too.

Proposal

Update terraform_type_keywords matcher?

@radeksimko radeksimko added enhancement New feature or request syntax labels Mar 7, 2022
@dbanck dbanck self-assigned this Mar 21, 2022
@radeksimko radeksimko transferred this issue from hashicorp/vscode-terraform Mar 21, 2022
@dbanck
Copy link
Member

dbanck commented Mar 24, 2022

Are we sure, we want to use keyword here? Why is the server reporting this as keyword?


Some findings:

Looking at our grammar, we're already using keyword.control for loops as default themes and the TextMate Grammar Docs docs are suggesting:
CleanShot 2022-03-24 at 12 22 09@2x

Looking at other languages (e.g. TypeScript), keyword is used for loops, too and types are reported as support.type.
CleanShot 2022-03-24 at 12 20 44@2x
CleanShot 2022-03-24 at 12 20 53@2x

We're using support.type currently for block types, e.g. variable or resource in our grammar and in the language server: https://github.com/hashicorp/terraform-ls/blob/031e30f62ab169104837fbb1e9ef2633ded73329/internal/lsp/token_encoder.go#L112-L114


I think a more consistent way would be to use storage.type for block types, like variable and support.type for types, like string and make sure to match grammar and semantic tokens:
CleanShot 2022-03-24 at 12 38 58@2x

@radeksimko
Copy link
Member Author

radeksimko commented Mar 24, 2022

The main reason is that variable (token type) is already used for traversals and I couldn't think of many other good options in semantic highlighting, other than keyword, bearing in mind the syntax should provide decent out-of-the-box experience without relying on explicit mapping in extension(s).

What you're suggesting makes sense 👍🏻 but I'm not sure how well we can map that to the default semantic tokens here:
https://github.com/hashicorp/terraform-ls/blob/031e30f62ab169104837fbb1e9ef2633ded73329/internal/lsp/semtok/lsp_token_types.go#L4-L26

On that note I'd be curious to see how any language server with semantic highlighting solves this - I'm guessing they get away with e.g. type token because there aren't "block types" unlike in our DSL?

@dbanck dbanck removed their assignment Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request syntax
Projects
None yet
Development

No branches or pull requests

2 participants