Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 57 additions & 5 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1412,14 +1412,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]);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
13 changes: 9 additions & 4 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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)) {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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) {
Expand Down
17 changes: 17 additions & 0 deletions src/test/ui/marker_trait_attr/issue-61651-type-mismatch.rs
Original file line number Diff line number Diff line change
@@ -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);
}