From 691b60a3f3bc0c28318ee866747da8c7f0af3d3a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 27 Oct 2026 07:30:58 -0700 Subject: [PATCH] [ty] Fix a panic when checking truthiness in a loop (#29759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Ty could panic on `assert predicate` inside a loop where `predicate` starts as a lambda and a later iteration rebinds it to a class. When cycle recovery added the class to the operand type, ty looked up its metaclass to determine its truthiness. That lookup could introduce a new inference cycle while Salsa was recovering the first one. Reachability now evaluates the predicate from the operand types that expression inference has already normalized. Outside loops, it caches the result so repeated walks of the same condition can reuse it. Inside a loop, a condition can depend on an assignment it guards, so the caller reads the inferred operands directly. Expression inference no longer stores predicate truthiness or recomputes it during recovery. The 163-project ecosystem run produced identical diagnostics. Checking isort took 291 ms, up from 248 ms on the base revision, which gives back some of the speedup from #29706. The average timing factor across projects was 1.02×. CodSpeed found no significant changes among the 214 benchmarks it compared. [Ecosystem diff](https://dqhfo45wdv0ek.cloudfront.net/ruff/pull-requests/29759/ty-ecosystem/40467940782/1/diff.html) · [Timings](https://dqhfo45wdv0ek.cloudfront.net/ruff/pull-requests/29759/ty-ecosystem/40467940782/1/timing.html) · [CodSpeed](https://github.com/astral-sh/ruff/pull/29759#issuecomment-6435864937) Closes https://github.com/astral-sh/ty/issues/4834. --- crates/ty_python_core/src/expression.rs | 2 +- .../ty_4834_predicate_truthiness_cycle.py | 11 +++ .../resources/mdtest/loops/for.md | 15 ++++ crates/ty_python_semantic/src/types/infer.rs | 75 ++++++------------- .../src/types/infer/builder.rs | 1 - .../src/types/infer/tests.rs | 1 - 6 files changed, 51 insertions(+), 54 deletions(-) create mode 100644 crates/ty_python_semantic/resources/corpus/ty_4834_predicate_truthiness_cycle.py diff --git a/crates/ty_python_core/src/expression.rs b/crates/ty_python_core/src/expression.rs index c766f22e0b..7ab9a85b49 100644 --- a/crates/ty_python_core/src/expression.rs +++ b/crates/ty_python_core/src/expression.rs @@ -91,7 +91,7 @@ pub enum ExpressionContext { pub enum ExpressionKind { /// An ordinary value expression, such as `1` in `self.x: int = 1`. Normal, - /// A predicate whose truthiness is recorded in its evaluation context. + /// A predicate evaluated in value or condition context. Predicate { context: ExpressionContext, in_loop: bool, diff --git a/crates/ty_python_semantic/resources/corpus/ty_4834_predicate_truthiness_cycle.py b/crates/ty_python_semantic/resources/corpus/ty_4834_predicate_truthiness_cycle.py new file mode 100644 index 0000000000..777246eb46 --- /dev/null +++ b/crates/ty_python_semantic/resources/corpus/ty_4834_predicate_truthiness_cycle.py @@ -0,0 +1,11 @@ +# Regression test for https://github.com/astral-sh/ty/issues/4834 + +class C("", (lambda: value, (predicate := lambda: value))): + pass + +# Read the lambda before the loop adds the class as another possible value of predicate. +predicate +while True: + assert predicate + value = 0 + predicate = C diff --git a/crates/ty_python_semantic/resources/mdtest/loops/for.md b/crates/ty_python_semantic/resources/mdtest/loops/for.md index 63c54a9a37..9a239c8ab8 100644 --- a/crates/ty_python_semantic/resources/mdtest/loops/for.md +++ b/crates/ty_python_semantic/resources/mdtest/loops/for.md @@ -1943,6 +1943,21 @@ for _ in range(1_000_000): x = 2 ``` +The condition can depend on an earlier iteration. Here `y` starts at `2` and only ever receives `1` +or `2` from `x`, so the loop never decrements `x`. + +```py +def f(n: int): + x = 1 + y = 2 + for _ in range(n): + if not y: + x, y = x - 1, x + else: + x, y = 2, x + reveal_type(x) # revealed: Literal[1, 2] +``` + ### Large reachability constraint graphs fall back to `Unknown` ```py diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs index f5f298a8cf..70a09a1ae7 100644 --- a/crates/ty_python_semantic/src/types/infer.rs +++ b/crates/ty_python_semantic/src/types/infer.rs @@ -45,7 +45,7 @@ use crate::ProgramEnvironment; use itertools::Either; -use ruff_db::parsed::{ParsedModuleRef, parsed_module}; +use ruff_db::parsed::parsed_module; use ruff_python_ast as ast; use ruff_text_size::{Ranged, TextRange}; use rustc_hash::FxHashMap; @@ -579,7 +579,7 @@ pub(crate) fn infer_expression_types<'db>( } /// A loop predicate can depend on bindings it guards in the previous iteration. -/// Reading inference directly keeps the extra truthiness query out of that cycle. +/// Evaluate it in the caller's query to keep an extra query out of that cycle. pub(crate) fn infer_predicate_truthiness<'db>( db: &'db dyn Db, expression: Expression<'db>, @@ -588,24 +588,38 @@ pub(crate) fn infer_predicate_truthiness<'db>( expression.kind(db), ExpressionKind::Predicate { in_loop: true, .. } ) { - recorded_predicate_truthiness(db, expression) + evaluate_predicate_truthiness(db, expression) } else { cached_predicate_truthiness(db, expression) } } -/// Keep reachability queries cached when an operand's type changes but its truthiness stays the same. +/// Reachability walks can revisit the same predicate for many different bindings. #[salsa::tracked( returns(copy), cycle_initial = |_, _, _| Truthiness::Ambiguous, heap_size = get_size2::GetSize::get_heap_size )] fn cached_predicate_truthiness<'db>(db: &'db dyn Db, expression: Expression<'db>) -> Truthiness { - recorded_predicate_truthiness(db, expression) + evaluate_predicate_truthiness(db, expression) } -fn recorded_predicate_truthiness<'db>(db: &'db dyn Db, expression: Expression<'db>) -> Truthiness { - infer_expression_types_impl(db, InferExpression::Bare(expression)).predicate_truthiness +/// A widened operand can require a new metaclass lookup, so evaluate truthiness after +/// expression inference has normalized the operand types. +fn evaluate_predicate_truthiness<'db>(db: &'db dyn Db, expression: Expression<'db>) -> Truthiness { + let ExpressionKind::Predicate { context, .. } = expression.kind(db) else { + return Truthiness::Ambiguous; + }; + let inference = infer_expression_types(db, expression, TypeContext::default()); + let env = ProgramEnvironment::from_scope(expression.scope(db)); + let module = parsed_module(db, expression.python_file(db)).load(db); + TruthinessAnalyzer::new( + db, + &env, + |node| inference.expression_type(node), + |node| inference.comparison_truthiness(node), + ) + .truthiness(expression.node_ref(db).node(&module), context) } #[salsa::tracked( @@ -614,7 +628,7 @@ fn recorded_predicate_truthiness<'db>(db: &'db dyn Db, expression: Expression<'d cycle_fn=|db, cycle, previous: &ExpressionInference<'db>, inference: ExpressionInference<'db>, input: InferExpression<'db>| { let (expression, _) = input.into_inner(db); let env = ProgramEnvironment::from_scope(expression.scope(db)); - inference.cycle_normalized(db, &env, previous, cycle, input) + inference.cycle_normalized(db, &env, previous, cycle) }, heap_size=ruff_memory_usage::heap_size )] @@ -639,7 +653,7 @@ pub(super) fn infer_expression_types_impl<'db>( let env = ProgramEnvironment::from_file(program_file); - let mut inference = TypeInferenceBuilder::new( + TypeInferenceBuilder::new( db, &env, InferenceRegion::Expression(expression, tcx), @@ -648,9 +662,7 @@ pub(super) fn infer_expression_types_impl<'db>( index, &module, ) - .finish_expression(); - inference.record_predicate_truthiness(db, &env, input, Some(module)); - inference + .finish_expression() } fn expression_cycle_initial<'db>( @@ -2027,9 +2039,6 @@ pub(crate) struct ExpressionInference<'db> { /// The types of every expression in this region. expressions: FrozenMap>, - /// The predicate's possible outcomes in its evaluation context. - predicate_truthiness: Truthiness, - extra: Option>>, /// The scope this region is part of. @@ -2098,31 +2107,6 @@ struct ExpressionInferenceExtra<'db> { } impl<'db> ExpressionInference<'db> { - /// Reachability always asks for a predicate's truthiness without an expected type. - fn record_predicate_truthiness( - &mut self, - db: &'db dyn Db, - env: &ProgramEnvironment<'db>, - input: InferExpression<'db>, - module: Option, - ) { - let InferExpression::Bare(expression) = input else { - return; - }; - let ExpressionKind::Predicate { context, .. } = expression.kind(db) else { - return; - }; - let module = - module.unwrap_or_else(|| parsed_module(db, expression.python_file(db)).load(db)); - self.predicate_truthiness = TruthinessAnalyzer::new( - db, - env, - |node| self.expression_type(node), - |node| self.comparison_truthiness(node), - ) - .truthiness(expression.node_ref(db).node(&module), context); - } - fn cycle_initial(scope: ScopeId<'db>, cycle_recovery: Type<'db>) -> Self { let _ = scope; Self { @@ -2130,7 +2114,6 @@ impl<'db> ExpressionInference<'db> { cycle_recovery: Some(cycle_recovery), ..ExpressionInferenceExtra::default() })), - predicate_truthiness: Truthiness::Ambiguous, expressions: FrozenMap::default(), #[cfg(debug_assertions)] scope, @@ -2143,7 +2126,6 @@ impl<'db> ExpressionInference<'db> { env: &ProgramEnvironment<'db>, previous: &ExpressionInference<'db>, cycle: &salsa::Cycle, - input: InferExpression<'db>, ) -> ExpressionInference<'db> { if let Some(extra) = self.extra.as_mut() { for (binding, binding_ty) in &mut extra.bindings { @@ -2169,15 +2151,6 @@ impl<'db> ExpressionInference<'db> { *ty = ty.cycle_normalized(db, env, previous_ty, cycle); } - // Normalizing the operands can change whether the predicate completes. Once the first - // provisional results have cleared, retain earlier outcomes so the predicate converges. - self.record_predicate_truthiness(db, env, input, None); - if cycle.iteration() > crate::TAINTED_CYCLES { - self.predicate_truthiness = previous - .predicate_truthiness - .union(self.predicate_truthiness); - } - if cycle.iteration() > crate::TAINTED_CYCLES && let Some(previous_extra) = previous.extra.as_deref() && !previous_extra.collection_use_constraints.is_empty() diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index ad5b8137fd..3fb1e5fa49 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -12949,7 +12949,6 @@ impl<'db> FullExpressionCacheEntry<'db> { }); ExpressionInference { - predicate_truthiness: Truthiness::Ambiguous, expressions: FrozenMap::from(self.expressions), extra, #[cfg(debug_assertions)] diff --git a/crates/ty_python_semantic/src/types/infer/tests.rs b/crates/ty_python_semantic/src/types/infer/tests.rs index 65bf0ed577..d2acdc23f7 100644 --- a/crates/ty_python_semantic/src/types/infer/tests.rs +++ b/crates/ty_python_semantic/src/types/infer/tests.rs @@ -574,7 +574,6 @@ fn comparison_truthiness_widens_across_sparse_cycle_results() -> anyhow::Result< let inference = |ty, truthiness: Option| { ( ExpressionInference { - predicate_truthiness: Truthiness::Ambiguous, expressions: [(expression, ty)].into_iter().collect(), extra: truthiness.map(|truthiness| { Box::new(ExpressionInferenceExtra {