[ty] Fix various overflows with recursive type aliases#24683
Open
charliermarsh wants to merge 6 commits intomainfrom
Open
[ty] Fix various overflows with recursive type aliases#24683charliermarsh wants to merge 6 commits intomainfrom
charliermarsh wants to merge 6 commits intomainfrom
Conversation
charliermarsh
added
bug
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 89.36%. The percentage of expected errors that received a diagnostic held steady at 85.49%. The number of fully passing files held steady at 88/134. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdowntrio
flake8
sphinx
prefect
|
|
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
5 times, most recently
from
755d446 to
2721ed3
Compare
charliermarsh
commented
Apr 21, 2026
charliermarsh
commented
Apr 21, 2026
Member
Author
|
We may just want to remove the |
Member
Author
|
A subset of these are fixed by #24773, so we can wait for that to land first. |
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
7 times, most recently
from
8d8d48e to
136a861
Compare
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
Member
Author
|
Much of the process here was: fixing the initial issues, then prompting Codex to find another, similar panic that wasn't yet solved, then fixing that, and repeating until there were no more findings. Then, I did an additional few passes to clean up. |
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
2 times, most recently
from
5e6c3ab to
af9aba1
Compare
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
6 times, most recently
from
cca5571 to
0681c47
Compare
charliermarsh
requested review from
BurntSushi,
Gankra and
MichaReiser
as code owners
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
3 times, most recently
from
f2a1995 to
7719f4c
Compare
charliermarsh
force-pushed
the
charlie/recursive-type-aliases
branch
from
7719f4c to
cac8ea5
Compare
Contributor
|
@mtshiba I know you did some work in this area recently. If you find the time (and if you are interested), would you mind taking a look at the general approach here to see if it aligns with what you had planned? I know that we took a few stabs at trying to fix panics/overflows in this area, so I'm wondering why previous attempts have fallen short and if we're really on the right trajectory here, or if we maybe need an entirely different approach? @charliermarsh Sorry that I didn't find the time to take a closer look so far |
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
2 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
This PR fixes a variety of panics that arise from recursive PEP 695 type aliases. (Codex estimates that it fixes 40-50 user-visible panics stemming from 8-12 underlying codepaths.)
Previously, a bunch of type operations handled aliases by directly expanding the alias RHS with
alias.value_type(db)and then continuing the same operation on the expanded type. That works for acyclic aliases, but recursive aliases can re-enter the same alias expansion before the outer operation completes, leading to stack overflows or cycle non-convergence.For example:
The core fix is to make alias visitation a guarded operation. Alias expansion now goes through
Type::visit_type_alias_value(...), backed by an active-alias stack keyed by the alias identity. When an operation reaches the same active alias again, it stops expanding that recursive edge and returns an operation-specific conservative fallback (likeUnknown, or a default). We have to take this into account anywhere that we try to access an alias value.I initially implemented this by threading a visitor through all type operations (first commit). That worked, but it required adding
_implvariants all across the API, and remembering to thread the visitor through in the correct places. So I did a second pass to use a thread-local guard, which significantly reduces the diff at the cost of some implicit magic. I am fine either approach, but figured this was much easier to review.Closes astral-sh/ty#3195.
Closes astral-sh/ty#3196.