From 690d5aa417f5d8f8d6157e5a897df5d7dd2ef59a Mon Sep 17 00:00:00 2001 From: lcnr Date: Mon, 6 May 2024 21:33:57 +0000 Subject: [PATCH] generalize hr alias: avoid unconstrainable infer vars --- compiler/rustc_hir_typeck/src/closure.rs | 32 ++++-------- .../src/infer/relate/generalize.rs | 44 ++++++++++++++-- .../src/solve/inspect/build.rs | 5 +- .../hr-alias-non-hr-alias-self-ty-1.rs | 36 +++++++++++++ .../hr-alias-non-hr-alias-self-ty-2.rs | 32 ++++++++++++ .../hr-alias-universe-lowering-ambiguity.rs | 51 +++++++++++++++++++ .../generalize/occurs-check-nested-alias.rs | 1 + 7 files changed, 175 insertions(+), 26 deletions(-) create mode 100644 tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-1.rs create mode 100644 tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-2.rs create mode 100644 tests/ui/traits/next-solver/generalize/hr-alias-universe-lowering-ambiguity.rs diff --git a/compiler/rustc_hir_typeck/src/closure.rs b/compiler/rustc_hir_typeck/src/closure.rs index 4883c7aff8bc4..8652692e7f721 100644 --- a/compiler/rustc_hir_typeck/src/closure.rs +++ b/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, diff --git a/compiler/rustc_infer/src/infer/relate/generalize.rs b/compiler/rustc_infer/src/infer/relate/generalize.rs index 74929daffe247..fb9198b4c952e 100644 --- a/compiler/rustc_infer/src/infer/relate/generalize.rs +++ b/compiler/rustc_infer/src/infer/relate/generalize.rs @@ -350,7 +350,13 @@ impl<'tcx> Generalizer<'_, 'tcx> { &mut self, alias: ty::AliasTy<'tcx>, ) -> Result, 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())) } } diff --git a/compiler/rustc_trait_selection/src/solve/inspect/build.rs b/compiler/rustc_trait_selection/src/solve/inspect/build.rs index 466d0d8006018..a6aebc8d65a47 100644 --- a/compiler/rustc_trait_selection/src/solve/inspect/build.rs +++ b/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!(), } } diff --git a/tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-1.rs b/tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-1.rs new file mode 100644 index 0000000000000..3be118a5b3940 --- /dev/null +++ b/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: +// +// <::Refl as HigherRanked>::Output<'a> +// alias-relate +// ::Output<'a> +// +// +// cc trait-system-refactor-initiative#110 + +#![allow(unused)] +trait HigherRanked { + type Output<'a>; +} +trait Id { + type Refl: HigherRanked; +} + +fn foo() -> for<'a> fn(<::Refl as HigherRanked>::Output<'a>) { + todo!() +} + +fn bar() { + // we generalize here + let x = foo::(); +} + +fn main() {} diff --git a/tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-2.rs b/tests/ui/traits/next-solver/generalize/hr-alias-non-hr-alias-self-ty-2.rs new file mode 100644 index 0000000000000..560eb34a977d8 --- /dev/null +++ b/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 +where + M::Refl: for<'a> Yokeable<'a>, +{ + try_map_project::(|_| todo!()) +} + +fn try_map_project(_f: F) -> M +where + M::Refl: for<'a> Yokeable<'a>, + F: for<'a> FnOnce(&'a ()) -> <::Refl as Yokeable<'a>>::Output, +{ + todo!() +} + +fn main() {} diff --git a/tests/ui/traits/next-solver/generalize/hr-alias-universe-lowering-ambiguity.rs b/tests/ui/traits/next-solver/generalize/hr-alias-universe-lowering-ambiguity.rs new file mode 100644 index 0000000000000..1e2ba81129dba --- /dev/null +++ b/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 Trait<'a>>(x: T) -> for<'a> fn(>::Assoc) { + |_| () +} + +fn unconstrained() -> 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(>::Assoc)` + // -> instantiate `?x.0` with `for<'a> fn(>::Assoc)` + x = foo(y); + + // Constrain `?y.1` to `()` + let _: () = y; + + // `AliasRelate(>::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. +} diff --git a/tests/ui/traits/next-solver/generalize/occurs-check-nested-alias.rs b/tests/ui/traits/next-solver/generalize/occurs-check-nested-alias.rs index 536e7e97c40de..00dc7a9d337d9 100644 --- a/tests/ui/traits/next-solver/generalize/occurs-check-nested-alias.rs +++ b/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.