Skip to content
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

Closed
wants to merge 8 commits into from
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 116 additions & 0 deletions text/0000-destructure-drop-structs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
- 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.
Copy link
Contributor

@petrochenkov petrochenkov Sep 3, 2017

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.

struct NonCopy;
let ds = DropStruct { individual_field: NonCopy, other_individual_field: NonCopy };
let f = ds.individual_field; // Moving out without destructuring.
// f is initialized, DROPPED
// ds is uninitialized, NOT DROPPED
// ds.individual_field is uninitialized, NOT DROPPED
// ds.other_individual_field is still initialized, DROPPED

Copy link
Author

@ghost ghost Sep 3, 2017

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)


# 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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 unsafe block).


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.

## Suggested approach:
This RFC proposes solution Approch 1.

# How We Teach This
[how-we-teach-this]: #how-we-teach-this

Probably with a small section in the Nomicon, that explains why destrcuturing might be dangerous and under what circumstances it is allowed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: destructuring


# Drawbacks
[drawbacks]: #drawbacks

Why should we *not* do this?

# Alternatives
[alternatives]: #alternatives
Copy link
Contributor

Choose a reason for hiding this comment

The 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 IndependentDrop for now (bikeshed welcome). Any type without an explicit Drop (autogenerated is fine) impl will be IndependentDrop iff all its fields are IndependentDrop. Destructuring of Drop types is only possible if the type is also IndependentDrop. Types like Vec will unsafe impl<T> IndependentDrop for Vec<T> {}.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading it three times, it doesn't actually sound that bad.
Are all fields required to implement IndependentDrop?

struct MyData {
    i: InteriorUnsafe,
    buf: Vec<u8>
}

MyData should be safe to destructure into InteriorUnsafe and Vec<u8>. The only thing that isn't save to destructure is InteriorUnsafe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all fields required to implement IndependentDrop?

If MyData implements Drop, then its fields need to implement IndependentDrop or you need to unsafe impl IndependentDrop for MyData {} and thus guaranteeing that your destructor doesn't do shenigans with its inner types during the invocation of Drop::drop.

Copy link
Author

Choose a reason for hiding this comment

The 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?