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.

Sign up for GitHub

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

Jump to bottom

Destructure drop structs #2061

Closed
wants to merge 8 commits into from
Closed

Destructure drop structs #2061

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
063be94
initial version for "Allow destructuring of structs that implement Drop"
Jul 8, 2017
a47a6ee
Update 0000-destructure-drop-structs.md
Jul 9, 2017
c646990
explain the two alternatives in greater detail
Jul 9, 2017
9fcb21a
choose one approach
Jul 9, 2017
97a7670
fix typo
Jul 10, 2017
949d7cb
Add @oli-obk's IndependentDrop idea.
Jul 10, 2017
8bbec82
prefer approach 2
Jul 25, 2017
a425623
explain destructuring and restrict the usage.
Sep 3, 2017
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
58 changes: 58 additions & 0 deletions text/0000-destructure-drop-structs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
- 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 •
edited
Loading

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 •
edited by ghost
Loading

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).

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