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

feat(theming): separate highlight for function declarations #4892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matoous
Copy link
Contributor

@matoous matoous commented Nov 26, 2022

[proposal]

Add separate highlighting scope for function (and method) declarations that allows styling the declarations differently from their calls. I noticed that in some editors the function declaration are styled differently from function calls which wasn't possible with the current highlight scopes.

If this gets the green light I will take a look at more languages and if they could be adjusted as well (although the scope of languages I am familiar with is quite limited).

Open question: do we also want to be able to style function and method declarations differently? Maybe adding also function.declaration.method would work.

Showcase:

after

Screenshot 2022-11-26 at 09 12 21

before

Screenshot 2022-11-26 at 09 12 42

@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Nov 27, 2022
@matoous matoous changed the title [PROPOSAL] feat(theming): separate highlight for function declarations feat(theming): separate highlight for function declarations Nov 29, 2022
@archseer
Copy link
Member

archseer commented Dec 4, 2022

Sublime/TextMate use entity.name.function:

Function and method names should be scoped using the following, but only when they are being invoked. When defined, they should use entity.name.function.

  • variable.function

https://www.sublimetext.com/docs/scope_naming.html#entity

WDYT @the-mikedavis should we introduce the entity group (needs to be documented in the book too)?

@archseer
Copy link
Member

archseer commented Dec 4, 2022

Actually, a better option would be @function for declarations, @variable.function for invocations. Would need a cleanup in themes though and it might break existing themes :/

@the-mikedavis
Copy link
Member

Yeah I think I would also prefer to change the function invocation scope instead of the definition scope. Changing the definition scope is tricky because a function definition could also be a method so you would need some way to combine those scopes

@matoous
Copy link
Contributor Author

matoous commented Dec 11, 2022

@archseer should this maybe rather be variable.other.function given we currently have variable.other.member for fields of composite datatypes?

Add separate highlighting scope for function (and method) declarations
that allows styling the declarations differently from their calls.
@matoous
Copy link
Contributor Author

matoous commented Dec 12, 2022

So I tried to experiment with the approach above but the issue I am hitting is: I want to distinguish function/method declarations and function/method calls. With the approach where new scope is added (e.g. variable.other.function, variable.other.method, or just variable.method) I can only distinguish method calls but regular function calls (or built-in function calls) are highlighted the same as declarations which brings me to following:

Keeping both function declaration and calls under stingle scope makes things very verbose if we want to add the ability to distinguish between those two, e.g. given:

- `function` - Function and method declarations.
  - `special` (preprocessor in C)
  - `builtin` - Built-in functions (e.q. `println!` in Rust)
  - `method` - Methods
  - `macro`

if I wanted to distinguish declaration from call I would have to add:

- `function` - Function and method declarations.
  - `call` - Function call
  - `special` (preprocessor in C)
    - `call`
  - `builtin` - Built-in functions (e.q. `println!` in Rust)     <--- this is implicitly a call
  - `method` - Methods
    - `call` - Method call
  - `macro`
    - `call` - Macro call

which is imho sub-optimal as it mixes declarations and calls breaking the inheritence benefits.

Thus I wonder if following proposal would make sense: we add 2 scopes under function: declaration and call (the terms themselves are subject to change, tis just a suggestion) to distinguish declarations and calls and both can branch further to distinguish the function/method types, e.g.:

- `function` - Functions and methods.
  - `call` - Function and method calls
    - `special` (preprocessor in C)
    - `builtin` - Built-in functions (e.q. `println!` in Rust)
    - `method` - Method call                                <-- could also be moved under `variable.other.function`
    - `macro`
  - `declaration` - Declaration
    - `method` - Methods
    - `macro`

Theoretically, this could also be extended to remove function.call.method and have this scope under variable.other.function or variable.method instead. This will still most likely require quite a lot of manual work to ensure that all themes keep working as intended (can do that if this gets the green light) but I believe it could serve us better long term. My only concern is that this might be overcomplicated as I am not sure how far we want to go with the complexity of the highlighting scopes that we offer.

@archseer
Copy link
Member

variable.other.function = I'd rather use variable.function to match old Sublime scopes

nvim-treesitter uses @function and @function.call but I agree these are a bit too close

I think you have a point with function.declaration.method etc but I think it's ok to make the theme author have to style the two scopes by hand since this is a more advanced theming that a lot of themes won't deal with in the first place.

My proposal:

  • function/function.{method, builtin} should be definitions/declarations
  • function.invoke/.call (or just variable.function to match existing Sublime/TextMate/VSCode grammars) should be any function or method invocation. It doesn't make sense to distinguish these, and we often can't on the grammar level anyway without a locals.scm query too which should cover this case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants