[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute #20333

carljm · 2025-09-10T15:00:14Z

Summary

Use Type::Divergent to avoid "too many iterations" panic on an infinitely-nested tuple in an implicit instance attribute.

The regression here is from checking all tuple elements to see if they contain a Divergent type. It's 5% on one project, 1% on another, and zero on the rest. I spent some time looking into eliminating this regression by tracking a flag on inference results to note if they could possibly contain any Divergent type, but this doesn't really work -- there are too many different ways a type containing a Divergent type could enter an inference result. Still thinking about whether there are other ways to reduce this. One option is if we see certain kinds of non-atomic types that are commonly expensive to check for Divergent, we could make has_divergent_type a Salsa query on those types.

Test Plan

Added mdtest.

Co-authored-by: Alex Waygood Alex.Waygood@Gmail.com

github-actions · 2025-09-10T15:02:29Z

Diagnostic diff on typing conformance tests

Changes were detected when running ty on typing conformance tests

--- old-output.txt	2025-09-11 13:14:44.923387518 +0000
+++ new-output.txt	2025-09-11 13:14:47.942419689 +0000
@@ -1,6 +1,6 @@
 WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(b816)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(12a7a)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/3713cd7/src/function/execute.rs:213:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(b816)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/3713cd7/src/function/execute.rs:213:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(12a7a)): execute: too many cycle iterations`
 _directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
 _directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
 _directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__`

github-actions · 2025-09-10T15:05:30Z

`mypy_primer` results

No ecosystem changes detected ✅

Memory usage changes were detected when running on open source projects

flake8 (https://github.com/pycqa/flake8)
-     struct fields = ~4MB
+     struct fields = ~5MB

codspeed-hq · 2025-09-10T15:21:42Z

CodSpeed WallTime Performance Report

Merging #20333 will degrade performances by 5.12%

_{Comparing cjm/tuplerec (1f8073d) with main (59c8fda)}

Summary

❌ 1 regression
✅ 7 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`medium[static-frame]`	7.8 s	8.2 s	-5.12%

github-actions · 2025-09-10T15:26:49Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

dcreager

I like this new Divergent type

…plicit attribute

AlexWaygood

All tests seem to pass if I apply this refactor to your branch (which gets rid of quite a few .expect() calls!) -- is there anything conceptually wrong with this?

diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs
index df251700f8..1d53611bf6 100644
--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -387,19 +387,19 @@ impl<'db> InferenceRegion<'db> {
     }
 }
 
-#[derive(Debug, Clone, Copy, Eq, PartialEq, get_size2::GetSize)]
-enum CycleRecovery {
+#[derive(Debug, Clone, Copy, Eq, PartialEq, get_size2::GetSize, salsa::Update)]
+enum CycleRecovery<'db> {
     /// An initial-value for fixpoint iteration; all types are `Type::Never`.
     Initial,
     /// A divergence-fallback value for fixpoint iteration; all types are `Divergent`.
-    Divergent,
+    Divergent(ScopeId<'db>),
 }
 
-impl CycleRecovery {
-    fn merge(self, other: Option<CycleRecovery>) -> CycleRecovery {
+impl<'db> CycleRecovery<'db> {
+    fn merge(self, other: Option<CycleRecovery<'db>>) -> Self {
         if let Some(other) = other {
             match (self, other) {
-                (Self::Divergent, _) | (_, Self::Divergent) => Self::Divergent,
+                (Self::Divergent(scope), _) | (_, Self::Divergent(scope)) => Self::Divergent(scope),
                 _ => Self::Initial,
             }
         } else {
@@ -407,10 +407,10 @@ impl CycleRecovery {
         }
     }
 
-    fn fallback_type<'db>(self, scope_fn: impl FnOnce() -> ScopeId<'db>) -> Type<'db> {
+    fn fallback_type(self) -> Type<'db> {
         match self {
             Self::Initial => Type::Never,
-            Self::Divergent => Type::divergent(scope_fn()),
+            Self::Divergent(scope) => Type::divergent(scope),
         }
     }
 }
@@ -428,13 +428,10 @@ pub(crate) struct ScopeInference<'db> {
 #[derive(Debug, Eq, PartialEq, get_size2::GetSize, salsa::Update, Default)]
 struct ScopeInferenceExtra<'db> {
     /// Is this a cycle-recovery inference result, and if so, what kind?
-    cycle_recovery: Option<CycleRecovery>,
+    cycle_recovery: Option<CycleRecovery<'db>>,
 
     /// The diagnostics for this region.
     diagnostics: TypeCheckDiagnostics,
-
-    /// The scope for which this is an inference result, if we are a divergent cycle recovery.
-    scope: Option<ScopeId<'db>>,
 }
 
 impl<'db> ScopeInference<'db> {
@@ -470,12 +467,9 @@ impl<'db> ScopeInference<'db> {
     }
 
     fn fallback_type(&self) -> Option<Type<'db>> {
-        self.extra.as_ref().and_then(|extra| {
-            extra.cycle_recovery.map(|recovery| {
-                recovery
-                    .fallback_type(|| extra.scope.expect("Divergent inference should have scope"))
-            })
-        })
+        self.extra
+            .as_ref()
+            .and_then(|extra| extra.cycle_recovery.map(CycleRecovery::fallback_type))
     }
 }
 
