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

[overload][intersection-types] undetected LSP violation in multiple inheritance #15790

Open
randolf-scholz opened this issue Jul 31, 2023 · 4 comments
Labels
bug mypy got something wrong

Comments

@randolf-scholz
Copy link
Contributor

randolf-scholz commented Jul 31, 2023

Bug Report

Essentially, the bug occurs when trying to define a subclass of C≤A&B, and implementing a shared method via @overload:

class A:
    def foo(self, x: Hashable) -> Hashable: ...
class B:
    def foo(self, x: Sized) -> Sized: ...
class C(A, B):
    @overload
    def foo(self, x: Hashable) -> Hashable: ...
    @overload
    def foo(self, x: Sized) -> Sized: ...

C is not a proper subclass of A & B since C.foo(Hashable & Sized) yields Hashable, violating LSP for B. mypy does not detect this issue. To resolve this, an extra overload of the form def foo(x: Hashable & Sized) -> ... is needed.

To Reproduce

A concrete demo with non-trivial types.
from typing import overload
from collections.abc import Hashable, Sized

class A:
    def foo(self, x: Hashable) -> int: 
        return hash(x)
    
class B:
    def foo(self, x: Sized) -> str: 
        return f"x has {len(x)} elements"
    
class C(A, B):
    @overload
    def foo(self, x: Hashable) -> int: ...
    @overload
    def foo(self, x: Sized) -> str: ... 
    
    def foo(self, x):  # type: ignore[no-untyped-def]
        if isinstance(x, Hashable):
            return A.foo(self, x)
        if isinstance(x, Sized):
            return B.foo(self, x)

# mypy does not complain about LSP
a_lsp: A = C()
b_lsp: B = C()
c: C = C()

# tuples are hashable and sized.
x: tuple[int, int, int] = (1,2,3)

# but the return types are incompatible!
reveal_type(a_lsp.foo(x))  # int
reveal_type(b_lsp.foo(x))  # str
reveal_type(c.foo(x))      # int, is not a subtype of str

This file parses even using --strict without raising any warnings/errors. (mypy-playground)
However, the inconsistency becomes obvious as c.foo(<tuple>) reveals as int, which is incompatible with b.foo(<tuple>).

Expected Behavior

@overload
def foo(self, x: Hashable) -> int: ...
@overload
def foo(self, x: Sized) -> str: ... 

Are not type-safe overloads to satisfy LSP for both superclasses. Mypy should notice this and issue a warning. Likely, to solve this issue in a satisfactory manner, intersection-types are needed (python/typing#213).

@randolf-scholz randolf-scholz added the bug mypy got something wrong label Jul 31, 2023
@randolf-scholz randolf-scholz changed the title [overload][intersection-types] multiple inheritance LSP violation [overload][intersection-types] undetected LSP violation in multiple inheritance Jul 31, 2023
erictraut pushed a commit to microsoft/pyright that referenced this issue Aug 1, 2023
… base classes that are combined using multiple inheritance implement the same method in an incompatible way. The bug caused the check to be skipped if the child class also implemented the same method as the two base classes. This addresses python/mypy#15790.
erictraut added a commit to microsoft/pyright that referenced this issue Aug 1, 2023
… base classes that are combined using multiple inheritance implement the same method in an incompatible way. The bug caused the check to be skipped if the child class also implemented the same method as the two base classes. This addresses python/mypy#15790. (#5617)

Co-authored-by: Eric Traut <erictr@microsoft.com>
@DiscordLiz
Copy link

I don't think this is a bug. A subclass defined this way is replaceably usable with each parent class. There is ongoing discussion about this in the intersection pep, this may be prematurely calling something a bug

@DiscordLiz
Copy link

DiscordLiz commented Aug 1, 2023

Here is an example where it can clearly be used as a drop in replacement for either base:

class ParamA:
    ...

class ParamB:
    ...

class ParamAB(ParamA, ParamB):
    ...

class A:
    def foo(self, x: ParamA) -> ParamA:
        ...

class B:
    def foo(self, x: ParamB) -> ParamB:
        ...

class AB(A, B):
    @overload
    def foo(self, x: ParamA) -> ParamA:
        ...

    @overload
    def foo(self, x: ParamB) -> ParamB:
        ...

    def foo(self, x):
        if isinstance(x, ParamA):
            if isinstance(x, ParamB):
                raise RuntimeError("Ambiguous use")
            return ParamA()
        else:
            return ParamB()

@DiscordLiz
Copy link

DiscordLiz commented Aug 1, 2023

There is disucssion about the soundness of this and the need for Not as well to correctly express the actual type which has come up in the intersections pep discussions. We need a way for these overloads to express that they don't accept types which overlap. Resolving this case is possible both now by saying in the immediate term that it should be forbidden as-is, or by saying that use of the method is ambiguous without further constraint.

@DiscordLiz
Copy link

DiscordLiz commented Aug 1, 2023

Copying the relevant comment portion: (note & for Intersection, ~ for Not)

a more expressive overload set for AB.foo would be

(overload) (ParamA & ParamB) -> Never
(overload) (ParamA & ~ParamB) -> ParamA
(overload) (ParamB & ~ParamA) -> ParamB

But if someone had an implementation that was

(overload) (ParamA & ParamB) -> ParamA & ParamB
(overload) (ParamA & ~ParamB) -> ParamA
(overload) (ParamB & ~ParamA) -> ParamB

it would also be valid.

As we don't currently have a way to express this, this should not be treated as incorrect, as it is possible to implement both of the behaviors the overload sets express.

erictraut pushed a commit to microsoft/pyright that referenced this issue Aug 4, 2023
…ther two base classes that are combined using multiple inheritance implement the same method in an incompatible way. The bug caused the check to be skipped if the child class also implemented the same method as the two base classes. This addresses python/mypy#15790. (#5617)"

This reverts commit fb56dcc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

2 participants