more review comments

Author: AlexWaygood

crates/ty/docs/rules.md

@@ -8,17 +8,17 @@ generally makes about types in Python:
 
 In order for a type checker's assumptions to be sound, it is crucial for the type checker to enforce
 the Liskov Substitution Principle on code that it checks. In practice, this usually manifests as
-three checks for a type checker to perform when it checks a subclass `B` of a class `A`:
+several checks for a type checker to perform when it checks a subclass `B` of a class `A`:
 
 1. Read-only attributes should only ever be overridden covariantly: if a property `A.p` resolves to
     `int` when accessed, accessing `B.p` should either resolve to `int` or a subtype of `int`.
-1. Method return types should only ever be overidden covariantly: if a method `A.f` returns `int`
+1. Method return types should only ever be overridden covariantly: if a method `A.f` returns `int`
     when called, calling `B.f` should also resolve to `int or a subtype of`int\`.
 1. Method parameters should only ever be overridden contravariantly: if a method `A.f` can be called
     with an argument of type `bool`, then the method `B.f` must also be callable with type `bool`
     (though it is permitted for the override to also accept other types)
 1. Mutable attributes should only ever be overridden invariantly: if a mutable attribute `A.attr`
-    resolves to type `str`, it can only be overidden on a subclass with exactly the same type.
+    resolves to type `str`, it can only be overridden on a subclass with exactly the same type.
 
 ## Method return types
 
@@ -53,7 +53,7 @@ class Sub1(Super):
     def method(self, x: int, /): ...  # fine
 
 class Sub2(Super):
-    def method(self, x: object, /): ...  # fine: `method` still accepts any argument of type `bool`
+    def method(self, x: object, /): ...  # fine: `method` still accepts any argument of type `int`
 
 class Sub4(Super):
     def method(self, x: int | str, /): ...  # fine
@@ -87,7 +87,7 @@ class Sub12(Super):
 
 class Sub13(Super):
     # Some calls permitted by the superclass are now no longer allowed
-    # (the method can no longer be passed with exactly one argument!)
+    # (the method can no longer be passed exactly one argument!)
     def method(self, x, y, /): ...  # error: [invalid-method-override]
 
 class Sub14(Super):
@@ -464,8 +464,6 @@ source-code definitions. There are several scenarios to consider here:
     subclass
 1. A "normal" method on a superclass is overridden by a synthesized method on a subclass
 1. A synthesized method on a superclass is overridden by a synthesized method on a subclass
-1. No methods are overridden, but the Liskov Substitution Principle is arguably violated anyway
-    because a generated method on a superclass is incompatible with subclassing(!)
 
 <!-- snapshot-diagnostics -->
 
@@ -490,13 +488,22 @@ class Bar(Foo):
 class Bar2(Foo):
     y: str
 
-# TODO: Although this class does not override any methods of `Foo`, it nonetheless
-# arguably violates the Liskov Substitution Principle. Instances of `Bar3` cannot be
-# substituted wherever an instance of `Foo` is expected, because the generated
-# `__lt__` method on `Foo` raises an error unless the r.h.s. and `l.h.s.` have exactly
-# the same `__class__` (it does not permit instances of `Foo` to be compared with
-# instances of subclasses of `Foo`). We could therefore consider treating all
-# `order=True` dataclasses as implicitly `@final` in order to enforce Liskov soundness.
+# TODO: Although this class does not override any methods of `Foo`, the design of the
+# `order=True` stdlib dataclasses feature itself arguably violates the Liskov Substitution
+# Principle! Instances of `Bar3` cannot be substituted wherever an instance of `Foo` is
+# expected, because the generated `__lt__` method on `Foo` raises an error unless the r.h.s.
+# and `l.h.s.` have exactly the same `__class__` (it does not permit instances of `Foo` to
+# be compared with instances of subclasses of `Foo`).
+#
+# Many users would probably like their type checkers to alert them to cases where instances
+# of subclasses cannot be substituted for instances of superclasses, as this violates many
+# assumptions a type checker will make and makes it likely that a type checker will fail to
+# catch type errors elsewhere in the user's code. We could therefore consider treating all
+# `order=True` dataclasses as implicitly `@final` in order to enforce soundness. However,
+# this probably shouldn't be reported with the same error code as Liskov violations, since
+# the error does not stem from any method signatures written by the user. The example is
+# only included here for completeness.
+#
 # Note that no other type checker catches this error as of 2025-11-21.
 class Bar3(Foo): ...
 

crates/ty_python_semantic/resources/mdtest/liskov.md

@@ -19,7 +19,7 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
  5 |     def method(self, x: int, /): ...  # fine
  6 | 
  7 | class Sub2(Super):
- 8 |     def method(self, x: object, /): ...  # fine: `method` still accepts any argument of type `bool`
+ 8 |     def method(self, x: object, /): ...  # fine: `method` still accepts any argument of type `int`
  9 | 
 10 | class Sub4(Super):
 11 |     def method(self, x: int | str, /): ...  # fine
@@ -53,7 +53,7 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
 39 | 
 40 | class Sub13(Super):
 41 |     # Some calls permitted by the superclass are now no longer allowed
-42 |     # (the method can no longer be passed with exactly one argument!)
+42 |     # (the method can no longer be passed exactly one argument!)
 43 |     def method(self, x, y, /): ...  # error: [invalid-method-override]
 44 | 
 45 | class Sub14(Super):
@@ -131,7 +131,7 @@ error[invalid-method-override]: Invalid override of method `method`
   --> src/mdtest_snippet.pyi:43:9
    |
 41 |     # Some calls permitted by the superclass are now no longer allowed
-42 |     # (the method can no longer be passed with exactly one argument!)
+42 |     # (the method can no longer be passed exactly one argument!)
 43 |     def method(self, x, y, /): ...  # error: [invalid-method-override]
    |         ^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Super.method`
 44 |

