Skip to content

Commit bcced1e

Browse files
connernilsenmeta-codesync[bot]
authored andcommitted
Generate dataclass ordering methods only on the decorated class
Summary: Pyrefly synthesized `__lt__`/`__le__`/`__gt__`/`__ge__` (plus `__eq__`/`__ne__`) on every class carrying ordered dataclass metadata, including plain subclasses that only inherit it. At runtime only the decorated class gets these methods, and subclasses inherit them. This caused two problems: - Plain subclasses showed signatures typed against themselves (`(self: C, other: C)` rather than `D`'s). - A user override between the dataclass and a plain subclass was hidden. For example, a `__ge__(self, other: object)` defined on an intermediate class still produced a false `unsupported-operation` error on its subclasses. The methods are now only synthesized when the class's dataclass metadata is its own (`!is_inherited`). With that, and with `total_ordering` no longer inventing comparison methods (previous diff), a normal member lookup reports the methods a class really uses. So `ordered-dataclass-inheritance` now uses that lookup instead of its own walk over class bodies. ___ landed-with-radar-review Differential Revision: D126992834 fbshipit-source-id: b870886b6f15041b9a9ab780f61068c017e22b3a
1 parent aada81c commit bcced1e

4 files changed

Lines changed: 86 additions & 25 deletions

File tree

‎pyrefly/lib/alt/class/dataclass.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,9 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
223223
ClassSynthesizedField::new_classvar(self.heap.mk_class_type(dataclass_fields_type)),
224224
);
225225

