Fix trait solving ICEs

[matthewjasper]

• Selection candidates that are known to be applicable are preferred
  over candidates that are not.
• Don't ICE if a projection/object candidate is no longer applicable
  (this can happen due to cycles in normalization)
• Normalize supertraits when finding trait object candidates

Closes #77653
Closes #77656

r? @nikomatsakis

[tavianator]

I can confirm that this fixes #77656, and https://github.com/tavianator/acap builds successfully now. Thanks!

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[spastorino]

@bors p=1

When this is merged it needs high priority because it fixes 2 P-critical issues.

[lqd]

If this needs "high priority", over rollups, it needs at least p=6: priorities 1 to 5 can be skipped by rollups

[spastorino]

If this needs "high priority", over rollups, it needs at least p=6: priorities 1 to 5 can be skipped by rollups

I'm not sure if we have a policy but I'd make this kind of PR higher priority than rollups, yeah.

[lqd]

I wasn't aware of that policy either before Manish mentioned it to me.

then let's do @bors p=6 for when the PR is reviewed

[nikomatsakis]

Hmm I was worried when I read the PR description, but this is relatively narrowly tailored

[nikomatsakis]

So, r=me, but I'd be happy to talk about this more with @matthewjasper -- they seemed to indicate a desire to talk it over. I'm like a little nervous about the changes to prefer one predicate over another but these do seem pretty narrowly tailored, and they avoid inference effects, so I'm not that worried.

[matthewjasper]

After trying to find other options here there doesn't seem to be a much better solution without a much larger change here (e.g. making rustc more like chalk). I think that I slightly prefer this change, which simplifies how winnowing works in exchange for evaluating normalization cycles to Recur instead of Unknown. The patch here is against the head of this PR.

@@ -521,12 +521,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                             result
                         }
                         Ok(Ok(None)) => Ok(EvaluatedToAmbig),
-                        // EvaluatedToRecur might also be acceptable here, but use
-                        // Unknown for now because it means that we won't dismiss a
-                        // selection candidate solely because it has a projection
-                        // cycle. This is closest to the previous behavior of
-                        // immediately erroring.
-                        Ok(Err(project::InProgress)) => Ok(EvaluatedToUnknown),
+                        Ok(Err(project::InProgress)) => Ok(EvaluatedToRecur),
                         Err(_) => Ok(EvaluatedToErr),
                     }
                 }
@@ -1387,9 +1382,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             | (ObjectCandidate(i), ObjectCandidate(j)) => {
                 // Arbitrarily pick the lower numbered candidate for backwards
                 // compatibility reasons. Don't let this affect inference.
-                other.evaluation.must_apply_modulo_regions()
-                    && !needs_infer
-                    && (i < j || !victim.evaluation.must_apply_modulo_regions())
+                i < j && !needs_infer
             }
             (ObjectCandidate(_), ProjectionCandidate(_))
             | (ProjectionCandidate(_), ObjectCandidate(_)) => {

[nikomatsakis]

@matthewjasper hmm I think I might prefer that patch too. EvaluateToRecur does seem appropriate for a cycle involving normalization.

[matthewjasper]

@bors r=nikomatsakis

[bors]

📌 Commit 56015a58635d8275f52998c07276adf0aed18cbc has been approved by nikomatsakis

[jonas-schievink]

@bors retry

[jonas-schievink]

@bors retry r-

[jonas-schievink]

@bors r=nikomatsakis

[bors]

📌 Commit 56015a58635d8275f52998c07276adf0aed18cbc has been approved by nikomatsakis

[JohnTitor]

@bors r-
Let me tell bors that this should be tested.

[matthewjasper]

seeing if a force-push helps
@bors r=nikomatsakis

[bors]

📌 Commit 64520d04b9eaa4899c1686ff02f9e51d0b8db5c4 has been approved by nikomatsakis

[bors]

⌛ Testing commit 64520d04b9eaa4899c1686ff02f9e51d0b8db5c4 with merge 8c26ccaa0b4c073a3d4a129650a486587ce7aac4...

[bors]

💔 Test failed - checks-actions

[matthewjasper]

@bors r=nikomatsakis

[bors]

📌 Commit 50dde2e has been approved by nikomatsakis

[bors]

⌛ Testing commit 50dde2e with merge 1dc1166c4fd24c3625904339077abd92217a0c5c...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 1dc1166c4fd24c3625904339077abd92217a0c5c to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

[matthewjasper]

Hmm, merge was against 8f0fa9d but master is at 6b9fbf2 cc @rust-lang/infra

[kennytm]

@bors retry

[bors]

⌛ Testing commit 50dde2e with merge a9cd294...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing a9cd294 to master...