crates/ty_python_semantic/resources/mdtest/snapshots/liskov.md_-_The_Liskov_Substitut…_-_Method_parameters_(d98059266bcc1e13).snap

@@ -32,31 +32,40 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md
 18 | class Bar2(Foo):
 19 |     y: str
 20 | 
-21 | # TODO: Although this class does not override any methods of `Foo`, it nonetheless
-22 | # arguably violates the Liskov Substitution Principle. Instances of `Bar3` cannot be
-23 | # substituted wherever an instance of `Foo` is expected, because the generated
-24 | # `__lt__` method on `Foo` raises an error unless the r.h.s. and `l.h.s.` have exactly
-25 | # the same `__class__` (it does not permit instances of `Foo` to be compared with
-26 | # instances of subclasses of `Foo`). We could therefore consider treating all
-27 | # `order=True` dataclasses as implicitly `@final` in order to enforce Liskov soundness.
-28 | # Note that no other type checker catches this error as of 2025-11-21.
-29 | class Bar3(Foo): ...
-30 | 
-31 | class Eggs:
-32 |     def __lt__(self, other: Eggs) -> bool: ...
-33 | 
-34 | # TODO: the generated `Ham.__lt__` method here incompatibly overrides `Eggs.__lt__`.
-35 | # We could consider emitting a diagnostic here. As of 2025-11-21, mypy reports a
-36 | # diagnostic here but pyright and pyrefly do not.
-37 | @dataclass(order=True)
-38 | class Ham(Eggs):
-39 |     x: int
-40 | 
-41 | class Baz(NamedTuple):
-42 |     x: int
-43 | 
-44 | class Spam(Baz):
-45 |     def _asdict(self) -> tuple[int, ...]: ...  # error: [invalid-method-override]
+21 | # TODO: Although this class does not override any methods of `Foo`, the design of the
+22 | # `order=True` stdlib dataclasses feature itself arguably violates the Liskov Substitution
+23 | # Principle! Instances of `Bar3` cannot be substituted wherever an instance of `Foo` is
+24 | # expected, because the generated `__lt__` method on `Foo` raises an error unless the r.h.s.
+25 | # and `l.h.s.` have exactly the same `__class__` (it does not permit instances of `Foo` to
+26 | # be compared with instances of subclasses of `Foo`).
+27 | #
+28 | # Many users would probably like their type checkers to alert them to cases where instances
+29 | # of subclasses cannot be substituted for instances of superclasses, as this violates many
+30 | # assumptions a type checker will make and makes it likely that a type checker will fail to
+31 | # catch type errors elsewhere in the user's code. We could therefore consider treating all
+32 | # `order=True` dataclasses as implicitly `@final` in order to enforce soundness. However,
+33 | # this probably shouldn't be reported with the same error code as Liskov violations, since
+34 | # the error does not stem from any method signatures written by the user. The example is
+35 | # only included here for completeness.
+36 | #
+37 | # Note that no other type checker catches this error as of 2025-11-21.
+38 | class Bar3(Foo): ...
+39 | 
+40 | class Eggs:
+41 |     def __lt__(self, other: Eggs) -> bool: ...
+42 | 
+43 | # TODO: the generated `Ham.__lt__` method here incompatibly overrides `Eggs.__lt__`.
+44 | # We could consider emitting a diagnostic here. As of 2025-11-21, mypy reports a
+45 | # diagnostic here but pyright and pyrefly do not.
+46 | @dataclass(order=True)
+47 | class Ham(Eggs):
+48 |     x: int
+49 | 
+50 | class Baz(NamedTuple):
+51 |     x: int
+52 | 
+53 | class Spam(Baz):
+54 |     def _asdict(self) -> tuple[int, ...]: ...  # error: [invalid-method-override]
 ```
 
 # Diagnostics
@@ -86,21 +95,21 @@ info: rule `invalid-method-override` is enabled by default
 
 ```
 error[invalid-method-override]: Invalid override of method `_asdict`
-  --> src/mdtest_snippet.pyi:45:9
+  --> src/mdtest_snippet.pyi:54:9
    |
-44 | class Spam(Baz):
-45 |     def _asdict(self) -> tuple[int, ...]: ...  # error: [invalid-method-override]
+53 | class Spam(Baz):
+54 |     def _asdict(self) -> tuple[int, ...]: ...  # error: [invalid-method-override]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Baz._asdict`
    |
 info: This violates the Liskov Substitution Principle
 info: `Baz._asdict` is a generated method created because `Baz` inherits from `typing.NamedTuple`
-  --> src/mdtest_snippet.pyi:41:7
+  --> src/mdtest_snippet.pyi:50:7
    |
-39 |     x: int
-40 |
-41 | class Baz(NamedTuple):
+48 |     x: int
+49 |
+50 | class Baz(NamedTuple):
    |       ^^^^^^^^^^^^^^^ Definition of `Baz`
-42 |     x: int
+51 |     x: int
    |
 info: rule `invalid-method-override` is enabled by default