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.

Sign up for GitHub

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

Jump to bottom

improve readability of winnowing #108937

Merged
merged 2 commits into from
Mar 11, 2023
Merged

improve readability of winnowing #108937

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3097453
improve readability of winnowing
lcnr Mar 9, 2023
14818e2
updating comment
lcnr Mar 10, 2023
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
107 changes: 67 additions & 40 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,14 +465,14 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
if candidates.len() > 1 {
let mut i = 0;
while i < candidates.len() {
let is_dup = (0..candidates.len()).filter(|&j| i != j).any(|j| {
let should_drop_i = (0..candidates.len()).filter(|&j| i != j).any(|j| {
self.candidate_should_be_dropped_in_favor_of(
&candidates[i],
&candidates[j],
needs_infer,
)
) == DropVictim::Yes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe .is_drop_victim() (or other check method) would be nicer here, idk.

});
if is_dup {
if should_drop_i {
debug!(candidate = ?candidates[i], "Dropping candidate #{}/{}", i, candidates.len());
candidates.swap_remove(i);
} else {
Expand Down Expand Up @@ -1842,16 +1842,22 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
ProjectionMatchesProjection::No
}
}
}

///////////////////////////////////////////////////////////////////////////
// WINNOW
//
// Winnowing is the process of attempting to resolve ambiguity by
// probing further. During the winnowing process, we unify all
// type variables and then we also attempt to evaluate recursive
// bounds to see if they are satisfied.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum DropVictim {
Yes,
No,
}

/// Returns `true` if `victim` should be dropped in favor of
/// ## Winnowing
///
/// Winnowing is the process of attempting to resolve ambiguity by
/// probing further. During the winnowing process, we unify all
/// type variables and then we also attempt to evaluate recursive
/// bounds to see if they are satisfied.
impl<'tcx> SelectionContext<'_, 'tcx> {
/// Returns `DropVictim::Yes` if `victim` should be dropped in favor of
/// `other`. Generally speaking we will drop duplicate
/// candidates and prefer where-clause candidates.
///
Expand All @@ -1861,9 +1867,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
victim: &EvaluatedCandidate<'tcx>,
other: &EvaluatedCandidate<'tcx>,
needs_infer: bool,
) -> bool {
) -> DropVictim {
if victim.candidate == other.candidate {
return true;
return DropVictim::Yes;
}

// Check if a bound would previously have been removed when normalizing
Expand All @@ -1887,11 +1893,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

// FIXME(@jswrenn): this should probably be more sophisticated
(TransmutabilityCandidate, _) | (_, TransmutabilityCandidate) => false,
(TransmutabilityCandidate, _) | (_, TransmutabilityCandidate) => DropVictim::No,

// (*)
(BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_), _) => true,
(_, BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_)) => false,
(BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_), _) => {
DropVictim::Yes
}
(_, BuiltinCandidate { has_nested: false } | ConstDestructCandidate(_)) => {
DropVictim::No
}

(ParamCandidate(other), ParamCandidate(victim)) => {
let same_except_bound_vars = other.skip_binder().trait_ref
Expand All @@ -1905,28 +1915,27 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// or the current one if tied (they should both evaluate to the same answer). This is
// probably best characterized as a "hack", since we might prefer to just do our
// best to *not* create essentially duplicate candidates in the first place.
other.bound_vars().len() <= victim.bound_vars().len()
if other.bound_vars().len() <= victim.bound_vars().len() {
DropVictim::Yes
} else {
DropVictim::No
}
} else if other.skip_binder().trait_ref == victim.skip_binder().trait_ref
&& victim.skip_binder().constness == ty::BoundConstness::NotConst
&& other.skip_binder().polarity == victim.skip_binder().polarity
{
// Drop otherwise equivalent non-const candidates in favor of const candidates.
true
DropVictim::Yes
} else {
false
DropVictim::No
}
}

// Drop otherwise equivalent non-const fn pointer candidates
(FnPointerCandidate { .. }, FnPointerCandidate { is_const: false }) => true,
(FnPointerCandidate { .. }, FnPointerCandidate { is_const: false }) => DropVictim::Yes,

// Global bounds from the where clause should be ignored
// here (see issue #50825). Otherwise, we have a where
// clause so don't go around looking for impls.
// Arbitrarily give param candidates priority
// over projection and object candidates.
(
ParamCandidate(ref cand),
ParamCandidate(ref other_cand),
ImplCandidate(..)
| ClosureCandidate { .. }
| GeneratorCandidate
Expand All @@ -1939,11 +1948,23 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| TraitAliasCandidate
| ObjectCandidate(_)
| ProjectionCandidate(..),
) => !is_global(cand),
(ObjectCandidate(_) | ProjectionCandidate(..), ParamCandidate(ref cand)) => {
) => {
if is_global(other_cand) {
DropVictim::No
} else {
// 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::Yes
}
}
(ObjectCandidate(_) | ProjectionCandidate(..), ParamCandidate(ref victim_cand)) => {
// Prefer these to a global where-clause bound
// (see issue #50825).
is_global(cand)
if is_global(victim_cand) { DropVictim::Yes } else { DropVictim::No }
}
(
ImplCandidate(_)
Expand All @@ -1956,18 +1977,22 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| TraitUpcastingUnsizeCandidate(_)
| BuiltinCandidate { has_nested: true }
| TraitAliasCandidate,
ParamCandidate(ref cand),
ParamCandidate(ref victim_cand),
) => {
// Prefer these to a global where-clause bound
// (see issue #50825).
is_global(cand) && other.evaluation.must_apply_modulo_regions()
if is_global(victim_cand) && other.evaluation.must_apply_modulo_regions() {
DropVictim::Yes
} else {
DropVictim::No
}
}

(ProjectionCandidate(i, _), ProjectionCandidate(j, _))
| (ObjectCandidate(i), ObjectCandidate(j)) => {
// Arbitrarily pick the lower numbered candidate for backwards
// compatibility reasons. Don't let this affect inference.
i < j && !needs_infer
if i < j && !needs_infer { DropVictim::Yes } else { DropVictim::No }
}
(ObjectCandidate(_), ProjectionCandidate(..))
| (ProjectionCandidate(..), ObjectCandidate(_)) => {
Expand All @@ -1987,7 +2012,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| TraitUpcastingUnsizeCandidate(_)
| BuiltinCandidate { .. }
| TraitAliasCandidate,
) => true,
) => DropVictim::Yes,

(
ImplCandidate(..)
Expand All @@ -2001,7 +2026,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| BuiltinCandidate { .. }
| TraitAliasCandidate,
ObjectCandidate(_) | ProjectionCandidate(..),
) => false,
) => DropVictim::No,

(&ImplCandidate(other_def), &ImplCandidate(victim_def)) => {
// See if we can toss out `victim` based on specialization.
Expand All @@ -2014,7 +2039,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let tcx = self.tcx();
if other.evaluation.must_apply_modulo_regions() {
if tcx.specializes((other_def, victim_def)) {
return true;
return DropVictim::Yes;
}
}

Expand Down Expand Up @@ -2060,13 +2085,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// 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
if needs_infer { DropVictim::No } else { DropVictim::Yes }
}
Some(_) => true,
None => false,
Some(_) => DropVictim::Yes,
None => DropVictim::No,
}
} else {
false
DropVictim::No
}
}

Expand All @@ -2092,10 +2117,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| TraitUpcastingUnsizeCandidate(_)
| BuiltinCandidate { has_nested: true }
| TraitAliasCandidate,
) => false,
) => DropVictim::No,
}
}
}

impl<'tcx> SelectionContext<'_, 'tcx> {
fn sized_conditions(
&mut self,
obligation: &TraitObligation<'tcx>,
Expand Down