obi1kenobi / cargo-semver-checks Goto Github PK

Since the rustdoc JSON data format is unstable, new nightly Rust releases will change the output and make this tool no longer work until it is adapted for the new output format.

Set up a GitHub Action to periodically re-run the test suite with the latest Rust nightly so we can discover these problems as early as possible.

Currently, before cargo publish, the new version of cargo-semver-checks is tested by running the most-recent-prior version of cargo-semver-checks on its API changes. This makes sure we don't break semver for any public APIs, in preparation for #67.

It would be nice to test the new release with itself as well, for two reasons:

• the new release may contain new checks which could detect and prevent new kinds of semver problems
• it's a low-cost way of getting more "real-world" test coverage of the new release before it's published on crates.io

For me, its up to the reviewer to decide if a compatibility break is acceptable. I would love to be able to label the PR.

This can either come from direct support or documentation on how to integrate with other actions

Current, everything is under cargo semver-checks check-release. Deeply nested subcommands like that should be in rare cases and we should be cautious of having long command names like that.

Do we need that extra subcommand? Is there another way of expressing that idea?

Let's first breakdown expected workfows (which will help with #47).

The overall aim is to help with API evolution,. including

• Report to reviewers that a breaking change was made when it shouldn't
• Report to reviewers that a breaking change was made to ensure it gets documented
• Check local changes for unexpected breaking changes
• Check local changes for breaking changes to document
• As a user of a crate, find out what changed in its API
• Get a suggested minimal version to upgrade to
• Check that we aren't upgrading to too small of a version

Describe your use case

There has long been an RFC for "private" dependencies, which would help crates ascertain that they don't depend on a (particular version of) a crate in public API. Unfortunately it seems like doing this is complex enough in Cargo that no one has gotten around to it.

From reading your blog post it seems like this tool would have access to all the relevant data to allow it to warn when a dependency’s type or trait appears in public API.

This is an important part of semver since bumping a public dependency across a semver-incompatible boundary of course means your own semver is affected, too.

Describe the solution you'd like

It would be great if there was a way to configure this tool to allow or deny dependencies from appearing in public API.

Alternatives, if applicable

No response

Additional Context

No response

With --rev, --tag, --branch flags so someone can compare the API against a repo

This would be exclusive with #50

