Skip to content

Commit

Permalink
Add the #[manually_drop] attribute.
Browse files Browse the repository at this point in the history
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 rust-lang#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...)
  • Loading branch information
ssbr committed Nov 15, 2022
1 parent a00f8ba commit 416d642
Show file tree
Hide file tree
Showing 26 changed files with 372 additions and 30 deletions.
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,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
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,10 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b
allowed_union_field(*elem, tcx, param_env, span)
}
_ => {
// 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)
}
}
Expand Down
8 changes: 7 additions & 1 deletion compiler/rustc_hir_typeck/src/place_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,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() {
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_mir_dataflow/src/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,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),
Expand Down Expand Up @@ -2017,6 +2018,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(
Expand Down Expand Up @@ -2195,6 +2207,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,
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,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;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,7 @@ symbols! {
main,
managed_boxes,
manually_drop,
manually_drop_attr,
map,
marker,
marker_trait_attr,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,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
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_ty_utils/src/needs_drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,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);
Expand All @@ -230,6 +227,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())
Expand Down
36 changes: 36 additions & 0 deletions src/doc/unstable-book/src/language-features/manually-drop-attr.md
Original file line number Diff line number Diff line change
@@ -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).
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#[manually_drop]
//~^ ERROR the `#[manually_drop]` attribute is an experimental feature
struct Foo {}

fn main() {}
Original file line number Diff line number Diff line change
@@ -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`.
8 changes: 8 additions & 0 deletions src/test/ui/manually_drop_attr/manually_drop-bad-item.rs
Original file line number Diff line number Diff line change
@@ -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() {}
17 changes: 17 additions & 0 deletions src/test/ui/manually_drop_attr/manually_drop-bad-item.stderr
Original file line number Diff line number Diff line change
@@ -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

81 changes: 81 additions & 0 deletions src/test/ui/manually_drop_attr/manually_drop-destructor.rs
Original file line number Diff line number Diff line change
@@ -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();
}
38 changes: 38 additions & 0 deletions src/test/ui/manually_drop_attr/manually_drop-dropck.rs
Original file line number Diff line number Diff line change
@@ -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() {}
17 changes: 17 additions & 0 deletions src/test/ui/manually_drop_attr/manually_drop-dropck.stderr
Original file line number Diff line number Diff line change
@@ -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`.

0 comments on commit 416d642

Please sign in to comment.