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

This sounds like exactly why ManuallyDrop exists.

It safely Derefs to its contents, and it's easy to be confident that one has maintained its invariants if the only place you use its unsafeness is in Drop::drop.

4 Likes

Ah, I see now what you meant. But yes, that forces the user of X to be aware of that make_droppable which is not very convenient.

Ah, ok. I'm not very familiar with the implementation details of the drop flag. But yes, X::precious would always be a Precious. Precious is non-copy, non-clone and has an impl Drop.

Is there in the example a probem of double-drop? The compiler knows exactly which fields it moves as parameters into my custom function, and which not.

Well, one of the things I want to do is actually in the example. I do that nowadays by using Option in order to be able to move out in drop. But as pointed out in that example, this could be solved much more elegantly by such a custom after_drop. It is more elegant because I statically know that precious is present. I simply don't want to bother with a possible self.precious == None even though that should never happen, but if I make a mistake, I would only find out at runtime. Checking this at compile time is simply better. The compiler has all the required knowledge to support this.

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.

How doesn’t it? Here’s your first motivating example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=edf81d6ed0bb4aca1359d9c9f138a252

use std::mem::{self, ManuallyDrop};

fn do_something_more_with_precious_resource(resource: PreciousResource) {}

struct PreciousResource;

struct Guard {
    resource: ManuallyDrop<PreciousResource>,
}

impl Guard {
    fn normal_and_frequent_access_to_resource(&self) {
        // For each access, just Deref
        let _: &PreciousResource = &*self.resource;
    }
}

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)

4 Likes

Oh true, I can leave the memory in invalid state behind. Great! Thanks!

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.

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).

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.)

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.)

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".)

3 Likes

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).

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.

3 Likes

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.

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

1 Like

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
    }
}
1 Like

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!

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?)

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.

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.

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.

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.

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.

1 Like

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.