-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Drop type destructuring #3738
Conversation
Co-authored-by: Waffle Lapkin <[email protected]>
4031570
to
704a3d2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I think there’s a 3rd alternative. Instead of statement or pattern, 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, The type The code examples above assume pattern matching can transparently “dereference” this layer of let Foo { a, b: _ } = *destructure!(x); why not have Finally, such a (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 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 Of course, this Also note that the naming of the type Footnotes
|
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
implementsDrop
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, butStruct
does not implementDrop
, 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment was marked as duplicate.
Sorry, something went wrong.
# 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, becausex.y
is moved out. So the "you can't move fields out ofDrop
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- users writing
let <pat> = <drop_val>;
and expecting that to disableDrop
; - users holding a value with an important
Drop
impl and accidentally disablingDrop
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"?
There was a problem hiding this comment.
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?
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
typeT
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 droppedT
'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.
There was a problem hiding this comment.
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_mut
s?
There was a problem hiding this comment.
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.
I don't like the name An alternative is to just allow moving fields out of 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)
|
@Aloso As for moving fields out of 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!()
onenum
s, but we also think that even usingdestructure!()
onenum
s will be very uncommon (as types implementingDrop
are almost alwaysstruct
s) and might not outweigh the increased implementation complexity and the fact that we need to communicate this one level matching rule to users.
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 I'm sure 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:
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. |
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. |
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 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 |
Perhaps it'd be less magic from the language perspective if destructuring a type with fn example(x: Foo) {
+ #[allow(forget_drop)]
let Foo { a } = x;
} |
I think with the just-allow-let approach (or, to a lesser degree, These usages might be expected to disable let Foo { .. } = x;
let Foo { ref y } = x; How about this one? let Foo { y } = x; This can only disable In other words, allowing plain |
With just-allow-let + the #[expect(forget_drop)]
let Foo { y } = x; which would error if I am not in fact disabling |
That’s fair, and that’s why I said that It still means there’s no way to disable drop by destructuring when all the fields are |
There was a problem hiding this 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.
fn example(x: Foo) { | ||
// `a` is moved out of `x`, Drop never runs for `x` | ||
destructure!(let Foo { a } = x); | ||
} |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
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) | ||
} |
There was a problem hiding this comment.
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.
@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. |
@kornelski (re this), I don't think this works as a lint. Also note that there are cases where you can "destructure" a |
@workingjubilee 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. |
@ahicks92 I don't feel like this RFC hinders Rust's reliability. The only potential footgun with Not only that, this can only realistically happen in the same module as the type definition ( 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. |
@WaffleLapkin 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. |
There was a problem hiding this 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. |
There was a problem hiding this comment.
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:
- Test the scrutinee against the pattern.
- Execute match guard expressions (using temporary shared borrows).
- If the pattern and the guard succeed, perform the moves and borrows specified by the pattern.
There was a problem hiding this comment.
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`: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
, aT
can be moved out of&move T
, - like
Box<T>
, if theT
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.
There was a problem hiding this comment.
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.
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'sDrop
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 implementDrop
. For example, assumingFoo
is a type that implementsDrop
and has fieldsa
andb
, you could write the following:Written together with @WaffleLapkin
Rendered