[ty] add support for intersections on the "actual" side of generic inference#23728
Merged
oconnor663 merged 3 commits intomainfrom Mar 10, 2026
Merged
[ty] add support for intersections on the "actual" side of generic inference#23728oconnor663 merged 3 commits intomainfrom
oconnor663 merged 3 commits intomainfrom
Conversation
Contributor
Author
|
@dcreager are you the best reviewer for this? This is my first time in this area of the code so please don't hesitate to ELI5. |
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 85.05%. The percentage of expected errors that received a diagnostic held steady at 78.05%. The number of fully passing files held steady at 63/132. |
oconnor663
commented
Mar 5, 2026
Comment on lines
+917
to
+933
| In the formal parameter position: | ||
|
|
||
| ```py | ||
| from typing import TypeVar, Iterable | ||
| from ty_extensions import Intersection | ||
|
|
||
| T = TypeVar("T") | ||
|
|
||
| class Foo: ... | ||
|
|
||
| def foo(x: Intersection[Iterable[T], Foo]) -> T: | ||
| return next(iter(x)) | ||
|
|
||
| class Bar(list[int], Foo): ... | ||
|
|
||
| reveal_type(foo(Bar())) # revealed: int | ||
| ``` |
Contributor
Author
There was a problem hiding this comment.
This test case is for the preexisting (Type::Intersection(formal), _) branch, which as far as I can tell wasn't tested before. The case below is for the new branch.
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53b61ecd90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2231
to
+2232
| for positive in actual_intersection.iter_positive(self.db) { | ||
| self.infer_map_impl(constraints, formal, positive, polarity, f, seen)?; |
There was a problem hiding this comment.
Skip failing positives when inferring actual intersections
This loop propagates Err from the first positive element that fails inference, but for (_, Type::Intersection(..)) a single failing positive does not imply the whole intersection is incompatible with formal. In bounded/constrained TypeVar cases, one positive can raise MismatchedBound/MismatchedConstraint while another positive would infer a valid mapping, so returning early here can produce false specialization errors (for example, intersected sequence element types where only one branch satisfies the TypeVar bound). This branch should mirror the union logic by trying all positives and only surfacing an error if none produce usable inference.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
This comment looks accurate to me? It seems like we are missing test coverage for an intersection with multiple positive elements, at least one of which matches the formal parameter and some of which do not.
I suspect we may already have handling for a similar case, where we handle matching a union formal parameter? That's another case where it's OK for any element of the union to match, rather than requiring that all do. (In some sense, that case is a closer parallel to this one than the intersection-formal-parameter case.)
Member
There was a problem hiding this comment.
Yes this is correct. This is showing the duality of the lhs vs rhs of an intersection. This should only return an error if every positive element raises an error. In the constraint set world, this clause will use when_any, like is done here in has_relation_to (note that reversed parameter order).
The intersection formal case, otoh, would use when_all, like here (and note that the has_relation_to version also checks the negative elements for disjointness).
I suspect we may already have handling for a similar case, where we handle matching a union formal parameter?
We do have that, just above! It has a special case return for when precisely one element matches, and implements the "only error if no element succeeds" logic. We can probably extract that into a helper method or closure and call it in both places.
Contributor
Author
There was a problem hiding this comment.
Ok I've pushed a new version based on our conversation this morning/afternoon.
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
36 | 16 | 8 |
unresolved-attribute |
30 | 0 | 2 |
invalid-assignment |
8 | 0 | 7 |
no-matching-overload |
12 | 2 | 0 |
invalid-return-type |
8 | 0 | 4 |
unsupported-operator |
10 | 0 | 0 |
unused-type-ignore-comment |
1 | 6 | 0 |
type-assertion-failure |
2 | 4 | 0 |
division-by-zero |
0 | 2 | 0 |
not-iterable |
2 | 0 | 0 |
call-non-callable |
1 | 0 | 0 |
| Total | 110 | 30 | 21 |
Member
I am! Taking a look now |
53b61ec to
d549858
Compare
oconnor663
commented
Mar 7, 2026
|
|
||
| # An intersection where both positive elements satisfy the bound. | ||
| def _(x: Intersection[Sequence[Sub1], Sequence[Sub2]]) -> None: | ||
| reveal_type(first(x)) # revealed: Sub1 | Sub2 |
Contributor
Author
There was a problem hiding this comment.
@dcreager we talked about the union Sub1 | Sub2 being the correct answer here, but now that I look at it it feels wrong. Should this be have a TODO: should be Sub1 & Sub2 or similar? Or have I maybe done something wrong in the implementation?
Member
There was a problem hiding this comment.
No I think this is right. As you mention above, "inference can fail for some elements but succeed for others". If this were Sub1 & Sub2, that would mean that inference had to succeed for all elements.
dcreager
approved these changes
Mar 9, 2026
|
|
||
| # An intersection where both positive elements satisfy the bound. | ||
| def _(x: Intersection[Sequence[Sub1], Sequence[Sub2]]) -> None: | ||
| reveal_type(first(x)) # revealed: Sub1 | Sub2 |
Member
There was a problem hiding this comment.
No I think this is right. As you mention above, "inference can fail for some elements but succeed for others". If this were Sub1 & Sub2, that would mean that inference had to succeed for all elements.
Comment on lines
+981
to
+982
| def _(x: Intersection[Sequence[Sub1], Sequence[Unrelated1]]) -> None: | ||
| reveal_type(first(x)) # revealed: Sub1 |
Member
There was a problem hiding this comment.
This a good example for understanding why union is correct. Unrelated1 does not satisfy T's upper bound, so it's not a valid solution. I think you technically end up with T = Never as the only solution. But then you want union so that simplifies T = Sub1 | Never = Sub1. If you used intersection you'd get T = Sub1 & Never = Never.
Comment on lines
+988
to
+991
| # An intersection with two positive elements, neither of which satisfies the bound. In this case, | ||
| # only the error related to the first element is reported. | ||
| def _(x: Intersection[Sequence[Unrelated1], Sequence[Unrelated2]]) -> None: | ||
| # error: [invalid-argument-type] "Argument to function `first` is incorrect: Argument type `Unrelated1` does not satisfy upper bound `Base` of type variable `T`" |
Member
There was a problem hiding this comment.
This does describe the current behavior, but we might consider a TODO to show all of the failures.
Comment on lines
+2239
to
+2240
| // If the typevar in this case is upper-bounded or constrained, it's sufficient for | ||
| // one intersection element to satisfy the constraint. They don't all have to. |
Member
There was a problem hiding this comment.
This is true, though upper bounds / constraints are not the only way that the intersection element might not satisfy the constraint. So I'd remove that bit from the comment:
// It's sufficient for only one intersection element to satisfy the constraint.
// They don't all have to.(You'll probably have to reflow that to pass lint)
Contributor
Author
|
Looking at the ecosystem results for from ty_extensions import Unknown
def f(s: str) -> None: ...
def test(maybe_list: Unknown) -> None:
if isinstance(maybe_list, list):
# both: revealed: Unknown & Top[list[Unknown]]
reveal_type(maybe_list)
# main: revealed: Unknown
# pr: revealed: Iterator[object]
reveal_type(iter(maybe_list))
for elem in iter(maybe_list):
# main: revealed: Unknown
# pr: revealed: object
reveal_type(elem)
# main: no error
# pr: error: [invalid-argument-type]
f(elem) |
Contributor
|
I would expect (I haven't looked at why this is happening in the implementation or how to fix it.) |
Contributor
Author
|
@carljm I think the cause there might be described by this TODO in |
Contributor
|
Looks likely! Seems like that could just be filed as a separate issue, if it doesn't look widespread. |
d549858 to
537a67d
Compare
Contributor
Author
|
Another set of ecosystem hits seems to boil down to the following effect: from typing import overload
@overload
def f[T](_: list[T], /) -> list[T]: ...
@overload
def f(_: list[str], /) -> list[str]: ...
def f(_): ...
# main: revealed: list[Unknown]
# pr: revealed: list[str | Unknown]
reveal_type(f([]))I think this change to how overloads are handled is responsible for the new What's happening here is that we intersect the overloaded argument types, and that somehow (I haven't followed all the steps) interacts with the changes in intersection handling in this PR. It's pretty surprising to me that we'd do an intersection here, but there's a TODO that suggests this might be a known but kind of unintended behavior: Claude breaks down the sequence of events like this:
Without this PR, step 3 there infers nothing, and |
Contributor
Author
|
astral-sh/ty#3001 aims to address the TODO I linked above. |
This was referenced Mar 9, 2026
Contributor
Author
Opened astral-sh/ty#3009. |
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
4 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 astral-sh/ty#2091.