From a994a99ec406e48c17660045ccce2634e82fe5f6 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 16 Feb 2015 06:57:38 -0500 Subject: [PATCH] Always prefer where-clauses over impls in trait selection. Fixes #22110. --- src/librustc/middle/traits/select.rs | 71 +++++-------------------- src/test/run-pass/traits-issue-22110.rs | 34 ++++++++++++ 2 files changed, 48 insertions(+), 57 deletions(-) create mode 100644 src/test/run-pass/traits-issue-22110.rs diff --git a/src/librustc/middle/traits/select.rs b/src/librustc/middle/traits/select.rs index 0df59c917edea..027415de998df 100644 --- a/src/librustc/middle/traits/select.rs +++ b/src/librustc/middle/traits/select.rs @@ -652,8 +652,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { let is_dup = (0..candidates.len()) .filter(|&j| i != j) - .any(|j| self.candidate_should_be_dropped_in_favor_of(stack, - &candidates[i], + .any(|j| self.candidate_should_be_dropped_in_favor_of(&candidates[i], &candidates[j])); if is_dup { debug!("Dropping candidate #{}/{}: {}", @@ -1235,31 +1234,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { self.evaluate_predicates_recursively(stack, selection.iter_nested()) } - /// Returns true if `candidate_i` should be dropped in favor of `candidate_j`. - /// - /// This is generally true if either: - /// - candidate i and candidate j are equivalent; or, - /// - candidate i is a concrete impl and candidate j is a where clause bound, - /// and the concrete impl is applicable to the types in the where clause bound. - /// - /// The last case refers to cases where there are blanket impls (often conditional - /// blanket impls) as well as a where clause. This can come down to one of two cases: - /// - /// - The impl is truly unconditional (it has no where clauses - /// of its own), in which case the where clause is - /// unnecessary, because coherence requires that we would - /// pick that particular impl anyhow (at least so long as we - /// don't have specialization). - /// - /// - The impl is conditional, in which case we may not have winnowed it out - /// because we don't know if the conditions apply, but the where clause is basically - /// telling us that there is some impl, though not necessarily the one we see. - /// - /// In both cases we prefer to take the where clause, which is - /// essentially harmless. See issue #18453 for more details of - /// a case where doing the opposite caused us harm. + /// Returns true if `candidate_i` should be dropped in favor of + /// `candidate_j`. Generally speaking we will drop duplicate + /// candidates and prefer where-clause candidates. fn candidate_should_be_dropped_in_favor_of<'o>(&mut self, - stack: &TraitObligationStack<'o, 'tcx>, candidate_i: &SelectionCandidate<'tcx>, candidate_j: &SelectionCandidate<'tcx>) -> bool @@ -1269,37 +1247,16 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } match (candidate_i, candidate_j) { - (&ImplCandidate(impl_def_id), &ParamCandidate(ref bound)) => { - debug!("Considering whether to drop param {} in favor of impl {}", - candidate_i.repr(self.tcx()), - candidate_j.repr(self.tcx())); - - self.infcx.probe(|snapshot| { - let (skol_obligation_trait_ref, skol_map) = - self.infcx().skolemize_late_bound_regions( - &stack.obligation.predicate, snapshot); - let impl_substs = - self.rematch_impl(impl_def_id, stack.obligation, snapshot, - &skol_map, skol_obligation_trait_ref.trait_ref.clone()); - let impl_trait_ref = - ty::impl_trait_ref(self.tcx(), impl_def_id).unwrap(); - let impl_trait_ref = - impl_trait_ref.subst(self.tcx(), &impl_substs.value); - let poly_impl_trait_ref = - ty::Binder(impl_trait_ref); - let origin = - infer::RelateOutputImplTypes(stack.obligation.cause.span); - self.infcx - .sub_poly_trait_refs(false, origin, poly_impl_trait_ref, bound.clone()) - .is_ok() - }) - } - (&BuiltinCandidate(_), &ParamCandidate(_)) => { - // If we have a where-clause like `Option : Send`, - // then we wind up in a situation where there is a - // default rule (`Option:Send if K:Send) and the - // where-clause that both seem applicable. Just take - // the where-clause in that case. + (&ImplCandidate(..), &ParamCandidate(..)) | + (&ClosureCandidate(..), &ParamCandidate(..)) | + (&FnPointerCandidate(..), &ParamCandidate(..)) | + (&BuiltinCandidate(..), &ParamCandidate(..)) => { + // We basically prefer always prefer to use a + // where-clause over another option. Where clauses + // impose the burden of finding the exact match onto + // the caller. Using an impl in preference of a where + // clause can also lead us to "overspecialize", as in + // #18453. true } (&ProjectionCandidate, &ParamCandidate(_)) => { diff --git a/src/test/run-pass/traits-issue-22110.rs b/src/test/run-pass/traits-issue-22110.rs new file mode 100644 index 0000000000000..9cdcf4945d8ba --- /dev/null +++ b/src/test/run-pass/traits-issue-22110.rs @@ -0,0 +1,34 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test an issue where we reported ambiguity between the where-clause +// and the blanket impl. The only important thing is that compilation +// succeeds here. Issue #22110. + +#![allow(dead_code)] + +trait Foo { + fn foo(&self, a: A); +} + +impl Foo for F { + fn foo(&self, _: A) { } +} + +fn baz Foo<(&'a A,)>>(_: F) { } + +fn components(t: fn(&A)) + where fn(&A) : for<'a> Foo<(&'a A,)>, +{ + baz(t) +} + +fn main() { +}