Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unintentional breaking API change in 0.1.18 from 0.1.17 #50

Closed
ssloboda opened this issue Sep 27, 2019 · 4 comments · Fixed by #51
Closed

Unintentional breaking API change in 0.1.18 from 0.1.17 #50

ssloboda opened this issue Sep 27, 2019 · 4 comments · Fixed by #51

Comments

@ssloboda
Copy link
Contributor

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
@jeromefroe
Copy link
Owner

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.

@ssloboda
Copy link
Contributor Author

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!

@jeromefroe
Copy link
Owner

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.

@ssloboda
Copy link
Contributor Author

No problem. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants