Skip to content

evaluate_goal avoid unnecessary step #142774

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+73 −163
Open

evaluate_goal avoid unnecessary step #142774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5132e6c
`evaluate_goal`: avoid unnecessary step
lcnr Jun 20, 2025
e8e32c3
inspect: merge `[Canonical]GoalEvaluation`
lcnr Jun 20, 2025
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
74 changes: 10 additions & 64 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::ops::ControlFlow;

#[cfg(feature = "nightly")]
use rustc_macros::HashStable_NoContext;
use rustc_type_ir::data_structures::{HashMap, HashSet, ensure_sufficient_stack};
use rustc_type_ir::data_structures::{HashMap, HashSet};
use rustc_type_ir::fast_reject::DeepRejectCtxt;
use rustc_type_ir::inherent::*;
use rustc_type_ir::relate::Relate;
Expand Down Expand Up @@ -339,13 +339,12 @@ where

/// Creates a nested evaluation context that shares the same search graph as the
/// one passed in. This is suitable for evaluation, granted that the search graph
/// has had the nested goal recorded on its stack ([`SearchGraph::with_new_goal`]),
/// but it's preferable to use other methods that call this one rather than this
/// method directly.
/// has had the nested goal recorded on its stack. This method only be used by
/// `search_graph::Delegate::compute_goal`.
///
/// This function takes care of setting up the inference context, setting the anchor,
/// and registering opaques from the canonicalized input.
fn enter_canonical<R>(
pub(super) fn enter_canonical<R>(
cx: I,
search_graph: &'a mut SearchGraph<D>,
canonical_input: CanonicalInput<I>,
Expand Down Expand Up @@ -401,56 +400,6 @@ where
result
}

/// The entry point of the solver.
///
/// This function deals with (coinductive) cycles, overflow, and caching
/// and then calls [`EvalCtxt::compute_goal`] which contains the actual
/// logic of the solver.
///
/// Instead of calling this function directly, use either [EvalCtxt::evaluate_goal]
/// if you're inside of the solver or [SolverDelegateEvalExt::evaluate_root_goal] if you're
/// outside of it.
#[instrument(level = "debug", skip(cx, search_graph, goal_evaluation), ret)]
fn evaluate_canonical_goal(
cx: I,
search_graph: &'a mut SearchGraph<D>,
canonical_input: CanonicalInput<I>,
step_kind_from_parent: PathKind,
goal_evaluation: &mut ProofTreeBuilder<D>,
) -> QueryResult<I> {
let mut canonical_goal_evaluation =
goal_evaluation.new_canonical_goal_evaluation(canonical_input);

// Deal with overflow, caching, and coinduction.
//
// The actual solver logic happens in `ecx.compute_goal`.
let result = ensure_sufficient_stack(|| {
search_graph.with_new_goal(
cx,
canonical_input,
step_kind_from_parent,
&mut canonical_goal_evaluation,
|search_graph, cx, canonical_input, canonical_goal_evaluation| {
EvalCtxt::enter_canonical(
cx,
search_graph,
canonical_input,
canonical_goal_evaluation,
|ecx, goal| {
let result = ecx.compute_goal(goal);
ecx.inspect.query_result(result);
result
},
)
},
)
});

canonical_goal_evaluation.query_result(result);
goal_evaluation.canonical_goal_evaluation(canonical_goal_evaluation);
result
}

/// Recursively evaluates `goal`, returning whether any inference vars have
/// been constrained and the certainty of the result.
fn evaluate_goal(
Expand Down Expand Up @@ -504,18 +453,16 @@ where
let (orig_values, canonical_goal) = self.canonicalize_goal(goal);
let mut goal_evaluation =
self.inspect.new_goal_evaluation(goal, &orig_values, goal_evaluation_kind);
let canonical_response = EvalCtxt::evaluate_canonical_goal(
let canonical_result = self.search_graph.evaluate_goal(
self.cx(),
self.search_graph,
canonical_goal,
self.step_kind_for_source(source),
&mut goal_evaluation,
);
let response = match canonical_response {
Err(e) => {
self.inspect.goal_evaluation(goal_evaluation);
return Err(e);
}
goal_evaluation.query_result(canonical_result);
self.inspect.goal_evaluation(goal_evaluation);
let response = match canonical_result {
Err(e) => return Err(e),
Ok(response) => response,
};
Comment on lines +464 to 467
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let response = match canonical_result {
Err(e) => return Err(e),
Ok(response) => response,
};
let response = canonical_result?

Copy link
Contributor Author

@lcnr lcnr Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

considered it, i actually kinda feel like explicitly matching is better than using ? on a local 🤔 but fine to apply this if u think it's significantly better :3

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think it is, but I understand verbosity as a signal that something unusual is going on can be useful. I just see this getting changed as a drive by in the future without a comment, and if you add a comment to the one liner you have as much signal as the expanded version


Expand All @@ -524,7 +471,6 @@ where

let (normalization_nested_goals, certainty) =
self.instantiate_and_apply_query_response(goal.param_env, &orig_values, response);
self.inspect.goal_evaluation(goal_evaluation);

// FIXME: We previously had an assert here that checked that recomputing
// a goal after applying its constraints did not change its response.
Expand Down Expand Up @@ -585,7 +531,7 @@ where
Ok((normalization_nested_goals, GoalEvaluation { certainty, has_changed, stalled_on }))
}

fn compute_goal(&mut self, goal: Goal<I, I::Predicate>) -> QueryResult<I> {
pub(super) fn compute_goal(&mut self, goal: Goal<I, I::Predicate>) -> QueryResult<I> {
let Goal { param_env, predicate } = goal;
let kind = predicate.kind();
if let Some(kind) = kind.no_bound_vars() {
Expand Down
88 changes: 18 additions & 70 deletions compiler/rustc_next_trait_solver/src/solve/inspect/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ use rustc_type_ir::{self as ty, Interner};
use crate::delegate::SolverDelegate;
use crate::solve::eval_ctxt::canonical;
use crate::solve::{
CanonicalInput, Certainty, GenerateProofTree, Goal, GoalEvaluationKind, GoalSource,
QueryResult, inspect,
Certainty, GenerateProofTree, Goal, GoalEvaluationKind, GoalSource, QueryResult, inspect,
};

/// The core data structure when building proof trees.
Expand Down Expand Up @@ -54,7 +53,6 @@ where
enum DebugSolver<I: Interner> {
Root,
GoalEvaluation(WipGoalEvaluation<I>),
CanonicalGoalEvaluation(WipCanonicalGoalEvaluation<I>),
CanonicalGoalEvaluationStep(WipCanonicalGoalEvaluationStep<I>),
}

Expand All @@ -64,12 +62,6 @@ impl<I: Interner> From<WipGoalEvaluation<I>> for DebugSolver<I> {
}
}

impl<I: Interner> From<WipCanonicalGoalEvaluation<I>> for DebugSolver<I> {
fn from(g: WipCanonicalGoalEvaluation<I>) -> DebugSolver<I> {
DebugSolver::CanonicalGoalEvaluation(g)
}
}

impl<I: Interner> From<WipCanonicalGoalEvaluationStep<I>> for DebugSolver<I> {
fn from(g: WipCanonicalGoalEvaluationStep<I>) -> DebugSolver<I> {
DebugSolver::CanonicalGoalEvaluationStep(g)
Expand All @@ -80,39 +72,23 @@ impl<I: Interner> From<WipCanonicalGoalEvaluationStep<I>> for DebugSolver<I> {
struct WipGoalEvaluation<I: Interner> {
pub uncanonicalized_goal: Goal<I, I::Predicate>,
pub orig_values: Vec<I::GenericArg>,
pub evaluation: Option<WipCanonicalGoalEvaluation<I>>,
pub encountered_overflow: bool,
/// After we finished evaluating this is moved into `kind`.
pub final_revision: Option<WipCanonicalGoalEvaluationStep<I>>,
pub result: Option<QueryResult<I>>,
}

impl<I: Interner> WipGoalEvaluation<I> {
fn finalize(self) -> inspect::GoalEvaluation<I> {
inspect::GoalEvaluation {
uncanonicalized_goal: self.uncanonicalized_goal,
orig_values: self.orig_values,
evaluation: self.evaluation.unwrap().finalize(),
}
}
}

#[derive_where(PartialEq, Eq, Debug; I: Interner)]
struct WipCanonicalGoalEvaluation<I: Interner> {
goal: CanonicalInput<I>,
encountered_overflow: bool,
/// Only used for uncached goals. After we finished evaluating
/// the goal, this is interned and moved into `kind`.
final_revision: Option<WipCanonicalGoalEvaluationStep<I>>,
result: Option<QueryResult<I>>,
}

impl<I: Interner> WipCanonicalGoalEvaluation<I> {
fn finalize(self) -> inspect::CanonicalGoalEvaluation<I> {
inspect::CanonicalGoalEvaluation {
goal: self.goal,
kind: if self.encountered_overflow {
assert!(self.final_revision.is_none());
inspect::CanonicalGoalEvaluationKind::Overflow
inspect::GoalEvaluationKind::Overflow
} else {
let final_revision = self.final_revision.unwrap().finalize();
inspect::CanonicalGoalEvaluationKind::Evaluation { final_revision }
inspect::GoalEvaluationKind::Evaluation { final_revision }
},
result: self.result.unwrap(),
}
Expand Down Expand Up @@ -256,55 +232,27 @@ impl<D: SolverDelegate<Interner = I>, I: Interner> ProofTreeBuilder<D> {

pub(in crate::solve) fn new_goal_evaluation(
&mut self,
goal: Goal<I, I::Predicate>,
uncanonicalized_goal: Goal<I, I::Predicate>,
orig_values: &[I::GenericArg],
kind: GoalEvaluationKind,
) -> ProofTreeBuilder<D> {
self.opt_nested(|| match kind {
GoalEvaluationKind::Root => Some(WipGoalEvaluation {
uncanonicalized_goal: goal,
uncanonicalized_goal,
orig_values: orig_values.to_vec(),
evaluation: None,
encountered_overflow: false,
final_revision: None,
result: None,
}),
GoalEvaluationKind::Nested => None,
})
}

pub(crate) fn new_canonical_goal_evaluation(
&mut self,
goal: CanonicalInput<I>,
) -> ProofTreeBuilder<D> {
self.nested(|| WipCanonicalGoalEvaluation {
goal,
encountered_overflow: false,
final_revision: None,
result: None,
})
}

pub(crate) fn canonical_goal_evaluation(
&mut self,
canonical_goal_evaluation: ProofTreeBuilder<D>,
) {
if let Some(this) = self.as_mut() {
match (this, *canonical_goal_evaluation.state.unwrap()) {
(
DebugSolver::GoalEvaluation(goal_evaluation),
DebugSolver::CanonicalGoalEvaluation(canonical_goal_evaluation),
) => {
let prev = goal_evaluation.evaluation.replace(canonical_goal_evaluation);
assert_eq!(prev, None);
}
_ => unreachable!(),
}
}
}

pub(crate) fn canonical_goal_evaluation_overflow(&mut self) {
if let Some(this) = self.as_mut() {
match this {
DebugSolver::CanonicalGoalEvaluation(canonical_goal_evaluation) => {
canonical_goal_evaluation.encountered_overflow = true;
DebugSolver::GoalEvaluation(goal_evaluation) => {
goal_evaluation.encountered_overflow = true;
}
_ => unreachable!(),
};
Expand Down Expand Up @@ -343,10 +291,10 @@ impl<D: SolverDelegate<Interner = I>, I: Interner> ProofTreeBuilder<D> {
if let Some(this) = self.as_mut() {
match (this, *goal_evaluation_step.state.unwrap()) {
(
DebugSolver::CanonicalGoalEvaluation(canonical_goal_evaluations),
DebugSolver::GoalEvaluation(goal_evaluation),
DebugSolver::CanonicalGoalEvaluationStep(goal_evaluation_step),
) => {
canonical_goal_evaluations.final_revision = Some(goal_evaluation_step);
goal_evaluation.final_revision = Some(goal_evaluation_step);
}
_ => unreachable!(),
}
Expand Down Expand Up @@ -489,8 +437,8 @@ impl<D: SolverDelegate<Interner = I>, I: Interner> ProofTreeBuilder<D> {
pub(crate) fn query_result(&mut self, result: QueryResult<I>) {
if let Some(this) = self.as_mut() {
match this {
DebugSolver::CanonicalGoalEvaluation(canonical_goal_evaluation) => {
assert_eq!(canonical_goal_evaluation.result.replace(result), None);
DebugSolver::GoalEvaluation(goal_evaluation) => {
assert_eq!(goal_evaluation.result.replace(result), None);
}
DebugSolver::CanonicalGoalEvaluationStep(evaluation_step) => {
assert_eq!(
Expand Down
22 changes: 19 additions & 3 deletions compiler/rustc_next_trait_solver/src/solve/search_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use std::convert::Infallible;
use std::marker::PhantomData;

use rustc_type_ir::data_structures::ensure_sufficient_stack;
use rustc_type_ir::search_graph::{self, PathKind};
use rustc_type_ir::solve::{CanonicalInput, Certainty, NoSolution, QueryResult};
use rustc_type_ir::{Interner, TypingMode};

use super::inspect::ProofTreeBuilder;
use super::{FIXPOINT_STEP_LIMIT, has_no_inference_or_external_constraints};
use crate::delegate::SolverDelegate;
use crate::solve::inspect::ProofTreeBuilder;
use crate::solve::{EvalCtxt, FIXPOINT_STEP_LIMIT, has_no_inference_or_external_constraints};

/// This type is never constructed. We only use it to implement `search_graph::Delegate`
/// for all types which impl `SolverDelegate` and doing it directly fails in coherence.
Expand Down Expand Up @@ -80,8 +81,8 @@ where

fn on_stack_overflow(
cx: I,
inspect: &mut ProofTreeBuilder<D>,
input: CanonicalInput<I>,
inspect: &mut ProofTreeBuilder<D>,
) -> QueryResult<I> {
inspect.canonical_goal_evaluation_overflow();
response_no_constraints(cx, input, Certainty::overflow(true))
Expand All @@ -106,6 +107,21 @@ where
let certainty = from_result.unwrap().value.certainty;
response_no_constraints(cx, for_input, certainty)
}

fn compute_goal(
search_graph: &mut SearchGraph<D>,
cx: I,
input: CanonicalInput<I>,
inspect: &mut Self::ProofTreeBuilder,
) -> QueryResult<I> {
ensure_sufficient_stack(|| {
EvalCtxt::enter_canonical(cx, search_graph, input, inspect, |ecx, goal| {
let result = ecx.compute_goal(goal);
ecx.inspect.query_result(result);
result
})
})
}
}

fn response_no_constraints<I: Interner>(
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_trait_selection/src/solve/inspect/analyse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub struct InspectGoal<'a, 'tcx> {
orig_values: Vec<ty::GenericArg<'tcx>>,
goal: Goal<'tcx, ty::Predicate<'tcx>>,
result: Result<Certainty, NoSolution>,
evaluation_kind: inspect::CanonicalGoalEvaluationKind<TyCtxt<'tcx>>,
evaluation_kind: inspect::GoalEvaluationKind<TyCtxt<'tcx>>,
normalizes_to_term_hack: Option<NormalizesToTermHack<'tcx>>,
source: GoalSource,
}
Expand Down Expand Up @@ -396,8 +396,8 @@ impl<'a, 'tcx> InspectGoal<'a, 'tcx> {
let mut candidates = vec![];
let last_eval_step = match &self.evaluation_kind {
// An annoying edge case in case the recursion limit is 0.
inspect::CanonicalGoalEvaluationKind::Overflow => return vec![],
inspect::CanonicalGoalEvaluationKind::Evaluation { final_revision } => final_revision,
inspect::GoalEvaluationKind::Overflow => return vec![],
inspect::GoalEvaluationKind::Evaluation { final_revision } => final_revision,
};

let mut nested_goals = vec![];
Expand Down Expand Up @@ -429,10 +429,10 @@ impl<'a, 'tcx> InspectGoal<'a, 'tcx> {
) -> Self {
let infcx = <&SolverDelegate<'tcx>>::from(infcx);

let inspect::GoalEvaluation { uncanonicalized_goal, orig_values, evaluation } = root;
let inspect::GoalEvaluation { uncanonicalized_goal, orig_values, kind, result } = root;
// If there's a normalizes-to goal, AND the evaluation result with the result of
// constraining the normalizes-to RHS and computing the nested goals.
let result = evaluation.result.and_then(|ok| {
let result = result.and_then(|ok| {
let nested_goals_certainty =
term_hack_and_nested_certainty.map_or(Ok(Certainty::Yes), |(_, c)| c)?;
Ok(ok.value.certainty.and(nested_goals_certainty))
Expand All @@ -444,7 +444,7 @@ impl<'a, 'tcx> InspectGoal<'a, 'tcx> {
orig_values,
goal: eager_resolve_vars(infcx, uncanonicalized_goal),
result,
evaluation_kind: evaluation.kind,
evaluation_kind: kind,
normalizes_to_term_hack: term_hack_and_nested_certainty.map(|(n, _)| n),
source,
}
Expand Down
Loading
Loading