Rollup merge of #67059 - TommasoBianchi:dropck_fix_pr, r=pnkfelix

Fix too restrictive checks on Drop impls

Fixes #34426. Fixes #58311.

This PR completes and extends #59497 (which has been inactive for a while now).
The problem generating both issues was that when checking that the `Predicate`s of the `Drop` impl are exactly the same as the ones of the struct definition, the check was essentially performed by a simple `==` operator, which was not handling correctly HRTBs and involved `Fn` types.

The implemented solution relies on the `relate` machinery to more correctly equate `Predicate`s, and on `anonymize_late_bound_regions` to handle HRTB in a more general way. As the `Relate` trait currently is implemented only for `TraitPredicate` and `ProjectionPredicate` (and as they were the ones generating problems), `relate` is used only for them while for other `Predicate`s the equality check is kept. I'm currently considering whether it would make sense to implement the `Relate` trait also for all other `Predicate`s to render the proposed solution more general.

3 files changed, 227 insertions, 17 deletions

diff --git a/src/librustc_typeck/check/dropck.rs b/src/librustc_typeck/check/dropck.rs
index fba933ab0ce..025bb05f63a 100644
--- a/src/librustc_typeck/check/dropck.rs
+++ b/src/librustc_typeck/check/dropck.rs
@@ -2,13 +2,15 @@ use crate::check::regionck::RegionCtxt;
 
 use crate::hir;
 use crate::hir::def_id::DefId;
+use crate::util::common::ErrorReported;
 use rustc::infer::outlives::env::OutlivesEnvironment;
 use rustc::infer::{InferOk, SuppressRegionErrors};
 use rustc::middle::region;
 use rustc::traits::{ObligationCause, TraitEngine, TraitEngineExt};
+use rustc::ty::error::TypeError;
+use rustc::ty::relate::{Relate, RelateResult, TypeRelation};
 use rustc::ty::subst::{Subst, SubstsRef};
-use rustc::ty::{self, Ty, TyCtxt};
-use crate::util::common::ErrorReported;
+use rustc::ty::{self, Predicate, Ty, TyCtxt};
 
 use syntax_pos::Span;
 
@@ -56,8 +58,10 @@ pub fn check_drop_impl(tcx: TyCtxt<'_>, drop_impl_did: DefId) -> Result<(), Erro
             // already checked by coherence, but compilation may
             // not have been terminated.
             let span = tcx.def_span(drop_impl_did);
-            tcx.sess.delay_span_bug(span,
-                &format!("should have been rejected by coherence check: {}", dtor_self_type));
+            tcx.sess.delay_span_bug(
+                span,
+                &format!("should have been rejected by coherence check: {}", dtor_self_type),
+            );
             Err(ErrorReported)
         }
     }
@@ -85,10 +89,7 @@ fn ensure_drop_params_and_item_params_correspond<'tcx>(
         let fresh_impl_self_ty = drop_impl_ty.subst(tcx, fresh_impl_substs);
 
         let cause = &ObligationCause::misc(drop_impl_span, drop_impl_hir_id);
-        match infcx
-            .at(cause, impl_param_env)
-            .eq(named_type, fresh_impl_self_ty)
-        {
+        match infcx.at(cause, impl_param_env).eq(named_type, fresh_impl_self_ty) {
             Ok(InferOk { obligations, .. }) => {
                 fulfillment_cx.register_predicate_obligations(infcx, obligations);
             }
@@ -99,12 +100,13 @@ fn ensure_drop_params_and_item_params_correspond<'tcx>(
                     drop_impl_span,
                     E0366,
                     "Implementations of Drop cannot be specialized"
-                ).span_note(
+                )
+                .span_note(
                     item_span,
                     "Use same sequence of generic type and region \
                      parameters that is on the struct/enum definition",
                 )
-                    .emit();
+                .emit();
                 return Err(ErrorReported);
             }
         }
