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

fix: lua function declaration name highlighting #250

Merged
merged 1 commit into from
Sep 23, 2023
Merged

Conversation

tmillr
Copy link
Member

@tmillr tmillr commented May 10, 2023

Within a function declaration, the entire function name should be the same color, regardless of whether it's a method etc., and including any punctuation within. This change introduces new treesitter-capture highlight groups which may each be independently overridden by the user.

Note: in Neovim, groups like @x.y.z fallback to @x implicitly if @x.y.z is not defined.

@tmillr
Copy link
Member Author

tmillr commented May 10, 2023

Before

Screenshot 2023-05-10 at 4 32 14 AM

After

Screenshot 2023-05-10 at 4 31 13 AM

Introducing the new highlights query appeared to be necessary after examining the default one provided by Neovim (i.e. it's inadequate for this pr). This new one does not override Neovims's default hl query but extends it.

The new groups allow the user to override the highlighting of each part of the function's name independently, including punctuation. By default, the entire function's name will fallback to @function for x.y() type declarations, and @method for x:y() type declarations. Standard function declarations (e.g. x()) remain unchanged as they are already the correct color and use the correct hl group @function.

Highlight groups for function x.y.z() type function Declarations

Group What's it for?
@function.namespace the x and y in x.y.z() (this is a new group)
@function.punctuation.delimiter the . in x.y.z() (this is a new group)
@function the z in x.y.z()

Highlight groups for function x.y:z() type method Declarations

Group What's it for?
@method.namespace the x and y in x.y:z() (this is a new group)
@function.punctuation.delimiter the . in x.y:z() (this is a new group)
@method.punctuation.delimiter the : in x.y:z() (this is a new group)
@method the z in x.y:z()

Now that I think of it, this pr/change is perhaps more appropriate for Neovim itself, but in the meantime feel free to incorporate it into this repo

@tmillr tmillr marked this pull request as draft May 10, 2023 11:43
@tmillr tmillr marked this pull request as ready for review May 10, 2023 13:28
@nullchilly
Copy link
Contributor

This PR belongs to nvim-treesitter

Also please take performance into consideration, this query isn't good.

@tmillr
Copy link
Member Author

tmillr commented May 11, 2023

This PR belongs to nvim-treesitter

Also please take performance into consideration, this query isn't good.

I get that. If you had read my post you would have seen this comment:

Now that I think of it, this pr/change is perhaps more appropriate for Neovim itself, but in the meantime feel free to incorporate it into this repo

Why isn't the performance very good? Do you have proof of this? There shouldn't be any issues with performance: the file is small and the query only recurses as needed/necessary depending upon the code (number of properties/dot expressions present) that the query is being applied to. The only thing is that there may be a small increase in parse time, however, ts queries are entirely parsed and compiled in C. The entire implementation for running a query is likewise in C, even on Neovim's side (i.e. the entire vim.treesitter API). AFAIK, there is no other way to accomplish this (not without compromising user customization) as ts queries do not have a method for recursion atm. Performance-wise, it shouldn't be much different from using the * or + quantifier (which only works for a sequence of sibling nodes and does not allow recursion).

Yes the solution is not pretty, perhaps a pinch harder to look at and maintain, but it gets the job done and there is no alternative to accomplish the very same thing that I am aware of. The only real caveat is that it only works up to a limited number of indices, but this should be sufficient for 99.9% of code.

If the query "isn't good", do you have an alternative better solution or suggestion?


Performance implications of this pr amount to all of:

  1. one small, extra file to read+parse one time when the first Lua buffer of a given nvim session is loaded
  2. a small, likely negligible, increase in query parse time (compared to the alternative query)

@lucario387
Copy link

lucario387 commented May 11, 2023

Unsure why but wouldn't

(function_declaration
  [
    (method_index_expression)
    (dot_index_expression)
  ] @func)

(method_index_expression ":" @punctuation)
(dot_index_expression ":" @punctuation)

suffice?
Edit: Well it's not the same as the query of this PR, but if having the whole func/method name be the same color then the query above should suffice

@tmillr
Copy link
Member Author

tmillr commented May 11, 2023

Unsure why but wouldn't

(function_declaration
  [
    (method_index_expression)
    (dot_index_expression)
  ] @func)

(method_index_expression ":" @punctuation)
(dot_index_expression ":" @punctuation)

suffice? Edit: Well it's not the same as the query of this PR, but if having the whole func/method name be the same color then the query above should suffice

Yeah something similar to this is what I originally had and would work for making the entire name one color, however it would not allow the end-user to highlight the different parts of the function declaration name independently (e.g. it could not be customized/overridden via the hl group overrides passed to require("gitub-theme").setup(), rather it would require the end-user to create their own query from scratch, hence the visual complexity and added capture/hl groups in my pr).

(method_index_expression ":" @punctuation)
(dot_index_expression ":" @punctuation)

This^ would be too general as this query would also apply to index expressions not in fn declaration names. Also, dot_index_expression is followed by "." not ":", etc.

...but yeah, that's the general idea and is what I originally had.

Also see

@tmillr
Copy link
Member Author

tmillr commented May 12, 2023

Time to read file and parse query, no ;; extends (19 runs each)

Lua
local uv = vim.loop

local function small_query()
  return vim.treesitter.query.parse('lua', assert(io.open('small_query.scm'):read('*a')))
end

local function big_query()
  return vim.treesitter.query.parse(
    'lua',
    assert(io.open('queries/lua/highlights.scm'):read('*a'))
  )
end

local t = uv.hrtime()
parsed = big_query()
vim.print(uv.hrtime() - t, '\n')
for i in {1..20}; do nvim -n -i NONE --clean --headless +'luafile f.lua' +cq; done
Small Query
  1. 1585250
  2. 1418042
  3. 1295959
  4. 1206125
  5. 1193875
  6. 1096875
  7. 1171542
  8. 1163708
  9. 1221959
  10. 1140583
  11. 1235916
  12. 1192917
  13. 1252458
  14. 1176250
  15. 1177792
  16. 1206292
  17. 1134291
  18. 1231666
  19. 1182917

AVG: 1225496ns (1.23ms)

Large Query
  1. 2716750
  2. 2421667
  3. 2247917
  4. 2262875
  5. 2082834
  6. 2081250
  7. 2044875
  8. 2164667
  9. 2078916
  10. 2117667
  11. 2044791
  12. 2150625
  13. 2044125
  14. 2096667
  15. 2136417
  16. 2133958
  17. 2037916
  18. 2104167
  19. 2050584

AVG: 2158877ns (2.16ms)


= +76.16% (~1.76 times slower)

Within a function declaration, the entire function name should be the
same color, regardless of whether it's a method etc., and including any
punctuation within. This change also introduces new treesitter-capture
highlight groups which may each be independently overridden by the user.

Note: in Neovim, groups like `@x.y.z` fallback to `@x` implicitly if
`@x.y.z` is not defined.
@tmillr
Copy link
Member Author

tmillr commented May 18, 2023

I reduced the size of the query/pattern slightly; it now covers up to 8 fields. I believe that this should still be sufficient for most Lua found out in the wild.

The maintainer of the Lua treesitter grammar said that they didn't want to change these recursive nodes to be sequential as this is a common pattern among all of the language grammars. There is an issue open in the treesitter repo for recursive query support, but it has been open for a couple of years now and hasn't gotten that much attention. I still need to incorporate these changes into the default highlight query and see if nvim-treesitter wants it, I haven't done that yet.

@ful1e5 ful1e5 merged commit dcc1e34 into projekt0n:main Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants