Skip to content

Commit ace1ba4

Browse files
authored
Rollup merge of rust-lang#101586 - ssbr:master, r=lcnr
Add the `#[manually_drop]` attribute. 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, and is gated behind the feature flag `manually_drop_attr`. Some additional notes: ### The meaning of `#[manually_drop]` `#[manually_drop]` means "do not destroy the fields automatically during destruction". `ManuallyDrop`, could "just", morally speaking, be `#[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]`. ### Insignificant 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. ### Users that expect `adt.is_manually_drop()` <-> "is `std::mem::ManuallyDrop`" 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. ## To-do in future PRs 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...)
2 parents 63c748e + 46b6233 commit ace1ba4

File tree

28 files changed

+385
-31
lines changed

28 files changed

+385
-31
lines changed

compiler/rustc_feature/src/active.rs

+2
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,8 @@ declare_features! (
439439
(active, lint_reasons, "1.31.0", Some(54503), None),
440440
/// Give access to additional metadata about declarative macro meta-variables.
441441
(active, macro_metavar_expr, "1.61.0", Some(83527), None),
442+
/// Allows `#[manually_drop]` on type definitions.
443+
(active, manually_drop_attr, "1.64.0", Some(100344), None),
442444
/// Allows `#[marker]` on certain traits allowing overlapping implementations.
443445
(active, marker_trait_attr, "1.30.0", Some(29864), None),
444446
/// A minimal, sound subset of specialization intended to be used by the

compiler/rustc_feature/src/builtin_attrs.rs

+4
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
480480
deprecated_safe, Normal, template!(List: r#"since = "version", note = "...""#), ErrorFollowing,
481481
experimental!(deprecated_safe),
482482
),
483+
// lang-team MCP 135
484+
gated!(
485+
manually_drop, Normal, template!(Word), WarnFollowing, manually_drop_attr, experimental!(manually_drop),
486+
),
483487

484488
// `#[collapse_debuginfo]`
485489
gated!(

compiler/rustc_hir_analysis/src/check/check.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,10 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b
113113
allowed_union_field(*elem, tcx, param_env, span)
114114
}
115115
_ => {
116-
// Fallback case: allow `ManuallyDrop` and things that are `Copy`.
117-
ty.ty_adt_def().is_some_and(|adt_def| adt_def.is_manually_drop())
116+
// Fallback case: if there is no destructor (including field drop glue), because it is
117+
// `Copy` or is `#[manually_drop]` with no `Drop`, then allow it.
118+
ty.ty_adt_def()
119+
.is_some_and(|adt_def| adt_def.is_manually_drop() && !adt_def.has_dtor(tcx))
118120
|| ty.is_copy_modulo_regions(tcx, param_env)
119121
}
120122
}

compiler/rustc_hir_typeck/src/place_op.rs

