generalize hr alias: avoid unconstrainable infer vars

Author: lcnr

compiler/rustc_hir_typeck/src/closure.rs

@@ -31,6 +31,7 @@ struct ExpectedSig<'tcx> {
     sig: ty::PolyFnSig<'tcx>,
 }
 
+#[derive(Debug)]
 struct ClosureSignatures<'tcx> {
     /// The signature users of the closure see.
     bound_sig: ty::PolyFnSig<'tcx>,
@@ -713,25 +714,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // [c2]: https://github.com/rust-lang/rust/pull/45072#issuecomment-341096796
         self.commit_if_ok(|_| {
             let mut all_obligations = vec![];
-            let inputs: Vec<_> = iter::zip(
-                decl.inputs,
-                supplied_sig.inputs().skip_binder(), // binder moved to (*) below
-            )
-            .map(|(hir_ty, &supplied_ty)| {
-                // Instantiate (this part of..) S to S', i.e., with fresh variables.
-                self.instantiate_binder_with_fresh_vars(
-                    hir_ty.span,
-                    BoundRegionConversionTime::FnCall,
-                    // (*) binder moved to here
-                    supplied_sig.inputs().rebind(supplied_ty),
-                )
-            })
-            .collect();
+            let supplied_sig = self.instantiate_binder_with_fresh_vars(
+                self.tcx.def_span(expr_def_id),
+                BoundRegionConversionTime::FnCall,
+                supplied_sig,
+            );
 
             // The liberated version of this signature should be a subtype
             // of the liberated form of the expectation.
             for ((hir_ty, &supplied_ty), expected_ty) in iter::zip(
-                iter::zip(decl.inputs, &inputs),
+                iter::zip(decl.inputs, supplied_sig.inputs()),
                 expected_sigs.liberated_sig.inputs(), // `liberated_sig` is E'.
             ) {
                 // Check that E' = S'.
@@ -744,11 +736,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 all_obligations.extend(obligations);
             }
 
-            let supplied_output_ty = self.instantiate_binder_with_fresh_vars(
-                decl.output.span(),
-                BoundRegionConversionTime::FnCall,
-                supplied_sig.output(),
-            );
+            let supplied_output_ty = supplied_sig.output();
             let cause = &self.misc(decl.output.span());
             let InferOk { value: (), obligations } = self.at(cause, self.param_env).eq(
                 DefineOpaqueTypes::Yes,
@@ -757,7 +745,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             )?;
             all_obligations.extend(obligations);
 
-            let inputs = inputs.into_iter().map(|ty| self.resolve_vars_if_possible(ty));
+            let inputs =
+                supplied_sig.inputs().into_iter().map(|&ty| self.resolve_vars_if_possible(ty));
 
             expected_sigs.liberated_sig = self.tcx.mk_fn_sig(
                 inputs,
@@ -1013,6 +1002,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         result
     }
 
+    #[instrument(level = "debug", skip(self), ret)]
     fn closure_sigs(
         &self,
         expr_def_id: LocalDefId,

compiler/rustc_infer/src/infer/relate/generalize.rs

@@ -350,7 +350,13 @@ impl<'tcx> Generalizer<'_, 'tcx> {
         &mut self,
         alias: ty::AliasTy<'tcx>,
     ) -> Result<Ty<'tcx>, TypeError<'tcx>> {
-        if self.infcx.next_trait_solver() && !alias.has_escaping_bound_vars() {
+        // We do not eagerly replace aliases with inference variables if they have
+        // escaping bound vars, see the method comment for details. However, when we
+        // are inside of an alias with escaping bound vars replacing nested aliases
+        // with inference variables can cause incorrect ambiguity.
+        //
+        // cc trait-system-refactor-initiative#110
+        if self.infcx.next_trait_solver() && !alias.has_escaping_bound_vars() && !self.in_alias {
             return Ok(self.infcx.next_ty_var_in_universe(
                 TypeVariableOrigin { param_def_id: None, span: self.span },
                 self.for_universe,
@@ -492,9 +498,30 @@ impl<'tcx> TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
                             let origin = inner.type_variables().var_origin(vid);
                             let new_var_id =
                                 inner.type_variables().new_var(self.for_universe, origin);
-                            let u = Ty::new_var(self.tcx(), new_var_id);
-                            debug!("replacing original vid={:?} with new={:?}", vid, u);
-                            Ok(u)
+                            // If we're in the new solver and create a new inference
+                            // variable inside of an alias we eagerly constrain that
+                            // inference variable to prevent unexpected ambiguity errors.
+                            //
+                            // This is incomplete as it pulls down the universe of the
+                            // original inference variable, even though the alias could
+                            // normalize to a type which does not refer to that type at
+                            // all. I don't expect this to cause unexpected errors in
+                            // practice.
+                            //
+                            // We only need to do so for type and const variables, as
+                            // region variables do not impact normalization, and will get
+                            // correctly constrained by `AliasRelate` later on.
+                            //
+                            // cc trait-system-refactor-initiative#108
+                            if self.infcx.next_trait_solver()
+                                && !self.infcx.intercrate
+                                && self.in_alias
+                            {
+                                inner.type_variables().equate(vid, new_var_id);
+                            }
+
+                            debug!("replacing original vid={:?} with new={:?}", vid, new_var_id);
+                            Ok(Ty::new_var(self.tcx(), new_var_id))
                         }
                     }
                 }
@@ -614,6 +641,15 @@ impl<'tcx> TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
                                     universe: self.for_universe,
                                 })
                                 .vid;
+
+                            // See the comment for type inference variables
+                            // for more details.
+                            if self.infcx.next_trait_solver()
+                                && !self.infcx.intercrate
+                                && self.in_alias
+                            {
+                                variable_table.union(vid, new_var_id);
+                            }
                             Ok(ty::Const::new_var(self.tcx(), new_var_id, c.ty()))
                         }
                     }

