Subtyping and inference of user defined variadic types #16076
Merged
Conversation
This file contains 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
|
Oh btw @mehdigmira could you please test this PR on your code? |
This comment has been minimized.
This comment has been minimized.
|
@ilevkivskyi still seems to fail with this code from __future__ import annotations
from typing import Generic, Tuple
from typing_extensions import TypeVarTuple, Unpack
Ts = TypeVarTuple("Ts")
class Array(Generic[Unpack[Ts]]):
def _close(self) -> None:
...
def close(self) -> None:
self._close()This generates an eroor mypy version is mypy 1.7.0+dev.38cd66c1f4662aba609bcd49a4c3f9f4707f4ce1 |
This comment has been minimized.
This comment has been minimized.
|
@mehdigmira Could you please try again? |
This comment has been minimized.
This comment has been minimized.
|
Found an other issue, but not sure if it is related to this from __future__ import annotations
from typing_extensions import TypeVarTuple
from typing import Any, Unpack
from collections.abc import Sequence
Ts = TypeVarTuple("Ts")
def f(data: Sequence[tuple[Unpack[Ts]]]) -> list[Any]:
return [d[0] for d in data] # error: List comprehension has incompatible type List[Unpack[Ts]]; expected List[Any] |
|
Also mypy seems to crash when typevartuple is used within an overload from __future__ import annotations
from typing_extensions import TypeVarTuple
from typing import Any, Generic, Unpack, overload
Ts = TypeVarTuple("Ts")
class Array(Generic[Unpack[Ts]]):
...
class A:
@overload
def f(self, x: tuple[Unpack[Ts]]) -> Array[Unpack[Ts]]:
...
@overload
def f(self, x: Any) -> Any:
...
def f(self, x: Any) -> Any:
... |
|
OK, thanks! The one with indexing is a known issue and it will go into one of the next PRs, the crash with overloads I will fix in this PR. |
This comment has been minimized.
This comment has been minimized.
|
@ilevkivskyi This is much better now, it seems to handle many code patterns. Noticed 2 issues though:
Sorry If the tracebacks are hard to debug, I'm not able to reproduce this with a self contained example yet. I'll see If I can find one. These crashes do not occur in no-incremental mode.
from typing import Any, Generic, TypeVarTuple, Unpack, TypeVar, overload, reveal_type
_Ts = TypeVarTuple("_Ts")
_T = TypeVar("_T")
_T2 = TypeVar("_T2")
class Container(Generic[_T]):
...
class Array(Generic[Unpack[_Ts]]):
...
@overload
def build(entity: Container[_T], /) -> Array[_T]:
...
@overload
def build(entity: Container[_T], entity2: Container[_T2], /) -> Array[_T, _T2]:
...
@overload
def build(*entities: Container[Any]) -> Array[Unpack[tuple[Any, ...]]]:
...
def build(*entities: Container[Any]) -> Array[Unpack[tuple[Any, ...]]]:
...
def test(a: Container[Any], b: Container[int], c: Container[str]):
reveal_type(build(a, b))Revealed type is |
|
@mehdigmira Thanks! I will take a look at these. About the another overload problem: although mypy indeed has special logic for |
|
@mehdigmira I was not able to reproduce the incremental crashes (I reproduced one that is quite similar to one you posted), but after staring for an hour at type deserialization code, I found two obvious bugs. I guess this may fix your problems with incremental mode. (The other two things, tuple indexing and overload inference, will go into separate PRs) |
|
@ilevkivskyi I still have the crash, but seems to only happen when using mypy python api. You can reproduce with the following git clone -b mypy-crash git@github.com:mehdigmira/sqlalchemy.git
cd sqlalchemy
python -m venv .venv/deser
source .venv/deser/bin/activate
python3 -m pip install -U git+https://github.com/ilevkivskyi/mypy.git@variadic-subtyping#egg=mypy
python test_mypy_crash.py |
|
An other crash (this time with a self contained example) from typing import Any, Generic, TypeVar, Unpack, overload
from typing_extensions import TypeVarTuple
_T = TypeVar("_T")
_Ts = TypeVarTuple("_Ts")
class Row(tuple[Unpack[_Ts]]):
...
class Query(Generic[_T]):
...
class RowReturningQuery(Query[Row[Unpack[_Ts]]]):
...
@overload
def func(self, __ent0: _T, /, *entities: Any) -> RowReturningQuery[_T, Unpack[tuple[Any, ...]]]:
...
@overload
def func(*entities: Any) -> Query[Any]:
...
def func(*entities: Any) -> Query[Any]:
...Traceback is I think this is introduced by the latest commit |
|
@mehdigmira Thanks! I think I fixed both now. FWIW one of the crashes falls into what should be in the next PRs, but since it is a crash and the fix is couple lines I added it here. |
This comment has been minimized.
This comment has been minimized.
|
@ilevkivskyi Yes, all good now ! |
There was a problem hiding this comment.
Thanks mehdigmira for testing and ilevkivskyi for fixing all the things! Should we add the test case from #16076 (comment) ?
This comment has been minimized.
This comment has been minimized.
JukkaL
approved these changes
Sep 13, 2023
| t1: Tuple[int, int] | ||
| t2: Tuple[int, Unpack[Tuple[int, ...]]] | ||
| t3: Tuple[Unpack[Tuple[int, ...]], int] | ||
| t4: Tuple[int, Unpack[Tuple[int, ...]], int] |
There was a problem hiding this comment.
What about Tuple[int, int, Unpack[Tuple[int, ...]] (and similarly for a suffix)?
| @@ -811,11 +846,82 @@ def visit_overloaded(self, t: Overloaded) -> ProperType: | |||
| return meet_types(t, call) | |||
| return meet_types(t.fallback, s) | |||
|
|
|||
| def meet_tuples(self, s: TupleType, t: TupleType) -> list[Type] | None: | |||
There was a problem hiding this comment.
What about adding more unit tests for meet_tuples? This is pretty tricky but should be easy to unit test. This can happen in a follow-up PR.
| @@ -440,6 +472,112 @@ def visit_overloaded(self, t: Overloaded) -> ProperType: | |||
| return join_types(t, call) | |||
| return join_types(t.fallback, s) | |||
|
|
|||
| def join_tuples(self, s: TupleType, t: TupleType) -> list[Type] | None: | |||
There was a problem hiding this comment.
What about adding more unit tests for join_tuples? This is pretty tricky but should be easy to unit test. This can happen in a follow-up PR.
There was a problem hiding this comment.
This was a good idea after all. Tests caught a silly bug (missing early return in one place), and also gave me an idea of how to handle some more edge cases essentially for free.
|
|
||
| Ts = TypeVarTuple("Ts") | ||
| class B(Generic[Unpack[Ts]]): ... | ||
| class C1(B[Unpack[Tuple[Any, ...]]]): ... |
There was a problem hiding this comment.
What about testing other cases of inheritance, such as using B[Unpack[Ts]] in the subclass, or a fixed number of non-Any types as type args (B[int, str]), etc.? This can happen in a follow-up PR (or you can create an issue about this).
There was a problem hiding this comment.
There are some existing tests, I just added few specific test cases for situations that required special-casing (and were not covered by existing test cases). I will check what important cases are currently missing and will add some.
OK, I can wait some time. I can work on adding (some of) the tests in the meantime. |
jhance
approved these changes
Sep 13, 2023
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
hauntsaninja
added a commit
to hauntsaninja/mypy
that referenced
this pull request
Sep 14, 2023
Follow up to python#16073 and python#16076 Fix needed for https://github.com/python/mypy/pull/16053/files#r1316481395 I add test cases that would have caught my previous incorrect PR. I add an explicit case for the new desirable behaviour we see with zip.
hauntsaninja
added a commit
that referenced
this pull request
Sep 14, 2023
Follow up to #16073 and #16076 Fix needed for https://github.com/python/mypy/pull/16053/files#r1316481395 I add test cases that would have caught my previous incorrect PR. I add an explicit case for the new desirable behaviour we see with zip.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The second part of support for user defined variadic types comes as a single PR, it was hard to split into smaller parts. This part covers subtyping and inference (and relies on the first part: type analysis, normalization, and expansion, concluded by #15991). Note btw that the third (and last) part that covers actually using all the stuff in
checkexpr.pywill likely come as several smaller PRs.Some comments on this PR:
*tuple[X, ...]were missing everywhere (even couple cases in the code I touched before). I added all that were either simple or important. We can handle more if users will ask, since it is quite tricky.variadic_tuple_subtype().Foo[*tuple[int, ...]]was considered a subtype ofFoo[int, int]. I think this is wrong. I didn't find where this is required in the PEP (see one case below however), and mypy currently considerstuple[int, ...]not a subtype oftuple[int, int](vice versa are subtypes), and similarly(*args: int)vs(x: int, y: int)for callables. Because of the logic I described in the first comment, the same logic now uniformly applies to instances as well.Foo[*tuple[Any, ...]](equivalent to bareFoo), and I agree we should do this. I added a minimal special case for this. Note we also do this for callables as well (*args: Anyis very different from*args: object). And I think we should special casetuple[Any, ...] <: tuple[int, int]as well. In the future we can even extend the special casing totuple[int, *tuple[Any, ...], int]in the spirit of Lenient handling of trivial Callable suffixes #15913TypeVarTupleTypeto betuple[object, ...]. I think it can never beobject(and this simplifies some subtyping corner cases).