RustCrypto / traits

Collection of cryptography-related traits
566 stars 183 forks source link

Accidental breakage from digest 0.10.1 to 0.10.2 #979

Closed aumetra closed 2 years ago

aumetra commented 2 years ago

digest 0.10.2 updated their dependency on crypto-common to 0.1.2 which introduced the function output_size on the trait OutputSizeUser.

Since now both the Digest trait and the OutputSizeUser trait have a function called output_size, calling output_size on a type parameter that is restrained to implementing the Digest trait is now ambiguous and breaks code that does so

Min repr: https://github.com/aumetra/digest-type-ambiguity-repr Min repr with the versions pinned to 0.10.1 and 0.1.1 (aka working): https://github.com/aumetra/digest-type-ambiguity-repr/tree/working

(I forgot to create this issue earlier. Sorry about that)

newpavlov commented 2 years ago

Hm, I wonder what is the best way to fix this issue... We can either remove output_size from Digest or from OutputSizeUser. Digest had it since 0.10.0, but having it on OutputSizeUser make things more consistent with other *User traits. For now, I lean towards the latter option.

de-vri-es commented 2 years ago

Another breakage was the addition of the KeyInit bound on Mac::new_from_slice. That broke jws: https://github.com/de-vri-es/jws/issues/3

I think that the update of the crypto-common dependency should have been a major version bump :(

At the very least either the old 0.10.0 and 0.10.1 should be yanked, or the new 0.10.2 and 0.10.3. Because right now if someone specifies 0.10.0 they could get anything from 0.10.0 to 0.10.3 and they're not compatible :(

tarcieri commented 2 years ago

It sounds like v0.10.0/v0.10.1 should be yanked to me

tarcieri commented 2 years ago

I think that the update of the crypto-common dependency should have been a major version bump :(

This is a tricky one, and it's not really covered by the API evolution guidelines for traits:

https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#traits

My understanding is creating ambiguities where none existed before, though it can break code, is generally not considered a major breaking change. Problems like this have impacted core/std a number of times, e.g. adding methods to Iterator which overlap with extension methods in itertools for example.

However, I agree it would probably be best to yank the older releases to prevent differing behaviors.

de-vri-es commented 2 years ago

In this case it's not only ambiguities though: some generic functions have new trait bounds that were not there before. That falls under tightening bounds I believe: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#major-change-tightening-bounds

/edit: Although it's not in a type definition, I would be surprised if adding new bounds on trait members would not be a major change.

tarcieri commented 2 years ago

I yanked v0.10.0 and v0.10.1 in #1012

de-vri-es commented 2 years ago

Thanks!