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(treesitter): revert to using iter_captures in highlighter #27901

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

lewis6991
Copy link
Member

Fixes #27895

@lewis6991 lewis6991 marked this pull request as ready for review March 17, 2024 18:37
@github-actions github-actions bot requested a review from bfredl March 17, 2024 18:40
Copy link
Member

@gpanders gpanders left a comment

Choose a reason for hiding this comment

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

I did consider doing this originally, but there was a concern that exposing the match table in iter_captures might surface other bugs. See #27274 (cc @wookayin). At this point it seems like we might just have to pick our poison though.

I had a branch where I was working on an independent fix for the issues that #27274 resolved, so perhaps I can revive that if @wookayin doesn't want to pick it back up.

@lewis6991
Copy link
Member Author

I did consider doing this originally, but there was a concern that exposing the match table in iter_captures might surface other bugs. See #27274 (cc @wookayin). At this point it seems like we might just have to pick our poison though.

I had a branch where I was working on an independent fix for the issues that #27274 resolved, so perhaps I can revive that if @wookayin doesn't want to pick it back up.

My god this is such a minefield!

It seems the most glaring problem in that PR is with the metadata table. The metadata table is match level, so regardless of where we go, I would say that it should always be provided with the match table.

@clason
Copy link
Member

clason commented Mar 17, 2024

Yeah, this is design debt inherited from the earliest implementation (and possibly even upstream). The prepare_iter_matches spaghetti in nvim-treesitter should have tipped us off...

@lewis6991 lewis6991 merged commit 3b29b39 into neovim:master Mar 17, 2024
30 of 31 checks passed
@clason
Copy link
Member

clason commented Mar 17, 2024

That was quick. (Lint was failing, btw.)

@lewis6991
Copy link
Member Author

Dam, pulled the trigger a bit quick. Will fixup at some point.

@wookayin
Copy link
Member

wookayin commented Mar 17, 2024

I just checked this one and #27895; so this PR does revert the use of iter_matches in TS highlighter (#27132). While this is LGTM to me, but can anyone explain why iter_matches would suffer from the bug #27895 whereas iter_captures aren't?

UPDATE: It looks like an "ordering" issue between multiple highlights across different capture/matches, as explained in #27895.

@lewis6991
Copy link
Member Author

Yes, as far as I understand it iter_captures priorities node order first, and then match order, I think, I'm not 100% sure tbh, but something like that.

gpanders added a commit to gpanders/neovim that referenced this pull request May 1, 2024
…neovim#27131)"

This reverts commit 15e77a5.

Subpriorities were added in neovim#27131
as a mechanism for enforcing query order when using iter_matches in the
Tree-sitter highlighter. However, iter_matches proved to have too many
complications to use in the highlighter so we eventually reverted back
to using iter_captures (neovim#27901).
Thus, subpriorities are no longer needed and can be removed.
gpanders added a commit that referenced this pull request May 1, 2024
#27131)" (#28585)

This reverts commit 15e77a5.

Subpriorities were added in #27131
as a mechanism for enforcing query order when using iter_matches in the
Tree-sitter highlighter. However, iter_matches proved to have too many
complications to use in the highlighter so we eventually reverted back
to using iter_captures (#27901).
Thus, subpriorities are no longer needed and can be removed.
phanen pushed a commit to phanen/neovim that referenced this pull request May 3, 2024
neovim#27131)" (neovim#28585)

This reverts commit 15e77a5.

Subpriorities were added in neovim#27131
as a mechanism for enforcing query order when using iter_matches in the
Tree-sitter highlighter. However, iter_matches proved to have too many
complications to use in the highlighter so we eventually reverted back
to using iter_captures (neovim#27901).
Thus, subpriorities are no longer needed and can be removed.
huangyingw pushed a commit to huangyingw/neovim that referenced this pull request May 31, 2024
neovim#27131)" (neovim#28585)

This reverts commit 15e77a5.

Subpriorities were added in neovim#27131
as a mechanism for enforcing query order when using iter_matches in the
Tree-sitter highlighter. However, iter_matches proved to have too many
complications to use in the highlighter so we eventually reverted back
to using iter_captures (neovim#27901).
Thus, subpriorities are no longer needed and can be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree-sitter highlighting no longer takes query range/specificity into account
4 participants