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

Improve documentation for Borrow #46518

Merged
merged 9 commits into from
Mar 20, 2018
147 changes: 132 additions & 15 deletions src/libcore/borrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,143 @@

#![stable(feature = "rust1", since = "1.0.0")]

/// A trait for borrowing data.
/// A trait identifying how borrowed data behaves.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this sentence confusing. How about:
"Implementors of Borrow<Borrowed> can be borrowed as Borrowed."

Now, that is a lot of borrowing, so perhaps the following reads better?
"Implementors of Borrow<T> can be borrowed as T."

Just an idea. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true for AsRef<T>, too, though. I don’t like the sentence either, but if at all possible in one sentence, the summary should express those strict guarantees implied by Borrow<T>. Or at least be enough of a tease that people will read on.

///
/// In general, there may be several ways to "borrow" a piece of data. The
/// typical ways of borrowing a type `T` are `&T` (a shared borrow) and `&mut T`
/// (a mutable borrow). But types like `Vec<T>` provide additional kinds of
/// borrows: the borrowed slices `&[T]` and `&mut [T]`.
/// If a type implements this trait, it signals that a reference to it behaves
/// exactly like a reference to `Borrowed`. As a consequence, if a trait is
/// implemented both by `Self` and `Borrowed`, all trait methods that
/// take a `&self` argument must produce the same result in both
/// implementations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too strong a statement in my opinion. For example the following is probably fine. Is there a way to make a stronger statement than the existing documentation but without applying it to "all trait methods"?

#![feature(get_type_id)]

use std::any::Any;
use std::borrow::Borrow;

fn assert_borrow<Q: ?Sized, K: Borrow<Q>>() {}

