Skip to content

Commit 87d5ef8

Browse files
Merge #10578
10578: Fix partialord codegen take 2 r=lnicola a=yoshuawuyts Fixes #10576. This reverts "generate `PartialOrd` to our previous match-based design, and in turn uses that to correctly take references for multi-value comparisons. This is a bit more verbose, but it should be more readable and easier to edit by end-users than multiple nested layers of borrows. I also manually verified every example in the Rust playground to ensure it works. Thanks! cc/ `@WaffleLapkin` Co-authored-by: Yoshua Wuyts <[email protected]>
2 parents 48c3be9 + e346d32 commit 87d5ef8

File tree

2 files changed

+54
-19
lines changed

2 files changed

+54
-19
lines changed

crates/ide_assists/src/handlers/replace_derive_with_manual_impl.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -722,7 +722,15 @@ struct Foo {
722722
723723
impl PartialOrd for Foo {
724724
$0fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
725-
(self.bin, self.bar, self.baz).partial_cmp(&(other.bin, other.bar, other.baz))
725+
match self.bin.partial_cmp(&other.bin) {
726+
Some(core::cmp::Ordering::Equal) => {}
727+
ord => return ord,
728+
}
729+
match self.bar.partial_cmp(&other.bar) {
730+
Some(core::cmp::Ordering::Equal) => {}
731+
ord => return ord,
732+
}
733+
self.baz.partial_cmp(&other.baz)
726734
}
727735
}
728736
"#,
@@ -743,7 +751,15 @@ struct Foo(usize, usize, usize);
743751
744752
impl PartialOrd for Foo {
745753
$0fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
746-
(self.0, self.1, self.2).partial_cmp(&(other.0, other.1, other.2))
754+
match self.0.partial_cmp(&other.0) {
755+
Some(core::cmp::Ordering::Equal) => {}
756+
ord => return ord,
757+
}
758+
match self.1.partial_cmp(&other.1) {
759+
Some(core::cmp::Ordering::Equal) => {}
760+
ord => return ord,
761+
}
762+
self.2.partial_cmp(&other.2)
747763
}
748764
}
749765
"#,

crates/ide_assists/src/utils/gen_trait_fn_body.rs

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -574,11 +574,24 @@ fn gen_partial_eq(adt: &ast::Adt, func: &ast::Fn) -> Option<()> {
574574
}
575575

576576
fn gen_partial_ord(adt: &ast::Adt, func: &ast::Fn) -> Option<()> {
577-
fn gen_partial_cmp_call(mut lhs: Vec<ast::Expr>, mut rhs: Vec<ast::Expr>) -> ast::Expr {
578-
let (lhs, rhs) = match (lhs.len(), rhs.len()) {
579-
(1, 1) => (lhs.pop().unwrap(), rhs.pop().unwrap()),
580-
_ => (make::expr_tuple(lhs.into_iter()), make::expr_tuple(rhs.into_iter())),
581-
};
577+
fn gen_partial_eq_match(match_target: ast::Expr) -> Option<ast::Stmt> {
578+
let mut arms = vec![];
579+
580+
let variant_name =
581+
make::path_pat(make::ext::path_from_idents(["core", "cmp", "Ordering", "Equal"])?);
582+
let lhs = make::tuple_struct_pat(make::ext::path_from_idents(["Some"])?, [variant_name]);
583+
arms.push(make::match_arm(Some(lhs.into()), None, make::expr_empty_block()));
584+
585+
arms.push(make::match_arm(
586+
[make::ident_pat(false, false, make::name("ord")).into()],
587+
None,
588+
make::expr_return(Some(make::expr_path(make::ext::ident_path("ord")))),
589+
));
590+
let list = make::match_arm_list(arms).indent(ast::edit::IndentLevel(1));
591+
Some(make::expr_stmt(make::expr_match(match_target, list)).into())
592+
}
593+
594+
fn gen_partial_cmp_call(lhs: ast::Expr, rhs: ast::Expr) -> ast::Expr {
582595
let rhs = make::expr_ref(rhs, false);
583596
let method = make::name_ref("partial_cmp");
584597
make::expr_method_call(lhs, method, make::arg_list(Some(rhs)))
@@ -594,35 +607,41 @@ fn gen_partial_ord(adt: &ast::Adt, func: &ast::Fn) -> Option<()> {
594607
ast::Adt::Enum(_) => return None,
595608
ast::Adt::Struct(strukt) => match strukt.field_list() {
596609
Some(ast::FieldList::RecordFieldList(field_list)) => {
597-
let mut l_fields = vec![];
598-
let mut r_fields = vec![];
610+
let mut exprs = vec![];
599611
for field in field_list.fields() {
600612
let lhs = make::expr_path(make::ext::ident_path("self"));
601613
let lhs = make::expr_field(lhs, &field.name()?.to_string());
602614
let rhs = make::expr_path(make::ext::ident_path("other"));
603615
let rhs = make::expr_field(rhs, &field.name()?.to_string());
604-
l_fields.push(lhs);
605-
r_fields.push(rhs);
616+
let ord = gen_partial_cmp_call(lhs, rhs);
617+
exprs.push(ord);
606618
}
607619

608-
let expr = gen_partial_cmp_call(l_fields, r_fields);
609-
make::block_expr(None, Some(expr)).indent(ast::edit::IndentLevel(1))
620+
let tail = exprs.pop();
621+
let stmts = exprs
622+
.into_iter()
623+
.map(gen_partial_eq_match)
624+
.collect::<Option<Vec<ast::Stmt>>>()?;
625+
make::block_expr(stmts.into_iter(), tail).indent(ast::edit::IndentLevel(1))
610626
}
611627

612628
Some(ast::FieldList::TupleFieldList(field_list)) => {
613-
let mut l_fields = vec![];
614-
let mut r_fields = vec![];
629+
let mut exprs = vec![];
615630
for (i, _) in field_list.fields().enumerate() {
616631
let idx = format!("{}", i);
617632
let lhs = make::expr_path(make::ext::ident_path("self"));
618633
let lhs = make::expr_field(lhs, &idx);
619634
let rhs = make::expr_path(make::ext::ident_path("other"));
620635
let rhs = make::expr_field(rhs, &idx);
621-
l_fields.push(lhs);
622-
r_fields.push(rhs);
636+
let ord = gen_partial_cmp_call(lhs, rhs);
637+
exprs.push(ord);
623638
}
624-
let expr = gen_partial_cmp_call(l_fields, r_fields);
625-
make::block_expr(None, Some(expr)).indent(ast::edit::IndentLevel(1))
639+
let tail = exprs.pop();
640+
let stmts = exprs
641+
.into_iter()
642+
.map(gen_partial_eq_match)
643+
.collect::<Option<Vec<ast::Stmt>>>()?;
644+
make::block_expr(stmts.into_iter(), tail).indent(ast::edit::IndentLevel(1))
626645
}
627646

628647
// No fields in the body means there's nothing to hash.

0 commit comments

Comments
 (0)