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

[STAL-1960] Add tree-sitter query wrapper #366

Merged
merged 4 commits into from
May 21, 2024
Merged

[STAL-1960] Add tree-sitter query wrapper #366

merged 4 commits into from
May 21, 2024

Conversation

jasonforal
Copy link
Collaborator

What problem are you trying to solve?

As we refactor the JS runtime, we want to be able to efficiently send data between v8 and Rust.

What is your solution?

This PR is part of a larger chain of PRs. However:

  • This PR is mainly about ergonomics, writing code now to make future code less verbose, and thus easier to review. We do this by wrapping tree_sitter::Query to provide a better API for our use case:
    • We want to be able to tag a tree_sitter::Node with a u32 id. Because this all starts with a tree-sitter query, which "captures" nodes, we implement a generic struct that will allow us to cleanly implement this (e.g. TSCaptureContent<tree_sitter::Node> will be transformed into TSCaptureContent<NodeId>)
  • However, there are also significant memory efficiency improvements here, here, and here.
  • A single commit converts our existing use to this new wrapper.

Alternatives considered

What the reviewer should know

  • Don't pay too much attention to 2bd26d5, other than verifying correctness. Basically all of the existing implementation of tree-sitter query captures will need to be deleted/refactored, so this commit is disposable.
  • The memory efficiency improvements will enable a future PR which will reuse a single tree_sitter::QueryCursor for every query, instead of allocating it and dropping it for every file/rule combo.

@jasonforal jasonforal requested a review from juli1 May 16, 2024 16:07
Comment on lines 141 to 146
enum MutCow<'a, T> {
Borrowed(&'a mut T),
Owned(T),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Is there anything that do the same thing in the rust standard library?
  2. Can we put a comment about why we need this?
  3. Also, do you think we will need this outside of the tree-sitter library, it might be worth exposing it as a rust utils function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Unfortunately not, the standard library has Cow<a, T>, but it doesn't work for mutable borrows.
  2. Good point -- I definitely should have done that.
  3. Hmm, we could possibly use it for a TreeCursor, but I am not 100% sure we'll end up doing that, so will just keep it private until we need it elsewhere.

@jasonforal
Copy link
Collaborator Author

Rebasing + merging

@jasonforal jasonforal merged commit 94e4975 into main May 21, 2024
46 checks passed
@jasonforal jasonforal deleted the jf/STAL-1960 branch May 21, 2024 15:14
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.

None yet

2 participants