Skip to content

Conversation

@tungol
Copy link
Contributor

@tungol tungol commented Oct 15, 2024

No description provided.

@github-actions
Copy link
Contributor

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

python-chess (https://github.com/niklasf/python-chess)
+ chess/pgn.py:194: error: No overload variant of "iter" matches argument type "object"  [call-overload]
+ chess/pgn.py:194: note: Possible overload variants:
+ chess/pgn.py:194: note:     def [_SupportsNextT: SupportsNext[Any]] iter(SupportsIter[_SupportsNextT], /) -> _SupportsNextT
+ chess/pgn.py:194: note:     def [_T] iter(_GetItemIterable[_T], /) -> Iterator[_T]
+ chess/pgn.py:194: note:     def [_T] iter(Callable[[], _T | None], None, /) -> Iterator[_T]
+ chess/pgn.py:194: note:     def [_T] iter(Callable[[], _T], object, /) -> Iterator[_T]

xarray (https://github.com/pydata/xarray)
+ xarray/core/dataset.py: note: In function "_get_chunk":
+ xarray/core/dataset.py:280: error: Argument 1 to "difference" of "set" has incompatible type "object"; expected "Iterable[Any]"  [arg-type]
+ xarray/core/dataset.py: note: At top level:

jax (https://github.com/google/jax)
+ jax/_src/numpy/linalg.py:2087: error: Unused "type: ignore" comment  [unused-ignore]

@tungol tungol marked this pull request as draft October 15, 2024 18:38
@AlexWaygood AlexWaygood changed the title Remove rundant inheritances from Iterator in itertools Remove redundant inheritances from Iterator in itertools Oct 15, 2024
@tungol
Copy link
Contributor Author

tungol commented Oct 16, 2024

Both mypy primer hits look like instances of python/mypy#4373 to me.

python-chess simplifies to:

import itertools

foo: list[int] = []
condition = True

iter(itertools.pairwise(foo) if condition else [])

xarray simplifies to

import itertools

cond = True
foo: list[int] = []
set().difference(range(1) if cond else itertools.accumulate(foo))

The unused ignore from jax is something involving an overloaded function that I don't fully understand yet. I played around with it a bunch, and ended up with this simplified form:

import itertools
from typing import overload


@overload
def foo(arg1: str) -> str: ...


@overload
def foo(arg1: str, arg2: int) -> int: ...


def foo(arg1: str, arg2: int | None = None) -> int | str:
    return 1


def bar() -> int | str:
    arrs: list[int] = []
    result = foo(*itertools.chain(arrs))
    reveal_type(result)
    return result

Without this change, this produces:

temp.py:19: error: No overload variant of "foo" matches argument type "chain[int]"  [call-overload]
temp.py:19: note: Possible overload variants:
temp.py:19: note:     def foo(arg1: str) -> str
temp.py:19: note:     def foo(arg1: str, arg2: int) -> int
temp.py:20: note: Revealed type is "Any"
Found 1 error in 1 file (checked 1 source file)

With this MR in place, mypy gives:

temp.py:20: note: Revealed type is "builtins.str"
Success: no issues found in 1 source file

This might also be a mypy bug? It seems like when itertools.chain is explicitly Iterator[_T], mypy knows that *chain() works out to _T. When chain is not explicitly Iterator, mypy is failing to pick up the type from chain.__next__() -> _T and inserting Any

I realize that this form of it could never really work; the original argument types are very different and include a *args factor that soaks up additional arguments, but this re-arranged version is a little clearer and doesn't affect the issue being surfaced.

@tungol tungol marked this pull request as ready for review October 16, 2024 03:56
@srittau srittau merged commit 281dd35 into python:main Oct 18, 2024
@tungol tungol deleted the itertools branch October 18, 2024 16:30
@AlexWaygood
Copy link
Member

I'm a bit concerned by the primer output here. Even if the root cause of these false positives is a mypy bug, we should try not to make changes to the stubs if they end up hurting users of type checkers more than they end up helping users of type checkers. All else being equal, I agree it's best to have a class's MRO in the stubs be as accurate as it can possibly be, but that shouldn't come at any cost.

@tungol
Copy link
Contributor Author

tungol commented Oct 18, 2024

Would you like me to put up an MR reversing the change?

@AlexWaygood
Copy link
Member

I would prefer that, yes :-)

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