pre-RFC: simple/safe pattern destruction of `Drop` types


#1
  • Feature Name: no_drop_destruction

UPDATE: disallowed _ binding pattern (see below for reasons)
The initial version did allow _ bindings and had a part which was slightly ambiguous over wether or not the behavior of _ is changed. While cleaning this up I became aware that it is better to not enable pattern matching for destructive patterns containing _.

Summary

Allow, but lint, using destruction patterns with Drop types in some cases.

In code:

#[allow(no_drop_destruction)]
let ImplementsDrop { some, fields } = implements_drop;

Motivation

There are many places where it can be use full to pattern destruct a type implementing Drop (without it calling drop) to e.g. implement type conversion.

A pretty common use-case are wrapper types which implement Drop and have a into_inner method. Currently this can be implemented by either:

  1. Put the inner type into a Option even if the only place where the option can become None is the into_inner function which then drops self.
    • But this requires calling unwrap() at all other places, which is a mess.
  2. Use unsafe code (awful but should work) :
    1. Make sure the struct is on the stack
    2. Then use something like let Type { ref mut field, ...} = &mut self to get &mut's to all fields.
    3. use std::ptr::read to unsafely move out all fields
    4. use std::mem::forget (which will leave just a bit a now irrelevant bytes on the stack).
    5. return from the into_inner function to “clean up” the stack

A example for a type affected by this is std::io::BufWriter.

This also doesn’t only affect wrapper types (which are pretty common, e.g. see combinators of iterators/futures) but also some other cases where you have a function which consumes self but doesn’t “end it” but instead convert it to another type.

Guide-level explanation

Types implementing Drop normally do not allow moving out any values from them, as they need to run some logic on when they leave the scope, which is not possible as it could easily lead to leaked resources and similar problems.

Still sometimes you might want to decompose a type implementing Drop into all it’s parts to then recompose it into a different type. You can do so by disabling the no_drop_destruction line.

For example if you have some wrapper type:

struct MyWrapper<A> {
    inner: A,
    field2: &'static str
}

impl<A> MyWrapper<A> {
    fn into_inner(self) -> A {
        #[allow(no_drop_destruction)]
        let MyWrapper { inner, field2 } = self;
        // We use `drop` for clarity and to not have a unused variable.
        drop(field2);
        inner
    }
}

impl<A> Drop for MyWrapper<A> {
    fn drop(&mut self) { println!("it dropped"); }
}

Note that if you pattern matching to destruct a Drop types you do have the responsibility all fields appropriately, even if they are pointers. This is also the reason why only destruction patterns work which map all fields.

For example you can use:

  • let MyWrapper { inner, field2 } = wrapper;
  • match wrapper { MyWrapper { inner, field2 } => {...} }

if you annotate the pattern with #[allow(no_drop_destruction)].

But you can not use this with following pattern:

  • .. (e.g. let MyWrapper { inner, .. } = wrapper;)
  • _ (e.g. let MyWrapper { inner, field2:_ } = wrapper;)

The reason for this is at it would allow partial movement on Drop types. The reason for it is that in rust destructive patterns are basically just syntax sugar for moving out all fields on-by-one. But assigning to _ doesn’t move anything. E.g. let (a, _) = c; can be roughly seen as let a = c.0; let _ = c.1; which doesn’t move the second value out of c so you still can access c.1 after it.

But when doing a pattern destruction of a type implementing Drop the you have the responsibility to handle all fields. Additionally if .. would be allowed it would be just to easy to add some leaks by adding a field if it would be allowed to use .. to discard all remaining fields.

Note that this only applies to the type(s) which implement Drop so if you have a pattern containing patterns the non drop types in it still can use those, e.g. following is fine:

  • let StructWhichIsNotDrop { wrapper: MyWrapper { inner, field2:_f }, .. } = foobar
  • let MyWrapper { inner: Some(_), field2:_f } = wrapper;

