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

AsRef/Borrow/BorrowMut need better documentation #44868

Closed
ghost opened this issue Sep 26, 2017 · 13 comments · Fixed by #59663
Closed

AsRef/Borrow/BorrowMut need better documentation #44868

ghost opened this issue Sep 26, 2017 · 13 comments · Fixed by #59663
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. P-medium Medium priority

Comments

@ghost
Copy link

ghost commented Sep 26, 2017

Links: AsRef, Borrow, BorrowMut.

Those docs have plenty of text. I've read it dozens of times and felt "uhhh, what" every single time. Well, the text sort of makes sense, but, in the end, it doesn't help me in deciding between AsRef and Borrow. So I just had to dig myself through the APIs and try figuring the mystery by myself. :)

Here are some notes I've collected afterwards. I wonder if we could get at least some of the following stuff into the official docs...

Conversion Using Borrow Using AsRef
&T -> &T borrows (*)
&Vec<T> -> &[T] borrows borrows
&String -> &str borrows borrows
&str -> &Path converts
&Path -> &OsStr converts
&OsStr -> &Path converts
&Path -> &Path borrows borrows

(*) should be 'borrows', but cannot be implemented yet due to coherence issues (I believe?)

Key takeaways:

  • Borrow is simple and strict. The hash of the borrowed reference must stay the same.
  • AsRef converts to a wider range of different types. The hash of the new reference is allowed to change.

Exercise 1

We want to implement a function that creates a directory. It must accept both &str and &Path as the argument. Which signature are we looking for?

fn mkdir<P: AsRef<Path>>(path: P);
fn mkdir<P: Borrow<Path>>(path: P);

Answer: In order to go from &str to &Path, we have to create a value of different type Path (&str is more primitive - it cannot be borrowed as Path). Since AsRef can borrow and convert, it is the correct option here.

Exercise 2

We want to check whether a value exists in a HashSet. Even if we have a set of type HashSet<String>, we'd like to be able to just do s.contains("foo") (passing a &str). Which one of the four method signatures is the right one?

impl<T: Eq + Hash> HashSet<T> {
    fn contains<Q: Hash + Eq>(&self, value: &Q) -> bool where T: AsRef<Q>;
    fn contains<Q: Hash + Eq>(&self, value: &Q) -> bool where Q: AsRef<T>;
    fn contains<Q: Hash + Eq>(&self, value: &Q) -> bool where T: Borrow<Q>;
    fn contains<Q: Hash + Eq>(&self, value: &Q) -> bool where Q: Borrow<T>;
}

Answer: We don't want to convert between totally different types, so the hash and structural equality of the value must be preserved after reference conversion. In other words, we only want simple borrowing. Conversion to a different type might potentially change the hash and is thus out of the question.

So Borrow is the right one here. But is it T: Borrow<Q> or Q: Borrow<T>? Well, if T is String and we're passing a &str to the method, we want to borrow T as Q. So the right bound is T: Borrow<Q>.

@frewsxcv
Copy link
Member

Possibly redundant with #36435 or #29701 or #24140

@TimNN TimNN added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Sep 27, 2017
@clarfonthey
Copy link
Contributor

clarfonthey commented Sep 28, 2017

I'm willing to work on this soon, updating both the book and the rustdoc. I think that the main thing that isn't conveyed right now is the fact that Borrow requires that the data be functionally the same (providing the same trait impls) whereas AsRef reinterprets the data differently (e.g. str -> Path).

@partim
Copy link
Contributor

partim commented Oct 4, 2017

I’ve been planning to write some text for the documentation of AsRef and Borrow. In particular, I think what is currently lacking is a one-sentence summary of the purpose of the two. This seems important to me because if one reads a type implementing a trait as ‘being something,’ Borrow is backwards: T: Borrow<U> can intuitively be read as T is a borrow of U when in fact it is the other way around. This had me seriously confused when looking at the signature of HashMap::get().

So, for Borrow my proposal would be something along the line of: ‘If a type implements Borrow<Borrowed>, it signals that the type Borrowed acts as a kind of pointer to the type.’ (Pointer may not be the right word here, but I feel that using ‘borrow’ to explain Borrow is a bit weird.)

Conversely, perhaps: ‘By implementing AsRef<T>, a type signals that it can provide a view of itself as T.’ (That one probably needs some work.)

@steveklabnik steveklabnik added the P-medium Medium priority label Oct 31, 2017
@partim

This comment has been minimized.

@clarfonthey

This comment has been minimized.

@prasannavl
Copy link

prasannavl commented Nov 19, 2017

This is the best documentation on these conversions so far, I've found. As someone else is working on the docs, at the very least - I'd link to this discussion, on the book.

kennytm added a commit to kennytm/rust that referenced this issue Mar 19, 2018
Improve documentation for Borrow

This is the first step in improving the documentation for all the reference conversion traits. It proposes new text for the trait documentation of `Borrow`. Since I feel it is a somewhat radical rewrite and includes a stricter contract for `Borrow` then the previous text—namely that *all* shared traits need to behave the same, not just a select few—, I wanted to get some feedback before continuing.

Apart from the ‘normative’ description, the new text also includes a fairly extensive explanation of how the trait is used in the examples section. I included it because every time I look at how `HashMap` uses the trait, I need to think for a while as the use is a bit twisted. So, I thought having this thinking written down as part of the trait itself might be useful. One could argue that this should go into The Book, and, while I really like having everything important in the docs, I can see the text moved there, too.

So, before I move on: is this new text any good? Do we feel it is correct, useful, comprehensive, and understandable?

