meet.py should consider all Tuple[t1, t2, ...] as subtypes of plain tuple #2465
Conversation
|
Thanks for the fix! I'm going to postpone merging this until after 0.4.6 as we are very close to the release. |
|
@JukkaL Just pinging this. (I am working on other thing that would be easier with this fix merged.) |
| @@ -243,6 +245,10 @@ def visit_tuple_type(self, t: TupleType) -> Type: | |||
| items.append(self.meet(t.items[i], self.s.items[i])) | |||
| # TODO: What if the fallbacks are different? | |||
| return TupleType(items, t.fallback) | |||
| # Special case: all Tuple[t1, t2, ...] are subtypes of plain tuple. | |||
| elif (isinstance(self.s, Instance) and self.s.type.fullname() == 'builtins.tuple' | |||
There was a problem hiding this comment.
We could also handle cases like the meet of Tuple[int, int] and Tuple[int, ...]. However, as this is slightly trickier, it's okay to add a comment here about this and create a follow-up issue. Which would you prefer?
Implementing the general case shouldn't be hard, though it's not urgent really. The meet of arbitrary Tuple[t1, <...>, tn] and Tuple[s, ...] (where <...> would not be a Python ellipsis but a mathematical one) would be Tuple[meet(t1, s), ..., meet(tn, s)], I think. So the meet of types Tuple[X, Y] and Tuple[Z, ...] would be Tuple[meet(X, Z), meet(Y, Z)], etc.
There was a problem hiding this comment.
@JukkaL OK, I made this apply for all homogeneous tuples (instead of special-casing bare tuple) as you propose.
|
|
||
| [builtins fixtures/tuple.pyi] | ||
| [out] | ||
| main: note: In function "f": |
There was a problem hiding this comment.
You can use # flags: --hide-error-context to hide the function names. Then you can use # E: ... for multiple functions in a single test case.
There was a problem hiding this comment.
@JukkaL Thanks for useful tip!
| reveal_type(v) # E: Revealed type is 'builtins.int' | ||
| [builtins fixtures/tuple.pyi] | ||
| [out] | ||
| main: note: In function "tuplify": |
There was a problem hiding this comment.
Again this would be clearer if you'd use # flags: --hide-error-context. It should perhaps be on by default for test cases, but we have a huge number of test cases that would be affected so it's unclear whether it would be worth the code churn.
|
Looks good to merge, other than the above issues. |
|
@JukkaL Thank you for review! I implemented your comments and added another small test that triggers the code for |
|
Looks good now. Thanks! |
Fixes #1691
Fixes #2456
This is a simple-minded fix. In the future we could add more rules for tuples in
meet.py(for example for homogeneous tuples, a tuple and an iterable etc).Tests are based on (extended) examples discussed in issues.