Destructuring Droppable structs

language design
1

Previous topics:

Proposal

Turn error E0509 into a lint, warning by default:

  • This still allows users to catch unintended destructuring which would bypass Drop.
  • While allowing users to override the compiler, when desired.

This differs from the 2019 proposal in that instead of a custom attribute and a lint, this proposal contends that just a lint is enough, since lints can be tuned. The 2019 proposal did list this possibility in its conclusion.

Motivation

The following code, while seemingly reasonable, is rejected:

struct Dummy {
    foo: i32,
    bar: String,
}

impl Drop for Dummy {
    fn drop(&mut self) {
        println!("Dropping Dummy {{ foo: {}, bar: {} }}", self.foo, self.bar);
    }
}

fn main() {
    let dummy = Dummy { foo: 2, bar: String::from("3") };
    let Dummy { foo, bar } = dummy;
    
    println!("{foo} + {bar} = 5");
}

Currently leads to:

error[E0509]: cannot move out of type `Dummy`, which implements the `Drop` trait
  --> src/main.rs:14:30
   |
14 |     let Dummy { foo, bar } = dummy;
   |                      ---     ^^^^^ cannot move out of here
   |                      |
   |                      data moved here
   |                      move occurs because `bar` has type `String`, which does not implement the `Copy` trait
   |
help: consider borrowing the pattern binding
   |
14 |     let Dummy { foo, ref bar } = dummy;
   |                      +++

For more information about this error, try `rustc --explain E0509`.
error: could not compile `playground` (bin "playground") due to 1 previous error

I do think warning the user that they may accidentally be skipping the Drop implementation of the type is a good idea, they may not have intended to. I do think that making this lint into a hard error is taking things one step too far.

The Rationale section of the 2019 proposal already offers a comprehensive list of reasons for doing this.

Q & A

But invoking drop on the instance would be UB!

Why yes, obviously. Hence the compiler wouldn't.

If an instance is moved then drop isn't invoked on it. If a destructuring is partial, then drop is only invoked on the fields not matched.

The new "move-destructure-for-droppable-type" capability would just follow those two existing rules.

But this allows users to bypass drop!

Users can already bypass drop, and do so routinely. The very forget function allows a user to bypass drop, and it's otherwise possible to leak types, etc..

Should it be restricted to the crate or module where drop is defined?

There'd be no point, really.

Destructuring is already restricted via visibility rules: only fields that are accessible via . are accessible via destructuring.

Users can already, painstakingly, swap out visible fields then forget the instance. Destructuring would not give them any more power, it'd just make the code easier to read.

The warning would remind users that bypassing Drop may lead to leaks, and from then on it's up to them.

6 Likes
2

It's currently not possible to bypass Drop for a type and drop any of it's private fields. So I think this form should require all fields are present (at least initially), and shouldn't work if you can't list all fields (some are private or type is non_exhaustive).

For example, this should not work. Even if a type has no fields.

let Foo { .. } = bar;

My worry is that some unsafe code may temporarily break safety invariants, which are fixed up in Drop before fields are dropped. Such types would become unsound if anyone could drop fields while skipping the Drop.

6 Likes
3

I have an RFC posted for exactly this functionality, though it takes a different approach: Move out of deref for `ManuallyDrop` by Jules-Bertholet · Pull Request #3466 · rust-lang/rfcs · GitHub

1 Like
4

As discussed in the Internals thread for this RFC, the issue with "just don't call Drop upon partial move" is that whether an assignment is a partial move or not depends on whether the field type is Copy. And that can change in any minor version, and can also depend on lifetimes, which aren't allowed to influence monomorphization:

#[derive(Clone)]
struct Foo<'a>(&'a ());

impl Copy for Foo<'static> {}

struct Bar<'a>(Foo<'a>);

impl<'a> Drop for Bar<'a> {
    fn drop(&mut self) {
        println!("goodbye world");
    }
}

fn do_thing<'a>(bar: Bar<'a>) {
    drop(bar.0); // This is a move only when `'a` is not `'static`
}

