From 4e4b08c1ee384f6946aae5c358e39cd7bb8e424d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 30 Nov 2025 11:04:59 -0500 Subject: [PATCH] Emit a diagnostic for subclassing with order=True --- crates/ty/docs/rules.md | 193 +++++++++++------- .../mdtest/dataclasses/dataclasses.md | 43 ++++ .../resources/mdtest/liskov.md | 31 +-- .../resources/mdtest/override.md | 2 +- ..._Synthesized_methods_(9e6e6c7368530460).snap | 141 ++++++++----- .../src/types/diagnostic.rs | 40 ++++ .../src/types/infer/builder.rs | 37 +++- .../e2e__commands__debug_command.snap | 1 + ty.schema.json | 10 + 9 files changed, 336 insertions(+), 162 deletions(-) diff --git a/crates/ty/docs/rules.md b/crates/ty/docs/rules.md index 78f757a6bb..c289f5986c 100644 --- a/crates/ty/docs/rules.md +++ b/crates/ty/docs/rules.md @@ -39,7 +39,7 @@ def test(): -> "int": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -63,7 +63,7 @@ Calling a non-callable object will raise a `TypeError` at runtime. Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -95,7 +95,7 @@ f(int) # error Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -126,7 +126,7 @@ a = 1 Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -158,7 +158,7 @@ class C(A, B): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -190,7 +190,7 @@ class B(A): ... Default level: error · Preview (since 1.0.0) · Related issues · -View source +View source @@ -218,7 +218,7 @@ type B = A Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -245,7 +245,7 @@ class B(A, A): ... Default level: error · Added in 0.0.1-alpha.12 · Related issues · -View source +View source @@ -357,7 +357,7 @@ def test(): -> "Literal[5]": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -387,7 +387,7 @@ class C(A, B): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -413,7 +413,7 @@ t[3] # IndexError: tuple index out of range Default level: error · Added in 0.0.1-alpha.12 · Related issues · -View source +View source @@ -502,7 +502,7 @@ an atypical memory layout. Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -529,7 +529,7 @@ func("foo") # error: [invalid-argument-type] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -557,7 +557,7 @@ a: int = '' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -591,7 +591,7 @@ C.instance_var = 3 # error: Cannot assign to instance variable Default level: error · Added in 0.0.1-alpha.19 · Related issues · -View source +View source @@ -627,7 +627,7 @@ asyncio.run(main()) Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -651,7 +651,7 @@ class A(42): ... # error: [invalid-base] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -678,7 +678,7 @@ with 1: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -707,7 +707,7 @@ a: str Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -751,7 +751,7 @@ except ZeroDivisionError: Default level: error · Added in 0.0.1-alpha.28 · Related issues · -View source +View source @@ -793,7 +793,7 @@ class D(A): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -826,7 +826,7 @@ class C[U](Generic[T]): ... Default level: error · Added in 0.0.1-alpha.17 · Related issues · -View source +View source @@ -865,7 +865,7 @@ carol = Person(name="Carol", age=25) # typo! Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -900,7 +900,7 @@ def f(t: TypeVar("U")): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -934,7 +934,7 @@ class B(metaclass=f): ... Default level: error · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -1041,7 +1041,7 @@ Correct use of `@override` is enforced by ty's `invalid-explicit-override` rule. Default level: error · Added in 0.0.1-alpha.19 · Related issues · -View source +View source @@ -1073,7 +1073,7 @@ TypeError: can only inherit from a NamedTuple type and Generic Default level: error · Preview (since 1.0.0) · Related issues · -View source +View source @@ -1103,7 +1103,7 @@ Baz = NewType("Baz", int | str) # error: invalid base for `typing.NewType` Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1153,7 +1153,7 @@ def foo(x: int) -> int: ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1179,7 +1179,7 @@ def f(a: int = ''): ... Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1210,7 +1210,7 @@ P2 = ParamSpec("S2") # error: ParamSpec name must match the variable it's assig Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1244,7 +1244,7 @@ TypeError: Protocols can only inherit from other protocols, got Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1293,7 +1293,7 @@ def g(): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1318,7 +1318,7 @@ def func() -> int: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1376,7 +1376,7 @@ TODO #14889 Default level: error · Added in 0.0.1-alpha.6 · Related issues · -View source +View source @@ -1403,7 +1403,7 @@ NewAlias = TypeAliasType(get_name(), int) # error: TypeAliasType name mus Default level: error · Added in 0.0.1-alpha.29 · Related issues · -View source +View source @@ -1450,7 +1450,7 @@ Bar[int] # error: too few arguments Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1480,7 +1480,7 @@ TYPE_CHECKING = '' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1510,7 +1510,7 @@ b: Annotated[int] # `Annotated` expects at least two arguments Default level: error · Added in 0.0.1-alpha.11 · Related issues · -View source +View source @@ -1544,7 +1544,7 @@ f(10) # Error Default level: error · Added in 0.0.1-alpha.11 · Related issues · -View source +View source @@ -1578,7 +1578,7 @@ class C: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1613,7 +1613,7 @@ T = TypeVar('T', bound=str) # valid bound TypeVar Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1638,7 +1638,7 @@ func() # TypeError: func() missing 1 required positional argument: 'x' Default level: error · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -1671,7 +1671,7 @@ alice["age"] # KeyError Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1700,7 +1700,7 @@ func("string") # error: [no-matching-overload] Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1724,7 +1724,7 @@ Subscripting an object that does not support it will raise a `TypeError` at runt Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1750,7 +1750,7 @@ for i in 34: # TypeError: 'int' object is not iterable Default level: error · Added in 0.0.1-alpha.29 · Related issues · -View source +View source @@ -1783,7 +1783,7 @@ class B(A): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1810,7 +1810,7 @@ f(1, x=2) # Error raised here Default level: error · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -1868,7 +1868,7 @@ def test(): -> "int": Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1898,7 +1898,7 @@ static_assert(int(2.0 * 3.0) == 6) # error: does not have a statically known tr Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1927,7 +1927,7 @@ class B(A): ... # Error raised here Default level: error · Preview (since 0.0.1-alpha.30) · Related issues · -View source +View source @@ -1961,7 +1961,7 @@ class F(NamedTuple): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -1988,7 +1988,7 @@ f("foo") # Error raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2016,7 +2016,7 @@ def _(x: int): Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2062,7 +2062,7 @@ class A: Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2089,7 +2089,7 @@ f(x=1, y=2) # Error raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2117,7 +2117,7 @@ A().foo # AttributeError: 'A' object has no attribute 'foo' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2142,7 +2142,7 @@ import foo # ModuleNotFoundError: No module named 'foo' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2167,7 +2167,7 @@ print(x) # NameError: name 'x' is not defined Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2204,7 +2204,7 @@ b1 < b2 < b1 # exception raised here Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2232,7 +2232,7 @@ A() + A() # TypeError: unsupported operand type(s) for +: 'A' and 'A' Default level: error · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2257,7 +2257,7 @@ l[1:10:0] # ValueError: slice step cannot be zero Default level: warn · Added in 0.0.1-alpha.20 · Related issues · -View source +View source @@ -2298,7 +2298,7 @@ class SubProto(BaseProto, Protocol): Default level: warn · Added in 0.0.1-alpha.16 · Related issues · -View source +View source @@ -2386,7 +2386,7 @@ a = 20 / 0 # type: ignore Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2414,7 +2414,7 @@ A.c # AttributeError: type object 'A' has no attribute 'c' Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2446,7 +2446,7 @@ A()[0] # TypeError: 'A' object is not subscriptable Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2478,7 +2478,7 @@ from module import a # ImportError: cannot import name 'a' from 'module' Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2499,13 +2499,58 @@ def f() -> int: cast(int, f()) # Redundant ``` +## `subclass-of-dataclass-with-order` + + +Default level: warn · +Preview (since 0.0.1-alpha.30) · +Related issues · +View source + + + +**What it does** + +Checks for classes that inherit from a dataclass with `order=True`. + +**Why is this bad?** + +When a dataclass has `order=True`, comparison methods (`__lt__`, `__le__`, `__gt__`, `__ge__`) +are generated that compare instances as tuples of their fields. These methods raise a +`TypeError` at runtime when comparing instances of different classes in the inheritance +hierarchy, even if one is a subclass of the other. + +This violates the [Liskov Substitution Principle] because child class instances cannot be +used in all contexts where parent class instances are expected. + +**Example** + + +```python +from dataclasses import dataclass + +@dataclass(order=True) +class Parent: + value: int + +class Child(Parent): # Error raised here + pass + +# At runtime, this raises TypeError: +# Child(1) < Parent(2) +``` + +Consider using `functools.total_ordering` instead, which does not have this limitation. + +[Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle + ## `undefined-reveal` Default level: warn · Added in 0.0.1-alpha.1 · Related issues · -View source +View source @@ -2529,7 +2574,7 @@ reveal_type(1) # NameError: name 'reveal_type' is not defined Default level: warn · Added in 0.0.1-alpha.15 · Related issues · -View source +View source @@ -2587,7 +2632,7 @@ def g(): Default level: warn · Added in 0.0.1-alpha.7 · Related issues · -View source +View source @@ -2626,7 +2671,7 @@ class D(C): ... # error: [unsupported-base] Default level: warn · Added in 0.0.1-alpha.22 · Related issues · -View source +View source @@ -2689,7 +2734,7 @@ def foo(x: int | str) -> int | str: Default level: ignore · Preview (since 0.0.1-alpha.1) · Related issues · -View source +View source @@ -2713,7 +2758,7 @@ Dividing by zero raises a `ZeroDivisionError` at runtime. Default level: ignore · Added in 0.0.1-alpha.1 · Related issues · -View source +View source diff --git a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md index ba5151fa61..fc45811cbc 100644 --- a/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md +++ b/crates/ty_python_semantic/resources/mdtest/dataclasses/dataclasses.md @@ -349,6 +349,49 @@ GenericWithOrder[int](1) < GenericWithOrder[int](1) GenericWithOrder[int](1) < GenericWithOrder[str]("a") # error: [unsupported-operator] ``` +Subclassing a dataclass with `order=True` is problematic because comparing instances of different +classes in the inheritance hierarchy will raise a `TypeError` at runtime. This violates the Liskov +Substitution Principle: + +```py +from dataclasses import dataclass + +@dataclass(order=True) +class Parent: + value: int + +class Child(Parent): # error: [subclass-of-dataclass-with-order] + pass + +# The comparison methods generated by @dataclass(order=True) compare instances +# as tuples of their fields. At runtime, this raises TypeError when comparing +# instances of different classes in the hierarchy: +# Child(42) < Parent(42) # TypeError! +``` + +This also applies when the child class is also a dataclass: + +```py +@dataclass(order=True) +class OrderedParent: + x: int + +@dataclass +class OrderedChild(OrderedParent): # error: [subclass-of-dataclass-with-order] + y: str +``` + +If the parent dataclass does not have `order=True`, no warning is emitted: + +```py +@dataclass +class UnorderedParent: + x: int + +class UnorderedChild(UnorderedParent): # No warning + pass +``` + If a class already defines one of the comparison methods, a `TypeError` is raised at runtime. Ideally, we would emit a diagnostic in that case: diff --git a/crates/ty_python_semantic/resources/mdtest/liskov.md b/crates/ty_python_semantic/resources/mdtest/liskov.md index 24a1c3e2c2..8d5cd454a4 100644 --- a/crates/ty_python_semantic/resources/mdtest/liskov.md +++ b/crates/ty_python_semantic/resources/mdtest/liskov.md @@ -475,37 +475,24 @@ from typing import NamedTuple class Foo: x: int -class Bar(Foo): +class Bar(Foo): # error: [subclass-of-dataclass-with-order] def __lt__(self, other: Bar) -> bool: ... # error: [invalid-method-override] -# TODO: specifying `order=True` on the subclass means that a `__lt__` method is -# generated that is incompatible with the generated `__lt__` method on the superclass. -# We could consider detecting this and emitting a diagnostic, though maybe it shouldn't -# be `invalid-method-override` since we'd emit it on the class definition rather than -# on any method definition. Note also that no other type checker complains about this -# as of 2025-11-21. +# Specifying `order=True` on the subclass means that a `__lt__` method is generated that +# is incompatible with the generated `__lt__` method on the superclass. We emit a diagnostic +# on the class definition because the design of `order=True` dataclasses themselves violates +# the Liskov Substitution Principle. @dataclass(order=True) -class Bar2(Foo): +class Bar2(Foo): # error: [subclass-of-dataclass-with-order] y: str -# 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 +# Although this class does not override any methods of `Foo`, the design of the +# `order=True` stdlib dataclasses feature itself 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): ... +class Bar3(Foo): ... # error: [subclass-of-dataclass-with-order] class Eggs: def __lt__(self, other: Eggs) -> bool: ... diff --git a/crates/ty_python_semantic/resources/mdtest/override.md b/crates/ty_python_semantic/resources/mdtest/override.md index c1ab23d790..1ccdac08dc 100644 --- a/crates/ty_python_semantic/resources/mdtest/override.md +++ b/crates/ty_python_semantic/resources/mdtest/override.md @@ -275,7 +275,7 @@ from dataclasses import dataclass class ParentDataclass: x: int -class Child(ParentDataclass): +class Child(ParentDataclass): # error: [subclass-of-dataclass-with-order] @override def __lt__(self, other: ParentDataclass) -> bool: ... # fine diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/liskov.md_-_The_Liskov_Substitut…_-_Synthesized_methods_(9e6e6c7368530460).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/liskov.md_-_The_Liskov_Substitut…_-_Synthesized_methods_(9e6e6c7368530460).snap index f50b1bd308..09c09edaee 100644 --- a/crates/ty_python_semantic/resources/mdtest/snapshots/liskov.md_-_The_Liskov_Substitut…_-_Synthesized_methods_(9e6e6c7368530460).snap +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/liskov.md_-_The_Liskov_Substitut…_-_Synthesized_methods_(9e6e6c7368530460).snap @@ -1,5 +1,6 @@ --- source: crates/ty_test/src/lib.rs +assertion_line: 523 expression: snapshot --- --- @@ -19,66 +20,67 @@ mdtest path: crates/ty_python_semantic/resources/mdtest/liskov.md 5 | class Foo: 6 | x: int 7 | - 8 | class Bar(Foo): + 8 | class Bar(Foo): # error: [subclass-of-dataclass-with-order] 9 | def __lt__(self, other: Bar) -> bool: ... # error: [invalid-method-override] 10 | -11 | # TODO: specifying `order=True` on the subclass means that a `__lt__` method is -12 | # generated that is incompatible with the generated `__lt__` method on the superclass. -13 | # We could consider detecting this and emitting a diagnostic, though maybe it shouldn't -14 | # be `invalid-method-override` since we'd emit it on the class definition rather than -15 | # on any method definition. Note also that no other type checker complains about this -16 | # as of 2025-11-21. -17 | @dataclass(order=True) -18 | class Bar2(Foo): -19 | y: str -20 | -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): ... +11 | # Specifying `order=True` on the subclass means that a `__lt__` method is generated that +12 | # is incompatible with the generated `__lt__` method on the superclass. We emit a diagnostic +13 | # on the class definition because the design of `order=True` dataclasses themselves violates +14 | # the Liskov Substitution Principle. +15 | @dataclass(order=True) +16 | class Bar2(Foo): # error: [subclass-of-dataclass-with-order] +17 | y: str +18 | +19 | # Although this class does not override any methods of `Foo`, the design of the +20 | # `order=True` stdlib dataclasses feature itself violates the Liskov Substitution +21 | # Principle! Instances of `Bar3` cannot be substituted wherever an instance of `Foo` is +22 | # expected, because the generated `__lt__` method on `Foo` raises an error unless the r.h.s. +23 | # and `l.h.s.` have exactly the same `__class__` (it does not permit instances of `Foo` to +24 | # be compared with instances of subclasses of `Foo`). +25 | class Bar3(Foo): ... # error: [subclass-of-dataclass-with-order] +26 | +27 | class Eggs: +28 | def __lt__(self, other: Eggs) -> bool: ... +29 | +30 | # TODO: the generated `Ham.__lt__` method here incompatibly overrides `Eggs.__lt__`. +31 | # We could consider emitting a diagnostic here. As of 2025-11-21, mypy reports a +32 | # diagnostic here but pyright and pyrefly do not. +33 | @dataclass(order=True) +34 | class Ham(Eggs): +35 | x: int +36 | +37 | class Baz(NamedTuple): +38 | x: int 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] +40 | class Spam(Baz): +41 | def _asdict(self) -> tuple[int, ...]: ... # error: [invalid-method-override] ``` # Diagnostics +``` +warning[subclass-of-dataclass-with-order]: Class `Bar` inherits from dataclass `Foo` which has `order=True` + --> src/mdtest_snippet.pyi:8:11 + | +6 | x: int +7 | +8 | class Bar(Foo): # error: [subclass-of-dataclass-with-order] + | ^^^ +9 | def __lt__(self, other: Bar) -> bool: ... # error: [invalid-method-override] + | +info: rule `subclass-of-dataclass-with-order` is enabled by default + +``` + ``` error[invalid-method-override]: Invalid override of method `__lt__` --> src/mdtest_snippet.pyi:9:9 | - 8 | class Bar(Foo): + 8 | class Bar(Foo): # error: [subclass-of-dataclass-with-order] 9 | def __lt__(self, other: Bar) -> bool: ... # error: [invalid-method-override] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Definition is incompatible with `Foo.__lt__` 10 | -11 | # TODO: specifying `order=True` on the subclass means that a `__lt__` method is +11 | # Specifying `order=True` on the subclass means that a `__lt__` method is generated that | info: This violates the Liskov Substitution Principle info: `Foo.__lt__` is a generated method created because `Foo` is a dataclass @@ -94,22 +96,51 @@ info: rule `invalid-method-override` is enabled by default ``` ``` -error[invalid-method-override]: Invalid override of method `_asdict` - --> src/mdtest_snippet.pyi:54:9 +warning[subclass-of-dataclass-with-order]: Class `Bar2` inherits from dataclass `Foo` which has `order=True` + --> src/mdtest_snippet.pyi:16:12 | -53 | class Spam(Baz): -54 | def _asdict(self) -> tuple[int, ...]: ... # error: [invalid-method-override] +14 | # the Liskov Substitution Principle. +15 | @dataclass(order=True) +16 | class Bar2(Foo): # error: [subclass-of-dataclass-with-order] + | ^^^ +17 | y: str + | +info: rule `subclass-of-dataclass-with-order` is enabled by default + +``` + +``` +warning[subclass-of-dataclass-with-order]: Class `Bar3` inherits from dataclass `Foo` which has `order=True` + --> src/mdtest_snippet.pyi:25:12 + | +23 | # and `l.h.s.` have exactly the same `__class__` (it does not permit instances of `Foo` to +24 | # be compared with instances of subclasses of `Foo`). +25 | class Bar3(Foo): ... # error: [subclass-of-dataclass-with-order] + | ^^^ +26 | +27 | class Eggs: + | +info: rule `subclass-of-dataclass-with-order` is enabled by default + +``` + +``` +error[invalid-method-override]: Invalid override of method `_asdict` + --> src/mdtest_snippet.pyi:41:9 + | +40 | class Spam(Baz): +41 | 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:50:7 + --> src/mdtest_snippet.pyi:37:7 | -48 | x: int -49 | -50 | class Baz(NamedTuple): +35 | x: int +36 | +37 | class Baz(NamedTuple): | ^^^^^^^^^^^^^^^ Definition of `Baz` -51 | x: int +38 | x: int | info: rule `invalid-method-override` is enabled by default diff --git a/crates/ty_python_semantic/src/types/diagnostic.rs b/crates/ty_python_semantic/src/types/diagnostic.rs index 8e92c495cf..f7e95d021a 100644 --- a/crates/ty_python_semantic/src/types/diagnostic.rs +++ b/crates/ty_python_semantic/src/types/diagnostic.rs @@ -122,6 +122,7 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) { registry.register_lint(&INVALID_METHOD_OVERRIDE); registry.register_lint(&INVALID_EXPLICIT_OVERRIDE); registry.register_lint(&SUPER_CALL_IN_NAMED_TUPLE_METHOD); + registry.register_lint(&SUBCLASS_OF_DATACLASS_WITH_ORDER); // String annotations registry.register_lint(&BYTE_STRING_TYPE_ANNOTATION); @@ -1643,6 +1644,45 @@ declare_lint! { } } +declare_lint! { + /// ## What it does + /// Checks for classes that inherit from a dataclass with `order=True`. + /// + /// ## Why is this bad? + /// When a dataclass has `order=True`, comparison methods (`__lt__`, `__le__`, `__gt__`, `__ge__`) + /// are generated that compare instances as tuples of their fields. These methods raise a + /// `TypeError` at runtime when comparing instances of different classes in the inheritance + /// hierarchy, even if one is a subclass of the other. + /// + /// This violates the [Liskov Substitution Principle] because child class instances cannot be + /// used in all contexts where parent class instances are expected. + /// + /// ## Example + /// + /// ```python + /// from dataclasses import dataclass + /// + /// @dataclass(order=True) + /// class Parent: + /// value: int + /// + /// class Child(Parent): # Error raised here + /// pass + /// + /// # At runtime, this raises TypeError: + /// # Child(1) < Parent(2) + /// ``` + /// + /// Consider using `functools.total_ordering` instead, which does not have this limitation. + /// + /// [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle + pub(crate) static SUBCLASS_OF_DATACLASS_WITH_ORDER = { + summary: "detects subclasses of dataclasses with `order=True`", + status: LintStatus::preview("0.0.1-alpha.30"), + default_level: Level::Warn, + } +} + declare_lint! { /// ## What it does /// Checks for methods that are decorated with `@override` but do not override any method in a superclass. diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index 4c97ad8c60..d7f0a28afd 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -63,9 +63,9 @@ use crate::types::diagnostic::{ INVALID_PARAMETER_DEFAULT, INVALID_PARAMSPEC, INVALID_PROTOCOL, INVALID_TYPE_ARGUMENTS, INVALID_TYPE_FORM, INVALID_TYPE_GUARD_CALL, INVALID_TYPE_VARIABLE_CONSTRAINTS, IncompatibleBases, NON_SUBSCRIPTABLE, POSSIBLY_MISSING_ATTRIBUTE, - POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT, SUBCLASS_OF_FINAL_CLASS, - UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL, UNRESOLVED_IMPORT, - UNRESOLVED_REFERENCE, UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY, + POSSIBLY_MISSING_IMPLICIT_CALL, POSSIBLY_MISSING_IMPORT, SUBCLASS_OF_DATACLASS_WITH_ORDER, + SUBCLASS_OF_FINAL_CLASS, UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE, UNRESOLVED_GLOBAL, + UNRESOLVED_IMPORT, UNRESOLVED_REFERENCE, UNSUPPORTED_OPERATOR, USELESS_OVERLOAD_BODY, hint_if_stdlib_attribute_exists_on_other_versions, hint_if_stdlib_submodule_exists_on_other_versions, report_attempted_protocol_instantiation, report_bad_dunder_set_call, report_cannot_pop_required_field_on_typed_dict, @@ -103,13 +103,14 @@ use crate::types::typed_dict::{ use crate::types::visitor::any_over_type; use crate::types::{ CallDunderError, CallableBinding, CallableType, CallableTypes, ClassLiteral, ClassType, - DataclassParams, DynamicType, InternedType, IntersectionBuilder, IntersectionType, KnownClass, - KnownInstanceType, LintDiagnosticGuard, MemberLookupPolicy, MetaclassCandidate, - PEP695TypeAliasType, ParameterForm, SpecialFormType, SubclassOfType, TrackedConstraintSet, - Truthiness, Type, TypeAliasType, TypeAndQualifiers, TypeContext, TypeQualifiers, - TypeVarBoundOrConstraints, TypeVarBoundOrConstraintsEvaluation, TypeVarDefaultEvaluation, - TypeVarIdentity, TypeVarInstance, TypeVarKind, TypeVarVariance, TypedDictType, UnionBuilder, - UnionType, UnionTypeInstance, binding_type, infer_scope_types, overrides, todo_type, + DataclassFlags, DataclassParams, DynamicType, InternedType, IntersectionBuilder, + IntersectionType, KnownClass, KnownInstanceType, LintDiagnosticGuard, MemberLookupPolicy, + MetaclassCandidate, PEP695TypeAliasType, ParameterForm, SpecialFormType, SubclassOfType, + TrackedConstraintSet, Truthiness, Type, TypeAliasType, TypeAndQualifiers, TypeContext, + TypeQualifiers, TypeVarBoundOrConstraints, TypeVarBoundOrConstraintsEvaluation, + TypeVarDefaultEvaluation, TypeVarIdentity, TypeVarInstance, TypeVarKind, TypeVarVariance, + TypedDictType, UnionBuilder, UnionType, UnionTypeInstance, binding_type, infer_scope_types, + overrides, todo_type, }; use crate::types::{ClassBase, add_inferred_python_version_hint_to_diagnostic}; use crate::unpack::{EvaluationMode, UnpackPosition}; @@ -743,6 +744,22 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { )); } } + + let (base_class_literal, _) = base_class.class_literal(self.db()); + if let Some(base_params) = base_class_literal.dataclass_params(self.db()) { + if base_params.flags(self.db()).contains(DataclassFlags::ORDER) { + if let Some(builder) = self + .context + .report_lint(&SUBCLASS_OF_DATACLASS_WITH_ORDER, &class_node.bases()[i]) + { + builder.into_diagnostic(format_args!( + "Class `{}` inherits from dataclass `{}` which has `order=True`", + class.name(self.db()), + base_class.name(self.db()), + )); + } + } + } } // (4) Check that the class's MRO is resolvable diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap b/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap index 0daa6c768a..40593fe663 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap @@ -92,6 +92,7 @@ Settings: Settings { "raw-string-type-annotation": Error (Default), "redundant-cast": Warning (Default), "static-assert-error": Error (Default), + "subclass-of-dataclass-with-order": Warning (Default), "subclass-of-final-class": Error (Default), "super-call-in-named-tuple-method": Error (Default), "too-many-positional-arguments": Error (Default), diff --git a/ty.schema.json b/ty.schema.json index e2325d6ac4..da8ddf4f91 100644 --- a/ty.schema.json +++ b/ty.schema.json @@ -963,6 +963,16 @@ } ] }, + "subclass-of-dataclass-with-order": { + "title": "detects subclasses of dataclasses with `order=True`", + "description": "## What it does\nChecks for classes that inherit from a dataclass with `order=True`.\n\n## Why is this bad?\nWhen a dataclass has `order=True`, comparison methods (`__lt__`, `__le__`, `__gt__`, `__ge__`)\nare generated that compare instances as tuples of their fields. These methods raise a\n`TypeError` at runtime when comparing instances of different classes in the inheritance\nhierarchy, even if one is a subclass of the other.\n\nThis violates the [Liskov Substitution Principle] because child class instances cannot be\nused in all contexts where parent class instances are expected.\n\n## Example\n\n```python\nfrom dataclasses import dataclass\n\n@dataclass(order=True)\nclass Parent:\n value: int\n\nclass Child(Parent): # Error raised here\n pass\n\n# At runtime, this raises TypeError:\n# Child(1) < Parent(2)\n```\n\nConsider using `functools.total_ordering` instead, which does not have this limitation.\n\n[Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle", + "default": "warn", + "oneOf": [ + { + "$ref": "#/definitions/Level" + } + ] + }, "subclass-of-final-class": { "title": "detects subclasses of final classes", "description": "## What it does\nChecks for classes that subclass final classes.\n\n## Why is this bad?\nDecorating a class with `@final` declares to the type checker that it should not be subclassed.\n\n## Example\n\n```python\nfrom typing import final\n\n@final\nclass A: ...\nclass B(A): ... # Error raised here\n```",