jeromefroe/lru-rs

Unintentional breaking API change in 0.1.18 from 0.1.17

Closed this issue · 4 comments

After running cargo update for a crate that I'm working on, I encountered a build failure due to the changes included in 0.1.18. This issue does not exist in 0.1.17. Patch updates should not be breaking API changes according to semver.

error[E0277]: the trait bound `lru::KeyRef<uuid::Uuid>: std::borrow::Borrow<&uuid::Uuid>` is not satisfied

1220 |         match cache.get_mut(&id) {
     |                     ^^^^^^^ the trait `std::borrow::Borrow<&uuid::Uuid>` is not implemented for `lru::KeyRef<uuid::Uuid>`
     |
     = help: the following implementations were found:
               <lru::KeyRef<K> as std::borrow::Borrow<K>>

error: aborting due to previous error

Ah! Sorry about that. Would it be possible for you to provide the type of the cache (or how it's constructed would probably be even more helpful)? At first glance it seems that the compiler is inferring that id is a reference to a UUID (&uuid::Uuid) instead of a UUID itself (uuid::Uuid) so that &id would become a reference to a reference and I'm not sure it's able to dereference the double reference properly. But I'm not really sure right now to be honest.

Yes, the double reference is the problem here (seems like the & was a vestige of some refactoring that made the & unnecessary but was never removed).

I was more just looking to complain/notify you that the patch bump introduced a change that might affect other consumers of this crate, even if their Cargo.tomls are set up to only pull in patch updates automatically (which, correct me if I'm wrong, should be sufficient to prevent breakage of code consuming other crates assuming that they follow semver). However, it seems unlikely that many others had the same extra & in their code so this probably won't be much of an issue - though I'd still argue that 0.1.17 to 0.1.18 shouldn't have been a patch bump.

Not sure how useful this actually is but the cache type is LruCache<uuid::Uuid, SomeStructWithProprietaryInfo> - I don't think the value type really matters here.

Appreciate your continued work on and maintenance of this crate!

Ahh, makes sense! Thanks for calling out the issue and sorry for the unintended surprise :) I'll release an 0.2.0 version to err on the side of caution.

No problem. Thank you!