tree-sitter / tree-sitter-python

Python grammar for tree-sitter
MIT License
372 stars 138 forks source link

bug: release for 0.21.0 #255

Closed jonhoo closed 5 months ago

jonhoo commented 8 months ago

Did you check existing issues?

Tree-Sitter CLI Version, if relevant (output of tree-sitter --version)

No response

Describe the bug

A couple of days ago, tree-sitter 0.21.0 was released. Would be good to get an equivalent release of tree-sitter-python since the two versions need to match up.

Steps To Reproduce/Bad Parse Tree

  1. Attempt to upgrade tree-sitter to 0.21.0 in a project using tree-sitter-python
  2. Get errors about version mismatches (see below)
  3. Be unable to update tree-sitter-python since there is no newer version
error[E0308]: mismatched types
   --> src/lib.rs:144:51
    |
144 |             find_yadr_sections_tree_sitter(input, tree_sitter_python::language(), on)
    |             ------------------------------        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `tree_sitter::Language`, found a different `tree_sitter::Language`
    |             |
    |             arguments to this function are incorrect
    |
    = note: `tree_sitter::Language` and `tree_sitter::Language` have similar names, but are actually distinct types
note: `tree_sitter::Language` is defined in crate `tree_sitter`
   --> /builds/infrastructure/yadr/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tree-sitter-0.20.10/binding_rust/lib.rs:43:1
    |
43  | pub struct Language(*const ffi::TSLanguage);
    | ^^^^^^^^^^^^^^^^^^^
note: `tree_sitter::Language` is defined in crate `tree_sitter`
   --> /builds/infrastructure/yadr/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tree-sitter-0.21.0/binding_rust/lib.rs:54:1
    |
54  | pub struct Language(*const ffi::TSLanguage);
    | ^^^^^^^^^^^^^^^^^^^
    = note: perhaps two different versions of crate `tree_sitter` are being used?

Expected Behavior/Parse Tree

n/a

Repro

n/a
jonhoo commented 8 months ago

This isn't really a bug, nor really a feature request, so both issue templates felt a little weird 😅

clason commented 8 months ago

Would be good to get an equivalent release of tree-sitter-python since the two versions need to match up.

[citation needed]

jonhoo commented 8 months ago

[citation needed]

See the compiler error included in the post.

clason commented 8 months ago

That just means the lockfile has to be updated to the new release (or, in fact, the lockfile should never have pinned an exact version). It doesn't mean that tree-sitter-python needs a new version, nor that the version has to match that of tree-sitter exactly.

jonhoo commented 8 months ago

No, that's not true. This is with a fully up-to-date lockfile. Rust (well, Cargo) considers different 0.x versions to be different major versions, and so when tree_sitter_python returns a tree_sitter::Language from tree_sitter 0.20.x, that type is considered to be distinct from (and incompatible/non-interchangeable with) tree_sitter::Language from tree_sitter 0.21.x. Hence the error.

clason commented 8 months ago

Yes, which means that the crate needs to (correctly) declare compatibility with multiple versions.

jonhoo commented 8 months ago

You're proposing https://github.com/tree-sitter/tree-sitter-python/blob/deba2badc88afd18e6cbd4341ee3c18c3a9bb4ed/Cargo.toml#L24 should be updated to something like

tree-sitter = ">= 0.20.10"

?

I'd generally caution against that — Cargo very quickly gets sad once version ranges that aren't just at semver major version boundaries are involved.

clason commented 8 months ago

That is what I'm proposing, yes, since the current pin is insane from a maintenance point of view.

And, again, nothing here implies that tree-sitter-python needs a release, especially not at the same version.

amaanq commented 8 months ago

I'd generally caution against that — Cargo very quickly gets sad once version ranges that aren't just at semver major version boundaries are involved.

Honestly, I think it's fine for this specific case, because tree_sitter is pulled in only to export the C function that returns a Language. This type is just a wrapper around the C struct under the hood, and it will be fine to bump versions as this struct will almost never change, and if it does, it won't be in an incompatible way.

jonhoo commented 8 months ago

In general, even if that kind of version range is appealing for exactly the reason you state, it also tends to come back to bite you in Rust, often in unintuitive ways. For example, since there will only be one major version of tree-sitter-python across 0.20 and 0.21, and Cargo will have to pick one version of tree-sitter to use, this will mean that consumers who may have multiple versions of tree-sitter in their dependency graph quickly run into problems when trying to get them to interoperate. There's a reason why exactly this pattern is called out in the Cargo docs as something you should be very careful with.

That's not to say "definitely don't do this", just to be aware that you may end up having to change it back later :)

amaanq commented 8 months ago

I'm not saying users who consume the actual library in their code should use this versioning strategy, it's only for these grammar bindings that wish to be used by these consumers such that whatever major version they decide to go with works totally fine in either case. So I still don't see how this would break, your code that uses this Language struct should be written for the appropriate version of tree-sitter (whether that be 0.20.10 or 0.21.0, or later on 0.22.0 etc). I think this is the easiest solution to prevent me having to manually update dozens of grammars every major update in the future, and really shouldn't cause breakage.