[ty] improve complex TDD-based narrowing#23201
Draft
mtshiba wants to merge 27 commits intoastral-sh:mainfrom
Draft
[ty] improve complex TDD-based narrowing#23201mtshiba wants to merge 27 commits intoastral-sh:mainfrom
mtshiba wants to merge 27 commits intoastral-sh:mainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownflake8
sphinx
trio
prefect
|
Merging this PR will degrade performance by 7.33%
Performance Changes
Comparing 
Footnotes
|
mtshiba
force-pushed
the
improve-23109
branch
from
d247e5c to
7182d3c
Compare
|
mtshiba
force-pushed
the
improve-23109
branch
from
13ce7d1 to
eb8977b
Compare
Collaborator
Author
|
My original idea didn't work, but instead I was able to suppress the exponential blowup by implementing a cache for |
mtshiba
force-pushed
the
improve-23109
branch
from
2503e23 to
6ded4be
Compare
mtshiba
force-pushed
the
improve-23109
branch
from
bda7146 to
61ea03f
Compare
mtshiba
force-pushed
the
improve-23109
branch
from
61ea03f to
0e2ca45
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unresolved-attribute |
3 | 4 | 60 |
invalid-key |
0 | 51 | 0 |
invalid-argument-type |
1 | 11 | 38 |
possibly-unresolved-reference |
0 | 21 | 0 |
invalid-assignment |
0 | 0 | 11 |
unresolved-reference |
0 | 10 | 0 |
not-subscriptable |
8 | 0 | 0 |
unsupported-operator |
1 | 0 | 6 |
type-assertion-failure |
1 | 1 | 3 |
invalid-return-type |
1 | 0 | 3 |
no-matching-overload |
3 | 1 | 0 |
unused-type-ignore-comment |
3 | 1 | 0 |
not-iterable |
0 | 0 | 3 |
index-out-of-bounds |
0 | 1 | 0 |
possibly-missing-attribute |
0 | 1 | 0 |
| Total | 21 | 102 | 124 |
mtshiba
changed the title
mtshiba
commented
Feb 12, 2026
| .. | ||
| }) => Some(!resolve_to_literal(operand)?), | ||
| _ => None, | ||
| #[derive(Copy, Clone)] |
Collaborator
Author
There was a problem hiding this comment.
In the end, this didn't seem to do much to improve the performance degradation, but I don't think there's anything bad that can come from leaving it.
| self.record_narrowing_constraint(negated_predicate); | ||
| self.record_reachability_constraint(negated_predicate); | ||
| let predicate_id = self.record_narrowing_constraint(negated_predicate); | ||
| self.record_reachability_constraint_id(predicate_id); |
Collaborator
Author
There was a problem hiding this comment.
Because the reachability constraint and the narrowing constraint point to the same predicate, we can reuse the same ID to save memory. That was the original intention, but this change was actually essential for this PR (it was essential for the Bindings::merge change https://github.com/astral-sh/ruff/pull/23201/changes#r2800954123).
Without this identification, it seems that narrowing would be mistakenly determined to be a non no-op when it should be no-op.
| place: ScopedPlaceId, | ||
| ) -> Type<'db> { | ||
| self.narrow_by_constraint_inner(db, predicates, id, base_ty, place, None) | ||
| let mut memo = FxHashMap::default(); |
Collaborator
Author
There was a problem hiding this comment.
Caching helps mitigate the exponential blowup reported in 5fd9a9c.
| { | ||
| ScopedNarrowingConstraint::ALWAYS_TRUE | ||
| } else { | ||
| // A branch contributes narrowing only when it is reachable. |
Collaborator
Author
There was a problem hiding this comment.
This technique is possible because narrowing and reachability are now managed in the same data structure. The narrowing of a/b is triggered when the reachability of a/b is ALWAYS_TRUE.
That is, gate each narrowing constraint with each reachability constraint with and.
Collaborator
Author
|
I've tried various things, but this is the limit. I'm out of ideas for optimization, and the codspeed profiling results show no abnormal hot spots. So I'll mark this PR as ready for review. |
mtshiba
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
carljm
requested changes
Feb 12, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Thank you for working on this!
Contributor
|
@sharkdp I'm wondering if you can review this, since you reviewed the previous diff? I'm realizing that it will take me some time to fully wrap my head around what exactly the previous diff did, and it seems necessary to understand that in order to review this diff well. |
Contributor
|
Sorry I didn't get to this today, it look very interesting. I'll review it next week. |
Collaborator
Author
|
Hmm, merging the main now results in one test failing. |
AlexWaygood
reviewed
Feb 13, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
(only a partial review, of the ConstExpr logic)
Comment on lines
+977
to
+988
| ast::Operator::FloorDiv => { | ||
| if right == 0 { | ||
| return None; | ||
| } | ||
| left.div_euclid(right) | ||
| } | ||
| ast::Operator::Mod => { | ||
| if right == 0 { | ||
| return None; | ||
| } | ||
| left.rem_euclid(right) | ||
| } |
Member
There was a problem hiding this comment.
Are you sure these give the correct result? This is different to the logic we have on the type-inference side in
ruff/crates/ty_python_semantic/src/types/infer/builder.rs
Lines 13977 to 13999 in c29a837
And tests start failing if I apply this patch to the main branch:
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index 32e9c34bc2..bbf2f58474 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -13974,17 +13974,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
Some(KnownClass::Float.to_instance(self.db()))
}
- (Type::IntLiteral(n), Type::IntLiteral(m), ast::Operator::FloorDiv) => Some({
- let mut q = n.checked_div(m);
- let r = n.checked_rem(m);
- // Division works differently in Python than in Rust. If the result is negative and
- // there is a remainder, the division rounds down (instead of towards zero):
- if n.is_negative() != m.is_negative() && r.unwrap_or(0) != 0 {
- q = q.map(|q| q - 1);
- }
- q.map(Type::IntLiteral)
- .unwrap_or_else(|| KnownClass::Int.to_instance(self.db()))
- }),
+ (Type::IntLiteral(n), Type::IntLiteral(m), ast::Operator::FloorDiv) => {
+ Some(Type::IntLiteral(n.div_euclid(m)))
+ }
(Type::IntLiteral(n), Type::IntLiteral(m), ast::Operator::Mod) => Some({
let mut r = n.checked_rem(m);
Comment on lines
+992
to
+999
| ast::Operator::LShift => { | ||
| let shift = u32::try_from(right).ok()?; | ||
| left.checked_shl(shift)? | ||
| } | ||
| ast::Operator::RShift => { | ||
| let shift = u32::try_from(right).ok()?; | ||
| left.checked_shr(shift)? | ||
| } |
Member
There was a problem hiding this comment.
we actually don't have these ones implemented on the type-inference side at all right now; we just fallback to the dunder methods on int. But that doesn't need to stop us from implementing it here.
Comment on lines
+1042
to
+1054
| let eq = |left: ConstExpr, right: ConstExpr| match ( | ||
| left.as_int(), | ||
| right.as_int(), | ||
| ) { | ||
| (Some(left), Some(right)) => Some(left == right), | ||
| _ => match (left, right) { | ||
| (ConstExpr::None, ConstExpr::None) | ||
| | (ConstExpr::Ellipsis, ConstExpr::Ellipsis) => Some(true), | ||
| (ConstExpr::None | ConstExpr::Ellipsis, _) | ||
| | (_, ConstExpr::None | ConstExpr::Ellipsis) => Some(false), | ||
| _ => None, | ||
| }, | ||
| }; |
Member
There was a problem hiding this comment.
Suggested change
| let eq = |left: ConstExpr, right: ConstExpr| match ( | |
| left.as_int(), | |
| right.as_int(), | |
| ) { | |
| (Some(left), Some(right)) => Some(left == right), | |
| _ => match (left, right) { | |
| (ConstExpr::None, ConstExpr::None) | |
| | (ConstExpr::Ellipsis, ConstExpr::Ellipsis) => Some(true), | |
| (ConstExpr::None | ConstExpr::Ellipsis, _) | |
| | (_, ConstExpr::None | ConstExpr::Ellipsis) => Some(false), | |
| _ => None, | |
| }, | |
| }; | |
| let eq = |left: ConstExpr, right: ConstExpr| match (left, right) { | |
| (ConstExpr::Int(left), ConstExpr::Int(right)) => { | |
| Some(left == right) | |
| } | |
| (ConstExpr::None, ConstExpr::None) | |
| | (ConstExpr::Ellipsis, ConstExpr::Ellipsis) => Some(true), | |
| (ConstExpr::None | ConstExpr::Ellipsis, _) | |
| | (_, ConstExpr::None | ConstExpr::Ellipsis) => Some(false), | |
| _ => None, | |
| }; |
Comment on lines
+948
to
+950
| ast::Expr::Name(ast::ExprName { id, .. }) if id == "TYPE_CHECKING" => { | ||
| Some(ConstExpr::Bool(true)) | ||
| } |
Member
There was a problem hiding this comment.
it might be better to use this helper function to identify TYPE_CHECKING constants, so that we have consistent handling everywhere. For example, in other places we treat any attribute expressions where all components are names and the last component is TYPE_CHECKING the same as symbols called TYPE_CHECKING:
ruff/crates/ty_python_semantic/src/semantic_index/builder.rs
Lines 3532 to 3549 in c29a837
mtshiba
commented
Feb 15, 2026
| // path(s) are reachable, rather than walking both branches. | ||
| // `ReturnsNever` always evaluates to `AlwaysTrue` or `AlwaysFalse`, | ||
| // never `Ambiguous`. | ||
| if matches!(predicate.node, PredicateNode::ReturnsNever(_)) { |
Collaborator
Author
There was a problem hiding this comment.
This special-casing appears to have been introduced for performance reasons, but is no longer necessary due to the addition of the generalized guard below.
This reverts commit c2fe06e.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An unsolved issue in #23109 is that the following narrowing doesn't work properly:
This was attempted to be fixed in ada1084, but was abandoned due to the exponential blowup (5fd9a9c).
I've come up with a different approach to this, so I'll try it out to see if it works.
Test Plan
mdtest updated