From c5b13577a5dfd8b4df7c9dbd16cbf043c21d399f Mon Sep 17 00:00:00 2001 From: Jakob Degen Date: Tue, 11 Jan 2022 21:29:10 -0500 Subject: [PATCH] Fix query cycle in needs drop code. In a previous PR, an optimization was introduced in the needs drop code to ensure that we recurse through the query system, thereby improving caching and not duplicting work. However, this optimization did not correctly check for possible cycles in doing this. This change introduces that check. --- compiler/rustc_ty_utils/src/needs_drop.rs | 101 +++++++++--------- .../issue-92724-needsdrop-query-cycle.rs | 13 +++ 2 files changed, 66 insertions(+), 48 deletions(-) create mode 100644 src/test/ui/closures/2229_closure_analysis/issue-92724-needsdrop-query-cycle.rs diff --git a/compiler/rustc_ty_utils/src/needs_drop.rs b/compiler/rustc_ty_utils/src/needs_drop.rs index fc309aa848ca1..9627b66a5b142 100644 --- a/compiler/rustc_ty_utils/src/needs_drop.rs +++ b/compiler/rustc_ty_utils/src/needs_drop.rs @@ -78,7 +78,7 @@ impl<'tcx, F> NeedsDropTypes<'tcx, F> { impl<'tcx, F, I> Iterator for NeedsDropTypes<'tcx, F> where - F: Fn(&ty::AdtDef, SubstsRef<'tcx>) -> NeedsDropResult, + F: Fn(&ty::AdtDef, SubstsRef<'tcx>, &FxHashSet>) -> NeedsDropResult, I: Iterator>, { type Item = NeedsDropResult>; @@ -142,7 +142,7 @@ 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) => { - let tys = match (self.adt_components)(adt_def, substs) { + let tys = match (self.adt_components)(adt_def, substs, &self.seen_tys) { Err(e) => return Some(Err(e)), Ok(tys) => tys, }; @@ -200,62 +200,67 @@ fn drop_tys_helper<'tcx>( tcx: TyCtxt<'tcx>, iter: impl IntoIterator>, only_significant: bool, + seen_tys: &FxHashSet>, ) -> NeedsDropResult>> { iter.into_iter().try_fold(Vec::new(), |mut vec, subty| { - match subty.kind() { - ty::Adt(adt_id, subst) => { - for subty in if only_significant { - tcx.adt_significant_drop_tys(adt_id.did)? - } else { - tcx.adt_drop_tys(adt_id.did)? - } { - vec.push(subty.subst(tcx, subst)); + if !seen_tys.contains(subty) { + match subty.kind() { + ty::Adt(adt_id, subst) => { + for subty in if only_significant { + tcx.adt_significant_drop_tys(adt_id.did)? + } else { + tcx.adt_drop_tys(adt_id.did)? + } { + vec.push(subty.subst(tcx, subst)); + } } - } - _ => vec.push(subty), - }; + _ => vec.push(subty), + }; + } Ok(vec) }) } - let adt_components = move |adt_def: &ty::AdtDef, substs: SubstsRef<'tcx>| { - if adt_def.is_manually_drop() { - debug!("drop_tys_helper: `{:?}` is manually drop", adt_def); - Ok(Vec::new()) - } else if let Some(dtor_info) = adt_has_dtor(adt_def) { - match dtor_info { - DtorType::Significant => { - debug!("drop_tys_helper: `{:?}` implements `Drop`", adt_def); - Err(AlwaysRequiresDrop) - } - DtorType::Insignificant => { - debug!("drop_tys_helper: `{:?}` drop is insignificant", adt_def); + let adt_components = + move |adt_def: &ty::AdtDef, substs: SubstsRef<'tcx>, seen_tys: &FxHashSet>| { + if adt_def.is_manually_drop() { + debug!("drop_tys_helper: `{:?}` is manually drop", adt_def); + Ok(Vec::new()) + } else if let Some(dtor_info) = adt_has_dtor(adt_def) { + match dtor_info { + DtorType::Significant => { + debug!("drop_tys_helper: `{:?}` implements `Drop`", adt_def); + Err(AlwaysRequiresDrop) + } + DtorType::Insignificant => { + 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. - // Eg: Vec dtor is insignificant when T=i32 but significant when T=Mutex. - with_query_cache(tcx, substs.types(), only_significant) + // Since the destructor is insignificant, we just want to make sure all of + // the passed in type parameters are also insignificant. + // Eg: Vec dtor is insignificant when T=i32 but significant when T=Mutex. + with_query_cache(tcx, substs.types(), only_significant, seen_tys) + } } + } else if adt_def.is_union() { + debug!("drop_tys_helper: `{:?}` is a union", adt_def); + Ok(Vec::new()) + } else { + with_query_cache( + tcx, + adt_def.all_fields().map(|field| { + let r = tcx.type_of(field.did).subst(tcx, substs); + debug!( + "drop_tys_helper: Subst into {:?} with {:?} gettng {:?}", + field, substs, r + ); + r + }), + only_significant, + seen_tys, + ) } - } else if adt_def.is_union() { - debug!("drop_tys_helper: `{:?}` is a union", adt_def); - Ok(Vec::new()) - } else { - with_query_cache( - tcx, - adt_def.all_fields().map(|field| { - let r = tcx.type_of(field.did).subst(tcx, substs); - debug!( - "drop_tys_helper: Subst into {:?} with {:?} gettng {:?}", - field, substs, r - ); - r - }), - only_significant, - ) - } - .map(|v| v.into_iter()) - }; + .map(|v| v.into_iter()) + }; NeedsDropTypes::new(tcx, param_env, ty, adt_components) } diff --git a/src/test/ui/closures/2229_closure_analysis/issue-92724-needsdrop-query-cycle.rs b/src/test/ui/closures/2229_closure_analysis/issue-92724-needsdrop-query-cycle.rs new file mode 100644 index 0000000000000..4e4c0e3ec41a6 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/issue-92724-needsdrop-query-cycle.rs @@ -0,0 +1,13 @@ +// ICEs if checking if there is a significant destructor causes a query cycle +// check-pass + +#![warn(rust_2021_incompatible_closure_captures)] +pub struct Foo(Bar); +pub struct Bar(Vec); + +impl Foo { + pub fn bar(self, v: Bar) -> Bar { + (|| v)() + } +} +fn main() {}