Auto merge of rust-lang#99217 - lcnr:implied-bounds-pre-norm, r=lcnr

consider unnormalized types for implied bounds

extracted, and slightly modified, from rust-lang#98900

The idea here is that generally, rustc is split into things which can assume its inputs are well formed[^1], and things which have verify that themselves.

Generally most predicates should only deal with well formed inputs, e.g. a `&'a &'b (): Trait` predicate should be able to assume that `'b: 'a` holds. Normalization can loosen wf requirements (see rust-lang#91068) and must therefore not be used in places which still have to check well formedness. The only such place should hopefully be `WellFormed` predicates

fixes rust-lang#87748 and rust-lang#98543

r? `@jackh726` cc `@rust-lang/types`

[^1]: These places may still encounter non-wf inputs and have to deal with them without causing an ICE as we may check for well formedness out of order.

Author: bors

compiler/rustc_borrowck/src/type_check/constraint_conversion.rs

@@ -6,7 +6,7 @@ use rustc_infer::infer::region_constraints::{GenericKind, VerifyBound};
 use rustc_infer::infer::{self, InferCtxt, SubregionOrigin};
 use rustc_middle::mir::ConstraintCategory;
 use rustc_middle::ty::subst::GenericArgKind;
-use rustc_middle::ty::TypeVisitable;
+use rustc_middle::ty::TypeFoldable;
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_span::{Span, DUMMY_SP};
 
@@ -109,23 +109,11 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> {
                 self.add_outlives(r1_vid, r2_vid);
             }
 
-            GenericArgKind::Type(mut t1) => {
+            GenericArgKind::Type(t1) => {
                 // we don't actually use this for anything, but
                 // the `TypeOutlives` code needs an origin.
                 let origin = infer::RelateParamBound(DUMMY_SP, t1, None);
 
-                // Placeholder regions need to be converted now because it may
-                // create new region variables, which can't be done later when
-                // verifying these bounds.
-                if t1.has_placeholders() {
-                    t1 = tcx.fold_regions(t1, |r, _| match *r {
-                        ty::RePlaceholder(placeholder) => {
-                            self.constraints.placeholder_region(self.infcx, placeholder)
-                        }
-                        _ => r,
-                    });
-                }
-
                 TypeOutlives::new(
                     &mut *self,
                     tcx,
@@ -143,14 +131,32 @@ impl<'a, 'tcx> ConstraintConversion<'a, 'tcx> {
         }
     }
 
+    /// Placeholder regions need to be converted eagerly because it may
+    /// create new region variables, which we must not do when verifying
+    /// our region bounds.
+    ///
+    /// FIXME: This should get removed once higher ranked region obligations
+    /// are dealt with during trait solving.
+    fn replace_placeholders_with_nll<T: TypeFoldable<'tcx>>(&mut self, value: T) -> T {
+        if value.has_placeholders() {
+            self.tcx.fold_regions(value, |r, _| match *r {
+                ty::RePlaceholder(placeholder) => {
+                    self.constraints.placeholder_region(self.infcx, placeholder)
+                }
+                _ => r,
+            })
+        } else {
+            value
+        }
+    }
+
     fn verify_to_type_test(
         &mut self,
         generic_kind: GenericKind<'tcx>,
         region: ty::Region<'tcx>,
         verify_bound: VerifyBound<'tcx>,
     ) -> TypeTest<'tcx> {
         let lower_bound = self.to_region_vid(region);
-
         TypeTest { generic_kind, lower_bound, locations: self.locations, verify_bound }
     }
 
@@ -198,6 +204,8 @@ impl<'a, 'b, 'tcx> TypeOutlivesDelegate<'tcx> for &'a mut ConstraintConversion<'
         a: ty::Region<'tcx>,
         bound: VerifyBound<'tcx>,
     ) {
+        let kind = self.replace_placeholders_with_nll(kind);
+        let bound = self.replace_placeholders_with_nll(bound);
         let type_test = self.verify_to_type_test(kind, a, bound);
         self.add_type_test(type_test);
     }

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

