Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support for variadic calls and indexing #16131

Merged
merged 6 commits into from
Sep 28, 2023

Conversation

ilevkivskyi
Copy link
Member

This improves support for two features that were supported but only partially: variadic calls, and variadic indexing. Some notes:

  • I did not add dedicated support for slicing of tuples with homogeneous variadic items (except for cases covered by TypeVarTuple support, i.e. those not involving splitting of variadic item). This is tricky and it is not clear what cases people actually want. I left a TODO for this.
  • I prohibit multiple variadic items in a call expression. Technically, we can support some situations involving these, but this is tricky, and prohibiting this would be in the "spirit" of the PEP, IMO.
  • I may have still missed some cases for the calls, since there are so many options. If you have ideas for additional test cases, please let me know.
  • It was necessary to fix overload ambiguity logic to make some tests pass. This goes beyond TypeVarTuple support, but I think this is a correct change (note btw @hauntsaninja this fixes one of tests you added).

@mehdigmira could you also please test this PR on your code base (and play with it in general)?

@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

It looks like the new errors are real. Because mypy previously handled zip() etc incorrectly (because of *args fallback overload), people didn't get errors where they used say lis[Any] instead of tuple[Any, ...].

@mehdigmira
Copy link

Works good overall 👍 . I think there are still some issues to be uncovered, I haven't tested everything yet. But I can provide the following issues already.

@ilevkivskyi Not sure all of the issues I'll report are within the scope of this MR. But I think It's good to mention them since you say "It was necessary to fix overload ambiguity logic to make some tests pass. This goes beyond TypeVarTuple support". I let you filter out issues that you think are irrelevant here :)

  • mypy reports an error because it thinks that overload 1 covers overload 2, which it does not IMO
from __future__ import annotations

from typing import Any, TypeVar, overload

_T = TypeVar("_T")


class A:
    @overload
    def __call__(self, *c: Any, return_value: _T, **kwargs: Any) -> _T:
        ...

    @overload
    def __call__(  # test.py:14: error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same or broader  [misc]
        self, *c: Any, **kwargs: Any
    ) -> Any:
        ...

    def __call__(self, *c: Any, **kwargs: Any) -> Any:
        ...
  • Tuple index out of range is reported here.
from __future__ import annotations

from typing import TypeVarTuple, Unpack

_Ts = TypeVarTuple("_Ts")


def f(tuple_data: tuple[Unpack[_Ts]]) -> None:
    # test.py:13: error: Tuple index out of range  [misc]
    # test.py:13: note: Variadic tuple can have length 0
    tuple_data[0]

I don't think it should since it's equivalent to this, where no error is reported

from __future__ import annotations

from typing import Any, TypeVar

_T = TypeVar("_T", bound=tuple[Any, ...])


def f(tuple_data: _T) -> None:
    tuple_data[0]

@mehdigmira
Copy link

mypy infers tuple[Any, ...] here, I'd expect it to infer tuple[Any]

from __future__ import annotations

from typing import Any, TypeVar, overload

_T0 = TypeVar("_T0")


@overload
def returning(__ent0: type[_T0], /) -> tuple[_T0]:
    ...


@overload
def returning(*cols: type[Any] | None) -> tuple[Any, ...]:
    ...


def returning(*cols: type[Any] | None) -> tuple[Any, ...]:
    ...


def f(col: type[Any]):
    reveal_type(returning(col))

@ilevkivskyi
Copy link
Member Author

@mehdigmira OK, thanks! So about these three:

  1. The overlapping overload thing: this is definitely a bug. Not related to this PR, but it is tangentially related to some ParamSpec work I did, so I am going to submit a separate PR for that.
  2. Tuple index thing: it is intentional, and I am not going to change it. Unsafe indexing was allowed for "old-style" tuples because there was no way to express "a tuple with one or more items" type, now you should use either tuple_data: tuple[T, Unpack[Ts]] or tuple_data: tuple[int, Unpack[tuple[int, ...]]], then you can write tuple_data[0]. In the future, we may relax some other similar index checks for variadic tuples. This is a new feature, and so we should not allow unnecessary unsafety.
  3. The second overload issue with too lenient return type inferred: it is kind of in a grey area, I would say tuple[Any] is probably better, but it looks relatively low priority, you can open a separate issue for this, so someone can look at it in the future.

@JukkaL @jhance As you can see from responses above I am not going to alter this PR now, no need to wait with your reviews.

@jhance I just accidentally noticed #15254. It looks like test cases from there already work (even the one that is commented out). I will add them later, if you know any other missing Callable-related test cases, please do let me know.

@mehdigmira
Copy link

mehdigmira commented Sep 21, 2023

Regarding 2.

Sorry, but I don't fully get the point: tuple[T, ...] means "tuple with arbitrary number of items of type T". And "arbitrary" should allow user to handle the tuple in the way they want to, to avoid false positives.

And this actually seems to be what mypy does: no error is logged here

from __future__ import annotations

from typing import Any


def f(tuple_data: tuple[Any, ...]) -> None:
    tuple_data[0]

So I don't get why mypy would log errors when using a typevartuple.
Since tuple[Any, ...] plays the same role for TypeVarTuple as Any plays for TypeVar (this seems to be the spirit of PEP646, as i understand it), I'd expect tuple[*Ts] and tuple[Any, ...] to be handled the same when Ts is unknown.

--

EDIT: tuple[Unpack[tuple[Any, ...]]] and tuple[Any, ...] are the same, but mypy handles them differently

from __future__ import annotations

from typing import Any, TypeVarTuple, Unpack

