Open Levitanus opened 5 years ago
There are lots of subtle special-casing in how Python calls __add__
/__iadd__
/__radd__
. There are therefore a couple of restrictions that must be satisfied in order to be type safe. I am not sure if this particular example is indeed unsafe, but I have found this comment where this error is generated:
# An inplace operator method such as __iadd__ might not be
# always introduced safely if a base class defined __add__.
# TODO can't come up with an example where this is
# necessary; now it's "just in case"
Unfortunately, they are not documented, and every time I am asked about this I spend an hour to write an example of unsafe behavior.
@Michael0x2a you probably have the most context about this. Will you have time to write some docs about dunder related additional checks/errors? (Probably the docs should be in https://mypy.readthedocs.io/en/latest/common_issues.html)
I did some poking around and it looks like this error message was intended to handle the following case:
class Parent:
def __add__(self, other: object) -> 'Parent':
return Parent()
class Child(Parent):
def __iadd__(self, other: object) -> str:
return "foo"
var: Parent = Child()
var += 3
# What is the type of 'var'?
This program is indeed not typesafe: at runtime, the final type of var
will be str
, since we're using Child's __iadd__
method. However, at typecheck time, the only type we can infer for var
is Parent: var
is declared to be of type Parent, the Parent class doesn't have an __iadd__
type, and so mypy falls back to using the __add__
method. This mirrors what would happen at runtime if var
were actually an instance of Parent, instead of Child.
The error message correctly flags that Child's __iadd__
is incompatible with Parent in this case.
However, this check doesn't seem to take into account to take into account is if Parent also defines an __iadd__
method: in that case, we would never fall back to using the __add__
method.
So basically, I think @Levitanus's code snippet ought to type-check, unless I'm missing something.
Anyways, I'll submit a PR to enhance this check and also look into improving either our error messages or our docs, as you suggested.
Hmm, actually, I think @Levitanus's code snippet is actually not typesafe if the child class is allowed to arbitrarily return NotImplemented
:
class A:
def __add__(self, other) -> int: ...
def __iadd__(self, other) -> 'A': ...
class C(A):
def __iadd__(self, other) -> 'C':
if isinstance(other, str):
return NotImplemented
return C()
var: A = C()
var += "foobar"
# At runtime, 'var' will be int, but mypy will infer 'A'.
In fact, if we assume any operator method can return NotImplemented
at any time, the definition of A
just by itself also wouldn't be typesafe:
class A:
def __add__(self, other) -> int:
return 3
def __iadd__(self, other) -> 'A':
if isinstance(other, int):
return NotImplemented
return A()
var = A()
var += 3
# Inferred type is 'A', but runtime type is 'int'?
So now I'm confused -- what even is our policy regarding NotImplemented
? Should we assume the user will return that only if the input argument doesn't match the declared type? Or should we assume any operator method can return it for any reason whatsoever?
Inferred type is 'A', but runtime type is 'int'?
Sorry, I missed something... Why is runtime type is int? It has to be 'A'. As I can conclude – simple operators return the type, which 'value' of class brings:
class A:
value: int
def __add__(self, other) -> int:
assert isinstance(other, int)
return self.value + other
def __iadd__(self, other) -> 'A':
if not isinstance(other, int):
# also, I'm not a pro programmer, but it seemed to me that raise TypeError
# is common solution at this case
return NotImplemented
# here we are making assignement operation.
# Can be Your case with "quasi immutable new A()"
return A(self.__add__(other))
# or
self.value = self.__add__(other)
return self
So, my point of view that __iadd__
method does not the work of adding something, but an assignment of addition operation to something of self-type.
P.S.
The error message correctly flags that Child's iadd is incompatible with Parent in this case.
And this point I now understand and conclude))
@Levitanus -- you should try running the code samples I posted.
In short, if some binary operator method returns NotImplemented, it's a signal meaning that it doesn't know what to do with the incoming type. Python will then try using the "next" operator method that can potentially return a reasonable result.
For example, if a.__add__(b)
returns NotImplemented, Python will then (usually) try b.__radd__(a)
before giving up.
Here, if a.__iadd__(b)
returns NotImplemented, Python will try a.__add__(b)
next before giving up.
This is basically why it's important for the behavior of __add__
and __radd__
as well as __add__
and __radd__
to be consistent with each other, to some degree. The problem is that what Python actually does can be more complicated then what I described above, which is why figuring out exactly what mypy ought to be doing can be challenging.
@Michael0x2a I think we had a similar discussion in https://github.com/python/mypy/issues/4985. The conclusion was that:
NotImplemented
whenever they want.other
in a subclass should be OK.In this case we should probably try to allow something like this:
class B:
def __iadd__(self, other) -> B:
...
class C(B):
def __iadd__(self, other) -> C:
...
since it is quite typical pattern.
(Anyway, documenting all kinds of unsafe operator overlap checks and their limitations is important independently of what we will decide here.)
I'm recieving an a error within folowing code:
I'm not sure that
__add__
and__iadd__
methods have to be compatible. And, if belief to the MyPy in the signature of class A, that not produce the error, MyPy also doesn't think they have to be compatible...