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.

Sign up for GitHub

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

Jump to bottom

Cross injection layers in tree-sitter motions #5176

Merged
merged 3 commits into from Mar 23, 2024
Merged

Cross injection layers in tree-sitter motions #5176

merged 3 commits into from Mar 23, 2024

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Dec 15, 2022 •
edited

Tree-sitter motions didn't work well for languages with injections such as Vue or JavaScript inside HTML since only the root layer's syntax-tree was considered. This change uses the range information for each injected language layer to find the appropriate syntax tree so that tree-sitter motions (A-p, A-i, A-o, A-n) work within injected content.

Here the tree-sitter motions work on the JavaScript syntax tree even though the root layer is HTML. Previously, A-o within the script tag would expand the selection to the whole script tag element.

Closes #2311

@the-mikedavis the-mikedavis added C-enhancement Category: Improvements A-tree-sitter Area: Tree-sitter S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 15, 2022
@the-mikedavis
Copy link
Member Author

I think this could be better if we introduce something that wraps the layers in helix_core::syntax::Syntax but has the same API as tree_sitter::TreeCursor.

  • Currently we use tree_sitter::Node::parent which can't cross injection layers, so A-n/A-p get 'stuck' at the edges of a layer
  • We already use TreeCursor for :tree-sitter-subtree, so if we have the same API, we can trivially support printing the subtree for injected content.

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 17, 2022
@the-mikedavis the-mikedavis marked this pull request as draft December 17, 2022 18:45
@pascalkuthe
Copy link
Member

I think this could be better if we introduce something that wraps the layers in helix_core::syntax::Syntax but has the same API as tree_sitter::TreeCursor.

* Currently we use `tree_sitter::Node::parent` which can't cross injection layers, so `A-n`/`A-p` get 'stuck' at the edges of a layer

* We already use TreeCursor for `:tree-sitter-subtree`, so if we have the same API, we can trivially support printing the subtree for injected content.

Sounds like such an iterator might go in the same direction as what you did in the rainbow brackets PR?
I don't have much time this weekend so I didn't get a chance to look at that in more detail so I might just have missunderstood you there.

@the-mikedavis
Copy link
Member Author

They would be similar but the iterator for #2857 covers queries and this would cover walking around the tree without queries. With both together, tree-sitter features are very close to working seemlessly with injections 😀

@pascalkuthe
Copy link
Member

They would be similar but the iterator for #2857 covers queries and this would cover walking around the tree without queries. With both together, tree-sitter features are very close to working seemlessly with injections grinning

That makes sense. I guess I was thinking that the query iterator could be a special case of the general tree iterator. It all seems pretty related to me. But it probably makes sense to keep the PRs separate. I am mostly just thinking out loud here so my ideas might not be possible as I dont't have time to look at code too closely right now :D

@kirawi
Copy link
Member

kirawi commented Dec 22, 2022

I just realized this is also an issue in Rust macros since we use injections for those.

@divarvel divarvel mentioned this pull request Jan 19, 2023
Open
9 tasks
@divarvel
Copy link
Contributor

@the-mikedavis what's the status on this PR? i'm interested in this feature and willing to pick it up if necessary/feasible

@the-mikedavis
Copy link
Member Author

I'm still working on this, I have some changes locally. The current approach works ok but it can have some odd edge cases on the injection boundaries. In order to make it work robustly we need to make a tree out of the injection layers and introduce a cursor type that works on that tree similar to tree_sitter::TreeCursor

@the-mikedavis the-mikedavis mentioned this pull request Jun 6, 2023
Closed
@EpocSquadron EpocSquadron mentioned this pull request Aug 27, 2023
Draft
@themixednuts
Copy link

Hey just wanted to check-in on this PR's status. Love the editor, just getting harder to keep using on my projects based on Svelte.

@the-mikedavis the-mikedavis force-pushed the md-tree-sitter-motions-across-textobjects branch 2 times, most recently from 4f95972 to c0bb3af Compare January 10, 2024 22:27
@the-mikedavis
Copy link
Member Author

the-mikedavis commented Jan 10, 2024 •
edited

