refactor winnowing

lcnr · lcnr · commit 0d45ef838aad · 2024-05-01T18:47:03.000Z
diff --git a/compiler/rustc_middle/src/traits/select.rs b/compiler/rustc_middle/src/traits/select.rs
@@ -121,7 +121,18 @@ pub enum SelectionCandidate<'tcx> {
     /// Implementation of transmutability trait.
     TransmutabilityCandidate,
 
-    ParamCandidate(ty::PolyTraitPredicate<'tcx>),
+    /// A candidate from the `ParamEnv`.
+    ParamCandidate {
+        /// The actual `where`-bound, e.g. `T: Trait`.
+        predicate: ty::PolyTraitPredicate<'tcx>,
+        /// `true` if the where-bound has no bound vars and does
+        /// not refer to any parameters or inference variables.
+        ///
+        /// We prefer all other candidates over global where-bounds.
+        /// Notably, global where-bounds do not shadow impls.
+        is_global: bool,
+    },
+
     ImplCandidate(DefId),
     AutoImplCandidate,
 
diff --git a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs
@@ -251,16 +251,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             all_bounds.filter(|p| p.def_id() == stack.obligation.predicate.def_id());
 
         // Keep only those bounds which may apply, and propagate overflow if it occurs.
-        for bound in matching_bounds {
-            if bound.skip_binder().polarity != stack.obligation.predicate.skip_binder().polarity {
+        for predicate in matching_bounds {
+            if predicate.skip_binder().polarity != stack.obligation.predicate.skip_binder().polarity
+            {
                 continue;
             }
 
             // FIXME(oli-obk): it is suspicious that we are dropping the constness and
             // polarity here.
-            let wc = self.where_clause_may_apply(stack, bound.map_bound(|t| t.trait_ref))?;
+            let wc = self.where_clause_may_apply(stack, predicate.map_bound(|t| t.trait_ref))?;
             if wc.may_apply() {
-                candidates.vec.push(ParamCandidate(bound));
+                let is_global = predicate.is_global() && !predicate.has_bound_vars();
+                candidates.vec.push(ParamCandidate { predicate, is_global });
             }
         }
 
diff --git a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs
@@ -56,9 +56,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 ImplSource::Builtin(BuiltinImplSource::Misc, data)
             }
 
-            ParamCandidate(param) => {
+            ParamCandidate { predicate, is_global: _ } => {
                 let obligations =
-                    self.confirm_param_candidate(obligation, param.map_bound(|t| t.trait_ref));
+                    self.confirm_param_candidate(obligation, predicate.map_bound(|t| t.trait_ref));
                 ImplSource::Param(obligations)
             }
 
diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs
@@ -1575,7 +1575,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             return false;
         }
         match result {
-            Ok(Some(SelectionCandidate::ParamCandidate(trait_ref))) => !trait_ref.has_infer(),
+            Ok(Some(SelectionCandidate::ParamCandidate { predicate, .. })) => {
+                !predicate.has_infer()
+            }
             _ => true,
         }
     }
@@ -1827,31 +1829,35 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
             return DropVictim::Yes;
         }
 
-        // 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();
-
-        // (*) Prefer `BuiltinCandidate { has_nested: false }`, `PointeeCandidate`,
-        // `DiscriminantKindCandidate`, `ConstDestructCandidate`
-        // 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,
-
-            // (*)
+            // Prefer `BuiltinCandidate { has_nested: false }`, `ConstDestructCandidate`
+            // to anything else.
+            //
+            // This is a fix for #53123 and prevents winnowing from accidentally extending the
+            // lifetime of a variable.
+            (
+                BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_),
+                BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_),
+            ) => bug!("two trivial builtin candidates: {other:?} {victim:?}"),
             (BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_), _) => {
                 DropVictim::Yes
             }
             (_, BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_)) => {
                 DropVictim::No
             }
 
