helix icon indicating copy to clipboard operation
helix copied to clipboard

Reverse tree-sitter query precedence ordering

Open the-mikedavis opened this issue 1 year ago • 10 comments

Currently our query precedence order follows the behavior of tree-sitter-cli (the tree-sitter-highlight crate) and is the reverse of the order followed by Neovim and now Zed. For example with a query like so:

((identifier) @constant
 (#match? @constant "^[A-Z][A-Z\\d_]*$"))
(identifier) @variable

Helix will currently treat the first pattern as a higher precedence than the second, so identifiers starting with capital letters will be highlighted as constants. In Neovim for example though, all identifiers would always be highlighted as variables since the later pattern is higher precedence.

In the next release, tree-sitter-highlight will reverse its ordering so that it follows Neovim / Zed. We should reverse our order as well so that queries are closer to being interoperable between editors. The ordering has always been a snag for language support PRs when translating queries from neovim anyways.

The actual change to helix-core/src/syntax.rs in order to reverse the precedence should be very small, and will probably be almost identical to the changes necessary upstream in tree-sitter (https://github.com/tree-sitter/tree-sitter/pull/2412/files#diff-538bdc046297d41717e7a863a54456d47829fb2e298963198cdfb32684500cdd), but we will need to modify all existing tree-sitter queries (runtime/queries/*/*.scm) to respect the new ordering.

Also see https://github.com/nvim-treesitter/nvim-treesitter/issues/3944#issuecomment-1458782497

the-mikedavis avatar Jan 26 '24 16:01 the-mikedavis

If someone is willing to submit a patch for a grammar they're specifically accustomed with, when should that happen?

postsolar avatar Jan 28 '24 19:01 postsolar

I pushed up a branch in https://github.com/helix-editor/helix/pull/9458: reverse-query-precedence-ordering. Feel free to PR against that branch for any languages you'd like to update

the-mikedavis avatar Jan 28 '24 19:01 the-mikedavis

Thank you, I sent a PR for PureScript and might look into Haskell a bit later. Another question though: could you give a link for this bit:

In the next release, tree-sitter-highlight will reverse its ordering so that it follows Neovim / Zed

I couldn't find that announcement in TS repo.

postsolar avatar Jan 29 '24 05:01 postsolar

I'm not sure if there's an announcement in the tree-sitter repo yet. I heard it in a matrix chat we have with some tree-sitter/neovim people

the-mikedavis avatar Jan 29 '24 14:01 the-mikedavis

Upstream TS did the release with reversed precedence order this week, at least that is my understanding of https://github.com/tree-sitter/tree-sitter/releases/tag/v0.21.0.

tgross35 avatar Mar 03 '24 03:03 tgross35

Hi @the-mikedavis ,

We should reverse our order as well so that queries are closer to being interoperable between editors. The ordering has always been a snag for language support PRs when translating queries from neovim anyways.

Does this mean that we can finally track upstream nvim-treesitter queries? What benefit are we bringing by doing our thing?

Please note that, there is no serious tone associated with my questions, just curious.

The primary reason I'm asking is there are issues with our limited support on treesitter, for example, make-range is not supported.

1-1 interoperability is important because that allows us to merge upstream and is supported by bigger community.

For example, in Java language,

The native functions definitions aren't considered as functions. Constructors are not considered functions. These two issues are already covered in upstream. But copying them as is not simple because we use inside/around instead of inner/outer. Additionally the make-range flexibility upstream has allows us to be more specific. I've fixed them locally but it sort of defeats the purpose of having OTB experience.

Same for the sticky context PR, writing rules for each language is a gruesome task, using the upstream rules will allow our efforts to help not just helix but every other editor using these rules...

daedroza avatar Mar 04 '24 06:03 daedroza

There is some effort to unify but no it won't be a 1-1 correspondence

pascalkuthe avatar Mar 04 '24 13:03 pascalkuthe

The actual change to helix-core/src/syntax.rs in order to reverse the precedence should be very small, and will probably be almost identical to the changes necessary upstream in tree-sitter (https://github.com/tree-sitter/tree-sitter/pull/2412/files#diff-538bdc046297d41717e7a863a54456d47829fb2e298963198cdfb32684500cdd), but we will need to modify all existing tree-sitter queries (runtime/queries/*/*.scm) to respect the new ordering.

Where exactly does the change need to be made? It would be great to have before the next release so queries get the chance to work out kinks, I may be able to help.

Does this mean that we can finally track upstream nvim-treesitter queries? What benefit are we bringing by doing our thing?

It is getting significantly better - nvim made the change to use Helix's captures about a month ago https://github.com/nvim-treesitter/nvim-treesitter/pull/5895. If this change to Helix lands then highlights.scm, locals.scm, and injections.scm will be more or less reusable, with a few exceptions (e.g. nvim uses var while helix uses variable)

tgross35 avatar Mar 09 '24 07:03 tgross35

See #9458. I would prefer to have the change merged after a release so that the queries can have some time in master for people to catch bugs before a release. If you want to help out, feel free to open up PR(s) against that branch (for example https://github.com/helix-editor/helix/pull/9476)

the-mikedavis avatar Mar 11 '24 13:03 the-mikedavis

I’m following up on this as I want to benefit from the effort there (context here).

hadronized avatar Jun 15 '24 14:06 hadronized

Would be helpful for script-porting colorschemes from neovim to helix I guess?

sharpchen avatar Sep 12 '24 02:09 sharpchen

This shouldn't have any effect on themes. Neovim uses slightly different captures than us so while this change will make it easier to port queries from Neovim, we won't be able to copy them without changes.

the-mikedavis avatar Sep 14 '24 21:09 the-mikedavis

Apologies for the unclear description. What I intended to say is that I've noticed the treesitter scopes often vary from those in neovim at the same position in certain code, resulting in the ported theme appearing different in helix due to the selection of a different scope. Does this issue relate?

sharpchen avatar Sep 15 '24 00:09 sharpchen

Yeah that's not related: this covers how you write highlights queries (for example in runtime/queries/rust/highlights.scm) and shouldn't have any effect on the highlights we end up producing or the captures / theme keys we use. If this refactor goes well there shouldn't be any difference in the highlights before and after the change.

the-mikedavis avatar Sep 17 '24 23:09 the-mikedavis

See #9458. I would prefer to have the change merged after a release so that the queries can have some time in master for people to catch bugs before a release. If you want to help out, feel free to open up PR(s) against that branch (for example #9476)

24.07 has been released since. Any thoughts on merging this branch now, or do you want to wait another release? Background is that I found some issues in master (for protobuf), and it would be nice to only have to fix them once.

sdoerner avatar Sep 24 '24 18:09 sdoerner

The branch to make this change isn't ready yet as it doesn't cover all languages. The protobuf queries are fairly simple so I think you should send a PR for the fixes you have in mind now. It should be easy to reverse the ordering for those afterwards (and #9458 doesn't reverse protobuf queries yet).

the-mikedavis avatar Sep 24 '24 21:09 the-mikedavis

I guess I’m going to start migrating KTS now!

hadronized avatar Feb 05 '25 18:02 hadronized