mypy icon indicating copy to clipboard operation
mypy copied to clipboard

Using type Self allows to violate Liskov Substitution Principle

Open AntPeixe opened this issue 2 years ago • 3 comments

Bug Report

By using Self in the type of a method's argument it's possible to violate the Liskov Substitution Principle without any warning.

(A clear and concise description of what the bug is.)

To Reproduce

Mypy raises no issue on this code although SubClass.update is clearly violating LSP.

from typing_extensions import Self

class BaseClass:
    foo: int
    def update(self, other: Self) -> None:
        self.foo = other.foo

class SubClass(BaseClass):
    bar: int
    def update(self, other: Self) -> None:  # problem here
        super().update(other)
        self.bar = other.bar

Expected Behavior

I'd expect that using Self would raise the warning similarly to specifying the class. Mypy raises the issue on the below.

from typing_extensions import Self

class BaseClass:
    foo: int
    def update(self, other: BaseClass) -> None:
        self.foo = other.foo

class SubClass(BaseClass):
    bar: int
    def update(self, other: SubClass) -> None:  # problem here
        super().update(other)
        self.bar = other.bar

Your Environment

  • Mypy version used: 1.4.1
  • Python version used: 3.10.8

AntPeixe avatar Jul 07 '23 08:07 AntPeixe

You get an error though if you try to call .update() from the first code snippet with an instance of BaseClass though. So I think that's safe enough?

EDIT: I guess you can still violate LSP if you first cast the SubClass object to BaseClass and then call .update().

tmke8 avatar Jul 08 '23 21:07 tmke8

I think this is really a matter of intent.

In the initial code listing, what you're telling the user of your class is the parameter to update must be an instance of the same class as the host function, and this is supposed to be true for all sub classes of BaseClass. I would consider this a special allowable exception to LSP that is explicitly documented in your code via the typehint Self. As long as all your subclasses conform to this "contract" (i.e. the parameter to update is always declared as type Self), your code works as "intended". Anyone using this code should understand that this implicitly means that instances of super classes can't be passed as a argument.

In the second code listing, SubClass is deviating from the original contract set by BaseClass in a way that violates LSP, which is why it would make sense for mypy to flag it as an error.

The big difference here is that in the first case, you're maintaining the same method signature, whereas in the second case you're changing it.

Self kind of sells itself as automagically making the the type substitution, but in reality it's a little more subtle than that.

jp-larose avatar Jul 11 '23 02:07 jp-larose

Self exists primarily for the type checker rather than as means of documenting intent. I agree there's a false positive here:

def f(x: BaseClass) -> None:
  x.update(BaseClass())

f(SubClass())  # attribute error getting bar

ikonst avatar Jul 11 '23 05:07 ikonst

In the second code listing, SubClass is deviating from the original contract set by BaseClass in a way that violates LSP, which is why it would make sense for mypy to flag it as an error.

I agree with this and encountered this problem as well. Here's my example, similar to the original example above:

from typing import List

from typing_extensions import Self

class Base:
    def add_to(self, l: List[Self]) -> None:
        l.append(self)

class Derived(Base):
    # This should not be allowed since it changes the type of `add_to`
    # incompatibly with `Base`, but mypy allows it because of `Self`!
    def add_to(self, l: List[Self]) -> None:
        l.append(self)
        
    def something_only_derived_can_do(self) -> None:
        return None

Mypy has no problem with the above, which seems like a bug.

It only raises the alarm when the function is actually used polymorphically(?), e.g.:

list_of_derived: List[Derived] = []

base = Base()
# HERE is where mypy finally realizes the problem:
base.add_to(list_of_derived)

This error is far from the actual source of the problem -- that Derived.add_to violates LSP.

lpulley avatar Sep 28 '23 15:09 lpulley

@lpulley The example you provided is exactly the type of situation that the Self type is intended to solve. The behaviour you're seeing from mypy is correct.

The mistake is thinking that Self replaces the type where it's first used in a class hierarchy. But its true purpose is to say the type of it's filling is the "current runtime class". See PEP 673 – Self Type.

In your example, when you instantiate Base, the signature of add_to is effectively add_to(self: Base, l: list[Base]) -> None. Likewise, when you instantiate Derived the signature off add_to for that object is effectively add_to(self: Derived, l: list[Derived]) -> None

The other major point of confusion is that list[Base] is neither a supertype nor a subtype of list[Derived], that is to say list[T] is invariant, and using Self doesn't change that. See the explanation in PEP 484. This is the true reason mypy complains about base.add_to(list_of_derived).

Type hinting and static type checking in Python is an ever moving target. That makes it difficult to keep track of all that's going on. But as far as I can tell, it's always progressing.

Does Self break LSP? Yes, but very intentionally and very purposefully. It's an exception to the rule. And I love it!

jp-larose avatar Oct 24 '23 04:10 jp-larose