-            (ParamCandidate(other), ParamCandidate(victim)) => {
+            // Global bounds from the where clause should be ignored
+            // here (see issue #50825).
+            (ParamCandidate { is_global: true, .. }, ParamCandidate { is_global: true, .. }) => {
+                DropVictim::No
+            }
+            (_, ParamCandidate { is_global: true, .. }) => DropVictim::Yes,
+            (ParamCandidate { is_global: true, .. }, _) => DropVictim::No,
+
+            (
+                ParamCandidate { is_global: false, predicate: other },
+                ParamCandidate { is_global: false, predicate: victim },
+            ) => {
                 let same_except_bound_vars = other.skip_binder().trait_ref
                     == victim.skip_binder().trait_ref
                     && other.skip_binder().polarity == victim.skip_binder().polarity
@@ -1868,63 +1874,8 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                 }
             }
 
-            (
-                ParamCandidate(ref 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(ref victim_cand)) => {
-                // Prefer these to a global where-clause bound
-                // (see issue #50825).
-                if is_global(victim_cand) { DropVictim::Yes } else { DropVictim::No }
-            }
-            (
-                ImplCandidate(_)
-                | AutoImplCandidate
-                | ClosureCandidate { .. }
-                | AsyncClosureCandidate
-                | AsyncFnKindHelperCandidate
-                | CoroutineCandidate
-                | FutureCandidate
-                | IteratorCandidate
-                | AsyncIteratorCandidate
-                | FnPointerCandidate { .. }
-                | BuiltinObjectCandidate
-                | BuiltinUnsizeCandidate
-                | TraitUpcastingUnsizeCandidate(_)
-                | BuiltinCandidate { has_nested: true }
-                | TraitAliasCandidate,
-                ParamCandidate(ref 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(),
-                )
-            }
+            (ParamCandidate { is_global: false, .. }, _) => DropVictim::Yes,
+            (_, ParamCandidate { is_global: false, .. }) => DropVictim::No,
 
             (ProjectionCandidate(i), ProjectionCandidate(j))
             | (ObjectCandidate(i), ObjectCandidate(j)) => {
@@ -1937,44 +1888,18 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                 bug!("Have both object and projection candidate")
             }
 
-            // Arbitrarily give projection and object candidates priority.
-            (
-                ObjectCandidate(_) | ProjectionCandidate(_),
-                ImplCandidate(..)
-                | AutoImplCandidate
-                | ClosureCandidate { .. }
-                | AsyncClosureCandidate
-                | AsyncFnKindHelperCandidate
-                | CoroutineCandidate
-                | FutureCandidate
-                | IteratorCandidate
-                | AsyncIteratorCandidate
-                | FnPointerCandidate { .. }
-                | BuiltinObjectCandidate
-                | BuiltinUnsizeCandidate
-                | TraitUpcastingUnsizeCandidate(_)
-                | BuiltinCandidate { .. }
-                | TraitAliasCandidate,
-            ) => DropVictim::Yes,
+            // Arbitrarily give projection candidates priority.
+            (ProjectionCandidate(_), _) => DropVictim::Yes,
+            (_, ProjectionCandidate(_)) => DropVictim::No,
 
-            (
-                ImplCandidate(..)
-                | AutoImplCandidate
-                | ClosureCandidate { .. }
-                | AsyncClosureCandidate
-                | AsyncFnKindHelperCandidate
-                | CoroutineCandidate
-                | FutureCandidate
-                | IteratorCandidate
-                | AsyncIteratorCandidate
-                | FnPointerCandidate { .. }
-                | BuiltinObjectCandidate
-                | BuiltinUnsizeCandidate
-                | TraitUpcastingUnsizeCandidate(_)
-                | BuiltinCandidate { .. }
-                | TraitAliasCandidate,
-                ObjectCandidate(_) | ProjectionCandidate(_),
-            ) => DropVictim::No,
+            // Need to prioritize builtin trait object impls as
+            // `<dyn Any as Any>::type_id` should use the vtable method
+            // and not the method provided by the user-defined impl
+            // `impl<T: ?Sized> Any for T { .. }`.
+            //
+            // cc #57893
+            (ObjectCandidate(_), _) => DropVictim::Yes,
+            (_, ObjectCandidate(_)) => DropVictim::No,
 
             (&ImplCandidate(other_def), &ImplCandidate(victim_def)) => {
                 // See if we can toss out `victim` based on specialization.
@@ -2054,49 +1979,25 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
                 }
             }
 
