-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Comment on lines
-481
to
487
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should suppress the 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__ |
||
|
|
||
| # 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: ... | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...ty |
||||||||||||||||
| /// 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 | ||||||||||||||||
|
Comment on lines
+1676
to
+1678
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| 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. | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+749
to
+762
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add an |
||
| } | ||
|
|
||
| // (4) Check that the class's MRO is resolvable | ||
|
|
||
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.