From 1ef87c6bcd2880e8d250d708e2b93118d6ae8c35 Mon Sep 17 00:00:00 2001 From: lcnr Date: Wed, 28 Oct 2026 16:58:02 +0100 Subject: [PATCH] do not prefer trivially true candidates --- .../src/solve/assembly/mod.rs | 4 +- .../src/solve/eval_ctxt/mod.rs | 63 ++++++++++++- .../rustc_next_trait_solver/src/solve/mod.rs | 93 ++++++------------- .../src/solve/trait_goals.rs | 40 ++------ ...-bound-eq-modulo-placeholder-constraint.rs | 29 ++++++ .../cycles/inductive-cycle-but-ok.rs | 44 --------- ...cycle-discarded-coinductive-constraints.rs | 36 ------- 7 files changed, 124 insertions(+), 185 deletions(-) create mode 100644 tests/ui/traits/next-solver/assembly/item-bound-eq-modulo-placeholder-constraint.rs delete mode 100644 tests/ui/traits/next-solver/cycles/inductive-cycle-but-ok.rs delete mode 100644 tests/ui/traits/next-solver/cycles/inductive-cycle-discarded-coinductive-constraints.rs diff --git a/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs b/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs index dcab703546a7e..a06b0625654c2 100644 --- a/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs +++ b/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs @@ -1339,7 +1339,7 @@ where candidates.retain(|c| matches!(c.source, CandidateSource::ParamEnv(_))); } - if let Some((response, _)) = self.try_merge_candidates(&candidates) { + if let Some(response) = self.try_merge_candidates(&candidates) { Ok(response) } else { self.flounder(&candidates).map_err(Into::into) @@ -1361,7 +1361,7 @@ where // means we can just ignore inference constraints and don't have to special-case // constraining the normalized-to `term`. self.filter_specialized_impls(AllowInferenceConstraints::Yes, &mut candidates); - if let Some((response, _)) = self.try_merge_candidates(&candidates) { + if let Some(response) = self.try_merge_candidates(&candidates) { Ok(response) } else { self.flounder(&candidates).map_err(Into::into) diff --git a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs index 16d8289fc9b2d..13281ad706003 100644 --- a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs +++ b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs @@ -18,9 +18,10 @@ use rustc_type_ir::solve::{ }; use rustc_type_ir::transmute::Answer; use rustc_type_ir::{ - self as ty, ClauseKind, Const, InferCtxtLike, Interner, MayBeErased, OpaqueTypeKey, - PredicateKind, PredicateProxy, Region, RegionVid, TypeFoldable, TypeSuperVisitable, - TypeVisitable, TypeVisitableExt, TypeVisitor, TypingMode, max_universe, + self as ty, CanonicalQueryInput, ClauseKind, Const, InferCtxtLike, Interner, MayBeErased, + OpaqueTypeKey, PredicateKind, PredicateProxy, Region, RegionVid, TypeFoldable, + TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor, TypingMode, + TypingModeEqWrapper, max_universe, }; use thin_vec::ThinVec; use tracing::{Level, debug, instrument, trace, warn}; @@ -583,6 +584,62 @@ where ) } + /// Whether two canonical responses are exactly equal except for their + /// region constraints. + pub(super) fn responses_equal_modulo_region_constraints( + &self, + a: &CanonicalResponse, + b: &CanonicalResponse, + ) -> bool { + if a == b { + return true; + } + + // We currently only ever use this function if `a` doesn't have region constraints. + // This means we only recanonicalize `b`. Could change this if necessary. + debug_assert!(a.value.external_constraints.region_constraints.is_empty()); + + // This is quite ugly. We instantiate the response as if it were a query input + // and then recanonicalize it without its region constraints. + // + // We can't simply compare all other fields of the response here as there can + // be some inference variables and placeholders created inside of a query which + // are only mentioned in the region constraints. This then changes the indices + // of things canonicalized later. + let cx: I = self.cx(); + let input: CanonicalQueryInput> = CanonicalQueryInput { + canonical: b.clone(), + typing_mode: TypingModeEqWrapper(self.typing_mode()), + }; + let (ref delegate, instantiated_response, _) = D::build_with_canonical(cx, &input); + let Response { certainty, var_values, var_values_is_identity, external_constraints } = + instantiated_response; + let ExternalConstraintsData { + region_constraints: _, + opaque_types, + normalization_nested_goals, + } = &*external_constraints; + let b_no_regions = canonicalize_response( + delegate, + ty::UniverseIndex::ROOT, + Response { + certainty, + var_values, + var_values_is_identity, + external_constraints: self.cx().mk_external_constraints(ExternalConstraintsData { + region_constraints: match cx.assumptions_on_binders() { + true => ExternalRegionConstraints::NextGen(RegionConstraint::new_true()), + false => ExternalRegionConstraints::Old(vec![]), + }, + opaque_types: opaque_types.clone(), + normalization_nested_goals: normalization_nested_goals.clone(), + }), + }, + ); + + a == &b_no_regions + } + pub(super) fn ignore_candidate_head_usages(&mut self, usages: CandidateHeadUsages) { self.search_graph.ignore_candidate_head_usages(usages); } diff --git a/compiler/rustc_next_trait_solver/src/solve/mod.rs b/compiler/rustc_next_trait_solver/src/solve/mod.rs index 4225280ea5b93..d46d40a0ec31a 100644 --- a/compiler/rustc_next_trait_solver/src/solve/mod.rs +++ b/compiler/rustc_next_trait_solver/src/solve/mod.rs @@ -82,35 +82,6 @@ fn has_only_region_constraints(response: ty::Canonical( - a: &CanonicalResponse, - b: &CanonicalResponse, -) -> bool { - let CanonicalResponse { - max_universe: a_max_universe, - var_kinds: a_var_kinds, - value: - Response { - certainty: a_certainty, - var_values: a_var_values, - var_values_is_identity: _, // ignore because it's purely derived from `var_values` - external_constraints: a_external_constraints, - }, - } = a; - - let ExternalConstraintsData { region_constraints: _, opaque_types, normalization_nested_goals } = - &**a_external_constraints; - - a_max_universe == &b.max_universe - && a_var_kinds == &b.var_kinds - && a_var_values == &b.value.var_values - && a_certainty == &b.value.certainty - && opaque_types == &b.value.external_constraints.opaque_types - && normalization_nested_goals == &b.value.external_constraints.normalization_nested_goals -} - impl<'a, D, I> EvalCtxt<'a, D> where D: SolverDelegate, @@ -310,62 +281,50 @@ where } } -#[derive(Debug)] -enum MergeCandidateInfo { - AlwaysApplicable(usize), - EqualResponse, -} - impl EvalCtxt<'_, D> where D: SolverDelegate, I: Interner, { /// Try to merge multiple possible ways to prove a goal, if that is not possible returns `None`. + /// We only merge candidates which are equal modulo region constraints. Long-term, we want to + /// emit an OR of the constraints of all candidates, for now we fail with ambiguity unless all + /// candidates have the same constraints or one candidate has no region constraints at all. /// /// In this case we tend to flounder and return ambiguity by calling `[EvalCtxt::flounder]`. #[instrument(level = "trace", skip(self), ret)] fn try_merge_candidates( &mut self, candidates: &[Candidate], - ) -> Option<(CanonicalResponse, MergeCandidateInfo)> { - if candidates.is_empty() { + ) -> Option> { + let Some((first, rest)) = candidates.split_first() else { return None; - } - - let always_applicable = candidates.iter().enumerate().find(|(_, candidate)| { - candidate.result.value.certainty == Certainty::Yes - && has_no_inference_or_external_constraints(candidate.result) - }); + }; - if let Some((i, candidate)) = always_applicable { - return Some((candidate.result, MergeCandidateInfo::AlwaysApplicable(i))); + // If all candidates are equal, just return that result. + if rest.iter().all(|c| first.result == c.result) { + return Some(first.result); } - let one: CanonicalResponse = candidates[0].result; - - if candidates[1..] + // If not, check whether one of the candidates has no region constraints. + // Long-term we likely want to also handle cases where one candidate has + // strictly weaker region constraints or emit an OR of the region constraints + // if candidates are otherwise equal. + // + // While we could also prefer candidates with weaker non-region constraints, + // that's currently not necessary. We intentionally do not prefer candidates + // if there's still am ambiguous candidate. We do so to potentially better + // handle region uniquification at the start of MIR borrowck, see #163928 for + // more details. + if let Some(choice) = candidates .iter() - .all(|candidate| equal_response_modulo_region_constraints(&one, &candidate.result)) + .find(|c| c.result.value.external_constraints.region_constraints.is_empty()) { - let region_constraints = &one.value.external_constraints.region_constraints; - if candidates[1..].iter().all(|candidate| { - &candidate.result.value.external_constraints.region_constraints - == region_constraints - }) { - return Some((one, MergeCandidateInfo::EqualResponse)); - } - - // If candidates differ only in region constraints, their merged region - // constraints are an `Or` of their respective constraints. If one of them - // has no region constraints, the `Or` constraint evaluates to `true`. - // - // This is a special case of `-Zassumptions-on-binders` and should be - // replaced eventually. - if let Some(candidate) = candidates.iter().find(|candidate| { - candidate.result.value.external_constraints.region_constraints.is_empty() - }) { - return Some((candidate.result, MergeCandidateInfo::EqualResponse)); + if candidates + .iter() + .all(|c| self.responses_equal_modulo_region_constraints(&choice.result, &c.result)) + { + return Some(choice.result); } } diff --git a/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs b/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs index 38bdaa898c455..5ae19b2eb6935 100644 --- a/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs +++ b/compiler/rustc_next_trait_solver/src/solve/trait_goals.rs @@ -24,7 +24,7 @@ use crate::solve::assembly::{ use crate::solve::inspect::ProbeKind; use crate::solve::{ BuiltinImplSource, CandidateSource, Certainty, EvalCtxt, Goal, GoalSource, MaybeCause, - MergeCandidateInfo, NoSolution, ParamEnvSource, StalledOnCoroutines, flatten_answer_tree, + NoSolution, ParamEnvSource, StalledOnCoroutines, flatten_answer_tree, has_only_region_constraints, }; @@ -1600,7 +1600,7 @@ where .all(|candidate| matches!(candidate.source, CandidateSource::BuiltinImpl(_))); if is_marker || all_builtin { - self.try_merge_candidates(candidates).map(|(response, _)| response) + self.try_merge_candidates(candidates) } else if candidates.len() > 1 { None } else { @@ -1613,7 +1613,7 @@ where candidates: &[Candidate], proven_via: TraitGoalProvenVia, ) -> (CanonicalResponse, Option) { - if let Some((response, _)) = self.try_merge_candidates(candidates) { + if let Some(response) = self.try_merge_candidates(candidates) { (response, Some(proven_via)) } else { (self.bail_with_ambiguity(candidates), None) @@ -1678,6 +1678,7 @@ where let alias_bounds: Vec<_> = candidates .extract_if(.., |c| matches!(c.source, CandidateSource::AliasBound(..))) .collect(); + candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages)); return Ok(self.merge_candidates_or_bail_with_ambiguity( &alias_bounds, TraitGoalProvenVia::AliasBound, @@ -1693,38 +1694,10 @@ where let where_bounds: Vec<_> = candidates .extract_if(.., |c| matches!(c.source, CandidateSource::ParamEnv(_))) .collect(); - let Some((response, info)) = self.try_merge_candidates(&where_bounds) else { + candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages)); + let Some(response) = self.try_merge_candidates(&where_bounds) else { return Ok((self.bail_with_ambiguity(&where_bounds), None)); }; - match info { - // If there's an always applicable candidate, the result of all - // other candidates does not matter. This means we can ignore - // them when checking whether we've reached a fixpoint. - // - // We always prefer the first always applicable candidate, even if a - // later candidate is also always applicable and would result in fewer - // reruns. We could slightly improve this by e.g. searching for another - // always applicable candidate which doesn't depend on any cycle heads. - // - // NOTE: This is optimization is observable in case there is an always - // applicable global candidate and another non-global candidate which only - // applies because of a provisional result. I can't even think of a test - // case where this would occur and even then, this would not be unsound. - // Supporting this makes the code more involved, so I am just going to - // ignore this for now. - MergeCandidateInfo::AlwaysApplicable(i) => { - for (j, c) in where_bounds.into_iter().enumerate() { - if i != j { - self.ignore_candidate_head_usages(c.head_usages) - } - } - // If a where-bound does not apply, we don't actually get a - // candidate for it. We manually track the head usages - // of all failed `ParamEnv` candidates instead. - self.ignore_candidate_head_usages(failed_candidate_info.param_env_head_usages); - } - MergeCandidateInfo::EqualResponse => {} - } return Ok((response, Some(TraitGoalProvenVia::ParamEnv))); } @@ -1733,6 +1706,7 @@ where let alias_bounds: Vec<_> = candidates .extract_if(.., |c| matches!(c.source, CandidateSource::AliasBound(_))) .collect(); + candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages)); return Ok(self.merge_candidates_or_bail_with_ambiguity( &alias_bounds, TraitGoalProvenVia::AliasBound, diff --git a/tests/ui/traits/next-solver/assembly/item-bound-eq-modulo-placeholder-constraint.rs b/tests/ui/traits/next-solver/assembly/item-bound-eq-modulo-placeholder-constraint.rs new file mode 100644 index 0000000000000..7e994a96890fd --- /dev/null +++ b/tests/ui/traits/next-solver/assembly/item-bound-eq-modulo-placeholder-constraint.rs @@ -0,0 +1,29 @@ +//@ revisions: current next +//@ ignore-compare-mode-next-solver (explicit revisions) +//@[next] compile-flags: -Znext-solver +//@ check-pass + +// When proving `C::Val<'a>: for<'b> PartialEq>` there are +// two candidates: +// - `C::Val<'a>: PartialEq>` +// - `for<'b> C::Val<'a>: PartialEq>` +// +// The first candidate results in an `'!b == 'a` constraint while the +// second one has no region constraints. The constraint of the first +// candidate means the canonical response has an entry for `'b` in its +// `var_values`. This previously caused us to not prefer the second +// constraint. +// +// FIXME(trait-system-refactor-initiative#305): Now, actually, the first +// candidate should just result in a leak check failure, but doesn't. +// That's a separate issue though :> +trait Cursor { + type Val<'a>: PartialEq + for<'b> PartialEq>; +} +impl Cursor for &C { + // for<'b> C::Val<'a>: PartialEq> + // - C::Val<'a>: PartialEq>` + // - for<'b> C::Val<'a>: PartialEq> + type Val<'a> = C::Val<'a>; +} +fn main() {} diff --git a/tests/ui/traits/next-solver/cycles/inductive-cycle-but-ok.rs b/tests/ui/traits/next-solver/cycles/inductive-cycle-but-ok.rs deleted file mode 100644 index edcf2e5472b49..0000000000000 --- a/tests/ui/traits/next-solver/cycles/inductive-cycle-but-ok.rs +++ /dev/null @@ -1,44 +0,0 @@ -//@ compile-flags: -Znext-solver -//@ check-pass -#![feature(trivial_bounds, marker_trait_attr)] -#![allow(trivial_bounds)] - -// This previously triggered a bug in the provisional cache. -// -// This has the proof tree -// - `Root: Trait` proven via impl -// - `MultipleCandidates: Trait` -// - candidate: overflow-impl -// - `Root: Trait` (inductive cycle ~> OVERFLOW) -// - candidate: trivial-impl ~> YES -// - merge respones ~> YES -// - `MultipleCandidates: Trait` (in provisional cache ~> OVERFLOW) -// -// We previously incorrectly treated the `MultipleCandidates: Trait` as -// overflow because it was in the cache and reached via an inductive cycle. -// It should be `YES`. - -struct Root; -struct MultipleCandidates; - -#[marker] -trait Trait {} -impl Trait for Root -where - MultipleCandidates: Trait, - MultipleCandidates: Trait, -{} - -// overflow-impl -impl Trait for MultipleCandidates -where - Root: Trait, -{} -// trivial-impl -impl Trait for MultipleCandidates {} - -fn impls_trait() {} - -fn main() { - impls_trait::(); -} diff --git a/tests/ui/traits/next-solver/cycles/inductive-cycle-discarded-coinductive-constraints.rs b/tests/ui/traits/next-solver/cycles/inductive-cycle-discarded-coinductive-constraints.rs deleted file mode 100644 index 527ca812efb8b..0000000000000 --- a/tests/ui/traits/next-solver/cycles/inductive-cycle-discarded-coinductive-constraints.rs +++ /dev/null @@ -1,36 +0,0 @@ -//@ check-pass -//@ compile-flags: -Znext-solver -#![feature(rustc_attrs, marker_trait_attr)] -#[rustc_coinductive] -trait Trait {} - -impl Trait for (T, U) -where - (U, T): Trait, - (T, U): Inductive, - (): ConstrainToU32, -{} - -trait ConstrainToU32 {} -impl ConstrainToU32 for () {} - -// We only prefer the candidate without an inductive cycle -// once the inductive cycle has the same constraints as the -// other goal. -#[marker] -trait Inductive {} -impl Inductive for (T, U) -where - (T, U): Trait, -{} - -impl Inductive for (u32, u32) {} - -fn impls_trait() -where - (T, U): Trait, -{} - -fn main() { - impls_trait::<_, _>(); -}