Improve documentation for Borrow #46518

partim · 2017-12-05T17:17:28Z

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 #44868 and #24140.)

rust-highfive · 2017-12-05T17:17:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kimundi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

steveklabnik · 2017-12-05T17:20:51Z

src/libcore/borrow.rs

@@ -12,26 +12,143 @@

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

-/// A trait for borrowing data.
+// impl Borrow<str> for String


these lines seem left in by accident?

steveklabnik · 2017-12-05T17:21:17Z

src/libcore/borrow.rs

+///
+/// As a consequence, this trait should only be implemented for types managing
+/// a value of another type without modifying its behavior. Examples are
+/// smart pointers such as [`Box`] or [`Rc`] as well the owned version of


these three types should have their <T>s

Ack.

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

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

steveklabnik · 2017-12-05T17:21:54Z

src/libcore/borrow.rs

+///
+/// # Examples
+///
+/// As a data collection, [`HashMap`] owns both keys and values. If the key’s


steveklabnik · 2017-12-05T17:22:45Z

I defiantly want @rust-lang/libs to sign off on this. Thank you for taking it on! This is really important work.

ghost · 2017-12-05T18:36:18Z

src/libcore/borrow.rs

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


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. :)

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.

ghost · 2017-12-05T18:40:11Z

src/libcore/borrow.rs

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


string -> String?

ghost · 2017-12-05T18:41:51Z

src/libcore/borrow.rs

+///
+/// impl<K, V> HashMap<K, V> {
+///     pub fn insert(&self, key: K, value: V) -> Option<V>
+///         where K: Hash + Eq


Let's format this example code using the new style:
https://github.com/rust-lang-nursery/rustfmt/blob/master/tests/target/where-clause-rfc.rs

ghost · 2017-12-05T21:41:37Z

src/libcore/borrow.rs

+/// }
+/// ```
+///
+/// The entire hash map is generic over the stored type for the key, `K`.


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

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.

ghost · 2017-12-05T21:45:53Z

src/libcore/borrow.rs

+/// smart pointers such as [`Box`] or [`Rc`] as well the owned version of
+/// slices such as [`Vec`].
+///
+/// A relaxed version that allows providing a reference to some other type


providing -> converting?

ghost · 2017-12-05T21:49:23Z

src/libcore/borrow.rs

+/// ```
+///
+/// The entire hash map is generic over the stored type for the key, `K`.
+/// When inserting a value, the map is given such a `K` and needs to find


"inserting a key-value pair,"

ghost · 2017-12-05T21:51:02Z

src/libcore/borrow.rs

+/// The entire hash map is generic over the stored type for the key, `K`.
+/// When inserting a value, 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` value. It therefore requires `K: Hash + Eq`.


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

ghost · 2017-12-05T21:52:49Z

src/libcore/borrow.rs

+/// result as `Q`’s by demanding that `K: Borrow<Q>`.
+///
+/// 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


image -> imagine

ghost · 2017-12-05T21:53:13Z

src/libcore/borrow.rs

+///
+/// 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
+/// type that wraps a string but compares ASCII letters case-insensitive:


case-insensitively?
as case-insensitive?

ghost · 2017-12-05T21:53:42Z

src/libcore/borrow.rs

-/// (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:


need -> needs

Ixrec · 2017-12-09T00:10:21Z

As an observer who did not follow any of the discussions leading up to this and always found AsRef/Borrow to be quite mysterious no matter how many times I reread that part of The Book, the description in this PR instantly made perfect sense to me.

carols10cents · 2017-12-12T19:00:49Z

Looks like there are some broken links causing the build to fail?