@@ -509,7 +503,7 @@ pub(crate) struct DefinitionInference<'db> {
 #[derive(Debug, Eq, PartialEq, get_size2::GetSize, salsa::Update, Default)]
 struct DefinitionInferenceExtra<'db> {
     /// Is this a cycle-recovery inference result, and if so, what kind?
-    cycle_recovery: Option<CycleRecovery>,
+    cycle_recovery: Option<CycleRecovery<'db>>,
 
     /// The definitions that are deferred.
     deferred: Box<[Definition<'db>]>,
@@ -519,9 +513,6 @@ struct DefinitionInferenceExtra<'db> {
 
     /// For function definitions, the undecorated type of the function.
     undecorated_type: Option<Type<'db>>,
-
-    /// The scope this region is part of, if we are a divergent cycle recovery.
-    scope: Option<ScopeId<'db>>,
 }
 
 impl<'db> DefinitionInference<'db> {
@@ -605,12 +596,9 @@ impl<'db> DefinitionInference<'db> {
     }
 
     fn fallback_type(&self) -> Option<Type<'db>> {
-        self.extra.as_ref().and_then(|extra| {
-            extra.cycle_recovery.map(|recovery| {
-                recovery
-                    .fallback_type(|| extra.scope.expect("Divergent inference should have scope"))
-            })
-        })
+        self.extra
+            .as_ref()
+            .and_then(|extra| extra.cycle_recovery.map(CycleRecovery::fallback_type))
     }
 
     pub(crate) fn undecorated_type(&self) -> Option<Type<'db>> {
@@ -643,13 +631,10 @@ struct ExpressionInferenceExtra<'db> {
     diagnostics: TypeCheckDiagnostics,
 
     /// Is this a cycle recovery inference result, and if so, what kind?
-    cycle_recovery: Option<CycleRecovery>,
+    cycle_recovery: Option<CycleRecovery<'db>>,
 
     /// `true` if all places in this expression are definitely bound
     all_definitely_bound: bool,