_Ts = TypeVarTuple("_Ts")


def f(tuple_data1: tuple[Unpack[tuple[Any, ...]]], tuple_data2: tuple[Any, ...]) -> None:
    tuple_data1[0]  # error
    tuple_data2[0]  # no error

@ilevkivskyi
Copy link
Member Author

tuple[Unpack[tuple[Any, ...]]] and tuple[Any, ...] are the same, but mypy handles them differently

This is a totally different question. And, unfortunately, we should allow this (for consistency and compatibility), and not just for Any. This should be an easy tweak.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/apply.py:1604: error: Incompatible return value type (got "tuple[defaultdict[Any, Any], tuple[Any, ...], ndarray[Any, dtype[signedinteger[Any]]]]", expected "tuple[dict[Any, Any], list[str], ndarray[Any, dtype[signedinteger[Any]]]]")  [return-value]
+ pandas/io/formats/format.py:944: error: List comprehension has incompatible type List[list[Any]]; expected List[tuple[Any, ...]]  [misc]
+ pandas/io/formats/format.py:950: error: List comprehension has incompatible type List[list[str]]; expected List[tuple[Any, ...]]  [misc]
+ pandas/io/formats/format.py:954: error: Incompatible return value type (got "list[tuple[Any, ...]]", expected "list[list[str]]")  [return-value]
+ pandas/core/generic.py:7825: error: Incompatible types in assignment (expression has type "list[Never]", variable has type "tuple[Any, ...]")  [assignment]
+ pandas/core/generic.py:7840: error: Incompatible types in assignment (expression has type "list[Never]", variable has type "tuple[Any, ...]")  [assignment]
+ pandas/core/reshape/melt.py:211: error: Incompatible types in assignment (expression has type "tuple[Any, ...]", variable has type "list[Any]")  [assignment]
+ pandas/core/reshape/concat.py:866: error: Incompatible types in assignment (expression has type "Index", variable has type "tuple[Any, ...]")  [assignment]
+ pandas/core/indexes/multi.py:3938: error: Argument 1 to "append" of "list" has incompatible type "list[Any]"; expected "tuple[Any, ...]"  [arg-type]
+ pandas/core/indexes/multi.py:3945: error: Argument 1 to "append" of "list" has incompatible type "list[Any]"; expected "tuple[Any, ...]"  [arg-type]

optuna (https://github.com/optuna/optuna)
+ optuna/importance/_fanova/_tree.py:67: error: Incompatible types in assignment (expression has type "tuple[Any, ...]", variable has type "list[Any]")  [assignment]

vision (https://github.com/pytorch/vision)
+ torchvision/datasets/video_utils.py:139: error: Need type annotation for "video_fps" (hint: "video_fps: List[<type>] = ...")  [var-annotated]

spack (https://github.com/spack/spack)
+ lib/spack/spack/parser.py:204: error: Unused "type: ignore" comment  [unused-ignore]
+ lib/spack/spack/parser.py:204: error: Item "None" of "Optional[Match[Any]]" has no attribute "lastgroup"  [union-attr]
+ lib/spack/spack/parser.py:204: note: Error code "union-attr" not covered by "type: ignore" comment
+ lib/spack/spack/parser.py:204: error: Invalid index type "Union[str, None, Any]" for "MappingProxyType[str, TokenType]"; expected type "str"  [index]
+ lib/spack/spack/parser.py:204: note: Error code "index" not covered by "type: ignore" comment
+ lib/spack/spack/parser.py:205: error: Unused "type: ignore" comment  [unused-ignore]
+ lib/spack/spack/parser.py:205: error: Item "None" of "Optional[Match[Any]]" has no attribute "group"  [union-attr]
+ lib/spack/spack/parser.py:205: note: Error code "union-attr" not covered by "type: ignore" comment
+ lib/spack/spack/parser.py:206: error: Unused "type: ignore" comment  [unused-ignore]
+ lib/spack/spack/parser.py:206: error: Item "None" of "Optional[Match[Any]]" has no attribute "start"  [union-attr]
+ lib/spack/spack/parser.py:206: note: Error code "union-attr" not covered by "type: ignore" comment
+ lib/spack/spack/parser.py:207: error: Unused "type: ignore" comment  [unused-ignore]
+ lib/spack/spack/parser.py:207: error: Item "None" of "Optional[Match[Any]]" has no attribute "end"  [union-attr]
+ lib/spack/spack/parser.py:207: note: Error code "union-attr" not covered by "type: ignore" comment
+ lib/spack/spack/parser.py:216: error: Unused "type: ignore" comment  [unused-ignore]

sockeye (https://github.com/awslabs/sockeye)
+ sockeye/data_io.py:695: error: Argument 1 to "combine_means" has incompatible type "tuple[Any, ...]"; expected "list[float | None]"  [arg-type]
+ sockeye/data_io.py:704: error: Argument 2 to "combine_means" has incompatible type "tuple[Any, ...]"; expected "list[int]"  [arg-type]
+ sockeye/data_io.py:705: error: Argument 3 to "combine_stds" has incompatible type "tuple[Any, ...]"; expected "list[int]"  [arg-type]
- sockeye/training.py:86: error: Unused "type: ignore" comment  [unused-ignore]
+ sockeye/translate.py:190: error: Incompatible types in assignment (expression has type "tuple[Any, ...]", variable has type "list[str]")  [assignment]

@ilevkivskyi ilevkivskyi merged commit 0291ec9 into python:master Sep 28, 2023
17 checks passed
@ilevkivskyi ilevkivskyi deleted the variadic-call branch September 28, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants