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

Bump tree-sitter to 0.24 (not a release) #794

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

ZedThree
Copy link
Contributor

@ZedThree ZedThree commented Nov 27, 2024

Bump tree-sitter to 0.24

Closes #684

Description

Biggest difficulty is that tree_sitter::QueryMatches is now a StreamingIterator, returning references rather than values. This makes it very difficult to store in a new type.

Instead make topiary_tree_sitter_facade::QueryMatch a trait rather than a new type. This seems to be the easiest way to preserve as much of the existing structure as possible.

Not tested for the wasm32 build, so possibly some changes required there to match.

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

Biggest difficulty is that `tree_sitter::QueryMatches` is now a
`StreamingIterator`, returning references rather than values. This
makes it very difficult to store in a new type.

Instead make `topiary_tree_sitter_facade::QueryMatch` a trait rather
than a new type. This seems to be the easiest way to preserve as much
of the existing structure as possible.

Not tested for the wasm32 build, so possibly some changes required
there to match.
@Xophmeister
Copy link
Member

Thank you for your contribution, @ZedThree 🙏

Will bumping to Tree-sitter 0.24 affect the grammars we support that, currently, haven't themselves upgraded to 0.24 (or maybe 0.23, I think you said, was where this transitive dependency problem was dropped)? As of writing, it looks like the default branches for the following grammars use:

Grammar Version
tree-sitter-bash 0.24 (dev-dependency)
tree-sitter-css 0.24 (dev-dependency)
tree-sitter-json 0.24 (dev-dependency)
tree-sitter-nickel >=0.21
tree-sitter-ocaml 0.24 (dev-dependency)
tree-sitter-ocamllex 0.20
tree-sitter-query 0.24.3 (dev-dependency)
tree-sitter-rust 0.24 (dev-dependency)
tree-sitter-toml ~0.20 (repo archived)

I suspect that the dynamic loading may have resolved this problem regardless, as tree-sitter-ocamllex and tree-sitter-toml1 both predate the current tree-sitter dependency (0.22.6) and seem to work regardless.

I would try this out myself, but unfortunately, I cannot get your fork to build:

$ cargo build
+ command cargo build
   Compiling topiary-config v0.5.1 (/home/chris/Projects/tweag/topiary/forks/PlasmaFAIR/topiary-config)
error[E0277]: the trait bound `topiary_tree_sitter_facade::Language: From<tree_sitter::Language>` is not satisfied
   --> topiary-config/src/language.rs:164:12
    |
164 |         Ok(topiary_tree_sitter_facade::Language::from(language))
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<tree_sitter::Language>` is not implemented for `topiary_tree_sitter_facade::Language`
    |
    = help: the trait `From<tree_sitter_language::LanguageFn>` is implemented for `topiary_tree_sitter_facade::Language`
    = help: for that trait implementation, expected `tree_sitter_language::LanguageFn`, found `tree_sitter::Language`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `topiary-config` (lib) due to 1 previous error

$ rustc --version
rustc 1.79.0 (129f3b996 2024-06-10) (built from a source tarball)

Footnotes

  1. I believe ~0.20 means >= 0.20.0 && < 0.21, although I'm vaguely aware that "all bets are off" when dealing with 0.* releases...so who knows!?

@yannham
Copy link
Member

yannham commented Nov 28, 2024

There's something wrong with the CI. I wonder if it thinks it's a release because of the version number in the title?

@yannham yannham changed the title Bump tree-sitter to 0.24 Bump tree-sitter to 0.24 (not a release) Nov 28, 2024
@ZedThree
Copy link
Contributor Author

@Xophmeister Fixed! Sorry, I was just being lazy and only building topiary-core locally, as that was the bit I was trying to use in another project 😄 Now all the topiary-cli tests pass locally for me.

I'm pretty sure it's using all of the languages, so it doesn't seem to be a problem that some of the languages are on earlier tree-sitter versions? Just had a quick look into this, and the TSLanguage struct returned by the tree_sitter_<language>() contains a version field, and the tree-sitter library checks that the language is compatible with the library. So, it seems to run fine, and I think it should give a sensible error message if it isn't!

@Xophmeister
Copy link
Member

Xophmeister commented Nov 28, 2024

@yannham IIRC not all CI jobs will run against forks. I believe this is to prevent bad actors from DDOS'ing a GitHub org's CI minutes, but the criteria for what runs or not is a mystery to me 🤷

Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

Thanks, @ZedThree 🙏

I've made a few suggestions where the formatter complains. Nothing significant, but it's preventing the CI tests (nix -L flake check) from passing locally.

Otherwise, as you say, it looks like all the other language formatters are working properly, despite being from different versions.

topiary-config/src/language.rs Outdated Show resolved Hide resolved
topiary-tree-sitter-facade/src/language.rs Outdated Show resolved Hide resolved
topiary-core/src/lib.rs Outdated Show resolved Hide resolved
@Xophmeister
Copy link
Member

Xophmeister commented Nov 28, 2024

@ZedThree Please could you also mention this change in the CHANGELOG.md? You are strongly encouraged to @-mention yourself: credit where credit's due 🙂

@ZedThree
Copy link
Contributor Author

Formatting done and changelog updated 👍

Copy link
Member

@nbacquey nbacquey left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for contributing @ZedThree!

@ZedThree
Copy link
Contributor Author

I figured out why your CI isn't running properly: it only runs on push:

You probably want it to also run on pull_request. That will give you a warning and a button to push manually on PRs raised from forks.

@Xophmeister Xophmeister merged commit 0f4a5f1 into tweag:main Nov 28, 2024
6 checks passed
@ZedThree ZedThree deleted the bump-tree-sitter-less-facade branch November 29, 2024 09:27
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.

Tree-sitter grammars must maintain compatibility with specific Tree-sitter
4 participants