Skip to content

Don't consider candidates with no failing where clauses when refining obligation causes in new solver #124771

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.

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

Merged
merged 1 commit into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 42 additions & 6 deletions compiler/rustc_trait_selection/src/solve/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_middle::ty::{self, TyCtxt};
use rustc_span::symbol::sym;

use super::eval_ctxt::GenerateProofTree;
use super::inspect::{ProofTreeInferCtxtExt, ProofTreeVisitor};
use super::inspect::{InspectCandidate, InspectGoal, ProofTreeInferCtxtExt, ProofTreeVisitor};
use super::{Certainty, InferCtxtEvalExt};

/// A trait engine using the new trait solver.
Expand Down Expand Up @@ -304,6 +304,46 @@ impl<'tcx> BestObligation<'tcx> {
self.obligation = old_obligation;
res
}

/// Filter out the candidates that aren't either error or ambiguous (depending
/// on what we are looking for), and also throw out candidates that have no
/// failing WC (or higher-ranked obligations, for which there should only be
/// one candidate anyways -- but I digress). This most likely means that the
/// goal just didn't unify at all, e.g. a param candidate with an alias in it.
fn non_trivial_candidates<'a>(
&self,
goal: &'a InspectGoal<'a, 'tcx>,
) -> Vec<InspectCandidate<'a, 'tcx>> {
let mut candidates = goal
.candidates()
.into_iter()
.filter(|candidate| match self.consider_ambiguities {
true => matches!(candidate.result(), Ok(Certainty::Maybe(_))),
false => matches!(candidate.result(), Err(_)),
})
.collect::<Vec<_>>();

// If we have >1 candidate, one may still be due to "boring" reasons, like
// an alias-relate that failed to hold when deeply evaluated. We really
// don't care about reasons like this.
if candidates.len() > 1 {
candidates.retain(|candidate| {
goal.infcx().probe(|_| {
Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of a shame we have to do this in a probe, but if we don't, then the instantiated nested goals from different candidates will affect eachother, causing weirdness and ICEs.

Luckily, we do this only when there's >1 candidate.

We could, perhaps, first filter out candidates with no GoalSource::ImplWhereBound | GoalSource::InstantiateHigherRanked without even instantiating them, and only instantiate them if necessary.

Also, I guess we should have both the source and the certainty in the proof tree without instantiating -- we just need to expose it, perhaps?

candidate.instantiate_nested_goals(self.span()).iter().any(|nested_goal| {
matches!(
nested_goal.source(),
GoalSource::ImplWhereBound | GoalSource::InstantiateHigherRanked
) && match self.consider_ambiguities {
true => matches!(nested_goal.result(), Ok(Certainty::Maybe(_))),
false => matches!(nested_goal.result(), Err(_)),
}
})
})
});
}

candidates
}
}

impl<'tcx> ProofTreeVisitor<'tcx> for BestObligation<'tcx> {
Expand All @@ -314,11 +354,7 @@ impl<'tcx> ProofTreeVisitor<'tcx> for BestObligation<'tcx> {
}

fn visit_goal(&mut self, goal: &super::inspect::InspectGoal<'_, 'tcx>) -> Self::Result {
// FIXME: Throw out candidates that have no failing WC and >0 failing misc goal.
// This most likely means that the goal just didn't unify at all, e.g. a param
// candidate with an alias in it.
let candidates = goal.candidates();

let candidates = self.non_trivial_candidates(goal);
let [candidate] = candidates.as_slice() else {
return ControlFlow::Break(self.obligation.clone());
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
trait Foo {}
trait Bar {}

impl<T> Foo for T where T: Bar {}
fn needs_foo(_: impl Foo) {}

trait Mirror {
type Mirror;
}
impl<T> Mirror for T {
type Mirror = T;
}

// Make sure the `Alias: Foo` bound doesn't "shadow" the impl, since the
// impl is really the only candidate we care about here for the purpose
// of error reporting.
fn hello<T>() where <T as Mirror>::Mirror: Foo {
needs_foo(());
//~^ ERROR the trait bound `(): Foo` is not satisfied
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0277]: the trait bound `(): Foo` is not satisfied
--> $DIR/where-clause-doesnt-apply.rs:18:15
|
LL | needs_foo(());
| --------- ^^ the trait `Bar` is not implemented for `()`, which is required by `(): Foo`
| |
| required by a bound introduced by this call
|
note: required for `()` to implement `Foo`
--> $DIR/where-clause-doesnt-apply.rs:4:9
|
LL | impl<T> Foo for T where T: Bar {}
| ^^^ ^ --- unsatisfied trait bound introduced here
note: required by a bound in `needs_foo`
--> $DIR/where-clause-doesnt-apply.rs:5:22
|
LL | fn needs_foo(_: impl Foo) {}
| ^^^ required by this bound in `needs_foo`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
Loading