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: report dependent modifiers as defaultLibrary #817

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Mar 2, 2022

Closes #710

The (LSP) modifier defaultLibrary better represents the meaning of dependent labels or attributes, as opposed to modification which was chosen mostly arbitrarily as part of the initial implementation in #331

Highlighting dependent labels was already possible prior to this patch but the change of the modifier here reflects the commitment to support custom themes in a more sensible (and hopefully stable) way.

Default VSCode themes do not assign meaning to any modifiers but it is possible to take advantage of this in a custom theme or simply by modifying the VSCode settings such as

    "editor.semanticTokenColorCustomizations": {
        "rules": {
            "enumMember.defaultLibrary": {
                "bold": true
            }
        }
    },

Before

(assuming customized settings as above & default theme)

Screenshot 2022-03-02 at 12 20 41 Screenshot 2022-03-02 at 12 20 31

After

(assuming customized settings as above & default theme)

Screenshot 2022-03-02 at 12 16 20 Screenshot 2022-03-02 at 12 16 37

cc @tringuyen-yw


variable name getting also reported as dependent is not intended, but I opened a separate issue for that problem: hashicorp/terraform-schema#96

@radeksimko radeksimko added enhancement New feature or request textDocument/semanticTokens Semantic syntax highlighting workspace/semanticTokens labels Mar 2, 2022
@radeksimko radeksimko added this to the v0.26.0 milestone Mar 2, 2022
@radeksimko radeksimko changed the title internal/lsp: report dependent modifiers as defaultLibrary internal/lsp: report dependent modifiers as defaultLibrary Mar 2, 2022
@radeksimko radeksimko self-assigned this Mar 2, 2022
@radeksimko radeksimko requested a review from a team March 2, 2022 12:29
@jpogran
Copy link
Contributor

jpogran commented Mar 2, 2022

Can we contribute this inside the extension as shown in https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#custom-textmate-scope-mappings?

@radeksimko
Copy link
Member Author

Can we contribute this inside the extension as shown in https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#custom-textmate-scope-mappings?

Yep, I suppose we could! That's a good idea.

@tringuyen-yw
Copy link

Although bold is better than nothing. In my opinion, it would be even better if the user can define the font attribute directly. A possible example of this kind of color syntax is possible with Intelli-J. See screenshot in this issue vscode-terraform, 710, Color syntax: add scope for resource_type and resource_name

In the case of VSCode, I can edit settings.json and add/override the textMateRules on the scope specific to the languange. Here is an example for Terraform HCL (excerpt from my settings.json). The links to howto customize the VSCode theme colors are given in the above Github issue.

"editor.tokenColorCustomizations": {
  "textMateRules": [
    {
      // Terraform keywords
      "scope": [
        "entity.name.type.terraform",
      ],
      "settings": {
        "foreground": "#8040FF", "fontStyle": ""
      }
    },
    {
      // name of nested block within a resource block
      "scope": [
        "entity.name.label.terraform",
      ],
      "settings": {
        "foreground": "#007CCC", "fontStyle": ""
      }
    },
  ]
}

Base on the example above, may be you can add some additional scopes to correspond to Terraform's resource-type and resource-id?

I don't think it is necessary to worry about the custom themes. For themes which are not aware about Terraform specific scopes, they would just theme the resource-type and resource-id as string. For those which can or for users who are willing to edit their settings.json manually, they would see the proper syntax color.

@radeksimko
Copy link
Member Author

Although bold is better than nothing. In my opinion, it would be even better if the user can define the font attribute directly.

The bold was just an example. As you can see from the attached code in the PR - it does not set any particular style, just "meaning". The main point here is exactly to allow users to define any styling they want, as shown in the snippet of config above - which just used bold.

I (mostly blindly) assumed that custom themes can simply assign their own styles to token types and modifiers but regardless we can certainly map these semantic tokens to some TextMate grammar scopes to make that possible or easier.