[01:11:23] core/borrow/trait.Borrow.html:64: broken link - core/boxed/struct.Box.html
[01:11:23] core/borrow/trait.Borrow.html:64: broken link - core/rc/struct.Rc.html
[01:11:23] core/borrow/trait.Borrow.html:65: broken link - core/vec/struct.Vec.html
[01:11:23] core/borrow/trait.Borrow.html:80: broken link - core/collections/struct.HashMap.html
[01:11:23] core/borrow/trait.Borrow.html:84: broken link - core/string/struct.String.html
[01:11:23] core/borrow/trait.Borrow.html:85: broken link - core/primitive.str.html
[01:11:25] alloc/borrow/trait.Borrow.html:68: broken link - alloc/convert/trait.AsRef.html
[01:11:25] alloc/borrow/trait.Borrow.html:74: broken link - alloc/convert/trait.AsRef.html
[01:11:25] alloc/borrow/trait.Borrow.html:80: broken link - alloc/collections/struct.HashMap.html
[01:11:25] alloc/borrow/trait.Borrow.html:85: broken link - alloc/primitive.str.html
[01:11:25] alloc/borrow/trait.Borrow.html:123: broken link - alloc/hash/trait.Hash.html
[01:11:26] thread 'main' panicked at 'found some broken links', /checkout/src/tools/linkchecker/main.rs:49:8

In the meantime:

I defiantly want @rust-lang/libs to sign off on this. Thank you for taking it on! This is really important work.

Could someone from @rust-lang/libs start the RFC approval process?

dtolnay · 2017-12-19T03:27:26Z

src/libcore/borrow.rs

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


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

dtolnay · 2017-12-19T03:28:32Z

src/libcore/borrow.rs

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


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

dtolnay · 2017-12-19T03:34:15Z

src/libcore/borrow.rs

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


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?

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

dtolnay · 2017-12-19T03:41:34Z

src/libcore/borrow.rs

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


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 }

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?

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())); }

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(&"..."); }

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.

dtolnay · 2017-12-19T03:49:25Z

r? @dtolnay

dtolnay · 2018-01-05T05:54:26Z

It may be valuable to try to push the intuition that Borrow is for types that would not be distinct in a less granular type system than Rust's. The Rust approach to ownership and borrowing, its standpoint in the systems programming space, its dedication to performance and low-level programmer control and zero cost abstractions, all necessitate in various ways that we expose multiple representations of certain types.

For example we can all think of programming languages in which the distinction between str and String does not exist. In a sense those two are one type and so Borrow<str> for String is appropriate.

Similar situation for T and Box<T>, or T and Rc<T>. In a less granular (particularly garbage collected) environment these are not distinct types.

Similar also for [T] and [T; 16], and really everything that can and does implement Borrow. What do you think @partim? Does that align with your intuition around Borrow and do you find it a useful way to frame the role of this trait?

carols10cents · 2018-01-22T21:26:28Z

Hi @partim, just checking in! Do @dtolnay's comments make sense?

partim · 2018-01-22T21:43:36Z

Absolutely! My plan was to answer with new text, but then Christmas and now (lucky me!) vacation. Sorry for the delays. I’ll try and propose a new summary and introduction soon.

pietroalbini · 2018-02-26T17:06:00Z

@partim hi from the release team! The reviewer added a few comments that should be addressed, could you fix them so this PR can be merged? Thanks!

partim · 2018-02-26T17:21:27Z

I was hoping to get some feedback from @dtolnay on my proposals for solving two of the comments before rewriting the text.

partim · 2018-02-27T15:32:41Z

Many thanks for the great feedback, @dtolnay!

I would like to discuss the headline for the trait. The current documentation has "A trait for borrowing data" which I think doesn’t convey the purpose of the trait very much. My current headline is "A trait identifying how borrowed data behaves" but that is probably even worse. The current text suggests something along the line of "A trait to signal a specialized representation" but that is clearly to lofty and not a good summary at all.

Does anyone have a good idea?

On the other hand, maybe it is a good idea to keep the old headline and modify the text to explicitely introduce the term "borrowing as something"?

dtolnay

The most important thing about the headline should be to concisely communicate that the data underlying a given reference, for example a &Vec<T>, can occasionally be borrowed in a selection of representations, in this case &Vec<T> and &[T]. This is sufficient to differentiate Borrow from Deref (which exposes a single referent, not a choice of representations) as well as AsRef (which is not limited to representations of just one conceptual underlying data).

pietroalbini · 2018-03-12T10:24:54Z

Ping from triage @partim! @dtolnay replied to your question, any chance you can finish this PR?

partim · 2018-03-17T13:42:43Z

