Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False-positives caused by newer versions of path dependencies of the scanned crate #902

Open
obi1kenobi opened this issue Sep 2, 2024 · 5 comments
Labels
C-bug Category: doesn't meet expectations

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Sep 2, 2024

Steps to reproduce the bug with the above code

When specifying a package by path dependency, cargo is only willing to use that package's own declared path dependencies and ignores newer SemVer-compatible dependency versions. It refuses to update to newer versions even if explicitly asked with cargo update or even cargo update --precise.

This causes false positives. For example:

  • tokio v1.40 made some types newly become UnwindSafe.
  • Auto-traits propagate automatically, making many of tokio-stream's types newly become UnwindSafe across a variety of tokio-stream versions when used with tokio v1.40.
  • SemVer checking an older version of tokio-stream now results in false positives:
    • The baseline is a registry version of tokio-stream, for which cargo resolves the tokio dependency to 1.40. That means the baseline's types are UnwindSafe.
    • The "current" (scan target) version of tokio-stream is a path dependency, for which cargo chooses the path dependency tokio version. That tokio is older than 1.40, so its types aren't UnwindSafe. As a result, the tokio-stream types aren't UnwindSafe either.
    • Since the older tokio-stream had``UnwindSafe` types, and the newer one did not, this is reported as a breaking change!
    • In Explicitly cargo update in path dependency workspaces. #901 I was under the impression that cargo update inside the placeholder project we generate when creating rustdoc JSON for tokio-stream (where we have a path dependency to it) would result in newer tokio. This is not the case, due to the issue described at the top.

Repro:

git clone [email protected]:tokio-rs/tokio.git
cd tokio
git checkout d7b7c6131774ab631be6529fef3680abfeeb4781
cd ..

mkdir repro
cd repro
cat <<EOF >Cargo.toml
[package]
name = "rustdoc"
version = "0.0.0"
edition = "2021"
publish = false

[workspace]
members = []

[dependencies.tokio-stream]
path = "../tokio/tokio-stream"
features = ["default", "fs", "io-util", "net", "signal", "sync", "time", "tokio-util"]

[lib]
path = "lib.rs"
required-features = []
EOF

mkdir src
touch src/lib.rs

# To generate the lockfile and confirm the manifest is valid.
cargo check

# Note tokio v1.25.0 is selected. This is the version that 
# the path dependency on tokio-stream pulls in.
cargo tree --invert --package tokio
# Outputs:
# tokio v1.25.0 (.../tokio/tokio)
# ├── tokio-stream v0.1.12 (.../tokio/tokio-stream)
# │   └── rustdoc v0.0.0 (.../tokio/target/semver-checks/local-tokio_stream-0_1_12)
# └── tokio-util v0.7.7 (.../tokio/tokio-util)
#     └── tokio-stream v0.1.12 (.../tokio/tokio-stream) (*)

# Observe a bunch of crates get updated.
# But tokio is not one of them, even though 1.40 is available!
cargo update

# Even explicitly attempting to update tokio fails!
cargo update --precise 1.40.0 tokio
# Outputs:
#     Updating crates.io index
#     Updating tokio v1.25.0 (/.../tokio/tokio) -> v1.25.0
# note: pass `--verbose` to see 21 unchanged dependencies behind latest

Actual Behaviour

Two items:

(1) There should be a way to run cargo update inside a project with a path dependency to force it to use latest SemVer-compatible versions of dependencies.

(2) cargo update --precise 1.40.0 tokio should either upgrade tokio to 1.40.0 as requested, or should exit with an error. It should not completely ignore the 1.40.0 argument and exit cleanly.

Expected Behaviour

(1) cargo update silently does not update versions of path dependencies of a path dependency. Instead, it pins the path dependencies and their path dependencies exactly. There does not appear to be a way to override this.

The first cargo update run after the setup above may update some non-path-related dependencies. Subsequent cargo update runs will look like this, and will not upgrade tokio to 1.40.0 nor mention it in any way.

$ cargo update
    Updating crates.io index
     Locking 0 packages to latest compatible versions
note: pass `--verbose` to see 21 unchanged dependencies behind latest

$ cargo update --verbose
    Updating crates.io index
     Locking 0 packages to latest compatible versions
   Unchanged mio v0.8.11 (latest: v1.0.2)
   Unchanged socket2 v0.4.10 (latest: v0.5.7)
   Unchanged wasi v0.11.0+wasi-snapshot-preview1 (latest: v0.13.2+wasi-0.2.1)
   Unchanged windows-sys v0.45.0 (latest: v0.59.0)
   Unchanged windows-sys v0.48.0 (latest: v0.59.0)
   Unchanged windows-targets v0.42.2 (latest: v0.52.6)
   Unchanged windows-targets v0.48.5 (latest: v0.52.6)
   Unchanged windows_aarch64_gnullvm v0.42.2 (latest: v0.52.6)
   Unchanged windows_aarch64_gnullvm v0.48.5 (latest: v0.52.6)
   Unchanged windows_aarch64_msvc v0.42.2 (latest: v0.52.6)
   Unchanged windows_aarch64_msvc v0.48.5 (latest: v0.52.6)
   Unchanged windows_i686_gnu v0.42.2 (latest: v0.52.6)
   Unchanged windows_i686_gnu v0.48.5 (latest: v0.52.6)
   Unchanged windows_i686_msvc v0.42.2 (latest: v0.52.6)
   Unchanged windows_i686_msvc v0.48.5 (latest: v0.52.6)
   Unchanged windows_x86_64_gnu v0.42.2 (latest: v0.52.6)
   Unchanged windows_x86_64_gnu v0.48.5 (latest: v0.52.6)
   Unchanged windows_x86_64_gnullvm v0.42.2 (latest: v0.52.6)
   Unchanged windows_x86_64_gnullvm v0.48.5 (latest: v0.52.6)
   Unchanged windows_x86_64_msvc v0.42.2 (latest: v0.52.6)
   Unchanged windows_x86_64_msvc v0.48.5 (latest: v0.52.6)

(2) cargo update --precise 1.40.0 tokio completely ignores the --precise 1.40.0 requirement, and exits cleanly as a no-op.

$ cargo update --precise 1.40.0 tokio
    Updating crates.io index
    Updating tokio v1.25.0 (/.../local_code/tokio/tokio) -> v1.25.0
note: pass `--verbose` to see 21 unchanged dependencies behind latest

Generated System Information

Software version

cargo-semver-checks 0.34.0

Operating system

Linux 5.15.153.1-microsoft-standard-WSL2

Command-line

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

cargo version

> cargo -V 
cargo 1.80.1 (376290515 2024-07-16)

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

No response

Additional Context

No response

@obi1kenobi obi1kenobi added the C-bug Category: doesn't meet expectations label Sep 2, 2024
@obi1kenobi
Copy link
Owner Author

The invocation of cargo-semver-checks that triggers the false-positives is this:

$ cargo semver-checks --manifest-path ../tokio/tokio-stream/Cargo.toml --release-type minor --exclude benches --exclude examples --exclude stress-test --exclude tests-build --exclude tests-integration
    Finished `release` profile [optimized] target(s) in 0.18s
     Running `target/release/cargo-semver-checks semver-checks --manifest-path ../tokio/tokio-stream/Cargo.toml --release-type minor --exclude benches --exclude examples --exclude stress-test --exclude tests-build --exclude tests-integration`
     Parsing tokio-stream v0.1.12 (current)
      Parsed [   0.869s] (current)
     Parsing tokio-stream v0.1.12 (baseline, cached)
      Parsed [   0.039s] (baseline)
    Checking tokio-stream v0.1.12 -> v0.1.12 (assume minor change)
     Checked [   0.014s] 80 checks: 79 pass, 1 fail, 0 warn, 7 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type ReceiverStream is no longer UnwindSafe, in /.../tokio/tokio-stream/src/wrappers/mpsc_bounded.rs:11
  type ReceiverStream is no longer RefUnwindSafe, in /.../tokio/tokio-stream/src/wrappers/mpsc_bounded.rs:11
  type UnboundedReceiverStream is no longer UnwindSafe, in /.../tokio/tokio-stream/src/wrappers/mpsc_unbounded.rs:11
  type UnboundedReceiverStream is no longer RefUnwindSafe, in /.../tokio/tokio-stream/src/wrappers/mpsc_unbounded.rs:11

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   0.966s] tokio-stream

@obi1kenobi
Copy link
Owner Author

cc @Darksonn on the off chance you notice this while working on tokio.

Scanning on new tokio versions should be fine, since the path dependency on tokio will point past 1.40.0. But re-scanning older commits can trigger this.

If you have opinions the cargo behavior, or ideas on how to get it to update transitive path dependencies to latest compatible versions, I'd love to hear them!

@Darksonn
Copy link

Darksonn commented Sep 2, 2024

Interesting. Our CI job doesn't get any lock file as input, so I don't think we will hit this.

It sounds like the proper solution would be for tokio-util to use the local Tokio in both builds being compared?

@obi1kenobi
Copy link
Owner Author

I agree, I don't think you'll hit it either. Just wanted to preemptively warn you just in case, so you don't end up debugging a phantom breaking change.

Yes, we'll have to figure out a way to use the same version of tokio (and all other dependencies) on both sides of the comparison. Otherwise it's apples to oranges. I'm hoping we can somehow get cargo to use the version number from the transitive dependency, instead of the path component of the transitive path dependency. But I haven't figured out a way to do that yet.

@Darksonn
Copy link

Darksonn commented Sep 2, 2024

Thanks for the heads up. I guess it could potentially affect our branches used for making backports to LTS releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations
Projects
None yet
Development

No branches or pull requests

2 participants