From 60a0782390e2063d2d0a24c798bfd37cb2a42611 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Dec 2022 23:03:31 +0100 Subject: [PATCH 1/3] fix and extend dropck documentation --- library/core/src/marker.rs | 15 +++----- library/core/src/ops/drop.rs | 67 ++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 11 deletions(-) diff --git a/library/core/src/marker.rs b/library/core/src/marker.rs index 07a7d45c7ebcc..de285c99cb1f9 100644 --- a/library/core/src/marker.rs +++ b/library/core/src/marker.rs @@ -668,16 +668,9 @@ impl !Sync for *mut T {} /// /// ## Ownership and the drop check /// -/// Adding a field of type `PhantomData` indicates that your -/// type owns data of type `T`. This in turn implies that when your -/// type is dropped, it may drop one or more instances of the type -/// `T`. This has bearing on the Rust compiler's [drop check] -/// analysis. -/// -/// If your struct does not in fact *own* the data of type `T`, it is -/// better to use a reference type, like `PhantomData<&'a T>` -/// (ideally) or `PhantomData<*const T>` (if no lifetime applies), so -/// as not to indicate ownership. +/// Adding a field of type `PhantomData` indicates that your type *owns* data of type `T`. This +/// in turn has effects on the Rust compiler's [drop check] analysis, but that only matters in very +/// specific circumstances. For the exact rules, see the [drop check] documentation. /// /// ## Layout /// @@ -685,7 +678,7 @@ impl !Sync for *mut T {} /// * `size_of::>() == 0` /// * `align_of::>() == 1` /// -/// [drop check]: ../../nomicon/dropck.html +/// [drop check]: Drop#drop-check #[lang = "phantom_data"] #[stable(feature = "rust1", since = "1.0.0")] pub struct PhantomData; diff --git a/library/core/src/ops/drop.rs b/library/core/src/ops/drop.rs index a2c3d978cc4fa..804d2c775fc02 100644 --- a/library/core/src/ops/drop.rs +++ b/library/core/src/ops/drop.rs @@ -132,6 +132,73 @@ /// are `Copy` get implicitly duplicated by the compiler, making it very /// hard to predict when, and how often destructors will be executed. As such, /// these types cannot have destructors. +/// +/// ## Drop check +/// +/// Dropping interacts with the borrow checker in subtle ways: when a type `T` is being implicitly +/// dropped as some variable of this type goes out of scope, the borrow checker needs to ensure that +/// calling `T`'s destructor at this moment is safe. In particular, it also needs to be safe to +/// recursively drop all the fields of `T`. For example, it is crucial that code like the following +/// is being rejected: +/// +/// ```compile_fail,E0597 +/// use std::cell::Cell; +/// +/// struct S<'a>(Cell>>, Box); +/// impl Drop for S<'_> { +/// fn drop(&mut self) { +/// if let Some(r) = self.0.get() { +/// // Print the contents of the `Box` in `r`. +/// println!("{}", r.1); +/// } +/// } +/// } +/// +/// fn main() { +/// // Set up two `S` that point to each other. +/// let s1 = S(Cell::new(None), Box::new(42)); +/// let s2 = S(Cell::new(Some(&s1)), Box::new(42)); +/// s1.0.set(Some(&s2)); +/// // Now they both get dropped. But whichever is the 2nd one +/// // to be dropped will access the `Box` in the first one, +/// // which is a use-after-free! +/// } +/// ``` +/// +/// The Nomicon discusses the need for [drop check in more detail][drop check]. +/// +/// To reject such code, the "drop check" analysis determines which types and lifetimes need to +/// still be live when `T` gets dropped: +/// - If `T` has no drop glue, then trivially nothing is required to be live. This is the case if +/// neither `T` nor any of its (recursive) fields have a destructor (`impl Drop`). [`PhantomData`] +/// and [`ManuallyDrop`] are considered to never have a destructor, no matter their field type. +/// - If `T` has drop glue, then, for all types `U` that are *owned* by any field of `T`, +/// recursively add the types and lifetimes that need to be live when `U` gets dropped. The set of +/// owned types is determined by recursively traversing `T`: +/// - Recursively descend through `PhantomData`, `Box`, tuples, and arrays (including arrays of +/// length 0). +/// - Stop at reference and raw pointer types as well as function pointers and function items; +/// they do not own anything. +/// - Stop at non-composite types (type parameters that remain generic in the current context and +/// base types such as integers and `bool`); these types are owned. +/// - When hitting an ADT with `impl Drop`, stop there; this type is owned. +/// - When hitting an ADT without `impl Drop`, recursively descend to its fields. (For an `enum`, +/// consider all fields of all variants.) +/// - Furthermore, if `T` implements `Drop`, then all generic (lifetime and type) parameters of `T` +/// must be live. +/// +/// In the above example, the last clause implies that `'a` must be live when `S<'a>` is dropped, +/// and hence the example is rejected. If we remove the `impl Drop`, the liveness requirement +/// disappears and the example is accepted. +/// +/// There exists an unstable way for a type to opt-out of the last clause; this is called "drop +/// check eyepatch" or `may_dangle`. For more details on this nightly-only feature, see the +/// [discussion in the Nomicon][nomicon]. +/// +/// [`ManuallyDrop`]: crate::mem::ManuallyDrop +/// [`PhantomData`]: crate::marker::PhantomData +/// [drop check]: ../../nomicon/dropck.html +/// [nomicon]: ../../nomicon/phantom-data.html#an-exception-the-special-case-of-the-standard-library-and-its-unstable-may_dangle #[lang = "drop"] #[stable(feature = "rust1", since = "1.0.0")] #[const_trait] From 967ebb9c3d21d1692b0fa51112eee71c1b4668c4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 15 Feb 2023 11:41:33 +0100 Subject: [PATCH 2/3] add test for may_dangle and ManuallyDrop --- tests/ui/drop/dropck-eyepatch-manuallydrop.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/ui/drop/dropck-eyepatch-manuallydrop.rs diff --git a/tests/ui/drop/dropck-eyepatch-manuallydrop.rs b/tests/ui/drop/dropck-eyepatch-manuallydrop.rs new file mode 100644 index 0000000000000..ff100cd941fd6 --- /dev/null +++ b/tests/ui/drop/dropck-eyepatch-manuallydrop.rs @@ -0,0 +1,22 @@ +// check-pass +//! This test checks that dropck knows that ManuallyDrop does not drop its field. +#![feature(dropck_eyepatch)] + +use std::mem::ManuallyDrop; + +struct S(ManuallyDrop); + +unsafe impl<#[may_dangle] T> Drop for S { + fn drop(&mut self) {} +} + +struct NonTrivialDrop<'a>(&'a str); +impl<'a> Drop for NonTrivialDrop<'a> { + fn drop(&mut self) {} +} + +fn main() { + let s = String::from("string"); + let _t = S(ManuallyDrop::new(NonTrivialDrop(&s))); + drop(s); +} From b93fd8355acbe8a4ffd53ad32c3d7f4798c15b98 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 12 May 2023 15:25:24 +0200 Subject: [PATCH 3/3] hedge for future changes Co-authored-by: lcnr --- library/core/src/marker.rs | 8 +++++--- library/core/src/ops/drop.rs | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/library/core/src/marker.rs b/library/core/src/marker.rs index de285c99cb1f9..2e8cab8601aba 100644 --- a/library/core/src/marker.rs +++ b/library/core/src/marker.rs @@ -668,9 +668,11 @@ impl !Sync for *mut T {} /// /// ## Ownership and the drop check /// -/// Adding a field of type `PhantomData` indicates that your type *owns* data of type `T`. This -/// in turn has effects on the Rust compiler's [drop check] analysis, but that only matters in very -/// specific circumstances. For the exact rules, see the [drop check] documentation. +/// The exact interaction of `PhantomData` with drop check **may change in the future**. +/// +/// Currently, adding a field of type `PhantomData` indicates that your type *owns* data of type +/// `T` in very rare circumstances. This in turn has effects on the Rust compiler's [drop check] +/// analysis. For the exact rules, see the [drop check] documentation. /// /// ## Layout /// diff --git a/library/core/src/ops/drop.rs b/library/core/src/ops/drop.rs index 804d2c775fc02..9ebf426be9513 100644 --- a/library/core/src/ops/drop.rs +++ b/library/core/src/ops/drop.rs @@ -168,7 +168,8 @@ /// The Nomicon discusses the need for [drop check in more detail][drop check]. /// /// To reject such code, the "drop check" analysis determines which types and lifetimes need to -/// still be live when `T` gets dropped: +/// still be live when `T` gets dropped. The exact details of this analysis are not yet +/// stably guaranteed and **subject to change**. Currently, the analysis works as follows: /// - If `T` has no drop glue, then trivially nothing is required to be live. This is the case if /// neither `T` nor any of its (recursive) fields have a destructor (`impl Drop`). [`PhantomData`] /// and [`ManuallyDrop`] are considered to never have a destructor, no matter their field type.