From 063be941120605420f02177c86913c1da17a9ab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20K=C3=B6ln?= Date: Sat, 8 Jul 2017 12:35:10 +0200 Subject: [PATCH 1/8] initial version for "Allow destructuring of structs that implement Drop" --- text/0000-destructure-drop-structs.md | 56 +++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 text/0000-destructure-drop-structs.md diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md new file mode 100644 index 00000000000..36375bc2135 --- /dev/null +++ b/text/0000-destructure-drop-structs.md @@ -0,0 +1,56 @@ +- 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 + +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 + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +# Alternatives +[alternatives]: #alternatives + +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? From a47a6ee19dcd7f50c43c719ddc1d32c55303eb25 Mon Sep 17 00:00:00 2001 From: s3bk Date: Sun, 9 Jul 2017 11:30:13 +0200 Subject: [PATCH 2/8] Update 0000-destructure-drop-structs.md --- text/0000-destructure-drop-structs.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index 36375bc2135..2eba1980339 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -39,6 +39,8 @@ Either restriction would prevent the issue of [[2]](https://github.com/rust-lang # 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. + # Drawbacks [drawbacks]: #drawbacks From c646990f40823caadee07985596e7a682b6b3df1 Mon Sep 17 00:00:00 2001 From: s3bk Date: Sun, 9 Jul 2017 19:19:13 +0200 Subject: [PATCH 3/8] explain the two alternatives in greater detail --- text/0000-destructure-drop-structs.md | 55 +++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index 2eba1980339..c77a65255d9 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -36,6 +36,61 @@ Another possiblity would be to restrict it to modules that are allowed to impeme 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. + # How We Teach This [how-we-teach-this]: #how-we-teach-this From 9fcb21aaea105051ef4e492f6b9087a02c4d8316 Mon Sep 17 00:00:00 2001 From: s3bk Date: Sun, 9 Jul 2017 19:22:40 +0200 Subject: [PATCH 4/8] choose one approach --- text/0000-destructure-drop-structs.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index c77a65255d9..f28ea73d652 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -91,6 +91,9 @@ Inside the module of declaration, no `unsafe` block is required. Otherwhise an ` 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 From 97a76705b03c0f1a49a13a70347d6ca706c97197 Mon Sep 17 00:00:00 2001 From: s3bk Date: Mon, 10 Jul 2017 10:54:35 +0200 Subject: [PATCH 5/8] fix typo --- text/0000-destructure-drop-structs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index f28ea73d652..7c4afdb344a 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -97,7 +97,7 @@ 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. +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 From 949d7cb5ad819479665323c5f6216783a6ef89c4 Mon Sep 17 00:00:00 2001 From: s3bk Date: Mon, 10 Jul 2017 13:17:35 +0200 Subject: [PATCH 6/8] Add @oli-obk's IndependentDrop idea. --- text/0000-destructure-drop-structs.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index 7c4afdb344a..2944de98893 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -91,6 +91,15 @@ Inside the module of declaration, no `unsafe` block is required. Otherwhise an ` 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 1. From 8bbec825516817d12f2f1ab7cc978b5618014292 Mon Sep 17 00:00:00 2001 From: s3bk Date: Tue, 25 Jul 2017 11:34:38 +0200 Subject: [PATCH 7/8] prefer approach 2 --- text/0000-destructure-drop-structs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index 2944de98893..601d00c750b 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -101,7 +101,7 @@ Pros: No `unsafe` block required and destructuring is not limited to the same mo Cons: Requires an additional trait that authors have to be aware of. ## Suggested approach: -This RFC proposes solution Approch 1. +This RFC proposes solution Approch 2. # How We Teach This [how-we-teach-this]: #how-we-teach-this From a425623942309dc77c42063f0d2019313e3b6def Mon Sep 17 00:00:00 2001 From: s3bk Date: Sun, 3 Sep 2017 18:57:13 +0200 Subject: [PATCH 8/8] explain destructuring and restrict the usage. --- text/0000-destructure-drop-structs.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/text/0000-destructure-drop-structs.md b/text/0000-destructure-drop-structs.md index 601d00c750b..a94a511982b 100644 --- a/text/0000-destructure-drop-structs.md +++ b/text/0000-destructure-drop-structs.md @@ -11,8 +11,19 @@ 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)`. +Destructuring allows to reverse the process of creating a struct from individual fields. +``` +struct Compound { a: A, b: B } +let a: A = ... +let b: B = ... +let c = Compound { a: a, b: b } // wraps and into the struct Compound +let Compound { a: a, b: b } = c; // destructures c into its fields. +// a and b are now the same they where before c was created +``` + +However this is currently not possible if the struct implements `Drop`. +The current way to destructure these cases anyway, requires to read non-copy fields via +`ptr::read(&struct.field)` and then `mem::forget(struct)` to avoid running drop. This leaves more room for error, than there would have to be: 1. forgetting to read all fields, possibly creating a memory leak, @@ -23,6 +34,10 @@ Allowing to destructure these types would ensure that: 1. unused fields are dropped, 2. only owned structs can be destructured. +It is not allowed to implicitly move fields out of structs implementing Drop. +This would make it too easy to accidentally trigger destructuring by accessing a field, +which then means that drop does not run and possibly cause a memory leak. + # Detailed design [design]: #detailed-design