From 9158fc2071d8b6424ca56bcebc1cf21d264e9d7d Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Sat, 23 Oct 2021 06:47:17 -0400 Subject: [PATCH 1/3] Fixes incorrect handling of ADT's drop requirements See https://github.com/rust-lang/rust/issues/90024#issuecomment-950105433 --- compiler/rustc_ty_utils/src/needs_drop.rs | 95 +++++++++++++---------- 1 file changed, 54 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_ty_utils/src/needs_drop.rs b/compiler/rustc_ty_utils/src/needs_drop.rs index 98415a84c56..dca93aff0cd 100644 --- a/compiler/rustc_ty_utils/src/needs_drop.rs +++ b/compiler/rustc_ty_utils/src/needs_drop.rs @@ -12,14 +12,12 @@ use rustc_span::{sym, DUMMY_SP}; type NeedsDropResult = Result; fn needs_drop_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool { - let adt_components = - move |adt_def: &ty::AdtDef, _| tcx.adt_drop_tys(adt_def.did).map(|tys| tys.iter()); - // If we don't know a type doesn't need drop, for example if it's a type // parameter without a `Copy` bound, then we conservatively return that it // needs drop. - let res = - NeedsDropTypes::new(tcx, query.param_env, query.value, adt_components).next().is_some(); + let adt_has_dtor = + |adt_def: &ty::AdtDef| adt_def.destructor(tcx).map(|_| DtorType::Significant); + let res = drop_tys_helper(tcx, query.value, query.param_env, adt_has_dtor).next().is_some(); debug!("needs_drop_raw({:?}) = {:?}", query, res); res @@ -29,12 +27,10 @@ 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(); + let res = + drop_tys_helper(tcx, query.value, query.param_env, adt_consider_insignificant_dtor(tcx)) + .next() + .is_some(); debug!("has_significant_drop_raw({:?}) = {:?}", query, res); res } @@ -140,15 +136,14 @@ where // `ManuallyDrop`. If it's a struct or enum without a `Drop` // impl then check whether the field types need `Drop`. ty::Adt(adt_def, substs) => { + debug!("Got value {:?} with substs {:?}", adt_def, substs); let tys = match (self.adt_components)(adt_def, substs) { Err(e) => return Some(Err(e)), Ok(tys) => tys, }; for required_ty in tys { - let subst_ty = tcx.normalize_erasing_regions( - self.param_env, - required_ty.subst(tcx, substs), - ); + let subst_ty = + tcx.normalize_erasing_regions(self.param_env, required_ty); queue_type(self, subst_ty); } } @@ -187,11 +182,12 @@ enum DtorType { // 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>( +fn drop_tys_helper<'tcx>( tcx: TyCtxt<'tcx>, - def_id: DefId, + ty: Ty<'tcx>, + param_env: rustc_middle::ty::ParamEnv<'tcx>, adt_has_dtor: impl Fn(&ty::AdtDef) -> Option, -) -> Result<&ty::List>, AlwaysRequiresDrop> { +) -> impl Iterator>> { let adt_components = move |adt_def: &ty::AdtDef, substs: SubstsRef<'tcx>| { if adt_def.is_manually_drop() { debug!("adt_drop_tys: `{:?}` is manually drop", adt_def); @@ -215,31 +211,25 @@ fn adt_drop_tys_helper<'tcx>( debug!("adt_drop_tys: `{:?}` is a union", adt_def); return Ok(Vec::new().into_iter()); } - Ok(adt_def.all_fields().map(|field| tcx.type_of(field.did)).collect::>().into_iter()) + debug!("Path"); + Ok(adt_def + .all_fields() + .map(|field| { + let r = tcx.type_of(field.did).subst(tcx, substs); + debug!("Subst into {:?} with {:?} gettng {:?}", field, substs, r); + r + }) + .collect::>() + .into_iter()) }; - let adt_ty = tcx.type_of(def_id); - let param_env = tcx.param_env(def_id); - let res: Result, _> = - NeedsDropTypes::new(tcx, param_env, adt_ty, adt_components).collect(); - - debug!("adt_drop_tys(`{}`) = `{:?}`", tcx.def_path_str(def_id), res); - res.map(|components| tcx.intern_type_list(&components)) + NeedsDropTypes::new(tcx, param_env, ty, adt_components) } -fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List>, AlwaysRequiresDrop> { - // This is for the "needs_drop" query, that considers all `Drop` impls, therefore all dtors are - // significant. - let adt_has_dtor = - |adt_def: &ty::AdtDef| adt_def.destructor(tcx).map(|_| DtorType::Significant); - 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| { +fn adt_consider_insignificant_dtor<'tcx>( + tcx: TyCtxt<'tcx>, +) -> impl Fn(&ty::AdtDef) -> Option + 'tcx { + move |adt_def: &ty::AdtDef| { let is_marked_insig = tcx.has_attr(adt_def.did, sym::rustc_insignificant_dtor); if is_marked_insig { // In some cases like `std::collections::HashMap` where the struct is a wrapper around @@ -256,8 +246,31 @@ fn adt_significant_drop_tys( // treat this as the simple case of Drop impl for type. None } - }; - adt_drop_tys_helper(tcx, def_id, adt_has_dtor) + } +} + +fn adt_drop_tys(tcx: TyCtxt<'_>, def_id: DefId) -> Result<&ty::List>, AlwaysRequiresDrop> { + // This is for the "adt_drop_tys" query, that considers all `Drop` impls, therefore all dtors are + // significant. + let adt_has_dtor = + |adt_def: &ty::AdtDef| adt_def.destructor(tcx).map(|_| DtorType::Significant); + drop_tys_helper(tcx, tcx.type_of(def_id), tcx.param_env(def_id), adt_has_dtor) + .collect::, _>>() + .map(|components| tcx.intern_type_list(&components)) +} + +fn adt_significant_drop_tys( + tcx: TyCtxt<'_>, + def_id: DefId, +) -> Result<&ty::List>, AlwaysRequiresDrop> { + drop_tys_helper( + tcx, + tcx.type_of(def_id), + tcx.param_env(def_id), + adt_consider_insignificant_dtor(tcx), + ) + .collect::, _>>() + .map(|components| tcx.intern_type_list(&components)) } pub(crate) fn provide(providers: &mut ty::query::Providers) { From eae42fd9d0d6e568282a4c782405058b64f1ef0b Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Sat, 23 Oct 2021 22:36:50 -0400 Subject: [PATCH 2/3] Add regresstion test for #90024. Uses 2 MCVEs from the issue tracker that test opposite sides of the problem. --- .../issue-90024-adt-correct-subst.rs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 src/test/ui/closures/2229_closure_analysis/migrations/issue-90024-adt-correct-subst.rs diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/issue-90024-adt-correct-subst.rs b/src/test/ui/closures/2229_closure_analysis/migrations/issue-90024-adt-correct-subst.rs new file mode 100644 index 00000000000..ed8cb042b3e --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/issue-90024-adt-correct-subst.rs @@ -0,0 +1,37 @@ +// Test that rustc doesn't ICE as in #90024. +// check-pass +// edition=2018 + +#![warn(rust_2021_incompatible_closure_captures)] + +// Checks there's no double-subst into the generic args, otherwise we get OOB +// MCVE by @lqd +pub struct Graph { + _edges: E, + _nodes: N, + _ix: Vec, +} +fn graph() -> Graph { + todo!() +} +fn first_ice() { + let g = graph::(); + let _ = || g; +} + +// Checks that there is a subst into the fields, otherwise we get normalization error +// MCVE by @cuviper +use std::iter::Empty; +struct Foo { + data: Vec, +} +pub fn second_ice() { + let v = Foo::> { data: vec![] }; + + (|| v.data[0])(); +} + +pub fn main() { + first_ice(); + second_ice(); +} From aff37f8f7b0a14493e17bc9fd7844f4392d08241 Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Mon, 25 Oct 2021 20:45:46 -0400 Subject: [PATCH 3/3] Clean up debug statements in needs_drop --- compiler/rustc_ty_utils/src/needs_drop.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_ty_utils/src/needs_drop.rs b/compiler/rustc_ty_utils/src/needs_drop.rs index dca93aff0cd..3f66e5b4ebf 100644 --- a/compiler/rustc_ty_utils/src/needs_drop.rs +++ b/compiler/rustc_ty_utils/src/needs_drop.rs @@ -136,7 +136,6 @@ where // `ManuallyDrop`. If it's a struct or enum without a `Drop` // impl then check whether the field types need `Drop`. ty::Adt(adt_def, substs) => { - debug!("Got value {:?} with substs {:?}", adt_def, substs); let tys = match (self.adt_components)(adt_def, substs) { Err(e) => return Some(Err(e)), Ok(tys) => tys, @@ -190,16 +189,16 @@ fn drop_tys_helper<'tcx>( ) -> impl Iterator>> { let adt_components = move |adt_def: &ty::AdtDef, substs: SubstsRef<'tcx>| { if adt_def.is_manually_drop() { - debug!("adt_drop_tys: `{:?}` is manually drop", adt_def); + debug!("drop_tys_helper: `{:?}` is manually drop", adt_def); return Ok(Vec::new().into_iter()); } else if let Some(dtor_info) = adt_has_dtor(adt_def) { match dtor_info { DtorType::Significant => { - debug!("adt_drop_tys: `{:?}` implements `Drop`", adt_def); + debug!("drop_tys_helper: `{:?}` implements `Drop`", adt_def); return Err(AlwaysRequiresDrop); } DtorType::Insignificant => { - debug!("adt_drop_tys: `{:?}` drop is insignificant", adt_def); + debug!("drop_tys_helper: `{:?}` drop is insignificant", adt_def); // Since the destructor is insignificant, we just want to make sure all of // the passed in type parameters are also insignificant. @@ -208,15 +207,14 @@ fn drop_tys_helper<'tcx>( } } } else if adt_def.is_union() { - debug!("adt_drop_tys: `{:?}` is a union", adt_def); + debug!("drop_tys_helper: `{:?}` is a union", adt_def); return Ok(Vec::new().into_iter()); } - debug!("Path"); Ok(adt_def .all_fields() .map(|field| { let r = tcx.type_of(field.did).subst(tcx, substs); - debug!("Subst into {:?} with {:?} gettng {:?}", field, substs, r); + debug!("drop_tys_helper: Subst into {:?} with {:?} gettng {:?}", field, substs, r); r }) .collect::>()