From 2e57845de7b81dd984a8010e191b7b8fbb62a1f9 Mon Sep 17 00:00:00 2001 From: lcnr Date: Mon, 16 Jun 2025 15:40:40 +0200 Subject: [PATCH 1/4] actually use a doc comment --- .../rustc_type_ir/src/search_graph/mod.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_type_ir/src/search_graph/mod.rs b/compiler/rustc_type_ir/src/search_graph/mod.rs index b59b4f9285400..2bd0fa6866ea2 100644 --- a/compiler/rustc_type_ir/src/search_graph/mod.rs +++ b/compiler/rustc_type_ir/src/search_graph/mod.rs @@ -1,16 +1,16 @@ -/// The search graph is responsible for caching and cycle detection in the trait -/// solver. Making sure that caching doesn't result in soundness bugs or unstable -/// query results is very challenging and makes this one of the most-involved -/// self-contained components of the compiler. -/// -/// We added fuzzing support to test its correctness. The fuzzers used to verify -/// the current implementation can be found in https://github.com/lcnr/search_graph_fuzz. -/// -/// This is just a quick overview of the general design, please check out the relevant -/// [rustc-dev-guide chapter](https://rustc-dev-guide.rust-lang.org/solve/caching.html) for -/// more details. Caching is split between a global cache and the per-cycle `provisional_cache`. -/// The global cache has to be completely unobservable, while the per-cycle cache may impact -/// behavior as long as the resulting behavior is still correct. +//! The search graph is responsible for caching and cycle detection in the trait +//! solver. Making sure that caching doesn't result in soundness bugs or unstable +//! query results is very challenging and makes this one of the most-involved +//! self-contained components of the compiler. +//! +//! We added fuzzing support to test its correctness. The fuzzers used to verify +//! the current implementation can be found in . +//! +//! This is just a quick overview of the general design, please check out the relevant +//! [rustc-dev-guide chapter](https://rustc-dev-guide.rust-lang.org/solve/caching.html) for +//! more details. Caching is split between a global cache and the per-cycle `provisional_cache`. +//! The global cache has to be completely unobservable, while the per-cycle cache may impact +//! behavior as long as the resulting behavior is still correct. use std::cmp::Ordering; use std::collections::BTreeMap; use std::collections::hash_map::Entry; From 8710c1fec9bb8d9a3f235c8143f8e94b98bed69c Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 17 Jun 2025 10:27:31 +0200 Subject: [PATCH 2/4] update comment --- compiler/rustc_type_ir/src/search_graph/mod.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_type_ir/src/search_graph/mod.rs b/compiler/rustc_type_ir/src/search_graph/mod.rs index 2bd0fa6866ea2..953dc2d1bc841 100644 --- a/compiler/rustc_type_ir/src/search_graph/mod.rs +++ b/compiler/rustc_type_ir/src/search_graph/mod.rs @@ -381,18 +381,16 @@ impl PathsToNested { /// The nested goals of each stack entry and the path from the /// stack entry to that nested goal. /// +/// They are used when checking whether reevaluating a global cache +/// would encounter a cycle or use a provisional cache entry given the +/// currentl search graph state. We need to disable the global cache +/// in this case as it could otherwise result in behaviorial differences. +/// Cycles can impact behavior. The cycle ABA may have different final +/// results from a the cycle BAB depending on the cycle root. +/// /// We only start tracking nested goals once we've either encountered /// overflow or a solver cycle. This is a performance optimization to /// avoid tracking nested goals on the happy path. -/// -/// We use nested goals for two reasons: -/// - when rebasing provisional cache entries -/// - when checking whether we have to ignore a global cache entry as reevaluating -/// it would encounter a cycle or use a provisional cache entry. -/// -/// We need to disable the global cache if using it would hide a cycle, as -/// cycles can impact behavior. The cycle ABA may have different final -/// results from a the cycle BAB depending on the cycle root. #[derive_where(Debug, Default, Clone; X: Cx)] struct NestedGoals { nested_goals: HashMap, From 75da5c4c29048d388e89512164fff08a67f3abe4 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 17 Jun 2025 10:56:26 +0200 Subject: [PATCH 3/4] reevaluate: reset `encountered_overflow` also return `EvaluationResult` instead of the final `StackEntry` to make sure we correctly track information between reruns. --- .../src/search_graph/global_cache.rs | 17 ++- .../rustc_type_ir/src/search_graph/mod.rs | 117 +++++++++++++----- .../rustc_type_ir/src/search_graph/stack.rs | 8 +- 3 files changed, 97 insertions(+), 45 deletions(-) diff --git a/compiler/rustc_type_ir/src/search_graph/global_cache.rs b/compiler/rustc_type_ir/src/search_graph/global_cache.rs index a2442660259dc..1b99cc820f1ab 100644 --- a/compiler/rustc_type_ir/src/search_graph/global_cache.rs +++ b/compiler/rustc_type_ir/src/search_graph/global_cache.rs @@ -2,6 +2,7 @@ use derive_where::derive_where; use super::{AvailableDepth, Cx, NestedGoals}; use crate::data_structures::HashMap; +use crate::search_graph::EvaluationResult; struct Success { required_depth: usize, @@ -43,28 +44,26 @@ impl GlobalCache { &mut self, cx: X, input: X::Input, - - origin_result: X::Result, + evaluation_result: EvaluationResult, dep_node: X::DepNodeIndex, - - required_depth: usize, - encountered_overflow: bool, - nested_goals: NestedGoals, ) { - let result = cx.mk_tracked(origin_result, dep_node); + let EvaluationResult { encountered_overflow, required_depth, heads, nested_goals, result } = + evaluation_result; + debug_assert!(heads.is_empty()); + let result = cx.mk_tracked(result, dep_node); let entry = self.map.entry(input).or_default(); if encountered_overflow { let with_overflow = WithOverflow { nested_goals, result }; let prev = entry.with_overflow.insert(required_depth, with_overflow); if let Some(prev) = &prev { assert!(cx.evaluation_is_concurrent()); - assert_eq!(cx.get_tracked(&prev.result), origin_result); + assert_eq!(cx.get_tracked(&prev.result), evaluation_result.result); } } else { let prev = entry.success.replace(Success { required_depth, nested_goals, result }); if let Some(prev) = &prev { assert!(cx.evaluation_is_concurrent()); - assert_eq!(cx.get_tracked(&prev.result), origin_result); + assert_eq!(cx.get_tracked(&prev.result), evaluation_result.result); } } } diff --git a/compiler/rustc_type_ir/src/search_graph/mod.rs b/compiler/rustc_type_ir/src/search_graph/mod.rs index 953dc2d1bc841..1a7ccb5b916c4 100644 --- a/compiler/rustc_type_ir/src/search_graph/mod.rs +++ b/compiler/rustc_type_ir/src/search_graph/mod.rs @@ -448,6 +448,43 @@ struct ProvisionalCacheEntry { result: X::Result, } +/// The final result of evaluating a goal. +/// +/// We reset `encountered_overflow` when reevaluating a goal, +/// but need to track whether we've hit the recursion limit at +/// all for correctness. +/// +/// We've previously simply returned the final `StackEntry` but this +/// made it easy to accidentally drop information from the previous +/// evaluation. +#[derive_where(Debug; X: Cx)] +struct EvaluationResult { + encountered_overflow: bool, + required_depth: usize, + heads: CycleHeads, + nested_goals: NestedGoals, + result: X::Result, +} + +impl EvaluationResult { + fn finalize( + final_entry: StackEntry, + encountered_overflow: bool, + result: X::Result, + ) -> EvaluationResult { + EvaluationResult { + encountered_overflow, + // Unlike `encountered_overflow`, we share `heads`, `required_depth`, + // and `nested_goals` between evaluations. + required_depth: final_entry.required_depth, + heads: final_entry.heads, + nested_goals: final_entry.nested_goals, + // We only care about the final result. + result, + } + } +} + pub struct SearchGraph, X: Cx = ::Cx> { root_depth: AvailableDepth, /// The stack of goals currently being computed. @@ -614,12 +651,12 @@ impl, X: Cx> SearchGraph { input, step_kind_from_parent, available_depth, + provisional_result: None, required_depth: 0, heads: Default::default(), encountered_overflow: false, has_been_used: None, nested_goals: Default::default(), - provisional_result: None, }); // This is for global caching, so we properly track query dependencies. @@ -628,7 +665,7 @@ impl, X: Cx> SearchGraph { // not tracked by the cache key and from outside of this anon task, it // must not be added to the global cache. Notably, this is the case for // trait solver cycles participants. - let ((final_entry, result), dep_node) = cx.with_cached_task(|| { + let (evaluation_result, dep_node) = cx.with_cached_task(|| { self.evaluate_goal_in_task(cx, input, inspect, &mut evaluate_goal) }); @@ -636,27 +673,34 @@ impl, X: Cx> SearchGraph { // lazily update its parent goal. Self::update_parent_goal( &mut self.stack, - final_entry.step_kind_from_parent, - final_entry.required_depth, - &final_entry.heads, - final_entry.encountered_overflow, - UpdateParentGoalCtxt::Ordinary(&final_entry.nested_goals), + step_kind_from_parent, + evaluation_result.required_depth, + &evaluation_result.heads, + evaluation_result.encountered_overflow, + UpdateParentGoalCtxt::Ordinary(&evaluation_result.nested_goals), ); + let result = evaluation_result.result; // We're now done with this goal. We only add the root of cycles to the global cache. // In case this goal is involved in a larger cycle add it to the provisional cache. - if final_entry.heads.is_empty() { + if evaluation_result.heads.is_empty() { if let Some((_scope, expected)) = validate_cache { // Do not try to move a goal into the cache again if we're testing // the global cache. - assert_eq!(result, expected, "input={input:?}"); + assert_eq!(evaluation_result.result, expected, "input={input:?}"); } else if D::inspect_is_noop(inspect) { - self.insert_global_cache(cx, final_entry, result, dep_node) + self.insert_global_cache(cx, input, evaluation_result, dep_node) } } else if D::ENABLE_PROVISIONAL_CACHE { debug_assert!(validate_cache.is_none(), "unexpected non-root: {input:?}"); let entry = self.provisional_cache.entry(input).or_default(); - let StackEntry { heads, encountered_overflow, .. } = final_entry; + let EvaluationResult { + encountered_overflow, + required_depth: _, + heads, + nested_goals: _, + result, + } = evaluation_result; let path_from_head = Self::cycle_path_kind( &self.stack, step_kind_from_parent, @@ -1022,18 +1066,24 @@ impl, X: Cx> SearchGraph { input: X::Input, inspect: &mut D::ProofTreeBuilder, mut evaluate_goal: impl FnMut(&mut Self, &mut D::ProofTreeBuilder) -> X::Result, - ) -> (StackEntry, X::Result) { + ) -> EvaluationResult { + // We reset `encountered_overflow` each time we rerun this goal + // but need to make sure we currently propagate it to the global + // cache even if only some of the evaluations actually reach the + // recursion limit. + let mut encountered_overflow = false; let mut i = 0; loop { let result = evaluate_goal(self, inspect); let stack_entry = self.stack.pop(); + encountered_overflow |= stack_entry.encountered_overflow; debug_assert_eq!(stack_entry.input, input); // If the current goal is not the root of a cycle, we are done. // // There are no provisional cache entries which depend on this goal. let Some(usage_kind) = stack_entry.has_been_used else { - return (stack_entry, result); + return EvaluationResult::finalize(stack_entry, encountered_overflow, result); }; // If it is a cycle head, we have to keep trying to prove it until @@ -1049,7 +1099,7 @@ impl, X: Cx> SearchGraph { // final result is equal to the initial response for that case. if self.reached_fixpoint(cx, &stack_entry, usage_kind, result) { self.rebase_provisional_cache_entries(&stack_entry, |_, result| result); - return (stack_entry, result); + return EvaluationResult::finalize(stack_entry, encountered_overflow, result); } // If computing this goal results in ambiguity with no constraints, @@ -1068,7 +1118,7 @@ impl, X: Cx> SearchGraph { self.rebase_provisional_cache_entries(&stack_entry, |input, _| { D::propagate_ambiguity(cx, input, result) }); - return (stack_entry, result); + return EvaluationResult::finalize(stack_entry, encountered_overflow, result); }; // If we've reached the fixpoint step limit, we bail with overflow and taint all @@ -1080,7 +1130,7 @@ impl, X: Cx> SearchGraph { self.rebase_provisional_cache_entries(&stack_entry, |input, _| { D::on_fixpoint_overflow(cx, input) }); - return (stack_entry, result); + return EvaluationResult::finalize(stack_entry, encountered_overflow, result); } // Clear all provisional cache entries which depend on a previous provisional @@ -1089,9 +1139,22 @@ impl, X: Cx> SearchGraph { debug!(?result, "fixpoint changed provisional results"); self.stack.push(StackEntry { - has_been_used: None, + input, + step_kind_from_parent: stack_entry.step_kind_from_parent, + available_depth: stack_entry.available_depth, provisional_result: Some(result), - ..stack_entry + // We can keep these goals from previous iterations as they are only + // ever read after finalizing this evaluation. + required_depth: stack_entry.required_depth, + heads: stack_entry.heads, + nested_goals: stack_entry.nested_goals, + // We reset these two fields when rerunning this goal. We could + // keep `encountered_overflow` as it's only used as a performance + // optimization. However, given that the proof tree will likely look + // similar to the previous iterations when reevaluating, it's better + // for caching if the reevaluation also starts out with `false`. + encountered_overflow: false, + has_been_used: None, }); } } @@ -1107,21 +1170,11 @@ impl, X: Cx> SearchGraph { fn insert_global_cache( &mut self, cx: X, - final_entry: StackEntry, - result: X::Result, + input: X::Input, + evaluation_result: EvaluationResult, dep_node: X::DepNodeIndex, ) { - debug!(?final_entry, ?result, "insert global cache"); - cx.with_global_cache(|cache| { - cache.insert( - cx, - final_entry.input, - result, - dep_node, - final_entry.required_depth, - final_entry.encountered_overflow, - final_entry.nested_goals, - ) - }) + debug!(?evaluation_result, "insert global cache"); + cx.with_global_cache(|cache| cache.insert(cx, input, evaluation_result, dep_node)) } } diff --git a/compiler/rustc_type_ir/src/search_graph/stack.rs b/compiler/rustc_type_ir/src/search_graph/stack.rs index 8bb247bf0554d..e0fd934df698f 100644 --- a/compiler/rustc_type_ir/src/search_graph/stack.rs +++ b/compiler/rustc_type_ir/src/search_graph/stack.rs @@ -26,6 +26,10 @@ pub(super) struct StackEntry { /// The available depth of a given goal, immutable. pub available_depth: AvailableDepth, + /// Starts out as `None` and gets set when rerunning this + /// goal in case we encounter a cycle. + pub provisional_result: Option, + /// The maximum depth required while evaluating this goal. pub required_depth: usize, @@ -42,10 +46,6 @@ pub(super) struct StackEntry { /// The nested goals of this goal, see the doc comment of the type. pub nested_goals: NestedGoals, - - /// Starts out as `None` and gets set when rerunning this - /// goal in case we encounter a cycle. - pub provisional_result: Option, } #[derive_where(Default; X: Cx)] From ecd65f870dc62352b15587d82e37a8e2b2480367 Mon Sep 17 00:00:00 2001 From: lcnr Date: Wed, 18 Jun 2025 17:22:04 +0200 Subject: [PATCH 4/4] `evaluate_goal`: accept different inputs --- .../src/solve/eval_ctxt/mod.rs | 2 +- compiler/rustc_type_ir/src/search_graph/mod.rs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs index 7ead0a6d6b776..00fd3ba80465e 100644 --- a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs +++ b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs @@ -430,7 +430,7 @@ where canonical_input, step_kind_from_parent, &mut canonical_goal_evaluation, - |search_graph, canonical_goal_evaluation| { + |search_graph, cx, canonical_input, canonical_goal_evaluation| { EvalCtxt::enter_canonical( cx, search_graph, diff --git a/compiler/rustc_type_ir/src/search_graph/mod.rs b/compiler/rustc_type_ir/src/search_graph/mod.rs index 1a7ccb5b916c4..a857da2fcd5e8 100644 --- a/compiler/rustc_type_ir/src/search_graph/mod.rs +++ b/compiler/rustc_type_ir/src/search_graph/mod.rs @@ -597,7 +597,7 @@ impl, X: Cx> SearchGraph { input: X::Input, step_kind_from_parent: PathKind, inspect: &mut D::ProofTreeBuilder, - mut evaluate_goal: impl FnMut(&mut Self, &mut D::ProofTreeBuilder) -> X::Result, + evaluate_goal: impl Fn(&mut Self, X, X::Input, &mut D::ProofTreeBuilder) -> X::Result + Copy, ) -> X::Result { let Some(available_depth) = AvailableDepth::allowed_depth_for_nested::(self.root_depth, &self.stack) @@ -665,9 +665,8 @@ impl, X: Cx> SearchGraph { // not tracked by the cache key and from outside of this anon task, it // must not be added to the global cache. Notably, this is the case for // trait solver cycles participants. - let (evaluation_result, dep_node) = cx.with_cached_task(|| { - self.evaluate_goal_in_task(cx, input, inspect, &mut evaluate_goal) - }); + let (evaluation_result, dep_node) = + cx.with_cached_task(|| self.evaluate_goal_in_task(cx, input, inspect, evaluate_goal)); // We've finished computing the goal and have popped it from the stack, // lazily update its parent goal. @@ -1065,7 +1064,7 @@ impl, X: Cx> SearchGraph { cx: X, input: X::Input, inspect: &mut D::ProofTreeBuilder, - mut evaluate_goal: impl FnMut(&mut Self, &mut D::ProofTreeBuilder) -> X::Result, + evaluate_goal: impl Fn(&mut Self, X, X::Input, &mut D::ProofTreeBuilder) -> X::Result + Copy, ) -> EvaluationResult { // We reset `encountered_overflow` each time we rerun this goal // but need to make sure we currently propagate it to the global @@ -1074,7 +1073,7 @@ impl, X: Cx> SearchGraph { let mut encountered_overflow = false; let mut i = 0; loop { - let result = evaluate_goal(self, inspect); + let result = evaluate_goal(self, cx, input, inspect); let stack_entry = self.stack.pop(); encountered_overflow |= stack_entry.encountered_overflow; debug_assert_eq!(stack_entry.input, input);