Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop type destructuring #3738

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jdonszelmann
Copy link

@jdonszelmann jdonszelmann commented Dec 8, 2024

Rust does not allow destructuring types which implement the Drop trait. This means that moving data out of such types is hard and error prone. The rationale is that once fields are moved out, the type's Drop implementation cannot run, which can be undesired. This RFC proposes to allow destructuring anyway, in certain situations.

drop_type_destructuring adds a new built-in macro, destructure. You can use this macro to destructure types that implement Drop. For example, assuming Foo is a type that implements Drop and has fields a and b, you could write the following:

fn example(x: Foo) {
    destructure!(let Foo { a, b: _ } = x);
    // yay we own `a` now, we can even drop it!
    drop(a);
}

Written together with @WaffleLapkin

Rendered

@jdonszelmann jdonszelmann changed the title initial text for drop-type-destructuring Drop type destructuring Dec 8, 2024
@jdonszelmann jdonszelmann force-pushed the drop-type-destructuring branch from 4031570 to 704a3d2 Compare December 8, 2024 00:37
@WaffleLapkin WaffleLapkin added T-lang Relevant to the language team, which will review and decide on the RFC. A-patterns Pattern matching related proposals & ideas A-drop Proposals relating to the Drop trait or drop semantics labels Dec 8, 2024
@ChayimFriedman2

This comment was marked as resolved.

@jdonszelmann

This comment was marked as resolved.

@steffahn
Copy link
Member

steffahn commented Dec 8, 2024

I think there’s a 3rd alternative. Instead of statement or pattern, destructure! could be an expression!

In that version, your examples look like:

fn example(x: Foo) {
    let Foo { a, b: _ } = destructure!(x);
    // yay we own `a` now, we can even drop it!
    drop(a);
}
fn example(x: Foo) {
    let mut a = vec![1, 2, 3];
    
    Foo { a, b: _ } = destructure!(x);
    // yay we own `a` now, we can even drop it!
    drop(a);
}

etc…

The motivating example could also be adapted. Either in a way that matches your “new” implementation

pub fn into_parts(self) -> (W, Result<Vec<u8>, WriterPanicked>) {
    let Self { buf, inner, panicked } = destructure!(self);
    let buf = if !this.panicked { Ok(buf) } else { Err(WriterPanicked { buf }) };

    (inner, buf)
}

or in a way that closely resembles the original code, but without unsafe:

pub fn into_parts(self) -> (W, Result<Vec<u8>, WriterPanicked>) {
    let this = destructure!(self);
    let buf = this.buf;
    let buf = if !this.panicked { Ok(buf) } else { Err(WriterPanicked { buf }) };
    let inner = this.inner;

    (inner, buf)
}

which one would probably further simplify to

pub fn into_parts(self) -> (W, Result<Vec<u8>, WriterPanicked>) {
    let this = destructure!(self);
    let buf = if !this.panicked { Ok(this.buf) } else { Err(WriterPanicked { this.buf }) };

    (this.inner, buf)
}

