Enforce semver rules

[NeoLegends]

Elm does this. Whenever somebody pushes code to their package management system, it checks the public API for incompatibilities and checks whether those are reflected in the associated version number. This would be great for crates.io! See http://elm-lang.org/.

(Moved from rust-lang/cargo#3074)

[bluss]

Different crates will have different standards for what a breaking change is.

[NeoLegends]

Could you elaborate that? I'm not really sure what you mean.

If my crate's public API changes and becomes incompatible, I can't see how there could be a different "opinion" on that?

[bluss]

(Edit: Maybe this is overly pessimistic. A tool to find changes is great. Here's some of the challenges of enforcement).

Some inspiration from the API evolution RFC https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md This is just for Rust, but it forms a role model as well.

Some breaking changes are just minor changes according to that policy:

• modules: New public items (example: Which crate doesn't do this?)
• Traits: Minor change: adding a defaulted item (example 1 from num, example 2 from itertools)

Other common issues:

• Changes that make type inference fail in some cases where it would previously not. Sometimes crate authors have to make a judgment of the severity.
• Bumping the required Rust version (some crates like to do this without a breaking change, others not) discussion, "updating the rust version is not a breaking change"
• Breaking changes in stable Rust that forces the hand of the maintainer into creating one of the other issues listed here. If say Rust 1.27 breaks your crate, do you abandon your crate's major version, or do you fix it? Either choice leaves your users with a problem (if the fix requires a major version bump, users must reconfigure crates. Version disparities may then cascade to reverse dependencies, hurts composability of crates until everyone is on the same version of an integration crate again [like serde, num-traits, etc]).
  • There are some unfortunate but unavoidable breaking changes: type parameter defaults was syntax removed, for example
  • And avoidable breaking changes: for example Adding trait methods, read_exact issue

Example from the messy real world:

• Behavioural changes can't be tested from the API and some are breaking and others are not (changing the output of Debug?).
• Some public traits are public to use and import, but not intended to be implemented
  • ndarray::Dimension's documentation is clear that this trait may change at will. It must be public so that it can be used as a trait bound, not because it's an interface that you'd implement.
  • Comment: Traits in Rust fill different roles, and not every public trait is an extension interface.

[tomaka]

Changes that make type inference fail in some cases where it would previously not. Sometimes crate authors have to make a judgment of the severity.

As an example, I accidentally broke glutin by replacing fn foo(_: String) with fn foo<T: Into<String>>(_: T), because some people were calling foo with foo("hello".into()).

[brson]

@badboy has done work on this: https://github.com/semantic-rs/semantic-rs

Edit: oh, hm it's not clear how deep the analysis in that crate is, or is intended to be, but I swear badboy has been exploring that.

[badboy]

Yes, yes I have. Currently there's no actual code analysis in that crate, it solely relies on commit message conventions. My plan was and still is to have some code analysis to determine certain things. (I have some very very basic code somewhere which can at least detect function signature changes).

Putting all rules from RFC1105 into code will be incredibly hard. We won't ever get to a point where we don't miss a thing. And we definitely don't want to stop users from publishing versions, for which the server thinks they are breaking, but they are actually not.
I'm not sure enforcing semver from the server side is possible. After all semver is too vague for that.

[louy2]

It seems many prefer at least some level of check on server to nothing.

[withoutboats]

Identifying every breaking change is impossible, since it seems easily reducible to the halting problem. But as a lint, this could be useful. For example:

• When a node of the public module/type graph has disappeared.
• When a type signature has changed. Preferably we'd distinguish between "more general" type parameters and hard breaks. Situations like @tomaka's can be resolved with a turbofish, but others are more serious breakages, and crates can set their own policies on what semver means to them.
• When adding an impl could be a breaking change. In theory this will be never, but there are certain exceptions (at least around auto traits).
• A breaking change to a dependency for which you've exported some symbols (libcpocalypse)

In general, some tool which documents the changes to the public API of your library has many uses. Highlighting breakages/potential breakages is even better. I don't know if there are any breakages that crates.io should block upload on; I think its a potentially good idea but also potentially problematic.

[cogman]

The main issues I see come up from specifically annotated/documented public APIs which are deprecated, unstable, or some other thing (beta? nightly? testing only?).

Unstable APIs in particular shouldn't force a major version bump.

I could see the argument for Deprecated methods requiring a major version bump.

[mickhrmweb]

Unstable APIs in particular shouldn't force a major version bump.

What if the unstable annotation was handled differently or ignored?

[eternaleye]

@badboy:

And we definitely don't want to stop users from publishing versions, for which the server thinks they are breaking, but they are actually not.

Could you expand on that? IMO, if a mechanical process can detect a change that is breaking, then it should block publishing without a major bump. If someone removes a public type from their API, that should, in fact, force at-least-one of (major version bump, revert the change) as far as I can tell.

[NeoLegends]

There are some examples in the RFC that are technically breaking, but will compile fine in most cases (adding a variant to an enum with a __NonExhaustive variant). It wouldn't make much sense to prevent publishing a packet in that case.
However, in the cases the RFC states that something is a breaking change in all or most cases, the checker should definetely block.

The point is, the RFC explicitly defines those cases, so all of them can be detected and dealt with automatically.

[badboy]

Could you expand on that? IMO, if a mechanical process can detect a change that is breaking, then it should block publishing without a major bump.

Bugs & false positives? I don't have a concrete example (and if I would we could easily fix that up front).
If this should be implemented it needs to be pretty conservative and catch only the minimum (removal of a public type should be easy to detect, other things not so much.)

[bluss]

What about soundness fixes? odds 0.2.17 was published without a trait bound that was needed for a function to be memory safe. A few minutes later I realized the mistake in the new function and posted the bug fix as 0.2.18, which restricted the function by a trait bound as was intended all along. fix commit

This would need an override.

[tomaka]

I think the best course of action is a plugin like clippy that would compare your current code with the previous version, and output warnings for breaking changes.
Then, it's the programmer's task to evaluate what's wrong.

If that works well, maybe Cargo can integrate it and add a confirmation message if you try to publish a version that has breaking changes.

[NeoLegends]

@cogman

Unstable APIs in particular shouldn't force a major version bump.

Unstable crates should use version numbers below 1.0.0, for which the checker could / should be disabled.

The more I think about this, the better I like the idea of not hard-blocking crates from being published for the reasons posted above, integrating such a feature as linter into cargo makes the most sense to me. There will be cases that cannot be caught by a programmatic analysis (removing releases that contain sensitive data? Yanking?).

[setharnold]

Unstable crates should use version numbers below 1.0.0, for which the checker could / should be disabled.

I'd suggest keeping it on before 1.0.0 as well: good habits learned during infancy of a project can carry over into good habits in adulthood.

Thanks

[jrmuizel]

FWIW, https://github.com/ibabushkin/rust-semverver is an implementation of a checker for this kind of thing.

[gerred]

I'd suggest keeping it on before 1.0.0 as well: good habits learned during infancy of a project can carry over into good habits in adulthood.

I strongly disagree about keeping the check on. There needs to be a period where a maintainer can signal that a project is in it's infancy and inherently unstable, and isn't forced into a contract before the project is ready to adopt it.

I see your point @setharnold and agree about good habit forming. I also view this as unnecessarily constrictive given a <1.0.0 project is still in it's initial development phase considering: https://semver.org/#spec-item-4. A downstream user should be expecting to pin on exact versions before 1.0.0 since there isn't yet a contract, and I'd rather see cargo warning on anything but exact version pinning on crates that are <1.0.0.

[sgrif]

@carols10cents Would you agree that if this were to be considered, it would require a formal RFC? (and hence this issue should be closed)

[carols10cents]

Yes, I think hammering out the details of exactly how a tool like rust-semverver would be integrated into crates.io should have an RFC.