Add the #[manually_drop] attribute.

ssbr · ssbr · commit 9308e4cf4565 · 2023-02-06T14:12:37.000-08:00
This attribute is equivalent to `#[lang = "manually_drop"]`, and the behavior of both are extended to allow the type to define `Drop`, in which case, that will still be run. Only the field drop glue is suppressed. This PR should essentially fully implement #100344. Some additional notes: `#[manually_drop]` means "do not destroy the fields automatically during destruction". `ManuallyDrop`, is (or could be) "just" `#[manually_drop] struct ManuallyDrop<T>(T);`. The intent is, per the MCP, to allow customization of the drop order, without making the interface for initializing or accessing the fields of the struct clumsier than a normal Rust type. It also -- and people did seem excited about this part -- lifts `ManuallyDrop` from being a special language supported feature to just another user of `#[manually_drop]`. There is the question of how this interacts with "insignificant" drop. I had trouble understanding the comments, but insignificant drop appears to just be a static analysis tool, and not something that changes behavior. (For example, it's used to detect if a language change will reorder drops in a meaningful way -- meaning, reorder the significant drops, not the insignificant ones.) Since it's unlikely to be used for `#[manually_drop]` types, I don't think it matters a whole lot. And where a destructor is defined, it would seem to make sense for `#[manually_drop]` types to match exactly the behavior of `union`, since they both have the shared property that field drop glue is suppressed. I looked for all locations that queried for `is_manually_drop` in any form, and found two difficult cases which are hardcoded for `ManuallyDrop` in particular. The first is a clippy lint for redundant clones. I don't understand why it special-cases `ManuallyDrop`, and it's almost certainly wrong for it to special-case `#[manually_drop]` types in general. However, without understanding the original rationale, I have trouble figuring the right fix. Perhaps it should simply be deleted altogether. The second is unions -- specifically, the logic for disabling `DerefMut`. `my_union.x.y = z` will automatically dereference `my_union.x` if it implements `DerefMut`, unless it is `ManuallyDrop`, in which case it will not. This is because `ManuallyDrop` is a pointer back to its content, and so this will automatically call `drop` on a probably uninitialized field, and is unsafe. This is true of `ManuallyDrop`, but not necessarily any other `#[manually_drop]` type. I believe the correct fix would, instead, be a way to mark and detect types which are a smart pointer whose pointee is within `self`. See, for example, this playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=76fb22a6214ce453538fc18ec35a839d But that needs to wait for a separate RFC. For now, we apply exactly the same restriction for `ManuallyDrop` and for any other `#[manually_drop]` type, even though it may be confusing. 1. Delete `#[lang = "manually_drop"]`. I'm not sure if anything special needs to be done here other than replacing it with `#[manually_drop]` -- is there a compatibility guarantee that must be upheld? 2. (Optional) Fix the redundant clone check to correctly handle `#[manually_drop]` structs that aren't `ManuallyDrop`. 3. When there is more experience with the feature (e.g. in Crubit) create a full RFC based on the MCP, and go through the remainder of the stabilization process. (Also, do things like generalize the union error messages to not specifically mention `ManuallyDrop`, but also mention `#[manually_drop]`. For as long as the feature is unstable, the error messages would be confusing if they referenced it...)
diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs
@@ -442,6 +442,8 @@ declare_features! (
     (active, lint_reasons, "1.31.0", Some(54503), None),
     /// Give access to additional metadata about declarative macro meta-variables.
     (active, macro_metavar_expr, "1.61.0", Some(83527), None),
+    /// Allows `#[manually_drop]` on type definitions.
+    (active, manually_drop_attr, "1.64.0", Some(100344), None),
     /// Allows `#[marker]` on certain traits allowing overlapping implementations.
     (active, marker_trait_attr, "1.30.0", Some(29864), None),
     /// A minimal, sound subset of specialization intended to be used by the
diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs
@@ -480,6 +480,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         deprecated_safe, Normal, template!(List: r#"since = "version", note = "...""#), ErrorFollowing,
         experimental!(deprecated_safe),
     ),
+    // lang-team MCP 135
+    gated!(
+        manually_drop, Normal, template!(Word), WarnFollowing, manually_drop_attr, experimental!(manually_drop),
+    ),
 
     // `#[collapse_debuginfo]`
     gated!(
diff --git a/compiler/rustc_hir_analysis/src/check/check.rs b/compiler/rustc_hir_analysis/src/check/check.rs
@@ -112,8 +112,10 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b
                     allowed_union_field(*elem, tcx, param_env)
                 }
                 _ => {
-                    // Fallback case: allow `ManuallyDrop` and things that are `Copy`.
-                    ty.ty_adt_def().is_some_and(|adt_def| adt_def.is_manually_drop())
+                    // Fallback case: if there is no destructor (including field drop glue), because it is
+                    // `Copy` or is `#[manually_drop]` with no `Drop`, then allow it.
+                    ty.ty_adt_def()
+                        .is_some_and(|adt_def| adt_def.is_manually_drop() && !adt_def.has_dtor(tcx))
                         || ty.is_copy_modulo_regions(tcx, param_env)
                 }
             }
