Auto merge of rust-lang#101680 - jackh726:implied-cleanup, r=lcnr

Fix implied outlives bounds logic for projections

The logic here is subtly wrong. I put a bit of an explanation in a767d7b5165cea8ee5cbe494a4a636c50ef67c9c.

TL;DR: we register outlives predicates to be proved, because wf code normalizes projections (from the unnormalized types) to type variables. This causes us to register those as constraints instead of implied. This was "fine", because we later added that implied bound in the normalized type, and delayed registering constraints. When I went to cleanup `free_region_relations` to *not* delay adding constraints, this bug was uncovered.

cc. `@aliemjay` because this caused your test failure in rust-lang#99832 (I only realized as I was writing this)

r? `@nikomatsakis`

Author: bors

compiler/rustc_borrowck/src/region_infer/values.rs

@@ -187,6 +187,7 @@ pub(crate) struct PlaceholderIndices {
 }
 
 impl PlaceholderIndices {
+    /// Returns the `PlaceholderIndex` for the inserted `PlaceholderRegion`
     pub(crate) fn insert(&mut self, placeholder: ty::PlaceholderRegion) -> PlaceholderIndex {
         let (index, _) = self.indices.insert_full(placeholder);
         index.into()

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

@@ -8,6 +8,7 @@ use rustc_infer::infer::InferCtxt;
 use rustc_middle::mir::ConstraintCategory;
 use rustc_middle::traits::query::OutlivesBound;
 use rustc_middle::ty::{self, RegionVid, Ty};
+use rustc_span::Span;
 use rustc_trait_selection::traits::query::type_op::{self, TypeOp};
 use std::rc::Rc;
 use type_op::TypeOpOutput;
@@ -217,8 +218,27 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         self.inverse_outlives.add(fr_b, fr_a);
     }
 
+    #[instrument(level = "debug", skip(self))]
     pub(crate) fn create(mut self) -> CreateResult<'tcx> {
         let span = self.infcx.tcx.def_span(self.universal_regions.defining_ty.def_id());
+
+        // Insert the facts we know from the predicates. Why? Why not.
+        let param_env = self.param_env;
+        self.add_outlives_bounds(outlives::explicit_outlives_bounds(param_env));
+
+        // - outlives is reflexive, so `'r: 'r` for every region `'r`
+        // - `'static: 'r` for every region `'r`
+        // - `'r: 'fn_body` for every (other) universally quantified
+        //   region `'r`, all of which are provided by our caller
+        let fr_static = self.universal_regions.fr_static;
+        let fr_fn_body = self.universal_regions.fr_fn_body;
+        for fr in self.universal_regions.universal_regions() {
+            debug!("build: relating free region {:?} to itself and to 'static", fr);
+            self.relate_universal_regions(fr, fr);
+            self.relate_universal_regions(fr_static, fr);
+            self.relate_universal_regions(fr, fr_fn_body);
+        }
+
         let unnormalized_input_output_tys = self
             .universal_regions
             .unnormalized_input_tys
@@ -236,78 +256,58 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         //   the `relations` is built.
         let mut normalized_inputs_and_output =
             Vec::with_capacity(self.universal_regions.unnormalized_input_tys.len() + 1);
-        let constraint_sets: Vec<_> = unnormalized_input_output_tys
-            .flat_map(|ty| {
-                debug!("build: input_or_output={:?}", ty);
-                // We add implied bounds from both the unnormalized and normalized ty.
-                // See issue #87748
-                let constraints_implied1 = self.add_implied_bounds(ty);
-                let TypeOpOutput { output: norm_ty, constraints: constraints1, .. } = self
-                    .param_env
-                    .and(type_op::normalize::Normalize::new(ty))
-                    .fully_perform(self.infcx)
-                    .unwrap_or_else(|_| {
-                        let reported = self
-                            .infcx
-                            .tcx
-                            .sess
-                            .delay_span_bug(span, &format!("failed to normalize {:?}", ty));
-                        TypeOpOutput {
-                            output: self.infcx.tcx.ty_error_with_guaranteed(reported),
-                            constraints: None,
-                            error_info: None,
-                        }
-                    });
-                // Note: we need this in examples like
-                // ```
-                // trait Foo {
-                //   type Bar;
-                //   fn foo(&self) -> &Self::Bar;
-                // }
-                // impl Foo for () {
-                //   type Bar = ();
-                //   fn foo(&self) -> &() {}
-                // }
-                // ```
-                // Both &Self::Bar and &() are WF
-                let constraints_implied2 =
-                    if ty != norm_ty { self.add_implied_bounds(norm_ty) } else { None };
-                normalized_inputs_and_output.push(norm_ty);
-                constraints1.into_iter().chain(constraints_implied1).chain(constraints_implied2)
-            })
-            .collect();
+        let mut constraints = vec![];
+        for ty in unnormalized_input_output_tys {
+            debug!("build: input_or_output={:?}", ty);
+            // We add implied bounds from both the unnormalized and normalized ty.
+            // See issue #87748
+            let constraints_unnorm = self.add_implied_bounds(ty);
+            if let Some(c) = constraints_unnorm {
+                constraints.push(c)
+            }
+            let TypeOpOutput { output: norm_ty, constraints: constraints_normalize, .. } = self
+                .param_env
+                .and(type_op::normalize::Normalize::new(ty))
+                .fully_perform(self.infcx)
+                .unwrap_or_else(|_| {
+                    self.infcx
+                        .tcx
+                        .sess
+                        .delay_span_bug(span, &format!("failed to normalize {:?}", ty));
+                    TypeOpOutput {
+                        output: self.infcx.tcx.ty_error(),
+                        constraints: None,
+                        error_info: None,
+                    }
+                });
+            if let Some(c) = constraints_normalize {
+                constraints.push(c)
+            }
 
-        // Insert the facts we know from the predicates. Why? Why not.
-        let param_env = self.param_env;
-        self.add_outlives_bounds(outlives::explicit_outlives_bounds(param_env));
+            // Note: we need this in examples like
+            // ```
+            // trait Foo {
+            //   type Bar;
+            //   fn foo(&self) -> &Self::Bar;
+            // }
+            // impl Foo for () {
+            //   type Bar = ();
+            //   fn foo(&self) ->&() {}
+            // }
+            // ```
+            // Both &Self::Bar and &() are WF
+            if ty != norm_ty {
+                let constraints_norm = self.add_implied_bounds(norm_ty);
+                if let Some(c) = constraints_norm {
+                    constraints.push(c)
+                }
+            }
 
-        // Finally:
-        // - outlives is reflexive, so `'r: 'r` for every region `'r`
-        // - `'static: 'r` for every region `'r`
-        // - `'r: 'fn_body` for every (other) universally quantified
-        //   region `'r`, all of which are provided by our caller
-        let fr_static = self.universal_regions.fr_static;
-        let fr_fn_body = self.universal_regions.fr_fn_body;
-        for fr in self.universal_regions.universal_regions() {
-            debug!("build: relating free region {:?} to itself and to 'static", fr);
-            self.relate_universal_regions(fr, fr);
-            self.relate_universal_regions(fr_static, fr);
-            self.relate_universal_regions(fr, fr_fn_body);
+            normalized_inputs_and_output.push(norm_ty);
         }
 
-        for data in &constraint_sets {
-            constraint_conversion::ConstraintConversion::new(
-                self.infcx,
-                &self.universal_regions,
-                &self.region_bound_pairs,
-                self.implicit_region_bound,
-                self.param_env,
-                Locations::All(span),
-                span,
-                ConstraintCategory::Internal,
-                &mut self.constraints,
-            )
-            .convert_all(data);
+        for c in constraints {
+            self.push_region_constraints(c, span);
         }
 
         CreateResult {
@@ -321,6 +321,24 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         }
     }
 
+    #[instrument(skip(self, data), level = "debug")]
+    fn push_region_constraints(&mut self, data: &QueryRegionConstraints<'tcx>, span: Span) {
+        debug!("constraints generated: {:#?}", data);
+
+        constraint_conversion::ConstraintConversion::new(
+            self.infcx,
+            &self.universal_regions,
+            &self.region_bound_pairs,
+            self.implicit_region_bound,
+            self.param_env,
+            Locations::All(span),
+            span,
+            ConstraintCategory::Internal,
+            &mut self.constraints,
+        )
+        .convert_all(data);
+    }
+
     /// Update the type of a single local, which should represent
     /// either the return type of the MIR or one of its arguments. At
     /// the same time, compute and add any implied bounds that come
@@ -332,6 +350,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
             .and(type_op::implied_outlives_bounds::ImpliedOutlivesBounds { ty })
             .fully_perform(self.infcx)
             .unwrap_or_else(|_| bug!("failed to compute implied bounds {:?}", ty));
+        debug!(?bounds, ?constraints);
         self.add_outlives_bounds(bounds);
         constraints
     }

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -910,6 +910,8 @@ pub(crate) struct MirTypeckRegionConstraints<'tcx> {
 }
 
 impl<'tcx> MirTypeckRegionConstraints<'tcx> {
+    /// Creates a `Region` for a given `PlaceholderRegion`, or returns the
+    /// region that corresponds to a previously created one.
     fn placeholder_region(
         &mut self,
         infcx: &InferCtxt<'tcx>,

compiler/rustc_infer/src/infer/outlives/verify.rs

@@ -207,6 +207,7 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> {
     ///
     /// In some cases, such as when `erased_ty` represents a `ty::Param`, however,
     /// the result is precise.
+    #[instrument(level = "debug", skip(self))]
     fn declared_generic_bounds_from_env_for_erased_ty(
         &self,
         erased_ty: Ty<'tcx>,

compiler/rustc_traits/src/implied_outlives_bounds.rs

@@ -70,47 +70,61 @@ fn compute_implied_outlives_bounds<'tcx>(
         let obligations = wf::obligations(ocx.infcx, param_env, CRATE_DEF_ID, 0, arg, DUMMY_SP)
             .unwrap_or_default();
 
-        // While these predicates should all be implied by other parts of
-        // the program, they are still relevant as they may constrain
-        // inference variables, which is necessary to add the correct
-        // implied bounds in some cases, mostly when dealing with projections.
-        ocx.register_obligations(
-            obligations.iter().filter(|o| o.predicate.has_non_region_infer()).cloned(),
-        );
-
-        // From the full set of obligations, just filter down to the
-        // region relationships.
-        outlives_bounds.extend(obligations.into_iter().filter_map(|obligation| {
+        for obligation in obligations {
+            debug!(?obligation);
             assert!(!obligation.has_escaping_bound_vars());
-            match obligation.predicate.kind().no_bound_vars() {
-                None => None,
-                Some(pred) => match pred {
-                    ty::PredicateKind::Clause(ty::Clause::Trait(..))
-                    | ty::PredicateKind::Subtype(..)
-                    | ty::PredicateKind::Coerce(..)
-                    | ty::PredicateKind::Clause(ty::Clause::Projection(..))
-                    | ty::PredicateKind::ClosureKind(..)
-                    | ty::PredicateKind::ObjectSafe(..)
-                    | ty::PredicateKind::ConstEvaluatable(..)
-                    | ty::PredicateKind::ConstEquate(..)
-                    | ty::PredicateKind::Ambiguous
-                    | ty::PredicateKind::TypeWellFormedFromEnv(..) => None,
-                    ty::PredicateKind::WellFormed(arg) => {
-                        wf_args.push(arg);
-                        None
+
+            // While these predicates should all be implied by other parts of
+            // the program, they are still relevant as they may constrain
+            // inference variables, which is necessary to add the correct
+            // implied bounds in some cases, mostly when dealing with projections.
+            //
+            // Another important point here: we only register `Projection`
+            // predicates, since otherwise we might register outlives
+            // predicates containing inference variables, and we don't
+            // learn anything new from those.
+            if obligation.predicate.has_non_region_infer() {
+                match obligation.predicate.kind().skip_binder() {
+                    ty::PredicateKind::Clause(ty::Clause::Projection(..)) => {
+                        ocx.register_obligation(obligation.clone());
                     }
+                    _ => {}
+                }
+            }
 
-                    ty::PredicateKind::Clause(ty::Clause::RegionOutlives(
-                        ty::OutlivesPredicate(r_a, r_b),
-                    )) => Some(ty::OutlivesPredicate(r_a.into(), r_b)),
+            let pred = match obligation.predicate.kind().no_bound_vars() {
+                None => continue,
+                Some(pred) => pred,
+            };
+            match pred {
+                ty::PredicateKind::Clause(ty::Clause::Trait(..))
+                | ty::PredicateKind::Subtype(..)
+                | ty::PredicateKind::Coerce(..)
+                | ty::PredicateKind::Clause(ty::Clause::Projection(..))
+                | ty::PredicateKind::ClosureKind(..)
+                | ty::PredicateKind::ObjectSafe(..)
+                | ty::PredicateKind::ConstEvaluatable(..)
+                | ty::PredicateKind::ConstEquate(..)
+                | ty::PredicateKind::Ambiguous
+                | ty::PredicateKind::TypeWellFormedFromEnv(..) => {}
+
+                // We need to search through *all* WellFormed predicates
+                ty::PredicateKind::WellFormed(arg) => {
+                    wf_args.push(arg);
+                }
+
+                // We need to register region relationships
+                ty::PredicateKind::Clause(ty::Clause::RegionOutlives(ty::OutlivesPredicate(
+                    r_a,
+                    r_b,
+                ))) => outlives_bounds.push(ty::OutlivesPredicate(r_a.into(), r_b)),
 
-                    ty::PredicateKind::Clause(ty::Clause::TypeOutlives(ty::OutlivesPredicate(
-                        ty_a,
-                        r_b,
-                    ))) => Some(ty::OutlivesPredicate(ty_a.into(), r_b)),
-                },
+                ty::PredicateKind::Clause(ty::Clause::TypeOutlives(ty::OutlivesPredicate(
+                    ty_a,
+                    r_b,
+                ))) => outlives_bounds.push(ty::OutlivesPredicate(ty_a.into(), r_b)),
             }
-        }));
+        }
     }
 
     // This call to `select_all_or_error` is necessary to constrain inference variables, which we

compiler/rustc_traits/src/lib.rs

@@ -4,6 +4,7 @@
 #![deny(rustc::untranslatable_diagnostic)]
 #![deny(rustc::diagnostic_outside_of_impl)]
 #![feature(let_chains)]
+#![feature(drain_filter)]
 #![recursion_limit = "256"]
 
 #[macro_use]

tests/ui/nll/issue-52057.rs

@@ -1,6 +1,6 @@
 // Regression test for #52057. There is an implied bound
-// that `I: 'a` where `'a` is the lifetime of `self` in `parse_first`;
-// but to observe that, one must normalize first.
+// that `I: 'x` where `'x` is the lifetime of the reference `&mut Self::Input`
+// in `parse_first`; but to observe that, one must normalize first.
 //
 // run-pass