-
-    /// The scope this region is part of (if we are a Divergent cycle recovery.)
-    scope: Option<ScopeId<'db>>,
 }
 
 impl<'db> ExpressionInference<'db> {
@@ -672,9 +657,8 @@ impl<'db> ExpressionInference<'db> {
         let _ = scope;
         Self {
             extra: Some(Box::new(ExpressionInferenceExtra {
-                cycle_recovery: Some(CycleRecovery::Divergent),
+                cycle_recovery: Some(CycleRecovery::Divergent(scope)),
                 all_definitely_bound: true,
-                scope: Some(scope),
                 ..ExpressionInferenceExtra::default()
             })),
             expressions: FxHashMap::default(),
@@ -700,12 +684,9 @@ impl<'db> ExpressionInference<'db> {
     }
 
     fn fallback_type(&self) -> Option<Type<'db>> {
-        self.extra.as_ref().and_then(|extra| {
-            extra.cycle_recovery.map(|recovery| {
-                recovery
-                    .fallback_type(|| extra.scope.expect("Divergent inference should have scope"))
-            })
-        })
+        self.extra
+            .as_ref()
+            .and_then(|extra| extra.cycle_recovery.map(CycleRecovery::fallback_type))
     }
 
     /// Returns true if all places in this expression are definitely bound.
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index b1e6fd3b4f..1264aa21fd 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -252,7 +252,7 @@ pub(super) struct TypeInferenceBuilder<'db, 'ast> {
     undecorated_type: Option<Type<'db>>,
 
     /// Did we merge in a sub-region with a cycle-recovery fallback, and if so, what kind?
-    cycle_recovery: Option<CycleRecovery>,
+    cycle_recovery: Option<CycleRecovery<'db>>,
 
     /// `true` if all places in this expression are definitely bound
     all_definitely_bound: bool,
@@ -293,7 +293,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
         }
     }
 
-    fn extend_cycle_recovery(&mut self, other_recovery: Option<CycleRecovery>) {
+    fn extend_cycle_recovery(&mut self, other_recovery: Option<CycleRecovery<'db>>) {
         match &mut self.cycle_recovery {
             Some(recovery) => *recovery = recovery.merge(other_recovery),
             recovery @ None => *recovery = other_recovery,
@@ -301,8 +301,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
     }
 
     fn fallback_type(&self) -> Option<Type<'db>> {
-        self.cycle_recovery
-            .map(|recovery| recovery.fallback_type(|| self.scope))
+        self.cycle_recovery.map(CycleRecovery::fallback_type)
     }
 
     fn extend_definition(&mut self, inference: &DefinitionInference<'db>) {
@@ -8866,7 +8865,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                     diagnostics,
                     cycle_recovery,
                     all_definitely_bound,
-                    scope: Some(scope),
                 })
             });
 
@@ -8915,7 +8913,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                 deferred: deferred.into_boxed_slice(),
                 diagnostics,
                 undecorated_type,
-                scope: Some(scope),
             })
         });
 
@@ -8981,7 +8978,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
             Box::new(ScopeInferenceExtra {
                 cycle_recovery,
                 diagnostics,
-                scope: Some(scope),
             })
         });

carljm · 2025-09-11T13:06:45Z

All tests seem to pass if I apply this refactor to your branch... is there anything conceptually wrong with this?

There sort of is; I'd actually started the same refactor and then stopped when I reached the "conceptually wrong" thing, which is the implementation of CycleRecovery::merge. When we merge two CycleRecovery::Divergent together, with potentially different scopes, that should never change the scope of whichever one we're mutating. It makes CycleRecovery::merge not a symmetric operation.

That said, I think your implementation actually achieves the necessary invariant in practice, but it's a bit subtle, and due solely to the way the clauses of the match pattern union are ordered, and the fact that TypeInferenceBuilder::extend_cycle_recovery always calls CycleRecovery::merge on self.cycle_recovery, not other.cycle_recovery. So it may still make sense to apply this change, just with a bit more explicit commentary about that being an important invariant.

