add new attribute rustc_insignificant_dtor and a query to check if a type has a significant drop

Author: null-sleep

compiler/rustc_feature/src/active.rs

@@ -700,6 +700,7 @@ pub const INCOMPLETE_FEATURES: &[Symbol] = &[
     sym::native_link_modifiers_verbatim,
     sym::native_link_modifiers_whole_archive,
     sym::native_link_modifiers_as_needed,
+    sym::rustc_insignificant_dtor,
 ];
 
 /// Some features are not allowed to be used together at the same time, if

compiler/rustc_feature/src/builtin_attrs.rs

@@ -556,6 +556,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
 
     rustc_attr!(TEST, rustc_outlives, Normal, template!(Word)),
     rustc_attr!(TEST, rustc_capture_analysis, Normal, template!(Word)),
+    rustc_attr!(TEST, rustc_insignificant_dtor, Normal, template!(Word)),
     rustc_attr!(TEST, rustc_variance, Normal, template!(Word)),
     rustc_attr!(TEST, rustc_layout, Normal, template!(List: "field1, field2, ...")),
     rustc_attr!(TEST, rustc_regions, Normal, template!(Word)),

compiler/rustc_middle/src/query/mod.rs

@@ -1035,6 +1035,10 @@ rustc_queries! {
     query needs_drop_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
         desc { "computing whether `{}` needs drop", env.value }
     }
+    /// Query backing `TyS::has_significant_drop_raw`.
+    query has_significant_drop_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
+        desc { "computing whether `{}` has a significant drop", env.value }
+    }
 
     /// Query backing `TyS::is_structural_eq_shallow`.
     ///
@@ -1055,6 +1059,17 @@ rustc_queries! {
         cache_on_disk_if { true }
     }
 
+    /// A list of types where the ADT requires drop if and only if any of those types
+    /// has significant drop. A type marked with the attribute `rustc_insignificant_dtor`
+    /// is considered to not be significant. A drop is significant if it is implemented
+    /// by the user or does anything that will have any observable behavior (other than
+    /// freeing up memory). If the ADT is known to have a significant destructor then
+    /// `Err(AlwaysRequiresDrop)` is returned.
+    query adt_significant_drop_tys(def_id: DefId) -> Result<&'tcx ty::List<Ty<'tcx>>, AlwaysRequiresDrop> {
+        desc { |tcx| "computing when `{}` has a significant destructor", tcx.def_path_str(def_id) }
+        cache_on_disk_if { false }
+    }
+
     query layout_raw(
         env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>
     ) -> Result<&'tcx rustc_target::abi::Layout, ty::layout::LayoutError<'tcx>> {

compiler/rustc_middle/src/ty/util.rs

@@ -791,6 +791,39 @@ impl<'tcx> ty::TyS<'tcx> {
         }
     }
 
+    /// Checks if `ty` has has a significant drop.
+    ///
+    /// Note that this method can return false even if `ty` has a destructor
+    /// attached; even if that is the case then the adt has been marked with
+    /// the attribute `rustc_insignificant_dtor`.
+    ///
+    /// Note that this method is used to check for change in drop order for
+    /// 2229 drop reorder migration analysis.
+    #[inline]
+    pub fn has_significant_drop(
+        &'tcx self,
+        tcx: TyCtxt<'tcx>,
+        param_env: ty::ParamEnv<'tcx>,
+    ) -> bool {
+        // Avoid querying in simple cases.
+        match needs_drop_components(self, &tcx.data_layout) {
+            Err(AlwaysRequiresDrop) => true,
+            Ok(components) => {
+                let query_ty = match *components {
+                    [] => return false,
+                    // If we've got a single component, call the query with that
+                    // to increase the chance that we hit the query cache.
+                    [component_ty] => component_ty,
+                    _ => self,
+                };
+                // This doesn't depend on regions, so try to minimize distinct
+                // query keys used.
+                let erased = tcx.normalize_erasing_regions(param_env, query_ty);
+                tcx.has_significant_drop_raw(param_env.and(erased))
+            }
+        }
+    }
+
     /// Returns `true` if equality for this type is both reflexive and structural.
     ///
     /// Reflexive equality for a type is indicated by an `Eq` impl for that type.

compiler/rustc_span/src/symbol.rs

@@ -1015,6 +1015,7 @@ symbols! {
         rustc_expected_cgu_reuse,
         rustc_if_this_changed,
         rustc_inherit_overflow_checks,
+        rustc_insignificant_dtor,
         rustc_layout,
         rustc_layout_scalar_valid_range_end,
         rustc_layout_scalar_valid_range_start,

compiler/rustc_ty_utils/src/needs_drop.rs

@@ -6,7 +6,7 @@ use rustc_middle::ty::subst::Subst;
 use rustc_middle::ty::util::{needs_drop_components, AlwaysRequiresDrop};
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_session::Limit;
-use rustc_span::DUMMY_SP;
+use rustc_span::{sym, DUMMY_SP};
 
 type NeedsDropResult<T> = Result<T, AlwaysRequiresDrop>;
 
@@ -21,6 +21,19 @@ fn needs_drop_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>
     res
 }
 
