Function argument type change does not require version change

[Gnurfos]

Steps to reproduce the bug with the above code

1. cargo init --lib
2. commit everything
3. Change files to get this:

> git diff
diff --git a/src/lib.rs b/src/lib.rs
index 7d12d9a..ee48c94 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,5 +1,5 @@
-pub fn add(left: usize, right: usize) -> usize {
-    left + right
+pub fn add(left: usize, right: u8) -> usize {
+    left + right as usize
 }

Actual Behaviour

cargo semver-checks --baseline-rev HEAD doesn't report any version change required:

> cargo semver-checks --baseline-rev HEAD
     Cloning HEAD
     Parsing test_func v0.1.0 (current)
     Parsing test_func v0.1.0 (baseline)
    Checking test_func v0.1.0 -> v0.1.0 (no change)
   Completed [   0.012s] 51 checks; 51 passed, 0 unnecessary

Expected Behaviour

I expect a major version bump to be required in such case, but that's my intuition, based on the impact for users of the library. The cargo book does not mention this kind of change in its list, so maybe I'm missing something.

Generated System Information

Software version

cargo-semver-checks 0.25.0

Operating system

Linux 5.15.0-91-generic

Command-line

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

cargo version

> cargo -V 
cargo 1.75.0 (1d8b05cdd 2023-11-20)

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]

Thanks for opening this! Currently, no lints check type information since they can't see it (#149) so all the type-related lints are on the not-yet-implemented list: #5.

While simpler cases like the one in your repro exist, this is a very hard problem in general since it has to take into account generic types, lifetimes, lifetime variance, whether traits are sealed, etc. In a sense, it's the "final boss" of semver checking. For example:

pub fn example(value: i64) { ... }

changing to

pub fn example(value: impl Into<i64>) { ... }

is not a breaking change. But if inside a trait, it is a breaking change. Except if the trait is sealed, in which case it isn't a breaking change. Etc. etc.

We'll need quite a bit more time and more funding before we can tackle this.

[Gnurfos]

Oh, I understand the difficulty now.
But wouldn't false positives (declaring a breaking change whenever any type seems to change) be preferable to false negatives ?

It sure is annoying if the versions change more than they really have to, but I can live with it.
On the other hand, in its current state, the whole system is not usable for someone like me.

[obi1kenobi]

It's a complicated tradeoff either way. I can share some logical and some empirical arguments on why cargo-semver-checks explicitly prefers false-negatives instead of false-positives whenever possible.

False negatives are always going to happen unless we solve the Halting problem. We will never be able to peer into a program deeply enough to be guaranteed to find all breaking changes in implementations. So cargo-semver-checks will never replace e.g. your test suite — not even relatively "blunt" tools in it like snapshot testing, let alone more focused testing techniques. That means cargo-semver-checks is always going to be the second line of defense against breaking changes — just trying to catch stuff you might have missed — instead of the first and only line of defense. At that point, we're choosing whether to have false-negatives only, or whether to have false-positives in addition, so we choose to not have false-positives.