+7-1
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
330330
}
331331
// If this is a union field, also throw an error for `DerefMut` of `ManuallyDrop` (see RFC 2514).
332332
// This helps avoid accidental drops.
333+
//
334+
// FIXME: This is not correct for `#[manually_drop]` in general, only when the struct is
335+
// a smart pointer whose pointee is at the same address, and whose pointee implements `Drop`.
336+
// Instead of checking for `#[manually_drop]`, this should likely be a more restricted check
337+
// for such types, or else union really should special-case and only permit `ManuallyDrop`, and
338+
// not `#[manually_drop]` types in general.
333339
if inside_union
334340
&& source.ty_adt_def().map_or(false, |adt| adt.is_manually_drop())
335341
{
336342
let mut err = self.tcx.sess.struct_span_err(
337343
expr.span,
338-
"not automatically applying `DerefMut` on `ManuallyDrop` union field",
344+
"not automatically applying `DerefMut` on manually dropped union field",
339345
);
340346
err.help(
341347
"writing to this reference calls the destructor for the old value",

compiler/rustc_middle/src/ty/adt.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ impl AdtDefData {
243243
if Some(did) == tcx.lang_items().owned_box() {
244244
flags |= AdtFlags::IS_BOX;
245245
}
246-
if Some(did) == tcx.lang_items().manually_drop() {
246+
if tcx.has_attr(did, sym::manually_drop) {
247247
flags |= AdtFlags::IS_MANUALLY_DROP;
248248
}
249249
if Some(did) == tcx.lang_items().unsafe_cell_type() {

compiler/rustc_mir_dataflow/src/elaborate_drops.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -443,8 +443,7 @@ where
443443
});
444444
}
445445

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();
448447
let contents_drop = if skip_contents {
449448
(self.succ, self.unwind)
450449
} else {

compiler/rustc_passes/src/check_attr.rs

+12
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ impl CheckAttrVisitor<'_> {
169169
sym::no_mangle => self.check_no_mangle(hir_id, attr, span, target),
170170
sym::deprecated => self.check_deprecated(hir_id, attr, span, target),
171171
sym::macro_use | sym::macro_escape => self.check_macro_use(hir_id, attr, target),
172+
sym::manually_drop => self.check_manually_drop(hir_id, attr, span, target),
172173
sym::path => self.check_generic_attr(hir_id, attr, target, Target::Mod),
173174
sym::plugin_registrar => self.check_plugin_registrar(hir_id, attr, target),
174175
sym::macro_export => self.check_macro_export(hir_id, attr, target),
@@ -2017,6 +2018,17 @@ impl CheckAttrVisitor<'_> {
20172018
}
20182019
}
20192020

2021+
fn check_manually_drop(&self, hir_id: HirId, attr: &Attribute, span: Span, target: Target) {
2022+
if !matches!(target, Target::Struct | Target::Enum) {
2023+
self.tcx.emit_spanned_lint(
2024+
UNUSED_ATTRIBUTES,
2025+
hir_id,
2026+
attr.span,
2027+
errors::ManuallyDropShouldBeAppliedToStructEnum { defn_span: span },
2028+
);
2029+
}
2030+
}
2031+
20202032
fn check_plugin_registrar(&self, hir_id: HirId, attr: &Attribute, target: Target) {
20212033
if target != Target::Fn {
20222034
self.tcx.emit_spanned_lint(

compiler/rustc_passes/src/errors.rs

+7
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,13 @@ pub struct MacroUse {
603603
pub name: Symbol,
604604
}
605605

606+
#[derive(LintDiagnostic)]
607+
#[diag(passes_should_be_applied_to_struct_enum)]
608+
pub struct ManuallyDropShouldBeAppliedToStructEnum {
609+
#[label]
610+
pub defn_span: Span,
611+
}
612+
606613
#[derive(LintDiagnostic)]
607614
#[diag(passes_macro_export)]
608615
pub struct MacroExport;

compiler/rustc_span/src/symbol.rs

+1
Original file line numberDiff line numberDiff line change
@@ -887,6 +887,7 @@ symbols! {
887887
main,
888888
managed_boxes,
889889
manually_drop,
890+
manually_drop_attr,
890891
map,
891892
marker,
892893
marker_trait_attr,

compiler/rustc_trait_selection/src/traits/query/dropck_outlives.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ pub fn trivial_dropck_outlives<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
4848
}
4949

5050
ty::Adt(def, _) => {
51-
if Some(def.did()) == tcx.lang_items().manually_drop() {
52-
// `ManuallyDrop` never has a dtor.
51+
if def.is_manually_drop() && !def.has_dtor(tcx) {
52+
// A `#[manually_drop]` type without a Drop impl (e.g. `ManuallyDrop`)
53+
// does not run any code at all when dropped.
5354
true
5455
} else {
5556
// Other types might. Moreover, PhantomData doesn't

compiler/rustc_ty_utils/src/needs_drop.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -212,10 +212,7 @@ fn drop_tys_helper<'tcx>(
212212
}
213213

214214
let adt_components = move |adt_def: ty::AdtDef<'tcx>, substs: SubstsRef<'tcx>| {
215-
if adt_def.is_manually_drop() {
216-
debug!("drop_tys_helper: `{:?}` is manually drop", adt_def);
217-
Ok(Vec::new())
218-
} else if let Some(dtor_info) = adt_has_dtor(adt_def) {
215+
if let Some(dtor_info) = adt_has_dtor(adt_def) {
219216
match dtor_info {
220217
DtorType::Significant => {
221218
debug!("drop_tys_helper: `{:?}` implements `Drop`", adt_def);
@@ -230,6 +227,9 @@ fn drop_tys_helper<'tcx>(
230227
Ok(substs.types().collect())
231228
}
232229
}
230+
} else if adt_def.is_manually_drop() {
231+
debug!("drop_tys_helper: `{:?}` is manually drop", adt_def);
232+
Ok(Vec::new())
233233
} else if adt_def.is_union() {
234234
debug!("drop_tys_helper: `{:?}` is a union", adt_def);
235235
Ok(Vec::new())

library/core/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@
200200
#![feature(lang_items)]
201201
#![feature(link_llvm_intrinsics)]
202202
#![feature(macro_metavar_expr)]
203+
#![cfg_attr(not(bootstrap), feature(manually_drop_attr))]
203204
#![feature(min_specialization)]
204205
#![feature(must_not_suspend)]
205206
#![feature(negative_impls)]

library/core/src/mem/manually_drop.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ use crate::ptr;
4444
/// [`mem::zeroed`]: crate::mem::zeroed
4545
/// [`MaybeUninit<T>`]: crate::mem::MaybeUninit
4646
#[stable(feature = "manually_drop", since = "1.20.0")]
47-
#[lang = "manually_drop"]
47+
#[cfg_attr(bootstrap, lang = "manually_drop")]
48+
#[cfg_attr(not(bootstrap), manually_drop)]
4849
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
4950
#[repr(transparent)]
5051
pub struct ManuallyDrop<T: ?Sized> {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# `manually_drop_attr`
2+
3+
The tracking issue for this feature is: [#100344]
4+
5+
[#100344]: https://github.com/rust-lang/rust/issues/100344
6+
7+
The `manually_drop_attr` feature enables the `#[manually_drop]` attribute, which disables the drop glue for the type it is applied to.
8+
9+
For example, `std::mem::ManuallyDrop` is implemented as follows:
10+
11+
```rs
12+
#[manually_drop]
13+
struct ManuallyDrop<T>(T);
14+
```
15+
16+
But you can also use the attribute to change the order in which fields are dropped, by overriding `Drop`:
17+
18+
```rs
19+
/// This struct changes the order in which `x` and `y` are dropped from the default.
20+
#[manually_drop]
21+
struct MyStruct {
22+
x: String,
23+
y: String,
24+
}
25+
26+
impl Drop for MyStruct {
27+
fn drop(&mut self) {
28+
unsafe {
29+
std::ptr::drop_in_place(&mut self.y);
30+
std::ptr::drop_in_place(&mut self.x);
31+
}
32+
}
33+
}
34+
```
35+
36+
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 numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#[manually_drop]
2+
//~^ ERROR the `#[manually_drop]` attribute is an experimental feature
3+
struct Foo {}
4+
5+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
error[E0658]: the `#[manually_drop]` attribute is an experimental feature
2+
--> $DIR/feature-gate-manually_drop_attr.rs:1:1
3+
|
4+
LL | #[manually_drop]
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: see issue #100344 <https://github.com/rust-lang/rust/issues/100344> for more information
8+
= help: add `#![feature(manually_drop_attr)]` to the crate attributes to enable
9+
10+
error: aborting due to previous error
11+
12+
For more information about this error, try `rustc --explain E0658`.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#![feature(manually_drop_attr)]
2+
#![forbid(unused_attributes)]
3+
#![manually_drop]
4+
//~^ ERROR attribute should be applied to a struct or enum
5+
6+
#[manually_drop]
7+
//~^ ERROR attribute should be applied to a struct or enum
8+
fn foo() {}
9+
10+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
error: attribute should be applied to a struct or enum
2+
--> $DIR/manually_drop-bad-item.rs:6:1
3+
|
4+
LL | #[manually_drop]
5+
| ^^^^^^^^^^^^^^^^
6+
LL |
7+
LL | fn foo() {}
8+
| ----------- not a struct or enum
9+
|
10+
note: the lint level is defined here
11+
--> $DIR/manually_drop-bad-item.rs:2:11
12+
|
13+
LL | #![forbid(unused_attributes)]
14+
| ^^^^^^^^^^^^^^^^^
15+
16+
error: attribute should be applied to a struct or enum
17+
--> $DIR/manually_drop-bad-item.rs:3:1
18+
|
19+
LL | #![manually_drop]
20+
| ^^^^^^^^^^^^^^^^^
21+
22+
error: aborting due to 2 previous errors
23+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
//! A test of `#[manually_drop]` on a type that *does* have a `Drop` impl.
2+
//!
3+
//! The mirror image of `manually_drop-nodestructor.rs`
4+
#![feature(manually_drop_attr)]
5+
// run-pass
6+
extern crate core;
7+
use core::cell::Cell;
8+
9+
struct DropCounter<'a>(&'a Cell<isize>);
10+
impl<'a> Drop for DropCounter<'a> {
11+
fn drop(&mut self) {
12+
self.0.set(self.0.get() + 1);
13+
}
14+
}
15+
16+
#[manually_drop]
17+
struct ManuallyDropped<'a> {
18+
field_1: DropCounter<'a>,
19+
field_2: DropCounter<'a>,
20+
}
21+
22+
impl<'a> Drop for ManuallyDropped<'a> {
23+
fn drop(&mut self) {
24+
// just do a LITTLE dropping.
25+
unsafe {
26+
core::ptr::drop_in_place(&mut self.field_1)
27+
}
28+
}
29+
}
30+
31+
#[manually_drop]
32+
enum ManuallyDroppedEnum<'a> {
33+
_A,
34+
B(DropCounter<'a>, DropCounter<'a>),
35+
}
36+
37+
impl<'a> Drop for ManuallyDroppedEnum<'a> {
38+
fn drop(&mut self) {
39+
// just do a LITTLE dropping.
40+
if let ManuallyDroppedEnum::B(a, _) = self {
41+
unsafe {
42+
core::ptr::drop_in_place(a);
43+
}
44+
}
45+
}
46+
}
47+
48+
/// Dropping a `#[manually_drop]` struct does not implicitly drop its fields.
49+
///
50+
/// (Though it does run `Drop`, which can choose to drop them explicitly.)
51+
fn test_destruction() {
52+
let counter = Cell::new(0);
53+
core::mem::drop(ManuallyDropped {
54+
field_1: DropCounter(&counter),
55+
field_2: DropCounter(&counter),
56+
});
57+
// We only run the drop specifically requested in the Drop impl.
58+
assert_eq!(counter.get(), 1);
59+
assert!(core::mem::needs_drop::<ManuallyDropped>());
60+
61+
core::mem::drop(ManuallyDroppedEnum::B(DropCounter(&counter), DropCounter(&counter)));
62+
assert_eq!(counter.get(), 2);
63+
assert!(core::mem::needs_drop::<ManuallyDroppedEnum>());
64+
65+
}
66+
67+
/// Assignment does still drop the fields.
68+
fn test_assignment() {
69+
let counter = Cell::new(0);
70+
let mut manually_dropped = ManuallyDropped {
71+
field_1: DropCounter(&counter),
72+
field_2: DropCounter(&counter),
73+
};
74+
assert_eq!(counter.get(), 0);
75+
manually_dropped.field_1 = DropCounter(&counter);
76+
manually_dropped.field_2 = DropCounter(&counter);
77+
assert_eq!(counter.get(), 2);
78+
}
79+
80+
fn main() {
81+
test_destruction();
82+
test_assignment();
83+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
//! The drop checker only complains about a `#[manually_drop]` type if it _itself_ defines `Drop`.
2+
3+
// FIXME: this does test dropck, does it also test needs_drop?
4+
5+
#![feature(manually_drop_attr)]
6+
7+
8+
// For example, this is absolutely fine:
9+
10+
#[manually_drop]
11+
struct ManuallyDrop<T>(T);
12+
13+
fn drop_out_of_order_ok<T>(x: T) {
14+
let mut manually_dropped = ManuallyDrop(None);
15+
// x will be dropped before manually_dropped.
16+
let x = x;
17+
// ... but this is still fine, because it doesn't have logic on Drop.
18+
manually_dropped.0 = Some(&x);
19+
}
20+
21+
// ... but this is not:
22+
23+
#[manually_drop]
24+
struct ManuallyDropWithDestructor<T>(T);
25+
impl<T> Drop for ManuallyDropWithDestructor<T> {
26+
fn drop(&mut self) {
27+
// maybe we read self.0 here!
28+
}
29+
}
30+
31+
fn drop_out_of_order_not_ok<T>(x: T) {
32+
let mut manually_dropped_bad = ManuallyDropWithDestructor(None);
33+
let x = x;
34+
manually_dropped_bad.0 = Some(&x);
35+
//~^ ERROR `x` does not live long enough
36+
}
37+
38+
fn main() {}

0 commit comments

Comments
 (0)