-            (AutoImplCandidate, ImplCandidate(_)) | (ImplCandidate(_), AutoImplCandidate) => {
-                DropVictim::No
-            }
-
-            (AutoImplCandidate, _) | (_, AutoImplCandidate) => {
-                bug!(
-                    "default implementations shouldn't be recorded \
-                    when there are other global candidates: {:?} {:?}",
-                    other,
-                    victim
-                );
-            }
-
-            // Everything else is ambiguous
+            // Treat all non-trivial builtin impls and user-defined impls the same way.
             (
                 ImplCandidate(_)
-                | ClosureCandidate { .. }
-                | AsyncClosureCandidate
-                | AsyncFnKindHelperCandidate
-                | CoroutineCandidate
-                | FutureCandidate
-                | IteratorCandidate
-                | AsyncIteratorCandidate
-                | FnPointerCandidate { .. }
-                | BuiltinObjectCandidate
-                | BuiltinUnsizeCandidate
-                | TraitUpcastingUnsizeCandidate(_)
+                | AutoImplCandidate
                 | BuiltinCandidate { has_nested: true }
-                | TraitAliasCandidate,
-                ImplCandidate(_)
-                | ClosureCandidate { .. }
                 | AsyncClosureCandidate
                 | AsyncFnKindHelperCandidate
                 | CoroutineCandidate
                 | FutureCandidate
                 | IteratorCandidate
                 | AsyncIteratorCandidate
                 | FnPointerCandidate { .. }
-                | BuiltinObjectCandidate
+                | ClosureCandidate { .. }
+                | TraitAliasCandidate
                 | BuiltinUnsizeCandidate
                 | TraitUpcastingUnsizeCandidate(_)
-                | BuiltinCandidate { has_nested: true }
-                | TraitAliasCandidate,
+                | TransmutabilityCandidate
+                | BuiltinObjectCandidate,
+                _,
             ) => DropVictim::No,
         }
     }
diff --git a/src/tools/tidy/src/issues.txt b/src/tools/tidy/src/issues.txt
@@ -4062,7 +4062,6 @@ ui/traits/issue-6128.rs
 ui/traits/issue-6334.rs
 ui/traits/issue-65284-suggest-generic-trait-bound.rs
 ui/traits/issue-65673.rs
-ui/traits/issue-66768.rs
 ui/traits/issue-68295.rs
 ui/traits/issue-7013.rs
 ui/traits/issue-70944.rs
diff --git a/tests/ui/associated-item/issue-105449.rs b/tests/ui/associated-item/issue-105449.rs
@@ -1,13 +1,14 @@
-//@ check-pass
 //@ compile-flags: -C debug_assertions=yes -Zunstable-options
 
