-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Emit a diagnostic for subclassing with order=True
#21704
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
base: main
Are you sure you want to change the base?
Conversation
order=Trueorder=True
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-11-30 16:19:49.924622031 +0000
+++ new-output.txt 2025-11-30 16:19:53.496646751 +0000
@@ -342,12 +342,14 @@
dataclasses_transform_field.py:64:16: error[unknown-argument] Argument `id` does not match any known parameter
dataclasses_transform_field.py:75:1: error[missing-argument] No argument provided for required parameter `name`
dataclasses_transform_field.py:75:16: error[too-many-positional-arguments] Too many positional arguments: expected 0, got 1
+dataclasses_transform_func.py:45:25: warning[subclass-of-dataclass-with-order] Class `Customer2Subclass` inherits from dataclass `Customer2` which has `order=True`
dataclasses_transform_func.py:56:1: error[invalid-assignment] Object of type `Literal[3]` is not assignable to attribute `name` of type `str`
dataclasses_transform_func.py:60:6: error[unsupported-operator] Operator `<` is not supported for types `Customer1` and `Customer1`
dataclasses_transform_func.py:64:36: error[unknown-argument] Argument `salary` does not match any known parameter
dataclasses_transform_func.py:70:8: error[missing-argument] No arguments provided for required parameters `id`, `name`
dataclasses_transform_func.py:70:18: error[too-many-positional-arguments] Too many positional arguments: expected 0, got 2
dataclasses_transform_func.py:76:36: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `T@create_model_frozen`
+dataclasses_transform_func.py:89:25: warning[subclass-of-dataclass-with-order] Class `Customer3Subclass` inherits from dataclass `Customer3` which has `order=True`
dataclasses_transform_func.py:96:1: error[invalid-assignment] Property `id` defined in `Customer3` is read-only
dataclasses_transform_meta.py:63:1: error[invalid-assignment] Property `id` defined in `Customer1` is read-only
dataclasses_transform_meta.py:66:8: error[missing-argument] No arguments provided for required parameters `id`, `name`
@@ -1039,4 +1041,4 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Unknown key "title" for TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions
-Found 1041 diagnostics
+Found 1043 diagnostics
|
|
bc1c309 to
4e4b08c
Compare
|
I believe those changes on the conformance tests are "correct" in-so-far as those are instances of the case we want to catch here. |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
subclass-of-dataclass-with-order |
77 | 0 | 0 |
invalid-argument-type |
0 | 2 | 0 |
unsupported-base |
0 | 2 | 0 |
| Total | 77 | 4 | 0 |
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
| classes in the inheritance hierarchy will raise a `TypeError` at runtime. This violates the Liskov | ||
| Substitution Principle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| classes in the inheritance hierarchy will raise a `TypeError` at runtime. This violates the Liskov | |
| Substitution Principle: | |
| classes in the inheritance hierarchy will raise a `TypeError` at runtime. The design of the stdlib | |
| feature therefore violates the Liskov Substitution Principle: |
|
|
||
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should suppress the subclass-of-dataclass-with-order diagnostic if all the comparison methods are overridden on the subclass? (That's the case here, since Bar2 is also decorated with order=True.) This case here does feel like it should be flagged using the error code we use for Liskov Subsitution Principle violations, because by specifying order=True the user has in a sense explicitly asked for an incompatible override.
And something like the following also seems safe, if the user has taken care to override all the unsound methods from the superclass:
from dataclasses import dataclass
@dataclass(order=True)
class A:
int
class B(A):
def __lt__(self, other: A) -> bool:
return True
def __le__(self, other: A) -> bool:
return True
__gt__ = __lt__
__ge__ = __le__| 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()), | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add an info subdiagnostic here explaining why this is problematic?
| /// class Parent: | ||
| /// value: int | ||
| /// | ||
| /// class Child(Parent): # Error raised here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// class Child(Parent): # Error raised here | |
| /// class Child(Parent): # Ty emits an error here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...ty
| /// Consider using `functools.total_ordering` instead, which does not have this limitation. | ||
| /// | ||
| /// [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Consider using `functools.total_ordering` instead, which does not have this limitation. | |
| /// | |
| /// [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle | |
| /// Consider using [`functools.total_ordering`][total_ordering] instead, which does not have this limitation. | |
| /// | |
| /// [Liskov Substitution Principle]: https://en.wikipedia.org/wiki/Liskov_substitution_principle | |
| /// [total_ordering]: https://docs.python.org/3/library/functools.html#functools.total_ordering |
Summary
Closes astral-sh/ty#1681.