Signatures of overloaded operators are incompatible with super type

[HazardousPeach]

I've noticed this problem, very similar to #3262 , except with overloaded operators.

Originally, #3262 made it so that code like this:

from typing import overload

class A:
    def f(self, x : 'A') -> 'A': ...

class B(A):
    @overload
    def f(self, x : 'A') -> 'A': ...
    @overload
    def f(self, x : 'B') -> 'B' : ...

Wouldn't typecheck.

#3263 appears to have fixed this. Unfortunately, while this works for normal method signatures, it doesn't appear to work for overloaded operators. So the very similar code:

from typing import overload

class A:
    def __add__(self, x : 'A') -> 'A': ...

class B(A):
    @overload
    def __add__(self, x : 'A') -> 'A': ...
    @overload
    def __add__(self, x : 'B') -> 'B' : ...

Still fails to typecheck, with the error message:

test.pyi:7: error: Signature of "__add__" incompatible with supertype "A"

I can't think of any reason why this should be a problem with operators, although maybe someone else can explain.

I'm on Python 3.6.3 and MyPy 0.600+dev (master branch from git)

[ilevkivskyi]

It looks like this is intentional. The point is __add__ and __radd__ needs to be mutually compatible. I found this code that explicitly prohibits overloads for special methods:

        if (not isinstance(original, Overloaded) and
              isinstance(override, Overloaded) and
              name in nodes.reverse_op_methods.keys()):
            # Operator method overrides cannot introduce overloading, as
            # this could be unsafe with reverse operator methods.
            fail = True

TBH, I don't see an immediate reason why this is unsafe (__radd__ doesn't even appear in this example), maybe @JukkaL can clarify this.

[Michael0x2a]

I think I might understand what's going on here: I believe mypy is correct in saying that overloads don't necessarily interact in a typesafe way with operator dunder methods. Here's an example: it's similar to the original code snippet, except that I changed the signature of B slightly.

from typing import overload

class A:
    def __add__(self, x : 'A') -> 'A': 
        if isinstance(x, A):
            return A()
        else:
            return NotImplemented

class B(A):
    @overload
    def __add__(self, x : 'Other') -> 'B' : ...
    @overload
    def __add__(self, x : 'A') -> 'A': ...
    def __add__(self, x):
        if isinstance(x, Other):
            return B()
        elif isinstance(x, A):
            return A()
        else:
            return NotImplemented

class Other:
    def __radd__(self, x: 'A') -> 'Other':
        if isinstance(x, A):
            return Other()
        else:
            return NotImplemented

# Example 1
actually_a: A = A()
reveal_type(actually_a + Other())  # Inferred type and actual type are both 'Other'

# Example 2
actually_b: A = B()
reveal_type(actually_b + Other())  # Inferred type is 'Other'; actual type is 'B'!

That said, I think the specific example OP originally posted is typesafe: I think B.__add__(...) in that example actually is a legitimate subtype of A.__add__(...) so there's no issue.

The problem now is that mypy doesn't understand how to perform subtype checks against overloaded functions: if you look at the visit_overloaded methods in subtypes.py, they all say something like "TODO: What's the right thing to do here?" (which is a legit question imo...).

So, here's what I think a fix might look like:

1. We modify the subtype visitors so they handle overloaded callables correctly.
2. We remove the check @ilevkivskyi found and rely exclusively on the if not is_subtype(override, original, ...) check that happens directly above.

Step 1 is obviously the hard part, though I sort of have an idea of how this might be done: we take the overload and remove alternatives where the param and return types are all strictly narrower so that all of the alternatives are distinct from one another, then take the callable/alternatives in the left and make sure they're a subset of the right.

Disclaimer: I haven't really thought about this too much, so I could be totally wrong about all of the above.

[ilevkivskyi]

they all say something like "TODO: What's the right thing to do here?" (which is a legit question imo...).

Hm, I think you mean ProperSubtypeVisitor, we already have a decent implementation of what you propose in SubtypeVisitor, which may be copied almost verbatim for proper subtypes (modulo adjustments due to recent clarifications for overloads).

[ilevkivskyi]

@Michael0x2a As I understand you are going to take care of this one during your re-working of overloads.

[ilevkivskyi]

Also this turns out to be a duplicate of #1987 but this one has more up-to-date discussion, so I closed the other one.