compiler/rustc_trait_selection/src/solve/inspect/build.rs

@@ -372,7 +372,10 @@ impl<'tcx> ProofTreeBuilder<'tcx> {
                 (
                     DebugSolver::GoalEvaluation(goal_evaluation),
                     DebugSolver::CanonicalGoalEvaluation(canonical_goal_evaluation),
-                ) => goal_evaluation.evaluation = Some(canonical_goal_evaluation),
+                ) => {
+                    let prev = goal_evaluation.evaluation.replace(canonical_goal_evaluation);
+                    assert_eq!(prev, None);
+                }
                 _ => unreachable!(),
             }
         }

tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-1.rs

@@ -0,0 +1,36 @@
+//@ revisions: old next
+//@[next] compile-flags: -Znext-solver
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@ check-pass
+
+// Generalizing an alias referencing escaping bound variables
+// is hard. We previously didn't replace this alias with inference
+// variables but did replace nested alias which do not reference
+// any bound variables. This caused us to stall with the following
+// goal, which cannot make any progress:
+//
+// <<T as Id>::Refl as HigherRanked>::Output<'a>
+//    alias-relate
+// <?unconstrained as HigherRanked>::Output<'a>
+//
+//
+// cc trait-system-refactor-initiative#110
+
+#![allow(unused)]
+trait HigherRanked {
+    type Output<'a>;
+}
+trait Id {
+    type Refl: HigherRanked;
+}
+
+fn foo<T: Id>() -> for<'a> fn(<<T as Id>::Refl as HigherRanked>::Output<'a>) {
+    todo!()
+}
+
+fn bar<T: Id>() {
+    // we generalize here
+    let x = foo::<T>();
+}
+
+fn main() {}

tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-2.rs

@@ -0,0 +1,32 @@
+//@ revisions: old next
+//@[next] compile-flags: -Znext-solver
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@ check-pass
+
+// A minimization of an ambiguity error in `icu_provider`.
+//
+// cc trait-system-refactor-initiative#110
+
+trait Yokeable<'a> {
+    type Output;
+}
+trait Id {
+    type Refl;
+}
+
+fn into_deserialized<M: Id>() -> M
+where
+    M::Refl: for<'a> Yokeable<'a>,
+{
+    try_map_project::<M, _>(|_| todo!())
+}
+
+fn try_map_project<M: Id, F>(_f: F) -> M
+where
+    M::Refl: for<'a> Yokeable<'a>,
+    F: for<'a> FnOnce(&'a ()) -> <<M as Id>::Refl as Yokeable<'a>>::Output,
+{
+    todo!()
+}
+
+fn main() {}

tests/ui/traits/next-solver/generalize/hr-alias-universe-lowering-ambiguity.rs

@@ -0,0 +1,51 @@
+//@ compile-flags: -Znext-solver
+//@ check-pass
+
+// A regression test for a fairly subtle issue with how we
+// generalize aliases referencing higher-ranked regions
+// which previously caused unexpected ambiguity errors.
+//
+// The explanations in the test may end up being out of date
+// in the future as we may refine our approach to generalization
+// going forward.
+//
+// cc trait-system-refactor-initiative#108
+trait Trait<'a> {
+    type Assoc;
+}
+
+impl<'a> Trait<'a> for () {
+    type Assoc = ();
+}
+
+fn foo<T: for<'a> Trait<'a>>(x: T) -> for<'a> fn(<T as Trait<'a>>::Assoc) {
+    |_| ()
+}
+
+fn unconstrained<T>() -> T {
+    todo!()
+}
+
+fn main() {
+    // create `?x.0` in the root universe
+    let mut x = unconstrained();
+
+    // bump the current universe of the inference context
+    let bump: for<'a, 'b> fn(&'a (), &'b ()) = |_, _| ();
+    let _: for<'a> fn(&'a (), &'a ()) = bump;
+
+    // create `?y.1` in a higher universe
+    let mut y = Default::default();
+
+    // relate `?x.0` with `for<'a> fn(<?y.1 as Trait<'a>>::Assoc)`
+    // -> instantiate `?x.0` with `for<'a> fn(<?y_new.0 as Trait<'a>>::Assoc)`
+    x = foo(y);
+
+    // Constrain `?y.1` to `()`
+    let _: () = y;
+
+    // `AliasRelate(<?y_new.0 as Trait<'a>>::Assoc, <() as Trait<'a>>::Assoc)`
+    // remains ambiguous unless we somehow constrain `?y_new.0` during
+    // generalization to be equal to `?y.1`, which is exactly what we
+    // did to fix this issue.
+}

tests/ui/traits/next-solver/generalize/occurs-check-nested-alias.rs

@@ -1,5 +1,6 @@
 //@ revisions: old next
 //@[next] compile-flags: -Znext-solver
+//@ ignore-compare-mode-next-solver (explicit revisions)
 //@ check-pass
 
 // case 3 of https://github.com/rust-lang/trait-system-refactor-initiative/issues/8.