Don't assemble non-env/bound candidates if projection is rigid

[compiler-errors]

Putting this up for an initial review, it's still missing comments, clean-up, and possibly a tweak to deal with ambiguities in the BestObligation folder.

This PR fixes rust-lang/trait-system-refactor-initiative#173. Specifically, we're creating an unnecessary query cycle in normalization by assembling an impl candidate even if we know later on during merge_candidates that we'll be filtering out that impl candidate.

This PR adjusts the merge_candidates to assemble only env/bound candidates if we have TraitGoalProvenVia::ParamEnv | TraitGoalProvenVia::AliasBound.

I'll leave some thoughts/comments in the code.

r? lcnr

[rustbot]

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[lcnr]

This PR has two separate effects

• we avoid fetching the type of impl associated items if they are shadowed anyways
  • avoid the query cycle in leptos, and generally avoids an unnecessary query dependency
• we avoid reassembling impl candidates if stuff is proven via where-bound, likely positively impacting perf

I feel 🤷 about the first case. I think we should fix #139788 regardless as using RPITITs recursively shouldn't cycle even if they are actually needed. It does match the behavior of the old solver so it feels generally reasonable to do so.

The second one is a bit worrying. I would be opposed/far more hesitant if it were to do the same when assembling trait goals as that definitely has an exponential performance impact (e.g. it would fix rust-lang/trait-system-refactor-initiative#109). I want to be very careful here as I expect us to consider impl candidates in more cases in the future and ideally doing so won't introduce new hangs :<

I expect that in theory we may have exponential performance differences depending on whether or not we check impl item bounds. I don't know if they are likely enough to actually cause issues for us in the future. I am still a bit on the fence about this PR, but generally in favor I think.

[compiler-errors]

I would be opposed/far more hesitant if it were to do the same when assembling trait goals as that definitely has an exponential performance impact (e.g. it would fix rust-lang/trait-system-refactor-initiative#109).

Well part of the point of this approach is we're already doing all the work of proving the trait goal in order to even compute the TraitGoalProvenVia, so my vibe is that all of the additional extra possibly exponential computational work here that we're winnowing out today would be due to GAT bounds.

Totally unrelated side-note that it would be nice if query cycles were recoverable, lol.

[bors]

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

[lcnr]

so my vibe is that all of the additional extra possibly exponential computational work here that we're winnowing out today would be due to GAT bounds.

yeah, exactly, the only hangs we'd be avoiding this way are due to GAT bounds. I guess this seems acceptable to me, so yeah. Let's go ahead with this change

[lcnr]

an alternative structure would be to change assemble_and_evaluate_candidates to also take

enum AssembledCandidates {
   All,
   /// Only assemble candidates from the environment,
   /// ignoring impls. We only look for `ParamEnv` and
   /// `AliasBound` candidates.
   FromEnv,
}

and then put assemble_impl_candidates behind an if/match 🤔

I feel like splitting assembly into multiple functions is making this a bit harder to reason about.

thoughts?