ZedThree commented 3 days ago

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:

[x] CHANGELOG.md updated
[ ] README.md up-to-date

Xophmeister commented 2 days ago

Thank you for your contribution, @ZedThree :pray:

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-toml[^1] 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)

[^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 commented 2 days ago

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

ZedThree commented 2 days ago

@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 :smile: 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 commented 2 days ago

@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 :shrug:

Xophmeister commented 2 days ago

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

ZedThree commented 2 days ago

Formatting done and changelog updated :+1:

ZedThree commented 2 days ago

I figured out why your CI isn't running properly: it only runs on push: https://github.com/tweag/topiary/blob/4dcdc0a474003166cec8a725286d5ad1aea5bb2f/.github/workflows/ci.yml#L1

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.

tweag / topiary