Skip to content

Commit aada81c

Browse files
connernilsenmeta-codesync[bot]
authored andcommitted
Let total_ordering see comparison methods generated on ancestors
Summary: `functools.total_ordering` only fills in ordering methods that a class doesn't already have, whether they're defined directly or inherited. Pyrefly decided what was missing using a lookup that skips every synthesized member in the MRO, so it couldn't see ordering methods generated on an ancestor dataclass. For a subclass of `dataclass(order=True)`, that caused two problems: - `total_ordering class B(D): pass` reported a false "must define at least one of the rich comparison methods" error. - With a hand-written `__lt__`, pyrefly made up `__gt__`/`__le__`/`__ge__` on the subclass from it. At runtime the subclass keeps `D`'s methods. The same blind spot applied to methods that `total_ordering` filled in on an ancestor. The lookup only needs to skip members synthesized on the class itself, because those are what's being computed. Members synthesized on ancestors are already settled, so a new lookup ignores only the class's own synthesized fields, and `total_ordering` uses it. ___ landed-with-radar-review Differential Revision: D126992833 fbshipit-source-id: ccc366ab1e9c0399165e02fe3ed6996eacf33324
1 parent 5d6ea02 commit aada81c

3 files changed

Lines changed: 68 additions & 3 deletions

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4944,6 +4944,23 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
49444944
})
49454945
}
49464946

4947+
/// Look up a member while ignoring fields synthesized on `cls` itself, but not those
4948+
/// synthesized on its ancestors. Use this while computing `cls`'s own synthesized fields.
4949+
pub fn get_class_member_ignoring_own_synthesized(
4950+
&self,
4951+
cls: &Class,
4952+
name: &Name,
4953+
) -> Option<WithDefiningClass<Cow<'_, ClassField>>> {
4954+
self.get_field_from_mro(cls, name, &|c, name| {
4955+
if c == cls {
4956+
self.get_non_synthesized_field_from_current_class_only(c, name)
4957+
} else {
4958+
self.get_field_from_current_class_only(c, name)
4959+
}
4960+
})
4961+
.filter(|field| !field.value.is_init_var())
4962+
}
4963+
49474964
pub(crate) fn get_synthesized_field_from_current_class_only(
49484965
&self,
49494966
cls: &Class,

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
4040
// The first field in the conversion order is the one that we will use to synthesize the method.
4141
for other_cmp in conversion_order {
4242
if let Some(other_cmp_field) = self
43-
.get_non_synthesized_class_member_and_defining_class(cls, other_cmp)
43+
.get_class_member_ignoring_own_synthesized(cls, other_cmp)
4444
.filter(|field| !field.defining_class.is_builtin("object"))
4545
{
4646
let ty = other_cmp_field.value.as_named_tuple_type();
@@ -61,7 +61,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
6161
}
6262
// The class must have one of the rich comparison dunder methods defined
6363
if !dunder::RICH_CMPS_TOTAL_ORDERING.iter().any(|cmp| {
64-
self.get_non_synthesized_class_member_and_defining_class(cls, cmp)
64+
self.get_class_member_ignoring_own_synthesized(cls, cmp)
6565
.is_some_and(|field| !field.defining_class.is_builtin("object"))
6666
}) {
6767
let total_ordering_metadata = metadata.total_ordering_metadata().unwrap();
@@ -79,7 +79,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
7979
let rich_cmps_to_synthesize: Vec<_> = dunder::RICH_CMPS_TOTAL_ORDERING
8080
.iter()
8181
.filter(|cmp| {
82-
self.get_non_synthesized_class_member_and_defining_class(cls, cmp)
82+
self.get_class_member_ignoring_own_synthesized(cls, cmp)
8383
.is_none_or(|field| field.defining_class.is_builtin("object"))
8484
})
8585
.collect();

‎pyrefly/lib/test/decorators.rs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,54 @@ class A:
567567
"#,
568568
);
569569

570+
// `total_ordering` only fills in methods the class doesn't already have, including methods
571+
// generated on an ancestor dataclass.
572+
testcase!(
573+
test_total_ordering_on_ordered_dataclass_subclass,
574+
r#"
575+
from dataclasses import dataclass
576+
from functools import total_ordering
577+
from typing import reveal_type
578+
579+
@dataclass(order=True)
580+
class D:
581+
x: int
582+
583+
@total_ordering
584+
class NoOwnMethods(D): # E: Cannot safely extend dataclass `D` with `order=True`
585+
pass
586+
587+
# `OwnLt` keeps `D`'s generated `__gt__`, so `OwnLt(1) > D(1)` fails.
588+
@total_ordering
589+
class OwnLt(D): # E: Cannot safely extend dataclass `D` with `order=True`
590+
def __lt__(self, other: object) -> bool:
591+
return True
592+
593+
reveal_type(OwnLt.__gt__) # E: revealed type: (self: OwnLt, other: OwnLt) -> bool
594+
"#,
595+
);
596+
597+
// Methods that `total_ordering` filled in on an ancestor count as defined, so `B` gets nothing new.
598+
testcase!(
599+
test_total_ordering_on_total_ordering_subclass,
600+
r#"
601+
from functools import total_ordering
602+
from typing import reveal_type
603+
604+
@total_ordering
605+
class A:
606+
def __lt__(self, other: "A") -> bool:
607+
return True
608+
609+
@total_ordering
610+
class B(A):
611+
def __gt__(self, other: object) -> bool:
612+
return True
613+
614+
reveal_type(B.__ge__) # E: revealed type: (self: B, other: A) -> bool
615+
"#,
616+
);
617+
570618
testcase!(
571619
test_total_ordering_inherited_rich_cmp,
572620
r#"

0 commit comments

Comments
 (0)