@@ -242,10 +242,9 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         let constraint_sets: Vec<_> = unnormalized_input_output_tys
             .flat_map(|ty| {
                 debug!("build: input_or_output={:?}", ty);
-                // We only add implied bounds for the normalized type as the unnormalized
-                // type may not actually get checked by the caller.
-                //
-                // Can otherwise be unsound, see #91068.
+                // 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))
@@ -273,9 +272,10 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
                 // }
                 // ```
                 // Both &Self::Bar and &() are WF
-                let constraints_implied = self.add_implied_bounds(norm_ty);
+                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_implied)
+                constraints1.into_iter().chain(constraints_implied1).chain(constraints_implied2)
             })
             .collect();
 

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -1448,16 +1448,23 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
                     ))
                 });
                 debug!(?sig);
-                let sig = self.normalize(sig, term_location);
-                self.check_call_dest(body, term, &sig, *destination, target, term_location);
-
+                // IMPORTANT: We have to prove well formed for the function signature before
+                // we normalize it, as otherwise types like `<&'a &'b () as Trait>::Assoc`
+                // get normalized away, causing us to ignore the `'b: 'a` bound used by the function.
+                //
+                // Normalization results in a well formed type if the input is well formed, so we
+                // don't have to check it twice.
+                //
+                // See #91068 for an example.
                 self.prove_predicates(
                     sig.inputs_and_output
                         .iter()
                         .map(|ty| ty::Binder::dummy(ty::PredicateKind::WellFormed(ty.into()))),
                     term_location.to_locations(),
                     ConstraintCategory::Boring,
                 );
+                let sig = self.normalize(sig, term_location);
+                self.check_call_dest(body, term, &sig, *destination, target, term_location);
 
                 // The ordinary liveness rules will ensure that all
                 // regions in the type of the callee are live here. We

compiler/rustc_infer/src/infer/region_constraints/mod.rs

@@ -187,7 +187,7 @@ pub enum GenericKind<'tcx> {
 /// }
 /// ```
 /// This is described with an `AnyRegion('a, 'b)` node.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, TypeFoldable, TypeVisitable)]
 pub enum VerifyBound<'tcx> {
     /// See [`VerifyIfEq`] docs
     IfEq(ty::Binder<'tcx, VerifyIfEq<'tcx>>),

compiler/rustc_middle/src/ty/mod.rs

@@ -594,6 +594,29 @@ impl<'tcx> Predicate<'tcx> {
         }
         self
     }
+
+    /// Whether this projection can be soundly normalized.
+    ///
+    /// Wf predicates must not be normalized, as normalization
+    /// can remove required bounds which would cause us to
+    /// unsoundly accept some programs. See #91068.
+    #[inline]
+    pub fn allow_normalization(self) -> bool {
+        match self.kind().skip_binder() {
+            PredicateKind::WellFormed(_) => false,
+            PredicateKind::Trait(_)
+            | PredicateKind::RegionOutlives(_)
+            | PredicateKind::TypeOutlives(_)
+            | PredicateKind::Projection(_)
+            | PredicateKind::ObjectSafe(_)
+            | PredicateKind::ClosureKind(_, _, _)
+            | PredicateKind::Subtype(_)
+            | PredicateKind::Coerce(_)
+            | PredicateKind::ConstEvaluatable(_)
+            | PredicateKind::ConstEquate(_, _)
+            | PredicateKind::TypeWellFormedFromEnv(_) => true,
+        }
+    }
 }
 
 impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for Predicate<'tcx> {

compiler/rustc_trait_selection/src/traits/project.rs

@@ -619,6 +619,15 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> {
             constant.eval(self.selcx.tcx(), self.param_env)
         }
     }
