Re-use struct fields on drop; was: Drop(&mut self) vs. drop(self)

[literda]

@CAD97

Just to clarify: this is specifically because of the semantics of ManuallyDrop . I’ve still put it into an unsafe state in Guard::drop , but because it cannot be accessed afterwards, and ManuallyDrop suppresses the drop impl, this is fine (so long as you don’t use it after invalidation).

That’s why it requires the unsafe block.

Yes. And while it works well, it still itches because it requires unsafe. What's your opinion about adding support for this kind of thing to the compiler? If there's no obvious reason why this wouldn't work, then I'll go on and work out a RFC and try to implement that.

[CAD97]

I do not have time to implement it right now (I say as I struggle to prevent myself from going and implementing it), but a #[derive(AfterDrop)] could be set up for this. Given that a safe abstraction can be built without language support, getting it into the stdlib is going to be an uphill battle if you want to fight for it; the Rust stdlib prefers to stick to fundamental abstractions.

Playground example

#[derive(AfterDrop)]
struct Guard {
    #[after_drop]
    resource: ManuallyDrop<Resource>,
    trivial: Trivial,
}

// derived
impl Drop for Guard {
    fn drop(&mut self) {
        let mut resource;
        unsafe {
            resource = mem::uninitialized();
            ptr::copy_nonoverlapping(&*self.resource, &mut resource, 1);
        }
        Self::after_drop(resource);
    }
}

You could probably use frunk's (Labelled)Generic to make AfterDrop<HList![..]> a trait, but it seems unnecessary. You could make the trait AfterDrop<(..,)> if you really want it to be a trait; the derive macro can construct the flat tuple (wheras HList is a cons-list tuple).

[scottmcm]

@literda

I don’t think that ManuallyDrop allows me to do what I would like. I can simply not use ManuallyDrop::into_inner(slot: ManuallyDrop<T>) -> T from a drop(&mut self) because I can not move slot .

Just ptr::read it:

impl Drop for Guard {
  fn drop(&mut self) {
    let resource = unsafe { ManuallyDrop::into_inner(ptr::read(&self.resource)) };
    do_something_more_with_precious_resource(resource);
  }
}

you shouldn't then read it again, but since the only place you're doing this is in drop, that's easy.

(It might be worth having an unsafe method for this combination specifically, to make it more obvious.)

@CAD97

impl Drop for Guard {
    fn drop(&mut self) {
        let mut out = unsafe { mem::uninitialized() };
        mem::swap(&mut out, &mut *self.resource);
        do_something_more_with_precious_resource(out)
    }
}

(This could also potentially do ptr::copy instead of mem::swap but I did the easy option)

Please treat mem::uninitialized() as deprecated; it will be soon. And ptr::read is simpler and better here.

(Also, reminder that "swap with a local" is usually better written with mem::replace.)

@literda

Yes. And while it works well, it still itches because it requires unsafe . What’s your opinion about adding support for this kind of thing to the compiler?

The attribute-and-stringly-connected version feels like too much of a hack for me to have great confidence in it getting accepted. I think it would have better luck if it was more like normal behaviour, just a bit special since Drop is already special.

Imagine, say, that you could do something like this:

impl DropPrime for Guard {
  fn drop_prime(Guard { resource }: Self) {
    do_something_more_with_precious_resource(resource);
  }
}

That's already legal syntax. It correctly drops all the fields if you don't put anything in the body, but it gives you ownership of them to move or forget as you wish. It wouldn't even need the "you can't call Drop::drop manually" restriction. (Just a separate ad-hoc restriction of "you must destructure in the parameter-pattern".)

[Riateche]

@literda

The compiler would not only need to prevent moving out self but also need to track which fields got moved out, and which still are to be dropped. This seems very hard.

The compiler already has this functionality:

struct A {
    a: String,
    b: String,
    c: String,
}
fn x(v: A) {
    y(v.a);
    // `v.b` and `v.c` are dropped here but not `v.a`
}

However, this doesn't work with types that implement Drop. So it should be possible to add a special case for drop(self) to permit moving fields out, since drop(self) is not going to drop self (again).

@Ixrec

