[ty] Avoid emitting Liskov repeated violations from grandparent to child (#22484)
## Summary If parent violates LSP against grandparent, and child has the same violation (but matches parent), we no longer flag the LSP violation on child, since it can't be fixed without violating parent. If parent violates LSP against grandparent, and child violates LSP against both parent and grandparent, we emit two diagnostics (one for each violation). If parent violates LSP against grandparent, and child violates LSP against parent (but not grandparent), we flag it. Closes https://github.com/astral-sh/ty/issues/2000.
This commit is contained in:
Charlie Marsh
@@ -140,7 +140,11 @@ If a child class's method definition is Liskov-compatible with the method defini
|
||||
class, Liskov compatibility must also nonetheless be checked with respect to the method definition
|
||||
on its grandparent class. This is because type checkers will treat the child class as a subtype of
|
||||
the grandparent class just as much as they treat it as a subtype of the parent class, so
|
||||
substitutability with respect to the grandparent class is just as important:
|
||||
substitutability with respect to the grandparent class is just as important.
|
||||
|
||||
However, if the parent class itself already has an LSP violation with an ancestor, we do not report
|
||||
the same violation for the child class. This is because the child class cannot fix the violation
|
||||
without introducing a new, worse violation against its immediate parent's contract.
|
||||
|
||||
<!-- snapshot-diagnostics -->
|
||||
|
||||
@@ -156,13 +160,31 @@ class Parent(Grandparent):
|
||||
def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
|
||||
class Child(Parent):
|
||||
# compatible with the signature of `Parent.method`, but not with `Grandparent.method`:
|
||||
def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
# compatible with the signature of `Parent.method`, but not with `Grandparent.method`.
|
||||
# However, since `Parent.method` already violates LSP with `Grandparent.method`,
|
||||
# we don't report the same violation for `Child` -- it's inherited from `Parent`.
|
||||
def method(self, x: str) -> None: ...
|
||||
|
||||
class OtherChild(Parent):
|
||||
# compatible with the signature of `Grandparent.method`, but not with `Parent.method`:
|
||||
def method(self, x: int) -> None: ... # error: [invalid-method-override]
|
||||
|
||||
class ChildWithNewViolation(Parent):
|
||||
# incompatible with BOTH `Parent.method` (str) and `Grandparent.method` (int).
|
||||
# We report the violation against the immediate parent (`Parent`), not the grandparent.
|
||||
def method(self, x: bytes) -> None: ... # error: [invalid-method-override]
|
||||
|
||||
class GrandparentWithReturnType:
|
||||
def method(self) -> int: ...
|
||||
|
||||
class ParentWithReturnType(GrandparentWithReturnType):
|
||||
def method(self) -> str: ... # error: [invalid-method-override]
|
||||
|
||||
class ChildWithReturnType(ParentWithReturnType):
|
||||
# Returns `int` again -- compatible with `GrandparentWithReturnType.method`,
|
||||
# but not with `ParentWithReturnType.method`. We report against the immediate parent.
|
||||
def method(self) -> int: ... # error: [invalid-method-override]
|
||||
|
||||
class GradualParent(Grandparent):
|
||||
def method(self, x: Any) -> None: ...
|
||||
|
||||
@@ -190,8 +212,9 @@ class C(B):
|
||||
foo = get
|
||||
|
||||
class D(C):
|
||||
# compatible with `C.get` and `B.get`, but not with `A.get`
|
||||
def get(self, my_default): ... # error: [invalid-method-override]
|
||||
# compatible with `C.get` and `B.get`, but not with `A.get`.
|
||||
# Since `B.get` already violates LSP with `A.get`, we don't report for `D`.
|
||||
def get(self, my_default): ...
|
||||
```
|
||||
|
||||
## Non-generic methods on generic classes work as expected
|
||||
|
||||
@@ -22,21 +22,39 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
|
||||
7 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
8 |
|
||||
9 | class Child(Parent):
|
||||
10 | # compatible with the signature of `Parent.method`, but not with `Grandparent.method`:
|
||||
11 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
12 |
|
||||
13 | class OtherChild(Parent):
|
||||
14 | # compatible with the signature of `Grandparent.method`, but not with `Parent.method`:
|
||||
15 | def method(self, x: int) -> None: ... # error: [invalid-method-override]
|
||||
16 |
|
||||
17 | class GradualParent(Grandparent):
|
||||
18 | def method(self, x: Any) -> None: ...
|
||||
19 |
|
||||
20 | class ThirdChild(GradualParent):
|
||||
21 | # `GradualParent.method` is compatible with the signature of `Grandparent.method`,
|
||||
22 | # and `ThirdChild.method` is compatible with the signature of `GradualParent.method`,
|
||||
23 | # but `ThirdChild.method` is not compatible with the signature of `Grandparent.method`
|
||||
24 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
10 | # compatible with the signature of `Parent.method`, but not with `Grandparent.method`.
|
||||
11 | # However, since `Parent.method` already violates LSP with `Grandparent.method`,
|
||||
12 | # we don't report the same violation for `Child` -- it's inherited from `Parent`.
|
||||
13 | def method(self, x: str) -> None: ...
|
||||
14 |
|
||||
15 | class OtherChild(Parent):
|
||||
16 | # compatible with the signature of `Grandparent.method`, but not with `Parent.method`:
|
||||
17 | def method(self, x: int) -> None: ... # error: [invalid-method-override]
|
||||
18 |
|
||||
19 | class ChildWithNewViolation(Parent):
|
||||
20 | # incompatible with BOTH `Parent.method` (str) and `Grandparent.method` (int).
|
||||
21 | # We report the violation against the immediate parent (`Parent`), not the grandparent.
|
||||
22 | def method(self, x: bytes) -> None: ... # error: [invalid-method-override]
|
||||
23 |
|
||||
24 | class GrandparentWithReturnType:
|
||||
25 | def method(self) -> int: ...
|
||||
26 |
|
||||
27 | class ParentWithReturnType(GrandparentWithReturnType):
|
||||
28 | def method(self) -> str: ... # error: [invalid-method-override]
|
||||
29 |
|
||||
30 | class ChildWithReturnType(ParentWithReturnType):
|
||||
31 | # Returns `int` again -- compatible with `GrandparentWithReturnType.method`,
|
||||
32 | # but not with `ParentWithReturnType.method`. We report against the immediate parent.
|
||||
33 | def method(self) -> int: ... # error: [invalid-method-override]
|
||||
34 |
|
||||
35 | class GradualParent(Grandparent):
|
||||
36 | def method(self, x: Any) -> None: ...
|
||||
37 |
|
||||
38 | class ThirdChild(GradualParent):
|
||||
39 | # `GradualParent.method` is compatible with the signature of `Grandparent.method`,
|
||||
40 | # and `ThirdChild.method` is compatible with the signature of `GradualParent.method`,
|
||||
41 | # but `ThirdChild.method` is not compatible with the signature of `Grandparent.method`
|
||||
42 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
```
|
||||
|
||||
## other_stub.pyi
|
||||
@@ -56,8 +74,9 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
|
||||
12 | foo = get
|
||||
13 |
|
||||
14 | class D(C):
|
||||
15 | # compatible with `C.get` and `B.get`, but not with `A.get`
|
||||
16 | def get(self, my_default): ... # error: [invalid-method-override]
|
||||
15 | # compatible with `C.get` and `B.get`, but not with `A.get`.
|
||||
16 | # Since `B.get` already violates LSP with `A.get`, we don't report for `D`.
|
||||
17 | def get(self, my_default): ...
|
||||
```
|
||||
|
||||
# Diagnostics
|
||||
@@ -83,38 +102,14 @@ info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:11:9
|
||||
--> src/stub.pyi:17:9
|
||||
|
|
||||
9 | class Child(Parent):
|
||||
10 | # compatible with the signature of `Parent.method`, but not with `Grandparent.method`:
|
||||
11 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Grandparent.method`
|
||||
12 |
|
||||
13 | class OtherChild(Parent):
|
||||
|
|
||||
::: src/stub.pyi:4:9
|
||||
|
|
||||
3 | class Grandparent:
|
||||
4 | def method(self, x: int) -> None: ...
|
||||
| ---------------------------- `Grandparent.method` defined here
|
||||
5 |
|
||||
6 | class Parent(Grandparent):
|
||||
|
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:15:9
|
||||
|
|
||||
13 | class OtherChild(Parent):
|
||||
14 | # compatible with the signature of `Grandparent.method`, but not with `Parent.method`:
|
||||
15 | def method(self, x: int) -> None: ... # error: [invalid-method-override]
|
||||
15 | class OtherChild(Parent):
|
||||
16 | # compatible with the signature of `Grandparent.method`, but not with `Parent.method`:
|
||||
17 | def method(self, x: int) -> None: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
16 |
|
||||
17 | class GradualParent(Grandparent):
|
||||
18 |
|
||||
19 | class ChildWithNewViolation(Parent):
|
||||
|
|
||||
::: src/stub.pyi:7:9
|
||||
|
|
||||
@@ -131,11 +126,75 @@ info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:24:9
|
||||
--> src/stub.pyi:22:9
|
||||
|
|
||||
22 | # and `ThirdChild.method` is compatible with the signature of `GradualParent.method`,
|
||||
23 | # but `ThirdChild.method` is not compatible with the signature of `Grandparent.method`
|
||||
24 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
20 | # incompatible with BOTH `Parent.method` (str) and `Grandparent.method` (int).
|
||||
21 | # We report the violation against the immediate parent (`Parent`), not the grandparent.
|
||||
22 | def method(self, x: bytes) -> None: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Parent.method`
|
||||
23 |
|
||||
24 | class GrandparentWithReturnType:
|
||||
|
|
||||
::: src/stub.pyi:7:9
|
||||
|
|
||||
6 | class Parent(Grandparent):
|
||||
7 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
| ---------------------------- `Parent.method` defined here
|
||||
8 |
|
||||
9 | class Child(Parent):
|
||||
|
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:25:9
|
||||
|
|
||||
24 | class GrandparentWithReturnType:
|
||||
25 | def method(self) -> int: ...
|
||||
| ------------------- `GrandparentWithReturnType.method` defined here
|
||||
26 |
|
||||
27 | class ParentWithReturnType(GrandparentWithReturnType):
|
||||
28 | def method(self) -> str: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `GrandparentWithReturnType.method`
|
||||
29 |
|
||||
30 | class ChildWithReturnType(ParentWithReturnType):
|
||||
|
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:28:9
|
||||
|
|
||||
27 | class ParentWithReturnType(GrandparentWithReturnType):
|
||||
28 | def method(self) -> str: ... # error: [invalid-method-override]
|
||||
| ------------------- `ParentWithReturnType.method` defined here
|
||||
29 |
|
||||
30 | class ChildWithReturnType(ParentWithReturnType):
|
||||
31 | # Returns `int` again -- compatible with `GrandparentWithReturnType.method`,
|
||||
32 | # but not with `ParentWithReturnType.method`. We report against the immediate parent.
|
||||
33 | def method(self) -> int: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `ParentWithReturnType.method`
|
||||
34 |
|
||||
35 | class GradualParent(Grandparent):
|
||||
|
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `method`
|
||||
--> src/stub.pyi:42:9
|
||||
|
|
||||
40 | # and `ThirdChild.method` is compatible with the signature of `GradualParent.method`,
|
||||
41 | # but `ThirdChild.method` is not compatible with the signature of `Grandparent.method`
|
||||
42 | def method(self, x: str) -> None: ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Grandparent.method`
|
||||
|
|
||||
::: src/stub.pyi:4:9
|
||||
@@ -169,25 +228,3 @@ info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
```
|
||||
error[invalid-method-override]: Invalid override of method `get`
|
||||
--> src/other_stub.pyi:16:9
|
||||
|
|
||||
14 | class D(C):
|
||||
15 | # compatible with `C.get` and `B.get`, but not with `A.get`
|
||||
16 | def get(self, my_default): ... # error: [invalid-method-override]
|
||||
| ^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `A.get`
|
||||
|
|
||||
::: src/other_stub.pyi:2:9
|
||||
|
|
||||
1 | class A:
|
||||
2 | def get(self, default): ...
|
||||
| ------------------ `A.get` defined here
|
||||
3 |
|
||||
4 | class B(A):
|
||||
|
|
||||
info: This violates the Liskov Substitution Principle
|
||||
info: rule `invalid-method-override` is enabled by default
|
||||
|
||||
```
|
||||
|
||||
Reference in New Issue
Block a user