I've decided to bring back the term "borrowing as" since that is what the trait is named. I am now quite happy with the text.

I have also rewritten the documentation for BorrowMut, trying to reflect the wording on Borrow and point to it.

I will provide a rewrite of the AsRef documentation next, unless you prefer to have that in a separate PR?

dtolnay

Looks good to me. Thanks for sticking with this. Let's have @rust-lang/docs take another look because this has changed significantly since Steve's last review.

steveklabnik · 2018-03-18T11:45:39Z

src/libcore/borrow.rs

+/// [`String`]: ../../std/string/struct.String.html
+/// [`borrow`]: #tymethod.borrow
+///
+///


can you remove one of these blank lines please?

steveklabnik · 2018-03-18T11:45:57Z

src/libcore/borrow.rs

+/// [`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
 ///


can you remove this line please?

steveklabnik

two tiny formatting nits, and then r=me

thank you so much again for this PR and sticking with it!

partim · 2018-03-18T12:07:49Z

All right, I suggest to start a new PR for AsRef to not delay this one any further.

Thank you, everyone, for your feedback and help!

dtolnay · 2018-03-18T16:42:10Z

@bors r+ rollup

bors · 2018-03-18T16:42:11Z

📌 Commit 13d94d6 has been approved by dtolnay

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

Rollup of 17 pull requests - Successful merges: #46518, #48810, #48834, #48902, #49004, #49092, #49096, #49099, #49104, #49125, #49139, #49152, #49157, #49161, #49166, #49176, #49184 - Failed merges:

Rewrite Borrow's trait documentation.

cba5f6b

rust-highfive assigned Kimundi Dec 5, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2017

partim mentioned this pull request Dec 5, 2017

Borrow and AsRef are redundant #24140

Closed

steveklabnik reviewed Dec 5, 2017

View reviewed changes

Remove trailing white space.

c4ea700

ghost reviewed Dec 5, 2017

View reviewed changes

Include feedback and try to make examples build on all channels.

85e8a9b

Fix documentation links.

fc6c638

dtolnay reviewed Dec 19, 2017

View reviewed changes

dtolnay assigned dtolnay and unassigned Kimundi Dec 19, 2017

kennytm added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jan 3, 2018

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 9, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2018

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2018

Further refinement of Borrow documentation.

44be054

dtolnay reviewed Mar 4, 2018

View reviewed changes

partim added 2 commits March 17, 2018 14:05

Bring back the phrase 'borrowing as' for what Borrow does.

5bef034

Rewrite the documentation for BorrowMut.

d664b89

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 17, 2018

dtolnay approved these changes Mar 17, 2018

View reviewed changes

dtolnay changed the title ~~[WIP] Improve documentation for Borrow, AsRef, and friends~~ Improve documentation for Borrow, AsRef, and friends Mar 17, 2018

steveklabnik reviewed Mar 18, 2018

View reviewed changes

steveklabnik approved these changes Mar 18, 2018

View reviewed changes

Fix formatting.

13d94d6

partim changed the title ~~Improve documentation for Borrow, AsRef, and friends~~ Improve documentation for Borrow Mar 18, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2018

kennytm mentioned this pull request Mar 19, 2018

Rollup of 17 pull requests #49190

Merged

bors merged commit 13d94d6 into rust-lang:master Mar 20, 2018

Improve documentation for Borrow #46518

Improve documentation for Borrow #46518

Conversation

partim commented Dec 5, 2017 • edited Loading

rust-highfive commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ixrec commented Dec 9, 2017

carols10cents commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtolnay commented Dec 19, 2017

dtolnay commented Jan 5, 2018

carols10cents commented Jan 22, 2018

partim commented Jan 22, 2018

pietroalbini commented Feb 26, 2018

partim commented Feb 26, 2018

partim commented Feb 27, 2018

dtolnay left a comment

Choose a reason for hiding this comment

pietroalbini commented Mar 12, 2018

partim commented Mar 17, 2018

dtolnay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik left a comment

Choose a reason for hiding this comment

partim commented Mar 18, 2018

dtolnay commented Mar 18, 2018

bors commented Mar 18, 2018

partim commented Dec 5, 2017 •

edited

Loading