hashicorp / syntax

TextMate grammars for highlighting HCL, HCL-based languages and Sentinel.
Mozilla Public License 2.0
25 stars 11 forks source link

Improve highlighting of primitive type declarations #27

Open radeksimko opened 2 years ago

radeksimko commented 2 years ago

Problem Statement

This is to follow up on https://github.com/hashicorp/terraform-ls/pull/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?

dbanck commented 2 years ago

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 commented 2 years ago

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?