(interestingly, it seems the compiler has a bug here: Implementing `Copy` can be a breaking change · Issue #126179 · rust-lang/rust · GitHub)

3 Likes
5

TIL you can destructure a struct with private fields

playground example

3 Likes
6

Thanks for picking this up!

7

How would such code handle using std::mem::take or std::mem::replace on public fields to swap them out, then forgetting the value?

Those are all safe operations, so unsafe code should be sound in their presence (or is broken).

I'm puzzled by the snippet: isn't move-vs-copy dependent on the field type (&'a (), always Copy) instead of the struct type? (Or did you mean to take Bar as an argument?)

If the copyability of Foo influences things, then I'd expect generics to require that Foo be constrained by Copy for copy to take place, so that in your example drop(foo.0) is always a move regardless of whether 'a is 'static because there's no where Foo<'a>: Copy bound.

In the into_bar function, you want self, not foo, as the name of the argument to ManuallyDrop.

I would favor a lint, instead of ManuallyDrop, due to the extra flexibility that white-listing a lint allows. Specifically, it'd be possible to white-list at module level, or function level, instead of wrapping every single instance in ManuallyDrop which is quite heavy-handed. But that's merely syntax bikeshedding, in a sense, and I'd be quite happy with your RFC as is.

I am more concerned by the examples brought by @fee1-dead and @ijackson (Hyrum's law...), though the fact that partial-borrow and replace-with are incompatible seem to require arbitrage from the language team already: clearly at least one of them makes an assumption it should not. In particular, I'm wondering how their code expects to deal with code taking/replacing the public fields then forgetting the value -- all safe operations.

1 Like
8

Yes, exactly, fixed now. Sorry

9

Hence why I said private fields. But types may have a mix of public and private fields.

Let's take a simple (if contrived) example:

struct PrintOnDrop(String);

struct Foo {
    pub name: String,
    data: PrintOnDrop
}

impl Drop for PrintOnDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

impl Drop for Foo {
    fn drop(&mut self) {
        self.data.0.clear();
        self.data.0.clone_from(&self.name);
    }
}

Let's say you moved name out of Foo, it's public after all. Then you couldn't run Drop for Foo. So if foo.data.0 contained invalid utf-8 bytes, there would be UB if Drop for PrintOnDrop. However, right now this api should be sound because, it's not possible to run Drop for PrintOnDrop without running Drop for Foo, which fixes up foo.data

3 Likes
10

I see the problem now. So the core issue is not about not running any implementation of Drop, but specifically about running Drop for fields without running Drop for the outer struct.

It does feel like quite a contrived situation. I have seen fix-ups on the Drop, with the pre-pooping your pants idiom, but those are specifically about keeping things safe without Drop.

I guess it means it's worth checking exactly how many occurrences of this strange pattern exists in the wider ecosystem, and whether they're worth it. It's hopefully a sufficiently exotic pattern that it won't prove an impediment.

11

why not just prevent destructuring structs that:

  1. have private fields (or are #[non_exhaustive])
  2. implement Drop

this should cover up this corner case, and imo destructuring is mostly useful for structs that are entirly public anyways.

4 Likes
12

Do you really mean private or do you rather mean non-accessible?

Private seems like a non-starter, since it seems perfectly reasonable for a developer of a crate to wish to pattern-match on their own structs. After all, a motivating usecase is specifically for such developers to be able to provide conversion/finalization methods without having to resort to unsafe code with ManuallyDrop, or to Option.

Non-accessible thus seems more appealing... but when a major usecase is the above-mentioned users, and they can still shoot themselves in the foot, it's unclear whether the restriction is worth it.

Perhaps, then, the better solution is just to allow one to opt-out of move-destructuring at the struct level (but still allow ref-destructuring). Then all users are equally safeguarded.

But whether such an opt-out is worth it is very much in question for me. So far we only have 1 unpublished crate and 1 rarely used (and potentially unsound) crates as examples of code that use this strange practice, and both are essentially "abusing" it in order to emulate partial borrows.

13

I think the simplest version here would be that only a method attached to an inherent impl (so, only local types) could be allowed to do this. That still seems like a nice win, and it seems okay for the local crate to decide that they might provide an alternative way of "dropping" the value. Have definitely run into the need for this before.

2 Likes
14

This now reminds me of linear types (here, with a non-pub alternative "sink" mechanism). What's the interaction of this with linear type proposals (I'm not sure of what progress/feasibility they have)? Maybe they would cover this feature in some way?

15

Very pragmatic, I like it.

I think it'd still make sense to have the warning by default -- given the consequences of accidentally not dropping -- so the end result for move-destructuring in the presence of a Drop implementation would be:

  • Outside of inherent impl: hard error.
  • Inside inherent impl: lint, warn by default.
16

FWIW, it's already possible to define a helper macro for this task:

struct Point { x: i32, y: i32 }

impl Drop for Point {
    fn drop(&mut self) {
        unreachable!()
    }
}

let p = Point { x: 42, y: 27 };
destructuring_drop! {
    let Point {
        x: value,
        y: _,
    } = p;
}
assert_eq!(dbg!(value), 42);

By having the macro expand to the following unsafe code:

let (value, _) = match *ManuallyDrop::new(p) {
    Point { ref x, ref y } => unsafe {
        (
            <*const _>::read(x),
            <*const _>::read(y),
        )
    },
};

Having the hard error become a lint (I don't care about its default polarity) seems like a less cumbersome and with-better-diagnostics way to achieve the same :slightly_smiling_face:

5 Likes
17

you can already pattern match on a reference to a droppable struct, so i'm not entirly sure what you mean.

struct Dummy {
    foo: i32,
    bar: String,
}

impl Drop for Dummy {
    fn drop(&mut self) {
        println!("Dropping Dummy {{ foo: {}, bar: {} }}", self.foo, self.bar);
    }
}

fn main() {
    let dummy = Dummy { foo: 2, bar: String::from("3") };
    let Dummy { ref foo, ref bar } = &dummy;
    println!("{foo} + {bar} = 5");
}
18

Just a reminder that whatever changes should obviously NOT change this.

19

(NOT A CONTRIBUTION)

I wrote about how I thought this rule of Rust should change in this post about the design of ownership: Ownership. It contais a pretty compelling motivating example that I got from David Tolnay from serde_json: the Value type is meant to be destructured, but it also wants to have a custom destructor.

This would fail to solve the example from serde_json, which is a public type meant to support destructuring as part of its public API. In my opinion, it does not make sense that drop prevents destructuring; if you want to make it illegal to destructure a type outside of this module, you should use privacy.

5 Likes
20

While I would tend to agree with you, unfortunately, if you read the previous attempts linked at the top, and the discussion here, you'll note that the previous attempts failed because of concerns for:

  1. API: a type should only be destructurable if the API designer wishes it to be, as the API designer may be rely on Drop.
  2. Safety: The reliance on Drop preventing destructuring may be the key that upholds safety invariants.

There's value in offering opt-in destructuring -- under full control of the API designer -- while privacy is an opt-out.

It's also not clear to me how privacy would work with enum: should an enum always be destructurable? You can't have a private variant, or private field, in an enum... so there's a loss of granularity there.

Another possibility would be an opt-in attribute, but it has the disadvantage (in its simplest form) of not giving control over who can destructure. Perhaps the API designer would want the liberty to destructure themselves without granting this liberty to the API users, so they can enforce that some things happen on destructuring/drop.

It may be possible to compound a simple opt-in attribute with privacy, but this still runs into the issue of the lack of granularity with enum.

@djc's simple solution is the opposite. It only grants the API designer the ability to destructure an no one else. This is opt-in, and allow the designer to enforce side-effects on destructuring.

It has the advantage of working with both enum and struct, but the disadvantage of not allowing the users to destructure.

Perhaps it should be combined with a simple attribute allowing full destructuring powers to the users for completeness.