[amirHdev]

Discarded impl candidates from non-global ParamEnv preferences can contribute candidate head usages causing unnecessary canonical cycle reruns

those head usages can be ignored when the candidates are discarded

cc https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/.60hypospray-0.2E1.2E2.60.20minimization/with/617255330

[lcnr]

if you do this after the extract_if you don't need to check the candidate source?

View changes since the review

[lcnr]

I think this doesn't fix, as now all the impl candidates don't apply due to Graph<M, STATE>: NotImplemented

use std::marker::PhantomData;
trait Dependencies<D> {}
struct Graph<M, const STATE: u8>(PhantomData<M>);
fn impls<T: Dependencies<D>, D>() {}

trait NotImplemented {}

macro_rules! example {
    ($(($($dep:ident => $next:literal),+)),+ $(,)?) => {
        fn hang<M>()
        where
            Graph<M, 0>: Dependencies<()>,
            $($(Graph<M, $next>: Dependencies<()>,)+)+
        {
            impls::<Graph<M, 0>, _>();
        }

        $(
            impl<M, const STATE: u8, $($dep,)+> Dependencies<($($dep,)+)>
                for Graph<M, STATE>
            where
                $(Graph<M, $next>: Dependencies<$dep>,)+
                Graph<M, STATE>: NotImplemented,
            {}
        )+
    };
}

example! {
    (D1 => 21, D2 => 22),
    (D1 => 31, D2 => 32, D3 => 33),
    (D1 => 41, D2 => 42, D3 => 43, D4 => 44),
    (D1 => 51, D2 => 52, D3 => 53, D4 => 54, D5 => 55),
}

fn main() {}

see https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/.60hypospray-0.2E1.2E2.60.20minimization/near/618185915 on how to also ignore usages from impls which don't apply

[amirHdev]

@lcnr thanks for catching that
I've updated it to track head usages from failed impls too

[rustbot]

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 19 candidates

[JonathanBrouwer]

r? types

[lcnr]

I feel unsure here... I do somewhat dislike the current candidate assembly setup if we're tracking failed candidates anyways and need to explicitly track which candidates don't matter.

My current vague vibe is that it may actually be better to have a Candidates struct where we collect/discard candidates eagerly instead of collecting all of them and doing stuff at the end 🤔

in a way that'd be more similar to the existing behavior of

rust/compiler/rustc_trait_selection/src/traits/project.rs

Lines 66 to 139 in ce37c4f

	enum ProjectionCandidateSet<'tcx> {
	None,
	Single(ProjectionCandidate<'tcx>),
	Ambiguous,
	Error(SelectionError<'tcx>),
	}
	impl<'tcx> ProjectionCandidateSet<'tcx> {
	fn mark_ambiguous(&mut self) {
	*self = ProjectionCandidateSet::Ambiguous;
	}
	fn mark_error(&mut self, err: SelectionError<'tcx>) {
	*self = ProjectionCandidateSet::Error(err);
	}
	// Returns true if the push was successful, or false if the candidate
	// was discarded -- this could be because of ambiguity, or because
	// a higher-priority candidate is already there.
	fn push_candidate(&mut self, candidate: ProjectionCandidate<'tcx>) -> bool {
	// This wacky variable is just used to try and
	// make code readable and avoid confusing paths.
	// It is assigned a "value" of `()` only on those
	// paths in which we wish to convert `*self` to
	// ambiguous (and return false, because the candidate
	// was not used). On other paths, it is not assigned,
	// and hence if those paths *could* reach the code that
	// comes after the match, this fn would not compile.
	let convert_to_ambiguous;
	match self {
	ProjectionCandidateSet::None => {
	*self = ProjectionCandidateSet::Single(candidate);
	return true;
	}
	ProjectionCandidateSet::Single(current) => {
	// Duplicates can happen inside ParamEnv. In the case, we
	// perform a lazy deduplication.
	if current == &candidate {
	return false;
	}
	// Prefer where-clauses. As in select, if there are multiple
	// candidates, we prefer where-clause candidates over impls. This
	// may seem a bit surprising, since impls are the source of
	// "truth" in some sense, but in fact some of the impls that SEEM
	// applicable are not, because of nested obligations. Where
	// clauses are the safer choice. See the comment on
	// `select::SelectionCandidate` and #21974 for more details.
	match (current, candidate) {
	(ProjectionCandidate::ParamEnv(..), ProjectionCandidate::ParamEnv(..)) => {
	convert_to_ambiguous = ()
	}
	(ProjectionCandidate::ParamEnv(..), _) => return false,
	(_, ProjectionCandidate::ParamEnv(..)) => bug!(
	"should never prefer non-param-env candidates over param-env candidates"
	),
	(_, _) => convert_to_ambiguous = (),
	}
	}
	ProjectionCandidateSet::Ambiguous | ProjectionCandidateSet::Error(..) => {
	return false;
	}
	}
	// We only ever get here when we moved from a single candidate
	// to ambiguous.
	let () = convert_to_ambiguous;
	*self = ProjectionCandidateSet::Ambiguous;
	false
	}
	}

🤔

View changes since the review

[rust-bors]

☔ The latest upstream changes (presumably #165244) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.