Auto merge of #68057 - Aaron1011:fix/marker-trait-selection, r=matthewjasper

Don't discard marker trait impls when inference variables are present

Fixes #61651

Previously, we would unconditionally discard impl candidates for marker
traits during trait selection. However, if the predicate had inference
variables, this could have the effect of constrainting inference
variables (due to a successful trait selection) when we would have
otherwise failed due to mutliple applicable impls,

This commit prevents marker trait impls from being discarded while the
obligation predicate has any inference variables, ensuring that
discarding impls will never cause us to incorrectly constraint inference
variables.

Author: bors

src/librustc/traits/select.rs

@@ -1417,14 +1417,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
 
         debug!("winnowed to {} candidates for {:?}: {:?}", candidates.len(), stack, candidates);
 
+        let needs_infer = stack.obligation.predicate.needs_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 is_dup = (0..candidates.len()).filter(|&j| i != j).any(|j| {
-                    self.candidate_should_be_dropped_in_favor_of(&candidates[i], &candidates[j])
+                    self.candidate_should_be_dropped_in_favor_of(
+                        &candidates[i],
+                        &candidates[j],
+                        needs_infer,
+                    )
                 });
                 if is_dup {
                     debug!("Dropping candidate #{}/{}: {:?}", i, candidates.len(), candidates[i]);
@@ -2258,6 +2264,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
         &mut self,
         victim: &EvaluatedCandidate<'tcx>,
         other: &EvaluatedCandidate<'tcx>,
+        needs_infer: bool,
     ) -> bool {
         if victim.candidate == other.candidate {
             return true;
@@ -2339,10 +2346,55 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                     match victim.candidate {
                         ImplCandidate(victim_def) => {
                             let tcx = self.tcx();
-                            return tcx.specializes((other_def, victim_def))
-                                || tcx
-                                    .impls_are_allowed_to_overlap(other_def, victim_def)
-                                    .is_some();
+                            if tcx.specializes((other_def, victim_def)) {
+                                return true;
+                            }
+                            return match tcx.impls_are_allowed_to_overlap(other_def, victim_def) {
+                                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
+                                    // constrainting 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 constrin)
+                                    // 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.
+                                    !needs_infer
+                                }
+                                Some(_) => true,
+                                None => false,
+                            };
                         }
                         ParamCandidate(ref cand) => {
                             // Prefer the impl to a global where clause candidate.

src/librustc/traits/specialize/specialization_graph.rs

@@ -163,7 +163,7 @@ impl<'tcx> Children {
                         tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling)
                     {
                         match overlap_kind {
-                            ty::ImplOverlapKind::Permitted => {}
+                            ty::ImplOverlapKind::Permitted { marker: _ } => {}
                             ty::ImplOverlapKind::Issue33140 => {
                                 last_lint = Some(FutureCompatOverlapError {
                                     error: overlap_error(overlap),

src/librustc/ty/mod.rs

@@ -2591,7 +2591,12 @@ impl<'tcx> ::std::ops::Deref for Attributes<'tcx> {
 #[derive(Debug, PartialEq, Eq)]
 pub enum ImplOverlapKind {
     /// These impls are always allowed to overlap.
-    Permitted,
+    Permitted {
+        /// Whether or not the impl is permitted due to the trait being
+        /// a marker trait (a trait with #[marker], or a trait with
+        /// no associated items and #![feature(overlapping_marker_traits)] enabled)
+        marker: bool,
+    },
     /// These impls are allowed to overlap, but that raises
     /// an issue #33140 future-compatibility warning.
     ///
@@ -2711,7 +2716,7 @@ impl<'tcx> TyCtxt<'tcx> {
         if self.impl_trait_ref(def_id1).map_or(false, |tr| tr.references_error())
             || self.impl_trait_ref(def_id2).map_or(false, |tr| tr.references_error())
         {
-            return Some(ImplOverlapKind::Permitted);
+            return Some(ImplOverlapKind::Permitted { marker: false });
         }
 
         match (self.impl_polarity(def_id1), self.impl_polarity(def_id2)) {
@@ -2721,7 +2726,7 @@ impl<'tcx> TyCtxt<'tcx> {
                     "impls_are_allowed_to_overlap({:?}, {:?}) = Some(Permitted) (reservations)",
                     def_id1, def_id2
                 );
-                return Some(ImplOverlapKind::Permitted);
+                return Some(ImplOverlapKind::Permitted { marker: false });
             }
             (ImplPolarity::Positive, ImplPolarity::Negative)
             | (ImplPolarity::Negative, ImplPolarity::Positive) => {
@@ -2757,7 +2762,7 @@ impl<'tcx> TyCtxt<'tcx> {
                 "impls_are_allowed_to_overlap({:?}, {:?}) = Some(Permitted) (marker overlap)",
                 def_id1, def_id2
             );
-            Some(ImplOverlapKind::Permitted)
+            Some(ImplOverlapKind::Permitted { marker: true })
         } else {
             if let Some(self_ty1) = self.issue33140_self_ty(def_id1) {
                 if let Some(self_ty2) = self.issue33140_self_ty(def_id2) {

src/test/ui/marker_trait_attr/issue-61651-type-mismatch.rs

@@ -0,0 +1,17 @@
+// check-pass
+// Regression test for issue #61651
+// Verifies that we don't try to constrain inference
+// variables due to the presence of multiple applicable
+// marker trait impls
+
+#![feature(marker_trait_attr)]
+
+#[marker] // Remove this line and it works?!?
+trait Foo<T> {}
+impl Foo<u16> for u8 {}
+impl Foo<[u8; 1]> for u8 {}
+fn foo<T: Foo<U>, U>(_: T) -> U { unimplemented!() }
+
+fn main() {
+    let _: u16 = foo(0_u8);
+}