fix: classify version requirements by Cargo's compatibility rules - #11
Merged
Conversation
Three classes of requirement were misread, and each failed the same way: a dependency change that a consumer has to react to was reported as compatible, so a crate re-exposing that dependency in its public API was not flagged. A 0.0.x bump was classified as `patch`. Cargo keys compatibility to the leftmost nonzero component, so `^0.0.1` admits only 0.0.1 and 0.0.1 -> 0.0.2 is breaking. Only the 0.x case was handled. A requirement carrying anything but a bare version was parsed as if the prefix were part of the number. `cargo metadata` reports `req` verbatim and only a leading caret was stripped, so `~0.29` left `~0` in the major component, which never equals `0` and so defeated every zero-major test. A requirement can also change without changing the version it starts at, and the old code read that as a patch bump. Narrowing `>=0.29,<0.31` to `>=0.29,<0.30` drops 0.30 from the set a consumer may resolve to, and `>=0.29` -> `>0.29` drops 0.29 itself. Deciding either needs the resolved versions, which a comparison of requirement text does not have. Add `req_norm` and `req_version` to reduce a requirement to the version it starts at — dropping whitespace, a leading caret, pre-release and build metadata, and any comparison operator, and taking a range by its floor — and give `classify_bump` a fourth result, `unknown`, for a requirement that changed without moving that version. Callers that must not raise a false alarm treat `unknown` as not-breaking, so no verdict flips on a widened ceiling; the public-dependency join treats it as a candidate, where reachability in the public API filters it, and reports it as a review item rather than asserting a break. Classification is pure, so each shape is now pinned by a table, alongside two integration cases covering a re-exposed 0.0.x and operator-prefixed dependency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Three classes of version requirement were misread, and each failed the same way: a
dependency change a consumer has to react to was reported as compatible, so a crate
re-exposing that dependency in its public API was not flagged.
patch. Cargo keys compatibility to the leftmostnonzero component, so
^0.0.1admits only 0.0.1 and 0.0.1 -> 0.0.2 is breaking. Onlythe 0.x case was handled.
part of the number.
cargo metadatareportsreqverbatim and only a leading caretwas stripped, so
~0.29left~0in the major component. That never equals0, whichdefeated every zero-major test:
~0.29->~0.30came backminor.read as a patch bump. Narrowing
>=0.29,<0.31to>=0.29,<0.30drops 0.30 from theset a consumer may resolve to;
>=0.29->>0.29drops 0.29 itself.The second is not hypothetical for this tool's own target: a
zcash_primitives0.29 ->0.30 bump is exactly the shape that must produce a lockstep-upgrade entry.
Solution
req_normandreq_versionreduce a requirement to the version it starts at — droppingwhitespace, a leading caret (bare and caret are the same set), pre-release and build
metadata, and any comparison operator, and taking a range by its floor.
Deciding case 3 needs resolved versions, which a comparison of requirement text does not
have. So rather than guess,
classify_bumpgains a fourth result,unknown, for arequirement that changed without moving that version:
0.0.1->0.0.2~0.29->~0.30,=0.0.1->=0.0.2>=0.29,<0.31->>=0.30,<0.320.29.*->0.30.*1.0.0-rc1->1.0.0,0.30.0-pre.0->0.30.00.29->^0.29>=0.29,<0.31->>=0.29,<0.30>=0.29,<0.31->>=0.29,<0.32>=0.29->>0.29*->*Callers treat
unknownasymmetrically, deliberately. The dependency verdict treats it asnot-breaking, so no run flips to
BREAKINGbecause a ceiling widened. Thepublic-dependency join treats it as a candidate, because there it is filtered by whether
the dependency is reachable in the crate's public API — and reports it as a review item
("check whether downstream users are affected") rather than asserting a break, since an
unknownmust not trigger a major version bump.Tests
Classification is pure, so every shape above is pinned by a table extracted and sourced
from the script. Two integration cases check that a re-exposed 0.0.x dependency and an
operator-prefixed requirement reach the public-dependency join.
Ran locally:
shellcheck -S warning zc tests/run.sh,bash -n zc,bash -n tests/run.sh,tests/run.sh(131 tests, 0 failures).