Destructure drop structs

[ghost]

Rendered

[sfackler]

The level of detail here seems low. Which solution is this RFC proposing? What are the pros and cons of the various options?

[ghost]

I have added more details and tried to explain the pros/cons. I have also suggested to use the first approach (using an unsafe block).

[oli-obk]

typo: destructuring

[oli-obk]

Another alternative is yet another automatically derived unsafe marker trait. Let's call it IndependentDrop for now (bikeshed welcome). Any type without an explicit Drop (autogenerated is fine) impl will be IndependentDrop iff all its fields are IndependentDrop. Destructuring of Drop types is only possible if the type is also IndependentDrop. Types like Vec will unsafe impl<T> IndependentDrop for Vec<T> {}.

[ghost]

After reading it three times, it doesn't actually sound that bad.
Are all fields required to implement IndependentDrop?

struct MyData {
    i: InteriorUnsafe,
    buf: Vec<u8>
}

MyData should be safe to destructure into InteriorUnsafe and Vec<u8>. The only thing that isn't save to destructure is InteriorUnsafe.

[oli-obk]

Are all fields required to implement IndependentDrop?

If MyData implements Drop, then its fields need to implement IndependentDrop or you need to unsafe impl IndependentDrop for MyData {} and thus guaranteeing that your destructor doesn't do shenigans with its inner types during the invocation of Drop::drop.

[ghost]

Now it makes perfect sense. Adding this alternative to the RFC.

[ahicks92]

This is going to require runtime tracking like drop flags, I think. What happens if you only move some fields out of the struct?

[ghost]

Oh. I was assuming that would not be allowed.
If it is allowed, then the remaining fields would be dropped as part of the destructing.

[oli-obk]

This is going to require runtime tracking like drop flags, I think.

We already do this tracking for the fields of non-Drop structs. And it's tracked without flags where possible. I don't think this would introduce any new issues in that direction

[brianchin]

Is there a particular reason that you discarded Approach 2/1+2 here? It seems to me that if there is a custom Drop implementation that there is some higher-level concept of a valid state within the struct, which would imply that most if not all of the fields would be hidden. As long as all fields need to be visible to destruct a struct with custom Drop, this seems like it would be safe.

In addition, it seems that an approach including #2 would open up some other possibilities for making more flexible safe APIs. For example, I'm thinking there is a way that drop-by-value semantics could be added compatibly to the language in that case.

[canndrew]

I've wanted this since forever, but I also think that approach 2 is the best option. If you want to destructure a type outside of its defining module, then the module author should have given you a method to consume the value and return the parts that you want (something they can only safely do using approach 2).

[ghost]

@brianchin

Is there a particular reason that you discarded Approach 2/1+2 here?

I don't think it's a good idea to have different semantics depending on the place of implementation.

@canndrew
Agreed, approach 2 is certainly the most elegant one. And as you point out, also a sufficient one.

Any arguments against Approach 2?

[ghost]

I have changed the preferred approach of the RFC to Approach 2.

[Ericson2314]

I think I prefer a trait based approach, though approach 2 would be better than nothing. Approach 3 isn't designed correctly, however. Try this:

1. The trait is called Destructure, because that is the operation. A type can only be destructured if it implements this trait---a very simple rule!
2. The trait is implemented for all non-Drop types by default. It doesn't matter whether fields implement the trait or not, because destructuring is a shallow operation. Drop types can opt-in with an explicit impl.
3. The trait is not unsafe. Destructuring is perfectly safe---it is up to the user to decide whether it is compatible with the invariants of their Drop instance.

[ghost]

Do we have a solid answer as to whether the take_mut crate (see #1736) is sound? I feel like it wasn't rejected for soundness reasons. The example given for why destructuring Drop structs is unsound is unsafe in the presence of take_mut without destructuring:

let mut i = InteriorUnsafe::new();
take_mut::take(&mut i.m, |m| {
    drop(m); // Oh no
    panic!();
});

Therefore I don't feel like there needs to be any restrictions on destructuring Drop structs, outside of maybe "non-destructured fields should be forgotten rather than dropped". Alternatively, because forgetting non-destructured fields will probably create horrible memory leaks, you could restrict it so that you can only fully destructure a Drop struct, no missing fields. This would sort of act like Approach#2 I suppose, unless all of the fields are public.

[petrochenkov]

It would be nice if the RFC actually said what is destructuring before proceeding to motivation.
Especially given that the problem is not only about destructuring (in patterns), but about moving out individual fields in general. It can be done through any field access.

struct NonCopy;
let ds = DropStruct { individual_field: NonCopy, other_individual_field: NonCopy };
let f = ds.individual_field; // Moving out without destructuring.
// f is initialized, DROPPED
// ds is uninitialized, NOT DROPPED
// ds.individual_field is uninitialized, NOT DROPPED
// ds.other_individual_field is still initialized, DROPPED

[ghost]

@petrochenkov
I did not intend to allow moving out individual fields, as that makes it too easy to forget the others.
(Will clarify this in the rfc)

[petrochenkov]

I'd rather say the unsafety is InteriorUnsafe's fault, it shouldn't make its field public if it has invariants like this.
So take_mut stays sound and moving out fields should be supported unconditionally.

[Diggsey]

I run into this fairly frequently, and the workarounds are usually very ugly.

[burdges]

There are several competing scenarios for here, including :

If you want to expose destructuring, then a Destructure trait makes the most sense, perhaps with a method that "finalized" the type before destructuring. In this case, I'd think Destructure could even enable a default implementation for Drop::drop so that one could write impl Drop for Type { } after impl Destructure for Type ..., although that does not matter much since being Destructure but not Drop gives an error.

If you just want to take apart your own type to avoid it's drop method, then you want to destructure it like any other type ala approach 2 :

impl MyLightWeightTransaction {
    ...
    pub fn approve_transaction(self) {
        let MyLightWeightTransaction { move_type, .. } = self;
        // We cannot use mom::forget because we have nested drop types, so this
        // destructuring avoids running drop while letting this drops run.
    }
}

I'm suspicious the ergonomics initiative "reforms" have broken this second usage by making some refs implicit here, maybe damaging approach 2. You can work around this usage by adding another a bool to MyLightWeightTransaction, or another variant, which nerfs the drop. At the type level, you can work around it by placing the data in a hidden inner type and the drop on a wrapper type. I do think code would be clearer if there was some convenient mem::drop_interior() function that forgot the type but ran drop recursively though.

[nikomatsakis]

@rfcbot fcp postpone

I could imagine wanting to do something like this eventually, but my feeling is that this is not the time. =) We are currently in the midst of a "from scratch" rewrite of the borrow checker to be basd on MIR, and so I am reluctant to make changes that would extend that task. Moreover, this particular task seems to get a bit harder in the MIR environment, because the distinction between let Foo { a, b } = foo and let a = foo.a; let b = foo.b; is desugared by then. (To me, this is probably a good thing; I would prefer we not adopt rules that make a difference between those two cases.)

I think I'd be open to revisiting this question once the MIR-based borrow checker is more stable, presuming that the proposal were rewritten to be based on MIR.

[rfcbot]

Team member @nikomatsakis has proposed to postpone this. The next step is review by the rest of the tagged teams:

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[aturon]

Closing as postponed. Thanks @s3bk for the RFC!

[kevincox]

Now that MIR and NLL have shipped it is a good time to revisit this?