deduplicate and clarify rules for converting pointers to references #128157
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
library/core/src/ptr/mod.rs
Outdated
| //! | ||
| //! * The pointer must point to a valid instance of `T`. | ||
| //! This means that the created reference can only refer to | ||
| //! uninitialized memory through careful use of `MaybeUninit`. |
There was a problem hiding this comment.
This is not quite true. Uninit memory in padding is also okay, and inside (some) unions it is okay, too.
This should probably just link to the reference for the definition of "valid". (Also, I don't think we use the term instance usually. The reference says "valid value of type T".)
There was a problem hiding this comment.
the rust reference does not seem to have a general definition for "valid", it only has sections on bit validity for certain types.
There was a problem hiding this comment.
It does, here. With rust-lang/reference#1540 that becomes a separate section that can even be hyperlinked.
There was a problem hiding this comment.
that pr got merged 2 days ago, but the new section doesn't even show up in the nightly reference.
any idea what the staging cycle is for the reference? if it's relativly short i'm fine blocking this PR on that getting updated so it can include that link, which is quite useful.
There was a problem hiding this comment.
Every 2 weeks.
I'm about to open a PR to update it, but it might take a while to get it reviewed.
library/core/src/ptr/mod.rs
Outdated
| //! except for data inside an `UnsafeCell` | ||
| // ^ previous documentation was somewhat unclear on if modifications through | ||
| // an UnsafeCell are safe even if they would seemingly violate the exclusivity | ||
| // of a mut ref. |
There was a problem hiding this comment.
Previous documentation was quite clear IMO, but note that the documentation is different for functions that return a mutable ref vs a shared ref. The UnsafeCell remark only applies to shared references. By deduplicating the docs you made the comment also apply to mutable references, which is wrong.
There was a problem hiding this comment.
i originally had a disclaimer that this does not apply to mutable references, but then i found a function which return a mutable reference and had the UnsafeCell remark, so i removed that and added this hidden comment. consulting the UnsafeCell documentation reveals that your assessment is correct, so i'll add it back.
There was a problem hiding this comment.
Ah probably someone made a copy-paste error then, this should not ever show up in a &mut-returning function.
also start deduplicating the docs that are getting moved to this section.
lolbinarycat
force-pushed
the
unify-ptr-ref-docs
branch
from
c354b8f to
3877a7b
Compare
|
I just pushed my final suggestions, otherwise it looks good, thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
…=cuviper deduplicate and clarify rules for converting pointers to references part of rust-lang#124669
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126013 (Add `#[warn(unreachable_pub)]` to a bunch of compiler crates) - rust-lang#128157 (deduplicate and clarify rules for converting pointers to references) - rust-lang#129032 (Document & implement the transmutation modeled by `BikeshedIntrinsicFrom`) - rust-lang#129250 (Do not ICE on non-ADT rcvr type when looking for crate version collision) - rust-lang#129340 (Remove Duplicate E0381 Label) - rust-lang#129560 ([rustdoc] Generate source link on impl associated types) - rust-lang#129622 (Remove a couple of unused feature enables) - rust-lang#129625 (Rename `ParenthesizedGenericArgs` to `GenericArgsMode`) - rust-lang#129626 (Remove `ParamMode::ExplicitNamed`) Failed merges: - rust-lang#128166 (Improved `checked_isqrt` and `isqrt` methods) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128157 - lolbinarycat:unify-ptr-ref-docs, r=cuviper deduplicate and clarify rules for converting pointers to references part of rust-lang#124669
| //! not get accessed (read or written) through any raw pointer, | ||
| //! except for data inside an `UnsafeCell`. | ||
| //! Note that aliased writes are always UB for mutable references, | ||
| //! even if they only modify `UnsafeCell` data. |
There was a problem hiding this comment.
This is wrong. It was copied from docs for a function that returns a mutable reference, but this section here claims to be about shared references.
EDIT: Actually this is not even right for mutable references. It's a weird mix of the two that is wrong for both cases.
| /// * The pointer must point to an initialized instance of `T`. | ||
| /// When calling this method, you have to ensure that *either* | ||
| /// the pointer is null *or* | ||
| /// the pointer is [convertible to a reference](crate::ptr#pointer-to-reference-conversion). |
There was a problem hiding this comment.
That section is about shared references though, and this function returns a mutable reference.
|
I will file a PR fixing the mistakes here, that's probably easier than going back and forth in the review. |
|
PR is up: #129652 |
fix Pointer to reference conversion docs The aliasing rules documented in rust-lang#128157 are wrong, this fixes them.
fix Pointer to reference conversion docs The aliasing rules documented in rust-lang#128157 are wrong, this fixes them.
Rollup merge of rust-lang#129652 - RalfJung:ptr-to-ref, r=traviscross fix Pointer to reference conversion docs The aliasing rules documented in rust-lang#128157 are wrong, this fixes them.
part of #124669