* origin/main: [ty] Minor fixes to `Protocol` tests (#20347) [ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute (#20333) [ty] Require that implementors of `Constraints` also implement `Debug` (#20348)

## Summary Use `Type::Divergent` to short-circuit diverging types in type expressions. This avoids panicking in a wide variety of cases of recursive type expressions. Avoids many panics (but not yet all -- I'll be tracking down the rest) from astral-sh/ty#256 by falling back to Divergent. For many of these recursive type aliases, we'd like to support them properly (i.e. really understand the recursive nature of the type, not just fall back to Divergent) but that will be future work. This switches `Type::has_divergent_type` from using `any_over_type` to a custom set of visit methods, because `any_over_type` visits more than we need to visit, and exercises some lazy attributes of type, causing significantly more work. This change means this diff doesn't regress perf; it even reclaims some of the perf regression from #20333. ## Test Plan Added mdtest for recursive type alias that panics on main. Verified that we can now type-check `packaging` (and projects depending on it) without panic; this will allow moving a number of mypy-primer projects from `bad.txt` to `good.txt` in a subsequent PR.

carljm added the ty Multi-file analysis & type inference label Sep 10, 2025

carljm mentioned this pull request Sep 10, 2025

Allow fallback to take more than one iteration to converge salsa-rs/salsa#991

Merged

carljm force-pushed the cjm/tuplerec branch from 5a78c17 to f3709a2 Compare September 10, 2025 15:07

carljm mentioned this pull request Sep 10, 2025

[ty] introduce DivergentType #20312

Merged

carljm force-pushed the cjm/tuplerec branch 3 times, most recently from c543681 to 41af298 Compare September 10, 2025 15:56

carljm changed the title ~~Cjm/tuplerec~~ [ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute Sep 10, 2025

carljm force-pushed the cjm/tuplerec branch 2 times, most recently from ff51ab6 to 8975fae Compare September 10, 2025 22:30

carljm marked this pull request as ready for review September 10, 2025 23:37

carljm requested review from AlexWaygood, dcreager and sharkdp as code owners September 10, 2025 23:37

dcreager approved these changes Sep 11, 2025

View reviewed changes

[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple im…

cfa6734

…plicit attribute

carljm force-pushed the cjm/tuplerec branch from 8975fae to cfa6734 Compare September 11, 2025 04:54

improved approach

2689da0

AlexWaygood reviewed Sep 11, 2025

View reviewed changes

keep the scope inside CycleRecovery::Divergent

1f8073d

carljm merged commit ffd4340 into main Sep 11, 2025
38 checks passed

carljm deleted the cjm/tuplerec branch September 11, 2025 13:51

carljm mentioned this pull request Sep 11, 2025

[ty] infer function's return type #17371

Open

1 task

carljm mentioned this pull request Sep 12, 2025

[ty] no more diverging query cycles in type expressions #20359

Merged

AlexWaygood mentioned this pull request Oct 1, 2025

Panic when building divergent tuple type for implicit instance attributes astral-sh/ty#837

Closed

mtshiba mentioned this pull request Oct 20, 2025

[ty] Fall back to Divergent for deeply nested specializations #20988

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute #20333

[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute #20333

Uh oh!

carljm commented Sep 10, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

codspeed-hq bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

dcreager left a comment

Uh oh!

AlexWaygood left a comment

Uh oh!

carljm commented Sep 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute #20333

[ty] use Type::Divergent to avoid panic in infinitely-nested-tuple implicit attribute #20333

Uh oh!

Conversation

carljm commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Diagnostic diff on typing conformance tests

Uh oh!

github-actions bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

codspeed-hq bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed WallTime Performance Report

Merging #20333 will degrade performances by 5.12%

Summary

Benchmarks breakdown

Uh oh!

github-actions bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

dcreager left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

carljm commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carljm commented Sep 10, 2025 •

edited

Loading

github-actions bot commented Sep 10, 2025 •

edited

Loading

github-actions bot commented Sep 10, 2025 •

edited

Loading

`mypy_primer` results

codspeed-hq bot commented Sep 10, 2025 •

edited

Loading

github-actions bot commented Sep 10, 2025 •

edited

Loading

`ruff-ecosystem` results

carljm commented Sep 11, 2025 •

edited

Loading