Safe drop-by-value seems trivial. Why can't I find it?

help
1

Very often, I lament that the Drop::drop function takes &mut self and not self which would be quite handy for eg. writing a mempool datastructure and plenty of other things.
But I can quite easily write the following and it seems totally safe to me:

use std::{
    mem::ManuallyDrop,
    ops::{Deref, DerefMut},
};

pub trait DropByValue {
    fn drop_by_value(self);
}

pub struct DroppedByValue<T: DropByValue>(ManuallyDrop<T>);

impl<T: DropByValue> DroppedByValue<T> {
    pub fn new(t: T) -> Self {
        Self(ManuallyDrop::new(t))
    }

    pub fn into_inner(mut self) -> T {
        unsafe {
            let mut this = ManuallyDrop::new(self);
            ManuallyDrop::take(&mut this.0)
        }
    }
}

impl<T: DropByValue> Drop for DroppedByValue<T> {
    fn drop(&mut self) {
        unsafe { ManuallyDrop::take(&mut self.0).drop_by_value() }
    }
}

impl<T: DropByValue> Deref for DroppedByValue<T> {
    type Target = T;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl<T: DropByValue> DerefMut for DroppedByValue<T> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

Now to use it, instead of writing

pub struct Foo {
  bar: Bar,
  baz: Baz,
}

impl Drop for Foo {
  fn drop(&mut self) {
    ...
  }
}

I would write

pub struct Foo(DroppedByValue<FooInner>);

struct FooInner {
  bar: Bar,
  baz: Baz,
}

impl DropByValue for FooInner {
  fn drop_by_value(self) {
    ...
  }
}

without ever having to write unsafe. Is there already an equivalent of this? Is it actually unsafe in some subtle way?

2 Likes
2

Generally the pattern of take(self); forget(self); is at risk of being subtly unsound. Prefer let this = ManuallyDrop::new(this); take(&mut *this); instead, which doesn't use/move self after you've invalidated it.

I this usage is (intended to be[1]) sound, but forget-first is generally preferable anyway.

(I am a member of T-opsem but speaking solely for myself, not for the team.)

I had a crate drop-take that offered something like this, but I yanked it for I don't recall why. The pattern generally works, but is at some risk of drop cycles if you aren't careful.

  1. Don't ask, it's complicated. ↩︎

2 Likes
3

In general, what do you want a concrete DropByValue impl to look like? If self is passed by value, then Drop will be called again at the end of the function. Or do you expect every single impl to correctly forget(self)? Because that's a huge footgun. The fact that Drop::drop() takes a reference is no accident.

4

@CAD97 I don't see how let this = ManuallyDrop::new(this); take(&mut *this); can be used here, seeing as how I also need an impl for Drop.

5

@H2CO3 I gave an example usage. See the Foo/FooInner datatypes. No mem::forget required. drop_by_value is called once on FooInner whenever Foo is dropped.

6

For the into_inner fn, i.e.

    pub fn into_inner(mut self) -> T {
        unsafe {
            let this = ManuallyDrop::new(self);
            ManuallyDrop::take(&mut this.0)
        }
    }
2 Likes
7

I think they’re just referring to rewriting

    pub fn into_inner(mut self) -> T {
        unsafe {
            let result = ManuallyDrop::take(&mut self.0);
            std::mem::forget(self);
            result
        }
    }

into

    pub fn into_inner(self) -> T {
        unsafe {
            let mut this = ManuallyDrop::new(self);
            ManuallyDrop::take(&mut this.0)
        }
    }

Edit: I was too slow :laughing:

2 Likes
8

@CAD97 Ah! A ManuallyDrop wrapping another ManuallyDrop. I didn't think of that. Nice :+1: . Edited to use that instead

9

The only way I could imagine getting a Drop-cycle is if FooInner::drop_by_value constructed a new Foo and dropped it, and of course this is just generally true of having drop construct a new instance of the thing it's meant to be dropping. I suppose it's rendered a little bit easier since you're guaranteed to have everything you need to construct a new Foo, but I still find it hard to believe it's very easy to do by accident.

11

Not really used much, but I’ve found this crate which seems to mirror the API you propose.

2 Likes
12

It's not a particularly large hazard the way you've set it up with a struct DroppedByValue wrapper, but it's more of a pitfall if you give dropping Foo access to Foo by value, instead of splitting it into two parts like you have here.

1 Like
13

Why does the builtin Drop::drop take &mut self? well, for one thing, drop_in_place needs to exist in order for a Vec<ComplexDrop> of size 100M to work properly without blowing out resource limits.

1 Like
14

I think I was more asking about why, given how useful this pattern seems to be, I couldn't find this alternative rather than why the default is the way it is.

15

This is really interesting and I am keen to see an answer. When you implement Drop for a type T it comes with some restrictions as far as I understand:

  1. T can't also be Copy.
  2. The drop checker requires that any generic arguments outlive T.
  3. You can't partially move out of T.

Are there any similar restrictions you'd need for DropByValue to be sound?

16

The restrictions directly transfer if the type DroppedByValue<T> is used, since DroppedByValue has an ordinary Drop implementation. (Well… and then, partial moves are additionally also prevented by the fact that DroppedByValue can only offer access to its contents via Deref[Mut].)

To be precise, there is no requirement that T itself is Copy or drop checker restrictions on T itself, so a type T: Copy can implement DropByValue just fine; but DroppedByValue<T> still won’t become Copy, and the drop checker will care about dead lifetimes in DroppedByValue<T>.

1 Like