tree-sitter / tree-sitter-javascript

Javascript grammar for tree-sitter
MIT License
314 stars 108 forks source link

Consider releasing 0.21 (and yanking 0.20.2 and 0.20.3) due to a breaking change #294

Closed jorendorff closed 3 months ago

jorendorff commented 3 months ago

0.20.2 changed the name of function expression nodes from function to function_expression.

Broadly speaking, point releases shouldn't break downstream users in the Cargo ecosystem, and 0.20.2 broke us in GitHub code search. We have tree queries that search for functions, and function is no longer a valid thing to ask for.

This might affect other users at any time. The fix would be to re-release 0.20.3 as version 0.21.0, and then yank 0.20.2 and 0.20.3.

I guess the question of when to bump is not really settled tree-sitter ecosystem. It seems bad for a simple cargo update to break users, though. There's a proposal here to treat breaking changes to grammars as requiring a version bump.

jorendorff commented 3 months ago

This got marked as a "bug" by an issue template and I can't change it. I don't mean to imply the parser has a bug.

amaanq commented 3 months ago

Hey, I've wanted to use semantic versioning for tree-sitter core and its grammars - but Max does not want to do that, not until at least core hits 1.0. And to be fair, the spec does say anything pre-1.0 is subject to breaking changes, even if it's a patch increase.

From the semantic versioning spec:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

It's also difficult to reason about what is a breaking change, it really depends on the downstream consumer's usage of it and queries. e.g. if I just use it for parsing or don't use the function node in my queries, then nothing broke technically. So to be strict about that, any change to any rule is a breaking change, which just is not feasible to work with, since only new rule additions would not be breaking technically (and could, by definition, still warrant a minor version bump).

dcreager commented 3 months ago

I've wanted to use semantic versioning for tree-sitter core and its grammars - but Max does not want to do that, not until at least core hits 1.0

Is that true of the grammars too, or just the core libraries? Can you point me to a discussion or issue where that was discussed? I mildly disagree with that for the core libraries, and strongly disagree for the grammars.

amaanq commented 3 months ago

Here's one place - https://github.com/tree-sitter/tree-sitter-regex/pull/19

In another - I can't remember where, but Max wants every grammar + core to effectively be within nearby versions (0.20.x, 0.21.x, etc), so grammars wouldn't get that bump till core does, which I disagree with tbh

jorendorff commented 3 months ago

Hey, I've wanted to use semantic versioning for tree-sitter core and its grammars - but Max does not want to do that, not until at least core hits 1.0. And to be fair, the spec does say anything pre-1.0 is subject to breaking changes, even if it's a patch increase.

From the semantic versioning spec:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Note that Cargo explicitly deviates from the semver spec for 0.y.z releases:

This compatibility convention is different from SemVer in the way it treats versions before 1.0.0. While SemVer says there is no compatibility before 1.0.0, Cargo considers 0.x.y to be compatible with 0.x.z, where y ≥ z and x > 0.

This is why cargo update broke us. In Cargo's view, 0.20.2 should not contain changes from 0.20.1 that are likely to break users.

jorendorff commented 3 months ago

Cargo also has a surprisingly worked-out convention for what changes are considered "major" (such that, for 0.x releases, a "major" change requires bumping x).

The first bullet point in the list is

This of course refers to Rust items, not "items" in the sense of tree-sitter grammar node types. But the tree-sitter grammar is what users code against.

This is a judgment call and it's certainly up to the package maintainers. I think most Rust folks would consider this at least sketchy though.

In another - I can't remember where, but Max wants every grammar + core to effectively be within nearby versions (0.20.x, 0.21.x, etc), so grammars wouldn't get that bump till core does, which I disagree with tbh

Yeah, I can see wanting to do that, but it sure is in conflict with how Cargo uses these version numbers...

dcreager commented 3 months ago

@amaanq thank you for the link! I had not seen that discussion since I'm not watching that grammar repo closely. I've moved some of the points into the higher level tree-sitter discussion in the interests of visibility: https://github.com/tree-sitter/tree-sitter/discussions/1768#discussioncomment-8349789

The main point from there that I'd bring back down here is really just to echo what @jorendorff said above: cargo (and npm and all the others) have specific interpretations of the semver spec, and we should make sure that the guarantees that we are and are not making line up with the version numbers that we publish to these package registries. If every grammar release is possibly backwards changing, we can work around that by using a =0.20.1 requirement, but that's not the default, so I anticipate that a lot of other folks will get bitten by this as well.

maxbrunsfeld commented 3 months ago

Hey, I don't mean to be a blocker if there is a clear plan for improvement, and humans willing to do the work.

There was a time when tree-sitter's own language ABI changed frequently, and grammars' syntax node names changed almost constantly, so that in practice, it was much more important for the version to communicate the ABI version than it was to try to model the syntax tree changing using a version number.

The situation is somewhat different now; the ABI hasn't needed to change in a while, and there exist a lot of mature grammars that don't change that much. I'm fine with changing the policy. Do folks need anything from me to begin the work of sorting things out and instituting a new embrace of semver?

maxbrunsfeld commented 3 months ago

In particular, are there crates.io or npm permissions that need to be granted to particular people at GitHub?

amaanq commented 3 months ago

Well, I can always institute the actions I proposed initially using release-please to automatically follow semver, and generate a changelog in every grammar - it would just require using conventional commits but I think that's a great thing all around. This is what I use in all of my own grammars to follow semver and it works great.