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

Add typemod for decorators #278

Closed
johannesrld opened this issue Apr 9, 2024 · 12 comments · Fixed by #925
Closed

Add typemod for decorators #278

johannesrld opened this issue Apr 9, 2024 · 12 comments · Fixed by #925

Comments

@johannesrld
Copy link

johannesrld commented Apr 9, 2024

Currently, lsp.type.decorator can be overwritten by a couple lsp tokens, for example @classmethod is overwritten by:

  • lsp.type.class
  • lsp.mod.builtin
  • lsp.mod.defaultLibrary
  • lsp.typemod.class.builtin
  • lsp.typemod.class.defaultLibrary

it would be nice to have something like lsp.mod.decorator, lsp.typemod.decorator.builtin or lsp.typemod.decorator.defaultLibrary so we can highlight decorators with the same colour regardless of if they're classes or functions

@DetachHead
Copy link
Owner

is mod and typemod a neovim-specific thing? i see it's mentioned here but i can't find anything about it in the LSP spec

happy to support it, tho i find it weird that semantic tokens are different in each editor

@johannesrld
Copy link
Author

Literally have no idea whatsoever unfortunately

@johannesrld
Copy link
Author

johannesrld commented Apr 9, 2024

Looking at the documentation, typemod and mod just seem to be for semantic token modifiers with typemod being more specific (for example you might have @lsp.mod.decorator if you want to highlight all decorators a specific colour, and then @lsp.typemod.class.decorator and @lsp.typemod.function.decorator to distinguish between function and class decorators)

the spec also says you can just define whatever you want and that the predefined values are just the default "base", for example pyright (and rust-analyzer) also changes the in operators semantic token based on if it's used in a control flow or if it's used to check for membership, for example:

image

image

edit: however, for anyone using python in neovim you can add this to after/queries/python/highlights.scm to get similar behaviour

;; extends
(for_statement "in" @keyword.repeat)
(for_in_clause "in" @keyword.repeat)

@RayJameson
Copy link

RayJameson commented Dec 3, 2024

I'm not sure if this the right issue, but I'm using this workaround for decorator highlighting (for neovim)

I've added this to basedpyright lsp configuration

        on_attach = function()
          vim.api.nvim_create_autocmd("LspTokenUpdate", {
            callback = function(args)
              local token = args.data.token
              if token.type == "decorator" and not token.modifiers.readonly then
                vim.lsp.semantic_tokens.highlight_token(
                  token,
                  args.buf,
                  args.data.client_id,
                  "@lsp.type.decorator.python",
                  { priority = 130 }
                )
              end
            end,
          })
        end,

this is the result
image

@DetachHead
Copy link
Owner

i think i know what the issue is here. some symbols get two different semantic tokens applied to them. currently decorators get both the decorator token type in addition to either class or function. vscode seems to prioritize the decorator one while neovim prioritizes the other.

i can fix this pretty easily, but in cases where the decorator is something other than a single name, how should those be handled? i'm leaning towards just falling back to using the tokens from each part of the decorator call like this, unless anyone has a better idea:

image

@RayJameson
Copy link

RayJameson commented Dec 3, 2024

vscode uses same color regardless of that (in pylance), I think it's something of opinionated matter, as for me, I would like to see one color for all parts, since decorators are always callables and there shouldn't be misunderstanding if module part is not colored differently, also I like how it looks visually, 3 different colors looks ugly for single (kinda) symbol to me

@RayJameson
Copy link

RayJameson commented Dec 3, 2024

And yes, your assumption is correct, here is a screenshot of inspect command (shows applied highlighting stuff)
As you can see both decorator and function have same priority
image

@DetachHead
Copy link
Owner

vscode uses same color regardless of that (in pylance), I think it's something of opinionated matter, as for me

as far as i can tell pylance doesn't use the decorator token type at all and just uses the token types of the individual symbol(s) that make up the decorator

I would like to see one color for all parts, since decorators are always callables and there shouldn't be misunderstanding if module part is not colored differently, also I like how it looks visually, 3 different colors looks ugly for single (kinda) symbol to me

my concern is that since decorators can be any expression, it seems like a bad idea to highlight every symbol inside the expression as such regardless of whether the individual symbol itself is a decorator. another example off the top of my head is functions that take an argument and return a decorator, eg:

class Foo:
    @foo(bar) # should "bar" be highlighted as a decorator as well?
    def a(self): ...

but anything can be a decorator:

class Foo:
    @baz(qux).asdf[a + b]() # should all of these symbols be highlighted as decorators?
    def b(self): ...

@RayJameson
Copy link

that's a great point, I didn't think about it. I believe in those cases falling back is the right move and actually it doesn't look as bad as I thought
image

@johannesrld
Copy link
Author

johannesrld commented Dec 4, 2024

I mean what would you even highlight as "decorator" in the case of @baz(qux).asdf[a + b]()? the actual decorator function thats called isn't directly referenced by its name in that example.

maybe the best idea would be to highlight everything as a decorator except parameters? (and maybe other things like punctuation and numbers). That would help make it clear that "this is a decorator" instead of just a random bit of syntax with a "@" at the start without completely trampling over highlighting.

@RayJameson
Copy link

RayJameson commented Dec 5, 2024

now that johannesrld said it I think it might be a better approach 🙂

@DetachHead
Copy link
Owner

thanks for the input guys, though i still think my idea to only do it on simple decorators like @foo makes the most sense. it's also less risky as it's less likely for parts of the expression to be incorrectly highlighted due to decorator expressions that i didn't account for

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants