Mypy: Mixins with conflicting signatures

[rowillia]

I'm finding it's quite common to have Python Libraries using mixins with signatures that mypy considers to be conflicting because one takes a different set of kwargs than the other. For example - https://github.com/pallets/itsdangerous/blob/master/itsdangerous.py#L881-L886 (which is simplified below).

class A(object):
    def load(self, payload):
        pass
class B(object):
    def load(self, payload, return_header=False):
        pass
    def dump(self, obj):
        pass
class C(A, B):
    pass

In this case, mypy will complain that A and B have conflicting signatures for load. In practice, Python will always dispatch load calls to A so C's signature for load should be load(self, payload). It would be incorrect to pass return_header to C().load.

What are your thoughts on having mypy reflect what Python will do at runtime here and just take the definition of A's load instead of throwing an error? The only way around this currently is to add **kwargs to As signature which is less safe.

[gvanrossum]

Sounds reasonable, but I haven't thought it through much. What if either
load() method were to use super()?

[rowillia]

@gvanrossum I think that should be OK, right? We would assume the signature we're dispatching to, right? It's definitely not a great practice as it breaks the liskov substitution principle (List[C] is covariant with List[A] but is not with List[B]), but I'm finding it to be quite common in popular libraries.

I think it would make sense to use the same logic as mypy does for inheritance. For example, in the below code example mypy is OK with OKSubA overriding load since it's compatible with Base's signature.

class Base(object):
    def load(self, payload: str) -> None:
        pass
class OKSub(Base):
    def load(self, payload: str, something_else=False) -> None:
        pass
class BadSub(Base):
    def load(self, payload: int, something_else=False) -> None:
        pass

[gvanrossum]

OK, you've convinced me. Unless @JukkaL disagrees we can do this.

[JukkaL]

Mypy treats inheritance as subtyping. In the original example, C is not (safely) a subtype of B, and that's why mypy gives the warning. For example, what about code like this:

def f(b: B) -> None:
    b.load(whatever, return_header=True)   # Fails at runtime if b is an instance of C

f(C())   # Should this be okay?

See also https://github.com/python/typing/issues/246 and https://github.com/python/typing/issues/241, which are related.

[rowillia]

@JukkaL Totally....This would be a case of weakening the type checker to support existing code. I don't think this is a great practice for the reasons you highlighted, but I've found it to be quite a common pattern in existing code bases. This seems to more arise from using Mixins for composition and code sharing as opposed to using Mixins to express a type hierarchy.

In the example above, it's safe to say instanceof(C(), A) == True but instanceof(C(), B) == False If we encounter a case of treating C as a B, we could warn and point to the declaration of B.load as the reason why C can't be treated as a B.

[JukkaL]

It still wouldn't safe in general. Consider this example:

class A(object):
    def load(self, payload):
        pass
class B(object):
    def load(self, payload, return_header=False):
        pass
    def load2(self, payload):
        self.load(payload, return_header=True)
class C(A, B):
    pass

C().load2(stuff)  # ouch

I'm not convinced that mypy should silently accept such code, but it would be nice if there was a documented way of using the idiom with mypy. Here are some possibilities:

1. Make sure # type: ignore used suitably does the right thing and document it.
2. Support implementation-only inheritance (python/typing#241). This may be hard to get right, though.
3. Support declaring B as "pure implementation" and type check it specially (python/typing#246).
4. Have a more specific way of ignoring just these errors when doing multiple inheritance.

The # type: ignore way would probably be the simplest, and it's my favorite for now. We could have that together with warning about relying on C being a subtype of B. 2 and 3 above could be type safe, but they are more complicated and have various usability and performance concerns.

[gvanrossum]

Using # type: ignore feels fine to me, but perhaps we could add one tweak:
invent a feature whereby you can ignore a specific error message on a given
line, rather than just ignoring everything on that line. This would require
us to number our error messages though...