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.
// impl Borrow<str> for String
Copy link
Member

Choose a reason for hiding this comment

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

these lines seem left in by accident?

// impl<T> Borrow<T> for Arc<T>
// impl<K> HashSet<K> { fn get<Q>(&self, q: &Q) where K: Borrow<Q> }

/// 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.

///
/// 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.

///
/// 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`] or [`Rc`] as well the owned version of
Copy link
Member

Choose a reason for hiding this comment

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

these three types should have their <T>s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Before I commit that: AsRef and Borrow don’t need theirs?

Copy link
Member

Choose a reason for hiding this comment

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

i always forget that those don't have parameters, but generally, they're part of the type's "proper name", so they get paramters. So like "vector" doesn't need one, but Vec<T> should have one, that kind of thing

/// slices such as [`Vec`].
///
/// A relaxed version that allows providing a reference to some other type
Copy link

Choose a reason for hiding this comment

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

providing -> converting?

/// without any further promises is available through [`AsRef`].
///
/// 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`]: ../boxed/struct.Box.html
/// [`Rc`]: ../rc/struct.Rc.html
/// [`Vec`]: ../vec/struct.Vec.html
/// [`AsRef`]: ../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`] owns both keys and values. If the key’s
Copy link
Member

Choose a reason for hiding this comment

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

<K, V>

/// 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`.
Copy link

Choose a reason for hiding this comment

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

string -> String?

///
/// Slightly simplified, the relevant parts of `HashMap` 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
Copy link

Choose a reason for hiding this comment

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

/// {
/// # 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 the stored type for the key, `K`.
Copy link

Choose a reason for hiding this comment

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

How about just "over the key type, K"?

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 wanted to already hint at the difference between K and Q here, i.e, that K is in fact some sort of owned type for the key. I’ll try and rewrite it in a way that expresses this more clearly.

/// When inserting a value, the map is given such a `K` and needs to find
Copy link

Choose a reason for hiding this comment

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

"inserting a key-value pair,"

/// the correct hash bucket and check if the key is already present based
/// on that `K` value. It therefore requires `K: Hash + Eq`.
Copy link

Choose a reason for hiding this comment

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

"that K value" is a bit confusing since it mixes up "value" as in "data" with "value" as in "V".

///
/// 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, image you have a
Copy link

Choose a reason for hiding this comment

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

image -> imagine

/// type that wraps a string but compares ASCII letters case-insensitive:
Copy link

Choose a reason for hiding this comment

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

case-insensitively?
as case-insensitive?

///
/// ```
/// 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 { }
/// ```
///
/// 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]`.
/// Because two equal values need to produce the same hash value, the
/// implementation of `Hash` need to reflect that, too:
Copy link

Choose a reason for hiding this comment

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

need -> needs

///
/// 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]>`.
/// ```
/// # 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)
/// }
/// }
/// }
/// ```
///
/// If you are implementing `Borrow` and both `Self` and `Borrowed` implement
/// `Hash`, `Eq`, and/or `Ord`, they must produce the same result.
/// 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.

///
/// `Borrow` is very similar to, but different than, `AsRef`. See
/// [the book][book] for more.
/// [`Hash`]: ../hash/trait.Hash.html
/// [`HashMap`]: ../collections/struct.HashMap.html
/// [`String`]: ../string/struct.String.html
/// [`str`]: ../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