-#[allow(dead_code)]
+// This is a mutated variant of #66768 which has been removed
+// as it no longer tests the original issue.
 fn problematic_function<Space>()
 where
     DefaultAlloc: FinAllok<R1, Space>,
 {
     let e = Edge2dElement;
     let _ = Into::<Point>::into(e.map_reference_coords());
+    //~^ ERROR the trait bound `Point: From<(Ure, R1, MStorage)>` is not satisfied
 }
 impl<N> Allocator<N, R0> for DefaultAlloc {
     type Buffer = MStorage;
diff --git a/tests/ui/associated-item/issue-105449.stderr b/tests/ui/associated-item/issue-105449.stderr
@@ -0,0 +1,15 @@
+error[E0277]: the trait bound `Point: From<(Ure, R1, MStorage)>` is not satisfied
+  --> $DIR/issue-105449.rs:10:33
+   |
+LL |     let _ = Into::<Point>::into(e.map_reference_coords());
+   |             ------------------- ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<(Ure, R1, MStorage)>` is not implemented for `Point`, which is required by `(Ure, R1, MStorage): Into<Point>`
+   |             |
+   |             required by a bound introduced by this call
+   |
+   = help: the trait `From<(Ure, Space, <DefaultAlloc as Allocator<Ure, Space>>::Buffer)>` is implemented for `Point`
+   = help: for that trait implementation, expected `Space`, found `R1`
+   = note: required for `(Ure, R1, MStorage)` to implement `Into<Point>`
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0277`.
diff --git a/tests/ui/lifetimes/issue-34979.rs b/tests/ui/lifetimes/issue-34979.rs
@@ -3,7 +3,7 @@ impl<'a, T> Foo for &'a T {}
 
 struct Ctx<'a>(&'a ())
 where
-    &'a (): Foo, //~ ERROR: type annotations needed
-    &'static (): Foo;
+    &'a (): Foo,
+    &'static (): Foo; //~ ERROR: mismatched types
 
 fn main() {}
diff --git a/tests/ui/lifetimes/issue-34979.stderr b/tests/ui/lifetimes/issue-34979.stderr
@@ -1,20 +1,18 @@
-error[E0283]: type annotations needed: cannot satisfy `&'a (): Foo`
-  --> $DIR/issue-34979.rs:6:13
+error[E0308]: mismatched types
+  --> $DIR/issue-34979.rs:7:18
    |
-LL |     &'a (): Foo,
-   |             ^^^
+LL |     &'static (): Foo;
+   |                  ^^^ lifetime mismatch
    |
-note: multiple `impl`s or `where` clauses satisfying `&'a (): Foo` found
-  --> $DIR/issue-34979.rs:2:1
+   = note: expected trait `<&'static () as Foo>`
+              found trait `<&'a () as Foo>`
+note: the lifetime `'a` as defined here...
+  --> $DIR/issue-34979.rs:4:12
    |
-LL | impl<'a, T> Foo for &'a T {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^
-...
-LL |     &'a (): Foo,
-   |             ^^^
-LL |     &'static (): Foo;
-   |                  ^^^
+LL | struct Ctx<'a>(&'a ())
+   |            ^^
+   = note: ...does not necessarily outlive the static lifetime
 
 error: aborting due to 1 previous error
 
-For more information about this error, try `rustc --explain E0283`.
+For more information about this error, try `rustc --explain E0308`.
diff --git a/tests/ui/traits/issue-66768.rs b/tests/ui/traits/issue-66768.rs
diff --git a/tests/ui/traits/normalize-conflicting-impls.rs b/tests/ui/traits/normalize-conflicting-impls.rs
diff --git a/tests/ui/traits/normalize-conflicting-impls.stderr b/tests/ui/traits/normalize-conflicting-impls.stderr

Original file line number	Diff line number	Diff line change
`@@ -56,9 +56,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {`
`56`	`56`	`ImplSource::Builtin(BuiltinImplSource::Misc, data)`
`57`	`57`	`}`
`58`	`58`
`59`		`- ParamCandidate(param) => {`
	`59`	`+ ParamCandidate { predicate, is_global: _ } => {`
`60`	`60`	`let obligations =`
`61`		`- self.confirm_param_candidate(obligation, param.map_bound(\|t\| t.trait_ref));`
	`61`	`+ self.confirm_param_candidate(obligation, predicate.map_bound(\|t\| t.trait_ref));`
`62`	`62`	`ImplSource::Param(obligations)`
`63`	`63`	`}`
`64`	`64`