Fix IndexError when unpacking empty tuple in type alias#20931
Fix IndexError when unpacking empty tuple in type alias#20931bxff wants to merge 2 commits intopython:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| type T[T, *Ts] = C[*Ts] | ||
|
|
||
| x: T[bool, Unpack[tuple[()]]] |
There was a problem hiding this comment.
Can you add a reveal type to make sure the type is being preserved?
There was a problem hiding this comment.
Done. reveal_type(x) -> __main__.C[Unpack[builtins.tuple]]
5f2afae to
debf2a5
Compare
This comment has been minimized.
This comment has been minimized.
test-data/unit/check-python312.test
Outdated
| type T[T, *Ts] = C[*Ts] | ||
|
|
||
| x: T[bool, Unpack[tuple[()]]] | ||
| reveal_type(x) # N: Revealed type is "__main__.C[Unpack[builtins.tuple]]" |
There was a problem hiding this comment.
Ok this is the wrong type.
There was a problem hiding this comment.
Agreed, but this is pre-existing, on unpatched master (without this fix), C[Unpack[tuple[()]]] already reveals identically to unparameterized C:
# on master, no fix applied
x: C[Unpack[tuple[()]]]
reveal_type(x) # C[Unpack[builtins.tuple[Any, ...]]]
y: C
reveal_type(y) # C[Unpack[builtins.tuple[Any, ...]]]
This PR only prevents the IndexError crash; the wrong type for empty tuple unpacks is a separate issue.
There was a problem hiding this comment.
I'm not sure that's identical behavior (note that the messages there have parameters provided to tuple). IIRC the issue you're showing is that we don't pass empty_index (or whatever the name is)... but in this case I don't think that's relevant, because if you unpack something empty that should be like x: T[bool] which currently has the reveal_type output.
Basically, I think what you show and this PR are different code paths (based on what I remember), so I think that behavior is irrelevant...? I'd be fine if this is a followup tbh, as long as you poke at it a bit and make sure it's not a 1 line change or something wrong with your crash fix.
There was a problem hiding this comment.
Ahh I see my mistake, you're right, I was comparing against the wrong code path. T[bool, Unpack[tuple[()]]] should just be T[bool] which gives C[()]. Fixed and updated the test.
There was a problem hiding this comment.
Gah, no, I misread what T was. My bad! Interesting that this also crashes:
class C[*Ts]:
pass
type T[T, *Ts] = C[*Ts]
x: T[bool]
reveal_type(x)So no Unpack[tuple[()]] necessary. Given that, I'm a bit surprised about the code modifications you made -- what's the tuple instance without .args coming from? (unless that's a different error!)
There was a problem hiding this comment.
Same error actually. T[bool] hits the same args[0] IndexError at expandtype.py:228. Tested both on 3.12 with the fix:
x: T[bool] -> C[()]
y: T[bool, Unpack[tuple[()]]] -> C[()]
I'll add T[bool] as a test case too.
There was a problem hiding this comment.
Hmm, so in args[0] it's synthesized based on the args from T, I assume? Interesting.... Sorry for all the comments! I'm not comfortable enough with this part of mypy to know if this is the right approach.
There was a problem hiding this comment.
No worries, appreciate the review! The T[bool] catch was really helpful.
8d4dd34 to
debf2a5
Compare
This comment has been minimized.
This comment has been minimized.
debf2a5 to
ab2bdd7
Compare
This comment has been minimized.
This comment has been minimized.
When expanding type arguments for a builtins.tuple instance, the normalization code accessed args[0] without checking if args was non-empty. This caused an IndexError when Unpack[tuple[()]] was used in a type alias, since the empty tuple expansion produces zero args. Fixes python#20913
dbc02ee to
4c1229e
Compare
for more information, see https://pre-commit.ci
A5rocks
left a comment
There was a problem hiding this comment.
I'm not completely comfortable with this, but I'm not familiar with this part of mypy so that doesn't mean anything :/
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Fixes #20913
mypy crashes with
IndexErrorwhen usingUnpack[tuple[()]](or just omitting the TypeVarTuple args entirely) in a PEP 695 type alias:The crash is in
visit_instance()— when expandingbuiltins.tupleargs that reduce to nothing,args[0]is accessed on an empty list.The fix handles this in two places:
visit_instance: whenbuiltins.tupleends up with empty args after expansion, returnTupleType([], ...)(the empty fixed-length tuple) instead of crashingexpand_type_list_with_unpack/expand_type_tuple_with_unpack: inlineUnpack[TupleType]results into their items, soUnpack[tuple[()]]correctly expands to nothingBoth
T[bool]andT[bool, Unpack[tuple[()]]]now correctly resolve toC[()].