From 5f548890b8f446c7681ab02be8ae62afda2b9c9f Mon Sep 17 00:00:00 2001 From: lcnr Date: Thu, 14 Nov 2024 18:50:32 +0100 Subject: [PATCH 1/2] add tests --- .../param-env-eager-norm-dedup.rs | 23 +++++++++++++++++++ .../winnowing/global-non-global-env-1.rs | 18 +++++++++++++++ .../winnowing/global-non-global-env-2.rs | 18 +++++++++++++++ .../winnowing/global-non-global-env-2.stderr | 17 ++++++++++++++ .../winnowing/global-non-global-env-3.rs | 20 ++++++++++++++++ .../winnowing/global-non-global-env-4.rs | 19 +++++++++++++++ .../winnowing/global-non-global-env-4.stderr | 17 ++++++++++++++ 7 files changed, 132 insertions(+) create mode 100644 tests/ui/const-generics/min_const_generics/param-env-eager-norm-dedup.rs create mode 100644 tests/ui/traits/winnowing/global-non-global-env-1.rs create mode 100644 tests/ui/traits/winnowing/global-non-global-env-2.rs create mode 100644 tests/ui/traits/winnowing/global-non-global-env-2.stderr create mode 100644 tests/ui/traits/winnowing/global-non-global-env-3.rs create mode 100644 tests/ui/traits/winnowing/global-non-global-env-4.rs create mode 100644 tests/ui/traits/winnowing/global-non-global-env-4.stderr diff --git a/tests/ui/const-generics/min_const_generics/param-env-eager-norm-dedup.rs b/tests/ui/const-generics/min_const_generics/param-env-eager-norm-dedup.rs new file mode 100644 index 0000000000000..9600b3875ba48 --- /dev/null +++ b/tests/ui/const-generics/min_const_generics/param-env-eager-norm-dedup.rs @@ -0,0 +1,23 @@ +//@ check-pass + +// This caused a regression in a crater run in #132325. +// +// The underlying issue is a really subtle implementation detail. +// +// When building the `param_env` for `Trait` we start out with its +// explicit predicates `Self: Trait` and `Self: for<'a> Super<'a, { 1 + 1 }>`. +// +// When normalizing the environment we also elaborate. This implicitly +// deduplicates its returned predicates. We currently first eagerly +// normalize constants in the unnormalized param env to avoid issues +// caused by our lack of deferred alias equality. +// +// So we actually elaborate `Self: Trait` and `Self: for<'a> Super<'a, 2>`, +// resulting in a third `Self: for<'a> Super<'a, { 1 + 1 }>` predicate which +// then gets normalized to `Self: for<'a> Super<'a, 2>` at which point we +// do not deduplicate however. By failing to handle equal where-bounds in +// candidate selection, this caused ambiguity when checking that `Trait` is +// well-formed. +trait Super<'a, const N: usize> {} +trait Trait: for<'a> Super<'a, { 1 + 1 }> {} +fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-1.rs b/tests/ui/traits/winnowing/global-non-global-env-1.rs new file mode 100644 index 0000000000000..d232d32dddffd --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-1.rs @@ -0,0 +1,18 @@ +//@ check-pass + +// A regression test for an edge case of candidate selection +// in the old trait solver, see #132325 for more details. + +trait Trait {} +impl Trait for () {} + +fn impls_trait, U>(_: T) -> U { todo!() } +fn foo() -> u32 +where + (): Trait, + (): Trait, +{ + impls_trait(()) +} + +fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-2.rs b/tests/ui/traits/winnowing/global-non-global-env-2.rs new file mode 100644 index 0000000000000..1647606934660 --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-2.rs @@ -0,0 +1,18 @@ +// A regression test for an edge case of candidate selection +// in the old trait solver, see #132325 for more details. Unlike +// the first test, this one has two impl candidates. + +trait Trait {} +impl Trait for () {} +impl Trait for () {} + +fn impls_trait, U>(_: T) -> U { todo!() } +fn foo() -> u32 +where + (): Trait, + (): Trait, +{ + impls_trait(()) //~ ERROR mismatched types +} + +fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-2.stderr b/tests/ui/traits/winnowing/global-non-global-env-2.stderr new file mode 100644 index 0000000000000..dee01c72e4820 --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-2.stderr @@ -0,0 +1,17 @@ +error[E0308]: mismatched types + --> $DIR/global-non-global-env-2.rs:15:5 + | +LL | fn foo() -> u32 + | - --- expected `u32` because of return type + | | + | found this type parameter +... +LL | impls_trait(()) + | ^^^^^^^^^^^^^^^ expected `u32`, found type parameter `T` + | + = note: expected type `u32` + found type parameter `T` + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0308`. diff --git a/tests/ui/traits/winnowing/global-non-global-env-3.rs b/tests/ui/traits/winnowing/global-non-global-env-3.rs new file mode 100644 index 0000000000000..008d07e41446a --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-3.rs @@ -0,0 +1,20 @@ +//@ check-pass + +// A regression test for an edge case of candidate selection +// in the old trait solver, see #132325 for more details. Unlike +// the second test, the where-bounds are in a different order. + +trait Trait {} +impl Trait for () {} +impl Trait for () {} + +fn impls_trait, U>(_: T) -> U { todo!() } +fn foo() -> u32 +where + (): Trait, + (): Trait, +{ + impls_trait(()) +} + +fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-4.rs b/tests/ui/traits/winnowing/global-non-global-env-4.rs new file mode 100644 index 0000000000000..1f35cf20f83dc --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-4.rs @@ -0,0 +1,19 @@ +// A regression test for an edge case of candidate selection +// in the old trait solver, see #132325 for more details. Unlike +// the third test, this one has 3 impl candidates. + +trait Trait {} +impl Trait for () {} +impl Trait for () {} +impl Trait for () {} + +fn impls_trait, U>(_: T) -> U { todo!() } +fn foo() -> u32 +where + (): Trait, + (): Trait, +{ + impls_trait(()) //~ ERROR mismatched types +} + +fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-4.stderr b/tests/ui/traits/winnowing/global-non-global-env-4.stderr new file mode 100644 index 0000000000000..6449f3feb08cf --- /dev/null +++ b/tests/ui/traits/winnowing/global-non-global-env-4.stderr @@ -0,0 +1,17 @@ +error[E0308]: mismatched types + --> $DIR/global-non-global-env-4.rs:16:5 + | +LL | fn foo() -> u32 + | - --- expected `u32` because of return type + | | + | found this type parameter +... +LL | impls_trait(()) + | ^^^^^^^^^^^^^^^ expected `u32`, found type parameter `T` + | + = note: expected type `u32` + found type parameter `T` + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0308`. From 3350b9faadd8e9c3cf87a9c8de266ea252a67c5c Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 29 Oct 2024 16:10:13 +0100 Subject: [PATCH 2/2] consistently handle global where-bounds --- compiler/rustc_trait_selection/src/lib.rs | 1 + .../src/traits/select/mod.rs | 546 ++++++++---------- .../winnowing/global-non-global-env-2.rs | 4 +- .../winnowing/global-non-global-env-2.stderr | 17 - .../winnowing/global-non-global-env-4.rs | 4 +- .../winnowing/global-non-global-env-4.stderr | 17 - 6 files changed, 248 insertions(+), 341 deletions(-) delete mode 100644 tests/ui/traits/winnowing/global-non-global-env-2.stderr delete mode 100644 tests/ui/traits/winnowing/global-non-global-env-4.stderr diff --git a/compiler/rustc_trait_selection/src/lib.rs b/compiler/rustc_trait_selection/src/lib.rs index 11d72106b221f..1af383e9200d0 100644 --- a/compiler/rustc_trait_selection/src/lib.rs +++ b/compiler/rustc_trait_selection/src/lib.rs @@ -23,6 +23,7 @@ #![feature(extract_if)] #![feature(if_let_guard)] #![feature(iter_intersperse)] +#![feature(iterator_try_reduce)] #![feature(let_chains)] #![feature(never_type)] #![feature(rustdoc_internals)] diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 25fe43e3a0e6f..32a2d5a6d93a6 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -445,7 +445,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // Winnow, but record the exact outcome of evaluation, which // is needed for specialization. Propagate overflow if it occurs. - let mut candidates = candidates + let candidates = candidates .into_iter() .map(|c| match self.evaluate_candidate(stack, &c) { Ok(eval) if eval.may_apply() => { @@ -458,40 +458,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { .flat_map(Result::transpose) .collect::, _>>()?; - debug!(?stack, ?candidates, "winnowed to {} candidates", candidates.len()); - - let has_non_region_infer = stack.obligation.predicate.has_non_region_infer(); - - // If there are STILL multiple candidates, we can further - // reduce the list by dropping duplicates -- including - // resolving specializations. - if candidates.len() > 1 { - let mut i = 0; - while i < candidates.len() { - let should_drop_i = (0..candidates.len()).filter(|&j| i != j).any(|j| { - self.candidate_should_be_dropped_in_favor_of( - &candidates[i], - &candidates[j], - has_non_region_infer, - ) == DropVictim::Yes - }); - if should_drop_i { - debug!(candidate = ?candidates[i], "Dropping candidate #{}/{}", i, candidates.len()); - candidates.swap_remove(i); - } else { - debug!(candidate = ?candidates[i], "Retaining candidate #{}/{}", i, candidates.len()); - i += 1; - - // If there are *STILL* multiple candidates, give up - // and report ambiguity. - if i > 1 { - debug!("multiple matches, ambig"); - return Ok(None); - } - } - } - } - + debug!(?stack, ?candidates, "{} potentially applicable candidates", candidates.len()); // If there are *NO* candidates, then there are no impls -- // that we know of, anyway. Note that in the case where there // are unbound type variables within the obligation, it might @@ -508,13 +475,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { // to have emitted at least one. if stack.obligation.predicate.references_error() { debug!(?stack.obligation.predicate, "found error type in predicate, treating as ambiguous"); - return Ok(None); + Ok(None) + } else { + Err(Unimplemented) + } + } else { + let has_non_region_infer = stack.obligation.predicate.has_non_region_infer(); + if let Some(candidate) = self.winnow_candidates(has_non_region_infer, candidates) { + self.filter_reservation_impls(candidate) + } else { + Ok(None) } - return Err(Unimplemented); } - - // Just one candidate left. - self.filter_reservation_impls(candidates.pop().unwrap().candidate) } /////////////////////////////////////////////////////////////////////////// @@ -1803,18 +1775,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -enum DropVictim { - Yes, - No, -} - -impl DropVictim { - fn drop_if(should_drop: bool) -> DropVictim { - if should_drop { DropVictim::Yes } else { DropVictim::No } - } -} - /// ## Winnowing /// /// Winnowing is the process of attempting to resolve ambiguity by @@ -1822,131 +1782,149 @@ impl DropVictim { /// type variables and then we also attempt to evaluate recursive /// bounds to see if they are satisfied. impl<'tcx> SelectionContext<'_, 'tcx> { - /// Returns `DropVictim::Yes` if `victim` should be dropped in favor of - /// `other`. Generally speaking we will drop duplicate - /// candidates and prefer where-clause candidates. + /// If there are multiple ways to prove a trait goal, we make some + /// *fairly arbitrary* choices about which candidate is actually used. /// - /// See the comment for "SelectionCandidate" for more details. - #[instrument(level = "debug", skip(self))] - fn candidate_should_be_dropped_in_favor_of( + /// For more details, look at the implementation of this method :) + #[instrument(level = "debug", skip(self), ret)] + fn winnow_candidates( &mut self, - victim: &EvaluatedCandidate<'tcx>, - other: &EvaluatedCandidate<'tcx>, has_non_region_infer: bool, - ) -> DropVictim { - if victim.candidate == other.candidate { - return DropVictim::Yes; + mut candidates: Vec>, + ) -> Option> { + if candidates.len() == 1 { + return Some(candidates.pop().unwrap().candidate); + } + + // We prefer trivial builtin candidates, i.e. builtin impls without any nested + // requirements, over all others. This is a fix for #53123 and prevents winnowing + // from accidentally extending the lifetime of a variable. + let mut trivial_builtin = candidates + .iter() + .filter(|c| matches!(c.candidate, BuiltinCandidate { has_nested: false })); + if let Some(_trivial) = trivial_builtin.next() { + // There should only ever be a single trivial builtin candidate + // as they would otherwise overlap. + debug_assert_eq!(trivial_builtin.next(), None); + return Some(BuiltinCandidate { has_nested: false }); } - // Check if a bound would previously have been removed when normalizing - // the param_env so that it can be given the lowest priority. See - // #50825 for the motivation for this. - let is_global = - |cand: ty::PolyTraitPredicate<'tcx>| cand.is_global() && !cand.has_bound_vars(); + // Before we consider where-bounds, we have to deduplicate them here and also + // drop where-bounds in case the same where-bound exists without bound vars. + // This is necessary as elaborating super-trait bounds may result in duplicates. + 'search_victim: loop { + for (i, this) in candidates.iter().enumerate() { + let ParamCandidate(this) = this.candidate else { continue }; + for (j, other) in candidates.iter().enumerate() { + if i == j { + continue; + } - // (*) Prefer `BuiltinCandidate { has_nested: false }`, `PointeeCandidate`, - // or `DiscriminantKindCandidate` to anything else. - // - // This is a fix for #53123 and prevents winnowing from accidentally extending the - // lifetime of a variable. - match (&other.candidate, &victim.candidate) { - // FIXME(@jswrenn): this should probably be more sophisticated - (TransmutabilityCandidate, _) | (_, TransmutabilityCandidate) => DropVictim::No, - - // (*) - (BuiltinCandidate { has_nested: false }, _) => DropVictim::Yes, - (_, BuiltinCandidate { has_nested: false }) => DropVictim::No, - - (ParamCandidate(other), ParamCandidate(victim)) => { - let same_except_bound_vars = other.skip_binder().trait_ref - == victim.skip_binder().trait_ref - && other.skip_binder().polarity == victim.skip_binder().polarity - && !other.skip_binder().trait_ref.has_escaping_bound_vars(); - if same_except_bound_vars { - // See issue #84398. In short, we can generate multiple ParamCandidates which are - // the same except for unused bound vars. Just pick the one with the fewest bound vars - // or the current one if tied (they should both evaluate to the same answer). This is - // probably best characterized as a "hack", since we might prefer to just do our - // best to *not* create essentially duplicate candidates in the first place. - DropVictim::drop_if(other.bound_vars().len() <= victim.bound_vars().len()) - } else { - DropVictim::No + let ParamCandidate(other) = other.candidate else { continue }; + if this == other { + candidates.remove(j); + continue 'search_victim; + } + + if this.skip_binder().trait_ref == other.skip_binder().trait_ref + && this.skip_binder().polarity == other.skip_binder().polarity + && !this.skip_binder().trait_ref.has_escaping_bound_vars() + { + candidates.remove(j); + continue 'search_victim; + } } } - ( - ParamCandidate(other_cand), - ImplCandidate(..) - | AutoImplCandidate - | ClosureCandidate { .. } - | AsyncClosureCandidate - | AsyncFnKindHelperCandidate - | CoroutineCandidate - | FutureCandidate - | IteratorCandidate - | AsyncIteratorCandidate - | FnPointerCandidate { .. } - | BuiltinObjectCandidate - | BuiltinUnsizeCandidate - | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { .. } - | TraitAliasCandidate - | ObjectCandidate(_) - | ProjectionCandidate(_), - ) => { - // We have a where clause so don't go around looking - // for impls. Arbitrarily give param candidates priority - // over projection and object candidates. - // - // Global bounds from the where clause should be ignored - // here (see issue #50825). - DropVictim::drop_if(!is_global(*other_cand)) - } - (ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(victim_cand)) => { - // Prefer these to a global where-clause bound - // (see issue #50825). - if is_global(*victim_cand) { DropVictim::Yes } else { DropVictim::No } + break; + } + + // The next highest priority is for non-global where-bounds. However, while we don't + // prefer global where-clauses here, we do bail with ambiguity when encountering both + // a global and a non-global where-clause. + // + // Our handling of where-bounds is generally fairly messy but necessary for backwards + // compatability, see #50825 for why we need to handle global where-bounds like this. + let is_global = |c: ty::PolyTraitPredicate<'tcx>| c.is_global() && !c.has_bound_vars(); + let param_candidates = candidates + .iter() + .filter_map(|c| if let ParamCandidate(p) = c.candidate { Some(p) } else { None }); + let mut has_global_bounds = false; + let mut param_candidate = None; + for c in param_candidates { + if is_global(c) { + has_global_bounds = true; + } else if param_candidate.replace(c).is_some() { + // Ambiguity, two potentially different where-clauses + return None; } - ( - ImplCandidate(_) - | AutoImplCandidate - | ClosureCandidate { .. } - | AsyncClosureCandidate - | AsyncFnKindHelperCandidate - | CoroutineCandidate - | FutureCandidate - | IteratorCandidate - | AsyncIteratorCandidate - | FnPointerCandidate { .. } - | BuiltinObjectCandidate - | BuiltinUnsizeCandidate - | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { has_nested: true } - | TraitAliasCandidate, - ParamCandidate(victim_cand), - ) => { - // Prefer these to a global where-clause bound - // (see issue #50825). - DropVictim::drop_if( - is_global(*victim_cand) && other.evaluation.must_apply_modulo_regions(), - ) + } + if let Some(predicate) = param_candidate { + // Ambiguity, a global and a non-global where-bound. + if has_global_bounds { + return None; + } else { + return Some(ParamCandidate(predicate)); } + } + + // Prefer alias-bounds over blanket impls for rigid associated types. This is + // fairly arbitrary but once again necessary for backwards compatibility. + // If there are multiple applicable candidates which don't affect type inference, + // choose the one with the lowest index. + let alias_bound = candidates + .iter() + .filter_map(|c| if let ProjectionCandidate(i) = c.candidate { Some(i) } else { None }) + .try_reduce(|c1, c2| if has_non_region_infer { None } else { Some(c1.min(c2)) }); + match alias_bound { + Some(Some(index)) => return Some(ProjectionCandidate(index)), + Some(None) => {} + None => return None, + } + + // Need to prioritize builtin trait object impls as `::type_id` + // should use the vtable method and not the method provided by the user-defined + // impl `impl Any for T { .. }`. This really shouldn't exist but is + // necessary due to #57893. We again arbitrarily prefer the applicable candidate + // with the lowest index. + let object_bound = candidates + .iter() + .filter_map(|c| if let ObjectCandidate(i) = c.candidate { Some(i) } else { None }) + .try_reduce(|c1, c2| if has_non_region_infer { None } else { Some(c1.min(c2)) }); + match object_bound { + Some(Some(index)) => return Some(ObjectCandidate(index)), + Some(None) => {} + None => return None, + } - (ProjectionCandidate(i), ProjectionCandidate(j)) - | (ObjectCandidate(i), ObjectCandidate(j)) => { - // Arbitrarily pick the lower numbered candidate for backwards - // compatibility reasons. Don't let this affect inference. - DropVictim::drop_if(i < j && !has_non_region_infer) + // Finally, handle overlapping user-written impls. + let impls = candidates.iter().filter_map(|c| { + if let ImplCandidate(def_id) = c.candidate { + Some((def_id, c.evaluation)) + } else { + None } - (ObjectCandidate(_), ProjectionCandidate(_)) - | (ProjectionCandidate(_), ObjectCandidate(_)) => { - bug!("Have both object and projection candidate") + }); + let mut impl_candidate = None; + for c in impls { + if let Some(prev) = impl_candidate.replace(c) { + if self.prefer_lhs_over_victim(has_non_region_infer, c, prev) { + // Ok, prefer `c` over the previous entry + } else if self.prefer_lhs_over_victim(has_non_region_infer, prev, c) { + // Ok, keep `prev` instead of the new entry + impl_candidate = Some(prev); + } else { + // Ambiguity, two potentially different where-clauses + return None; + } } - - // Arbitrarily give projection and object candidates priority. - ( - ObjectCandidate(_) | ProjectionCandidate(_), - ImplCandidate(..) + } + if let Some((def_id, _evaluation)) = impl_candidate { + // Don't use impl candidates which overlap with other candidates. + // This should pretty much only ever happen with malformed impls. + if candidates.iter().all(|c| match c.candidate { + BuiltinCandidate { has_nested: _ } + | TransmutabilityCandidate | AutoImplCandidate | ClosureCandidate { .. } | AsyncClosureCandidate @@ -1955,155 +1933,113 @@ impl<'tcx> SelectionContext<'_, 'tcx> { | FutureCandidate | IteratorCandidate | AsyncIteratorCandidate - | FnPointerCandidate { .. } - | BuiltinObjectCandidate - | BuiltinUnsizeCandidate + | FnPointerCandidate + | TraitAliasCandidate | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { .. } - | TraitAliasCandidate, - ) => DropVictim::Yes, - - ( - ImplCandidate(..) - | AutoImplCandidate - | ClosureCandidate { .. } - | AsyncClosureCandidate - | AsyncFnKindHelperCandidate - | CoroutineCandidate - | FutureCandidate - | IteratorCandidate - | AsyncIteratorCandidate - | FnPointerCandidate { .. } | BuiltinObjectCandidate - | BuiltinUnsizeCandidate - | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { .. } - | TraitAliasCandidate, - ObjectCandidate(_) | ProjectionCandidate(_), - ) => DropVictim::No, - - (&ImplCandidate(other_def), &ImplCandidate(victim_def)) => { - // See if we can toss out `victim` based on specialization. - // While this requires us to know *for sure* that the `other` impl applies - // we still use modulo regions here. - // - // This is fine as specialization currently assumes that specializing - // impls have to be always applicable, meaning that the only allowed - // region constraints may be constraints also present on the default impl. - let tcx = self.tcx(); - if other.evaluation.must_apply_modulo_regions() - && tcx.specializes((other_def, victim_def)) - { - return DropVictim::Yes; - } - - match tcx.impls_are_allowed_to_overlap(other_def, victim_def) { - // For #33140 the impl headers must be exactly equal, the trait must not have - // any associated items and there are no where-clauses. - // - // We can just arbitrarily drop one of the impls. - Some(ty::ImplOverlapKind::FutureCompatOrderDepTraitObjects) => { - assert_eq!(other.evaluation, victim.evaluation); - DropVictim::Yes - } - // For candidates which already reference errors it doesn't really - // matter what we do 🤷 - Some(ty::ImplOverlapKind::Permitted { marker: false }) => { - DropVictim::drop_if(other.evaluation.must_apply_considering_regions()) - } - Some(ty::ImplOverlapKind::Permitted { marker: true }) => { - // Subtle: If the predicate we are evaluating has inference - // variables, do *not* allow discarding candidates due to - // marker trait impls. - // - // Without this restriction, we could end up accidentally - // constraining inference variables based on an arbitrarily - // chosen trait impl. - // - // Imagine we have the following code: - // - // ```rust - // #[marker] trait MyTrait {} - // impl MyTrait for u8 {} - // impl MyTrait for bool {} - // ``` - // - // And we are evaluating the predicate `<_#0t as MyTrait>`. - // - // During selection, we will end up with one candidate for each - // impl of `MyTrait`. If we were to discard one impl in favor - // of the other, we would be left with one candidate, causing - // us to "successfully" select the predicate, unifying - // _#0t with (for example) `u8`. - // - // However, we have no reason to believe that this unification - // is correct - we've essentially just picked an arbitrary - // *possibility* for _#0t, and required that this be the *only* - // possibility. - // - // Eventually, we will either: - // 1) Unify all inference variables in the predicate through - // some other means (e.g. type-checking of a function). We will - // then be in a position to drop marker trait candidates - // without constraining inference variables (since there are - // none left to constrain) - // 2) Be left with some unconstrained inference variables. We - // will then correctly report an inference error, since the - // existence of multiple marker trait impls tells us nothing - // about which one should actually apply. - DropVictim::drop_if( - !has_non_region_infer - && other.evaluation.must_apply_considering_regions(), - ) - } - None => DropVictim::No, - } + | BuiltinUnsizeCandidate => false, + // Non-global param candidates have already been handled, global + // where-bounds get ignored. + ParamCandidate(_) | ImplCandidate(_) => true, + ProjectionCandidate(_) | ObjectCandidate(_) => unreachable!(), + }) { + return Some(ImplCandidate(def_id)); + } else { + return None; } + } - (AutoImplCandidate, ImplCandidate(_)) | (ImplCandidate(_), AutoImplCandidate) => { - DropVictim::No - } + if candidates.len() == 1 { + Some(candidates.pop().unwrap().candidate) + } else { + // Also try ignoring all global where-bounds and check whether we end + // with a unique candidate in this case. + let mut not_a_global_where_bound = candidates + .into_iter() + .filter(|c| !matches!(c.candidate, ParamCandidate(p) if is_global(p))); + not_a_global_where_bound + .next() + .map(|c| c.candidate) + .filter(|_| not_a_global_where_bound.next().is_none()) + } + } - (AutoImplCandidate, _) | (_, AutoImplCandidate) => { - bug!( - "default implementations shouldn't be recorded \ - when there are other global candidates: {:?} {:?}", - other, - victim - ); + fn prefer_lhs_over_victim( + &self, + has_non_region_infer: bool, + (lhs, lhs_evaluation): (DefId, EvaluationResult), + (victim, victim_evaluation): (DefId, EvaluationResult), + ) -> bool { + let tcx = self.tcx(); + // See if we can toss out `victim` based on specialization. + // + // While this requires us to know *for sure* that the `lhs` impl applies + // we still use modulo regions here. This is fine as specialization currently + // assumes that specializing impls have to be always applicable, meaning that + // the only allowed region constraints may be constraints also present on the default impl. + if lhs_evaluation.must_apply_modulo_regions() { + if tcx.specializes((lhs, victim)) { + return true; } + } - // Everything else is ambiguous - ( - ImplCandidate(_) - | ClosureCandidate { .. } - | AsyncClosureCandidate - | AsyncFnKindHelperCandidate - | CoroutineCandidate - | FutureCandidate - | IteratorCandidate - | AsyncIteratorCandidate - | FnPointerCandidate { .. } - | BuiltinObjectCandidate - | BuiltinUnsizeCandidate - | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { has_nested: true } - | TraitAliasCandidate, - ImplCandidate(_) - | ClosureCandidate { .. } - | AsyncClosureCandidate - | AsyncFnKindHelperCandidate - | CoroutineCandidate - | FutureCandidate - | IteratorCandidate - | AsyncIteratorCandidate - | FnPointerCandidate { .. } - | BuiltinObjectCandidate - | BuiltinUnsizeCandidate - | TraitUpcastingUnsizeCandidate(_) - | BuiltinCandidate { has_nested: true } - | TraitAliasCandidate, - ) => DropVictim::No, + match tcx.impls_are_allowed_to_overlap(lhs, victim) { + // For #33140 the impl headers must be exactly equal, the trait must not have + // any associated items and there are no where-clauses. + // + // We can just arbitrarily drop one of the impls. + Some(ty::ImplOverlapKind::FutureCompatOrderDepTraitObjects) => { + assert_eq!(lhs_evaluation, victim_evaluation); + true + } + // For candidates which already reference errors it doesn't really + // matter what we do 🤷 + Some(ty::ImplOverlapKind::Permitted { marker: false }) => { + lhs_evaluation.must_apply_considering_regions() + } + Some(ty::ImplOverlapKind::Permitted { marker: true }) => { + // Subtle: If the predicate we are evaluating has inference + // variables, do *not* allow discarding candidates due to + // marker trait impls. + // + // Without this restriction, we could end up accidentally + // constraining inference variables based on an arbitrarily + // chosen trait impl. + // + // Imagine we have the following code: + // + // ```rust + // #[marker] trait MyTrait {} + // impl MyTrait for u8 {} + // impl MyTrait for bool {} + // ``` + // + // And we are evaluating the predicate `<_#0t as MyTrait>`. + // + // During selection, we will end up with one candidate for each + // impl of `MyTrait`. If we were to discard one impl in favor + // of the other, we would be left with one candidate, causing + // us to "successfully" select the predicate, unifying + // _#0t with (for example) `u8`. + // + // However, we have no reason to believe that this unification + // is correct - we've essentially just picked an arbitrary + // *possibility* for _#0t, and required that this be the *only* + // possibility. + // + // Eventually, we will either: + // 1) Unify all inference variables in the predicate through + // some other means (e.g. type-checking of a function). We will + // then be in a position to drop marker trait candidates + // without constraining inference variables (since there are + // none left to constrain) + // 2) Be left with some unconstrained inference variables. We + // will then correctly report an inference error, since the + // existence of multiple marker trait impls tells us nothing + // about which one should actually apply. + !has_non_region_infer && lhs_evaluation.must_apply_considering_regions() + } + None => false, } } } diff --git a/tests/ui/traits/winnowing/global-non-global-env-2.rs b/tests/ui/traits/winnowing/global-non-global-env-2.rs index 1647606934660..c73d0f06cd95b 100644 --- a/tests/ui/traits/winnowing/global-non-global-env-2.rs +++ b/tests/ui/traits/winnowing/global-non-global-env-2.rs @@ -1,3 +1,5 @@ +//@ check-pass + // A regression test for an edge case of candidate selection // in the old trait solver, see #132325 for more details. Unlike // the first test, this one has two impl candidates. @@ -12,7 +14,7 @@ where (): Trait, (): Trait, { - impls_trait(()) //~ ERROR mismatched types + impls_trait(()) } fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-2.stderr b/tests/ui/traits/winnowing/global-non-global-env-2.stderr deleted file mode 100644 index dee01c72e4820..0000000000000 --- a/tests/ui/traits/winnowing/global-non-global-env-2.stderr +++ /dev/null @@ -1,17 +0,0 @@ -error[E0308]: mismatched types - --> $DIR/global-non-global-env-2.rs:15:5 - | -LL | fn foo() -> u32 - | - --- expected `u32` because of return type - | | - | found this type parameter -... -LL | impls_trait(()) - | ^^^^^^^^^^^^^^^ expected `u32`, found type parameter `T` - | - = note: expected type `u32` - found type parameter `T` - -error: aborting due to 1 previous error - -For more information about this error, try `rustc --explain E0308`. diff --git a/tests/ui/traits/winnowing/global-non-global-env-4.rs b/tests/ui/traits/winnowing/global-non-global-env-4.rs index 1f35cf20f83dc..74793620c9e7d 100644 --- a/tests/ui/traits/winnowing/global-non-global-env-4.rs +++ b/tests/ui/traits/winnowing/global-non-global-env-4.rs @@ -1,3 +1,5 @@ +//@ check-pass + // A regression test for an edge case of candidate selection // in the old trait solver, see #132325 for more details. Unlike // the third test, this one has 3 impl candidates. @@ -13,7 +15,7 @@ where (): Trait, (): Trait, { - impls_trait(()) //~ ERROR mismatched types + impls_trait(()) } fn main() {} diff --git a/tests/ui/traits/winnowing/global-non-global-env-4.stderr b/tests/ui/traits/winnowing/global-non-global-env-4.stderr deleted file mode 100644 index 6449f3feb08cf..0000000000000 --- a/tests/ui/traits/winnowing/global-non-global-env-4.stderr +++ /dev/null @@ -1,17 +0,0 @@ -error[E0308]: mismatched types - --> $DIR/global-non-global-env-4.rs:16:5 - | -LL | fn foo() -> u32 - | - --- expected `u32` because of return type - | | - | found this type parameter -... -LL | impls_trait(()) - | ^^^^^^^^^^^^^^^ expected `u32`, found type parameter `T` - | - = note: expected type `u32` - found type parameter `T` - -error: aborting due to 1 previous error - -For more information about this error, try `rustc --explain E0308`.