+
+    #[inline]
+    fn fold_predicate(&mut self, p: ty::Predicate<'tcx>) -> ty::Predicate<'tcx> {
+        if p.allow_normalization() && needs_normalization(&p, self.param_env.reveal()) {
+            p.super_fold_with(self)
+        } else {
+            p
+        }
+    }
 }
 
 pub struct BoundVarReplacer<'me, 'tcx> {

compiler/rustc_trait_selection/src/traits/query/normalize.rs

@@ -351,4 +351,16 @@ impl<'cx, 'tcx> FallibleTypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
             mir::ConstantKind::Val(_, _) => constant.try_super_fold_with(self)?,
         })
     }
+
+    #[inline]
+    fn try_fold_predicate(
+        &mut self,
+        p: ty::Predicate<'tcx>,
+    ) -> Result<ty::Predicate<'tcx>, Self::Error> {
+        if p.allow_normalization() && needs_normalization(&p, self.param_env.reveal()) {
+            p.try_super_fold_with(self)
+        } else {
+            Ok(p)
+        }
+    }
 }

compiler/rustc_typeck/src/check/compare_method.rs

@@ -264,8 +264,13 @@ fn compare_predicate_entailment<'tcx>(
 
         let trait_sig = tcx.bound_fn_sig(trait_m.def_id).subst(tcx, trait_to_placeholder_substs);
         let trait_sig = tcx.liberate_late_bound_regions(impl_m.def_id, trait_sig);
+        // Next, add all inputs and output as well-formed tys. Importantly,
+        // we have to do this before normalization, since the normalized ty may
+        // not contain the input parameters. See issue #87748.
+        wf_tys.extend(trait_sig.inputs_and_output.iter());
         let trait_sig = ocx.normalize(norm_cause, param_env, trait_sig);
-        // Add the resulting inputs and output as well-formed.
+        // We also have to add the normalized trait signature
+        // as we don't normalize during implied bounds computation.
         wf_tys.extend(trait_sig.inputs_and_output.iter());
         let trait_fty = tcx.mk_fn_ptr(ty::Binder::dummy(trait_sig));
 

src/test/ui/associated-types/issue-59324.rs

@@ -15,9 +15,9 @@ pub trait ThriftService<Bug: NotFoo>:
 {
     fn get_service(
     //~^ ERROR the trait bound `Bug: Foo` is not satisfied
+    //~| ERROR the trait bound `Bug: Foo` is not satisfied
         &self,
     ) -> Self::AssocType;
-    //~^ the trait bound `Bug: Foo` is not satisfied
 }
 
 fn with_factory<H>(factory: dyn ThriftService<()>) {}

src/test/ui/associated-types/issue-59324.stderr

@@ -20,7 +20,7 @@ LL | |
 LL | |
 LL | |     Service<AssocType = <Bug as Foo>::OnlyFoo>
 ...  |
-LL | |
+LL | |     ) -> Self::AssocType;
 LL | | }
    | |_^ the trait `Foo` is not implemented for `Bug`
    |
@@ -34,6 +34,7 @@ error[E0277]: the trait bound `Bug: Foo` is not satisfied
    |
 LL | /     fn get_service(
 LL | |
+LL | |
 LL | |         &self,
 LL | |     ) -> Self::AssocType;
    | |_________________________^ the trait `Foo` is not implemented for `Bug`
@@ -50,10 +51,10 @@ LL | fn with_factory<H>(factory: dyn ThriftService<()>) {}
    |                             ^^^^^^^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `()`
 
 error[E0277]: the trait bound `Bug: Foo` is not satisfied
-  --> $DIR/issue-59324.rs:19:10
+  --> $DIR/issue-59324.rs:16:8
    |
