[ty] Avoid stack overflow with recursive typevar#23652
Merged
charliermarsh merged 6 commits intomainfrom Mar 4, 2026
Merged
Conversation
charliermarsh
added
bug
Typing conformance resultsNo changes detected ✅ |
charliermarsh
force-pushed
the
charlie/self-recurse
branch
from
d971798 to
1c93d1b
Compare
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
AlexWaygood
reviewed
Mar 2, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
This fixes the issue, but it feels like it's patching a symptom rather than addressing the underlying cause. TypeVarInstance::default_type is already a wrapper around the low-level field _default_type that's stored directly on the Salsa struct. It feels like we should be adding cycle handling to TypeVarInstance::default_type, since apparently any access of t.default_type(db) is enough to trigger infinite recursion currently, given a sufficiently pathological typevar?
You can take a look at the BinaryComparisonVisitor, and the way we thread that through infer_binary_type_comparison, for an example of how we might add cycle detection to lazy_default
Member
Author
|
It's proving to be harder to do that here because |
Member
|
can you add an inner function to |
charliermarsh
force-pushed
the
charlie/self-recurse
branch
from
e04f35d to
96e2835
Compare
AlexWaygood
reviewed
Mar 3, 2026
Comment on lines
+7241
to
+7246
| { | ||
| let mut seen_typevars = seen_typevars.borrow_mut(); | ||
| if !seen_typevars.insert(typevar) { | ||
| return false; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
can be a bit simpler
Suggested change
| { | |
| let mut seen_typevars = seen_typevars.borrow_mut(); | |
| if !seen_typevars.insert(typevar) { | |
| return false; | |
| } | |
| } | |
| if !seen_typevars.borrow_mut().insert(typevar) { | |
| return false; | |
| } |
(I'm still working through the rest of the PR, just posting this now so I don't forget about it...)
Comment on lines
+7270
to
+7275
| { | ||
| let mut seen_type_aliases = seen_type_aliases.borrow_mut(); | ||
| if !seen_type_aliases.insert(type_alias) { | ||
| return false; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| { | |
| let mut seen_type_aliases = seen_type_aliases.borrow_mut(); | |
| if !seen_type_aliases.insert(type_alias) { | |
| return false; | |
| } | |
| } | |
| if !seen_type_aliases.borrow_mut().insert(type_alias) { | |
| return false; | |
| } |
Member
|
I think you can improve readability for the fn type_is_self_referential(
self,
db: &'db dyn Db,
ty: Type<'db>,
visitor: &TypeVarDefaultVisitor<'db>,
) -> bool {
#[derive(Copy, Clone)]
struct State<'db, 'a> {
db: &'db dyn Db,
visitor: &'a TypeVarDefaultVisitor<'db>,
seen_typevars: &'a RefCell<FxHashSet<TypeVarInstance<'db>>>,
seen_type_aliases: &'a RefCell<FxHashSet<TypeAliasType<'db>>>,
}
fn typevar_default_is_self_referential<'db>(
state: State<'db, '_>,
typevar: TypeVarInstance<'db>,
self_identity: TypeVarIdentity<'db>,
) -> bool {
if typevar.identity(state.db) == self_identity {
return true;
}
if !state.seen_typevars.borrow_mut().insert(typevar) {
return false;
}
typevar
.default_type_impl(state.db, state.visitor)
.is_some_and(|default_ty| {
type_is_self_referential_impl(state, default_ty, self_identity)
})
}
fn type_alias_is_self_referential<'db>(
state: State<'db, '_>,
type_alias: TypeAliasType<'db>,
self_identity: TypeVarIdentity<'db>,
) -> bool {
if !state.seen_type_aliases.borrow_mut().insert(type_alias) {
return false;
}
type_is_self_referential_impl(state, type_alias.raw_value_type(state.db), self_identity)
}
fn type_is_self_referential_impl<'db>(
state: State<'db, '_>,
ty: Type<'db>,
self_identity: TypeVarIdentity<'db>,
) -> bool {
any_over_type(state.db, ty, false, |inner_ty| match inner_ty {
Type::TypeVar(bound_typevar) => typevar_default_is_self_referential(
state,
bound_typevar.typevar(state.db),
self_identity,
),
Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) => {
typevar_default_is_self_referential(state, typevar, self_identity)
}
Type::TypeAlias(alias) => {
type_alias_is_self_referential(state, alias, self_identity)
}
Type::KnownInstance(KnownInstanceType::TypeAliasType(alias)) => {
type_alias_is_self_referential(state, alias, self_identity)
}
_ => false,
})
}
let seen_typevars = RefCell::new(FxHashSet::default());
let seen_type_aliases = RefCell::new(FxHashSet::default());
let state = State {
db,
visitor,
seen_typevars: &seen_typevars,
seen_type_aliases: &seen_type_aliases,
};
type_is_self_referential_impl(state, ty, self.identity(db))
} |
AlexWaygood
approved these changes
Mar 3, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Modulo my suggestions regarding readability, this LGTM, thanks!
Comment on lines
-540
to
-543
| if typevar.default_type(db).is_some() { | ||
| typevar_with_defaults += 1; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
What's the motivation for this change?
Member
Author
There was a problem hiding this comment.
I think this was a mistake, but re-adding it surfaced another panic, and I had to add cycle handling for bound_typevar_default_type...
Member
Author
There was a problem hiding this comment.
(I think this specific one at least could be avoided if I just did a syntactic query for the default type presence.)
charliermarsh
force-pushed
the
charlie/self-recurse
branch
from
9469e1f to
b4b57eb
Compare
carljm
added a commit
that referenced
this pull request
Mar 16, 2026
* main: [ty] Split up `types/class.rs` (#23714) [ty] Rework module resolution to be breadth-first instead of depth-first (#22449) [ty] Move tests and type-alias-related code out of `types.rs` (#23711) [ty] Move TypeVar-related code to a `types::typevar` submodule (#23710) Fail CI on new linter ecosystem panics (#23597) [ty] Fix handling of non-Python text documents [ty] Move `CallableType`, and related methods/types, to a new `types::callable` submodule (#23707) [ty] Avoid stack overflow with recursive typevar (#23652) [ty] Add a diagnostic for an unused awaitable (#23650) [ty] Fix union `*args` binding for optional positional parameters (#23124) [`refurb`] Fix `FURB101` and `FURB103` false positives when I/O variable is used later (#23542) [ty] Fix type checking for multi-member enums within in a function block (#23683) [ty] Improve folding for decorators (#23543) [`airflow`] Extract common utilities for use in new rules (#23630)
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
3 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
Use a syntactic check rather than eagerly resolving the type.
Closes astral-sh/ty#2889