struct Foo(bool, u8);
impl Drop for Foo {
fn drop(&mut self) {
println!("{}", self.1);
if !self.0 {
let mut x = std::mem::replace(self, Foo(true, 1));
x.0 = true;
}
}
}
fn main() {
Foo(false, 0);
}
This is fine... Unless you have Pin. It'd be nice if Pin-using traits could use where Self: !Drop and there was a PinDrop that could be used for these cases instead.
In particular, a trait like:
trait UsesPin {
fn foo(Pin<&Self>);
}
while perfectly safe to implement, and actually quite usable (because Pin<&Self> is Deref), is inherently unsound, unless it's marked as unsafe trait instead.
Do you have a demonstraition for that claim. I can't think of any reason this is inherently unsound, since Pin is generally unsafe to construct for !Unpin types.
Taking another look at the post, I realise the potential issue.
So the issue is the fact that you can mem::replace in Drop. This is actually some unsoundness at an intersection:
Box::pin, which is a way to safely construct a Pined reference with a !Unpin type.
The fact that Unpin is safe to unimplement
The fact that drop recieves a &mut self, and not a Pin<&mut Self>.
I would assume the question is "Is this actually unsound", IE. how much of the Pinning guarantee is exposed to consumers.
There is no unsoundness there, since the fields of self, such as self.1, were never pinned to begin with: self was, and self does get properly dropped before being moved or deallocated.
Addendum
to clarify: its drop glue (such as its Drop::drop() implementation) starts being called before being moved or deallocated. See @CAD97's post below for more details about this.
The unsoundness would come from a Pin<&Foo> -> Pin<&PinSensitive<u8>> (field) projection, if the Drop impl of Foo moved the PinSensitive<u8> field. But in order to write such a projection, one needs to use unsafe, with which they assert the non-existence of such a Drop impl.
self is the thing getting mem::replaced in the Drop impl, and that's what been pinned. Foo is a !Unpin type that has been safely pinned, and then its pinning invariant was violated by it's own Drop impl (so arguably, it's unsound to Pin Foo at all).
If pinning a type for the purposes of calling Drop on it is always safe (which it is), then this should solve the issue. The main drawback is that it breaks existing Drop impls. But whatever solution gets picked involves breaking at least something, be it Box::pin or Drop or something else. Drop seems like the biggest footgun tho, so even patching Box::pin wouldn't solve the problem because many crates would likely do the same mistake. Please just patch Drop instead.
I do repeat my question, as to whether or not this is actually unsound, IE. is there a way for external code to be broken by relying on the Pinning invariant of Foo, w/o the crate declaring Foo itself using unsafe code (which would be it's own violation). Or, another way of writing it, can a downstream crate rely on the fact Foo is pinned? The answer, arguably, would have to be no, if it's unknown, to resolve the unsoundness.
I'd also note that a breaking change in this case would likely be a non-starter, given the fact that one particular reason to implement !Unpin is because you have fun Drop logic, so almost every !Unpin type in the wild, I would assume, is going to either be Drop, or have Drop fields.
For sure. We believe it's possible to break it backwards-compatibly by making the Unpin requirement a forbid-by-default-lint, and introduce the PinDrop trait with the for T: Drop impl. This means any crates that use Drop with !Unpin would remain working and all that, but they should really change to using PinDrop instead.
It's always sound to create a Pin for a type before dropping it in place (assuming PinDrop semantics), regardless of whether or not it's Unpin.
This was discussed when Pin was first conceptualized and when it was stabilized.
The resolution is an important one: Pin guarantees the object will remain accessable at the same address from the point it is first observed behind a pin until Drop::drop is called. The pinning guarantee ends the instant that the drop glue calls Drop::drop.
The type itself is then under the contract to not do any operations that would cause any utilization of the pinned state (that necessarily used unsafe) to be incorrect.
This is just another flavor of the fact that the unsafe barrier is the implementation module. Safe code within the privacy barrier (here, Drop::drop) can easily break unsafe invariants relied on by safe code. This is not a hole in Rust's safety, but in fact a key part of it.
(Though maybe we should suggest that all types that have a pinning invariant implement Drop within their main safety barrier, because otherwise the Drop impl could be implemented anywhere in the crate.)
As already said on discord, this is not unsound. Pinning is a guarantee that a user gives to a type, it means that the user won't move it ever again, but the type itself is free to do what it wants. If you're going to move the type in the drop implementation then you have to consider that when you do something unsafe somewhere else, but by itself it can't bring any unsoundness. The std::pin module documentation has a whole section related to this.