False negative: moving traits behind a non-default feature flag

[adamchalmers]

Steps to reproduce the bug with the above code

1. Clone the repo https://github.com/KittyCAD/modeling-api and check out the branch achalmers/remove-one-enum
2. Notice that the PR adds a new Cargo feature "diesel", which is not a default feature. It also moves a lot of Diesel trait implementations behind the --features diesel flag, which should be a breaking change.

Actual Behaviour

Cargo semver-checks reports no semver violations

Expected Behaviour

Cargo semver-checks should report a semver violation

Generated System Information

Software version

cargo-semver-checks 0.25.0

Operating system

macOS 13.4.1 (Darwin 22.5.0)

Command-line

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

cargo version

> cargo -V 
cargo 1.74.1 (ecb9851af 2023-10-18)

Compile time information

• Profile: release
• Target triple: aarch64-apple-darwin
• Family: unix
• OS: macos
• Architecture: aarch64
• Pointer width: 64
• Endian: little
• CPU features: aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,fp16,frintts,jsconv,lor,lse,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,vh
• Host: aarch64-apple-darwin

Build Configuration

No response

Additional Context

No response

[obi1kenobi]

Thanks for the report! This feels more like a shortcoming in our docs / FAQ / GitHub Action than anything else — I'd love your help in resolving it!

When checking, cargo-semver-checks has to choose a combination of features to build rustdoc with. Rustdoc doesn't currently support any way to build JSON that describes which pieces of code are visible under what combination of features, so we can't do a "single pass" check for all feature combinations.

We also cannot afford to check every combination of features by default, since that's 2^n combinations which would rapidly become impractical.

By default, cargo-semver-checks uses a heuristic to enable all features that "don't look private" in order to try to test as much code as possible: https://github.com/obi1kenobi/cargo-semver-checks#what-features-does-cargo-semver-checks-enable-in-the-tested-crates

But as you point out, this is also not a perfect choice since it will miss breakage that happens when only a subset of features is enabled. This is why it allows choosing other feature combinations: cargo semver-checks --default-features should catch the issue you point out.

My question to you is: how can we make this situation better for the next person?

Would you change something in the README / FAQ to convey this better? Perhaps suggest that users whose packages have non-default features run cargo-semver-checks both with only default features and with our heuristic for "all public features"?

[adamchalmers]

Thanks for explaining! I think ideally I'd like semver-checks to build and analyze Rustdoc twice: once with the current behaviour (all features that don't look private) and once with just the default features. I think a lot of Rust programmers use crates with just the default features.

That way, you'd catch the common problem of "this struct used to be visible by default, but now it's only visible if I enable a new Cargo feature for the crate". In my experience this is a common problem for semver in the Rust ecosystem :)

[obi1kenobi]

Yeah, totally fair.

The reasons we haven't implemented this yet are questions of ergonomics and UX:

• Because most crates don't have any features (default or otherwise), we don't want to run it twice for everyone. Checking for non-default features can be implemented, but it's a bit tricky around edge cases — e.g. what should happen if the "current" and "baseline" versions don't have the same answer for "does the crate have non-default features" ?
• The "default features" and the current behavior scans are completely independent, and could run in parallel in separate jobs. The rustdoc build is the "long pole" in our performance profile, and sequentially building it twice would probably be a non-starter for some of the bigger crates and workspaces that use cargo-semver-checks. So perhaps the right answer is to implement this in our GitHub Action? That of course makes the feature detection I described above even more complex...

What do you think?

Btw, I'd love to have KittyCAD as a GitHub sponsor of cargo-semver-checks — and I'd be happy to make solving this problem a top priority if it would push you over the activation energy barrier for that :)

[adamchalmers]

Checking for non-default features can be implemented, but it's a bit tricky around edge cases

Agreed. A good first step might be to only run the check once if the [features] table in Cargo.toml is totally absent -- this would hit most of the crates without features, which is most crates as you say :)

We like sponsoring the open-source tech we rely on! I'm still only experimenting with semver-checks, right now I just run it for curiosity because I'm not actually using semver yet (just publishing some things on crates.io for ease of deployment). But if in the future it becomes part of our workflow, we'll sponsor you :)