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

Lines changed: 2 additions & 0 deletions
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

Lines changed: 4 additions & 0 deletions
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

Lines changed: 4 additions & 2 deletions
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

Lines changed: 7 additions & 1 deletion
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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 1 addition & 2 deletions
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

Lines changed: 12 additions & 0 deletions
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

Lines changed: 7 additions & 0 deletions
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

Lines changed: 1 addition & 0 deletions
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

Lines changed: 3 additions & 2 deletions
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

0 commit comments

Comments
 (0)