Uh oh!
There was an error while loading. Please reload this page.
rephrase UnsafeCell doc - #48201
Conversation
Make UnsafeCell doc easier to follow
rust-highfive
commented
Feb 14, 2018
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TimNN (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. |
TimNN
commented
Feb 18, 2018
Re-assigning to docs team. |
emilyalbini
commented
Feb 26, 2018
Thanks for the PR @NovemberZulu! @steveklabnik, or someone else from @rust-lang/docs, can we get a review on this PR? |
| /// that there are no active mutable references or mutations when an immutable reference is obtained | ||
| /// from the cell. This is often done via runtime checks. | ||
| /// to do this. When `UnsafeCell<T>` _itself_ is immutably aliased, it is still safe to obtain | ||
| /// a mutable reference to its _interior_ and/or to mutate the interior. However, it is up to |
There was a problem hiding this comment.
I'm not sure that putting italic in here brings anything.
GuillaumeGomez
commented
Feb 27, 2018
Except for my little concern, it seems good to me. |
remove italic as per @GuillaumeGomez suggestion
I would really like someone from @rust-lang/libs or @rust-lang/lang to sign off here; changing an unsafe lang item's docs is something i'm super wary of doing without their input. |
| /// that there are no active mutable references or mutations when an immutable reference is obtained | ||
| /// from the cell. This is often done via runtime checks. | ||
| /// to do this. When `UnsafeCell<T>` itself is immutably aliased, it is still safe to obtain | ||
| /// a mutable reference to its interior and/or to mutate the interior. However, it is up to |
There was a problem hiding this comment.
Can you reformat this as a list? Several of the entries are long, and the sentence becomes hard to follow.
| /// to do this. When `UnsafeCell<T>` itself is immutably aliased, it is still safe to obtain | ||
| /// a mutable reference to its interior and/or to mutate the interior. However, it is up to | ||
| /// the abstraction designer to ensure that no two mutable references obtained this way are active | ||
| /// at the same time, there are no active immutable reference when a mutable reference is obtained |
There was a problem hiding this comment.
These rules are kind of confusingly stated. I think it should be something like the following:
(1) While a mutable reference exists, no second reference or of any kind may exist.
(2) While a reference of any kind exists, all mutations must go through that reference. This means that mutations cannot occur while an immutable reference exists.
There was a problem hiding this comment.
I'll try to merge three rules into one, so it will be easier to follow.
NovemberZulu
commented
Feb 27, 2018
As far as I understand -- and I quite possible I am wrong -- simply storing a "bad" reference is not a crime, it is just some bytes in memory, but actually using it is UB, so I talk about active references in the doc. What exactly is "using" as opposed to stored is another story, and should be defined in memory model, I believe. |
nikomatsakis
commented
Feb 28, 2018
I tried to offer some feedback but found it harder than expected. I will say that I continue to be strongly opposed to the term "immutable reference" -- I know we use it a lot, but particularly in the context of UnsafeCell, it strikes me as quite confusing. I'd prefer that we either say "shared reference" or just I personally found the current form of "English-y paragraph" sort of confusing, to be honest. I think I'd prefer something that says like:
The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious:
|
NovemberZulu
commented
Feb 28, 2018
I totally agree that proper wording here is a challenge -- the text must be univocal, concise, yet as simple as possible. I think that "shared reference" might be a bit confusing, because of "shared with what?" question. I think the best way is to state explicitly allowed code flows, at least for single-thread access -- multi-thread access is way more complicated (does Rust strictly define happens-before relation?) -- something like this:
The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious:
|
nikomatsakis
commented
Feb 28, 2018
We inherit the C++11 model, basically. |
NovemberZulu
commented
Feb 28, 2018
(sigh) I was so hoping to get away from it... |
emberian
commented
Feb 28, 2018
@NovemberZulu why? because of how complex it is? |
NovemberZulu
commented
Feb 28, 2018
Because, as with numerous other C++ features, you never know when it is going to bite you in the ass |
emberian
commented
Feb 28, 2018
I'm confused. The C++11 memory model is extremely well understood. http://www.cl.cam.ac.uk/~pes20/weakmemory/#CPP |
NovemberZulu
commented
Feb 28, 2018
C++ is hardly an esoteric or obscure language, and the documentation is both readily available and extensive, that is certainly true. Still, to my mind, it is too easy to end up with a code that looks OK, seems to work the way you expect it to work, but is, in fact, UB. P.S. I think we are digressing. |
steveklabnik
commented
Mar 1, 2018
@nikomatsakis we use "reference" and "mutable reference", officially. Or at least, that's what I've been keeping consistent in every bit of docs I come across, it's hard, of course. I agree "immutable reference" is a bit misleading, which is part of why we chose this this way so long ago. |
NovemberZulu
commented
Mar 1, 2018
I am afraid I have been using "reference" as an umbrella term for both Anyway, may be we can just use |
Kimundi
commented
Mar 1, 2018
@NovemberZulu: A |
NovemberZulu
commented
Mar 1, 2018
Yes, true, and this is exactly the case we have. I still have a mental picture of |
nikomatsakis
commented
Mar 1, 2018
That make sense, but it seems like sometimes we will want to clarify that we mean an |
If "reference" isn't clear enough, I often write
|
NovemberZulu
commented
Mar 7, 2018
Another attempt at rewording: If you have a reference The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious:
To assist with proper design, the following scenarios are explicitly declared legal for single-thread code:
|
nikomatsakis
commented
Mar 8, 2018
@NovemberZulu I like it quite a lot |
based on @nikomatsakis texthg
NovemberZulu
commented
Mar 8, 2018
Updated the pull request. I feel the transition between old and new text is bit rough. Comments are welcome! |
emilyalbini
commented
Mar 12, 2018
Ping from triage @steveklabnik! The author pushed new commits, could you review them? |
steveklabnik
commented
Mar 12, 2018
I think it reads great, thanks! @bors: r+ rollup |
bors
commented
Mar 12, 2018
📌 Commit 55be283 has been approved by |
rephrase UnsafeCell doc As shown by discussions on users.rust-lang.org [[1]], [[2]], UnsafeCell doc is not totally clear. I tried to made the doc univocal regarding what is allowed and what is not. The edits are based on my understanding following [[1]]. [1]: https://users.rust-lang.org/t/unsafecell-behavior-details/1560 [2]: https://users.rust-lang.org/t/is-there-a-better-way-to-overload-index-indexmut-for-a-rc-refcell/15591/12
NovemberZulu
commented
Mar 13, 2018
Everyone, thank you very much for your support! |
As shown by discussions on users.rust-lang.org [1], [2], UnsafeCell doc is not totally clear. I tried to made the doc univocal regarding what is allowed and what is not. The edits are based on my understanding following [1].