@@ -194,6 +196,8 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>(
     let assumptions_in_impl_context = generic_assumptions.instantiate(tcx, &self_to_impl_substs);
     let assumptions_in_impl_context = assumptions_in_impl_context.predicates;
 
+    let self_param_env = tcx.param_env(self_type_did);
+
     // An earlier version of this code attempted to do this checking
     // via the traits::fulfill machinery. However, it ran into trouble
     // since the fulfill machinery merely turns outlives-predicates
@@ -207,14 +211,35 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>(
         // to take on a structure that is roughly an alpha-renaming of
         // the generic parameters of the item definition.)
 
-        // This path now just checks *all* predicates via the direct
-        // lookup, rather than using fulfill machinery.
+        // This path now just checks *all* predicates via an instantiation of
+        // the `SimpleEqRelation`, which simply forwards to the `relate` machinery
+        // after taking care of anonymizing late bound regions.
         //
         // However, it may be more efficient in the future to batch
-        // the analysis together via the fulfill , rather than the
-        // repeated `contains` calls.
+        // the analysis together via the fulfill (see comment above regarding
+        // the usage of the fulfill machinery), rather than the
+        // repeated `.iter().any(..)` calls.
 
-        if !assumptions_in_impl_context.contains(&predicate) {
+        // This closure is a more robust way to check `Predicate` equality
+        // than simple `==` checks (which were the previous implementation).
+        // It relies on `ty::relate` for `TraitPredicate` and `ProjectionPredicate`
+        // (which implement the Relate trait), while delegating on simple equality
+        // for the other `Predicate`.
+        // This implementation solves (Issue #59497) and (Issue #58311).
+        // It is unclear to me at the moment whether the approach based on `relate`
+        // could be extended easily also to the other `Predicate`.
+        let predicate_matches_closure = |p: &'_ Predicate<'tcx>| {
+            let mut relator: SimpleEqRelation<'tcx> = SimpleEqRelation::new(tcx, self_param_env);
+            match (predicate, p) {
+                (Predicate::Trait(a), Predicate::Trait(b)) => relator.relate(a, b).is_ok(),
+                (Predicate::Projection(a), Predicate::Projection(b)) => {
+                    relator.relate(a, b).is_ok()
+                }
+                _ => predicate == p,
+            }
+        };
+
+        if !assumptions_in_impl_context.iter().any(predicate_matches_closure) {
             let item_span = tcx.hir().span(self_type_hir_id);
             struct_span_err!(
                 tcx.sess,
@@ -222,12 +247,13 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>(
                 E0367,
                 "The requirement `{}` is added only by the Drop impl.",
                 predicate
-            ).span_note(
+            )
+            .span_note(
                 item_span,
                 "The same requirement must be part of \
                  the struct/enum definition",
             )
-                .emit();
+            .emit();
             result = Err(ErrorReported);
         }
     }
@@ -253,3 +279,99 @@ crate fn check_drop_obligations<'a, 'tcx>(
 
     Ok(())
 }
+
+// This is an implementation of the TypeRelation trait with the
+// aim of simply comparing for equality (without side-effects).
+// It is not intended to be used anywhere else other than here.
+crate struct SimpleEqRelation<'tcx> {
+    tcx: TyCtxt<'tcx>,
+    param_env: ty::ParamEnv<'tcx>,
+}
+
+impl<'tcx> SimpleEqRelation<'tcx> {
+    fn new(tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> SimpleEqRelation<'tcx> {
+        SimpleEqRelation { tcx, param_env }
+    }
+}
+
+impl TypeRelation<'tcx> for SimpleEqRelation<'tcx> {
+    fn tcx(&self) -> TyCtxt<'tcx> {
+        self.tcx
+    }
+
+    fn param_env(&self) -> ty::ParamEnv<'tcx> {
+        self.param_env
+    }
+
+    fn tag(&self) -> &'static str {
+        "dropck::SimpleEqRelation"
+    }
+
+    fn a_is_expected(&self) -> bool {
+        true
+    }
+
+    fn relate_with_variance<T: Relate<'tcx>>(
+        &mut self,
+        _: ty::Variance,
+        a: &T,
+        b: &T,
+    ) -> RelateResult<'tcx, T> {
+        // Here we ignore variance because we require drop impl's types
+        // to be *exactly* the same as to the ones in the struct definition.
+        self.relate(a, b)
+    }
+
+    fn tys(&mut self, a: Ty<'tcx>, b: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> {
+        debug!("SimpleEqRelation::tys(a={:?}, b={:?})", a, b);
+        ty::relate::super_relate_tys(self, a, b)
+    }
+
+    fn regions(
+        &mut self,
+        a: ty::Region<'tcx>,
+        b: ty::Region<'tcx>,
+    ) -> RelateResult<'tcx, ty::Region<'tcx>> {
+        debug!("SimpleEqRelation::regions(a={:?}, b={:?})", a, b);
+
+        // We can just equate the regions because LBRs have been
+        // already anonymized.
+        if a == b {
+            Ok(a)
+        } else {
+            // I'm not sure is this `TypeError` is the right one, but
+            // it should not matter as it won't be checked (the dropck
+            // will emit its own, more informative and higher-level errors
+            // in case anything goes wrong).
+            Err(TypeError::RegionsPlaceholderMismatch)
+        }
+    }
+
+    fn consts(
+        &mut self,
+        a: &'tcx ty::Const<'tcx>,
+        b: &'tcx ty::Const<'tcx>,
+    ) -> RelateResult<'tcx, &'tcx ty::Const<'tcx>> {
+        debug!("SimpleEqRelation::consts(a={:?}, b={:?})", a, b);
+        ty::relate::super_relate_consts(self, a, b)
+    }
+
+    fn binders<T>(
+        &mut self,
+        a: &ty::Binder<T>,
+        b: &ty::Binder<T>,
+    ) -> RelateResult<'tcx, ty::Binder<T>>
+    where
+        T: Relate<'tcx>,
+    {
+        debug!("SimpleEqRelation::binders({:?}: {:?}", a, b);
+
+        // Anonymizing the LBRs is necessary to solve (Issue #59497).
+        // After we do so, it should be totally fine to skip the binders.
+        let anon_a = self.tcx.anonymize_late_bound_regions(a);
+        let anon_b = self.tcx.anonymize_late_bound_regions(b);
+        self.relate(anon_a.skip_binder(), anon_b.skip_binder())?;
+
+        Ok(a.clone())
+    }
+}
diff --git a/src/test/ui/dropck/dropck_fn_type.rs b/src/test/ui/dropck/dropck_fn_type.rs
new file mode 100644
index 00000000000..2934217df34
--- /dev/null
+++ b/src/test/ui/dropck/dropck_fn_type.rs
@@ -0,0 +1,20 @@
+// run-pass
+//! Regression test for #58311, regarding the usage of Fn types in drop impls
+
+// All of this Drop impls should compile.
+
+#[allow(dead_code)]
+struct S<F: Fn() -> [u8; 1]>(F);
+
+impl<F: Fn() -> [u8; 1]> Drop for S<F> {
+    fn drop(&mut self) {}
+}
+
+#[allow(dead_code)]
+struct P<A, F: FnOnce() -> [A; 10]>(F);
+
+impl<A, F: FnOnce() -> [A; 10]> Drop for P<A, F> {
+    fn drop(&mut self) {}
+}
+
+fn main() {}
diff --git a/src/test/ui/dropck/dropck_traits.rs b/src/test/ui/dropck/dropck_traits.rs
new file mode 100644
index 00000000000..98e8e88a259
--- /dev/null
+++ b/src/test/ui/dropck/dropck_traits.rs
@@ -0,0 +1,68 @@
+// run-pass
+//! Regression test for #34426, regarding HRTB in drop impls
+
+// All of this Drop impls should compile.
+
+pub trait Lifetime<'a> {}
+impl<'a> Lifetime<'a> for i32 {}
+
+#[allow(dead_code)]
+struct Foo<L>
+where
+    for<'a> L: Lifetime<'a>,
+{
+    l: L,
+}
+
+impl<L> Drop for Foo<L>
+where
+    for<'a> L: Lifetime<'a>,
+{
+    fn drop(&mut self) {}
+}
+
+#[allow(dead_code)]
+struct Foo2<L>
+where
+    for<'a> L: Lifetime<'a>,
+{
+    l: L,
+}
+
+impl<T: for<'a> Lifetime<'a>> Drop for Foo2<T>
+where
+    for<'x> T: Lifetime<'x>,
+{
+    fn drop(&mut self) {}
+}
+
+pub trait Lifetime2<'a, 'b> {}
+impl<'a, 'b> Lifetime2<'a, 'b> for i32 {}
+
+#[allow(dead_code)]
+struct Bar<L>
+where
+    for<'a, 'b> L: Lifetime2<'a, 'b>,
+{
+    l: L,
+}
+
+impl<L> Drop for Bar<L>
+where
+    for<'a, 'b> L: Lifetime2<'a, 'b>,
+{
+    fn drop(&mut self) {}
+}
+
+#[allow(dead_code)]
+struct FnHolder<T: for<'a> Fn(&'a T, dyn for<'b> Lifetime2<'a, 'b>) -> u8>(T);
+
+impl<T: for<'a> Fn(&'a T, dyn for<'b> Lifetime2<'a, 'b>) -> u8> Drop for FnHolder<T> {
+    fn drop(&mut self) {}
+}
+
+fn main() {
+    let _foo = Foo { l: 0 };
+
+    let _bar = Bar { l: 0 };
+}