From 96d1330a6d5e0b5a2a9d2519b17cb40ab967f02d Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Fri, 14 Apr 2023 14:59:36 -0300 Subject: [PATCH 1/5] Split may_dangle --- text/3414-split-maydangle.md | 154 +++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 text/3414-split-maydangle.md diff --git a/text/3414-split-maydangle.md b/text/3414-split-maydangle.md new file mode 100644 index 00000000000..029d8559136 --- /dev/null +++ b/text/3414-split-maydangle.md @@ -0,0 +1,154 @@ +- Feature Name: `split_maydangle` +- Start Date: 2023-02-13 +- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) + +# Summary +[summary]: #summary + +Add `#[needs_drop]`, ignore `PhantomData` for outlives requirements. + +# Motivation +[motivation]: #motivation + +This fails to compile: + +```rust +use core::marker::PhantomData; + +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn to_pd(_: T) -> PhantomData { + PhantomData +} + +pub fn foo() { + let mut x; + { + let s = String::from("temporary"); + let p = PrintOnDrop(&s); + x = (to_pd(p), String::new()); + } +} +``` + +And yet, this compiles: + +```rust +use core::marker::PhantomData; + +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn to_pd(_: T) -> PhantomData { + PhantomData +} + +pub fn foo() { + let mut x; + { + let s = String::from("temporary"); + let p = PrintOnDrop(&s); + x = (to_pd(p), ()); + } +} +``` + +Since the values in the tuple are unrelated, they should not affect each other. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +A type marked `#[needs_drop]` gets checked for liveness at drop. This is +necessary for `Vec`: + +```rust +struct Vec { + ... +} + +unsafe impl<#[needs_drop] #[may_dangle] T> Drop for Vec { + fn drop(&mut self) { + ... + } +} +``` + +So that this compiles: + +```rust +fn main() { + let mut v = vec![]; + { + v.push(&String::from("temporary")); + } +} +``` + +But this cannot compile, as it would be unsound: + +```rust +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut v = vec![]; + { + v.push(PrintOnDrop(&*String::from("temporary"))); + } +} +``` + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +This RFC removes the dropck/outlives constraints from `PhantomData` and moves +them into the relevant `Drop` impls instead. + +# Drawbacks +[drawbacks]: #drawbacks + +Requires mild churn to update things to the new way. Failing to update wouldn't +break existing code, but would allow unsound code to compile. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +A type which doesn't need drop should never have dropck/outlives contraints, +but due to the rushed way in which `may_dangle` was implemented, `PhantomData` +ended up having this unfortunate behaviour. This RFC removes this behaviour and +allows strictly more code to compile. + +# Prior art +[prior-art]: #prior-art + +- Compiler MCP 563: It is the exact same thing as this RFC, but a full RFC + seemed appropriate due to observable changes on stable, even if they are + fairly obscure. +- Unsound dropck elaboration for `BTreeMap`: +- `may_dangle`: RFC 1238, RFC 1327 +- This is effectively split from RFC PR 3390 and is not intended for + stabilization. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +N/A + +# Future possibilities +[future-possibilities]: #future-possibilities + +The full RFC 3390, and stabilization. From 5fee8ded611b3857fa6c1f7ecf84d607d2eb8e95 Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Mon, 17 Apr 2023 16:38:56 -0300 Subject: [PATCH 2/5] Improve clarity around `#[may_dangle]` Particularly reference-level explanation. --- text/3414-split-maydangle.md | 130 +++++++++++++++++++++++++++++++---- 1 file changed, 117 insertions(+), 13 deletions(-) diff --git a/text/3414-split-maydangle.md b/text/3414-split-maydangle.md index 029d8559136..a777a372c85 100644 --- a/text/3414-split-maydangle.md +++ b/text/3414-split-maydangle.md @@ -6,11 +6,16 @@ # Summary [summary]: #summary -Add `#[needs_drop]`, ignore `PhantomData` for outlives requirements. +Refine `#[may_dangle]`, treat `PhantomData` like `ManuallyDrop`. # Motivation [motivation]: #motivation +Forgetting to put `PhantomData` in a type annotated with `#[may_dangle]` has +caused multiple soundness issues in the past, notably [rust-lang/rust#76367] and +[rust-lang/rust#99408]. Further, `PhantomData` having dropck behaviour leads to +"spooky-dropck-at-a-distance": + This fails to compile: ```rust @@ -63,12 +68,17 @@ pub fn foo() { } ``` -Since the values in the tuple are unrelated, they should not affect each other. +`PhantomData`'s dropck behaviour is only checked if the type (in this case, the +tuple) marks `needs_drop`, which is confusing. Unrelated tuple elements +shouldn't affect `PhantomData` behaviour. + +[rust-lang/rust#76367]: https://github.com/rust-lang/rust/issues/76367 +[rust-lang/rust#99408]: https://github.com/rust-lang/rust/issues/99408 # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -A type marked `#[needs_drop]` gets checked for liveness at drop. This is +A type marked `#[may_dangle(drop)]` gets checked for liveness at drop. This is necessary for `Vec`: ```rust @@ -76,7 +86,7 @@ struct Vec { ... } -unsafe impl<#[needs_drop] #[may_dangle] T> Drop for Vec { +unsafe impl<#[may_dangle(drop)] T> Drop for Vec { fn drop(&mut self) { ... } @@ -118,11 +128,92 @@ fn main() { This RFC removes the dropck/outlives constraints from `PhantomData` and moves them into the relevant `Drop` impls instead. +Instead of relying on `PhantomData`, there are now 3 forms of `may_dangle`: + +For lifetime parameters: `#[may_dangle] 'a`, places no constraints on `'a`. This +is unchanged from the current form. + +For type parameters that are dropped, they need to be annotated with +`#[may_dangle(drop)]`. These type parameters will be checked as if in a struct +like so: + +```rust +struct Foo(T); + +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut foo; + { + foo = Foo(PrintOnDrop(&*String::from("temporary"))); + } + // ERROR: Foo dropped here, runs destructor for PrintOnDrop +} +``` + +For type parameters that are not dropped, `#[may_dangle(borrow)]` can be used. +These are checked as if in a struct like so: + +```rust +struct Foo(*const T); + +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut foo; + { + foo = Foo(&PrintOnDrop(&*String::from("temporary"))); + } + // no error here +} +``` + +This removes the need for the soundness-bearing `PhantomData` entirely. + +Effectively, a type is checked to be *safe to drop* by the following procedure: + +- If the type has no type or lifetime parameters, it is *safe to drop*. +- If the type is any of the below, it is *safe to drop*: + References, raw pointers, function pointers, function items, `PhantomData` + and `ManuallyDrop`. +- If the type has a `Drop` impl, or is a trait object: + - For every lifetime parameter: + - If the lifetime parameter is marked `#[may_dangle]`, continue. + - If the lifetime is dead, the type is not *safe to drop*. + - For every type parameter: + - If the type parameter is marked `#[may_dangle(borrow)]`, continue. + - If the type parameter is marked `#[may_dangle(drop)]`: + - If the type parameter is not *safe to drop*, then the type is not + *safe to drop*. + - Continue. + - If the type parameter contains lifetimes which are dead, the type is + not *safe to drop*. +- If the type does not have a `Drop` impl: + - For every field: + - If the field type is not *safe to drop*, then the type is not *safe to + drop*. + +(N.B. you cannot add `#[may_dangle]` to a trait object's parameters.) + +This is different from the status quo in that 1. we skip checking fields +entirely in the `Drop` case, and rely only on the `#[may_dangle]`, and 2. we +always treat `PhantomData` as *safe to drop*. + # Drawbacks [drawbacks]: #drawbacks -Requires mild churn to update things to the new way. Failing to update wouldn't -break existing code, but would allow unsound code to compile. +Requires mild churn to update things to the new way. Doesn't provide any +safeguards about getting `drop` vs `borrow` correct. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives @@ -132,23 +223,36 @@ but due to the rushed way in which `may_dangle` was implemented, `PhantomData` ended up having this unfortunate behaviour. This RFC removes this behaviour and allows strictly more code to compile. +Lifetimes cannot be dropped, so it wouldn't make sense to have +`#[may_dangle(drop)]` for lifetimes. We adopt a single form for them. + +This proposal attempts to be as minimal as possible and provides no additional +checking of the `Drop` impl for correctness. An alternative would be to have +something like [RFC PR 3390] which does provide a mechanism for checking the +`Drop` impl for correctness. + # Prior art [prior-art]: #prior-art -- Compiler MCP 563: It is the exact same thing as this RFC, but a full RFC - seemed appropriate due to observable changes on stable, even if they are - fairly obscure. -- Unsound dropck elaboration for `BTreeMap`: +- Compiler MCP 563: Mostly deals with checking the `Drop` impl's correctness, + but involves some aspects of this RFC. A full RFC seems appropriate due to + the observable changes to stable, namely the `PhantomData` behaviour. - `may_dangle`: RFC 1238, RFC 1327 -- This is effectively split from RFC PR 3390 and is not intended for +- This is effectively split from [RFC PR 3390] and is not intended for stabilization. # Unresolved questions [unresolved-questions]: #unresolved-questions -N/A +## `#[may_dangle] T` as alias for `#[may_dangle(drop)] T`. + +Most existing uses of `#[may_dangle] T` are actually `#[may_dangle(drop)] T`, so +to avoid churn we could just make them an alias. This is relevant for e.g. +`hashbrown`, a crate which is used by `std` to provide `HashMap`. # Future possibilities [future-possibilities]: #future-possibilities -The full RFC 3390, and stabilization. +The full [RFC PR 3390], and stabilization. + +[RFC PR 3390]: https://github.com/rust-lang/rfcs/pull/3390 From 18ea8fa30a1619429aab7cd4b616338f95e023aa Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Thu, 20 Apr 2023 21:39:28 -0300 Subject: [PATCH 3/5] Focus strictly on spooky-dropck --- ...-deprecate-spooky-dropck-at-a-distance.md} | 152 +++++++++++++++--- 1 file changed, 128 insertions(+), 24 deletions(-) rename text/{3414-split-maydangle.md => 3414-deprecate-spooky-dropck-at-a-distance.md} (60%) diff --git a/text/3414-split-maydangle.md b/text/3414-deprecate-spooky-dropck-at-a-distance.md similarity index 60% rename from text/3414-split-maydangle.md rename to text/3414-deprecate-spooky-dropck-at-a-distance.md index a777a372c85..1a16cac134e 100644 --- a/text/3414-split-maydangle.md +++ b/text/3414-deprecate-spooky-dropck-at-a-distance.md @@ -1,20 +1,18 @@ -- Feature Name: `split_maydangle` +- Feature Name: `deprecate_spooky_dropck_at_a_distance` - Start Date: 2023-02-13 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- RFC PR: [rust-lang/rfcs#3414](https://github.com/rust-lang/rfcs/pull/3414) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) # Summary [summary]: #summary -Refine `#[may_dangle]`, treat `PhantomData` like `ManuallyDrop`. +Never add outlives requirements for a non-`needs_drop` type. Adjust +`#[may_dangle]` for the required semantics. # Motivation [motivation]: #motivation -Forgetting to put `PhantomData` in a type annotated with `#[may_dangle]` has -caused multiple soundness issues in the past, notably [rust-lang/rust#76367] and -[rust-lang/rust#99408]. Further, `PhantomData` having dropck behaviour leads to -"spooky-dropck-at-a-distance": +`PhantomData` having dropck behaviour leads to "spooky-dropck-at-a-distance": This fails to compile: @@ -70,14 +68,118 @@ pub fn foo() { `PhantomData`'s dropck behaviour is only checked if the type (in this case, the tuple) marks `needs_drop`, which is confusing. Unrelated tuple elements -shouldn't affect `PhantomData` behaviour. +shouldn't affect `PhantomData` behaviour, but the above example shows that they +do. This RFC makes it so they don't. -[rust-lang/rust#76367]: https://github.com/rust-lang/rust/issues/76367 -[rust-lang/rust#99408]: https://github.com/rust-lang/rust/issues/99408 +Likewise, `[T; 0]` produces the same effect: it is not `needs_drop`, but adds +outlive requirements, thus also exhibiting "spooky-dropck-at-a-distance". + +Simply defining every non-`needs_drop` type as being pure w.r.t. drop would, +however, break `#[may_dangle]`, so we need to adapt it. # Guide-level explanation [guide-level-explanation]: #guide-level-explanation +## Changes to dropck (stable) + +Types like `fn(T)`, `ManuallyDrop`, `PhantomData`, `[T; 0]` and `&'a T`, +that don't need to be dropped, place no liveness requirements on `'a` or `T` +when they go out of scope - even if `T` would otherwise introduce liveness +requirements. + +In other words, this compiles: + +```rust +fn main() { + let mut x; + { + x = &String::new(); + // String implicitly dropped here + } + // x implicitly dropped here +} +``` + +But a type which does need to be dropped, introduces liveness requirements: + +```rust +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut x; + { + x = PrintOnDrop(&*String::new()); + // String implicitly dropped here + } + // x implicitly dropped here + // ERROR: temporary may not live long enough +} +``` + +Unless it's in one of the above types: + +```rust +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut x; + { + x = &PrintOnDrop(&*String::new()); + // PrintOnDrop implicitly dropped here + // String implicitly dropped here + } + // x implicitly dropped here +} +``` + +As a special case, some built-in types, like `Vec`, `Box`, `Rc`, `Arc`, +`HashMap`, among others, despite needing to be dropped, do not place liveness +requirements unless `T` demands liveness requirements. This is okay: + +```rust +fn main() { + let mut x = vec![]; + { + x.push(&String::new()); + // String implicitly dropped here + } + // x implicitly dropped here +} +``` + +But this isn't: + +```rust +struct PrintOnDrop<'s>(&'s str); +impl<'s> Drop for PrintOnDrop<'s> { + fn drop(&mut self) { + println!("{}", self.0); + } +} + +fn main() { + let mut x = vec![]; + { + x.push(PrintOnDrop(&*String::new())); + // String implicitly dropped here + } + // x implicitly dropped here + // ERROR: temporary may not live long enough +} +``` + +## Changes to `#[may_dangle]` (unstable) + A type marked `#[may_dangle(drop)]` gets checked for liveness at drop. This is necessary for `Vec`: @@ -125,8 +227,8 @@ fn main() { # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -This RFC removes the dropck/outlives constraints from `PhantomData` and moves -them into the relevant `Drop` impls instead. +This RFC removes the dropck/outlives constraints from `PhantomData` and `[T; 0]` +and moves them into the relevant `Drop` impls instead. Instead of relying on `PhantomData`, there are now 3 forms of `may_dangle`: @@ -178,14 +280,13 @@ fn main() { } ``` -This removes the need for the soundness-bearing `PhantomData` entirely. - Effectively, a type is checked to be *safe to drop* by the following procedure: - If the type has no type or lifetime parameters, it is *safe to drop*. - If the type is any of the below, it is *safe to drop*: - References, raw pointers, function pointers, function items, `PhantomData` - and `ManuallyDrop`. + References, raw pointers, function pointers, function items, `PhantomData`, + `ManuallyDrop` and empty arrays. In other words, all (at the time of + writing) non-`needs_drop` types. - If the type has a `Drop` impl, or is a trait object: - For every lifetime parameter: - If the lifetime parameter is marked `#[may_dangle]`, continue. @@ -212,24 +313,27 @@ always treat `PhantomData` as *safe to drop*. # Drawbacks [drawbacks]: #drawbacks -Requires mild churn to update things to the new way. Doesn't provide any -safeguards about getting `drop` vs `borrow` correct. +Due to `#[may_dangle]` changes, this requires mild churn to update things to the +new way. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives A type which doesn't need drop should never have dropck/outlives contraints, but due to the rushed way in which `may_dangle` was implemented, `PhantomData` -ended up having this unfortunate behaviour. This RFC removes this behaviour and -allows strictly more code to compile. +ended up having this unfortunate "spooky-dropck-at-a-distance" behaviour. This +RFC removes this behaviour and allows strictly more code to compile. Lifetimes cannot be dropped, so it wouldn't make sense to have `#[may_dangle(drop)]` for lifetimes. We adopt a single form for them. -This proposal attempts to be as minimal as possible and provides no additional -checking of the `Drop` impl for correctness. An alternative would be to have -something like [RFC PR 3390] which does provide a mechanism for checking the -`Drop` impl for correctness. +This proposal attempts to be as minimal as possible and focuses entirely on the +"spooky-dropck-at-a-distance" behaviour. It also distinguishes between stable +behaviour and unstable behaviour, opting *not* to document unstable behaviour, +which is subject to change, as part of stable behaviour. While the unstable +behaviour *is* relevant to dropck, particularly where collection types (`Vec`, +`HashMap`, etc) expose some details about it to stable code, we opt to document +it separately instead. # Prior art [prior-art]: #prior-art From 286369e2d4cf52ce633b485067b03fc21315c706 Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Sat, 22 Apr 2023 17:31:52 -0300 Subject: [PATCH 4/5] Apply feedback from review --- ...4-deprecate-spooky-dropck-at-a-distance.md | 138 +++++++++++++----- 1 file changed, 98 insertions(+), 40 deletions(-) diff --git a/text/3414-deprecate-spooky-dropck-at-a-distance.md b/text/3414-deprecate-spooky-dropck-at-a-distance.md index 1a16cac134e..e5f4f8cfb12 100644 --- a/text/3414-deprecate-spooky-dropck-at-a-distance.md +++ b/text/3414-deprecate-spooky-dropck-at-a-distance.md @@ -40,7 +40,7 @@ pub fn foo() { } ``` -And yet, this compiles: +And yet, this compiles, because now the tuple has a `()` and not a `String`: ```rust use core::marker::PhantomData; @@ -61,7 +61,7 @@ pub fn foo() { { let s = String::from("temporary"); let p = PrintOnDrop(&s); - x = (to_pd(p), ()); + x = (to_pd(p), ()); // changed from `String::new()` to `()`. } } ``` @@ -93,7 +93,8 @@ In other words, this compiles: fn main() { let mut x; { - x = &String::new(); + let s = String::new(); + x = &s; // String implicitly dropped here } // x implicitly dropped here @@ -113,7 +114,8 @@ impl<'s> Drop for PrintOnDrop<'s> { fn main() { let mut x; { - x = PrintOnDrop(&*String::new()); + let s = String::new(); + x = PrintOnDrop(&*s); // String implicitly dropped here } // x implicitly dropped here @@ -121,7 +123,7 @@ fn main() { } ``` -Unless it's in one of the above types: +Unless it's in one of the previously mentioned types: ```rust struct PrintOnDrop<'s>(&'s str); @@ -134,30 +136,35 @@ impl<'s> Drop for PrintOnDrop<'s> { fn main() { let mut x; { - x = &PrintOnDrop(&*String::new()); + let s = String::new(); + let p = PrintOnDrop(&*s); + x = &p; // PrintOnDrop implicitly dropped here // String implicitly dropped here } - // x implicitly dropped here + // x implicitly dropped here, with type &'p PrintOnDrop<'s> } ``` As a special case, some built-in types, like `Vec`, `Box`, `Rc`, `Arc`, `HashMap`, among others, despite needing to be dropped, do not place liveness -requirements unless `T` demands liveness requirements. This is okay: +requirements unless `T` demands liveness requirements. This is okay, as the +contained type is a reference, which does not have liveness requirements: ```rust fn main() { let mut x = vec![]; { - x.push(&String::new()); + let s = String::new(); + x.push(&s); // String implicitly dropped here } // x implicitly dropped here } ``` -But this isn't: +But this isn't, as it now contains a `PrintOnDrop<'_>`, which does have liveness +requirements: ```rust struct PrintOnDrop<'s>(&'s str); @@ -170,11 +177,14 @@ impl<'s> Drop for PrintOnDrop<'s> { fn main() { let mut x = vec![]; { - x.push(PrintOnDrop(&*String::new())); + let s = String::new(); + x.push(PrintOnDrop(&*s)); // String implicitly dropped here } - // x implicitly dropped here - // ERROR: temporary may not live long enough + // x implicitly dropped here, as are any `PrintOnDrop<'_>` values contained + // within. + // ERROR: `s` may not live long enough (required by the `Drop` impl of + // `PrintOnDrop<'_>`, which may be called when `Vec<_>` goes out of scope) } ``` @@ -201,12 +211,16 @@ So that this compiles: fn main() { let mut v = vec![]; { - v.push(&String::from("temporary")); + let s = String::from("dropped before Vec"); + v.push(&s); } } ``` -But this cannot compile, as it would be unsound: +(References stored in `Vec` do not need to be live when `Vec` is dropped.) + +But this cannot compile, for it would be unsound to drop a `PrintOnDrop` after +its borrowee, as it would try to print the borrowee and cause an use-after-free: ```rust struct PrintOnDrop<'s>(&'s str); @@ -219,11 +233,14 @@ impl<'s> Drop for PrintOnDrop<'s> { fn main() { let mut v = vec![]; { - v.push(PrintOnDrop(&*String::from("temporary"))); + let s = String::from("dropped before Vec"); + v.push(PrintOnDrop(&*s)); } } ``` +(`PrintOnDrop<'_>` stored in `Vec` do need to be live when `Vec` is dropped.) + # Reference-level explanation [reference-level-explanation]: #reference-level-explanation @@ -236,47 +253,88 @@ For lifetime parameters: `#[may_dangle] 'a`, places no constraints on `'a`. This is unchanged from the current form. For type parameters that are dropped, they need to be annotated with -`#[may_dangle(drop)]`. These type parameters will be checked as if in a struct -like so: +`#[may_dangle(drop)]`. This is correct for types which call +`core::ptr::drop_in_place::`, directly or indirectly, e.g. `Vec`, +`Box`, etc. + +For type parameters that are not dropped, `#[may_dangle(borrow)]` can be used. +This is correct for types that never drop `T`, such as `Weak`. This must +never be used for types which call `drop_in_place::`. + +These are okay: ```rust -struct Foo(T); +struct Weak { + inner: *const RcBox +} -struct PrintOnDrop<'s>(&'s str); -impl<'s> Drop for PrintOnDrop<'s> { +unsafe impl<#[may_dangle(borrow)] T> Drop for Weak { fn drop(&mut self) { - println!("{}", self.0); + if ... { + // SAFETY: deallocates the `RcBox`, but does not drop `T`. + unsafe { + self.deallocate_inner() + } + } } } -fn main() { - let mut foo; - { - foo = Foo(PrintOnDrop(&*String::from("temporary"))); - } - // ERROR: Foo dropped here, runs destructor for PrintOnDrop +struct Vec { + len: usize, + capacity: usize, + buf: *const T, +} + +unsafe impl<#[may_dangle(drop)] T> Drop for Vec { + fn drop(&mut self) { + for i in (0..self.len) { + // SAFETY: `#[may_dangle(drop)]` allows us to call `drop_in_place`. + unsafe { + drop_in_place(self.buf.offset(i)); + } + } + } +} + +struct PrintOnDrop(T); + +impl Drop for PrintOnDrop { + fn drop(&mut self) { + println!("{}", self.0); + } } ``` -For type parameters that are not dropped, `#[may_dangle(borrow)]` can be used. -These are checked as if in a struct like so: +These are unsound: ```rust -struct Foo(*const T); +struct Vec { + len: usize, + capacity: usize, + buf: *const T, +} -struct PrintOnDrop<'s>(&'s str); -impl<'s> Drop for PrintOnDrop<'s> { +unsafe impl<#[may_dangle(borrow)] T> Drop for Vec { fn drop(&mut self) { - println!("{}", self.0); + for i in (0..self.len) { + // UNSOUND: `#[may_dangle(borrow)]` does NOT allow us to call + // `drop_in_place`. this can lead to use-after-frees. + unsafe { + drop_in_place(self.buf.offset(i)); + } + } } } -fn main() { - let mut foo; - { - foo = Foo(&PrintOnDrop(&*String::from("temporary"))); - } - // no error here +struct PrintOnDrop(T); + +unsafe impl<#[may_dangle(drop)] T> Drop for PrintOnDrop { + fn drop(&mut self) { + // UNSOUND: `#[may_dangle(drop)]` only allows us to call `drop_in_place` + // but we're printing the value instead. this can lead to + // use-after-frees. + println!("{}", self.0); + } } ``` From 408970a62d3715f1de4f6622054948208ae98f86 Mon Sep 17 00:00:00 2001 From: SoniEx2 Date: Tue, 16 May 2023 13:21:08 -0300 Subject: [PATCH 5/5] Rename away from spooky-dropck-at-a-distance --- ...stance.md => 3414-tweak-phantomdata-drop.md} | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) rename text/{3414-deprecate-spooky-dropck-at-a-distance.md => 3414-tweak-phantomdata-drop.md} (95%) diff --git a/text/3414-deprecate-spooky-dropck-at-a-distance.md b/text/3414-tweak-phantomdata-drop.md similarity index 95% rename from text/3414-deprecate-spooky-dropck-at-a-distance.md rename to text/3414-tweak-phantomdata-drop.md index e5f4f8cfb12..0037ee6bc2c 100644 --- a/text/3414-deprecate-spooky-dropck-at-a-distance.md +++ b/text/3414-tweak-phantomdata-drop.md @@ -1,4 +1,4 @@ -- Feature Name: `deprecate_spooky_dropck_at_a_distance` +- Feature Name: `tweak_phantomdata_drop` - Start Date: 2023-02-13 - RFC PR: [rust-lang/rfcs#3414](https://github.com/rust-lang/rfcs/pull/3414) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) @@ -6,13 +6,14 @@ # Summary [summary]: #summary -Never add outlives requirements for a non-`needs_drop` type. Adjust -`#[may_dangle]` for the required semantics. +Never add outlives requirements for a non-`needs_drop` type, particularly +`PhantomData` but also `[T; 0]`. Adjust `#[may_dangle]` for the required +semantics. # Motivation [motivation]: #motivation -`PhantomData` having dropck behaviour leads to "spooky-dropck-at-a-distance": +`PhantomData` having dropck behaviour leads to somewhat surprising behaviour: This fails to compile: @@ -72,7 +73,7 @@ shouldn't affect `PhantomData` behaviour, but the above example shows that they do. This RFC makes it so they don't. Likewise, `[T; 0]` produces the same effect: it is not `needs_drop`, but adds -outlive requirements, thus also exhibiting "spooky-dropck-at-a-distance". +outlive requirements, thus also exhibiting the surprising behaviour. Simply defining every non-`needs_drop` type as being pure w.r.t. drop would, however, break `#[may_dangle]`, so we need to adapt it. @@ -379,14 +380,14 @@ new way. A type which doesn't need drop should never have dropck/outlives contraints, but due to the rushed way in which `may_dangle` was implemented, `PhantomData` -ended up having this unfortunate "spooky-dropck-at-a-distance" behaviour. This -RFC removes this behaviour and allows strictly more code to compile. +ended up having this unfortunate behaviour. This RFC removes this behaviour and +allows strictly more code to compile. Lifetimes cannot be dropped, so it wouldn't make sense to have `#[may_dangle(drop)]` for lifetimes. We adopt a single form for them. This proposal attempts to be as minimal as possible and focuses entirely on the -"spooky-dropck-at-a-distance" behaviour. It also distinguishes between stable +phantomdata-needsdrop-dropck interaction. It also distinguishes between stable behaviour and unstable behaviour, opting *not* to document unstable behaviour, which is subject to change, as part of stable behaviour. While the unstable behaviour *is* relevant to dropck, particularly where collection types (`Vec`,