+fn has_significant_drop_raw<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>,
+) -> bool {
+    let significant_drop_fields =
+        move |adt_def: &ty::AdtDef| tcx.adt_significant_drop_tys(adt_def.did).map(|tys| tys.iter());
+    let res = NeedsDropTypes::new(tcx, query.param_env, query.value, significant_drop_fields)
+        .next()
+        .is_some();
+    debug!("has_significant_drop_raw({:?}) = {:?}", query, res);
+    res
+}
+
 struct NeedsDropTypes<'tcx, F> {
     tcx: TyCtxt<'tcx>,
     param_env: ty::ParamEnv<'tcx>,
@@ -155,12 +168,20 @@ where
     }
 }
 
-fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List<Ty<'_>>, AlwaysRequiresDrop> {
+// This is a helper function for `adt_drop_tys` and `adt_significant_drop_tys`.
+// Depending on the implentation of `adt_has_dtor`, it is used to check if the
+// ADT has a destructor or if the ADT only has a significant destructor. For
+// understanding significant destructor look at `adt_significant_drop_tys`.
+fn adt_drop_tys_helper(
+    tcx: TyCtxt<'_>,
+    def_id: DefId,
+    adt_has_dtor: impl Fn(&ty::AdtDef) -> bool,
+) -> Result<&ty::List<Ty<'_>>, AlwaysRequiresDrop> {
     let adt_components = move |adt_def: &ty::AdtDef| {
         if adt_def.is_manually_drop() {
             debug!("adt_drop_tys: `{:?}` is manually drop", adt_def);
             return Ok(Vec::new().into_iter());
-        } else if adt_def.destructor(tcx).is_some() {
+        } else if adt_has_dtor(adt_def) {
             debug!("adt_drop_tys: `{:?}` implements `Drop`", adt_def);
             return Err(AlwaysRequiresDrop);
         } else if adt_def.is_union() {
@@ -179,6 +200,30 @@ fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List<Ty<'_>>, Alw
     res.map(|components| tcx.intern_type_list(&components))
 }
 
+fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List<Ty<'_>>, AlwaysRequiresDrop> {
+    let adt_has_dtor = |adt_def: &ty::AdtDef| adt_def.destructor(tcx).is_some();
+    adt_drop_tys_helper(tcx, def_id, adt_has_dtor)
+}
+
+fn adt_significant_drop_tys(
+    tcx: TyCtxt<'_>,
+    def_id: DefId,
+) -> Result<&ty::List<Ty<'_>>, AlwaysRequiresDrop> {
+    let adt_has_dtor = |adt_def: &ty::AdtDef| {
+        adt_def
+            .destructor(tcx)
+            .map(|dtor| !tcx.has_attr(dtor.did, sym::rustc_insignificant_dtor))
+            .unwrap_or(false)
+    };
+    adt_drop_tys_helper(tcx, def_id, adt_has_dtor)
+}
+
 pub(crate) fn provide(providers: &mut ty::query::Providers) {
-    *providers = ty::query::Providers { needs_drop_raw, adt_drop_tys, ..*providers };
+    *providers = ty::query::Providers {
+        needs_drop_raw,
+        has_significant_drop_raw,
+        adt_drop_tys,
+        adt_significant_drop_tys,
+        ..*providers
+    };
 }

compiler/rustc_typeck/src/check/upvar.rs

@@ -706,6 +706,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///   enabled, **and**
     /// - It wasn't completely captured by the closure, **and**
     /// - One of the paths starting at this root variable, that is not captured needs Drop.
+    ///
+    /// This function only returns true for significant drops. A type is considerent to have a
+    /// significant drop if it's Drop implementation is not annotated by `rustc_insignificant_dtor`.
     fn compute_2229_migrations_for_drop(
         &self,
         closure_def_id: DefId,
@@ -716,7 +719,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ) -> bool {
         let ty = self.infcx.resolve_vars_if_possible(self.node_ty(var_hir_id));
 
-        if !ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local())) {
+        if !ty.has_significant_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local())) {
             return false;
         }
 
@@ -835,11 +838,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// using list of `Projection` slices), it returns true if there is a path that is not
     /// captured starting at this root variable that implements Drop.
     ///
-    /// FIXME(project-rfc-2229#35): This should return true only for significant drops.
-    ///                             A drop is significant if it's implemented by the user or does
-    ///                             anything that will have any observable behavior (other than
-    ///                             freeing up memory).
-    ///
     /// The way this function works is at a given call it looks at type `base_path_ty` of some base
     /// path say P and then list of projection slices which represent the different captures moved
     /// into the closure starting off of P.
