-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,368 @@ | ||
- Feature Name: `drop_type_destructuring` | ||
- Start Date: 2024-12-08 | ||
- RFC PR: [rust-lang/rfcs#3738](https://github.com/rust-lang/rfcs/pull/3738) | ||
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) | ||
|
||
# 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. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
There are real use-cases in which you do want to move out of types that implement `Drop`. Two main types of patterns that often need it are: | ||
|
||
- implementing methods like `into_raw_parts` which explicitly hand over their internals and move the burden of dropping those | ||
- "guard" types which might need to be "disarmed" | ||
|
||
In both cases, you want to avoid the regular `Drop` implementation from running at all and get ownership of the fields. | ||
|
||
Right now, code that wants to do this needs to use `unsafe`. For example, [`std::io::BufWriter::into_parts()`](https://doc.rust-lang.org/stable/std/io/struct.BufWriter.html#method.into_parts) has to perform the following gymastics using `ManuallyDrop`: | ||
|
||
```rust | ||
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) | ||
} | ||
Comment on lines
+24
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A safer workaround is to use 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. |
||
``` | ||
|
||
When writing this RFC, we even spent some time to make sure this example from `std` was even correct. By exposing `drop_type_destructuring`, we can reduce the complexity of such use cases. | ||
|
||
Through this, we can avoid `Drop`ping types for which running `Drop` is very important. However, this is already the case because you could use `mem::forget`. | ||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
`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: | ||
|
||
```rust | ||
fn example(x: Foo) { | ||
destructure!(let Foo { a, b: _ } = x); | ||
// yay we own `a` now, we can even drop it! | ||
drop(a); | ||
} | ||
``` | ||
|
||
This moves the field `a` out of `x`, and immediately drops `b`. | ||
Instead of creating a new binding, you can also assign to an existing one by leaving out the `let`, similarly to destructuring assignment: | ||
|
||
```rust | ||
fn example(x: Foo) { | ||
let mut a = vec![1, 2, 3]; | ||
|
||
destructure!(Foo { a, b: _ } = x); | ||
// yay we own `a` now, we can even drop it! | ||
drop(a); | ||
} | ||
``` | ||
|
||
When a type implements `Drop`, this can be because the order in which fields are dropped is very important. It could be unsound to do it in the wrong order. When you write a destructuring, soundly dropping the fields is now your responsibility. | ||
|
||
This can be a heavy burden, so if you are the author of a module or crate, you might want to limit other people destructuring your type. The rule for this is that you can only use `destructure!()` on types in a location where you could also construct that type. This means that in that location, all fields must be visible. | ||
|
||
Importantly, this means that this code is not valid: | ||
|
||
```rust | ||
mod foo { | ||
pub struct Bar { | ||
pub a: Vec<u8>, | ||
b: Vec<u8>, | ||
} | ||
} | ||
|
||
fn example(x: foo::Bar) { | ||
destructure!(let Foo{ a, .. } = x) | ||
} | ||
``` | ||
|
||
By using `..`, we could ignore the fact that `b` was not visible and move out `a` anyway. This is undesirable, as it would implicitly run `drop(b)` even though `b` was not accessible here. In fact, if there was a requirement that `a` was dropped before `b`, it would never be sound to destructure a type `Bar` in a location where `b` is not accessible. | ||
|
||
Finally, you might ask why we need a macro for this at all. This is because for some types, the behavior is slightly different. | ||
Let's use the following example. In Rust right now, this does not compile: | ||
|
||
```rust | ||
struct Foo { | ||
a: Vec<u64> | ||
} | ||
|
||
impl Drop for Foo { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn example(x: Foo) { | ||
// error: cannot move out of type `Foo`, which implements the `Drop` trait | ||
let Foo { a } = x; | ||
} | ||
``` | ||
|
||
This is because we move the field `a` out, and `Foo` implements `Drop`. | ||
The following does compile: | ||
|
||
```rust | ||
struct Foo { | ||
a: Vec<u64> | ||
} | ||
|
||
impl Drop for Foo { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn example(x: Foo) { | ||
// fine, because we don't actually move any fields out | ||
let Foo { .. } = x; | ||
} | ||
``` | ||
|
||
Similarly, the following also works because we don't have to move out any fields, we can just copy `a`, because `u64` implements `Copy`. | ||
|
||
```rust | ||
struct Foo { | ||
a: u64 | ||
} | ||
|
||
impl Drop for Foo { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn example(x: Foo) { | ||
// Totally fine, we just copy a | ||
let Foo { a } = x; | ||
} | ||
``` | ||
|
||
In the two examples above, `Drop` still runs for `x`. | ||
`destructure!()` represents a different operation. When we use it, we actually move the fields out of the type, and prevent `Drop` from running. | ||
|
||
```rust | ||
struct Foo { | ||
a: u64 | ||
} | ||
|
||
impl Drop for Foo { | ||
fn drop(&mut self) {} | ||
} | ||
|
||
fn example(x: Foo) { | ||
// `a` is moved out of `x`, Drop never runs for `x` | ||
destructure!(let Foo { a } = x); | ||
} | ||
Comment on lines
+152
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
``` | ||
|
||
Preventing `Drop` from running is not inherently problematic in Rust. You can already do this for any type through `mem::forget` (and other means), which is safe. | ||
|
||
|
||
Finally, the following is an example of how one could implement the example from [Motivation](#motivation): | ||
|
||
```rust | ||
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) | ||
} | ||
``` | ||
|
||
Now with `destructure!`: | ||
|
||
```rust | ||
pub fn into_parts(self) -> (W, Result<Vec<u8>, WriterPanicked>) { | ||
destructure!(let Self { buf, inner, panicked } = self); | ||
let buf = if !this.panicked { Ok(buf) } else { Err(WriterPanicked { buf }) }; | ||
|
||
(inner, buf) | ||
} | ||
``` | ||
|
||
We don't even need unsafe anymore :tada: | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
Comment on lines
+189
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. I believe reliance on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I'm not sure what this allows in addition to what In addition to the fact that if all your fields are public, doing dangerous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can only // 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 commentThe 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
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Dropping the fields in a different order I think, though maybe you can do that with nested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nested |
||
|
||
A built-in macro with the following "signature" is added to the standard library: | ||
|
||
```rust | ||
#[macro_export] | ||
macro_rules! destructure { | ||
((let)? $pat:pat = $e:expr) => { /* built-in */ } | ||
} | ||
``` | ||
|
||
The result of the expansion checks the following requirements and produces a compilation error if any of them are not met: | ||
- `$pat` must be an irrefutable pattern | ||
- `$e` must be an owned expression that can be matched by pattern `$pat` | ||
- `$e`'s type must be an ADT (`struct` or `enum`) | ||
- The type of `$e` must be constructable in the current context, i.e. | ||
- All fields must be visible to the current module wrt privacy | ||
- If the type is marked with `#[non_exhaustive]`, it must be defined in the current crate | ||
|
||
The semantic of this macro is equivalent to the following: | ||
1. Wrapping `$e` in a `ManuallyDrop` | ||
2. Copying **all** fields of `$e` (for enums all fields of the active variant) | ||
3. If the `let` token is present in the macro invocation, fields that are listed in `$pat` are binded to their respective bindings or patterns. Otherwise, they are assigned similarly to destructuring assignment | ||
4. If `..` is present in `$pat`, all fields that are not mentioned in it are dropped in the order of definition (simialrly to how [normal `let`/destructuring assignment works](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3abd3aebd3378690ff3d2006e12d4120)) | ||
--- | ||
|
||
This is the technical portion of the RFC. Explain the design in sufficient detail that: | ||
|
||
- Its interaction with other features is clear. | ||
- It is reasonably clear how the feature would be implemented. | ||
- Corner cases are dissected by example. | ||
|
||
The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think this is basically just equivalent to the Also:
|
||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
Comment on lines
+220
to
+221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the really obvious alternative: "allow a plain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is important to syntactically mark the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: It seems like allowing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
I do not understand the use of "reliably" here. This never disables
Not really? The problem statement of this RFC is very specifically to allow moving fields out of 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 We might want to spell this out more explicitly though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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
This comment was marked as duplicate.
Sorry, something went wrong. |
||
|
||
## Do not support destructuring assignment | ||
|
||
Instead of allowing optional `let` token, we could always ommit it, and consider `destructure!` to always introduce new bindings. | ||
|
||
It's not evident that destructure assignment is particularly useful in situations where `destructure!` would be used. | ||
|
||
## Expand to a pattern | ||
|
||
Instead of making this macro expand to a statement, containing a pattern and a right hand side, one could imagine a macro that expands *just* to a pattern: | ||
|
||
```rust | ||
let destructure!(Foo { a, b }) = x; | ||
``` | ||
|
||
On its own this could be considered, it also means that whether to use `let` or not (the previous alternative) isn't a concern for the macro. However, by doing this, we might imply that you can also use `destructure!` in other places you might see a pattern: | ||
|
||
```rust | ||
let destructure!(Foo::A { a }) = x else { | ||
panic!("not Foo::A") | ||
}; | ||
|
||
match x { | ||
destructure!(Foo::A { a }) => /* ... */, | ||
Foo::B { .. } => panic!("unexpected Foo::B") | ||
} | ||
``` | ||
|
||
Now, it makes sense that patterns in `destructure!` can be refutable. Even this can be okay, as long as only the top-level pattern is refutable. If we allow nested patterns to be refutable as well, we can construct the following scenario: | ||
|
||
```rust | ||
match y { | ||
destructure!(Bar { a, b: 1 }) => /* ... */, | ||
_ => panic!(".b != 1") | ||
} | ||
``` | ||
|
||
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 commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
In theory this is not impossible to implement. By making this expand to code that uses `ManuallyDrop`, and when a pattern is rejected semantically move values back. However, one can construct even more complicated scenarios than just this one, and actually defining the semantics of this operations quickly becomes incredibly complicated. | ||
|
||
Just as a small example of that, consider this: | ||
|
||
```rust | ||
match z { | ||
destructure!(Baz { a, b: destructure!(Bar { a, b: 1 }) }) => /* ... */, | ||
_ => panic!(".b.b != 1") | ||
} | ||
``` | ||
|
||
Where both `Baz` and `Bar` implement `Drop`. | ||
|
||
As a final note on this, making `destructure!()` expand to a pattern on its own is possible, and even refutability is possible if we only allow "one level" of refutability (`Some(_)`, but not `Some(1)`). However, we simply do not think there are many good uses for this. | ||
The one scenario we could imagine is that might be useful for `destructure!()` on `enum`s, but we also think that even using `destructure!()` on `enum`s will be very uncommon (as types implementing `Drop` are almost always `struct`s) and might not outweigh the increased implementation complexity and the fact that we need to communicate this one level matching rule to users. | ||
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
@WaffleLapkin wrote a crude polyfill for this using a macro and `ManuallyDrop`: | ||
|
||
```rust | ||
/// Destructures `$e` using a provided pattern. | ||
/// | ||
/// Importantly, this works with types which implement `Drop` (ofc, this doesn't run the destructor). | ||
#[macro_export] | ||
macro_rules! destructure { | ||
($Type:ident { $($f:tt $(: $rename:pat)? ),+ $(,)? } = $e:expr) => ( | ||
// FIXME: use $crate:: paths | ||
let tmp = $crate::unstd::_macro_reexport::core::mem::ManuallyDrop::new($e); | ||
|
||
// assert that `$e` is an owned expression, rather than `&Type` | ||
if false { | ||
#[allow(unreachable_code)] | ||
let _assert_owned_expr = [&tmp, &$crate::unstd::_macro_reexport::core::mem::ManuallyDrop::new($Type { $($f: todo!()),* })]; | ||
}; | ||
|
||
$( | ||
let $crate::destructure!(@_internal_pat_helper $f $($rename)?) | ||
// safety: `$e` is of type `$Type<..>` (as asserted above), | ||
// so we have ownership over it's fields. | ||
// `$f` is a field of `$Type<..>` (as asserted above). | ||
// `$e` is moved into a `ManuallyDrop`, which means its `drop` won't be run, | ||
// so we can safely move out its | ||
// the pointer is created from a reference and is thus valid | ||
= unsafe { $crate::unstd::_macro_reexport::core::ptr::read(&tmp.$f) }; | ||
)+ | ||
|
||
// remove the temporary we don't need anymore. | ||
// doesn't actually drop, since `ManuallyDrop`. | ||
_ = {tmp}; | ||
); | ||
(@_internal_pat_helper $f:tt) => ($f); | ||
(@_internal_pat_helper $f:tt $rename:pat) => ($rename); | ||
} | ||
``` | ||
Example usage: | ||
```rust | ||
// `destructure` does not run the destructor, so this **doesn't** unlock the lock. | ||
destructure!(Lock { file, mode: _ } = self); | ||
``` | ||
This polyfill has multiple downsides, such as: | ||
- It does not support ommiting fields with `..` (this is not possible to support in the polyfill, as you are unable to prove that an expression is owned, without specifying its full type) | ||
- It doesn't work with enums with multiple variants, even if you could specify an irrefutable pattern (this theoretically can be supported, but is quite complicated) | ||
- It does not work with tuple struct syntax (impossible to support arbitrary number of fields, hard to support a set number) | ||
|
||
--- | ||
|
||
Discuss prior art, both the good and the bad, in relation to this proposal. | ||
A few examples of what this can include are: | ||
|
||
- For language, library, cargo, tools, and compiler proposals: Does this feature exist in other programming languages and what experience have their community had? | ||
- For community proposals: Is this done by some other community and what were their experiences with it? | ||
- For other teams: What lessons can we learn from what other communities have done here? | ||
- Papers: Are there any published papers or great posts that discuss this? If you have some relevant papers to refer to, this can serve as a more detailed theoretical background. | ||
|
||
This section is intended to encourage you as an author to think about the lessons from other languages, provide readers of your RFC with a fuller picture. | ||
If there is no prior art, that is fine - your ideas are interesting to us whether they are brand new or if it is an adaptation from other languages. | ||
|
||
Note that while precedent set by other languages is some motivation, it does not on its own motivate an RFC. | ||
Please also take into consideration that rust sometimes intentionally diverges from common language features. | ||
|
||
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
These mainly relate to the above-mentioned alternatives: | ||
|
||
- Do we support `let` inside? | ||
- Should `destructure!()` expand to a statement, or a pattern? | ||
|
||
# Future possibilities | ||
[future-possibilities]: #future-possibilities | ||
|
||
If we were to choose for the version that expands to a pattern, we could try to define the semantics of refutable patterns with more than one level, assuming this is even possible in a satisfactory and importantly sound manner. | ||
|
||
In general, we could add some lints in relation to this feature. | ||
General usage of `destructure!` is often okay, especially in places where there is visibility of all fields, but it could be used in unintentional ways so a lint in `clippy::pedantic` for using `destructure` might be considered. | ||
A lint for using `destructure!` on types that don't implement `Drop` at all makes sense regardless. The suggestion there can be to remove `destructure!` completely as it is not necessary. | ||
|
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:
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?
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 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.
Except it is more strict, because moving fields out of
Drop
types is forbidden even if you put something back afterwards: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.
Is
let X { y } = x
is disallowed whenX: Drop
,Y: !Copy
simply because it would be weird and seemingly inconsistent forlet X { y } = x
to be legal whilelet 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 originalx
. That option would still permitlet 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 writinglet AudioLock = the_lock;
orlet AudioLock {} = the_lock;
would be a foot-gun because it wouldn't disableDrop
because all fields areCopy
(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 footgunThere 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:
let <pat> = <drop_val>;
and expecting that to disableDrop
;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?