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

Unpacking an iterable with an __iter__ method that returns self leads to variable being silently inferred as Any #14819

Closed
AlexWaygood opened this issue Mar 2, 2023 · 9 comments · Fixed by #14821
Assignees
Labels
bug mypy got something wrong topic-type-variables

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 2, 2023

This is similar to #14811, but still reproduces on master following bed49ab:

from typing import TypeVar

T = TypeVar("T")

class Foo:
    count: int
    def __init__(self) -> None:
        self.count = 0
    def __iter__(self: T) -> T:
        return self
    def __next__(self) -> int:
        self.count += 1
        if self.count > 3:
            raise StopIteration
        return self.count

a, b, c = Foo()
reveal_type(a)  # note: Revealed type is "Any"

Mypy should have enough information here to infer that the type of a is int.

Mypy versions tested on

@AlexWaygood AlexWaygood added bug mypy got something wrong topic-type-variables labels Mar 2, 2023
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 2, 2023

This one-line change fixes the issue:

--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -3633,7 +3633,7 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
     ) -> None:
         rvalue_type = get_proper_type(rvalue_type)
         if self.type_is_iterable(rvalue_type) and isinstance(rvalue_type, Instance):
-            item_type = self.iterable_item_type(rvalue_type)
+            _, item_type = self.analyze_iterable_item_type_without_expression(rvalue_type, context)

But it causes this test to fail:

[case testInferringTypesFromIterable]
from typing import Iterable
class Nums(Iterable[int]):
def __iter__(self): pass
def __next__(self): pass
a, b = Nums()
if int():
a = b = 1
if int():
a = '' # E: Incompatible types in assignment (expression has type "str", variable has type "int")
if int():
b = '' # E: Incompatible types in assignment (expression has type "str", variable has type "int")
[builtins fixtures/for.pyi]

The test succeeds once again if we add annotations:

--- a/test-data/unit/check-inference.test
+++ b/test-data/unit/check-inference.test
@@ -375,10 +375,10 @@ def f(d: Any) -> None:
     b.x

 [case testInferringTypesFromIterable]
-from typing import Iterable
+from typing import Iterable, Iterator
 class Nums(Iterable[int]):
-    def __iter__(self): pass
-    def __next__(self): pass
+    def __iter__(self) -> Iterator[int]: pass
+    def __next__(self) -> int: pass

@sobolevn, do you think this would be a reasonable fix? It makes me wonder why we have the iterable_item_type() method at all, and whether other uses of this method in checker.py should also be converted to analyze_iterable_item_type_without_expression()

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 2, 2023

I can't tell whether the required change to testInferringTypesFromIterable would be a feature or a bug. Mypy usually infers that unannotated methods return Any. It feels weird to me that this test case is currently asserting that mypy should infer a non-Any type here, despite both __iter__ and __next__ being unannotated.

@sobolevn
Copy link
Member

sobolevn commented Mar 2, 2023

It makes me wonder why we have the iterable_item_type() method at all, and whether other uses of this method in checker.py should also be converted to analyze_iterable_item_type_without_expression()

I don't really know. It was my first Iterable-related PR.

I can't tell whether the required change to testInferringTypesFromIterable would be a feature or a bug

It is really hard to tell whether your solution is a bug or an improvement (sadly, python's type system is not documented enough).

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 2, 2023

Thanks! I guess I'll experiment with a broader refactor and see if anything (else) breaks :)

@AlexWaygood AlexWaygood self-assigned this Mar 2, 2023
@AlexWaygood
Copy link
Member Author

The test was introduced in 2013, in 6ecbd1f

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 2, 2023

There is another test in a different file that asserts the same behaviour, so I'm going for a slightly different approach:

[case testMultipleAssignmentAndGenericSubtyping]
from typing import Iterable
n, s = None, None # type: int, str
class Nums(Iterable[int]):
def __iter__(self): pass
def __next__(self): pass
n, n = Nums()
s, s = Nums() # E: Incompatible types in assignment (expression has type "int", variable has type "str")
[builtins fixtures/for.pyi]
[out]

@sterliakov
Copy link
Contributor

Is the behaviour in those two tests really expected? As per docs, all untyped methods should be treated as annotated with Any for all arguments and return type, while here this rule is violated (__iter__ is unannotated, so it's essentialy unpacking Any, which should give n-tuple of Anys of size corresponding to LHS). The inherited signature should not be taken into account. The proposed change fixes this behaviour and makes handling of such cases more consistent - what's the reasoning behind that tests?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 2, 2023

@sterliakov, I don't disagree with anything you're saying. I'm just somewhat hesitant to make that change here, given that these tests have been in place for 10 years or more -- it could be the case that many people are depending on this being established mypy behaviour. The fact that two tests assert the same behaviour indicates that it was a deliberate decision, even if we might disagree with that decision now.

Fixing the original bug in this issue thread while maintaining the behaviour asserted in these two tests actually appears to be fairly trivial, so for now, I'm planning on opening a PR to do that. I'll open a followup issue after that to discuss whether the behaviour that's asserted in these two tests should be changed.

JukkaL pushed a commit that referenced this issue Mar 3, 2023
…rit from `typing.Iterator` (#14821)

Fixes #14819.

Mypy currently silently infers an `Any` type when unpacking an iterator
that doesn't explicitly inherit from `typing.Iterator` (i.e., an
iterator that's a _structural_ subtype of `typing.Iterator`, but not a
_nominal_ subtype):

```python
from typing import TypeVar

T = TypeVar("T")

class Foo:
    count: int
    def __init__(self) -> None:
        self.count = 0
    def __iter__(self: T) -> T:
        return self
    def __next__(self) -> int:
        self.count += 1
        if self.count > 3:
            raise StopIteration
        return self.count

a, b, c = Foo()
reveal_type(a)  # note: Revealed type is "Any"
```

However, we have enough information here to infer that the type of `a`
should really be `int`. This PR fixes that bug.

There's discussion on the issue thread about an alternative solution
that would involve changing some mypy behaviour that's been established
for around 10 years. For now, I haven't gone for that solution.
@AlexWaygood
Copy link
Member Author

@sterliakov, the behaviour does appear to be consistent with some other parts of mypy also. For example:

from typing import Iterable

class Foo(Iterable[int]):
    def __iter__(self):
        yield from range(5)

x = list(Foo())
reveal_type(x)  # Revealed type is "builtins.list[builtins.int]"

https://mypy-play.net/?mypy=latest&python=3.11&gist=689e0a0950b2f48cf9608ffe03c47926

I guess that this is a general principle applied by mypy that I wasn't aware of, where classes that directly inherit from protocols are assumed to have the more specific signature of their superclass, even if the method on the subclass is unannotated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-type-variables
Projects
None yet
3 participants