[ty] Normalize divergent types with oscillating cycle heads#22818
Closed
ibraheemdev wants to merge 2 commits intoibraheem/lambda-tcxfrom
Closed
[ty] Normalize divergent types with oscillating cycle heads#22818ibraheemdev wants to merge 2 commits intoibraheem/lambda-tcxfrom
ibraheemdev wants to merge 2 commits intoibraheem/lambda-tcxfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
ibraheemdev
force-pushed
the
ibraheem/lambda-tcx-cycle-panic
branch
from
8bbd73c to
e6e9f14
Compare
ibraheemdev
force-pushed
the
ibraheem/lambda-tcx
branch
from
a8bb4fc to
d050295
Compare
ibraheemdev
force-pushed
the
ibraheem/lambda-tcx-cycle-panic
branch
from
e6e9f14 to
5654adf
Compare
Member
Author
|
Looks like this causes some new panics... |
ibraheemdev
force-pushed
the
ibraheem/lambda-tcx-cycle-panic
branch
from
74fee24 to
f0b06c5
Compare
ibraheemdev
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Member
Author
Member
|
Can you say a bit more about your reasoning here for this fix? What was the issue and why do you think this is correct (even if you don't feel sure about it). What are the parts that you feel unsure about? |
AlexWaygood
reviewed
Jan 23, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
not an expert on our cycle normalization, but here's two nits 😄
| @@ -0,0 +1,25 @@ | |||
| # This test would previous panic with: `infer_definition_types(Id(1406)): execute: too many cycle iterations`. | |||
Member
There was a problem hiding this comment.
Suggested change
| # This test would previous panic with: `infer_definition_types(Id(1406)): execute: too many cycle iterations`. | |
| # This test would previously panic with: `infer_definition_types(Id(1406)): execute: too many cycle iterations`. |
| }) | ||
| } | ||
|
|
||
| pub(super) fn any_over_type_mut<'db>( |
Member
There was a problem hiding this comment.
can't we just keep it so that there's only one any_over_type* function? Your branch seems to compile fine with this patch applied; I don't think it's too burdensome to say that you always have to pass in a mutable reference to any_over_type:
Details
diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs
index 996270b511..e58c67800c 100644
--- a/crates/ty_python_semantic/src/types.rs
+++ b/crates/ty_python_semantic/src/types.rs
@@ -76,7 +76,7 @@ use crate::types::typed_dict::TypedDictField;
pub(crate) use crate::types::typed_dict::{TypedDictParams, TypedDictType, walk_typed_dict_type};
pub use crate::types::variance::TypeVarVariance;
use crate::types::variance::VarianceInferable;
-use crate::types::visitor::{any_over_type, any_over_type_mut};
+use crate::types::visitor::any_over_type;
use crate::unpack::EvaluationMode;
use crate::{Db, FxOrderSet, Program};
pub use class::KnownClass;
@@ -914,7 +914,7 @@ impl<'db> Type<'db> {
db,
*self,
false,
- &|ty| matches!(ty, Type::TypeVar(tv) if tv.typevar(db).is_self(db)),
+ &mut |ty| matches!(ty, Type::TypeVar(tv) if tv.typevar(db).is_self(db)),
)
}
@@ -960,7 +960,7 @@ impl<'db> Type<'db> {
_ => {
let has_divergent_type_in_cycle = |ty| {
- any_over_type(db, ty, false, &|ty| {
+ any_over_type(db, ty, false, &mut |ty| {
ty.as_divergent()
.is_some_and(|DivergentType { id }| cycle.head_ids().contains(&id))
})
@@ -979,7 +979,7 @@ impl<'db> Type<'db> {
//
// Without this, we may encounter oscillation as the cycle heads oscillate,
// despite the same divergent types being inferred in each iteration.
- any_over_type_mut(db, self, false, &mut |ty| {
+ any_over_type(db, self, false, &mut |ty| {
if let Some(DivergentType { id }) = ty.as_divergent() {
cycle_heads.insert(id);
}
@@ -1254,7 +1254,7 @@ impl<'db> Type<'db> {
}
pub(crate) fn has_typevar(self, db: &'db dyn Db) -> bool {
- any_over_type(db, self, false, &|ty| matches!(ty, Type::TypeVar(_)))
+ any_over_type(db, self, false, &mut |ty| matches!(ty, Type::TypeVar(_)))
}
pub(crate) const fn as_special_form(self) -> Option<SpecialFormType> {
@@ -6196,7 +6196,7 @@ impl<'db> Type<'db> {
}
});
- let is_recursive = any_over_type(db, alias.raw_value_type(db).expand_eagerly(db), false, &|ty| ty.is_divergent());
+ let is_recursive = any_over_type(db, alias.raw_value_type(db).expand_eagerly(db), false, &mut |ty| ty.is_divergent());
// If the type mapping does not result in any change to this (non-recursive) type alias, do not expand it.
//
@@ -8117,7 +8117,7 @@ impl<'db> TypeVarInstance<'db> {
fn type_is_self_referential(self, db: &'db dyn Db, ty: Type<'db>) -> bool {
let identity = self.identity(db);
- any_over_type(db, ty, false, &|ty| match ty {
+ any_over_type(db, ty, false, &mut |ty| match ty {
Type::TypeVar(bound_typevar) => identity == bound_typevar.typevar(db).identity(db),
Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) => {
identity == typevar.identity(db)
diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs
index d0cde1e3b1..d0a059b647 100644
--- a/crates/ty_python_semantic/src/types/constraints.rs
+++ b/crates/ty_python_semantic/src/types/constraints.rs
@@ -1995,7 +1995,7 @@ impl<'db> InteriorNode<'db> {
#[salsa::tracked(heap_size=ruff_memory_usage::heap_size)]
fn exists_one(self, db: &'db dyn Db, bound_typevar: BoundTypeVarIdentity<'db>) -> Node<'db> {
let mut path = self.path_assignments(db);
- let mentions_typevar = |ty: Type<'db>| match ty {
+ let mut mentions_typevar = |ty: Type<'db>| match ty {
Type::TypeVar(haystack) => haystack.identity(db) == bound_typevar,
_ => false,
};
@@ -2012,10 +2012,10 @@ impl<'db> InteriorNode<'db> {
if constraint.typevar(db).identity(db) == bound_typevar {
return true;
}
- if any_over_type(db, constraint.lower(db), false, &mentions_typevar) {
+ if any_over_type(db, constraint.lower(db), false, &mut mentions_typevar) {
return true;
}
- if any_over_type(db, constraint.upper(db), false, &mentions_typevar) {
+ if any_over_type(db, constraint.upper(db), false, &mut mentions_typevar) {
return true;
}
false
diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs
index 516bf600fc..d03729869b 100644
--- a/crates/ty_python_semantic/src/types/function.rs
+++ b/crates/ty_python_semantic/src/types/function.rs
@@ -1709,11 +1709,11 @@ impl KnownFunction {
let [Some(casted_type), Some(source_type)] = parameter_types else {
return;
};
- let contains_unknown_or_todo =
+ let mut contains_unknown_or_todo =
|ty| matches!(ty, Type::Dynamic(dynamic) if dynamic != DynamicType::Any);
if source_type.is_equivalent_to(db, *casted_type)
- && !any_over_type(db, *source_type, true, &contains_unknown_or_todo)
- && !any_over_type(db, *casted_type, true, &contains_unknown_or_todo)
+ && !any_over_type(db, *source_type, true, &mut contains_unknown_or_todo)
+ && !any_over_type(db, *casted_type, true, &mut contains_unknown_or_todo)
{
if let Some(builder) = context.report_lint(&REDUNDANT_CAST, call_expression) {
let source_display = source_type.display(db).to_string();
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index 1206e928e0..f04dac2a5e 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -15193,7 +15193,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
} else {
return Err(GenericContextError::InvalidArgument);
}
- } else if any_over_type(db, *typevar, true, &|ty| match ty {
+ } else if any_over_type(db, *typevar, true, &mut |ty| match ty {
Type::Dynamic(DynamicType::TodoUnpack | DynamicType::TodoStarredExpression) => true,
Type::NominalInstance(nominal) => {
nominal.has_known_class(db, KnownClass::TypeVarTuple)
diff --git a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
index 81edb423d5..4a164725e3 100644
--- a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
@@ -818,7 +818,7 @@ impl<'db> TypeInferenceBuilder<'db, '_> {
// instead of two. So until we properly support these, specialize all remaining type
// variables with a `@Todo` type (since we don't know which of the type arguments
// belongs to the remaining type variables).
- if any_over_type(self.db(), value_ty, true, &|ty| ty.is_divergent()) {
+ if any_over_type(self.db(), value_ty, true, &mut |ty| ty.is_divergent()) {
let value_ty = value_ty.apply_specialization(
db,
generic_context.specialize(
diff --git a/crates/ty_python_semantic/src/types/visitor.rs b/crates/ty_python_semantic/src/types/visitor.rs
index c7b87c0706..6c2bbc602e 100644
--- a/crates/ty_python_semantic/src/types/visitor.rs
+++ b/crates/ty_python_semantic/src/types/visitor.rs
@@ -296,17 +296,6 @@ impl<'db> TypeCollector<'db> {
/// (value of a type alias, attributes of a class-based protocol, bounds/constraints of a typevar)
/// are visited or not.
pub(super) fn any_over_type<'db>(
- db: &'db dyn Db,
- ty: Type<'db>,
- should_visit_lazy_type_attributes: bool,
- query: &dyn Fn(Type<'db>) -> bool,
-) -> bool {
- any_over_type_mut(db, ty, should_visit_lazy_type_attributes, &mut |ty| {
- query(ty)
- })
-}
-
-pub(super) fn any_over_type_mut<'db>(
db: &'db dyn Db,
ty: Type<'db>,
should_visit_lazy_type_attributes: bool,
~/dev/ruff (ibraheem/lambda-tcx-cycle-panic)⚡ % gd
diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs
index 996270b511..e58c67800c 100644
--- a/crates/ty_python_semantic/src/types.rs
+++ b/crates/ty_python_semantic/src/types.rs
@@ -76,7 +76,7 @@ use crate::types::typed_dict::TypedDictField;
pub(crate) use crate::types::typed_dict::{TypedDictParams, TypedDictType, walk_typed_dict_type};
pub use crate::types::variance::TypeVarVariance;
use crate::types::variance::VarianceInferable;
-use crate::types::visitor::{any_over_type, any_over_type_mut};
+use crate::types::visitor::any_over_type;
use crate::unpack::EvaluationMode;
use crate::{Db, FxOrderSet, Program};
pub use class::KnownClass;
@@ -914,7 +914,7 @@ impl<'db> Type<'db> {
db,
*self,
false,
- &|ty| matches!(ty, Type::TypeVar(tv) if tv.typevar(db).is_self(db)),
+ &mut |ty| matches!(ty, Type::TypeVar(tv) if tv.typevar(db).is_self(db)),
)
}
@@ -960,7 +960,7 @@ impl<'db> Type<'db> {
_ => {
let has_divergent_type_in_cycle = |ty| {
- any_over_type(db, ty, false, &|ty| {
+ any_over_type(db, ty, false, &mut |ty| {
ty.as_divergent()
.is_some_and(|DivergentType { id }| cycle.head_ids().contains(&id))
})
@@ -979,7 +979,7 @@ impl<'db> Type<'db> {
//
// Without this, we may encounter oscillation as the cycle heads oscillate,
// despite the same divergent types being inferred in each iteration.
- any_over_type_mut(db, self, false, &mut |ty| {
+ any_over_type(db, self, false, &mut |ty| {
if let Some(DivergentType { id }) = ty.as_divergent() {
cycle_heads.insert(id);
}
@@ -1254,7 +1254,7 @@ impl<'db> Type<'db> {
}
pub(crate) fn has_typevar(self, db: &'db dyn Db) -> bool {
- any_over_type(db, self, false, &|ty| matches!(ty, Type::TypeVar(_)))
+ any_over_type(db, self, false, &mut |ty| matches!(ty, Type::TypeVar(_)))
}
pub(crate) const fn as_special_form(self) -> Option<SpecialFormType> {
@@ -6196,7 +6196,7 @@ impl<'db> Type<'db> {
}
});
- let is_recursive = any_over_type(db, alias.raw_value_type(db).expand_eagerly(db), false, &|ty| ty.is_divergent());
+ let is_recursive = any_over_type(db, alias.raw_value_type(db).expand_eagerly(db), false, &mut |ty| ty.is_divergent());
// If the type mapping does not result in any change to this (non-recursive) type alias, do not expand it.
//
@@ -8117,7 +8117,7 @@ impl<'db> TypeVarInstance<'db> {
fn type_is_self_referential(self, db: &'db dyn Db, ty: Type<'db>) -> bool {
let identity = self.identity(db);
- any_over_type(db, ty, false, &|ty| match ty {
+ any_over_type(db, ty, false, &mut |ty| match ty {
Type::TypeVar(bound_typevar) => identity == bound_typevar.typevar(db).identity(db),
Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) => {
identity == typevar.identity(db)
diff --git a/crates/ty_python_semantic/src/types/constraints.rs b/crates/ty_python_semantic/src/types/constraints.rs
index d0cde1e3b1..d0a059b647 100644
--- a/crates/ty_python_semantic/src/types/constraints.rs
+++ b/crates/ty_python_semantic/src/types/constraints.rs
@@ -1995,7 +1995,7 @@ impl<'db> InteriorNode<'db> {
#[salsa::tracked(heap_size=ruff_memory_usage::heap_size)]
fn exists_one(self, db: &'db dyn Db, bound_typevar: BoundTypeVarIdentity<'db>) -> Node<'db> {
let mut path = self.path_assignments(db);
- let mentions_typevar = |ty: Type<'db>| match ty {
+ let mut mentions_typevar = |ty: Type<'db>| match ty {
Type::TypeVar(haystack) => haystack.identity(db) == bound_typevar,
_ => false,
};
@@ -2012,10 +2012,10 @@ impl<'db> InteriorNode<'db> {
if constraint.typevar(db).identity(db) == bound_typevar {
return true;
}
- if any_over_type(db, constraint.lower(db), false, &mentions_typevar) {
+ if any_over_type(db, constraint.lower(db), false, &mut mentions_typevar) {
return true;
}
- if any_over_type(db, constraint.upper(db), false, &mentions_typevar) {
+ if any_over_type(db, constraint.upper(db), false, &mut mentions_typevar) {
return true;
}
false
diff --git a/crates/ty_python_semantic/src/types/function.rs b/crates/ty_python_semantic/src/types/function.rs
index 516bf600fc..d03729869b 100644
--- a/crates/ty_python_semantic/src/types/function.rs
+++ b/crates/ty_python_semantic/src/types/function.rs
@@ -1709,11 +1709,11 @@ impl KnownFunction {
let [Some(casted_type), Some(source_type)] = parameter_types else {
return;
};
- let contains_unknown_or_todo =
+ let mut contains_unknown_or_todo =
|ty| matches!(ty, Type::Dynamic(dynamic) if dynamic != DynamicType::Any);
if source_type.is_equivalent_to(db, *casted_type)
- && !any_over_type(db, *source_type, true, &contains_unknown_or_todo)
- && !any_over_type(db, *casted_type, true, &contains_unknown_or_todo)
+ && !any_over_type(db, *source_type, true, &mut contains_unknown_or_todo)
+ && !any_over_type(db, *casted_type, true, &mut contains_unknown_or_todo)
{
if let Some(builder) = context.report_lint(&REDUNDANT_CAST, call_expression) {
let source_display = source_type.display(db).to_string();
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index 1206e928e0..f04dac2a5e 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -15193,7 +15193,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
} else {
return Err(GenericContextError::InvalidArgument);
}
- } else if any_over_type(db, *typevar, true, &|ty| match ty {
+ } else if any_over_type(db, *typevar, true, &mut |ty| match ty {
Type::Dynamic(DynamicType::TodoUnpack | DynamicType::TodoStarredExpression) => true,
Type::NominalInstance(nominal) => {
nominal.has_known_class(db, KnownClass::TypeVarTuple)
diff --git a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
index 81edb423d5..4a164725e3 100644
--- a/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder/type_expression.rs
@@ -818,7 +818,7 @@ impl<'db> TypeInferenceBuilder<'db, '_> {
// instead of two. So until we properly support these, specialize all remaining type
// variables with a `@Todo` type (since we don't know which of the type arguments
// belongs to the remaining type variables).
- if any_over_type(self.db(), value_ty, true, &|ty| ty.is_divergent()) {
+ if any_over_type(self.db(), value_ty, true, &mut |ty| ty.is_divergent()) {
let value_ty = value_ty.apply_specialization(
db,
generic_context.specialize(
diff --git a/crates/ty_python_semantic/src/types/visitor.rs b/crates/ty_python_semantic/src/types/visitor.rs
index c7b87c0706..6c2bbc602e 100644
--- a/crates/ty_python_semantic/src/types/visitor.rs
+++ b/crates/ty_python_semantic/src/types/visitor.rs
@@ -296,17 +296,6 @@ impl<'db> TypeCollector<'db> {
/// (value of a type alias, attributes of a class-based protocol, bounds/constraints of a typevar)
/// are visited or not.
pub(super) fn any_over_type<'db>(
- db: &'db dyn Db,
- ty: Type<'db>,
- should_visit_lazy_type_attributes: bool,
- query: &dyn Fn(Type<'db>) -> bool,
-) -> bool {
- any_over_type_mut(db, ty, should_visit_lazy_type_attributes, &mut |ty| {
- query(ty)
- })
-}
-
-pub(super) fn any_over_type_mut<'db>(
db: &'db dyn Db,
ty: Type<'db>,
should_visit_lazy_type_attributes: bool,
MichaReiser
reviewed
Jan 23, 2026
| // | ||
| // Without this, we may encounter oscillation as the cycle heads oscillate, | ||
| // despite the same divergent types being inferred in each iteration. | ||
| any_over_type_mut(db, self, false, &mut |ty| { |
Member
There was a problem hiding this comment.
Is this different from always replacing all Divergent types found in the type?
@mtshiba as our convergences expert: does this change make sense to you? Any idea what could be the reason for the too many iterations error on the lambda PR?
Member
Author
There was a problem hiding this comment.
I originally thought we could only replace Divergent types that occur in the current cycle heads or the previous type, but that seems to cause more panics. This should be equivalent to replacing all Divergent types.
Collaborator
There was a problem hiding this comment.
I'm looking into this...
This is just my impression at the moment, but I think there's another, more effective solution.
Member
Author
The comments explain my reasoning, the non-convergence on the lambda PR is caused by the cycle heads oscillating across iterations, while the inferred type remains identical, and so continuing to normalize divergent types from previous cycles allows us to converge. The reason I'm unsure is that I don't understand why the cycle heads are oscillating, and why we are able to infer a divergent type from a previous cycle head, but it seems plausible to me that that is possible and not the root of the issue. I also don't exactly understand why we limited normalization to the current cycle heads in the first place. |
Contributor
|
@mtshiba You mentioned you were looking into alternative approaches here; any findings? |
Contributor
It's because If cycle heads can oscillate it seems we need a broader concept of "the current cycle", though :/ |
Collaborator
|
Sorry for the delay.
|
Member
Author
|
Going to close this because it's unlikely to be merged anymore. |
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
5 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.
Fixes the panic in #22633, but I'm not entirely sure this is the correct fix.