diff --git a/compiler/rustc_hir_typeck/src/place_op.rs b/compiler/rustc_hir_typeck/src/place_op.rs
@@ -330,12 +330,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         }
                         // If this is a union field, also throw an error for `DerefMut` of `ManuallyDrop` (see RFC 2514).
                         // This helps avoid accidental drops.
+                        //
+                        // FIXME: This is not correct for `#[manually_drop]` in general, only when the struct is
+                        // a smart pointer whose pointee is at the same address, and whose pointee implements `Drop`.
+                        // Instead of checking for `#[manually_drop]`, this should likely be a more restricted check
+                        // for such types, or else union really should special-case and only permit `ManuallyDrop`, and
+                        // not `#[manually_drop]` types in general.
                         if inside_union
                             && source.ty_adt_def().map_or(false, |adt| adt.is_manually_drop())
                         {
                             let mut err = self.tcx.sess.struct_span_err(
                                 expr.span,
-                                "not automatically applying `DerefMut` on `ManuallyDrop` union field",
+                                "not automatically applying `DerefMut` on manually dropped union field",
                             );
                             err.help(
                                 "writing to this reference calls the destructor for the old value",
diff --git a/compiler/rustc_middle/src/ty/adt.rs b/compiler/rustc_middle/src/ty/adt.rs
@@ -241,7 +241,7 @@ impl AdtDefData {
         if Some(did) == tcx.lang_items().owned_box() {
             flags |= AdtFlags::IS_BOX;
         }
-        if Some(did) == tcx.lang_items().manually_drop() {
+        if Some(did) == tcx.lang_items().manually_drop() || tcx.has_attr(did, sym::manually_drop) {
             flags |= AdtFlags::IS_MANUALLY_DROP;
         }
         if Some(did) == tcx.lang_items().unsafe_cell_type() {
diff --git a/compiler/rustc_mir_dataflow/src/elaborate_drops.rs b/compiler/rustc_mir_dataflow/src/elaborate_drops.rs
@@ -443,8 +443,7 @@ where
             });
         }
 
-        let skip_contents =
-            adt.is_union() || Some(adt.did()) == self.tcx().lang_items().manually_drop();
+        let skip_contents = adt.is_union() || adt.is_manually_drop();
         let contents_drop = if skip_contents {
             (self.succ, self.unwind)
         } else {
diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs
@@ -193,6 +193,7 @@ impl CheckAttrVisitor<'_> {
                 sym::no_mangle => self.check_no_mangle(hir_id, attr, span, target),
                 sym::deprecated => self.check_deprecated(hir_id, attr, span, target),
                 sym::macro_use | sym::macro_escape => self.check_macro_use(hir_id, attr, target),
+                sym::manually_drop => self.check_manually_drop(hir_id, attr, span, target),
                 sym::path => self.check_generic_attr(hir_id, attr, target, Target::Mod),
                 sym::plugin_registrar => self.check_plugin_registrar(hir_id, attr, target),
                 sym::macro_export => self.check_macro_export(hir_id, attr, target),
@@ -2091,6 +2092,17 @@ impl CheckAttrVisitor<'_> {
         }
     }
 
+    fn check_manually_drop(&self, hir_id: HirId, attr: &Attribute, span: Span, target: Target) {
+        if !matches!(target, Target::Struct | Target::Enum) {
+            self.tcx.emit_spanned_lint(
+                UNUSED_ATTRIBUTES,
+                hir_id,
+                attr.span,
+                errors::ManuallyDropShouldBeAppliedToStructEnum { defn_span: span },
+            );
+        }
+    }
+
     fn check_plugin_registrar(&self, hir_id: HirId, attr: &Attribute, target: Target) {
         if target != Target::Fn {
             self.tcx.emit_spanned_lint(
@@ -2370,6 +2382,7 @@ fn check_invalid_crate_level_attr(tcx: TyCtxt<'_>, attrs: &[Attribute]) {
         sym::rustc_main,
         sym::unix_sigpipe,
         sym::derive,
+        sym::manually_drop,
         sym::test,
         sym::test_case,
         sym::global_allocator,
diff --git a/compiler/rustc_passes/src/errors.rs b/compiler/rustc_passes/src/errors.rs
@@ -639,6 +639,13 @@ pub struct MacroUse {
     pub name: Symbol,
 }
 
+#[derive(LintDiagnostic)]
+#[diag(passes_should_be_applied_to_struct_enum)]
+pub struct ManuallyDropShouldBeAppliedToStructEnum {
+    #[label]
+    pub defn_span: Span,
+}
+
 #[derive(LintDiagnostic)]
 #[diag(passes_macro_export)]
 pub struct MacroExport;
diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs
@@ -902,6 +902,7 @@ symbols! {
         main,
         managed_boxes,
         manually_drop,
+        manually_drop_attr,
         map,
         marker,
         marker_trait_attr,
diff --git a/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs b/compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs
@@ -49,8 +49,9 @@ pub fn trivial_dropck_outlives<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
         }
 
         ty::Adt(def, _) => {
-            if Some(def.did()) == tcx.lang_items().manually_drop() {
-                // `ManuallyDrop` never has a dtor.
+            if def.is_manually_drop() && !def.has_dtor(tcx) {
+                // A `#[manually_drop]` type without a Drop impl (e.g. `ManuallyDrop`)
+                // does not run any code at all when dropped.
                 true
             } else {
                 // Other types might. Moreover, PhantomData doesn't
diff --git a/compiler/rustc_ty_utils/src/needs_drop.rs b/compiler/rustc_ty_utils/src/needs_drop.rs
@@ -219,10 +219,7 @@ fn drop_tys_helper<'tcx>(
     }
 
     let adt_components = move |adt_def: ty::AdtDef<'tcx>, substs: SubstsRef<'tcx>| {
-        if adt_def.is_manually_drop() {
-            debug!("drop_tys_helper: `{:?}` is manually drop", adt_def);
-            Ok(Vec::new())
-        } else if let Some(dtor_info) = adt_has_dtor(adt_def) {
+        if let Some(dtor_info) = adt_has_dtor(adt_def) {
             match dtor_info {
                 DtorType::Significant => {
                     debug!("drop_tys_helper: `{:?}` implements `Drop`", adt_def);
@@ -237,6 +234,9 @@ fn drop_tys_helper<'tcx>(
                     Ok(substs.types().collect())
                 }
             }
+        } else if adt_def.is_manually_drop() {
+            debug!("drop_tys_helper: `{:?}` is manually drop", adt_def);
+            Ok(Vec::new())
         } else if adt_def.is_union() {
             debug!("drop_tys_helper: `{:?}` is a union", adt_def);
             Ok(Vec::new())
diff --git a/src/doc/unstable-book/src/language-features/manually-drop-attr.md b/src/doc/unstable-book/src/language-features/manually-drop-attr.md
@@ -0,0 +1,36 @@
+# `manually_drop_attr`
+
+The tracking issue for this feature is: [#100344]
+
+[#100344]: https://github.com/rust-lang/rust/issues/100344
+
+The `manually_drop_attr` feature enables the `#[manually_drop]` attribute, which disables the drop glue for the type it is applied to.
+
+For example, `std::mem::ManuallyDrop` is implemented as follows:
+
+```rs
+#[manually_drop]
+struct ManuallyDrop<T>(T);
+```
+
+But you can also use the attribute to change the order in which fields are dropped, by overriding `Drop`:
+
+```rs
+/// This struct changes the order in which `x` and `y` are dropped from the default.
+#[manually_drop]
+struct MyStruct {
+    x: String,
+    y: String,
+}
+
+impl Drop for MyStruct {
+    fn drop(&mut self) {
+        unsafe {
+            std::ptr::drop_in_place(&mut self.y);
+            std::ptr::drop_in_place(&mut self.x);
+        }
+    }
+}
+```
+
+This can be useful in combination with `repr(C)`, to decouple the layout from the destruction order. See MCP [#135](https://github.com/rust-lang/lang-team/issues/135).
diff --git a/src/test/ui/manually_drop_attr/feature-gate-manually_drop_attr.rs b/src/test/ui/manually_drop_attr/feature-gate-manually_drop_attr.rs
@@ -0,0 +1,5 @@
+#[manually_drop]
+//~^ ERROR the `#[manually_drop]` attribute is an experimental feature
+struct Foo {}
+
+fn main() {}
diff --git a/src/test/ui/manually_drop_attr/feature-gate-manually_drop_attr.stderr b/src/test/ui/manually_drop_attr/feature-gate-manually_drop_attr.stderr
@@ -0,0 +1,12 @@
+error[E0658]: the `#[manually_drop]` attribute is an experimental feature
+  --> $DIR/feature-gate-manually_drop_attr.rs:1:1
+   |
+LL | #[manually_drop]
+   | ^^^^^^^^^^^^^^^^
+   |
+   = note: see issue #100344 <https://github.com/rust-lang/rust/issues/100344> for more information
+   = help: add `#![feature(manually_drop_attr)]` to the crate attributes to enable
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0658`.
diff --git a/src/test/ui/manually_drop_attr/manually_drop-bad-item.rs b/src/test/ui/manually_drop_attr/manually_drop-bad-item.rs
@@ -0,0 +1,8 @@
+#![feature(manually_drop_attr)]
+#![forbid(unused_attributes)]
+
+#[manually_drop]
+//~^ ERROR attribute should be applied to a struct or enum
+fn foo() {}
+
+fn main() {}
diff --git a/src/test/ui/manually_drop_attr/manually_drop-bad-item.stderr b/src/test/ui/manually_drop_attr/manually_drop-bad-item.stderr
@@ -0,0 +1,17 @@
+error: attribute should be applied to a struct or enum
+  --> $DIR/manually_drop-bad-item.rs:4:1
+   |
+LL | #[manually_drop]
+   | ^^^^^^^^^^^^^^^^
+LL |
+LL | fn foo() {}
+   | ----------- not a struct or enum
+   |
+note: the lint level is defined here
+  --> $DIR/manually_drop-bad-item.rs:2:11
+   |
+LL | #![forbid(unused_attributes)]
+   |           ^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+
diff --git a/src/test/ui/manually_drop_attr/manually_drop-destructor.rs b/src/test/ui/manually_drop_attr/manually_drop-destructor.rs
@@ -0,0 +1,81 @@
+//! A test of `#[manually_drop]` on a type that *does* have a `Drop` impl.
+//!
+//! The mirror image of `manually_drop-nodestructor.rs`
+#![feature(manually_drop_attr)]
+// run-pass
+extern crate core;
+use core::cell::Cell;
+
+struct DropCounter<'a>(&'a Cell<isize>);
+impl<'a> Drop for DropCounter<'a> {
+    fn drop(&mut self) {
+        self.0.set(self.0.get() + 1);
+    }
+}
+
+#[manually_drop]
+struct ManuallyDropped<'a> {
+    field_1: DropCounter<'a>,
+    field_2: DropCounter<'a>,
+}
+
+impl<'a> Drop for ManuallyDropped<'a> {
+    fn drop(&mut self) {
+        // just do a LITTLE dropping.
+        unsafe {
+            core::ptr::drop_in_place(&mut self.field_1)
+        }
+    }
+}
+
+#[manually_drop]
+enum ManuallyDroppedEnum<'a> {
+    A(DropCounter<'a>, DropCounter<'a>),
+}
+
+impl<'a> Drop for ManuallyDroppedEnum<'a> {
+    fn drop(&mut self) {
+        // just do a LITTLE dropping.
+        let ManuallyDroppedEnum::A(a, _) = self;
+        unsafe {
+            core::ptr::drop_in_place(a);
+        }
+    }
+}
+
+/// Dropping a `#[manually_drop]` struct does not implicitly drop its fields.
+///
+/// (Though it does run `Drop`, which can choose to drop them explicitly.)
+fn test_destruction() {
+    let counter = Cell::new(0);
+    core::mem::drop(ManuallyDropped {
+        field_1: DropCounter(&counter),
+        field_2: DropCounter(&counter),
+    });
+    // We only run the drop specifically requested in the Drop impl.
+    assert_eq!(counter.get(), 1);
+    assert!(core::mem::needs_drop::<ManuallyDropped>());
+
+    core::mem::drop(ManuallyDroppedEnum::A(DropCounter(&counter), DropCounter(&counter)));
+    assert_eq!(counter.get(), 2);
+    assert!(core::mem::needs_drop::<ManuallyDroppedEnum>());
+
+}
+
+/// Assignment does still drop the fields.
+fn test_assignment() {
+    let counter = Cell::new(0);
+    let mut manually_dropped = ManuallyDropped {
+        field_1: DropCounter(&counter),
+        field_2: DropCounter(&counter),
+    };
+    assert_eq!(counter.get(), 0);
+    manually_dropped.field_1 = DropCounter(&counter);
+    manually_dropped.field_2 = DropCounter(&counter);
+    assert_eq!(counter.get(), 2);
+}
+
+fn main() {
+    test_destruction();
+    test_assignment();
+}
diff --git a/src/test/ui/manually_drop_attr/manually_drop-dropck.rs b/src/test/ui/manually_drop_attr/manually_drop-dropck.rs
@@ -0,0 +1,38 @@
+//! The drop checker only complains about a `#[manually_drop]` type if it _itself_ defines `Drop`.
+
+// FIXME: this does test dropck, does it also test needs_drop?
+
+#![feature(manually_drop_attr)]
+
+
+// For example, this is absolutely fine:
+
+#[manually_drop]
+struct ManuallyDrop<T>(T);
+
+fn drop_out_of_order_ok<T>(x: T) {
+    let mut manually_dropped = ManuallyDrop(None);
+    // x will be dropped before manually_dropped.
+    let x = x;
+    // ... but this is still fine, because it doesn't have logic on Drop.
+    manually_dropped.0 = Some(&x);
+}
+
+// ... but this is not:
+
+#[manually_drop]
+struct ManuallyDropWithDestructor<T>(T);
+impl<T> Drop for ManuallyDropWithDestructor<T> {
+    fn drop(&mut self) {
+        // maybe we read self.0 here!
+    }
+}
+
+fn drop_out_of_order_not_ok<T>(x: T) {
+    let mut manually_dropped_bad = ManuallyDropWithDestructor(None);
+    let x = x;
+    manually_dropped_bad.0 = Some(&x);
+    //~^ ERROR `x` does not live long enough
+}
+
+fn main() {}
diff --git a/src/test/ui/manually_drop_attr/manually_drop-dropck.stderr b/src/test/ui/manually_drop_attr/manually_drop-dropck.stderr
@@ -0,0 +1,17 @@
+error[E0597]: `x` does not live long enough
+  --> $DIR/manually_drop-dropck.rs:34:35
+   |
+LL |     manually_dropped_bad.0 = Some(&x);
+   |                                   ^^ borrowed value does not live long enough
+LL |
+LL | }
+   | -
+   | |
+   | `x` dropped here while still borrowed
+   | borrow might be used here, when `manually_dropped_bad` is dropped and runs the `Drop` code for type `ManuallyDropWithDestructor`
+   |
+   = note: values in a scope are dropped in the opposite order they are defined
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0597`.
diff --git a/src/test/ui/manually_drop_attr/manually_drop-in-union.rs b/src/test/ui/manually_drop_attr/manually_drop-in-union.rs
diff --git a/src/test/ui/manually_drop_attr/manually_drop-in-union.stderr b/src/test/ui/manually_drop_attr/manually_drop-in-union.stderr
diff --git a/src/test/ui/manually_drop_attr/manually_drop-nodestructor.rs b/src/test/ui/manually_drop_attr/manually_drop-nodestructor.rs
diff --git a/src/tools/clippy/clippy_lints/src/redundant_clone.rs b/src/tools/clippy/clippy_lints/src/redundant_clone.rs
diff --git a/tests/ui/union/union-deref.mirunsafeck.stderr b/tests/ui/union/union-deref.mirunsafeck.stderr
diff --git a/tests/ui/union/union-deref.rs b/tests/ui/union/union-deref.rs
diff --git a/tests/ui/union/union-deref.thirunsafeck.stderr b/tests/ui/union/union-deref.thirunsafeck.stderr

Original file line number	Diff line number	Diff line change
`@@ -112,8 +112,10 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b`
`112`	`112`	`allowed_union_field(*elem, tcx, param_env)`
`113`	`113`	`}`
`114`	`114`	`_ => {`
`115`		- // Fallback case: allow `ManuallyDrop` and things that are `Copy`.
`116`		`- ty.ty_adt_def().is_some_and(\|adt_def\| adt_def.is_manually_drop())`
	`115`	`+ // Fallback case: if there is no destructor (including field drop glue), because it is`
	`116`	+ // `Copy` or is `#[manually_drop]` with no `Drop`, then allow it.
	`117`	`+ ty.ty_adt_def()`
	`118`	`+ .is_some_and(\|adt_def\| adt_def.is_manually_drop() && !adt_def.has_dtor(tcx))`
`117`	`119`	`\|\| ty.is_copy_modulo_regions(tcx, param_env)`
`118`	`120`	`}`
`119`	`121`	`}`
Original file line number	Diff line number	Diff line change
`@@ -241,7 +241,7 @@ impl AdtDefData {`
`241`	`241`	`if Some(did) == tcx.lang_items().owned_box() {`
`242`	`242`	`flags \|= AdtFlags::IS_BOX;`
`243`	`243`	`}`
`244`		`- if Some(did) == tcx.lang_items().manually_drop() {`
	`244`	`+ if Some(did) == tcx.lang_items().manually_drop() \|\| tcx.has_attr(did, sym::manually_drop) {`
`245`	`245`	`flags \|= AdtFlags::IS_MANUALLY_DROP;`
`246`	`246`	`}`
`247`	`247`	`if Some(did) == tcx.lang_items().unsafe_cell_type() {`
Original file line number	Diff line number	Diff line change
`@@ -443,8 +443,7 @@ where`
`443`	`443`	`});`
`444`	`444`	`}`
`445`	`445`
`446`		`- let skip_contents =`
`447`		`- adt.is_union() \|\| Some(adt.did()) == self.tcx().lang_items().manually_drop();`
	`446`	`+ let skip_contents = adt.is_union() \|\| adt.is_manually_drop();`
`448`	`447`	`let contents_drop = if skip_contents {`
`449`	`448`	`(self.succ, self.unwind)`
`450`	`449`	`} else {`