Fix marker trait winnowing depending on impl order

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -758,7 +758,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                     if pred.0.has_free_regions()
                         || pred.0.has_bound_regions()
                         || pred.0.has_non_region_infer()
-                        || pred.0.has_non_region_infer()
+                        || pred.0.has_non_region_param()
                     {
                         Ok(EvaluatedToOkModuloRegions)
                     } else {

tests/ui/marker_trait_attr/overlapping-impl-1-modulo-regions.rs

@@ -1,12 +1,12 @@
-//@ known-bug: #109481
+//@ check-pass
+//@ revisions: current next
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@[next] compile-flags: -Znext-solver
 //
-// While the `T: Copy` is always applicable when checking
-// that the impl `impl<T: Copy> F for T {}` is well formed,
-// the old trait solver can only approximate this by checking
-// that there are no inference variables in the obligation and
-// no region constraints in the evaluation result.
+// Regression test for #109481 and #84917.  See #153847.
 //
-// Because of this we end up with ambiguity here.
+// A bug previously made marker trait winnowing order-dependent,
+// producing a spurious E0310 here.
 #![feature(marker_trait_attr)]
 
 #[marker]

tests/ui/marker_trait_attr/overlapping-impl-assoc-ty-binding-static.rs

@@ -0,0 +1,79 @@
+//@ check-pass
+//@ revisions: current next
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@[next] compile-flags: -Znext-solver
+//
+// This demonstrates that the `has_non_region_infer()`
+// disjunctive check in the `TypeOutlives` handler in
+// `evaluate_predicate_recursively` is load bearing with respect to
+// marker trait winnowing.
+//
+// It's harder than one might imagine to demonstrate this.  The outer
+// obligation must be a marker trait predicate, otherwise winnowing
+// gives up on any nontrivial multi-candidate case.  This prevents us
+// from simply making the outer obligation a type-outlives obligation.
+//
+// But the outer obligation must also be free of nonregion infer
+// vars.  Otherwise, when we're considering candidates, each
+// call to `prefer_lhs_over_victim` will return `false` and
+// we'll get ambiguity.
+//
+// Together, these imply that the infer var must come from the
+// impl.  It can't come from our starting obligation.  So we need a
+// subobligation, produced by candidate evaluation, that is both 1) a
+// `TypeOutlives` predicate and 2) has an infer var.
+//
+// To make the would-be bug visible, we need 1) the candidate with the
+// infer var to be picked as the winner, 2) the winner's WCs to fail,
+// and 3) at least one other candidate in the winnowing set that gets
+// marked `EvaluatedToOK`.
+//
+// How do we introduce an inference variable from an impl?  All
+// inference variables used in the trait ref (the trait and its
+// generic arguments, including `Self`) will have been resolved
+// and substituted by this point.  We can't simply constrain a type
+// parameter only by a lifetime -- that will produce an error about
+// the parameter not being constrained (E0207).  Fortunately, if
+// we bind that parameter to a trait associated type it will be
+// treated as constrained.  Then, we can add a type-outlives bound
+// on it.  This then survives as an infer var all the way to the
+// `TypeOutlives` handler.
+//
+// Without the `has_non_region_infer()` check in the `TypeOutlives`
+// handler, this `?U: 'static` would produce `EvaluatedToOk` rather
+// than `EvaluatedToOkModuloRegions`.  Because this impl appears
+// second, marker trait winnowing would pick it as the winner and we'd
+// register the `?U: 'static` obligation.  That would then fail, after
+// we substitute `?U = X`, and we'd get a spurious "may not live long
+// enough" (E0310) error.
+//
+// With the check, the second impl gets `EvaluatedToOkModuloRegions`,
+// so the first impl correctly wins.
+//
+// See #153847.
+#![feature(marker_trait_attr)]
+
+trait Assoc {
+    type Ty;
+}
+impl<T> Assoc for T {
+    type Ty = T;
+}
+
+#[marker]
+trait Marker {}
+
+impl<T> Marker for T where T: Assoc {}
+impl<T, U> Marker for T
+where
+    T: Assoc<Ty = U>,
+    U: 'static,
+{
+}
+
+fn requires<T: Marker>() {}
+fn use_marker<X>() {
+    requires::<X>();
+}
+
+fn main() {}

tests/ui/marker_trait_attr/overlapping-impl-infer-resolved-to-param.rs

@@ -0,0 +1,52 @@
+//@ check-pass
+//@ revisions: current next
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@[next] compile-flags: -Znext-solver
+//
+// This demonstrates that the `has_non_region_param()`
+// disjunctive check in the `TypeOutlives` handler in
+// `evaluate_predicate_recursively` is load bearing with respect to
+// marker trait winnowing.
+//
+// Conversely, this does *not* show that the `has_non_region_infer()`
+// check is load bearing -- you could be forgiven for thinking that
+// it would.  The `f::<u8, _>` will result in a `u8: Marker<?U>`
+// obligation; candidate evaluation will produce a `?U: 'static`
+// subobligation.  The infer var *will* reach the check in
+// question.  But then, in `prefer_lhs_over_victim`, we check for
+// `!has_non_region_infer` (there's a long comment there describing
+// the subtle reason why).  This causes us to defer making a
+// selection.  At a later point, when we check again, we'll have
+// equated and substituted `?U = X`, so there won't be an infer var,
+// but there will be a type parameter.
+//
+// Without the `has_non_region_param()` check in the
+// `TypeOutlives` handler, the substituted `X: 'static` obligation
+// from the impl would produce `EvaluatedToOk` rather than
+// `EvaluatedToOkModuloRegions`.  Because this impl appears second,
+// marker trait winnowing would pick it as the winner and we'd
+// register the `X: 'static` obligation.  That would then fail, and
+// we'd get a spurious "may not live long enough" (E0310) error.
+//
+// With the check, the second impl gets `EvaluatedToOkModuloRegions`,
+// so the first impl correctly wins.
+//
+// See #153847.
+#![feature(marker_trait_attr)]
+
+#[marker]
+trait Marker<T> {}
+
+impl<T> Marker<T> for u8 {}
+impl<T: 'static> Marker<T> for u8 {}
+
+fn f<T: Marker<U>, U>() -> U {
+    loop {}
+}
+
+fn g<X>() {
+    let x = f::<u8, _>();
+    let _: X = x;
+}
+
+fn main() {}

tests/ui/marker_trait_attr/overlapping-impl-projection-static.rs

@@ -0,0 +1,41 @@
+//@ check-pass
+//@ revisions: current next
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@[next] compile-flags: -Znext-solver
+//
+// This demonstrates that the `has_non_region_param()`
+// disjunctive check in the `TypeOutlives` handler in
+// `evaluate_predicate_recursively` is load bearing with respect to
+// marker trait winnowing.
+//
+// This test is interesting because the projection reaches the
+// `TypeOutlives` handler unnormalized.
+//
+// If not for the `has_non_region_param()` disjunct, the
+// `<T as Assoc>::Ty: 'static` would get `EvaluatedToOk` rather
+// than `EvaluatedToOkModuloRegions` and cause winnowing to select
+// the second impl.  The `'static` bound then gets registered as
+// a region obligation that borrowck can't discharge, causing a
+// spurious "may not live long enough" (E0310) error to be reported
+// against the first impl.
+//
+// With the check, the second impl gets `EvaluatedToOkModuloRegions`,
+// so the first impl correctly wins.
+//
+// See #153847.
+#![feature(marker_trait_attr)]
+
+#[marker]
+trait Marker {}
+
+trait Assoc {
+    type Ty;
+}
+impl<T> Assoc for T {
+    type Ty = T;
+}
+
+impl<T: Assoc> Marker for T where <T as Assoc>::Ty: Copy {}
+impl<T: Assoc> Marker for T where <T as Assoc>::Ty: 'static {}
+
+fn main() {}