Empirically, tools that have false-positives cause frustration among their users, and have a hard time getting adopted (and surviving!) as a result. For example, see how vigorous the debate is on adopting static typing in Python, where the type-checkers often have false-positives on dynamic code (not a skill issue — it's genuinely hard to avoid false-positives there).

cargo-semver-checks is not the first attempt at linting semver in Rust. One prior attempt was called cargo-breaking and was based on diffing ASTs, which caused more false-positives but also caught function signature changes. Many smart people put a lot of hard work into it, but it's my impression that it never got broad adoption among large Rust projects, and it was ultimately abandoned in favor of cargo-semver-checks.

If you have a big project (and cargo-semver-checks is used by the biggest crate on crates.io), you're likely to have hundreds if not thousands of false-positives each time. Triaging those would be tedious, unsatisfying, and ultimately unproductive. The status quo ante is "we'll watch out for breaking changes manually" which works okay even though it demonstrably isn't perfect. It costs a team far less to deal with sporadic-but-painful breaking changes when they slip through, than to triage hundreds of false-positives on every release. Note that some of the teams that use cargo-semver-checks ship a dozen package releases per day.

Of all linters, false-positives are a particularly hard sell for semver linters, for the above reasons.

[obi1kenobi]

It sure is annoying if the versions change more than they really have to, but I can live with it.

To address this directly: if you want to always be safe from semver violations, you can always bump your project's major version. This has its own tradeoffs, though, right? So while I'm not familiar with the details of your project, presumably "I can live with it" is not infinitely elastic?

[Gnurfos]

To address this directly: if you want to always be safe from semver violations, you can always bump your project's major version. This has its own tradeoffs, though, right? So while I'm not familiar with the details of your project, presumably "I can live with it" is not infinitely elastic?

Our use case is:

We have a network of crates, depending on each other, each in its own repository. Developers merge in changes and do not concern themselves with what version upgrade this requires (as you brilliantly argued would be unreliable anyway).

The only issues unit tests can catch are within a crate, so we cannot tell if a version bump was done at the right level, before other crates start using the new version. This comes potentially much later, and in my experience (especially in Rust), having a dependency "lying" about semver compatibility is one of the most annoying thing that can happen (things break without you changing anything, not even versions, and are very difficult to investigate). So I'd like to weed out as many false negatives as possible, and this one seems like the biggest most common one.

We're contemplating always bumping the major version as you suggest, but that would mean a lot of excessive new versions (of transitively dependent crates), while a good amount of our changes can be definitely identified as non-breaking.

A lint rule saying "It looks like this field/argument's type has changed. It might be non-breaking but I'm not sure (yet) => major" would be perfect for us, I think.

Assuming this is not enough to convince you to change you mind on this:

• do you think such a lint would be easy to write ?
• is there a way for users to use a custom sets of lints at runtime, short of forking cargo-semver-checks ?

[obi1kenobi]

It sounds like you have a very interesting use case! I'd love to hear more about it.

Are these crates public on crates.io / somewhere else I can take a look, or private? How do the developers currently communicate to each other what has changed?

I'll take a look at whether we can put something in cargo-semver-checks together to hit this use case.

[Gnurfos]

Unfortunately, the crates are not public.
I think it could be called "your typical corporate code base": some crates are very low level (like date, string, or localization utilities), some are high level "services", and in between is a lot of business domain logic, and a few "frameworks".

We're migrating a big application to Rust. The Rust code is still recent (hence why we're still figuring out things like versioning) but a lot is functional already. We are a small team, so communication at this point is not a big issue. One way I envision future scaling, is maybe generating automatic aggregated change logs. But unless someone is waiting for a bugfix in your crate (in which case they'll get notified), there is no need to advertise everywhere when you push a change to a crate.

By the way, since we're using a private crates registry too, I see that you recommended working around #160 by using --baseline-rev, but can this be used when we have several packages in one repository (we do that, too)? I imagine some cases where 2 packages might need a different baseline revision.

[obi1kenobi]

Generating automatic aggregated change logs is something that folks have requested in the past as well. It's a related feature to both the custom lints system you described, and is also related to the logic for applying conservative semver bumps if a possibly-breaking change might have happened.

All of these are too far out of scope for cargo-semver-checks. To my knowledge, no free tool exists that can do what you need.

But I'm working on new commercially-available tools (either as SaaS or as a self-contained binary you can run yourself) that can check all those boxes. The expertise I've earned from cargo-semver-checks gives me a big edge in building such tools. That means it'd be cheaper to buy a license while I build and maintain the tools, than to build and maintain them yourself. The revenue I'd generate would also help fund future cargo-semver-checks development, which would also help the Rust community overall.

If you have some budget allocated to solving the problems you described, I'd love to chat more!

[Gnurfos]

I made it sound bigger than it is, but we're really a small team and have to deal with business needs at the same time as this Rust migration, so we don't have much budget for external things.

I am at the moment looking at how hard it would be to add private registry support to cargo-semver-checks. If this ends well, pragmatically I'd look next at running a forked cargo-semver-checks with the added rule I mentioned earlier. I am afraid that's the extent to which we can contribute for now.

For change logs, we're starting from basically nothing (technical), so we will probably just hack together something based on git logs, and that will serve us OK for a while.

[obi1kenobi]

Okay, best of luck! I'd appreciate any PRs you make.

A heads-up that the current schema for writing rules is completely ignorant of type information. It's as if types of fields, parameters, etc. simply doesn't exist. You can see this in the playground.

So in order to add that rule, you'll first need to figure out a way to represent type information (#149). That's not a trivial problem to solve, and I'd be very interested to hear how you ended up solving it! You'll also need to fork the trustfall-rustdoc-adapter crate to do it, since that's where the schema is defined and implemented.

Personally, I'd be shocked if it ends up cheaper in the end to do all this yourself instead of to have me do it for you. You use a custom registry, presumably you used an off-the-shelf one instead of building it in-house, right? To my knowledge, I don't know of a custom registry that isn't a paid product. I'm saying this to point out there might be room to pay for a product specifically so that you can focus on those business needs, instead of building Rust tooling. If you do end up starting that conversation internally, I'd love to be part of it! If not, no hard feelings.

Good luck and let me know how it all goes!

[Gnurfos]

Thanks! Our whole Rust migration effort is quite big and doesn't directly add business value. Within this, tooling is only a small part, and we so far go for "good enough", since this already better than the legacy solutions.
We sometimes roll our own tools, sometimes use existing code (like docs.rs).
Once the migration is over, we will maybe have more time and budget to look back and try to improve tooling.

[jplatte]

Hi! Came here from the latest blog post. I noticed this example:

pub fn example(value: i64) { ... }

changing to

pub fn example(value: impl Into<i64>) { ... }

is not a breaking change.

Which to me is definitely a breaking change, though it's sometimes categorized as "minor". Making a previously not generic parameter generic breaks code that relied on inference knowing the parameter, like foo::example(0i32.into()) in this example. I guess you probably already know that, but I think it is a piece of information that is important to be restated on this issue.

[obi1kenobi]

Indeed. I'm hoping that at some point, we can have a new lint category for changes that are breaking but not semver-major such as that one.

The API evolution RFC and the cargo semver reference are the two authoritative docs we have on how semver in Rust is expected to work, so that kind of lint doesn't fit in the existing semver-major / minor categories. But I can definitely see that it would be good to e.g. flag during code review so that maintainers can make an explicit decision based on all the facts, instead of just slipping through. Another lint like this in my mind is "you are adding a new exhaustive pub struct / enum, consider making it #[non_exhaustive]," since that's something that's easy to add on day 1 (and even remove later) but a major breaking change to do on day 2.

Also, thank you for your sponsorship @jplatte, I appreciate it 🙇‍♂️

[jplatte]

Another lint like this in my mind is "you are adding a new exhaustive pub struct / enum, consider making it #[non_exhaustive]," since that's something that's easy to add on day 1 (and even remove later) but a major breaking change to do on day 2.

Yeah, this would be really nice too, though IME the existing opt-in clippy lints for exhaustive enums and structs already help a lot.