Auto merge of rust-lang#93449 - JakobDegen:restrict-hasdrop-optimization, r=cjgillot

Restrict query recursion in `needs_significant_drop`

Overly aggressive use of the query system to improve caching lead to query cycles and consequently ICEs. This patch fixes this by restricting the use of the query system as a cache to those cases where it is definitely correct.

Closes rust-lang#92725 .

This is essentially a revert of rust-lang#90845 for the significant drop case only. The general `needs_drop` still does the same thing. The hope is that this is enough to preserve the performance improvements of that PR while fixing the ICE. Should get a perf run to verify that this is the case.

cc `@cjgillot`

Author: bors

compiler/rustc_ty_utils/src/needs_drop.rs

@@ -199,16 +199,11 @@ fn drop_tys_helper<'tcx>(
     fn with_query_cache<'tcx>(
         tcx: TyCtxt<'tcx>,
         iter: impl IntoIterator<Item = Ty<'tcx>>,
-        only_significant: bool,
     ) -> NeedsDropResult<Vec<Ty<'tcx>>> {
         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)?
-                    } {
+                    for subty in tcx.adt_drop_tys(adt_id.did)? {
                         vec.push(subty.subst(tcx, subst));
                     }
                 }
@@ -234,25 +229,28 @@ fn drop_tys_helper<'tcx>(
                     // Since the destructor is insignificant, we just want to make sure all of
                     // the passed in type parameters are also insignificant.
                     // Eg: Vec<T> dtor is insignificant when T=i32 but significant when T=Mutex.
-                    with_query_cache(tcx, substs.types(), only_significant)
+                    Ok(substs.types().collect())
                 }
             }
         } 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,
-            )
+            let field_tys = 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
+            });
+            if only_significant {
+                // We can't recurse through the query system here because we might induce a cycle
+                Ok(field_tys.collect())
+            } else {
+                // We can use the query system if we consider all drops significant. In that case,
+                // ADTs are `needs_drop` exactly if they `impl Drop` or if any of their "transitive"
+                // fields do. There can be no cycles here, because ADTs cannot contain themselves as
+                // fields.
+                with_query_cache(tcx, field_tys)
+            }
         }
         .map(|v| v.into_iter())
     };

src/test/ui/closures/2229_closure_analysis/issue-92724-needsdrop-query-cycle.rs

@@ -0,0 +1,14 @@
+// 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(Baz);
+pub struct Baz(Vec<Foo>);
+
+impl Foo {
+    pub fn baz(self, v: Baz) -> Baz {
+        (|| v)()
+    }
+}
+fn main() {}