[ty] bidirectional type inference using function return type annotations #20528
+442
−58
Conversation
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
…e of expressions in return statements
mtshiba
changed the title
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
sharkdp
pushed a commit
that referenced
this pull request
Sep 23, 2025
…ith `is_disjoint_from` (#20538) ## Summary I found this bug while working on #20528. The minimum reproducible code is: ```python from __future__ import annotations from typing import NamedTuple from ty_extensions import is_disjoint_from, static_assert class Path(NamedTuple): prev: Path | None key: str static_assert(not is_disjoint_from(Path, Path)) ``` A stack overflow occurs when a nominal instance type inherits from `NamedTuple` and is defined recursively. This PR fixes this bug. ## Test Plan mdtest updated
mtshiba
force-pushed
the
bidi-return-type
branch
from
d899f83 to
2d882b3
Compare
mtshiba
commented
Sep 24, 2025
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
12 | 15 | 16 |
invalid-return-type |
4 | 11 | 26 |
missing-typed-dict-key |
11 | 0 | 0 |
invalid-key |
3 | 0 | 0 |
no-matching-overload |
3 | 0 | 0 |
possibly-missing-attribute |
1 | 0 | 0 |
unused-ignore-comment |
1 | 0 | 0 |
| Total | 35 | 26 | 42 |
mtshiba
force-pushed
the
bidi-return-type
branch
from
2d882b3 to
ef75f0a
Compare
AlexWaygood
reviewed
Sep 24, 2025
There was a problem hiding this comment.
This looks pretty good to me! Some small comments below. @dcreager and/or @ibraheemdev will probably be better reviewers, though; I haven't done much work on our bidirectional inference yet
dcreager
reviewed
Sep 25, 2025
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-Authored-By: Ibraheem Ahmed <ibraheem@ibraheem.ca>
mtshiba
force-pushed
the
bidi-return-type
branch
from
87be495 to
1b6a505
Compare
Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
mtshiba
force-pushed
the
bidi-return-type
branch
from
25c8ffc to
0249ba2
Compare
…iteral::raw_signature`
CodSpeed Performance ReportMerging #20528 will not alter performanceComparing Summary
Footnotes
|
…verloadLiteral::raw_signature`" This reverts commit 9507d72.
ibraheemdev
reviewed
Oct 6, 2025
Comment on lines
327
to
329
| def _(x: int | None): | ||
| # TODO: should be `int` | ||
| reveal_type(union_param(x)) # revealed: Unknown |
There was a problem hiding this comment.
This is revealed as int | None in main. The change is due to the addition of a branch that does nothing when matching in SpecializationBuilder::infer.
As already mentioned, specialization between unions requires special handling, and falling back to the (Type::Union(_), _) => ... branch could result in incorrect specialization. The default mapping T = Unknown is suboptimal but not incorrect, and the mapping T = int | None is also suboptimal, so I'm not sure whether this should be considered a regression.
Specialization between unions will be a future work.
There was a problem hiding this comment.
This is timely — I just added that same branch with an initial "better than nothing" heuristic in #20749.
There was a problem hiding this comment.
(Note that the heuristic in 20749 will not handle @mtshiba's example, since it only engages when formal has precisely one top-level typevar, and actual has none.)
There was a problem hiding this comment.
I think that's fine, this should be eventually fixed by @dcreager's constraint solver work, so I wouldn't worry too much about it.
There was a problem hiding this comment.
OK, I merged the latest changes and made a minor fix to prevent incorrect specialization.
|
I think I addressed all review comments. codspeed reports a slight performance regression, but for the same reason as #20635, I believe it is difficult to avoid this. The ecosystem reports are generally good, some false positive errors are due to TODOs in |
ibraheemdev
approved these changes
Oct 7, 2025
|
I think this might need minor updating now that I merged #20806 (since it should fix a test TODO added in that PR)? @ibraheemdev feel free to merge this if it looks ready to you. |
|
This looks great, thanks. |
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.
Summary
Implements bidirectional type inference using function return type annotations.
This PR was originally proposed to solve astral-sh/ty#1167, but this does not fully resolve it on its own.
Additionally, I believe we need to allow dataclasses to generate their own
__new__methods, use constructor return types for inference, and a mechanism to discard type narrowing like& ~AlwaysFalsyif necessary (at a more general level than this PR).Test Plan
mdtest/bidirectional.mdis added.