cargo-semver-checks icon indicating copy to clipboard operation
cargo-semver-checks copied to clipboard

False negative: major bumping of dependency with exposed API is not detected

Open dodomorandi opened this issue 1 year ago • 3 comments

Steps to reproduce the bug with the above code

  • Create a new crate with cargo new inner --lib and chdir inside.
  • Add a dependency for which we are going to expose a member and later perform a dep bump. This examples uses time:
    cargo add [email protected]
    
  • Edit src/lib.rs so it contains the following:
    pub struct Wrapper {
      pub date: time::Date
    }
    
  • Create a new commit:
    git add . && git commit -m "Initial commit"
    
  • Bump time to 0.3:
    cargo add [email protected]
    
  • Add another commit (useful for further checks):
    git commit -a -m "bump time"
    
  • Run cargo-semver-checks:
    cargo semver-checks check-release --baseline-rev HEAD^
    

Further check to ensure it is a breaking change

  • Checkout to previous commit:

    git checkout HEAD^
    
  • Create a worktree with the latest commit in a different path:

    git worktree add ../inner-new main
    

    Note that if you are using an old version of git, you could need to use master instead of main

  • Change back to the parent folder and run cargo new --lib outer, then chdir inside outer.

  • Add the first version of inner as dependency:

    cargo add inner --path ../inner
    
  • Also add [email protected] as dependency: cargo add [email protected]

  • Edit the src/lib.rs so it contains the following:

    use inner::Wrapper;
    use time::Date;
    
    pub fn get_wrapped(wrapped: Wrapper) -> Date {
        wrapped.date
    }
    
  • Run cargo check, assess that everything is fine.

  • Use the new version of inner:

    cargo add inner --path ../inner-new
    
  • Run cargo check again. Now you should expect a mismatching time::Date type.

Actual Behaviour

No breaking changes are reported

Expected Behaviour

A breaking change should be detected: bumping the major version of a dependency when the crate exposes something from its API is breaking.

Generated System Information

Software version

cargo-semver-checks 0.20.0 (cf03e7f)

Operating system

Linux 6.2.13-arch1-1

Command-line

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

cargo version

> cargo -V
cargo 1.69.0 (6e9a83356 2023-04-12)

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,sse,sse2
  • Host: x86_64-unknown-linux-gnu

Build Configuration

Should not be relevant, but here the ~/.cargo/config.toml:

[registries.crates-io]
protocol = "sparse"

[target.x86_64-unknown-linux-gnu]
rustflags = ["-Clink-arg=-fuse-ld=lld", "-Clink-arg=-Wl,--no-rosegment"]

[target.x86_64-unknown-linux-musl]
rustflags = ["-Clink-arg=-fuse-ld=lld", "-Clink-arg=-Wl,--no-rosegment"]

[target.arm-unknown-linux-musleabihf]
linker = "arm-linux-musleabihf-gcc"

Additional Context

The issue has been discovered while bumping the wot-td dependency of the crate wot-serve.

CC @lu-zero

dodomorandi avatar May 04 '23 06:05 dodomorandi

Thanks for the thorough report!

Semver in Rust is extremely complex, and there are easily hundreds of rules that aren't implemented yet: https://github.com/obi1kenobi/cargo-semver-checks#will-cargo-semver-checks-catch-every-semver-violation

The tracking issue for not-yet-implemented lints is #5. We're treating these as unimplemented features, and saving the C-bug label for issues like false-positives, or situations where an existing lint fails to catch the issue it purportedly is designed for. We don't have a lint for this case, so I'm going to re-tag this as a feature request.

Also, if you might be interested in contributing to the project by writing new lints, I'd be happy to mentor!

obi1kenobi avatar May 04 '23 16:05 obi1kenobi

Thank you @obi1kenobi!

You are right, it totally make sense to track the missing rules in a single issue instead of spreading them all around without any central document. I just did not find anything specifically related to the missed detection, I should have search more carefully :sweat_smile:

I think I can try to implement the rule for this on my spare time (and maybe others, who knows), it could be an interesting experience. Also, I definitely love this project, I would be very happy to give a small hand. I am gonna try to write down something, if I need help I will ask, thank you in advance! :heart:

dodomorandi avatar May 04 '23 17:05 dodomorandi

Unfortunately I don't think this rule is straightforward to implement since currently cargo-semver-checks doesn't see manifest information — it isn't exposed in the Trustfall schema it queries and it isn't particularly easy to add. I wouldn't recommend it as a starting point.

A few easier starting points:

  • https://github.com/obi1kenobi/cargo-semver-checks/issues/302
  • https://github.com/obi1kenobi/cargo-semver-checks/issues/366
  • any of the items in https://github.com/obi1kenobi/cargo-semver-checks/issues/241 though note that they are in order of increasing difficulty

These run the gamut from "how can I help in half an hour" to "I've got a few afternoons / a few weekends to put in." I'm happy to be an issue concierge if you let me know what sort of thing you'd be most excited about.

obi1kenobi avatar May 04 '23 18:05 obi1kenobi