From 4840cd8117010e057109e0233f403e526d309d9f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 9 Jan 2020 10:01:20 -0500 Subject: [PATCH] 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. --- src/librustc/traits/select.rs | 62 +++++++++++++++++-- .../traits/specialize/specialization_graph.rs | 2 +- src/librustc/ty/mod.rs | 13 ++-- .../issue-61651-type-mismatch.rs | 17 +++++ 4 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 src/test/ui/marker_trait_attr/issue-61651-type-mismatch.rs diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 1b1cb1b36e09a..491496e2c62b3 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -1412,6 +1412,8 @@ 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. @@ -1419,7 +1421,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { 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]); @@ -2253,6 +2259,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; @@ -2334,10 +2341,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. diff --git a/src/librustc/traits/specialize/specialization_graph.rs b/src/librustc/traits/specialize/specialization_graph.rs index c176f139bf868..9509b6220eb0e 100644 --- a/src/librustc/traits/specialize/specialization_graph.rs +++ b/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), diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 747e6e8da99af..d27a29d31c0c3 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -2662,7 +2662,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. /// @@ -2833,7 +2838,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)) { @@ -2843,7 +2848,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) => { @@ -2879,7 +2884,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) { diff --git a/src/test/ui/marker_trait_attr/issue-61651-type-mismatch.rs b/src/test/ui/marker_trait_attr/issue-61651-type-mismatch.rs new file mode 100644 index 0000000000000..0af706615e31f --- /dev/null +++ b/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 {} +impl Foo for u8 {} +impl Foo<[u8; 1]> for u8 {} +fn foo, U>(_: T) -> U { unimplemented!() } + +fn main() { + let _: u16 = foo(0_u8); +}