fn main() {
    assert_borrow::<str, String>();
    let str_id = <str as Any>::get_type_id("");
    let string_id = <String as Any>::get_type_id(&String::new());
    println!("{}", str_id == string_id); // false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception could perhaps be formulated as ‘trait methods that concern the type of a value rather than a value.’ It’s a bit awkward; perhaps there is a better way to formulate this?

Are there any exceptions where the value itself concerned, though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is one that proclaims to involve the "val" of a value.

use std::mem;

trait Layout {
    fn size_of_val(&self) -> usize {
        mem::size_of_val(self)
    }
    fn align_of_val(&self) -> usize {
        mem::align_of_val(self)
    }
}
impl<T: ?Sized> Layout for T {}

fn main() {
    println!("size of str: {}", <str as Layout>::size_of_val(""));
    println!("align of str: {}", <str as Layout>::align_of_val(""));
    println!("size of String: {}", <String as Layout>::size_of_val(&String::new()));
    println!("align of String: {}", <String as Layout>::align_of_val(&String::new()));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is one from the standard library that behaves differently between Q and K and the value is concerned.

use std::borrow::Borrow;

fn assert_borrow<Q: ?Sized, K: Borrow<Q>>() {}

fn main() {
    assert_borrow::<str, &str>();

    // copies the data
    let _: String = <str as ToOwned>::to_owned("...");

    // does not copy the data
    let _: &str = <&str as ToOwned>::to_owned(&"...");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps distinguish between ‘management of data’ and ‘properties of data’? It is too vague for my liking, but it sounds like there won’t be a hard-fast rule and it will always be a judgment call.

///
/// When writing generic code, it is often desirable to abstract over all ways
/// of borrowing data from a given type. That is the role of the `Borrow`
/// trait: if `T: Borrow<U>`, then `&U` can be borrowed from `&T`. A given
/// type can be borrowed as multiple different types. In particular, `Vec<T>:
/// Borrow<Vec<T>>` and `Vec<T>: Borrow<[T]>`.
/// As a consequence, this trait should only be implemented for types managing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reword this sentence or the previous one, consecutive sentences should not both start with "As a consequence".

/// a value of another type without modifying its behavior. Examples are
/// smart pointers such as [`Box<T>`] or [`Rc<T>`] as well the owned version
/// of slices such as [`Vec<T>`].
///
/// If you are implementing `Borrow` and both `Self` and `Borrowed` implement
/// `Hash`, `Eq`, and/or `Ord`, they must produce the same result.
/// A relaxed version that allows converting a reference to some other type
/// without any further promises is available through [`AsRef`].
///
/// `Borrow` is very similar to, but different than, `AsRef`. See
/// [the book][book] for more.
/// When writing generic code, a use of `Borrow` should always be justified
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this guidance apply in the case of ToOwned which uses Borrow only as a way to make Cow's Deref work?

match *self {
    Borrowed(borrowed) => borrowed,
    Owned(ref owned) => owned.borrow(),
}

ToOwned certainly counts as "generic code" but the use of Borrow is not justified by additional trait bounds. Would you consider this an incorrect use of Borrow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative proposal: ‘When using Borrow as a trait bound’? And tone down the text to be more along the line of suggesting instead of demanding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable. It may be appropriate to phrase more as justification than as instruction. Instead of instructing a particular use: "here is how you should always do it" or "here is how we suggest that you do it," more like a justification "here is how you will typically find it used and why."

/// by additional trait bounds, making it clear that the two types need to
/// behave identically in a certain context. If the code should merely be
/// able to operate on any type that can produce a reference to a given type,
/// you should use [`AsRef`] instead.
///
/// The companion trait [`BorrowMut`] provides the same guarantees for
/// mutable references.
///
/// [`Box<T>`]: ../../std/boxed/struct.Box.html
/// [`Rc<T>`]: ../../std/rc/struct.Rc.html
/// [`Vec<T>`]: ../../std/vec/struct.Vec.html
/// [`AsRef`]: ../../std/convert/trait.AsRef.html
/// [`BorrowMut`]: trait.BorrowMut.html
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove one of these blank lines please?

/// # Examples
///
/// As a data collection, [`HashMap<K, V>`] owns both keys and values. If
/// the key’s actual data is wrapped in a managing type of some kind, it
/// should, however, still be possible to search for a value using a
/// reference to the key’s data. For instance, if the key is a string, then
/// it is likely stored with the hash map as a [`String`], while it should
/// be possible to search using a [`&str`][`str`]. Thus, `insert` needs to
/// operate on a `String` while `get` needs to be able to use a `&str`.
///
/// Slightly simplified, the relevant parts of `HashMap<K, V>` look like
/// this:
///
/// ```
/// use std::borrow::Borrow;
/// use std::hash::Hash;
///
/// pub struct HashMap<K, V> {
/// # marker: ::std::marker::PhantomData<(K, V)>,
/// // fields omitted
/// }
///
/// impl<K, V> HashMap<K, V> {
/// pub fn insert(&self, key: K, value: V) -> Option<V>
/// where K: Hash + Eq
/// {
/// # unimplemented!()
/// // ...
/// }
///
/// pub fn get<Q>(&self, k: &Q) -> Option<&V>
/// where
/// K: Borrow<Q>,
/// Q: Hash + Eq + ?Sized
/// {
/// # unimplemented!()
/// // ...
/// }
/// }
/// ```
///
/// The entire hash map is generic over a key type `K`. Because these keys
/// are stored by with the hash map, this type as to own the key’s data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two typos: "stored by with the hash map" and "this type as to own"

/// When inserting a key-value pair, the map is given such a `K` and needs
/// to find the correct hash bucket and check if the key is already present
/// based on that `K`. It therefore requires `K: Hash + Eq`.
///
/// In order to search for a value based on the key’s data, the `get` method
/// is generic over some type `Q`. Technically, it needs to convert that `Q`
/// into a `K` in order to use `K`’s [`Hash`] implementation to be able to
/// arrive at the same hash value as during insertion in order to look into
/// the right hash bucket. Since `K` is some kind of owned value, this likely
/// would involve cloning and isn’t really practical.
///
/// Instead, `get` relies on `Q`’s implementation of `Hash` and uses `Borrow`
/// to indicate that `K`’s implementation of `Hash` must produce the same
/// result as `Q`’s by demanding that `K: Borrow<Q>`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a little more to it. If this were the end of the story, then a bound Q: Borrow<K> would work just as well. In fact that would seem to match the previous paragraph better -- Q: Borrow<K> means you can take the &Q and borrow a &K and use K's Hash function to find the right bucket. Would it be possible to clarify what is going on and why Q: Borrow<K> is not just as good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rewritten the explanation, calling out what Q really is supposed to be. Is it more clear now?

///
/// As a consequence, the hash map breaks if a `K` wrapping a `Q` value
/// produces a different hash than `Q`. For instance, imagine you have a
/// type that wraps a string but compares ASCII letters ignoring their case:
///
/// ```
/// # #[allow(unused_imports)]
/// use std::ascii::AsciiExt;
///
/// pub struct CIString(String);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Rust conventionally acronyms count as single words so this would be called CiString. But since this is example code and not an API that users are going to be writing code against, I would swing more toward verbose and self-explanatory. Maybe something like CaseInsensitiveString.

///
/// impl PartialEq for CIString {
/// fn eq(&self, other: &Self) -> bool {
/// self.0.eq_ignore_ascii_case(&other.0)
/// }
/// }
///
/// impl Eq for CIString { }
/// ```
///
/// Because two equal values need to produce the same hash value, the
/// implementation of `Hash` needs to reflect that, too:
///
/// ```
/// # #[allow(unused_imports)] use std::ascii::AsciiExt;
/// # use std::hash::{Hash, Hasher};
/// # pub struct CIString(String);
/// impl Hash for CIString {
/// fn hash<H: Hasher>(&self, state: &mut H) {
/// for c in self.0.as_bytes() {
/// c.to_ascii_lowercase().hash(state)
/// }
/// }
/// }
/// ```
///
/// Can `CIString` implement `Borrow<str>`? It certainly can provide a
/// reference to a string slice via its contained owned string. But because
/// its `Hash` implementation differs, it cannot fulfill the guarantee for
/// `Borrow` that all common trait implementations must behave the same way
/// and must not, in fact, implement `Borrow<str>`. If it wants to allow
/// others access to the underlying `str`, it can do that via `AsRef<str>`
/// which doesn’t carry any such restrictions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case insensitive string example is really clear and helpful! Makes sense why you would not want a Borrow impl. Nicely done.

///
/// [`Hash`]: ../../std/hash/trait.Hash.html
/// [`HashMap<K, V>`]: ../../std/collections/struct.HashMap.html
/// [`String`]: ../../std/string/struct.String.html
/// [`str`]: ../../std/primitive.str.html
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this line please?

/// [book]: ../../book/first-edition/borrow-and-asref.html
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Borrow<Borrowed: ?Sized> {
/// Immutably borrows from an owned value.
Expand Down