We will keep entity.name.type.terraform or entity.name.type.terraform in mind when introducing the mapping in the extension. Generally the two would make sense from the semantics perspective but we also need to ensure the config is reasonably highlighted in the default themes - which was a challenge in the past. Either way - that sounds like a problem to discuss deal with and discuss on the extension side.

This PR here merely enables the mapping, nothing more (yet).

Base on the example above, may be you can add some additional scopes to correspond to Terraform's resource-type and resource-id?

I'm afraid we are at the mercy of LSP here as we can only work with these language-agnostic types and modifiers here

// Types predefined in LSP spec
TokenTypeClass TokenType = "class"
TokenTypeComment TokenType = "comment"
TokenTypeEnum TokenType = "enum"
TokenTypeEnumMember TokenType = "enumMember"
TokenTypeEvent TokenType = "event"
TokenTypeFunction TokenType = "function"
TokenTypeInterface TokenType = "interface"
TokenTypeKeyword TokenType = "keyword"
TokenTypeMacro TokenType = "macro"
TokenTypeMethod TokenType = "method"
TokenTypeModifier TokenType = "modifier"
TokenTypeNamespace TokenType = "namespace"
TokenTypeNumber TokenType = "number"
TokenTypeOperator TokenType = "operator"
TokenTypeParameter TokenType = "parameter"
TokenTypeProperty TokenType = "property"
TokenTypeRegexp TokenType = "regexp"
TokenTypeString TokenType = "string"
TokenTypeStruct TokenType = "struct"
TokenTypeType TokenType = "type"
TokenTypeTypeParameter TokenType = "typeParameter"
TokenTypeVariable TokenType = "variable"
// Modifiers predefined in LSP spec
TokenModifierDeclaration TokenModifier = "declaration"
TokenModifierDefinition TokenModifier = "definition"
TokenModifierReadonly TokenModifier = "readonly"
TokenModifierStatic TokenModifier = "static"
TokenModifierDeprecated TokenModifier = "deprecated"
TokenModifierAbstract TokenModifier = "abstract"
TokenModifierAsync TokenModifier = "async"
TokenModifierModification TokenModifier = "modification"
TokenModifierDocumentation TokenModifier = "documentation"
TokenModifierDefaultLibrary TokenModifier = "defaultLibrary"

The protocol doesn't seem to allow just arbitrary custom token types at this point and we struggled to find the right mapping to HCL constructs.

However I'm assuming that you could work with terraform in the scope name & a scope we eventually pick for dependent labels.

Did you want to style resource labels differently from data labels? If so, then that I'm afraid is not a use case we can meet through semantic-token based highlighting due to the above limitations. We could in theory introduce some custom scopes to a dedicated static Terraform grammar however.

@radeksimko
Copy link
Member Author

Actually there is this https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#custom-token-types-and-modifiers which suggests that custom token types are allowed, but it's unclear to me what implications does that have on the server. Unlike with TM scopes there's only one token type that we can assign to the same range, so this would suggest that any client that doesn't support that custom token type is just out of luck? 🤔

@radeksimko
Copy link
Member Author

radeksimko commented Mar 4, 2022

@tringuyen-yw I believe we have a plan forward with regards to supporting custom themes via custom token types and modifiers, see hashicorp/vscode-terraform#958 with more details.

In the meantime though I'd still like to go through with this PR. The only difference after hashicorp/vscode-terraform#958 is implemented fully (on LS side) is that defaultLibrary becomes a fallback for e.g. hcl-dependent modifier depending on what client capabilities are received.

cc @hashicorp/tf-editor-experience for review

Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@radeksimko radeksimko merged commit 9e3106c into main Mar 4, 2022
@radeksimko radeksimko deleted the f-sem-tokens-dependent-modifier branch March 4, 2022 12:09
@github-actions
Copy link

This functionality has been released in v0.26.0 of the language server.
If you use the official Terraform VS Code extension, it will prompt you to upgrade to this version automatically upon next launch or within the next 24 hours.

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 Apr 21, 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.

None yet

4 participants