@@ -895,7 +893,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///     (Ty((w.p).x), [ &[] ])     (Ty((w.p).y), []) // IMP 2
     ///          |                             |
     ///          v                             v
-    ///        false                     NeedsDrop(Ty(w.p.y))
+    ///        false              NeedsSignificantDrop(Ty(w.p.y))
     ///                                        |
     ///                                        v
     ///                                      true
@@ -939,7 +937,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         captured_by_move_projs: Vec<&[Projection<'tcx>]>,
     ) -> bool {
         let needs_drop = |ty: Ty<'tcx>| {
-            ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local()))
+            ty.has_significant_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local()))
         };
 
         let is_drop_defined_for_ty = |ty: Ty<'tcx>| {

src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.fixed

@@ -0,0 +1,67 @@
+// run-rustfix
+
+#![deny(disjoint_capture_migration)]
+//~^ NOTE: the lint level is defined here
+
+#![feature(rustc_attrs)]
+#![allow(unused)]
+
+struct InsignificantDropPoint {
+    x: i32,
+    y: i32,
+}
+
+impl Drop for InsignificantDropPoint {
+    #[rustc_insignificant_dtor]
+    fn drop(&mut self) {}
+}
+
+struct SigDrop;
+
+impl Drop for SigDrop {
+    fn drop(&mut self) {}
+}
+
+struct GenericStruct<T>(T, T);
+
+struct Wrapper<T>(GenericStruct<T>, i32);
+
+impl<T> Drop for GenericStruct<T> {
+    #[rustc_insignificant_dtor]
+    fn drop(&mut self) {}
+}
+
+// `SigDrop` implements drop and therefore needs to be migrated.
+fn significant_drop_needs_migration() {
+    let t = (SigDrop {}, SigDrop {});
+
+    let c = || { let _ = &t; 
+    //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| HELP: add a dummy let to cause `t` to be fully captured
+        let _t = t.0;
+    };
+
+    c();
+}
+
+// Even if a type implements an insignificant drop, if it's
+// elements have a significant drop then the overall type is
+// consdered to have an significant drop. Since the elements
+// of `GenericStruct` implement drop, migration is required.
+fn generic_struct_with_significant_drop_needs_migration() {
+    let t = Wrapper(GenericStruct(SigDrop {}, SigDrop {}), 5);
+
+    // move is used to force i32 to be copied instead of being a ref
+    let c = move || { let _ = &t; 
+    //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| HELP: add a dummy let to cause `t` to be fully captured
+        let _t = t.1;
+    };
+
+    c();
+}
+
+fn main() {
+    significant_drop_needs_migration();
+    generic_struct_with_significant_drop_needs_migration();
+}

src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.rs

@@ -0,0 +1,67 @@
+// run-rustfix
+
+#![deny(disjoint_capture_migration)]
+//~^ NOTE: the lint level is defined here
+
+#![feature(rustc_attrs)]
+#![allow(unused)]
+
+struct InsignificantDropPoint {
+    x: i32,
+    y: i32,
+}
+
+impl Drop for InsignificantDropPoint {
+    #[rustc_insignificant_dtor]
+    fn drop(&mut self) {}
+}
+
+struct SigDrop;
+
+impl Drop for SigDrop {
+    fn drop(&mut self) {}
+}
+
+struct GenericStruct<T>(T, T);
+
+struct Wrapper<T>(GenericStruct<T>, i32);
+
+impl<T> Drop for GenericStruct<T> {
+    #[rustc_insignificant_dtor]
+    fn drop(&mut self) {}
+}
+
+// `SigDrop` implements drop and therefore needs to be migrated.
+fn significant_drop_needs_migration() {
+    let t = (SigDrop {}, SigDrop {});
+
+    let c = || {
+    //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| HELP: add a dummy let to cause `t` to be fully captured
+        let _t = t.0;
+    };
+
+    c();
+}
+
+// Even if a type implements an insignificant drop, if it's
+// elements have a significant drop then the overall type is
+// consdered to have an significant drop. Since the elements
+// of `GenericStruct` implement drop, migration is required.
+fn generic_struct_with_significant_drop_needs_migration() {
+    let t = Wrapper(GenericStruct(SigDrop {}, SigDrop {}), 5);
+
+    // move is used to force i32 to be copied instead of being a ref
+    let c = move || {
+    //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields`
+    //~| HELP: add a dummy let to cause `t` to be fully captured
+        let _t = t.1;
+    };
+
+    c();
+}
+
+fn main() {
+    significant_drop_needs_migration();
+    generic_struct_with_significant_drop_needs_migration();
+}