Skip to content

Conversation

@tungol
Copy link
Contributor

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]

@tungoltungol marked this pull request as draft October 15, 2024 18:38
@AlexWaygoodAlexWaygood changed the title Remove rundant inheritances from Iterator in itertoolsRemove redundant inheritances from Iterator in itertoolsOct 15, 2024
@tungol
Copy link
ContributorAuthor

tungol commented Oct 16, 2024

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

python-chess simplifies to:

importitertoolsfoo: list[int] = [] condition=Trueiter(itertools.pairwise(foo) ifconditionelse [])

xarray simplifies to

importitertoolscond=Truefoo: list[int] = [] set().difference(range(1) ifcondelseitertools.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:

importitertoolsfromtypingimportoverload@overloaddeffoo(arg1: str) ->str: ... @overloaddeffoo(arg1: str, arg2: int) ->int: ... deffoo(arg1: str, arg2: int|None=None) ->int|str: return1defbar() ->int|str: arrs: list[int] = [] result=foo(*itertools.chain(arrs)) reveal_type(result) returnresult

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.

@tungoltungol marked this pull request as ready for review October 16, 2024 03:56
@srittausrittau merged commit 281dd35 into python:mainOct 18, 2024
@tungoltungol 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
ContributorAuthor

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

@AlexWaygood
Copy link
Member

I would prefer that, yes :-)

Sign up for freeto 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

@tungol@AlexWaygood@srittau