Hi! I have some concerns about use of rustdoc for non-documentation purposes. In my experience rustdoc regularly has regressions/bugs (a recent example w/ my crate: rust-lang/rust#100204; a long standing bug: rust-lang/rust#83375) (I'm sorry, rustdoc team ;-;) which make it a questionable choice for correctness checks like this crate.

A better (in my opinion anyway) way of getting information about crates would be to directly call compiler APIs. The drawback though is that compiler APIs are unstable, so then this tool would need to be shipped with the compiler like clippy or rustdoc that are linked with a specific version of the compiler (I'm not sure how doable this is). And of course, there is an implementation cost, someone needs to do this.

See clap-cargo for help

This is a list of all not-yet-implemented checks that would be useful to have. Some of these require new schema and adapter implementations as well, tracked in #241.

In addition to checking for semver violations, there are certain changes that are not breaking and don't even require a minor version, but can still be frustrating in downstream crates without a minor or major version bump. Crates should be able to opt into such warnings on an individual basis.

For example, based on this poll (with small sample size: ~40 respondents), ~40% of users expect that upgrading to a new patch version of a crate should not generate new lints or compiler warnings. The split between expecting a new minor version and a new major version was approximately 3-to-1.

Major version required

• #578
• exhaustive enum becomes #[non_exhaustive]: #143
• repr(C) plain struct has fields reordered
• tuple struct has fields reordered
• tuple enum variant has fields reordered
• reordering the variants of an enum that does #[derive(PartialOrd)]
  • this will change the runtime behavior of the ordering
  • context: rust-lang/rust#51561 (comment)
  • also, #305 may hint at more breaking changes here
• pub struct pub field removed
• pub struct constructible with struct literal adds pub field (#233)
• pub struct constructible with struct literal adds non-pub field, and cannot be constructed with a literal from outside its own crate anymore (#233)
• pub struct pub field changes type: #148, blocked on #149
• pub enum variant field changes type: blocked on #149
• #554
• pub enum struct variant adds field: #238
• pub enum struct variant removes field: #153
• pub enum variant discriminant removed
• pub enum variant discriminant changed value
• removed direct re-export of an enum variant: #291
• struct with public fields changes to another kind (more details: rust-lang/cargo#10871 (comment))
  • unit struct to plain struct is breaking since the implicit constructor disappears (Rust for Rustaceans, Chapter 3, "Type Modifications", page 51)
  • #242
• union-related lints: #633
• #482
• pub fn moved, deleted, or renamed (#22, #23, #24)
• pub fn changed return type: blocked on #149
• pub fn added argument
• pub fn removed argument
• pub fn changed arguments in a backward-incompatible way
  • This one is hard: it's possible to go from e.g. taking &str to taking S: Into<String> without breaking
  • when it's not breaking, it requires a minor version
• #190
• #191
• #503
• ABI breaking changes in #[no_mangle] or #[export_name] functions: #502
• pub method moved into a trait
  • even if the trait is pub, it needs to be imported in the scope to have its methods be available
  • Make sure to test for both trait-provided (default impl) methods and explicitly implemented methods for the trait. See the test cases added in #24 for example.
• repr(C) removed from struct or enum (#25)
• repr(transparent) removed from struct or enum (#26, #28)
• repr(u*) and repr(i*) changed/removed from enum (#29, #30)
• #74
  • breaking because of:
    • https://jack.wrenn.fyi/blog/semver-snares-alignment/
    • https://old.reddit.com/r/rust/comments/kr41sq/semver_snares_sizedness_and_size/
• repr(align(N)) changed to a lower N, if that actually changes the alignment of the type
  • It's possible that changing to a smaller N doesn't immediately cause a breaking change, because one of the contained fields has higher alignment requirements and ends up in practice causing the overall type's alignment to remain unchanged.
  • Should we warn on this anyway, though? Or should we only warn if alignment has changed in practice?
  • The argument for "warn always" is that the contractual promise has changed, and any excess alignment is now a fluke and implementation detail that could change in the future.
  • https://doc.rust-lang.org/cargo/reference/semver.html#repr-align-n-change
• repr(align(N)) changed to a higher N, if that actually changes the alignment of the type
  • This case is harder -- making a stronger promise than before is only breaking if it in practice imposes a higher requirement. If a field of the type already required higher alignment such that the new alignment is a no-op compared to the old alignment, that's not breaking and we shouldn't report that.
  • We shouldn't add a lint here until we can check the alignment of contained fields.
  • https://doc.rust-lang.org/cargo/reference/semver.html#repr-align-n-change
• #632
• repr(packed(N)) changed to a lower N
  • same arguments as repr(align(N)) with lower N discussed above -- we probably want a lint for this
  • be careful, because repr(packed) is also valid syntax! the N is implied as N=1 if missing
  • https://doc.rust-lang.org/cargo/reference/semver.html#repr-packed-n-change
• repr(packed(N)) changed to a higher N
  • same arguments as repr(align(N)) with higher N discussed above -- we should probably hold off until we can get layout info for fields
  • https://doc.rust-lang.org/cargo/reference/semver.html#repr-packed-n-change
• type is no longer Sized / Send / Sync / Unpin / UnwindSafe / RefUnwindSafe (auto traits) (#31)
• type made Copy (appears to be breaking because of rust-lang/rust#100905 )
• #73
• non-sealed trait added method
• #294
• non-sealed trait removed associated const default value
• non-sealed trait added associated type
• trait newly became sealed
  • this is partially covered by the "trait is not importable" rule, since one way to seal a trait is to make it unimportable
  • but another way to seal a trait is to make implementing or calling it require a private ZST that downstream crates cannot name, and this is not covered
  • reference: https://fosdem.org/2023/schedule/event/rust_rust_api_design_learnings/ time code ~00:30:00
• trait removed/renamed associated type
• #441
  • example breaking change in the real-world: https://github.com/awslabs/smithy-rs/pull/2577/files#diff-82252c5a4ad7e9d34be9254db1afe172d0f01c067b121de20ff7f7f29d7cf223L24
• #232
  • example of why this is breaking: https://twitter.com/i2talics/status/1559607755975057408
• #231
• #250
• #368
• #605
• #606
• type no longer implements pub trait
• implementing an existing pub trait for an existing type
  • breaking because the trait's methods or associated types on that type may be ambiguous relative to those from traits implemented for that type in another crate (Rust for Rustaceans, chapter 3, pg. 52, "Trait Implementations")
• implementing a new pub trait for an existing type, if the new pub trait is in a prelude module that gets imported with a wildcard
  • similar to above, same source (Rust for Rustaceans, chapter 3, pg. 52, "Trait Implementations")
  • normally the trait has to be in scope for its methods to be available, but the wildcard import will bring it in scope here
• blanket impl added for an existing trait
  • the blanket impl can cause a conflict with a downstream type that also implements the trait on one of its own types, if that type is covered by the blanket impl -- source: Rust for Rustaceans, chapter 2, pg. 30, "Blanket Implementations")
• blanket impl added over a fundamental type (&T, Box etc.)
  • similar reasoning as above -- source: Rust for Rustaceans, chapter 2, pg. 30, "Fundamental Types"
• added new implementation of existing trait that does not contain at least one new local type (and that type satisfies the exemption from the orphan rule)
  • source: Rust for Rustaceans, chapter 2, pg 31. "Covered Implementations"
• upgrading to new major version of dependency while exporting a type that implements a trait from the dependency (new major version -> "it's not the same trait as before"): libp2p/rust-libp2p#3170 (comment)
• #635
• changes on associated types / trait bounds
  • more details in thread here: https://twitter.com/compiler_errors/status/1652410429501771778
  • adding a trait bound on a generic type in a function or type signature
  • adding a trait bound on an associated type
  • removing a trait bound from an associated type
  • adding ?Sized on a trait associated type, breaks fn bad<T: Tr>() -> T::Assoc: https://twitter.com/withoutboats/status/1701274760653533637
  • removing ?Sized on a trait associated type, breaks impls that used an unsized type there
  • removing ?Sized bound from from a generic type in a function or type signature
  • adding ?Sized bound to a generic type in a function or type signature: #532
  • removing a trait bound from return position impl Trait
  • adding ?Sized in return position impl Trait (say -> Box<impl Foo> changing to -> Box<impl Foo + ?Sized>)
  • removing a bound on trait impl: #142
  • adding trait bounds on trait impl
• Auto trait impls for impl Trait in return type.
  • Requires a superset of the required schema additions as pub fn changed return type
• #338
• pub type typedef changes the order of generic arguments (regular, lifetime, or const generics) relative to the underlying type
• pub type typedef adds a new generic parameter
• pub type typedef removes a generic parameter
• pub type typedef removes a default value for a generic parameter
• pub type typedef changes a default value for a generic parameter
• variance of type lifetime parameters changed
  • example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bb4738835367fd9ef604a8fcd728699b
  • source: https://twitter.com/tenellous/status/1620907046911754240
  • implementation idea: #480
• addition of Drop implementations changing type parameter lifetime requirements
  • source: tokio-rs/tracing#2578
• any ABI change on a fn pointer argument to a function
  • in Rust, ABIs are considered equal by comparing their literal strings
  • changing pub fn blah(extern "C" fn()) to pub fn blah(extern "C-unwind" fn()) will almost certainly break people
• pub const moved, deleted, or renamed
  • changing to pub static is still breaking: cannot use static in const fn; cannot initialize another const with a static like pub const MY_CONST = other::PREV_CONST_NOW_STATIC
  • might be good to eventually split this into two lints, one for each case, for the sake of good error messages
• pub static moved, deleted, or renamed -- but not changed to pub const
  • lint variants: at top level of module / inside an impl block
• pub static changed to pub const, with caveats
  • lint variants: at top level of module / inside an impl block
  • breaking change if the type has interior mutability -- let foo: &'static T = &MY_UNSAFE_CELL_STATIC is fine but doesn't work if the value is a const instead of static
  • breaking change if the type is std::mem::needs_drop::<T>() (not the same as T: Drop -- String is not drop itself but its contents need dropping)
    • breaking change is around lifetime promotion: this doesn't work with const but works fine for static
• public API type that implements serde::Serialize + serde::Deserialize gains new fields that are not #[serde(default)]
  • This breaks the serialization format.
  • Idea from here: https://twitter.com/ManishEarth/status/1647675422648463360
• more lint ideas here: rust-lang/cargo#12169

Minor version recommended

• #57
• #159
  • Code in downstream crates that did not use a value of that type will get a compiler lint.
  • New lints for existing code requires a minor version: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-introducing-new-lint-warningserrors
• new pub struct added
• pub fields added on pub struct
  • don't report this if the entire struct is new
• new pub enum added
• new pub enum variant added
  • don't report this if the entire enum is new
• pub enum variant discriminant added
• new pub inherent method added
  • don't report this if the entire type is new
• new pub union added
• pub type typedef adds a default value for a generic parameter

Project-defined whether major / minor / patch version required

For example, because they are technically breaking but projects may often treat them as non-major.

• Raising the Minimum Supported Rust Version (MSRV) for the crate
• Changing the size of a type
  • Example courtesy of cargo-breaking README file

General opt-in warnings

• Depending on a #[non_exhaustive] type from another crate to remain a 1-ZST usable in #[repr(transparent)]
  • This is a semver hazard from the user's side. The other crate's type correctly declared that field additions (including sized ones) are non-breaking.
  • Related issue: rust-lang/rust#78586
• pub type is not equivalent to pub use -- enum (or pub use) replaced by pub type is currently breaking because use upstream::Enum::* won't work through pub type
  • #413
  • not a major change, as described here: https://predr.ag/blog/re-exporting-enum-with-type-alias-breaking-change-not-major/
  • happened in: time-rs/time#675
  • will stop being a hazard if this feature is built: rust-lang/rust#73191
• Crate does not enforce some of the recommended allow-by-default lints that are built into Rust and/or clippy: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html
  • For example, own public types should be Debug i.e. the Rust missing_debug_implementations lint: https://twitter.com/Lucretiel/status/1558287048892637184
• Types with a new() -> Self method should be Default: https://rust-lang.github.io/api-guidelines/interoperability.html
• Don't require FusedIterator in generic bounds, instead use Iterator.fuse(): https://doc.rust-lang.org/std/iter/trait.FusedIterator.html
• Newly adding #[inline] on a pub fn in the public API, since that may have unexpected negative perf impact in some cases (e.g. slower cargo test if that crate is a dependency and compiled with opt-level = 3): https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Cargo.20wizard.3A.20automating.20Cargo.20project.20configuration/near/425816132
• public API type that implements serde::Serialize + serde::Deserialize has private fields
  • Because those private fields can be manipulated by serializing, editing, then deserializing.
  • Idea from here: https://twitter.com/ManishEarth/status/1647680963122724864

Opt-in warnings for difficult-to-reverse changes

• Removing #[non_exhaustive] from an item
  • Per semver, removing #[non_exhaustive] can be done in a patch release, but adding it back would then require a new major version.
• Adding an enum variant in a #[non_exhaustive] enum
  • Per semver, adding variants to a non-exhaustive enum can be done in a patch release, but removing them again afterward would require a new major version.
• Removing the last non-pub field in an exhaustive public struct
  • Structs that are not #[non_exhaustive] and have only public fields can be constructed with a struct literal. Removing the ability to construct a struct with a struct literal is a breaking change and requires a new major version.
• Making an item importable in more than one way
  • If an item is in a pub mod and is also exported with pub use, it can become importable in multiple ways. This is easy to miss. Removing an import path is breaking, so perhaps we should warn that this is happening. Related to #35.
• Making a trait object-safe if it previously was not
  • Object safety then becomes part of the API contract, and breaking object safety is semver-major.
• A 1-ZST (1-byte-aligned zero-sized-type) type no longer being a 1-ZST
  • This is "possibly breaking" and whether it's breaking or not depends on the intent of the type, and can't be determined programmatically. A #[enforce_1zst] attribute could signal that the type should remain a 1-ZST and that deviations from that are breaking.
  • This can break downstream even if the type is #[non_exhaustive], until rust-lang/rust#78586 is resolved and prevents this.
• Leaking or re-exporting another crate's type in one's own API
  • for example, having a function that returns a value of another crate's API
  • this can cause coupling to the other crate's version, and can be a pain
  • there are legitimate reasons to do this sometimes, but it should be an intentional decision and probably worth flagging in review
• Making a type Send/Sync/Sized/Unpin or other auto traits, when it previously wasn't.
  • this is possible to do indirectly, e.g. by removing the last field that prevented the type from (auto-)implementing those traits
  • reverting this is a breaking change

More checks to triage here

• rust-lang/cargo#8736
• https://doc.rust-lang.org/cargo/reference/features.html#semver-compatibility
• https://rust-lang.github.io/api-guidelines/interoperability.html
• https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md
• https://www.lurklurk.org/effective-rust/semver.html
• https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
• #5 (comment)

Cargo is dual-licensed so I assume dual licensing will make it easier to merge. The sooner, the better to get buy-in from all copyright holders (ie contributors)

Note:: I am speaking from a position of ignorance on licensing

Right now, glob imports are ignored for purposes of calculating the "importable paths" of an item.

This is likely to cause false-negative issues where items aren't scanned but they should be. If moving from a non-glob to a glob pub use, this could also cause a false-positive "item was removed" issue. The circumstances to trigger a false-positive are exceedingly rare, but this should be fixed soon regardless.

Point to another path to compare APIs

This is exclusive with #50, #51

There are valid use cases for running additional (not-necessarily-semver) user-provided checks against a codebase. A few examples:

• Ensuring that a builder pattern struct always uses either &mut self or self and doesn't mix across them.
• Ensuring that certain types don't leak out through the API, even though they are public (perhaps from another crate).

These are all reasonable queries runnable over the existing schema and Trustfall adapter used here. We just need to design the CLI for specifying the additional checks, and the file format (serde struct) the additional check files should use.

Tangentially related to #5.

I was trying cargo-semver-check for the first time on a small project of mine and this was the result:

$ cargo semver-checks check-release
    Updating index
     Parsing escape8259 v0.5.1 (current)
     Parsing escape8259 0.5.0 (baseline)
Error: Failed to parse rustdoc JSON output file "/home/eric/work/rust/escape8259/target/semver-checks/registry-escape8259-0_5_0/target/semver-checks/target/doc/escape8259.json"

Caused by:
    missing field `name` at line 1 column 774

My project is here; my working directory is clean and I have the top of the master branch checked out (09529d6).

My default toolchain is 1.63, and I cargo-semver-check is 0.9.0.

Steps to reproduce the bug with the above code

On Windows 10:

git clone https://github.com/Bromeon/js-sandbox.git
cd js-sandbox
cargo semver-checks check-release -p js-sandbox

Actual Behaviour

cargo semver-checks check-release -p js-sandbox
    Updating index
     Parsing js-sandbox v0.2.0-rc.0 (current)
Error: Failed when running cargo-doc on <path>\js-sandbox\Cargo.toml: error: no such subcommand: `+nightly`

Expected Behaviour

Getting output about semver compatibility, or at least a logic-related error.

Additional Context

I tried with both cargo-semver-checks versions 0.9.1 and 0.9.2, same results.
I also tried alternative ways to invoke Cargo:

rustup run nightly cargo semver-checks check-release
    Updating index
     Parsing js-sandbox v0.2.0-rc.0 (current)
Error: Failed when running cargo-doc on <path>\js-sandbox\Cargo.toml: error: no such subcommand: `+nightly`

        Cargo does not handle `+toolchain` directives.
        Did you mean to invoke `cargo` through `rustup` instead?

It works when I run it on WSL2 (Linux) from the same Windows machine.

Debug Output

Software version

cargo-semver-checks 0.9.2

Operating system

Windows 6.2.9200

Command-line

C:\Users\<me>\.cargo\bin\cargo-semver-checks.exe semver-checks check-release -p js-sandbox --bugreport

cargo nightly version

> cargo +nightly -V
cargo 1.65.0-nightly (6da726708 2022-08-23)

Compile time information

• Profile: release
• Target triple: x86_64-pc-windows-msvc
• Family: windows
• OS: windows
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,sse,sse2
• Host: x86_64-pc-windows-msvc

It would be useful to export the discovered semver issues and/or warnings in a machine-readable format (e.g. JSON).

For example, this would allow the automatic inclusion of changes that may cause lints in downstream crates into a changelog. See this tweet for details: https://twitter.com/russellrcohen/status/1549065325186007044

Related to #5.

Not sure what the easiest and best way to do this is.

Every so often, new Rust nightlies might cause the rustdoc JSON output format to change in a backward-incompatible way.

cargo-semver-checks will remain compatible with the most recent nightly's output, and we have a test that ensures that. However, users may still be using an older (or newer!) nightly that is incompatible with their cargo-semver-checks version.

If the JSON fails to parse, we should:

• check the nightly version to see if it's known to not work because it's too old (related: #88), then advise the user to upgrade to a newer nightly Rust
• otherwise, the nightly may be too new for the cargo-semver-checks version and we should advise the user to try upgrading to a newer cargo-semver-checks (as in #98)

Steps to reproduce the bug with the above code

git clone https://github.com/diesel-rs/diesel
cd diesel
git checkout v2.0.0
cargo semver-checks check-release --manifest-path diesel/Cargo.toml --baseline-version 1.4.8

Actual Behaviour

cargo semver-checks check-release --manifest-path diesel/Cargo.toml --baseline-version 1.4.8
    Updating index
     Parsing diesel v2.0.0 (current)
     Parsing diesel 1.4.8 (baseline)
Error: Failed to parse rustdoc JSON output file "/home/weiznich/Documents/rust/diesel/target/semver-checks/target/doc/diesel.json"

Caused by:
    recursion limit exceeded at line 1 column 203398

(The column number varies between different runs)

Expected Behaviour

cargo semver-checks check-release reports a number of API breaking changes between diesel 1.4 and diesel 2.0

Additional Context

No response

Debug Output

Software version

cargo-semver-checks 0.9.1

Operating system

Linux 5.18.18-200.fc36.x86_64

Command-line

/home/weiznich/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo nightly version

> cargo +nightly -V 
cargo 1.65.0-nightly (4ed54cecc 2022-08-27)

Compile time information

• Profile: release
• Target triple: x86_64-unknown-linux-gnu
• Family: unix
• OS: linux
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,llvm14-builtins-abi,sse,sse2
• Host: x86_64-unknown-linux-gnu

Breaking because of:
https://jack.wrenn.fyi/blog/semver-snares-alignment/
https://old.reddit.com/r/rust/comments/kr41sq/semver_snares_sizedness_and_size/

Now documented at:
https://doc.rust-lang.org/cargo/reference/semver.html#type-layout

Sometimes it's useful to be able to change the alert level of specific lints for a particular item or an entire module. Clippy and rustc lints have already stabilized syntax like #[deny(lint_name)] and #[allow(lint_name)], and our syntax choices would ideally be reasonably compatible with those.

The kinds of controls we'd like to include are:

• Forbid: change not allowed regardless of version bump
• Major change: only allowed in major version bumps
• Minor change: only allowed in minor version bumps
• Informational: allowed in all version bumps, but nevertheless reported; for example, if human judgment is necessary to determine whether the change is concerning or not
• Ignored: not reported at all

The lint level control adjustments should allow both raising and lowering the level of a given lint. For example:

• semver-minor lints should be possible to be upgraded to semver-major
• semver-major lints should be possible to be downgraded to semver-minor
• it should be possible to annotate an entire module as downgrading all semver lints to ignored / informational, e.g. in macro-only or explicitly-unstable modules.

Semver says deprecations are minor changes.

Reference: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute

Will require separate checks for:

• items (struct, trait, fn, etc.)
  • struct, enum
  • union
  • trait
  • fn
  • const / static (GlobalValue)
  • other items?
• enum variants
• struct fields
• trait items
• macro definitions
• external blocks

Unfortunately, rustdoc JSON output does not seem to output re-export information in paths, so we don't seem to be able to get information on re-exports which we can query for semver issues.

Upstream tracking issue: rust-lang/rust#93522

Blockers

• #84
• #47
• #86
• #49
• #50
• #51
• #52
• #53
• #54
• #55
• #58
• #48 so we can verify the CLI is general enough
• Feature parity with cargo-semverver
• rust-lang/cargo#10882 (see #321 for context)
• rust-lang/cargo#10881 (see #315 for context)
• rust-lang/rust#44663

Nice-to-haves

• #60
• #56
• #57
• #59

Open questions

• Integrated into cargo or separate rustup component like clippy?

Things to remove

• Leading _ is hidden (blocked on rust-lang/cargo#10882)
• bench, nostd, no_std, private, unstable, unstable[-_].*, nightly (blocked on rust-lang/cargo#10882, rust-lang/cargo#10881)
• --default-features, --only-explicit-features

Merge Proposal (example):

Motivation

Drawbacks

Behavior

Alternatives

Prior Art

Future Possibilities

Technically, semver says that these are minor changes.

Not all want to treat it as such and is an example of why people would want control over lints treatments, see #58

Currently, termcolor detection thinks that GitHub Actions don't support color, but they do. Setting CARGO_TERM_COLOR=always makes cargo's own output colorized but cargo-semver-checks output remains plain, which is jarring.

https://doc.rust-lang.org/cargo/reference/config.html#termcolor
https://docs.rs/termcolor/latest/termcolor/enum.ColorChoice.html

The API extends past the rust code to the manifest

Possible checks include

• Explicit feature removed (major breaking)
• Implicit feature (optional dependency) removed (informative)
  • This is separate because pre-1.60's namespaced features, crates couldn't hide these
• Implicit feature added via removing dep: reference to optional dependency (informative)
• Explicit feature no longer implies another explicit feature (major breaking)
• Feature no longer implies an implicit feature (informative)

Consider a more cargo test / cargo nextest-like output, where the individual tests show as pass/fail but the summary is at the very end. This would speed up the time-to-known-outcome, as the result iterators would only be .peek()-ed once and then saved until the final summary is generated. This is the laziest of all possible evaluations.

Also consider fewer sections in the left-hand side, and maybe focus on just a "Help" section together with each failure in the final summary. This way, folks don't have to figure out what each section does.

Steps to reproduce the bug with the above code

If a crate has one or more previously published versions and you have been making changes to master without incrementing the version in Cargo.toml,¹ running cargo semver-checks check-release does not behave as it should; namely, it assumes the version number in Cargo.toml is unpublished and when searching for prior versions of the crate does not use the available information to correct that assumption.

The behavior differs depending on the crate's publication history:

• If there is only one published version and Cargo.toml still has that same version number, cargo-semver-checks (as of 0.9.2) reports "No published versions for ..."
• If there are multiple published versions, I'm guessing that cargo-semver-checks will check the current Cargo.toml version (assuming it to be represented by the current code in git) against any previous releases rather than checking the current code against the version indicated in Cargo.toml.

I won't presume to tell you that you can't assume Cargo.toml is updated to a new version number as soon as the previous version ships, but I can tell you that this assumption isn't going to cover everyone.

¹ a common school of thought holds that your git tag x.y.z should be the commit that bumps the official version number to x.y.z

Actual Behaviour

See above

Expected Behaviour

If the version in Cargo.toml is discovered to already have been published, ideally cargo-semver-checks would continue as if the patch version were being incremented and report changes diffed against the already published version with the same version number currently in Cargo.toml.

Failing that (and that would be really disappointing if we can't make this happen - I'm willing to help), then an error should be emitted as soon as it is discovered that the version number in Cargo.toml has already been published. This would prevent the currently unambiguously incorrect behavior (ranging from incorrect error messages to diffing against the wrong version) from being reached, at the very least.

Additional Context

I was going to open a PR with just the following change, but I realized that it wasn't enough because that would only cover the case where a crate has only one previously published version and Cargo.toml still points to that version number:

diff --git a/src/baseline.rs b/src/baseline.rs
index 06f0d20..1b9714f 100644
--- a/src/baseline.rs
+++ b/src/baseline.rs
@@ -222,7 +222,7 @@ impl BaselineLoader for RegistryBaseline {
                 .rev()
                 .find(|v| v.pre.is_empty())
                 .or_else(|| instances.last())
-                .with_context(|| anyhow::format_err!("No published versions for {}", name))?;
+                .with_context(|| anyhow::format_err!("No published versions for {}. Did you try incrementing the version in Cargo.toml?", name))?;
             instance.to_string()
         } else {
             let instance = crate_

Debug Output

N/A

I'm thinking "if this were merged into cargo, what name should it have?"

Quick thoughts on criteria

• Brief
• Informative, including the idea of non-semver checks

Ideas

• cargo api (currently taken)
• cargo crate-api (I have this one)
• cargo diff (available)
• cargo preflight (reserved by @obi1kenobi)

Like #6 being an output mode, this would be another that would provide a snippet for including in a changelog. No-touch additions (automatically insert into file, using a particular users format, etc) are non-goals. The focus is on a starting point as the user will likely want to add more information as well and we want to encourage users to do so rather than it being "good enough".

The hope is this will encourage people to write changelogs more.

For unknown reasons, rustdoc JSON output sometimes does not include information about all the traits implemented by a type unless --document-private-items is specified. For example, in this clap semver regression:

• The following invocation on v3.1.18 generates JSON stating that UnwindSafe is implemented for ArgMatches: cargo rustdoc --lib --all-features -- --document-private-items -Zunstable-options --output-format json
• However, the following invocation has no mention of UnwindSafe for ArgMatches at all: cargo rustdoc --lib --all-features -- -Zunstable-options --output-format json

It's unclear to me at the moment whether this is expected behavior from rustdoc, or a bug. Will triage and use this as a tracking issue.

In the meantime:

• the docs should recommend --document-private-items and --all-features in the rustdoc invocation
• the GitHub Action should use both of these flags by default

The language needs to convey that this is incomplete as

• Not all checks are written
• There are semantic breaking changes that are not checked

Steps to reproduce the bug with the above code

It is possible to use #[doc(hidden)] to implement seemingly-breaking changes without actually breaking semver compatibility.

Since cargo-semver-checks only uses the json rustdoc output to form its analysis, the claim that cargo-semver-checks has no false positives should probably have this shortcoming noted in the readme.

e.g. these two versions of the same code below are technically semver compatible:

pub struct Foo {}
pub struct Bar {}

#[doc(hidden)]
pub struct Foo {}
pub struct Bar {}

as merely hiding a type (or as is more often, a method) from the documentation doesn't break the semver contract.

or, for a more interesting case, the following:

pub struct Fo {}

pub struct Foo {}
// The previous release included a typo in the type name so the following is kept for backwards-compatibility:
#[doc(hidden)]
pub type Fo = Foo;

There is a lot more dark magic that can done that would break docs but preserve backwards compatibility. Relying solely on the docs is a great and efficient way of implementing straightforward semver breakage cases, but I don't think it's fair to claim that any tool using this approach has no false positives.

Actual Behaviour

A breaking change is reported.

Expected Behaviour

No breaking change should be reported (though I realize this is fundamentally not possible for this crate as it is written).
As such, the "no false positives" claim should be removed or at least the caveats thoroughly mentioned.

Additional Context

No response

Debug Output

No response

With just cargo semver-checks in package directory, it should take the current version and select the previous one. For pre-release versions, it should select the previous non-pre-release as breaking changes between prereleases is fine.

At the moment, rustdoc is coupled to a specific range of nightly versions. We should clearly document this for users (and maybe provide helpful runtime errors about it) so users who let their nightly versions go stale have a positive path forward.

Needs Trustfall support for tagging a value inside a @fold and then using it outside the fold.

The query for "the import specifically is missing, but the item is present" would look something like:

{
    CrateDiff {
        baseline {
            item {
                ... on Enum {
                    visibility_limit @filter(op: "=", value: ["$public"]) @output
                    name @output @tag

                    # this is the part that isn't supported at the moment:
                    # we currently can't use this tag outside of its own @fold scope
                    importable_path @fold {
                        path @tag
                    }

                    importable_path {
                        missing_path: path @tag
                    }

                    span_: span @optional {
                        filename @output
                        begin_line @output
                    }
                }
            }
        }
        current {
            item {
                ... on Enum {
                    visibility_limit @filter(op: "=", value: ["$public"])
                    name @filter(op: "=", value: ["%name"])

                    # at least one of the importable paths matches, so we know this should be the same enum
                    importable_path @fold @transform(op: "count") @filter(op: ">", value: ["$zero"]) {
                        path @filter(op: "one_of", value: ["%path"])  # tagged value is list-typed because of fold
                    }

                    # but some importable path is also missing from the current paths, which is the error
                    importable_path @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
                        path @filter(op: "=", value: ["%missing_path"])
                    }
                }
            }
        }
    }
}

The query for "the item itself is gone, it's not just imports" would look like this:

{
    CrateDiff {
        baseline {
            item {
                ... on Enum {
                    visibility_limit @filter(op: "=", value: ["$public"]) @output
                    name @output @tag

                    importable_path @fold {
                        path @tag
                    }

                    span_: span @optional {
                        filename @output
                        begin_line @output
                    }
                }
            }
        }
        current @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
            item {
                ... on Enum {
                    visibility_limit @filter(op: "=", value: ["$public"])
                    name @filter(op: "=", value: ["%name"])

                    # at least one of the importable paths matches, so we know this should be the same enum
                    importable_path @fold @transform(op: "count") @filter(op: ">", value: ["$zero"]) {
                        path @filter(op: "one_of", value: ["%path"])  # tagged value is list-typed because of fold
                    }
                }
            }
        }
    }
}

https://rust-lang.github.io/unsafe-code-guidelines/layout/enums.html#explicit-repr-annotation-with-c-compatibility

Currently, I suspect that #[repr(C, u8)] won't get detected as either #[repr(C)] or as #[repr(u8)], which I believe can lead to both false-positives and false-negatives. Due to the false-positives, this is a bug.

H/t https://twitter.com/bitshiftmask/status/1562185690208735233 + a private Twitter account I'm not going to name, you know who you are :) Thank you both!

This blog post describes how to make unnameable types i.e. pub types that can be returned from the public API but can't be imported and used directly because they reside in a private module:
https://seanmonstar.com/post/693574545047683072/pattern-matching-and-backwards-compatibility

The post's has this example:

#[non_exhaustive]
pub enum Error {
    Connect(Connect),
    // other top-level errors
}

#[non_exhaustive]
pub enum Connect {
    Resolve(unnameable::Resolve),
    Handshake(unnameable::Handshake),
}

mod unnameable {
    pub struct Resolve(());
    pub struct Handshake(());
}

It seems that some of the semver rules might not apply in the usual ways to these types. For example, maintainers might frequently decide that renaming an unnameable type shouldn't be considered breaking.

We should define cargo-semver-checks' behavior with respect to unnameable types and back it up with test cases.

Requires at least

• #53
• #52

But would also benefit from #51, #50

Providing a library alongside the cli tool would be much appreciated for tools like https://GitHub.com/paritytech/cargo-unleash even before it becomes part of cargo

Related: #61

CC @Xanewok @gnunicorn

semverver is another tool for checking semver violations. Its approach at a high level is to compare rlibs rather than rustdocs.

semverver was originally published as part of GSoC 2017, but has recently been adopted into the rust-lang org to hopefully keep it functional.

Obviously there will be differences in what changes both tools do and don't catch, and keeping a scorecard of that probably isn't really worth the effort. It would be nice, however, to call out semverver as an alternative and list some high-level differences resulting from the different approaches. As an initial intuition, that would include at least

• semver-checks by design only checks rustdoc-exposed information, so ignores any #[doc(hidden)] items, which semverver has access to to check.
• semver-checks works off of (mostly?) stable metadata from rustdoc; semverver is by design tied to only work on a specific nightly.

Personally, I think diffing the documented surface is the "more correct" approach here, as semver is about public exposed API. Relying on details not contained in the documentation is relying on details outside of the semver guarantee.

Describe your use case

Prebuilt binaries can cut ~2-3min from the execution time of the cargo-semver-checks GitHub Action. Other Rust binary crates tend to make and upload prebuilt binaries as part of the release process, and make them part of the GitHub release. We should probably follow suit.

Describe the solution you'd like

Pre-build binaries for the most common targets and upload them to each GitHub release.

Action that might be useful: https://github.com/taiki-e/upload-rust-binary-action#example-workflow-cross-compilation

Limit the GitHub token permissions like the following: https://github.com/taiki-e/cargo-hack/blob/202e6e59d491c9202ce148c9ef423853267226db/.github/workflows/release.yml#L3-L5

Available target runners on GitHub Actions: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#choosing-github-hosted-runners

Examples from other crates:

• https://github.com/taiki-e/cargo-hack/blob/202e6e59d491c9202ce148c9ef423853267226db/.github/workflows/release.yml#L47-L84
• https://github.com/LukeMathWalker/cargo-chef/blob/main/.github/workflows/publish_binaries.yml#L18-L43
• https://github.com/LukeMathWalker/bunyan/blob/main/.github/workflows/publish_binaries.yml#L18-L35

Alternatives, if applicable

No response

Additional Context

No response

Ran this on clap and finally tried to do some work in the repo again just to find that git status is reporting all sorts of crazy stuff.

With the implementation, I was hoping to do something light weight like a shallow clone but apparently I have no idea how that git2 call works

Consider the following code, in an imaginary lib crate.

pub mod foo {
  pub struct Bar;
}

pub use foo::Bar;

Bar is now importable both directly as lib::Bar and as lib::foo::Bar. If one of these import paths becomes unavailable in a future release, that's a breaking change.

I believe (but haven't verified) that this should get caught by the current checks, but the error message will be misleading: it will say that the item was removed or renamed, and will not provide the import path that stopped being available.

There is currently a test case that non-breaking moves do not cause false-positive errors. We'll also need a test that breaking moves and import changes cause true-positive errors with a good error message.

Compare the currently selected package against one from the registry at --base-version

Like the compiler, help for the lints should be built-in.