Skip to content
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
15 changes: 15 additions & 0 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ pub trait SolverDelegateEvalExt: SolverDelegate {
stalled_on: Option<GoalStalledOn<Self::Interner>>,
) -> Result<GoalEvaluation<Self::Interner>, NoSolution>;

/// Checks whether a stalled goal would remain stalled if re-evaluated, without consuming
/// `stalled_on`.
fn goal_remains_stalled(&self, stalled_on: &GoalStalledOn<Self::Interner>)
-> Option<Certainty>;

/// Checks whether evaluating `goal` may hold while treating not-yet-defined
/// opaque types as being kind of rigid.
///
Expand Down Expand Up @@ -260,6 +265,16 @@ where
}
}

fn goal_remains_stalled(
&self,
stalled_on: &GoalStalledOn<Self::Interner>,
) -> Option<Certainty> {
match rerunning_stalled_goal_may_make_progress(self, Some(stalled_on)) {
RerunStalled::WontMakeProgress(certainty) => Some(certainty),
RerunStalled::MayMakeProgress => None,
}
}

#[instrument(level = "debug", skip(self), ret)]
fn root_goal_may_hold_opaque_types_jank(
&self,
Expand Down
57 changes: 44 additions & 13 deletions compiler/rustc_trait_selection/src/solve/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ impl<'tcx, E: 'tcx> FulfillmentCtxt<'tcx, E> {
}

fn inspect_evaluated_obligation(
&self,
infcx: &InferCtxt<'tcx>,
obligation: &PredicateObligation<'tcx>,
result: &Result<GoalEvaluation<TyCtxt<'tcx>>, NoSolution>,
Expand Down Expand Up @@ -196,22 +195,39 @@ where
fn try_evaluate_obligations(&mut self, infcx: &InferCtxt<'tcx>) -> TraitErrors<E> {
assert_eq!(self.usable_in_snapshot, infcx.num_open_snapshots());
let mut errors = TraitErrors::NoErrors;
let delegate = <&SolverDelegate<'tcx>>::from(infcx);
loop {
let mut any_changed = false;
for (mut obligation, stalled_on) in mem::take(&mut self.obligations.pending) {
let goal = obligation.as_goal();
let delegate = <&SolverDelegate<'tcx>>::from(infcx);
let mut overflowed = false;

self.obligations.pending.retain_mut(|(obligation, opt_stalled_on)| {
if overflowed {
Comment thread
nnethercote marked this conversation as resolved.
return false;
}

let result = delegate.evaluate_root_goal(goal, obligation.cause.span, stalled_on);
self.inspect_evaluated_obligation(infcx, &obligation, &result);
// Common case: still stalled; keep the obligation. This path is extremely hot in
// some cases; there can be thousands of pending obligations.
if let Some(stalled_on) = opt_stalled_on
&& let Some(certainty) = delegate.goal_remains_stalled(stalled_on)
&& matches!(certainty, Certainty::Maybe(_))

@lcnr lcnr Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should change stalled_on to contain a MaybeCause instead of a Certainty. It is always Certainty::Maybe

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As a follow-up?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sure

{
return true;
}

let result = delegate.evaluate_root_goal(

@jdonszelmann jdonszelmann Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note: the first thing evaluate_root_goal does is to check again goal_remains_stalled.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is some repeated work: rerunning_stalled_goal_may_make_progress can be called twice. It's idempotent and the wasted work doesn't matter. Here's some Cachegrind output:

          .                         // Common case: no inspector, still stalled; keep the obligation. This path is
          .                         // extremely hot in some cases; there can be thousands of pending obligations.
114,082,625 (0.8%)                  if !has_inspector
228,165,250 (1.6%)                      && let Some(stalled_on) = opt_stalled_on
          .                             && let Some(certainty) = delegate.goal_remains_stalled(stalled_on)
          .                             && matches!(certainty, Certainty::Maybe(_))
          .                         { 
          .                             return true;
          .                         }
          .
    622,248 (0.0%)                  let result = delegate.evaluate_root_goal(
          .                             obligation.as_goal(),
    155,562 (0.0%)                      obligation.cause.span,                                                                                   
          .                             opt_stalled_on.take(),                                                                                   
          .                         );      

The common case is more than 100x hotter than what follows. It would be possible to refactor evaluate_root_goal to avoid this wasted work, but evaluate_root_goal has four call sites and they would all need some changes and I don't think it's worthwhile.

obligation.as_goal(),
obligation.cause.span,
opt_stalled_on.take(),
);
Self::inspect_evaluated_obligation(infcx, &obligation, &result);
let GoalEvaluation { goal, certainty, has_changed, stalled_on } = match result {
Ok(result) => result,
Err(NoSolution) => {
errors.push(E::from_solver_error(
infcx,
NextSolverError::TrueError(obligation),
NextSolverError::TrueError(obligation.clone()),
));
continue;
return false;
}
};

Expand All @@ -229,9 +245,11 @@ where
obligation.recursion_depth += 1;

if !infcx.tcx.recursion_limit().value_within_limit(obligation.recursion_depth) {
self.obligations.on_fulfillment_overflow(infcx);
// Only return true errors that we have accumulated while processing.
return errors;
// At this point we want to stop evaluating goals. We can't break out of
// `retain_mut`, so instead we set this flag which causes all other
// elements to be skipped.
overflowed = true;
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this drops all obligations from the pending obligations without pushing anything into errors, which is afaict what's causing the unsoundness in rust-lang/trait-system-refactor-initiative#294

@lcnr lcnr Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't fully trust the LLM generated explanation in that issue, but this code does at least look quite sus and we should definitely assert that there's some error in the error paths :>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah on_fulfillment_overflow is just outdated xx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lcnr This isn't the true cause. The original reproducer in rust-lang/trait-system-refactor-initiative#294 (comment) reproduces with RUSTC_BOOTSTRAP=1 on rust 1.92.0.

@lcnr lcnr Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or actually, this looks preexisting '^^ even before this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did my usual "rewrite it to be faster without changing its behaviour" thing and my understanding of this code is shallow. Is it safe to assume that someone with a deeper knowledge than me will follow up, or do you want me to do something?

} else {
any_changed = true;
}
Expand All @@ -253,11 +271,24 @@ where
if infcx.in_hir_typeck
&& (obligation.has_non_region_infer() || obligation.has_free_regions())
{
infcx.push_hir_typeck_potentially_region_dependent_goal(obligation);
infcx.push_hir_typeck_potentially_region_dependent_goal(
obligation.clone(),
Comment thread
jdonszelmann marked this conversation as resolved.
);
}
false
}
Certainty::Maybe(_) => {
// Update `opt_stalled_on` goal, for the next retain_mut, because we are
// running until a fixpoint.
*opt_stalled_on = stalled_on;
Comment thread
nnethercote marked this conversation as resolved.
true
}
Certainty::Maybe(_) => self.obligations.register(obligation, stalled_on),
}
});
if overflowed {
self.obligations.on_fulfillment_overflow(infcx);
// Only return true errors that we have accumulated while processing.
return errors;
}

if !any_changed {
Expand Down
Loading