(This PR is in response to rust-lang#44868 and rust-lang#24140.)
@jonhoo
Copy link
Contributor

jonhoo commented May 28, 2018

I continuously get confused between T: Borrow<Q> and Q: Borrow<T>. I think the mental rule to remember is that Borrow is really BorrowAs. Then it all becomes a lot clearer. Of course, we can't actually rename the trait at this point.

A related discussion to this (I think) is what a user should do if they want to take any type that can be borrowed into an &T including T. In that case you must use Borrow, since there is no impl AsRef<T> for T.

@steveklabnik steveklabnik added the E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. label May 29, 2018
@partim
Copy link
Contributor

partim commented May 29, 2018

@jonhoo, we’ve tried to build this sort of mental model for Borrow in its new documentation. If you haven’t yet, could you perhaps check if it feels more clear?

As for the missing blanket impl AsRef<T> for <T>, I haven’t found that to be all that much of an issue in practice, since in most cases I use AsRef<T> for some concrete TAsRef<[u8]> and AsRef<Path> come to mind, and those types so far have always had that impl anyway.

You can’t use Borrow<T> in these cases, necessarily, since it comes with additional restrictions. The new documentation has the example of a newtype for a string that compares without considering ASCII case. While that newtype can impl AsRef<str>, it mustn’t have impl Borrow<str> as that would break hash maps and the like.

@jonhoo
Copy link
Contributor

jonhoo commented May 29, 2018

@partim The new documentation is certainly better! I think the confusion stems mostly from the name of the trait though. When I read T: Borrow<Q>, I instinctively think "I can borrow T from a Q", whereas the reality is the other way around. Even reading the example it seems pretty weird: my intuition would be that we need Q to satisfy some bound for it to be usable as a key, whereas it's phrased as a bound on K. It's almost like Borrow is "backwards"?

I think one thing the documentation could use is an explicit "which should I implement and why" section. It's currently mixed in with the CaseInsensitiveString example, but I think it could use being called out.

I also found the statement "For instance, a Box<T> can be borrowed as T while a String can be borrowed as str" somewhat weird. I understand that it's technically correct, but I'd probably be more inclined to say that Box<T> can be borrowed as &T (and similarly &str).

For the lack of a T: Borrow<T>, I think this is where some libraries use Q: Into<Cow<T>>, and maybe that's worth referencing here too?

@matklad
Copy link
Member

matklad commented Apr 3, 2019

I've taken a stab at this at #59663. It seems like explicitly mentioning that every impl of Borrow must maintain Eq, Ord, Hash would do the trick.

Centril added a commit to Centril/rust that referenced this issue Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
Centril added a commit to Centril/rust that referenced this issue Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
Centril added a commit to Centril/rust that referenced this issue Apr 3, 2019
Be more direct about borrow contract

I always was confused by the difference between Borrow and AsRef, despite the fact that I've read all available docs at least a dozen of times.

I finally grokked the difference between the two when I realized the Borrow invariant:

> If you implement Borrow, you **must** make sure that Eq, Ord and Hash implementations are equivalent for borrowed and owned data

My problem was that this invariant is not stated explicitly in documentation, and instead some  vague and philosophical notions are used.

So I suggest to mention the requirements of `Borrow` very explicitly: instead of "use Borrow when X and use AsRef when Y", let's phrase this as `Borrow` differs from `AsRef` in `W`, so that's why `Borrow` is for `X` and `AsRef` is for `Y`.

Note that this change could be seen as tightening contract of the Borrow. Let's say Alice has written the following code:

```rust
#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
struct Person {
    first_name: String,
    last_name: String,
}

impl Borrow<str> for Person {
      fn borrow(&self) -> &str { self.first_name.as_str() }
}
```

Now Bob uses this `Person` struct, puts it into `HashMap` and tries to look it up using `&str` for the first name. Bob's code naturally fails.

The question is, who is to blame: Alice, who has written the impl, or Bob, who uses the HashMap. If I read the current docs literally, I would say that `Bob` is to blame: `Eq` and `Hash` bounds appear on HashMap, so it is the HashMap which requires that they are consistent. By using a type for which the `Borrow` impl does not yield well-behaved `Eq`, Bob is violating contract of HashMap.

If, as this PR proposes, we unconditionally require that Eq & friends for borrow should be valid, then the blame shifts to Alice, which I think is more reasonable.

closes rust-lang#44868
@FeldrinH
Copy link

I wanted to say that despite this issue being closed and the docs presumably improved, I still found the table, explanation and examples here to be clearer and more illuminating than any of the material in the docs.

@GrantGryczan
Copy link

GrantGryczan commented Dec 24, 2023

I agree, and it took me a while to find this thread. I'd argue the docs should be updated with a lot of the information here. I didn't get from the docs that Borrow is meant to be read as "borrow as" and maintain the original semantics of the value (which is a more intuitive but possibly less accurate way of saying Eq, Ord, and Hash are preserved), whereas AsRef is applicable for conversions between related reference types (or something like that).

As a Rust newbie, this also leaves me wondering why I shouldn't just use Into or From for conversions instead of AsRef. I'm guessing it has something to do with Into being generally more expensive, but I don't have a strong grasp of the reasoning for this, or when I'd use either at all. So now I'm going to have to spend a bit of time researching that too.

@clarfonthey
Copy link
Contributor

I think that regardless, "this needs better documentation" is a pretty hard issue to track. Documentation can pretty much always be improved, and without concrete things that need to be done, it's better to just encourage people to do further revisions until everyone is satisfied rather than keeping an issue open to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants