Skip to content

std::borrow::Borrow should have big fat warnings all over it #27109

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

Closed
chris-morgan opened this issue Jul 18, 2015 · 1 comment
Closed

std::borrow::Borrow should have big fat warnings all over it #27109

chris-morgan opened this issue Jul 18, 2015 · 1 comment

Comments

@chris-morgan
Copy link
Member

There are hidden assumptions around Borrow that don’t appear to be included in any of the documentation that I can find. Most notably, HashMap’s use of Borrow requires that K and Q (where K: Borrow<Q>) hash the same; if they don’t, your HashMap will be mysteriously broken when you try indexing it by Q.

I’d suggest making Borrow an unsafe trait, but unsafe doesn’t mean blah blah mumble mumble memory safety mumble 😛

Anyway, we need some serious warnings on at least the Borrow documentation. Something including things like “if you are implementing Borrow and both Self and Borrowed implement Hash, they must produce the same result, for HashMap’s sake.”

@Gankra
Copy link
Contributor

Gankra commented Jul 18, 2015

From the trait:

Borrow is very similar to, but different than, AsRef. See the book (link) for more.

From the book:

We can see how they’re kind of the same: they both deal with owned and borrowed versions of some type. However, they’re a bit different.

Choose Borrow when you want to abstract over different kinds of borrowing, or when you’re building a datastructure that treats owned and borrowed values in equivalent ways, such as hashing and comparison.

However I agree it could be more prominent.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Aug 5, 2015
This should be a bit more prominent.

Fixes rust-lang#27109
steveklabnik added a commit to steveklabnik/rust that referenced this issue Aug 5, 2015
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

No branches or pull requests

3 participants