From 99f60ec41179fa648a827cc99b6d110541e88c0c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 27 Jul 2023 03:16:34 +0000 Subject: [PATCH 1/2] Revert "don't uniquify regions when canonicalizing" This reverts commit 171f5414705194067557cd7b70bd680308b9cced. --- .../src/solve/canonicalize.rs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/canonicalize.rs b/compiler/rustc_trait_selection/src/solve/canonicalize.rs index 255620489ff05..88771f90756d0 100644 --- a/compiler/rustc_trait_selection/src/solve/canonicalize.rs +++ b/compiler/rustc_trait_selection/src/solve/canonicalize.rs @@ -125,9 +125,8 @@ impl<'a, 'tcx> Canonicalizer<'a, 'tcx> { // - var_infos: [E0, U1, E1, U1, E1, E6, U6], curr_compressed_uv: 1, next_orig_uv: 6 // - var_infos: [E0, U1, E1, U1, E1, E2, U2], curr_compressed_uv: 2, next_orig_uv: - // - // This algorithm runs in `O(nm)` where `n` is the number of different universe - // indices in the input and `m` is the number of canonical variables. - // This should be fine as both `n` and `m` are expected to be small. + // This algorithm runs in `O(n²)` where `n` is the number of different universe + // indices in the input. This should be fine as `n` is expected to be small. let mut curr_compressed_uv = ty::UniverseIndex::ROOT; let mut existential_in_new_uv = false; let mut next_orig_uv = Some(ty::UniverseIndex::ROOT); @@ -263,14 +262,18 @@ impl<'tcx> TypeFolder> for Canonicalizer<'_, 'tcx> { ty::ReError(_) => return r, }; - let var = ty::BoundVar::from( - self.variables.iter().position(|&v| v == r.into()).unwrap_or_else(|| { - let var = self.variables.len(); - self.variables.push(r.into()); - self.primitive_var_infos.push(CanonicalVarInfo { kind }); - var - }), - ); + let existing_bound_var = match self.canonicalize_mode { + CanonicalizeMode::Input => None, + CanonicalizeMode::Response { .. } => { + self.variables.iter().position(|&v| v == r.into()).map(ty::BoundVar::from) + } + }; + let var = existing_bound_var.unwrap_or_else(|| { + let var = ty::BoundVar::from(self.variables.len()); + self.variables.push(r.into()); + self.primitive_var_infos.push(CanonicalVarInfo { kind }); + var + }); let br = ty::BoundRegion { var, kind: BrAnon(None) }; ty::Region::new_late_bound(self.interner(), self.binder_index, br) } From 1ffc6ca9a513d1a3a928cc1d32b21d55d6326bb3 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 27 Jul 2023 04:00:18 +0000 Subject: [PATCH 2/2] Consider a goal as NOT changed if its response is identity modulo regions --- .../src/solve/eval_ctxt.rs | 2 +- tests/ui/impl-trait/autoderef.rs | 2 +- ...dont-loop-fulfill-on-region-constraints.rs | 32 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 tests/ui/traits/new-solver/dont-loop-fulfill-on-region-constraints.rs diff --git a/compiler/rustc_trait_selection/src/solve/eval_ctxt.rs b/compiler/rustc_trait_selection/src/solve/eval_ctxt.rs index f48a992d32758..9e3b0bbc4fb26 100644 --- a/compiler/rustc_trait_selection/src/solve/eval_ctxt.rs +++ b/compiler/rustc_trait_selection/src/solve/eval_ctxt.rs @@ -344,7 +344,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> { Ok(response) => response, }; - let has_changed = !canonical_response.value.var_values.is_identity() + let has_changed = !canonical_response.value.var_values.is_identity_modulo_regions() || !canonical_response.value.external_constraints.opaque_types.is_empty(); let (certainty, nested_goals) = match self.instantiate_and_apply_query_response( goal.param_env, diff --git a/tests/ui/impl-trait/autoderef.rs b/tests/ui/impl-trait/autoderef.rs index 0d07a549640f0..cd2cdd9e3b3cf 100644 --- a/tests/ui/impl-trait/autoderef.rs +++ b/tests/ui/impl-trait/autoderef.rs @@ -1,5 +1,5 @@ // revisions: current next -//[next] compile-flag: -Ztrait-solver=next +//[next] compile-flags: -Ztrait-solver=next // check-pass use std::path::Path; diff --git a/tests/ui/traits/new-solver/dont-loop-fulfill-on-region-constraints.rs b/tests/ui/traits/new-solver/dont-loop-fulfill-on-region-constraints.rs new file mode 100644 index 0000000000000..b241e3bf86521 --- /dev/null +++ b/tests/ui/traits/new-solver/dont-loop-fulfill-on-region-constraints.rs @@ -0,0 +1,32 @@ +// compile-flags: -Ztrait-solver=next +// check-pass + +trait Eq<'a, 'b, T> {} + +trait Ambig {} +impl Ambig for () {} + +impl<'a, T> Eq<'a, 'a, T> for () where T: Ambig {} + +fn eq<'a, 'b, T>(t: T) +where + (): Eq<'a, 'b, T>, +{ +} + +fn test<'r>() { + let mut x = Default::default(); + + // When we evaluate `(): Eq<'r, 'r, ?0>` we uniquify the regions. + // That leads us to evaluate `(): Eq<'?0, '?1, ?0>`. The response of this + // will be ambiguous (because `?0: Ambig` is ambig) and also not an "identity" + // response, since the region constraints will contain `'?0 == '?1` (so + // `is_changed` will return true). Since it's both ambig and changed, + // fulfillment will both re-register the goal AND loop again. This hits the + // overflow limit. This should neither be considered overflow, nor ICE. + eq::<'r, 'r, _>(x); + + x = (); +} + +fn main() {}