From a7e1cec621d751b1c10b00b45c5b228df1b7d46d Mon Sep 17 00:00:00 2001 From: Dhruv Jauhar Date: Tue, 13 Apr 2021 03:43:11 -0400 Subject: [PATCH] add new attribute rustc_insignificant_dtor and a query to check if a type has a significant drop --- compiler/rustc_feature/src/active.rs | 1 + compiler/rustc_feature/src/builtin_attrs.rs | 1 + compiler/rustc_middle/src/query/mod.rs | 15 +++++ compiler/rustc_middle/src/ty/util.rs | 33 +++++++++ compiler/rustc_span/src/symbol.rs | 1 + compiler/rustc_ty_utils/src/needs_drop.rs | 53 +++++++++++++-- compiler/rustc_typeck/src/check/upvar.rs | 14 ++-- .../insignificant_drop_attr_migrations.fixed | 67 +++++++++++++++++++ .../insignificant_drop_attr_migrations.rs | 67 +++++++++++++++++++ .../insignificant_drop_attr_migrations.stderr | 47 +++++++++++++ .../insignificant_drop_attr_no_migrations.rs | 45 +++++++++++++ 11 files changed, 332 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.fixed create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.rs create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.stderr create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_no_migrations.rs diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs index 535cb13276646..9d96a9baa50c8 100644 --- a/compiler/rustc_feature/src/active.rs +++ b/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 diff --git a/compiler/rustc_feature/src/builtin_attrs.rs b/compiler/rustc_feature/src/builtin_attrs.rs index 51d69167f7b3e..e7e128f8a9b21 100644 --- a/compiler/rustc_feature/src/builtin_attrs.rs +++ b/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)), diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index fea2aec38a167..dfb67d92521ca 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/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>, 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>> { diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index e365928c15f91..7bf69b9e637e9 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/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. diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 0b3dece09aecc..c9816c2d59913 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/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, diff --git a/compiler/rustc_ty_utils/src/needs_drop.rs b/compiler/rustc_ty_utils/src/needs_drop.rs index 64f82817d3944..bc8f10e15db8c 100644 --- a/compiler/rustc_ty_utils/src/needs_drop.rs +++ b/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 = Result; @@ -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>, 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>, 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>, Alw res.map(|components| tcx.intern_type_list(&components)) } +fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List>, 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>, 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 + }; } diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 7a2b5b26ef437..ff506ef8727b9 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/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>| { diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.fixed b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.fixed new file mode 100644 index 0000000000000..e89cc2c8fb361 --- /dev/null +++ b/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); + +struct Wrapper(GenericStruct, i32); + +impl Drop for GenericStruct { + #[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(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.rs b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.rs new file mode 100644 index 0000000000000..e16cd9d52b78c --- /dev/null +++ b/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); + +struct Wrapper(GenericStruct, i32); + +impl Drop for GenericStruct { + #[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(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.stderr new file mode 100644 index 0000000000000..2b141656be2a8 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_migrations.stderr @@ -0,0 +1,47 @@ +error: drop order affected for closure because of `capture_disjoint_fields` + --> $DIR/insignificant_drop_attr_migrations.rs:38:13 + | +LL | let c = || { + | _____________^ +LL | | +LL | | +LL | | let _t = t.0; +LL | | }; + | |_____^ + | +note: the lint level is defined here + --> $DIR/insignificant_drop_attr_migrations.rs:3:9 + | +LL | #![deny(disjoint_capture_migration)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | }; + | + +error: drop order affected for closure because of `capture_disjoint_fields` + --> $DIR/insignificant_drop_attr_migrations.rs:55:13 + | +LL | let c = move || { + | _____________^ +LL | | +LL | | +LL | | let _t = t.1; +LL | | }; + | |_____^ + | +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = move || { let _ = &t; +LL | +LL | +LL | let _t = t.1; +LL | }; + | + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_no_migrations.rs b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_no_migrations.rs new file mode 100644 index 0000000000000..a00377456ac8d --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop_attr_no_migrations.rs @@ -0,0 +1,45 @@ +// run-pass + +#![deny(disjoint_capture_migration)] +#![feature(rustc_attrs)] +#![allow(unused)] + +struct InsignificantDropPoint { + x: i32, + y: i32, +} + +impl Drop for InsignificantDropPoint { + #[rustc_insignificant_dtor] + fn drop(&mut self) {} +} + +struct GenericStruct(T, T); + +// No drop reordering is required as the elements of `t` implement insignificant drop +fn insignificant_drop_does_not_need_migration() { + let t = (InsignificantDropPoint { x: 4, y: 9 }, InsignificantDropPoint { x: 4, y: 9 }); + + let c = || { + let _t = t.0; + }; + + c(); +} + +// Generic struct whose elements don't have significant drops don't need drop reordering +fn generic_struct_with_insignificant_drop_does_not_need_migration() { + let t = + GenericStruct(InsignificantDropPoint { x: 4, y: 9 }, InsignificantDropPoint { x: 4, y: 9 }); + + let c = || { + let _t = t.0; + }; + + c(); +} + +fn main() { + insignificant_drop_does_not_need_migration(); + generic_struct_with_insignificant_drop_does_not_need_migration(); +}