rust-lang-deprecated / failure

Error management
https://rust-lang-nursery.github.io/failure/
Apache License 2.0
1.43k stars 140 forks source link

Addition of name member to Fail trait is a semver-breaking change #312

Open jdm opened 5 years ago

jdm commented 5 years ago

https://github.com/rust-lang-nursery/failure/commit/df68d7224451846155fe97eb68bc030eb2dd9607 added a member to a public trait, but it was published as 0.1.5. This breaks the semver guarantees and can cause code to stop compiling when failure_derive gets updated independently of the failure crate.

BurntSushi commented 5 years ago

Adding a new method to a trait with a default implementation is generally not considered a breaking change. See the API evolution RFC: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-adding-a-defaulted-item

If this is causing an issue with failure_derive, could you give an example?

jdm commented 5 years ago

One of the Servo contributors added a dependency to https://github.com/jrmuizel/raqote/ to Servo, which depends on https://crates.io/crates/font-kit and transitively the failure crate. This apparently introduced failure_derive into Servo's build (it's not present on master), which brought in failure_derive 0.1.5 while Servo was depending on failure 0.1.3. This caused the font-kit build to error out on a derive(Fail) because the name method did not exist in the trait that was being derived.

BurntSushi commented 5 years ago

Interesting. But I don't think that's a semver violation. It's probably a result of an incorrect minimal version specification in failure_derive: https://github.com/rust-lang-nursery/failure/blob/7b7d03cc6ef247eb3835a6c7f91aa81d1d1d0afb/failure_derive/Cargo.toml#L19

Cargo has a way to do a minimal version check which should catch bugs like this on failure's side, but this has proved difficult to enact in practice since maintainers of core crates in the ecosystem don't specify correct minimal versions.

dekellum commented 5 years ago

The _failurederivefailure is only a dev. dependency. The other direction, failure → _failurederive, should be more relevant. For failure 0.1.5 it is as follows. Its been using this same pattern since 0.1.1.

[dependencies.failure_derive]
optional = true
version = "0.1.5"
path = "./failure_derive"

As reported in #234, the failure 0.1.2 release broke actix due to a similar combination of _failurederive 0.1.2 and failure 0.1.1. As these combinations aren't really being tested for new releases here, perhaps it would be more reliable in the long run if the above dependency pattern was narrowed to a single version, e.g. version = "=0.1.5", for subsequent releases?

In the meantime, the workaround suggested in #234 might fix Servo?