I got stuck for a while trying to use the tree cursor with :tree-sitter-subtree. Combined injections make that impossible since multiple nodes can point to the same child layer. In the future we could still print within injections with :tree-sitter-subtree by selecting the layer the selection range is contained in and printing only that layer (edit: #9309). But in this PR I'm only updating A-p/A-o/A-i/A-n.

@the-mikedavis the-mikedavis marked this pull request as ready for review January 10, 2024 22:40
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2024
@the-mikedavis the-mikedavis mentioned this pull request Jan 12, 2024
Open
@the-mikedavis the-mikedavis mentioned this pull request Jan 25, 2024
Open
11 tasks
@the-mikedavis the-mikedavis force-pushed the md-tree-sitter-motions-across-textobjects branch 2 times, most recently from c17141d to 8b5e751 Compare February 13, 2024 21:22
@archseer archseer added this to the next milestone Feb 25, 2024
@the-mikedavis the-mikedavis mentioned this pull request Feb 29, 2024
Closed
@the-mikedavis the-mikedavis mentioned this pull request Mar 17, 2024
Closed
pascalkuthe
pascalkuthe reviewed Mar 17, 2024
View reviewed changes
helix-core/src/syntax/tree_cursor.rs Outdated Show resolved Hide resolved
helix-core/src/syntax/tree_cursor.rs Outdated Show resolved Hide resolved
helix-core/src/syntax.rs Show resolved Hide resolved
helix-core/src/syntax/tree_cursor.rs Show resolved Hide resolved
helix-core/src/syntax.rs Outdated Show resolved Hide resolved
helix-core/src/object.rs Outdated Show resolved Hide resolved
helix-core/src/object.rs Outdated Show resolved Hide resolved
helix-core/src/object.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe modified the milestones: next, 24.03 Mar 17, 2024
@pascalkuthe
Copy link
Member

lets be ambitious and shhot for the next release with this one, I think this mostly looks good, I just found one perf issue and some smaller stuff

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Mar 18, 2024
@the-mikedavis
syntax: Track parent LanguageLayer IDs
c14af06 
This commit adds a `parent` field to the `LanguageLayer`. This
information is conveniently already available when we parse injections.
This will be used in the child commit to create a type that can
traverse injection layers using this information.
@the-mikedavis the-mikedavis force-pushed the md-tree-sitter-motions-across-textobjects branch from 8b5e751 to 7257462 Compare March 19, 2024 13:23
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2024
pascalkuthe
pascalkuthe reviewed Mar 19, 2024
View reviewed changes
helix-core/src/object.rs Show resolved Hide resolved
helix-core/src/syntax/tree_cursor.rs Outdated Show resolved Hide resolved
helix-core/src/syntax/tree_cursor.rs Outdated Show resolved Hide resolved
@the-mikedavis
Add a TreeCursor type that travels over injection layers
1459f57 
This uses the layer parentage information from the parent commit to
traverse the layers. It's a similar API to `tree_sitter:TreeCursor`
but internally it does not use a `tree_sitter::TreeCursor` currently
because that interface is behaving very unexpectedly. Using the
`next_sibling`/`prev_sibling`/`parent` API on `tree_sitter::Node`
reflects the previous code's behavior so this should result in no
surprising changes.
@the-mikedavis
Reimplement tree motions in terms of syntax::TreeCursor
6f47990 
This uses the new TreeCursor type from the parent commit to reimplement
the tree-sitter motions (`A-p/o/i/n`). Other tree-sitter related
features like textobjects are not touched with this change and will
need a different, unrelated approach to solve.
@the-mikedavis the-mikedavis force-pushed the md-tree-sitter-motions-across-textobjects branch from 7257462 to 6f47990 Compare March 19, 2024 15:05
@cgahr
Copy link
Contributor

cgahr commented Mar 22, 2024

Out of curiosity: does this also work for indent queries? Using the above example, if we have an html document and write js in a <script> tag, does this block use the injection queries defined for js?

@the-mikedavis
Copy link
Member Author

No this doesn't interact with queries. #9320 has some of the foundations for running queries across injection layers but it's limited to textobjects to keep the PR small. Moving indentation queries to use that functionality would be a follow-up change

@archseer archseer merged commit 68b2157 into helix-editor:master Mar 23, 2024
6 checks passed
@the-mikedavis the-mikedavis deleted the md-tree-sitter-motions-across-textobjects branch March 23, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archseer archseer archseer approved these changes

@pascalkuthe pascalkuthe pascalkuthe approved these changes

Assignees
No one assigned
Labels
A-tree-sitter Area: Tree-sitter C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
24.03
Development

Successfully merging this pull request may close these issues.

Using expand to parent syntax node inside an injected context selects the whole injection
7 participants
@the-mikedavis @pascalkuthe @kirawi @divarvel @themixednuts @cgahr @archseer