In this version, destructure!(…expr…) wraps the value of expr: Foo into a value of type Destructure<Foo>. The place destructure! is use must fulfill the relevant visibility restrictions (all fields visible, no external #[non_exhaustive] struct…).

The type Destructure<T: ?Sized> is a transparent wrapper around T that acts partially like ManuallyDrop, except it only disable the outermost drop implementation of T itself, but keeps all drop glue. Furthermore, it dereferences to T in an owned-access manner (like Box), but with the additional power that when T is a struct or enum, one can move out of fields of T even when T implements Drop.

The code examples above assume pattern matching can transparently “dereference” this layer of Destructure, otherwise it’d have to be written like

let Foo { a, b: _ } = *destructure!(x);

why not have destructure!(…) itself expand to the equivalent of *destructure!(…) above?
I like that the let this = destructure!(self); approach with subsequent field access is also an option; and given match ergonomics exist, and implicit dereferencing of Box and other deref-patterns seems desirable (IMO), I think enabling matching Deref<Foo> directly against a Foo { …… } pattern seems a good design after all.


Finally, such a Destructure type is essentially all we need in order to offer a way to implement Drop in a by-value manner. (There probably exist many prior discussions on that kind of point, one in particular I could find right now would be #1180 in this very repo.)

(some code examples on this point)

The trait would be

trait DestructuringDrop {
    fn drop(self: Destructure<Self>);
}

and then you can do stuff like

struct S {
    value: Owned,
    other_fields: Something
}
impl DestructuringDrop for S {
    fn drop(self: Destructure<Self>) {
       // whatever custom drop code
       // we van get ownership of the `value` by just
       let v: Owned = this.value;
       // can also pass elsewhere
       function_taking_owner(v);
       
       // other_fields will still drop automatically
       // insofar they haven’t been moved out of
    }
}

Similar to Drop, this is only meant to be implemented, not as a trait bound. For backwards compatibility (e.g. the tricks some macros like pin-project play to prevent Drop impls), implementing DestructuringDrop for a type will also make the type fulfill T: Drop bounds; also implementing either is mutually exclusive. Finally, existing Drop implementations will effectively desugar to DestructuringDrop, roughly as if the following impl existed

impl<T: ?Sized + Drop> DestructuringDrop for `T` {
    fn drop(mut self: Destructure<Self>) {
        // dereferences `Destructure<Self>` to `Self` and takes a mutable reference of that place
        <T as Drop>::drop(&mut *this);
        // after `Drop::drop` is called, no fields of `Self` are moved out yet here, so at the end of this function,
        // all destructors of fields run recursively
    }
}

as you can see DestructuringDrop replaces Drop and all drop glue (for types with a DestructuringDrop implementation, that is). Finally, the above “desugaring” only truly works if there’s a #[feature(unsized_fn_params)] style support to handle ?Sized and furthermore assumes the argument passed by value will not actually move to a different place in memory, so &mut *this can still refer to the dropped value in its original place.

Of course, this Drop in a by-value is only supposed to be meant as a future possibility, however a quite nice one, in my opinion.

Also note that the naming of the type Destructure<T> is a placeholder – with this interpretation of the feature, of course other names might be more fitting. I might even call it ManuallyDrop if we didn’t have that already1.

Footnotes

  1. That being said, since ManuallyDrop also disables the Drop code of a type, as another future possibility, ManuallyDrop<T> could gain the same capabilities of moving fields values by-value like Destructure<T> does above.

    In fact, when first writing this comment I wanted to propose that ManuallyDrop itself fills the role described here – that is until I realized that it’s far too easy to accidentally leave some fields behind in the ManuallyDrop (in particular matching against a pattern that doesn’t bind all fields would do that) even though the intention is just to disable the outermost layer of Drop code, and still take proper care of all the fields without leaking them, too.

Comment on lines +229 to +230
# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
Copy link
Member

@Nadrieril Nadrieril Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the really obvious alternative: "allow a plain let Struct { .. } = foo; without a macro". Specifically, we could allow this in exactly the cases where you propose the macro to work, i.e. when the type is constructible in the current context. I'd like to see why this alternative is not considered viable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is important to syntactically mark the fact that destructure!() is a different operation than this. We talk about this in the guide level explanation. For types implementing Copy, destructure!() moves anyway and doesn't run the original value's destructor at all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah apologies, I skipped over the guide-level explanation because I asumed the technical details would be in the reference-level section.

Here's what I was missing, which imo should be highlighted in the motivation section: let Struct { <something> } = foo does not reliably prevent drop from running, because only a non-Copy binding would force a move out of foo. Hence I seem to understand that the problem statement of this RFC includes "find a reliable way to disable drop".

It seems like allowing let Struct { <something> } = foo is pretty orthogonal in fact: we could allow it and this RFC would still make sense. The argument against is that users might think this disables Drop when it doesn't always. I would really like to see that spelled out in the "Alternatives" section.

Copy link
Author

@jdonszelmann jdonszelmann Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, I'll talk to @WaffleLapkin (when it wakes up :P) and we'll see what modifications to make. I'll let you know when we have so you can take another look if you'd like :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I was missing, which imo should be highlighted in the motivation section: let Struct { <something> } = foo does not reliably prevent drop from running,

I do not understand the use of "reliably" here. This never disables foo's drop because it's one of these situations:

  • Struct implements Drop and <something> causes a move, which is currently error
  • <something> doesn't cause a move, so drops are not affected at all
  • <something> causes a move, but Struct does not implement Drop, which means that only the drop for the moved field is disabled (other fields still dropped)...

Hence I seem to understand that the problem statement of this RFC includes "find a reliable way to disable drop".

Not really? The problem statement of this RFC is very specifically to allow moving fields out of Drop types. Disabling drop is a requirement for that -- you have to disable the drop, because it gets access to the fields and so can't work when a field is moved out.

The rest of the RFC is "how do we make this operation not confusing and least footguny". Simply allowing normal destructuring of types which implement Drop is a footgun, since the syntax doesn't clearly differentiate between disabling drop or copying fields out.

We might want to spell this out more explicitly though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply allowing normal destructuring of types which implement Drop is a footgun, since the syntax doesn't clearly differentiate between disabling drop or copying fields out.

Yes, that's precisely the point I was trying to make! By "not reliable", I mean "allowing the obvious thing is footgunny because nothing would syntactically indicate whether you're moving out or not". Hence why I would like to see this spelled out, because that was not obvious to me on first read of the RFC.

Copy link
Contributor

@oli-obk oli-obk Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be remedied by just turning the hard error into a lint. Then you #[expect] the lint wherever you want the destructuring to happen and subsequently the drop getting avoided. Though that has the downside of adding a drop impl not causing existing destructuring sites to error in other crates

This comment was marked as duplicate.

# Summary
[summary]: #summary

Rust does not allow destructuring types which implement the `Drop` trait. This means that moving data out of such types is hard and error prone. The rationale is that once fields are moved out, the type's `Drop` implementation cannot run, which can be undesired. This RFC proposes to allow destructuring anyway, in certain situations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add more detail here? The reasons for this language quirk are crucial to this RFC, what exactly is it trying to prevent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this example:

let x = X { y: Y }; // X: Drop, Y: !Copy
let y = x.y;
// implicitly this line contains `drop(x)`

The implicit drop(x) would be an error, because x.y is moved out. So the "you can't move fields out of Drop types" is a specialization of "use of partially moved value".

We could say that let y = x.y disables the drop, but that is a footgun, see #3738 (comment).

Do you think we should write this more clearly in the RFC?

Copy link
Member

@Nadrieril Nadrieril Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should write this more clearly in the RFC?

I would quite like that. My mental model makes me expect let y = x.y to disable the drop, and it's not obvious to me that this is a footgun, so I'd appreciate a motivation + example for why we don't allow this.

Copy link

@Aloso Aloso Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the "you can't move fields out of Drop types" is a specialization of "use of partially moved value".

Except it is more strict, because moving fields out of Drop types is forbidden even if you put something back afterwards:

// only allowed if `Foo` does NOT implement Drop
let mut foo = Foo {
    string: "old value".to_string(),
};
drop(foo.string);
foo.string = "new value".to_string();
drop(foo);

EDIT: I realized why this is necessary. If a panic occurred between moving the field out and moving a new value in, the struct would be in an invalid state while unwinding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this example:

let x = X { y: Y }; // X: Drop, Y: !Copy
let y = x.y;
// implicitly this line contains `drop(x)`

The implicit drop(x) would be an error, because x.y is moved out. So the "you can't move fields out of Drop types" is a specialization of "use of partially moved value".

We could say that let y = x.y disables the drop, but that is a footgun, see #3738 (comment).

Do you think we should write this more clearly in the RFC?

Is let X { y } = x is disallowed when X: Drop, Y: !Copy simply because it would be weird and seemingly inconsistent for let X { y } = x to be legal while let y = x.y is illegal? Or is there a deeper reason?

The reason I ask is that, as discussed in this thread, an alternative design would just be to permit let X { y } = x, and have it semantically move all of the fields rather than dropping the original x. That option would still permit let y = x.y to remain illegal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand in what cases ths obvious "just let the user destructure any type" would be a footgun. Could someone provide concrete examples of types that have both a significant Drop impl and public fields where this could cause issues? I've never personally encountered such a case.

Copy link
Member

@programmerjake programmerjake Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a little searching, I found a unit type with a Drop impl, if just destructuring using normal syntax was allowed (turns out it is!), then writing let AudioLock = the_lock; or let AudioLock {} = the_lock; would be a foot-gun because it wouldn't disable Drop because all fields are Copy (vacuously true).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, pattern matching a unit type with a Drop impl is already a footgun

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example! I think there are two footguns at play:

  1. users writing let <pat> = <drop_val>; and expecting that to disable Drop;
  2. users holding a value with an important Drop impl and accidentally disabling Drop by pattern-matching on it.

My understanding of the RFC is that is argues against "just let destructuring work" because that would create footgun 2. I am still skeptical that this is a real issue and open to seeing examples of that. What you just showed is footgun 1, which already exists and arguably would be made worse if we "just let destructuring work"?

Copy link
Member

@workingjubilee workingjubilee Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a real issue. I can't show you the code that causes the problem because it doesn't exist yet, as we don't permit it in the actual common cases. But there are cases where something has a meaningful Drop impl but a certain function is intentionally supposed to disable that Drop, and so it is problematic if the specially marked operation is not, er, specially marked.

Allocated container types, for instance, where leaking is kind of a big deal, but you still want to support disassembling them... like the example?

Comment on lines +189 to +190
# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation
Copy link
Member

@Nadrieril Nadrieril Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something important is missing: this is adding a new capability to safe code. Unsafe code writers could until today rely on the fact that safe code cannot take ownership of the value of fields in a struct with a Drop impl. As such, this RFC is technically a breaking change with soundness implications.

It's probably a rare case that a struct would have important drop-related invariants and be publicly constructible, so this may be acceptable, but this should be noted nonetheless.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe reliance on &mut _ not permitting ownership stealing is unsound in presence of take_mut, which has proven to be far too useful and natural to disallow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I'm not sure what this allows in addition to what take_mut already allows (which I guess technically isn't blessed, but I would not assume it's cursed either).

In addition to the fact that if all your fields are public, doing dangerous unsafe in Drop sounds extremely sketchy. In basically all scenarios Drop types have all fields private.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can only take_mut() if you’re able to produce a replacement value. If there’s no way to get a replacement value for the fields, I think you can’t “steal” a field using take_mut(). E.g.

// crate foo {
pub struct Singleton(());

impl Singleton {
    pub fn claim() -> Self {
        static ONCE: AtomicBool = AtomicBool::new(true);
        if ONCE.swap(false, Relaxed) { Self(()) }
        else { panic!() }
    }
}

pub struct Complex {
    pub field: Singleton,
}

impl Complex {
    pub fn claim() -> Self {
        Self { field: Singleton::claim() }
    }
}

impl Drop for Complex { ... }

// crate bar
let mut complex = Complex::claim();
// not possible with `take_mut`:
// no way to provide replacement value
destruct!(Complex { field } = complex);

I’m not sure how important that is though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can though!

take_mut(&mut complex.field, |f: Singleton | {
    // temporary ownership
    identity(f)
});

take_mut(&mut complex.field, |f: Singleton | {
    drop(f);
    panic!() // terminate the program
});

So at most this breaks something like

if a Drop type T with all public fields is passed to a closure, one of the following happens:

  • T::drop is called and all its fields are dropped (modulo panic in drop)
  • T is forgotten and none of its fields are dropped
  • T's fields are dropped (/owned) and the program terminates before reaching the end of the closure

And while I assume it's possible to craft a program that requires exactly this assumption, I don't think it's reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I'm not sure what this allows in addition to what take_mut already allows

Dropping the fields in a different order I think, though maybe you can do that with nested take_muts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested take_mut do allow you to drop fields in whichever order you'd like.

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Dec 8, 2024

@steffahn my concern is that implementing and defining Destructure is a lot more complex, it's quite a bit of magic. At the same time I see some of the appeal. I think we should at least add your proposal as an alternative.

Upd: see 2a7cfb0

@Aloso
Copy link

Aloso commented Dec 10, 2024

I don't like the name destructure!(), because the name does not convey the purpose (preventing Drop from being called). A better name would be forget!().


An alternative is to just allow moving fields out of ManuallyDrop<T>. I think this would be much more elegant, and doesn't require macro magic, or any other new syntax. It is also more versatile:

let mut foo = ManuallyDrop::new(foo);

// you can move fields by destructuring:
let Foo { some_field, .. } = *foo;

// or, you can move fields by assigning them:
let some_field = foo.some_field;

// or, you can move fields by passing them to a function:
do_something(foo.some_field)

Box allows moving out fields as well, so adding the same feature to ManuallyDrop seems reasonable.

@WaffleLapkin
Copy link
Member

@Aloso forget! does not convey the meaning either IMO. the purpose is to get fields out, which requires not running drop...


As for moving fields out of ManuallyDrop... This has multiple issues.

First of all, it's a bit of a footgun, as you need to not forget to move out all fields with significant drops. Second of all, you kind-of still need to have a macro, because we still want to check that all fields are accessible from current scope.

You can read more details about a similar approach here: https://github.com/jdonszelmann/rfcs/blob/drop-type-destructuring/text/3738-drop-type-destructuring.md#macro-expression-and-magic-type


When implementing this change as a macro expanding to a statement, we do not think there are many drawbacks. However, in the coming section with alternatives we discuss another possible expansion to a pattern which may have a few advantages, but also has many drawbacks which we discuss there.

# Rationale and alternatives

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative that I haven't seen mentioned is having a macro that behaves like a match. Something like:

match_and_forget!(foo { Foo { .. } => something)

The main benefit, is that it would be straightforward to get that to work for enums:

match_and_forget!( thing {
    Variant1(x) => something(x),
    Variant2{ y, z } => something_else(y, z),
});

And it also would allow using it as an expression.

The downside is that it is a lot more verbose for simply binding fields to variables for a struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is basically just equivalent to the destructure! as a pattern, but more verbose for simple cases. It also suffers from similar inability to do nested matches.

Also:

The one scenario we could imagine is that might be useful for destructure!() on enums, but we also think that even using destructure!() on enums will be very uncommon (as types implementing Drop are almost always structs) and might not outweigh the increased implementation complexity and the fact that we need to communicate this one level matching rule to users.

@ahicks92
Copy link

I have rarely needed this and I also sort of blinked at the name. I don't know that we'll find a good one but I will propose steal! if we are bikeshedding. Or maybe destructure_drop!, since destructuring assignment is already a concept.

I'm sure Manually Drop is special in some way that I don't know, but making it more special probably isn't the way. I can implement my own ManuallyDrop today, minus perhaps aliasing concerns: forget(self.value.take()) where value is Option<T>.

TLDR of the below: sometimes you need this, but it replaces the problem of not having it with the problem of having it. I support it because I don't believe in hamstringing capabilities and have needed it, but fear the day someone inexperienced uses it and I get paged at 2 AM because prod is down.

There might be a good case for saying "don't use this unless you are sure you need it" in learning materials. Consider:

  • Some crate publishes some sort of guard with public fields (say, internal to a monorepo. There aren't good reasons IMO to put such a thing on crates.io, but for better or worse crate is also synonymous with something like "build unit". I have committed such sins myself).
  • Someone new to Rust tries to move out of this guard, and gets an error.
  • But destructure! saves the day.
  • Next month someone else gets to find out that actually no that broke in prod.

Given that Drop impls are often RAII-like things, and those things are often concurrency etc. that'd almost certainly not show up in unit tests for example. I'm not sure how one would spot such mistakes other than to say every use is suspect. That's not even talking about leaks yet. Leaks are a theoretical concern in the sense that they are safe, but not in the sense that a well-meaning dev who doesn't know Rust well may fail to understand the implications and put this somewhere important.

I have seen even senior-level devs, all the way up to someone who is alone worth my entire team by themself, end up getting stuck in the position of trying to use Rust professionally and having to make compromises on a deadline while they learn. Today most of such compromises are relatively harmless: you might overuse Option or maybe you do Arc in async code or whatever. It produces meh but working code. This however kind of doesn't maintain that property. It gets rid of the compiler screaming in an insidious way.

@tmccombs
Copy link

Related to @ahicks92 's concerns, I wonder if in addition to having all fields public, this should also require that the struct implements a, possibly unsafe, marker trait to opt in to this.

In fact, that may be necessary for backwards compatibility, since there could be existing structs where all fields are public that have a Drop implementation that it relies on being called.

@ahicks92
Copy link

I don't see a backward compatibility concern because the macro itself is new. As long as you can't have code which would change behavior, the macro doesn't cause a problem. A marker trait might be interesting but broadly speaking I can't think of an example of something on crates.io which has public fields because really, mostly, Rust devs don't do that as a whole. It is very easy to opt out: add a PhantomData<()>. There are already lots of ways adding a private field to a struct whose fields were previously all public breaks API compatibility.

This is the kind of thing where I'd use it and I don't see a problem with it other than it being a weird corner of the language, but I've had to teach others and watch qualified people struggle in ways where this could be the tool reached for. Semantically it's fine, in so far as the question being whether or not we want this. I will leave that to "official" people in other words; as far as that aspect I've needed it on occasion but all those times have been memorable and annoying.

I also have trouble thinking of a case where if you intentionally use it you break something. It feels like finding a way for this to be a footgun for people who know why it exists is really hard and you'd have to do it on purpose. I've seen concerns around unsafe code raised, but today you have to deal with mem::forget already and figuring out some way to write code which both makes all fields public in a public API and also relies on drop order or something for safety seems like quite the tall order.

@kornelski
Copy link
Contributor

kornelski commented Dec 19, 2024

Perhaps it'd be less magic from the language perspective if destructuring a type with Drop was a deny-by-default error that can be disabled with allow/expect? After all, forgetting is safe, so the concern here is only checking with the user did you really mean to forget?, and that's what lints do.

 fn example(x: Foo) {
+    #[allow(forget_drop)]
     let Foo { a } = x;
 }

@GoldsteinE
Copy link

GoldsteinE commented Dec 21, 2024

I think with the just-allow-let approach (or, to a lesser degree, #[allow(forget_drop)] approach) it’s much less obvious which usage disables Drop.

These usages might be expected to disable Drop, but they can never do it, that would be a breaking change:

let Foo { .. } = x;
let Foo { ref y } = x;

How about this one?

let Foo { y } = x;

This can only disable Drop if type of y is not Copy — otherwise it’s currently valid (it copies the field and then runs the destructor), so we can’t change it to disabling Drop (or maybe we can over an edition, but that would be extremely confusing and error-prone).

In other words, allowing plain let (or even let with #[allow]) to disable destructor means that we can’t locally determine whether this particular let does that. An explicit operation to disable Drop is much more readable IMHO.

@Nadrieril
Copy link
Member

With just-allow-let + the forget_drop lint, I'd write this:

#[expect(forget_drop)]
let Foo { y } = x;

which would error if I am not in fact disabling Drop there.

@GoldsteinE
Copy link

GoldsteinE commented Dec 21, 2024

That’s fair, and that’s why I said that forget_drop suffers from this problem to a lesser degree. Maybe we could even additionally lint against allow(forget_drop), since it is a footgun.

It still means there’s no way to disable drop by destructuring when all the fields are Copy though.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be useful!

As for the "lint" proposals: In general I don't like the "clippy strategy" of "make it a lint, then allow lints" for a simple and somewhat painful reason that is often elided in these discussions of theoretical implementations: lints are often not required to be correct, nor in general held to the same standard. Other things that are "technically just a lint" nonetheless have language-level hard enforcement, like unsafe. I see this as just another example of such. While it is true you can't expect Drops to always be run in Rust, we've already covered why it can be a serious problem to "accidentally" not run a Drop. Most of the ways of silencing a Drop require a significant and visible opt-in.

Comment on lines +152 to +155
fn example(x: Foo) {
// `a` is moved out of `x`, Drop never runs for `x`
destructure!(let Foo { a } = x);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've definitely written code that would have been much-improved... or at least written faster... by this being available! Thank you!

# Summary
[summary]: #summary

Rust does not allow destructuring types which implement the `Drop` trait. This means that moving data out of such types is hard and error prone. The rationale is that once fields are moved out, the type's `Drop` implementation cannot run, which can be undesired. This RFC proposes to allow destructuring anyway, in certain situations.
Copy link
Member

@workingjubilee workingjubilee Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a real issue. I can't show you the code that causes the problem because it doesn't exist yet, as we don't permit it in the actual common cases. But there are cases where something has a meaningful Drop impl but a certain function is intentionally supposed to disable that Drop, and so it is problematic if the specially marked operation is not, er, specially marked.

Allocated container types, for instance, where leaking is kind of a big deal, but you still want to support disassembling them... like the example?

Comment on lines +24 to +33
pub fn into_parts(self) -> (W, Result<Vec<u8>, WriterPanicked>) {
let mut this = ManuallyDrop::new(self);
let buf = mem::take(&mut this.buf);
let buf = if !this.panicked { Ok(buf) } else { Err(WriterPanicked { buf }) };

// SAFETY: double-drops are prevented by putting `this` in a ManuallyDrop that is never dropped
let inner = unsafe { ptr::read(&this.inner) };

(inner, buf)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A safer workaround is to use Option, Option::take, and decomposing a struct further into components, so that you can "hide" the pieces that need to run the Drop impl from a destructuring pattern. But "Option + decomposition just for the sake of workarounds" is also a really bad negative pattern if you didn't actually need to decompose anything or make the value, er, optional, since it makes the code open to incorrectness, as you may actually want to always have the value there and may want to always have certain values together... except in the code that wants to perform this trick.

I can't produce a good example right now, unfortunately, because I lost the logs of the conversation where I explained the (goofy) solution to this question in one case.

@workingjubilee
Copy link
Member

@ahicks92 I think your concern is valid.

However, I've seen people give up on using Rust entirely and decide to do all their major work in C, for projects that would benefit from memory safety, because they happened to not be able to figure out the bizarre workarounds that Rust requires in these cases. So, certainly, teaching people how to do something correctly is desirable, but I think it's important to not overstate the cost of this next to "do nothing", if the "do nothing" is "people do in fact do nothing, and the current state of affairs is abominable". If you evaluate this entirely in the context of a workplace that has already chosen Rust, sure, but other people are out there looking at the complications of this case... yes, specifically this case... and walking away from the language because of it.

@WaffleLapkin
Copy link
Member

@kornelski (re this), I don't think this works as a lint. destructure! is fundamentally different from normal destructuring (sort of). Since it stops drop from happening.

Also note that there are cases where you can "destructure" a Drop type today -- specifically if all the fields you access are Copy. It would be somewhat confusing if turning off or not drop dependent on Copy-ness of the moved fields, even with a lint. A lint would just be a crutch, I really do believe we need a separate operation here. (although I've also thought of the lint approach at some point, it makes sense, but I don't think it's a good idea in the end)

@ahicks92
Copy link

@workingjubilee
I adopted Rust for a few reasons many of which are personal interest. I brought Rust into work however for reliability. From my perspective reliability is the first thing. We were able to write services that worked almost the first time and ran for months even on less-than-ideal sets of dependencies. I care more about preserving this property than I do encouraging antipatterns for the sake of adoption. Leaks etc. aren't unsafe, but they are unreliable. Saying this now, I almost feel like Rust discussions should almost adopt unreliable as a concrete term.

There is a C mindset. I am skeptical that solving this minor problem would get someone in a C mindset to adopt Rust. Maybe though the people in charge have a different opinion on the important trade-off. In any case we aren't debating having it, we're debating (over)teaching it. Changing how it's taught isn't a breaking change, let us say. I do find your counterpoint valid though. I just don't agree with it.

@WaffleLapkin
Copy link
Member

@ahicks92 I don't feel like this RFC hinders Rust's reliability. The only potential footgun with destructure! is not running a type's drop. But this can already happen in a number of ways — Box::leak, mem::forget, ManuallyDrop, Rc cycles, double-panics... Adding one more way to forget a value doesn't feel important — if you can catch all existing things in review, you can probably catch this one too.

Not only that, this can only realistically happen in the same module as the type definition (Drop types almost always have private fields), which means the programmer/reviewer are more aware of it, probably.

If you are still afraid this might negatively affect your codebase, I'd recommend clippy, you can configure it to warn against certain macros: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros.

@ahicks92
Copy link

@WaffleLapkin
I have only so far made a point about misuse by new Rust coders, and that I'm concerned about teaching it. I was going to say that almost the first thing I said is that I support adding it but reading back, while this is factually true, I see how it's unclear and I should probably have pulled it out more obviously. Apologies for the lack of clarity. If I was going to phrase my stance in a sentence: I support adding it but view it more like an intrinsic in the C sense, such that it is "special" and I would want people new to Rust to understand that.

I'm currently between jobs and also my last team was great and I'd have been able to say "no bad" and been listened to. Indeed the reason I draw from my experience is because it was interesting to see an otherwise experienced native-level team (in many cases far my senior) struggle, and just how they did so. That said I did not know clippy could flag macros, it is good to know that this arising won't mean that I am all alone manually code reviewing, and I'm sure I will find some other use for that eventually in any case. For something like this in so far as it would affect me personally being able to mitigate it is indeed the important part.

Copy link
Contributor

@kpreid kpreid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see this RFC or something like it happen; elegant, safe, Option-free “drop guard disarming” (and hopefully also move-out-of-Drop; see my comment) would make Rust better at letting people use the capabilities of Rust’s ownership model.

}
```

Here, if we were to move fields out of `Bar` from left to right, we might move `a`, then check whether `b` matches `1`, and if it doesn't we already moved `a` out.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure this disadvantage is real. Why does the move have to happen before fully matching? As far as I know, the existing semantics of patterns are compatible with the steps being:

  1. Test the scrutinee against the pattern.
  2. Execute match guard expressions (using temporary shared borrows).
  3. If the pattern and the guard succeed, perform the moves and borrows specified by the pattern.

Copy link
Member

@Nadrieril Nadrieril Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed how rust works today and while it's not yet guaranteed behavior I expect this is what we'll guarantee once we get around to it.

}
```

The upside of this is that `Destructure` type can be used as input into functions which are expected to not drop `T`, such as potential `DropOwned`:
Copy link
Contributor

@kpreid kpreid Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A future version of Rust that had &move references (a kind of pointer which transfers ownership of the value, but doesn't own the memory it points to) could simply use the statement or pattern destructure!:

impl DropOwned for X {
    fn drop(self: &move Self) {
        destructure!(let Self(inner) = *self);
        drop(inner);
    }
}

This would have the advantage, compared to Destructure<Self>, of not forcing a move of the X value itself (only the fields that the author chooses to move out of it), and thus doesn’t change or magically fudge the ABI — in particular, the above DropOwned::drop() does exactly what drop_in_place() does (drop the entire value), whether or not the author of the code does so explicitly.

This would also align neatly with “pinned drop” (executing drop glue on a value that is statically known to be pinned), by offering a Pin<&move Self> instead. At that point, we have a drop with all the properties missing from today's Drop::drop(): it can move fields out, and it is unable to accidentally break pinning.

Copy link
Member

@workingjubilee workingjubilee Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..."a kind of pointer which transfers ownership of the value, but doesn't own the memory it points to"? ...que? That feels like it is not quite the thing you are trying to say.

Copy link
Contributor

@kpreid kpreid Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what error you intend to point out. My understanding of the &move idea (which I could not find an existing writeup of to link to, or I would have) is that:

  • like Box<T>, a T can be moved out of &move T,
  • like Box<T>, if the T is not moved out, then it is dropped when &move T is dropped,
  • unlike Box<T>, no heap allocation is freed (because the allocation might belong to e.g. a parent stack frame, or some other data structure).

Thus, &move T owns the value but not the allocation. Does that clarify?

…that said, I just realized that this is incompatible with Pin without also having unleakable/linear types (you mustn't deallocate a pinned value without moving it). So, my last paragraph was a bit incomplete.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my go-to ~writeup on the topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-drop Proposals relating to the Drop trait or drop semantics A-patterns Pattern matching related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.