226-
if dataclass.kws.order {
226+
// Only the decorated class gets these methods at runtime. Subclasses that merely inherit
227+
// the metadata inherit the methods, including any override defined in between.
228+
if dataclass.kws.order && !dataclass.is_inherited {
227229
fields.extend(self.get_dataclass_rich_comparison_methods(cls));
228230
}
229231
if dataclass.kws.match_args {

‎pyrefly/lib/alt/solve.rs‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3310,9 +3310,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
33103310

33113311
/// Stdlib dataclass ordering methods only compare instances of the exact same class, so
33123312
/// comparing instances of a class and its base raises `TypeError` when both fall back to such
3313-
/// generated methods. Which methods a class uses is resolved by walking its MRO through class
3314-
/// bodies, because pyrefly also synthesizes ordering methods on classes that merely inherit
3315-
/// dataclass metadata, which would hide user-defined overrides from a member lookup.
3313+
/// generated methods.
33163314
fn check_ordered_dataclass_inheritance(
33173315
&self,
33183316
cls: &Class,
@@ -3324,29 +3322,33 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
33243322
if metadata.is_new_type() || metadata.dataclass_metadata().is_none() {
33253323
return;
33263324
}
3327-
// The class whose generated exact-class ordering method `c` uses for `op`, if any.
3325+
// The class whose generated exact-class ordering method `c` uses for `op`, if any. A
3326+
// member that its defining class does not declare in its body was synthesized there; it
3327+
// is a generated exact-class method only if that class has its own ordered stdlib
3328+
// dataclass metadata.
33283329
let generator_for = |c: &Class, op: &Name| -> Option<Class> {
3329-
let mro = self.get_mro_for_class(c);
3330-
for k in iter::once(c).chain(mro.ancestors_no_object().iter().map(|a| a.class_object()))
3330+
let defining_class = self
3331+
.get_class_member_with_defining_class(c, op)?
3332+
.defining_class;
3333+
if self
3334+
.get_class_fields(&defining_class)
3335+
.is_some_and(|f| f.contains(op))
33313336
{
3332-
if self.get_class_fields(k).is_some_and(|f| f.contains(op)) {
3333-
return None;
3334-
}
3335-
if let Some(dm) = self.get_metadata_for_class(k).dataclass_metadata()
3336-
&& !dm.is_inherited
3337-
&& dm.kws.order
3338-
{
3339-
return matches!(
3340-
dm.kind,
3341-
DataclassKind::Dataclass {
3342-
is_stdlib_dataclass: true,
3343-
..
3344-
}
3345-
)
3346-
.then(|| k.dupe());
3347-
}
3337+
return None;
33483338
}
3349-
None
3339+
let dm = self
3340+
.get_metadata_for_class(&defining_class)
3341+
.dataclass_metadata()?;
3342+
(dm.kws.order
3343+
&& !dm.is_inherited
3344+
&& matches!(
3345+
dm.kind,
3346+
DataclassKind::Dataclass {
3347+
is_stdlib_dataclass: true,
3348+
..
3349+
}
3350+
))
3351+
.then_some(defining_class)
33503352
};
33513353
// `cls() < base()` tries `cls.__lt__` and then the reflected `base.__gt__`, so it fails
33523354
// when both are generated. User-defined methods are assumed to accept mixed instances.

‎pyrefly/lib/test/dataclasses.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1308,6 +1308,32 @@ class E(C): # E: Cannot safely extend dataclass `D` with `order=True`
13081308
"#,
13091309
);
13101310

1311+
// Only the decorated class gets generated ordering methods at runtime; subclasses inherit them.
1312+
testcase!(
1313+
test_ordering_methods_inherited_by_subclass,
1314+
r#"
1315+
from dataclasses import dataclass
1316+
from typing import reveal_type
1317+
@dataclass(order=True)
1318+
class D:
1319+
x: int
1320+
1321+
class C(D): # E: Cannot safely extend dataclass `D` with `order=True`
1322+
pass
1323+
1324+
reveal_type(C.__lt__) # E: revealed type: (self: D, other: D) -> bool
1325+
1326+
class OverridesGe(D): # E: Cannot safely extend dataclass `D` with `order=True`
1327+
def __ge__(self, other: object) -> bool:
1328+
return True
1329+
1330+
class InheritsGe(OverridesGe): # E: Cannot safely extend dataclass `D` with `order=True`
1331+
pass
1332+
1333+
InheritsGe(1) >= 3
1334+
"#,
1335+
);
1336+
13111337
testcase!(
13121338
test_parent_dataclass_with_order_overridden,
13131339
r#"

‎pyrefly/lib/test/decorators.rs‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,12 @@ class OwnLt(D): # E: Cannot safely extend dataclass `D` with `order=True`
590590
def __lt__(self, other: object) -> bool:
591591
return True
592592
593-
reveal_type(OwnLt.__gt__) # E: revealed type: (self: OwnLt, other: OwnLt) -> bool
593+
reveal_type(OwnLt.__gt__) # E: revealed type: (self: D, other: D) -> bool
594+
595+
# `OwnLt` still uses `D`'s generated `__gt__`, so comparing it with `OrderedOverOwnLt` can fail.
596+
@dataclass(order=True)
597+
class OrderedOverOwnLt(OwnLt): # E: Cannot safely extend dataclass `D` with `order=True`
598+
y: int = 0
594599
"#,
595600
);
596601

@@ -615,6 +620,32 @@ reveal_type(B.__ge__) # E: revealed type: (self: B, other: A) -> bool
615620
"#,
616621
);
617622

623+
// Methods that `total_ordering` fills in on a mixin are user-defined as far as the dataclass is
624+
// concerned, so they replace the dataclass's generated methods that come later in the MRO.
625+
testcase!(
626+
test_total_ordering_mixin_before_ordered_dataclass,
627+
r#"
628+
from dataclasses import dataclass
629+
from functools import total_ordering
630+
631+
@dataclass(order=True)
632+
class D:
633+
x: int
634+
635+
@total_ordering
636+
class Mix:
637+
def __lt__(self, other: object) -> bool:
638+
return True
639+
640+
class C(Mix, D):
641+
pass
642+
643+
@dataclass(order=True)
644+
class E(C):
645+
y: int = 0
646+
"#,
647+
);
648+
618649
testcase!(
619650
test_total_ordering_inherited_rich_cmp,
620651
r#"

0 commit comments

Comments
 (0)