For most value semantic types u64 , you don’t even need to introduce an Option since picking any random valid value will still happily do nothing on drop. So just to make sure, PreciousResource is a more interesting type than that either has non-trivial Drop logic for all values, or for which no valid value can be quickly and easily pulled out of thin air, right?

This is a step back for Rust. Pattern matching (including on Option) and move semantics help to avoid invalid states and random values. Broken move semantics in drop(&mut self) nullify these benefits completely.

[literda]

@CAD97

but a #[derive(AfterDrop)] could be set up for this

Right, a derive covers almost everything except manually spelling out ManuallyDrop in the struct declaration. I did a first quick implementation and it works fine for me.

@scottmcm

Imagine, say, that you could do something like this:

impl DropPrime for Guard {
  fn drop_prime(Guard { resource }: Self) {
    do_something_more_with_precious_resource(resource);
  }
}

That’s already legal syntax. It correctly drops all the fields if you don’t put anything in the body, but it gives you ownership of them to move or forget as you wish. It wouldn’t even need the “you can’t call Drop::drop manually” restriction. (Just a separate ad-hoc restriction of “you must destructure in the parameter-pattern”.)

Thanks, I like that idea very much, I'll look into this.

[CAD97]

Update: I just published drop-take.

Trait drop_take::DropTake

Take ownership of members of a type during Drop::drop instead of only having a &mut to them.

This trait can be derived, but doing so does not implement the trait; rather, it implements Drop and compels you to implement DropTake .

Your DropTake implementation receives a tuple of all values marked #[drop_take] , with their value extracted from the container by Take .

If you need access to other non- #[drop_take] values, either also #[drop_take] them, or use Take::take (or the functions it is a wrapper for) directly in Drop::drop .

Example

use std::mem::ManuallyDrop;
use drop_take::DropTake;

struct Peach;
struct Banana;
struct Melon;

#[derive(DropTake)]
struct FruitBox {
    #[drop_take]
    peach: ManuallyDrop<Peach>,
    melon: Melon,
    #[drop_take]
    banana: ManuallyDrop<Banana>,
}

impl DropTake for FruitBox {
    type Values = (Peach, Banana);
    fn drop_take((peach, banana): Self::Values) {
        // use `peach` and `banana` by value
        // they're dropped at the end of scope
    }
}

[earthengine]

I think this Take trait should be named UnsafeTake. For some types like Cell and ManuallyDrop, the take method is indeed unsafe; but for Option, it is actually safe!

[CAD97]

Check again. Cell::take is safe. (With another look at the function, though, I’m unsure whether it holds the invariant of my Take trait, as you will end up dropping a value from the “consumed” container: a value of Default::default. It’s not a safety problem, though; just a small gotcha.)

I don’t disagree; I chose Take instead of UnsafeTake merely because it was shorter and I had to choose something. My biggest reason for not calling it UnsafeTake was that it isn’t an unsafe trait. (Should it be?)

[earthengine]

Cell::take is safe only for Default types. But your Take trait is unconditional. So if we have a Take with a safe take method unconditional, Cell cannot implement it.

Should UnsafeTake be an unsafe trait? I think yes, if all purpose of it is to use its only unsafe method.

[CAD97]

Cell::take doesn’t exist for T: !Default. It can’t. I have impl<T: Default> Take for Cell<T>.

And you’ve not given an actual reason for the trait to be unsafe. An unsafe trait means that it’s unsafe to implement. Arbitrary safe code in a Take::take implementation could not be used to create unsafety (as I understand the trait), so it shouldn’t be unsafe to implement.

[jjpe]

@literda

Currently, with Drop::drop(&mut self) the struct needs to wrap the resource into some other container to be able to move it out from within Drop::drop

This is not necessarily true. There's always mem::swap(), which can swap a value in one location (e.g. a struct) with another one without needing a wrapper type. This does come at the cost of creating that new value though.

[literda]

@jjpe

This does come at the cost of creating that new value though.

It's not only the added costs, but you also must be able to create such a replacement value in the first place.

And, "null-like" values for the sole purpose of replacement are exactly what I want to avoid.

[literda]

@CAD97

Update: I just published drop-take .

Nice, at first glance it looks pretty much like what I do so far as well, except that I specify the function to be used for destructuring, instead of adding a trait.

As we discussed before, macros solve all of the problem except for the need to spell out ManuallyDrop in the struct declaration.