Improve #Safety of various methods in core::ptr

[amosonn]

For read, read_unaligned,read_volatile, replace, and drop_in_place:

• The value they point to must be properly initialized

For replace, additionally:

• The pointer must be readable

[rust-highfive]

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

[withoutboats]

r? @RalfJung

[RalfJung]

Hm... this one is tricky, actually. Namely, in general, this is not sufficient... the term "initialized" here, in the sense it is used in these docs, is basically what I called "validity invariant"; however, for drop_in_place, in general we even need the "safety invariant" to hold. Except that for some types we know that dropping them doesn't do anything, so e.g. for ManuallyDrop<Vec<u8>>, we do not need the safety invariant to hold.

I think I'd be more comfortable with something that points out that, since T's destructor is going to be called on the value, UB may be caused if the value is not actually valid for destructing -- the details of this depend on the type.

Cc @Centril @rust-lang/wg-unsafe-code-guidelines

[nikomatsakis]

This sounds quite reasonable to me.

[amosonn]

I would leave this bullet there, since this first produces a &mut T to call drop on, so if it isn't initialized this is inst-UB (like in all the other methods, which return a T).

Then we can add something like:

"The T value to_drop points to must be valid for destructing, which may mean it must uphold additional invariants - this is type dependent."

Is this sufficient? Is there a link for further reading which would be relevant?

[amosonn]

@RalfJung Hmm, reading through the post you linked to, you're stating that (at least currently) having a &mut T to a non-fully-initialized T is actually Ok. Does this mean we should remove that bullet about "properly initialized", since, depending on the destructor, we might e.g. not be reading some field, making it fine for it to be uninitialized?

[RalfJung]

I would leave this bullet there, since this first produces a &mut T to call drop on, so if it isn't initialized this is inst-UB (like in all the other methods, which return a T).

Arguably it wouldn't do that for types that don't even need dropping, would it?

Though, conservatively, it is probably best to declare e.g. drop_in_place(0 as *mut i32) UB for now even if we want to relax this later.

"The T value to_drop points to must be valid for destructing, which may mean it must uphold additional invariants - this is type dependent."

Yeah, adding something like this would be better.
I don't know of a helpful link here.

[amosonn]

For anyone who stubles upon this: it seems that relying on a type not being Drop if it's Copy is not necessarily wished for (AFAII, related to making UnsafeCell impl Copy):
#60715 (comment)

[RalfJung]

I think you misunderstood that other post... even when Cell: Copy, I doubt we'll ever allow types that are Copy + Drop. Since Copy types are implicitly duplicated all the time, all of these copies would have to have their destructor running -- that is the kind of implicit cost Rust tries hard to avoid.

[amosonn]

Well, it does directly say

People already abuse it for making sure a type has a no-op Drop impl, which is its own headache [..]

But this might of course be a misguided complaint?

[RalfJung]

I think @pythonesque just wanted to say that saying T: Copy is a rather roundabout and bad way to say "T has a no-op Drop". It is correct, but (a) the intent is unclear (it looks like you want to copy the type but really you care about its lack of a drop shim), and (b) it is imprecise (many types, such as &mut T, have a no-op Drop but are not Copy).

[amosonn]

Ok, thanks for the explanation!

[RalfJung]

Thanks!

@bors r+ rollup

[bors]

📌 Commit 943e653 has been approved by RalfJung