[ty] Add support for subscripts on intersections#22654
Merged
charliermarsh merged 6 commits intomainfrom Jan 24, 2026
Merged
Conversation
charliermarsh
commented
Jan 17, 2026
Typing conformance resultsNo changes detected ✅ |
|
Member
Author
|
(Needs work.) |
charliermarsh
force-pushed
the
charlie/sub
branch
from
71ab6e8 to
741b4cd
Compare
Member
Author
|
I can't tell if the pip errors are expected. Here's a smaller example: TYPE_IMMEDIATE = 0
TYPE_ARRAY = 1
TYPE_MAP = 2
TYPE_RAW = 3
TYPE_BIN = 4
TYPE_EXT = 5
_NO_FORMAT_USED = ""
_MSGPACK_HEADERS = {
0xC4: (1, _NO_FORMAT_USED, TYPE_BIN),
0xC5: (2, ">H", TYPE_BIN),
0xC6: (4, ">I", TYPE_BIN),
0xC7: (2, "Bb", TYPE_EXT),
0xC8: (3, ">Hb", TYPE_EXT),
0xC9: (5, ">Ib", TYPE_EXT),
0xCA: (4, ">f"),
0xCB: (8, ">d"),
0xCC: (1, _NO_FORMAT_USED),
}
b: int
if 0xC4 <= b <= 0xC6:
size, fmt, typ = _MSGPACK_HEADERS[b]
elif 0xCA <= b <= 0xCB:
size, fmt, typ = _MSGPACK_HEADERS[b]Mypy infers |
Member
Author
|
I guess that actually matches our behavior on |
charliermarsh
force-pushed
the
charlie/sub
branch
from
741b4cd to
4f58a74
Compare
Member
Author
|
I think the |
Member
Author
|
I think the |
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
charliermarsh
changed the title
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
576 | 63 | 33 |
not-subscriptable |
83 | 0 | 0 |
invalid-assignment |
29 | 0 | 10 |
possibly-missing-attribute |
17 | 6 | 15 |
unused-ignore-comment |
2 | 26 | 0 |
unsupported-operator |
3 | 3 | 9 |
invalid-return-type |
5 | 1 | 8 |
unresolved-attribute |
10 | 0 | 0 |
call-non-callable |
9 | 0 | 0 |
no-matching-overload |
5 | 0 | 0 |
not-iterable |
4 | 0 | 0 |
type-assertion-failure |
0 | 1 | 1 |
invalid-await |
0 | 1 | 0 |
| Total | 743 | 101 | 76 |
Member
Author
|
I’d like to do more investigation of the ecosystem changes (unless this is obviously correct). |
Member
Author
|
I think the |
Member
Author
|
I think this spack diagnostic is correct, with: # fetch from first entry in urls to save time
if hasattr(self, "urls") and self.urls:
urls.append(self.urls[0]) |
Member
Author
|
It's a lot of new diagnostics... but my impression from checking around is that this is merely surfacing existing ty behavior in more contexts now. I don't see anything obviously wrong from my spot checks. |
Member
|
This leads to one new false positive on docstring-adder (currently full of intersection-subscript But I agree that this seems like a pre-existing issue with our narrowing behaviour, not an issue with this PR |
Member
Author
|
I will fix that in a separate PR. |
AlexWaygood
reviewed
Jan 18, 2026
charliermarsh
force-pushed
the
charlie/sub
branch
from
46cde76 to
fd3345a
Compare
Member
Author
|
Okay, I think this example demonstrates the difference... class Foo:
"""A class with __getitem__ that takes int keys"""
def __getitem__(self, key: int) -> str:
return ""
class Bar:
"""A class without __getitem__"""
pass
def test(x: Foo) -> None:
# Narrow x to Foo & Bar via isinstance
if isinstance(x, Bar):
# x is now Foo & Bar
# Foo has __getitem__(int) -> str
# Bar has no __getitem__
# Subscripting with str should fail on Foo (wrong type)
# and Bar doesn't support subscripting at all
result = x["hello"] # "hello" is str, Foo expects int
reveal_type(result)On On So on this branch, we emit an error per failing member. |
Member
Author
|
I fixed the "skip members that don't support Here's another difference (which seems right): class Foo:
def __getitem__(self, key: int) -> str:
return ""
class Bar:
def __getitem__(self, key: str) -> int:
return 0
def test(x: Foo) -> None:
if isinstance(x, Bar):
# x is Foo & Bar - both have __getitem__ but with different key types
result = x[[1, 2, 3]] # list is neither int nor strOn On |
charliermarsh
force-pushed
the
charlie/int-method
branch
6 times, most recently
from
4d6aa20 to
2ee10b1
Compare
charliermarsh
added a commit
that referenced
this pull request
Jan 23, 2026
## Summary A refactor in anticipation of #22654.
charliermarsh
force-pushed
the
charlie/sub
branch
3 times, most recently
from
fe1434f to
0ec165b
Compare
Contributor
|
It looks like a number of ecosystem projects are failing now? |
charliermarsh
force-pushed
the
charlie/sub
branch
from
0ec165b to
bdc3d6f
Compare
charliermarsh
force-pushed
the
charlie/sub
branch
from
bdc3d6f to
7100e76
Compare
AlexWaygood
reviewed
Jan 23, 2026
Member
|
This makes such a huge difference to the experience of using ty on the docstring-adder repo. There's no here's what some of the inlay hints look like on Screenshot
|
carljm
approved these changes
Jan 24, 2026
Comment on lines
+409
to
+411
| // All elements failed. Check if any element has the method available | ||
| // (even if the call failed). If so, filter out `NotSubscriptable` errors | ||
| // for elements that lack the method. |
Contributor
There was a problem hiding this comment.
In my intersection-calls PR, I have a similar situation with 1) "not callable" vs 2) "calling top callable" vs 3) "specific argument error", where I want to show only (3) if there are any, or only (2) if there are any. I use a concept of "error priority" there, where I calculate the max error priority and filter down to only errors with that priority.
But I think that makes more sense when there are >2 priority levels; with just two, I think it makes sense to do it a bit less generically, as you do here.
| .first() | ||
| .expect("`positive_elements_or_object` guarantees at least one element"); | ||
|
|
||
| let result_ty = first.result_type(); |
Contributor
There was a problem hiding this comment.
Why do we arbitrarily pick the first result type here, but build an intersection of result types in the any_has_method case above?
Relatedly, it seems kind of redundant that we even handle these separately. Couldn't we optionally filter the errors in the any_has_method case, but then just have one version of the "return the errors" code?
Comment on lines
+478
to
+487
| (Type::Intersection(intersection), _) => { | ||
| Some(map_intersection_subscript(db, intersection, |element| { | ||
| element.subscript(db, slice_ty, expr_context) | ||
| })) | ||
| } | ||
|
|
||
| (_, Type::Intersection(intersection)) => { | ||
| Some(map_intersection_subscript(db, intersection, |element| { | ||
| value_ty.subscript(db, element, expr_context) | ||
| })) |
Contributor
There was a problem hiding this comment.
Should we add a test where both value_ty and slice_ty are intersections, just to make sure the behavior falls out reasonably? (I expect it will.)
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
Closes astral-sh/ty#2072.