So using you can only use pattern destruction on types implementing Drop if you move each field of the drop type into a variable (_ isn’t a variable)

Reference-level explanation

Allowing destruction patterns for types impl. Drop

  1. Allow destructive patterns which cover all fields for types implementing Drop.

    • in more complex patterns patterns consisting of multiple “sub-patterns” only the fields belonging to types implementing Drop need to be covered
  2. A pattern covers all fields if each field is has is assigned to a variable.

    • _ is not a variable.
    • .. in a struct doesn’t assign the fields behind it.
  3. Matching a whole type with _/.. which implements Drop is still allowed

    • This is already allowed ( e.g. let _ = MyWrapper { inner })
    • Matching the whole type is more then matching all fields of it, so using MyWrapper {..} as a pattern is still not allowed if MyWrapper implements Drop
  4. If a pattern is used on a type implementing Drop and the patterns moves out values from that type, then the types Drop::drop implementation will not be run

    • Naturally you can only use patterns which cover all fields like mentioned above

For example:

  • Type(_f)
  • Type { field: _ f}
  • Enum::Variant(_f)
  • Enum::Variant { field:_f}

Due to 3. it is also possible to have a _ match arm, e.g.:

match enum_with_drop {
    Enum::Variant(_f) => { unimplemented!(); },
    _ => { unimplemented!(); }
}

Which means that following would be possible in the same way it is in current rust for non drop types:

match enum_with_drop {
    Enum::Variant(f) => { return Some(f); },
    _ => {}
}
drop(enum_with_drop);
return None;

This explicitly does not allow moving out single fields of a struct, it’s strictly limited to patterns which cover all fields, even through rust internally might just move fields out one-by-one.

Restricting usage of new destructive Drop patterns (lint)

When a struct implementing Drop is pattern destructed the code doing so has the responsibility to handle all fields, which can include handling some pointers e.g. to clear up some resources no longer needed. As such adding a field to the struct should make all destructive patterns on it fail to compile, which is why .. in struct patterns is excluded.

Additionally there should be some form of “opt-in” as people should preferably be aware that they are destructing a drop type. The simplest way to implement this is to have a lint which by default produces an error if a Drop implementing type is destructed. This allows the usage of #[allow(no_drop_destruction)] to “opt-in” for a specific expression and has the benefit of not needing any new attributes, and being compatible with all lint related tooling.

Edge Cases

There is a number of edge-cases mainly wrt. using a lint as a “opt-in” mechanism. Which is that if you attribute a expression with #[allow(no_drop_destruction)] it allows it for the whole expression even if it contains multiple drop destruction patterns, which I believe is fine (see below), but for the case it get’s replaced with a new attribute here are some additional parts:

  1. using a destruction pattern on a drop types should be fine independent of where the pattern appear, it can be in another pattern even if it is another drop destruction pattern.
  2. in a destruction pattern, you should be able to further match/destruct on the fields of the struct/(variants of the enum/ items of the tuple struct) like normally, just hiding some fields of a drop impl. struct is not allowed
  3. For a pattern containing potentially multiple sub-patterns of drop implementing structs a single annotations should be enough.

Drawbacks

(this section needs some updates wrt. the updates wrt. _ and the added open questions)

I do not believe that this RFC has any drawbacks compared to the current status because of the reasons listed next. Still there are some drawbacks to using lint’s as a “opt-in” mechanism which are listed further below.

