From c60e870f8b4ad90b802e44f8b1b6fa96a225b21f Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 4 Jun 2024 16:29:34 +0200 Subject: [PATCH 1/3] move fixpoitn check into separate fn --- .../src/solve/search_graph.rs | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/search_graph.rs b/compiler/rustc_trait_selection/src/solve/search_graph.rs index 84878fea10139..df6774343a00c 100644 --- a/compiler/rustc_trait_selection/src/solve/search_graph.rs +++ b/compiler/rustc_trait_selection/src/solve/search_graph.rs @@ -504,22 +504,8 @@ impl<'tcx> SearchGraph> { self.stack.next_index(), ); - // Check whether we reached a fixpoint, either because the final result - // is equal to the provisional result of the previous iteration, or because - // this was only the root of either coinductive or inductive cycles, and the - // final result is equal to the initial response for that case. - let reached_fixpoint = if let Some(r) = stack_entry.provisional_result { - r == result - } else if stack_entry.has_been_used == HasBeenUsed::COINDUCTIVE_CYCLE { - Self::response_no_constraints(tcx, input, Certainty::Yes) == result - } else if stack_entry.has_been_used == HasBeenUsed::INDUCTIVE_CYCLE { - Self::response_no_constraints(tcx, input, Certainty::overflow(false)) == result - } else { - false - }; - // If we did not reach a fixpoint, update the provisional result and reevaluate. - if reached_fixpoint { + if self.reached_fixpoint(tcx, input, &stack_entry, result) { StepResult::Done(stack_entry, result) } else { let depth = self.stack.push(StackEntry { @@ -532,6 +518,28 @@ impl<'tcx> SearchGraph> { } } + /// Check whether we reached a fixpoint, either because the final result + /// is equal to the provisional result of the previous iteration, or because + /// this was only the root of either coinductive or inductive cycles, and the + /// final result is equal to the initial response for that case. + fn reached_fixpoint( + &self, + tcx: TyCtxt<'tcx>, + input: CanonicalInput>, + stack_entry: &StackEntry>, + result: QueryResult>, + ) -> bool { + if let Some(r) = stack_entry.provisional_result { + r == result + } else if stack_entry.has_been_used == HasBeenUsed::COINDUCTIVE_CYCLE { + Self::response_no_constraints(tcx, input, Certainty::Yes) == result + } else if stack_entry.has_been_used == HasBeenUsed::INDUCTIVE_CYCLE { + Self::response_no_constraints(tcx, input, Certainty::overflow(false)) == result + } else { + false + } + } + fn response_no_constraints( tcx: TyCtxt<'tcx>, goal: CanonicalInput>, From c7bebced36d96f3080f2ef2ab2f684a6288c19a7 Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 4 Jun 2024 16:34:20 +0200 Subject: [PATCH 2/3] `response_no_constraints` to free function --- .../src/solve/search_graph.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/search_graph.rs b/compiler/rustc_trait_selection/src/solve/search_graph.rs index df6774343a00c..b46acb42de9f5 100644 --- a/compiler/rustc_trait_selection/src/solve/search_graph.rs +++ b/compiler/rustc_trait_selection/src/solve/search_graph.rs @@ -277,7 +277,7 @@ impl<'tcx> SearchGraph> { inspect .canonical_goal_evaluation_kind(inspect::WipCanonicalGoalEvaluationKind::Overflow); - return Self::response_no_constraints(tcx, input, Certainty::overflow(true)); + return response_no_constraints(tcx, input, Certainty::overflow(true)); }; if let Some(result) = self.lookup_global_cache(tcx, input, available_depth, inspect) { @@ -334,9 +334,9 @@ impl<'tcx> SearchGraph> { return if let Some(result) = self.stack[stack_depth].provisional_result { result } else if is_coinductive_cycle { - Self::response_no_constraints(tcx, input, Certainty::Yes) + response_no_constraints(tcx, input, Certainty::Yes) } else { - Self::response_no_constraints(tcx, input, Certainty::overflow(false)) + response_no_constraints(tcx, input, Certainty::overflow(false)) }; } else { // No entry, we push this goal on the stack and try to prove it. @@ -373,7 +373,7 @@ impl<'tcx> SearchGraph> { debug!("canonical cycle overflow"); let current_entry = self.pop_stack(); debug_assert!(current_entry.has_been_used.is_empty()); - let result = Self::response_no_constraints(tcx, input, Certainty::overflow(false)); + let result = response_no_constraints(tcx, input, Certainty::overflow(false)); (current_entry, result) }); @@ -532,21 +532,21 @@ impl<'tcx> SearchGraph> { if let Some(r) = stack_entry.provisional_result { r == result } else if stack_entry.has_been_used == HasBeenUsed::COINDUCTIVE_CYCLE { - Self::response_no_constraints(tcx, input, Certainty::Yes) == result + response_no_constraints(tcx, input, Certainty::Yes) == result } else if stack_entry.has_been_used == HasBeenUsed::INDUCTIVE_CYCLE { - Self::response_no_constraints(tcx, input, Certainty::overflow(false)) == result + response_no_constraints(tcx, input, Certainty::overflow(false)) == result } else { false } } +} - fn response_no_constraints( - tcx: TyCtxt<'tcx>, - goal: CanonicalInput>, - certainty: Certainty, - ) -> QueryResult> { - Ok(super::response_no_constraints_raw(tcx, goal.max_universe, goal.variables, certainty)) - } +fn response_no_constraints<'tcx>( + tcx: TyCtxt<'tcx>, + goal: CanonicalInput>, + certainty: Certainty, +) -> QueryResult> { + Ok(super::response_no_constraints_raw(tcx, goal.max_universe, goal.variables, certainty)) } impl SearchGraph { From e9036d67b76910f8aecc1ed67377b371bca4e59e Mon Sep 17 00:00:00 2001 From: lcnr Date: Tue, 4 Jun 2024 17:36:06 +0200 Subject: [PATCH 3/3] do not rerun fixpoints on ambiguity --- .../rustc_trait_selection/src/solve/mod.rs | 12 +++-- .../src/solve/search_graph.rs | 51 ++++++++++++++----- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/mod.rs b/compiler/rustc_trait_selection/src/solve/mod.rs index 68a4831c3350b..694a2f133e5a4 100644 --- a/compiler/rustc_trait_selection/src/solve/mod.rs +++ b/compiler/rustc_trait_selection/src/solve/mod.rs @@ -77,9 +77,15 @@ enum GoalEvaluationKind { #[extension(trait CanonicalResponseExt)] impl<'tcx> Canonical<'tcx, Response>> { fn has_no_inference_or_external_constraints(&self) -> bool { - self.value.external_constraints.region_constraints.is_empty() - && self.value.var_values.is_identity() - && self.value.external_constraints.opaque_types.is_empty() + let ExternalConstraintsData { + ref region_constraints, + ref opaque_types, + ref normalization_nested_goals, + } = *self.value.external_constraints; + self.value.var_values.is_identity() + && region_constraints.is_empty() + && opaque_types.is_empty() + && normalization_nested_goals.is_empty() } } diff --git a/compiler/rustc_trait_selection/src/solve/search_graph.rs b/compiler/rustc_trait_selection/src/solve/search_graph.rs index b46acb42de9f5..524a3121407ed 100644 --- a/compiler/rustc_trait_selection/src/solve/search_graph.rs +++ b/compiler/rustc_trait_selection/src/solve/search_graph.rs @@ -16,6 +16,7 @@ use rustc_type_ir::Interner; use super::inspect; use super::inspect::ProofTreeBuilder; use super::SolverMode; +use crate::solve::CanonicalResponseExt; use crate::solve::FIXPOINT_STEP_LIMIT; rustc_index::newtype_index! { @@ -364,7 +365,7 @@ impl<'tcx> SearchGraph> { let ((final_entry, result), dep_node) = tcx.dep_graph.with_anon_task(tcx, dep_kinds::TraitSelect, || { for _ in 0..FIXPOINT_STEP_LIMIT { - match self.fixpoint_step_in_task(tcx, input, inspect, &mut prove_goal) { + match self.fixpoint_step_in_task(input, inspect, &mut prove_goal) { StepResult::Done(final_entry, result) => return (final_entry, result), StepResult::HasChanged => debug!("fixpoint changed provisional results"), } @@ -473,7 +474,6 @@ impl<'tcx> SearchGraph> { /// point we are done. fn fixpoint_step_in_task( &mut self, - tcx: TyCtxt<'tcx>, input: CanonicalInput>, inspect: &mut ProofTreeBuilder>, prove_goal: &mut F, @@ -505,7 +505,7 @@ impl<'tcx> SearchGraph> { ); // If we did not reach a fixpoint, update the provisional result and reevaluate. - if self.reached_fixpoint(tcx, input, &stack_entry, result) { + if self.reached_fixpoint(&stack_entry, result) { StepResult::Done(stack_entry, result) } else { let depth = self.stack.push(StackEntry { @@ -518,24 +518,39 @@ impl<'tcx> SearchGraph> { } } - /// Check whether we reached a fixpoint, either because the final result - /// is equal to the provisional result of the previous iteration, or because - /// this was only the root of either coinductive or inductive cycles, and the - /// final result is equal to the initial response for that case. + /// Check whether we reached a fixpoint we're done with the current cycle head. + /// + /// This is the case if the final result is equal to the used provisional + /// result of the previous iteration or if the result of this iteration is + /// ambiguity with no constraints. We do not care about the previous provisional + /// result in this case. fn reached_fixpoint( &self, - tcx: TyCtxt<'tcx>, - input: CanonicalInput>, stack_entry: &StackEntry>, result: QueryResult>, ) -> bool { - if let Some(r) = stack_entry.provisional_result { + if is_response_no_constraints(result, |c| matches!(c, Certainty::Maybe(_))) { + // If computing this goal results in ambiguity with no constraints, + // we do not rerun it. It's incredibly difficult to get a different + // response in the next iteration in this case. These changes would + // likely either be caused by incompleteness or can change the maybe + // cause from ambiguity to overflow. Returning ambiguity always + // preserves soundness and completeness even if the goal is be known + // to succeed or fail. + // + // This prevents exponential blowup affecting multiple major crates. + true + } else if let Some(r) = stack_entry.provisional_result { + // If we already have a provisional result for this cycle head from + // a previous iteration, simply check for a fixpoint. r == result } else if stack_entry.has_been_used == HasBeenUsed::COINDUCTIVE_CYCLE { - response_no_constraints(tcx, input, Certainty::Yes) == result - } else if stack_entry.has_been_used == HasBeenUsed::INDUCTIVE_CYCLE { - response_no_constraints(tcx, input, Certainty::overflow(false)) == result + // If we only used this cycle head with inductive cycles, and the final + // response is trivially true, no need to rerun. + is_response_no_constraints(result, |c| c == Certainty::Yes) } else { + // No need to check exclusively inductive cycles, as these are covered by the + // general overflow branch above. false } } @@ -549,6 +564,16 @@ fn response_no_constraints<'tcx>( Ok(super::response_no_constraints_raw(tcx, goal.max_universe, goal.variables, certainty)) } +fn is_response_no_constraints<'tcx>( + result: QueryResult>, + check_certainty: impl FnOnce(Certainty) -> bool, +) -> bool { + result.is_ok_and(|response| { + response.has_no_inference_or_external_constraints() + && check_certainty(response.value.certainty) + }) +} + impl SearchGraph { #[allow(rustc::potential_query_instability)] fn check_invariants(&self) {