-LL |     ) -> Self::AssocType;
-   |          ^^^^^^^^^^^^^^^ the trait `Foo` is not implemented for `Bug`
+LL |     fn get_service(
+   |        ^^^^^^^^^^^ the trait `Foo` is not implemented for `Bug`
    |
 help: consider further restricting this bound
    |

src/test/ui/fn/implied-bounds-unnorm-associated-type-2.rs

@@ -1,4 +1,4 @@
-// check-pass
+// check-fail
 
 trait Trait {
     type Type;
@@ -17,6 +17,7 @@ where
 
 fn g<'a, 'b>() {
     f::<'a, 'b>(());
+    //~^ ERROR lifetime may not live long enough
 }
 
 fn main() {}

src/test/ui/fn/implied-bounds-unnorm-associated-type-2.stderr

@@ -0,0 +1,17 @@
+error: lifetime may not live long enough
+  --> $DIR/implied-bounds-unnorm-associated-type-2.rs:19:5
+   |
+LL | fn g<'a, 'b>() {
+   |      --  -- lifetime `'b` defined here
+   |      |
+   |      lifetime `'a` defined here
+LL |     f::<'a, 'b>(());
+   |     ^^^^^^^^^^^^^^^ requires that `'b` must outlive `'a`
+   |
+   = help: consider adding the following bound: `'b: 'a`
+   = note: requirement occurs because of a function pointer to `f`
+   = note: the function `f` is invariant over the parameter `'a`
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
+
+error: aborting due to previous error
+

src/test/ui/fn/implied-bounds-unnorm-associated-type-3.rs

@@ -1,6 +1,4 @@
-// check-fail
-// See issue #91899. If we treat unnormalized args as WF, `Self` can also be a
-// source of unsoundness.
+// check-pass
 
 pub trait Yokeable<'a>: 'static {
     type Output: 'a;
@@ -17,7 +15,6 @@ pub trait ZeroCopyFrom<C: ?Sized>: for<'a> Yokeable<'a> {
 
 impl<T> ZeroCopyFrom<[T]> for &'static [T] {
     fn zero_copy_from<'b>(cart: &'b [T]) -> &'b [T] {
-        //~^ the parameter
         cart
     }
 }

src/test/ui/fn/implied-bounds-unnorm-associated-type-3.stderr

@@ -0,0 +1,24 @@
+// A regression test for #98543
+
+trait Trait {
+    type Type;
+}
+
+impl<T> Trait for T {
+    type Type = ();
+}
+
+fn f<'a, 'b>(s: &'b str, _: <&'a &'b () as Trait>::Type) -> &'a str
+where
+    &'a &'b (): Trait, // <- adding this bound is the change from #91068
+{
+    s
+}
+
+fn main() {
+    let x = String::from("Hello World!");
+    let y = f(&x, ());
+    drop(x);
+    //~^ ERROR cannot move out of `x` because it is borrowed
+    println!("{}", y);
+}

src/test/ui/fn/implied-bounds-unnorm-associated-type-4.rs

@@ -0,0 +1,14 @@
+error[E0505]: cannot move out of `x` because it is borrowed
+  --> $DIR/implied-bounds-unnorm-associated-type-4.rs:21:10
+   |
+LL |     let y = f(&x, ());
+   |               -- borrow of `x` occurs here
+LL |     drop(x);
+   |          ^ move out of `x` occurs here
+LL |
+LL |     println!("{}", y);
+   |                    - borrow later used here
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0505`.

src/test/ui/fn/implied-bounds-unnorm-associated-type-4.stderr

@@ -0,0 +1,23 @@
+trait Trait<'a>: 'a {
+    type Type;
+}
+
+// if the `T: 'a` bound gets implied we would probably get ub here again
+impl<'a, T> Trait<'a> for T {
+    //~^ ERROR the parameter type `T` may not live long enough
+    type Type = ();
+}
+
+fn f<'a, 'b>(s: &'b str, _: <&'b () as Trait<'a>>::Type) -> &'a str
+where
+    &'b (): Trait<'a>,
+{
+    s
+}
+
+fn main() {
+    let x = String::from("Hello World!");
+    let y = f(&x, ());
+    drop(x);
+    println!("{}", y);
+}