From d1d9f365c607f04534fddc3640e053426830a3ce Mon Sep 17 00:00:00 2001 From: David Peter Date: Sat, 14 Mar 2026 08:04:09 +0100 Subject: [PATCH] [ty] Completely remove the `NoReturn` shortcut optimization (#23378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Function calls are relevant for control flow analysis because they may return `Never`/`NoReturn`, and can therefore be terminal. In order to support this, we record `ReturnsNever(…)` constraints during semantic index building for statement-level calls (in almost all situations). These constraints keep track of the call expression such that they can be evaluated during reachability analysis and type narrowing. For example, if we see a call `f(a, b, c)`, we keep track of this full call expression. The return type could depend on the arguments (overloads, generics), so to determine if a call is terminal, we need to evaluate the full call expression. For performance reasons, this analysis contained a short-cut where we looked at the return type annotation of the invoked callable (`f`). Under certain conditions, we could immediately see that a call would definitely be terminal. This previously helped with performance, but is now apparently detrimental. The optimization recently caused problems for generic function calls, so we had to exclude those. It now turns out there was another bug, as I figured out by looking at the ecosystem results on this PR. When the callable expression could not be upcasted to a callable type, we assumed that the call would never be terminal. However, if the callable itself is `Never`, that should also be considered a terminal call. This can happen in unreachable code. Consider this rather weird case, that I extracted from a hydpy ecosystem hit: ```py from typing import Never, Literal def fail() -> Never: raise def _(x: Literal["a", "b"]): if x == "a": if 1 + 1 == 2: return fail() if x == "b": return reveal_type(x) ``` On `main`, the revealed type of `x` is `Literal["a"]`, which is wrong. Since the `fail()` call itself happens in unreachable code, we infer `Never` for `fail` itself, and therefore considered the `if x == "a"` branch to *not* be terminal. On this branch, that bug is fixed and we correctly reveal `Never` for the type of `x`. So the idea here is to get rid of this optimization all together and to simply evaluate the full call expression in all cases, which also makes this much easier to reason about. (I find the `AlwaysFalse`/`AlwaysTrue`-answer of this evaluation very rather confusing, because it's sort of negated twice; I plan to change this in a follow-up) We previously merged https://github.com/astral-sh/ruff/pull/19867 to address [a performance problem](https://github.com/astral-sh/ty/issues/968) in this part of the codebase. It seems to me that the original problem was related to the short-cut path, but I'm not sure if this could bring back the lock congestion problem. In any case, this PR seems to be a performance win across the board on our usual benchmarks. And there is also a small decrease in memory usage. image The disappearing diagnostics all seem to be related to cases like above, where the terminal call happened in unreachable code. Existing tests. --- .../src/semantic_index/builder.rs | 59 +++++++---------- .../src/semantic_index/predicate.rs | 12 +--- .../reachability_constraints.rs | 65 ++----------------- crates/ty_python_semantic/src/types/narrow.rs | 7 +- 4 files changed, 32 insertions(+), 111 deletions(-) diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index ea24d8594a6f0..02e6047f04712 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -35,8 +35,8 @@ use crate::semantic_index::expression::{Expression, ExpressionKind}; use crate::semantic_index::member::MemberExprBuilder; use crate::semantic_index::place::{PlaceExpr, PlaceTableBuilder, ScopedPlaceId}; use crate::semantic_index::predicate::{ - CallableAndCallExpr, ClassPatternKind, PatternPredicate, PatternPredicateKind, Predicate, - PredicateNode, PredicateOrLiteral, ScopedPredicateId, StarImportPlaceholderPredicate, + ClassPatternKind, PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, + PredicateOrLiteral, ScopedPredicateId, StarImportPlaceholderPredicate, }; use crate::semantic_index::re_exports::exported_names; use crate::semantic_index::reachability_constraints::{ @@ -2784,44 +2784,29 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { // We also only add these inside function scopes, since considering module-level // constraints can affect the type of imported symbols, leading to a lot more // work in third-party code. - let call_info = match value.as_ref() { - ast::Expr::Call(ast::ExprCall { func, .. }) => { - Some((func.as_ref(), value.as_ref(), false)) - } - ast::Expr::Await(ast::ExprAwait { value: inner, .. }) => match inner.as_ref() { - ast::Expr::Call(ast::ExprCall { func, .. }) => { - Some((func.as_ref(), value.as_ref(), true)) - } - _ => None, - }, - _ => None, + let is_call = match value.as_ref() { + ast::Expr::Call(_) => true, + ast::Expr::Await(ast::ExprAwait { value: inner, .. }) => inner.is_call_expr(), + _ => false, }; - if let Some((func, expr, is_await)) = call_info { - if !self.source_type.is_stub() && self.in_function_scope() { - let callable = self.add_standalone_expression(func); - let call_expr = self.add_standalone_expression(expr); - - let predicate = Predicate { - node: PredicateNode::ReturnsNever(CallableAndCallExpr { - callable, - call_expr, - is_await, - }), - is_positive: false, - }; - let constraint = self.record_reachability_constraint( - PredicateOrLiteral::Predicate(predicate), - ); + if is_call && !self.source_type.is_stub() && self.in_function_scope() { + let call_expr = self.add_standalone_expression(value.as_ref()); - // Also gate narrowing by this constraint: if the call returns - // `Never`, any narrowing in the current branch should be - // invalidated (since this path is unreachable). This enables - // narrowing to be preserved after if-statements where one branch - // calls a `NoReturn` function like `sys.exit()`. - self.current_use_def_map_mut() - .record_narrowing_constraint_for_all_places(constraint); - } + let predicate = Predicate { + node: PredicateNode::ReturnsNever(call_expr), + is_positive: false, + }; + let constraint = self + .record_reachability_constraint(PredicateOrLiteral::Predicate(predicate)); + + // Also gate narrowing by this constraint: if the call returns + // `Never`, any narrowing in the current branch should be + // invalidated (since this path is unreachable). This enables + // narrowing to be preserved after if-statements where one branch + // calls a `NoReturn` function like `sys.exit()`. + self.current_use_def_map_mut() + .record_narrowing_constraint_for_all_places(constraint); } } _ => { diff --git a/crates/ty_python_semantic/src/semantic_index/predicate.rs b/crates/ty_python_semantic/src/semantic_index/predicate.rs index 39a2b2fbeaa9c..3aa5374b2721a 100644 --- a/crates/ty_python_semantic/src/semantic_index/predicate.rs +++ b/crates/ty_python_semantic/src/semantic_index/predicate.rs @@ -98,20 +98,10 @@ impl PredicateOrLiteral<'_> { } } -#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, salsa::Update, get_size2::GetSize)] -pub(crate) struct CallableAndCallExpr<'db> { - pub(crate) callable: Expression<'db>, - pub(crate) call_expr: Expression<'db>, - /// Whether the call is wrapped in an `await` expression. If `true`, `call_expr` refers to the - /// `await` expression rather than the call itself. This is used to detect terminal `await`s of - /// async functions that return `Never`. - pub(crate) is_await: bool, -} - #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, salsa::Update, get_size2::GetSize)] pub(crate) enum PredicateNode<'db> { Expression(Expression<'db>), - ReturnsNever(CallableAndCallExpr<'db>), + ReturnsNever(Expression<'db>), Pattern(PatternPredicate<'db>), StarImportPlaceholder(StarImportPlaceholderPredicate<'db>), } diff --git a/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs b/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs index a678ec1930efc..bc03dce8b0228 100644 --- a/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs +++ b/crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs @@ -205,12 +205,11 @@ use crate::rank::RankBitBox; use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::place_table; use crate::semantic_index::predicate::{ - CallableAndCallExpr, PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, - Predicates, ScopedPredicateId, + PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, Predicates, ScopedPredicateId, }; use crate::types::{ - CallableTypes, IntersectionBuilder, KnownClass, NarrowingConstraint, Truthiness, Type, - TypeContext, UnionBuilder, UnionType, infer_expression_type, infer_narrowing_constraint, + IntersectionBuilder, KnownClass, NarrowingConstraint, Truthiness, Type, TypeContext, + UnionBuilder, UnionType, infer_expression_type, infer_narrowing_constraint, }; /// A ternary formula that defines under what conditions a binding is visible. (A ternary formula @@ -1090,62 +1089,12 @@ impl ReachabilityConstraints { .bool(db) .negate_if(!predicate.is_positive) } - PredicateNode::ReturnsNever(CallableAndCallExpr { - callable, - call_expr, - is_await, - }) => { - // We first infer just the type of the callable. In the most likely case that the - // function is not marked with `NoReturn`, or that it always returns `NoReturn`, - // doing so allows us to avoid the more expensive work of inferring the entire call - // expression (which could involve inferring argument types to possibly run the overload - // selection algorithm). - // Avoiding this on the happy-path is important because these constraints can be - // very large in number, since we add them on all statement level function calls. - let ty = infer_expression_type(db, callable, TypeContext::default()); - - // Short-circuit for well known types that are known not to return `Never` when called. - // Without the short-circuit, we've seen that threads keep blocking each other - // because they all try to acquire Salsa's `CallableType` lock that ensures each type - // is only interned once. The lock is so heavily congested because there are only - // very few dynamic types, in which case Salsa's sharding the locks by value - // doesn't help much. - // See . - if matches!(ty, Type::Dynamic(_)) { - return Truthiness::AlwaysFalse.negate_if(!predicate.is_positive); - } - - let overloads_iterator = if let Some(callable) = ty - .try_upcast_to_callable(db) - .and_then(CallableTypes::exactly_one) - { - callable.signatures(db).overloads.iter() - } else { - return Truthiness::AlwaysFalse.negate_if(!predicate.is_positive); - }; - - let mut no_overloads_return_never = true; - let mut all_overloads_return_never = true; - let mut any_overload_is_generic = false; - - for overload in overloads_iterator { - let returns_never = overload.return_ty.is_equivalent_to(db, Type::Never); - no_overloads_return_never &= !returns_never; - all_overloads_return_never &= returns_never; - any_overload_is_generic |= overload.return_ty.has_typevar(db); - } - - if no_overloads_return_never && !any_overload_is_generic && !is_await { - Truthiness::AlwaysFalse - } else if all_overloads_return_never { + PredicateNode::ReturnsNever(call_expr) => { + let call_expr_ty = infer_expression_type(db, call_expr, TypeContext::default()); + if call_expr_ty.is_equivalent_to(db, Type::Never) { Truthiness::AlwaysTrue } else { - let call_expr_ty = infer_expression_type(db, call_expr, TypeContext::default()); - if call_expr_ty.is_equivalent_to(db, Type::Never) { - Truthiness::AlwaysTrue - } else { - Truthiness::AlwaysFalse - } + Truthiness::AlwaysFalse } .negate_if(!predicate.is_positive) } diff --git a/crates/ty_python_semantic/src/types/narrow.rs b/crates/ty_python_semantic/src/types/narrow.rs index ea783b83363da..f621b3b59bcb6 100644 --- a/crates/ty_python_semantic/src/types/narrow.rs +++ b/crates/ty_python_semantic/src/types/narrow.rs @@ -3,8 +3,7 @@ use crate::semantic_index::expression::Expression; use crate::semantic_index::place::{PlaceExpr, PlaceTable, PlaceTableBuilder, ScopedPlaceId}; use crate::semantic_index::place_table; use crate::semantic_index::predicate::{ - CallableAndCallExpr, ClassPatternKind, PatternPredicate, PatternPredicateKind, Predicate, - PredicateNode, + ClassPatternKind, PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, }; use crate::semantic_index::scope::ScopeId; use crate::subscript::PyIndex; @@ -762,9 +761,7 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> { match self.predicate { PredicateNode::Expression(expression) => expression.scope(self.db), PredicateNode::Pattern(pattern) => pattern.scope(self.db), - PredicateNode::ReturnsNever(CallableAndCallExpr { callable, .. }) => { - callable.scope(self.db) - } + PredicateNode::ReturnsNever(call_expr) => call_expr.scope(self.db), PredicateNode::StarImportPlaceholder(definition) => definition.scope(self.db), } }