The original restriction on moving values out for drop types was based on:

  1. The fact that it’s really easy to mess it up wrt. to moving out fields and partial destruction
    • which doesn’t apply as only full pattern based destruction is allowed).
  2. It is possible to circumvent drop being run without unsafe
    • Which rust learned is always possible, i.e. mem::forget is stable + safe by now
    • Types still can “protect” them self as against no_drop_destruction as it requires all fields to be visible for the calling code and most Drop types have at last some private fields, which means that only code in the modules (and sub-modules) of the type impl. Drop can use the destruction patterns. (Again another reason to not allow the .. pattern.

Drawbacks of using lints as “opt-in”

The draw back of using lints as a “opt-in” mechanism is that programmers can enable it is that they always apply to a full scope, i.e. they can just enable it crate wide and if it’s enabled for a match statement it’s enabled for the whole statement including the match arm “bodies”, not just a patterns in the match statement. This could be avoided by using a custom attribut e.g. #[drop_destruction] instead.

I do not believe that this should be a problem in practice as I believe that due to the constraints this RFC putts on the patterns it would still be reasonable if there would be no lint or similar. _I.e. I believe that having a linte/“opt-in” is just a small improvement and not required on itself for this rfc.

Rationale and alternatives

I believe that this approach to allowing some limited form of pattern destruction is preferable to some alternatives a it is clear, simple and straight forward. It does not require any new syntax, trait’s or attributes (but a single additional no_drop_destruction lint is recommended).

Due to the limitations it has wrt. the patterns it avoids the problems which originally caused moving out of fields of types implementing Drop to be disallowed. (As far as I can tell).

Alternatives

  • Do not change anything and force people to add overhead in form of (should be) unnecessary options or suboptimal unsafe code.
  • A modified version of this proposal which behaves different wrt. the “opt-in” lint (see unresolved questions).

There is another proposal, which would add pattern destruction of types but is focuses on a alternative version of Drop which accepts self instead of &mut self, because of this I would say that it is less a alternative but more another proposal which could benefit from this proposal. It also should be noted, that it requires the type to implement a specific trait, which is sub-optimal for simple by pattern-destruction of Drop types as wether or not it’s ok to do so is a property of the code doing so (does it handle all fields) and not the type on it self.

Prior art

This RFC is about lifting a rust specific constraint, there might be some priority art but I’m not aware of any.

Unresolved questions

The current pre-RFC does not specify wether or not you can bind some or the values by reference. And how this interacts which the ergonomic parts of rust which allow omitting ref and similar in some places. Both approaches of allowing and disallowing it have benefits and drawbacks and I will update this section once I’m more clear about them.

While this RFC includes a restriction of using pattern-destruction on drop types through a lint, it can be seen as a somewhat open question wether:

  • We should drop the lint part.
  • Make the lint warn by default instead of error.
  • Replace the lint with a custom attribute
    • which produces an error if not given
    • or a warning if not given

I prefer a lint as it works with all the lint tooling and prefer to start with defaulting to error as we can always later on make the lint only warn if it seems to be a better solution.

Future possibilities

This RFC could be used as basis for some possible future RFCs like e.g. have a alternative to Drop which accepts self instead of &mut self. But this is widely out of the scope of this RFC and not the motivation why this RFC is required.


#2

Another drawback is that you are changing the meaning of ignore (as in _) patterns to mean “drop the field immediately” for variables of types which implement Drop.

This code compiles fine:

    let pair=("hello".to_string(),"world".to_string());

    let (y,_)=pair;
    
    let _=pair;

    println!("{}",pair.1);

In Rust,the _ pattern only drops temporary expressions,variables don’t get dropped if you just assign to that pattern.

Try creating some Drop types which print to the screen when dropped and look at when they print with different patterns.


#3

This RFC does not change any semantics around ignore (_). It just allows to use destructing patterns on types which implement Drop, “ignore” still works like it always did. It just will not run drop on the type which was destructed into it’s parts, but the parts will drop/behave like always.

Through I might have to take a look into the changes which had been made in the past to make pattern matching more ergonomic (by inserting ref etc. and maybe reformulate some sentences in the RFC that make it more clear.

Thx, for the feedback.


#4

If I’m reading this correctly, you want to be able to move out of a Drop type without having it actually drop. I don’t believe there is a formulation in which this is actually sound? Maybe removing that restriction in unsafe is what you want, but that sounds like a huge footgun and yet another knob in unsafe that people will abuse into the ground.

It is never a good idea to pick apart foreign RAII types, and moving out of your own RAII types can also be quite dangerous. You might just write a private unsafe function to break it apart and forget the original type, which is a sufficiently “dumb” operation you could do it by macro. This should be a hard-to-reach-for operation, which is why I think the current situation requiring the utterly hair-raising ptr::read is acceptable.


#5

@drXor:

  • It’s fully sound and safe to do.

    • Rust realized already that safety/soundness cannot rely on Drop to be run.
      That is why std::mem::forget is does not need unsafe.
    • Once you destruct you do no longer drop.
  • At worst you would leak resources.

    • If you destruct you take responsible to handle all resources.
    • Decomposing a (drop) struct into all it’s parts should never be unsafe.
    • If you do some e.g. “self borrow” magic or similar in which case dropping all fields by them self might be unsafe you already broke rust safety guarantees (at last if the fields are not private).
      • You still can do this due to encapsulation, making this fields only visible for you, but if you have any private fields then its also the case that only you can do the destruction as it has to cover all fields and excludes ..
      • To think about why this already broke safety guarantees (at last if the fields are not private) consider what someone with std::mem::swap/std::mem::replace might be able to do. Especially if that person has multiple instances of your type.
  • The “private unsafe” function is not just unnecessary unsafe code, but also potentially tricky and more then one.

    • Form simple wrapper you will go with moving the type to the stack, ptr::read-ing all fields and forgetting the value.
    • But for enums this can easiler get more complex, especially if you have more complex pattern matching.
    • Compound patterns have to split up into multiple ones + ptr::read + mem::forget, but this can easily have a slightly different semantic then the a single pattern and fastly requires 10+ lines of unsafe code just for slightly more complex patterns.
    • Forgetting any forget is hazardous and potentially a security vulnerability.
    • Rust actually doesn’t have pattern destruction but just something which feels like it (see the updated pre-RFC) which is something many people are not aware of expecting them to write that part of unsafe code correct is a bad idea.
  • I agree that it’s mostly used “internally” but the moment you have a private field it also can only be done by you anyway and there are some cases where using it in other places is nice to and restricting it to some visibility modifier would require some form of custom grammar/attributes.

  • It’s not picking apart RAII but instead extending it to allow a well formed transformation of one type into another which currently (unnecessarily) requires unsafe code.

    • Also due to requiring visibility to all fields most types taking advantage of RAII will not be able to be picked apart from anything but them self.
    • Through you might sometimes need unsafe code to reconstruct the type you transformed it into.
  • If you look through std and futures you will find a number of places where either there was a Option added unnecessarily to work around this problem introducing an addition unwrap in nearly every function. Or they don’t provide any into_inner method due to the overhead.

    • The unsafe workaround was never chosen as it’s very easy to brake it when refactoring, adding fields etc. and the cost of messing it up is a use-after-free. Also std, futures, and other libraries try to have as little unsafe code as necessary. With this RFC they could provide the same functionality with more readable/intuitive code, potentially better performance and not any of the drawbacks of the unsafe hack workaround.

#6

@matt1985: I updated the RCF.

When clearing up what I meant it to do, I became aware that it needs additional changes. While the handling of _ is possible without changing it’s semantic it would be suboptimal for this case and might lead to more irritation. So I restricted it to patterns which do not bind to _. Through if it seems to be viable it could later on be extended to other patterns, too.

I mean with the way patterns currently work wrt. _ in rust you can do thinks like:

let S { f1, f2, f3:_, f4:_ } = x;
// yes, same x
let S { f1:_, f2:_, ref f3,  ref f4} = x;

which work around error[E0009]: cannot bind by-move and by-ref in the same pattern which is normally not a problem but is suboptimal wrt. making sure pattern destruction is done correctly.

Even wore you could do:

let S { f1:_, f2:_, f3:_, f4:_ } = x;
drop(x)

Which would be ambiguous as it should do both “destruct” without calling drop and dropping. This should be fixed with the updated RFC.