-
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
Destructure drop structs #2061
Destructure drop structs #2061
Changes from 7 commits
063be94
a47a6ee
c646990
9fcb21a
97a7670
949d7cb
8bbec82
a425623
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,125 @@ | ||
- Feature Name: Destructure structs implementing Drop | ||
- Start Date: 2017-07-08 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Allow destructuring of structs that implement Drop. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Currently destructuring of structs that implement Drop, requires to read non-copy fields via | ||
`ptr::read(&struct.field)` followed by `mem::forget(struct)`. | ||
|
||
This leaves more room for error, than there would have to be: | ||
1. forgetting to read all fields, possibly creating a memory leak, | ||
2. acidentally working with `&struct` instead of `struct`, leading to a double free. | ||
(The fields are copied, but mem::forget only forgets the reference.) | ||
|
||
Allowing to destructure these types would ensure that: | ||
1. unused fields are dropped, | ||
2. only owned structs can be destructured. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
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. The level of detail here seems low. Which solution is this RFC proposing? What are the pros and cons of the various options? 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 have added more details and tried to explain the pros/cons. I have also suggested to use the first approach (using an |
||
|
||
As | ||
previously shown destructuring of structs with destructures may create unsoundness | ||
[[1]](https://github.com/rust-lang/rust/issues/3147) | ||
[[2]](https://github.com/rust-lang/rust/issues/26940). | ||
|
||
One possible solution would be to make this an `unsafe` operation and only allow it inside an unsafe `block`. | ||
Another possiblity would be to restrict it to modules that are allowed to impement the type in question. | ||
|
||
Either restriction would prevent the issue of [[2]](https://github.com/rust-lang/rust/issues/26940). | ||
|
||
Say there is the following struct [(taken from here)](https://github.com/rust-lang/rust/issues/26940#issuecomment-120502565): | ||
```rust | ||
pub struct MaybeCrashOnDrop { c: bool } | ||
impl Drop for MaybeCrashOnDrop { | ||
fn drop(&mut self) { | ||
if self.c { | ||
unsafe { *(1 as *mut u8) = 0 } | ||
} | ||
} | ||
} | ||
pub struct InteriorUnsafe { pub m: MaybeCrashOnDrop } | ||
impl InteriorUnsafe { | ||
pub fn new() -> InteriorUnsafe { | ||
InteriorUnsafe { m: MaybeCrashOnDrop{ c: true } } | ||
} | ||
} | ||
impl Drop for InteriorUnsafe { | ||
fn drop(&mut self) { | ||
self.m.c = false; | ||
} | ||
} | ||
``` | ||
|
||
## Approach 1: | ||
```rust | ||
let i = InteriorUnsafe::new(); | ||
unsafe { | ||
let InteriorUnsafe { m: does_crash } = i; | ||
} | ||
``` | ||
Here full responsibility is given to the user, ensuring that the destructuring is sound. | ||
|
||
Pros: More flexibility without resorting to ptr::read & mem::forget. | ||
|
||
Cons: It is still a special case of destructuring and requires an `unsafe` block. | ||
|
||
## Approach 2: | ||
```rust | ||
// somewhere in the same module where InteriorUnsafe was declared | ||
let i = InteriorUnsafe::new(); | ||
let InteriorUnsafe { m: does_crash } = i; | ||
``` | ||
Destructuring does not require an `unsafe` block, but is limited to the module where the type to be destructured was defined. | ||
This ensures no third party may create unsound behaviour by incorrectly destructuring a foreign type. | ||
|
||
Pros: No unsafe block required | ||
|
||
Cons: If a struct implementing Drop needs to be destructured outside the module of definiton, the ptr::read & mem::forget are still required. However this will only affect structs with public fields. | ||
|
||
## Approach 1 & 2: | ||
Inside the module of declaration, no `unsafe` block is required. Otherwhise an `unsafe` block is required. | ||
|
||
Pros: does not require an `unsafe` block in most cases. | ||
Cons: more complexity than required. | ||
|
||
## Approach 3: | ||
Introduce an `unsafe` auto-trait `IndependentDrop`. | ||
It is automatically derived when all fields implement `IndependentDrop`. | ||
|
||
Destructuring of structs is possible if they do not implement `Drop`, or when they implement `IndependentDrop`. | ||
|
||
Pros: No `unsafe` block required and destructuring is not limited to the same module. | ||
Cons: Requires an additional trait that authors have to be aware of. | ||
|
||
## Suggested approach: | ||
This RFC proposes solution Approch 2. | ||
|
||
# How We Teach This | ||
[how-we-teach-this]: #how-we-teach-this | ||
|
||
Probably with a small section in the Nomicon, that explains why destructuring might be dangerous and under what circumstances it is allowed. | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
Why should we *not* do this? | ||
|
||
# Alternatives | ||
[alternatives]: #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 is yet another automatically derived unsafe marker trait. Let's call it 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. After reading it three times, it doesn't actually sound that bad. struct MyData {
i: InteriorUnsafe,
buf: Vec<u8>
}
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.
If 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. Now it makes perfect sense. Adding this alternative to the RFC. |
||
|
||
The trivial alternative is to keep it as is, and keep using `ptr::read` & `mem::forget`. | ||
This could possibly be automated with a macro. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
Which restrictions are required to make this sound? |
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.
It would be nice if the RFC actually said what is destructuring before proceeding to motivation.
Especially given that the problem is not only about destructuring (in patterns), but about moving out individual fields in general. It can be done through any field access.
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.
@petrochenkov
I did not intend to allow moving out individual fields, as that makes it too easy to forget the others.
(Will clarify this in the rfc)