[ilevkivskyi]

@Michael0x2a Could you clarify what what do you mean by

1. We modify the subtype visitors so they handle overloaded callables correctly.

As I mentioned above there is already a decent check for overload subtypes. Also in your example B.__add__ is clearly a subtype of A.__add__, so there will be no error reported. But there is still a problem. Does this mean that your recent operator overlapping PR still misses some cases of unsafe overlaps between op-methods? Or are you proposing to have special stricter subtype checks for op-methods?

[ilevkivskyi]

Another comment: if I will not use overload at all, it seems to me there is still the problem you shown. For example:

class B(A):
     def __add__(self, x: Union[A, Other]) -> A: ...

has exactly the same problem. So it looks like this problem has nothing to do with overloads.

[Michael0x2a]

@ilevkivskyi -- yes, I think I misdiagnosed the underlying bug in my first post above. I was mistakenly under the assumption that both subtype visitors were missing logic for whether an overloaded function is a subtype of another callable, when in reality only the proper subtype visitor was missing that logic.

My newest hypothesis is that the existing subtype logic for overloads is buggy or too over-zealous in some way, though I'm not 100% sure of that -- I tried reading through the existing implementation in SubtypeVisitor a little while back, and TBH found it a bit difficult to follow.

My new plan is at some point to audit the subtyping logic for overloads and simplify it/re-think it if possible. (In particular, I'm hoping that the more tighter checks on overload definitions will let us remove/simplify some of the subtyping logic.)

My latest operator overlapping PR only changes how mypy behaves when a reverse operator is present. The examples up above only contain __add__, not __radd__, so it's not surprising to me the latest refactor had no effect.

[Michael0x2a]

Another comment: if I will not use overload at all, it seems to me there is still the problem you shown.

Huh, really? That's confusing.

I guess just about the only thing I'm certain about at this point then is that my recent refactor is unrelated to the problems raised in this PR.

[ilevkivskyi]

My latest operator overlapping PR only changes how mypy behaves when a reverse operator is present. The examples up above only contain __add__, not __radd__, so it's not surprising to me the latest refactor had no effect.

Other has __radd__, and I think this is the crux. The check for unsafe overlap should simply not only check overlap with A.__add__, but also with __add__ in all its subclasses. I think this will fix both the original problem you shown and the union variant. What do you think?

[ilevkivskyi]

Just to clarify, I didn't think much about this, but it seems to me that the fact that A doesn't unsafely overlap with B doesn't guarantee that A doesn't unsafely overlap with a subtype of B.

[ilevkivskyi]

I guess just about the only thing I'm certain about at this point then is that my recent refactor is unrelated to the problems raised in this PR.

I don't want to say your overlap PR did something bad (or insufficiently good) :-) I really like it. What I want to say is that fixing the original problem in this issue while avoiding the problem you shown above (and its union variant) might look like this:

• Remove the check I found in checker.py (removes the false positive)
• Apply the unsafe overlap check you recently improved to any overrides in subclasses (avoids new false negatives).

Does this make sense to you?

[ilevkivskyi]

OK, after some more thinking my solution is problematic in incremental mode (and generally looks awkward). Another option (a bit sad) is to have special stricter subtyping checks for dunders (something like the domain shouldn't be wider in the subtype).

[ilevkivskyi]

On the other hand, "refining" the type of other in the subtype is the most common use case. I am not sure I have ever seen a situation when someone widened domain for dunders in a subtype. I will make a PR now.

[Michael0x2a]

I don't want to say your overlap PR did something bad (or insufficiently good) :-) I really like it.

Oh, I didn't mean to come off as defensive -- it's just that the very original post doesn't contain an __radd__ anywhere, and the recent refactor only touches code that kicks in when an __radd__ is present.

Other has radd, and I think this is the crux. The check for unsafe overlap should simply not only check overlap with A.add, but also with add in all its subclasses. I think this will fix both the original problem you shown and the union variant. What do you think?

idk if I like this approach -- it seems expensive, and I also don't know how well it'll play when mixing together different libraries. (E.g. what if the library defines a bunch of stuff that includes __add__, then my own code defines __radd__ or something? I'm not sure what could go wrong, but it feels like something could.)

Another option (a bit sad) is to have special stricter subtyping checks for dunders (something like the domain shouldn't be wider in the subtype).

I think this is probably the most pragmatic solution, yeah.

On the other hand, "refining" the type of other in the subtype is the most common use case.

Yeah, agreed. The only real use-case I've seen for doing weird stuff with operators is when you're trying to create a custom DSL (e.g. something like what pyparsing does) -- but I don't think it's worth investing too much time into supporting these types of use cases (at least, until somebody complains, at least).

[Michael0x2a]

(I still think it's worth cleaning up the subtyping for overloads code at some point though -- setting everything aside, it's a bit difficult to read IMO.)

[ilevkivskyi]

(I still think it's worth cleaning up the subtyping for overloads code at some point though -- setting everything aside, it's a bit difficult to read IMO.)

Yes, I agree it is hard to read. Plus we need to copy it to ProperSubtypeVisitor for consistency. But this can be an independent PR.

[Levitanus]

Sorry for necroposting, but it seemed relevant to the issue:

NTU = Union['KspVar[NT]', 'AstBase[NT]', 'ProcessNum[NT]', NT]
NotVarNTU = Union['AstBase[NT]', 'ProcessNum[NT]', NT]
FT = TypeVar('FT', bound=Callable[..., Any])


def ducktype_num_magic(method: FT) -> FT:

    def wrpapper(self: T, other: NTU[NT]) -> Any:
        other = self._check_for_int(other)  # type: ignore
        value = get_value(other)
        if not isinstance(value, self._ref_type):
            raise TypeError(
                f'incompatible type: {type(other)} -> {NTU[self._ref_type]}')
        return method(self, other)
    return cast(FT, wrpapper)


class ProcessNum(Magic[NT], Generic[NT]):
    _ref_type: Type[NT]

    @ducktype_num_magic
    def __add__(self, other: NTU[NT]) -> 'AstAdd[NT]':
        return AstAdd(self, other)

    @ducktype_num_magic
    def __radd__(self, other: NTU[NT]) -> 'AstAdd[NT]':
        return AstAdd(self, other)
    
    @overload  # Signatures of __add__ and __iadd__ are incompatible
    def __iadd__(self, other: NotVarNTU[NT]) -> KspVar[NT]:
        ...

    @overload
    def __iadd__(self, other: KspVar[NT]) -> KspVar[NT]:
        ...

    @ducktype_num_magic
    def __iadd__(self: T, other: NTU[NT]) -> Union[T, KspVar[NT]]:
        if isinstance(self, AstBase):
            raise TypeError('assignement is not supported')
        if isinstance(other, KspVar):
            name = self.name  # type: ignore
            ret_obj = other.copy(name.name, name.prefix, name.postfix)
        else:
            ret_obj = cast(KspVar[NT], self)
        ret_obj._value = get_value(other)
        return ret_obj

There is no error without overloaded variants, So I can't undestand where problem is.
KspVar is subclass of NumericSupport, AstBase not, but can handle some nums

[tgpfeiffer]

Hm, for me, with mypy 0.770 I still have the problem

# testtype.py:15: error: Signature of "__mul__" incompatible with supertype "A"
# testtype.py:15: note: Overloaded operator methods can't have wider argument types in overrides

for this code:

from typing import TypeVar, overload, Union

T = TypeVar("T", bound="A")


class A:
    def __mul__(self: T, other: int) -> T:
        raise NotImplementedError()


U = TypeVar("U", bound="B")


class B(A):
    @overload
    def __mul__(self: U, other: U) -> U:
        ...

    @overload
    def __mul__(self: U, other: int) -> U:
        ...

    def __mul__(self: U, other: Union[int, U]) -> U:
        raise NotImplementedError()

where this code passes validation if f is used instead of __mul__. The PR comment in #5505 says:

Mypy previously disallowed introducing overloads for operator methods in overrides because this can interact unsafely with reverse methods. However it looks like this is safe for the situations where the domain of the override is not extended.

I am not sure I understand that comment, does it mean that the __mul__ overrides above are not safe, while the f ones are?

[ilevkivskyi]

does it mean that the __mul__ overrides above are not safe, while the f ones are?

Yes, exactly, this is because with __mul__ there is __rmul__ that may be defined in some downstream module, so all the operator dunders have stricter subclassing rules. Above Michael posted an example very similar to yours that illustrates the unsafety.

[finite-state-machine]

Could this error be relaxed if the class were marked @final?