Skip to content

Commit f5daf9f

Browse files
borsbors
committed
Auto merge of #155598 - makai410:partial-ord, r=<try>
Implement fast path for `derive(PartialOrd)` when deriving `Ord`
2 parents 1bfcb28 + 2fd68fb commit f5daf9f

16 files changed

Lines changed: 225 additions & 275 deletions

‎compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
use rustc_ast::{ExprKind, ItemKind, MetaItem, PatKind, Safety};
1+
use rustc_ast::{ExprKind, ItemKind, MetaItem, PatKind, Safety, ast};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_span::{Ident, Span, sym};
4-
use thin_vec::thin_vec;
4+
use thin_vec::{ThinVec, thin_vec};
55

66
use crate::deriving::generic::ty::*;
77
use crate::deriving::generic::*;
@@ -41,6 +41,38 @@ pub(crate) fn expand_deriving_partial_ord(
4141
} else {
4242
true
4343
};
44+
45+
let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
46+
let has_derive_ord = cx.resolver.has_derive_ord(container_id);
47+
let is_simple_candidate = |params: &ThinVec<ast::GenericParam>| -> bool {
48+
has_derive_ord
49+
&& !params.iter().any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. }))
50+
};
51+
52+
let default_substructure = combine_substructure(Box::new(|cx, span, substr| {
53+
cs_partial_cmp(cx, span, substr, discr_then_data)
54+
}));
55+
let simple_substructure = combine_substructure(Box::new(|cx, span, _| {
56+
cs_partial_cmp_simple(cx, span, cx.expr_ident(span, Ident::new(sym::other, span)))
57+
}));
58+
let (is_simple, substructure) = match item {
59+
Annotatable::Item(annitem) => match &annitem.kind {
60+
// For unit structs/zero-variant enums, the default generated code is better.
61+
ItemKind::Struct(.., ast::VariantData::Unit(..)) => (false, default_substructure),
62+
ItemKind::Enum(.., enum_def) if enum_def.variants.is_empty() => {
63+
(false, default_substructure)
64+
}
65+
ItemKind::Struct(_, ast::Generics { params, .. }, _)
66+
| ItemKind::Enum(_, ast::Generics { params, .. }, _)
67+
if is_simple_candidate(params) =>
68+
{
69+
(true, simple_substructure)
70+
}
71+
_ => (false, default_substructure),
72+
},
73+
_ => (false, default_substructure),
74+
};
75+
4476
let partial_cmp_def = MethodDef {
4577
name: sym::partial_cmp,
4678
generics: Bounds::empty(),
@@ -49,9 +81,7 @@ pub(crate) fn expand_deriving_partial_ord(
4981
ret_ty,
5082
attributes: thin_vec![cx.attr_word(sym::inline, span)],
5183
fieldless_variants_strategy: FieldlessVariantsStrategy::Unify,
52-
combine_substructure: combine_substructure(Box::new(|cx, span, substr| {
53-
cs_partial_cmp(cx, span, substr, discr_then_data)
54-
})),
84+
combine_substructure: substructure,
5585
};
5686

5787
let trait_def = TraitDef {
@@ -68,7 +98,18 @@ pub(crate) fn expand_deriving_partial_ord(
6898
safety: Safety::Default,
6999
document: true,
70100
};
71-
trait_def.expand(cx, mitem, item, push)
101+
trait_def.expand_ext(cx, mitem, item, push, is_simple)
102+
}
103+
104+
// Special case for the type deriving both `PartialOrd` and `Ord`. Builds:
105+
// ```
106+
// Some(::core::cmp::Ord::cmp(self, other))
107+
// ```
108+
fn cs_partial_cmp_simple(cx: &ExtCtxt<'_>, span: Span, other_expr: Box<ast::Expr>) -> BlockOrExpr {
109+
let ord_cmp_path = cx.std_path(&[sym::cmp, sym::Ord, sym::cmp]);
110+
let cmp_expr =
111+
cx.expr_call_global(span, ord_cmp_path, thin_vec![cx.expr_self(span), other_expr]);
112+
BlockOrExpr::new_expr(cx.expr_some(span, cmp_expr))
72113
}
73114

74115
fn cs_partial_cmp(
@@ -98,7 +139,8 @@ fn cs_partial_cmp(
98139
|cx, fold| match fold {
99140
CsFold::Single(field) => {
100141
let [other_expr] = &field.other_selflike_exprs[..] else {
101-
cx.dcx().span_bug(field.span, "not exactly 2 arguments in `derive(Ord)`");
142+
cx.dcx()
143+
.span_bug(field.span, "not exactly 2 arguments in `derive(PartialOrd)`");
102144
};
103145
let args = thin_vec![field.self_expr.clone(), other_expr.clone()];
104146
cx.expr_call_global(field.span, partial_cmp_path.clone(), args)

‎compiler/rustc_expand/src/base.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1117,6 +1117,8 @@ pub trait ResolverExpand {
11171117
// Resolver interfaces for specific built-in macros.
11181118
/// Does `#[derive(...)]` attribute with the given `ExpnId` have built-in `Copy` inside it?
11191119
fn has_derive_copy(&self, expn_id: LocalExpnId) -> bool;
1120+
/// Does `#[derive(...)]` attribute with the given `ExpnId` have built-in `Ord` inside it?
1121+
fn has_derive_ord(&self, expn_id: LocalExpnId) -> bool;
11201122
/// Resolve paths inside the `#[derive(...)]` attribute with the given `ExpnId`.
11211123
fn resolve_derives(
11221124
&mut self,

‎compiler/rustc_resolve/src/lib.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,7 +1234,10 @@ impl ExternPreludeEntry<'_> {
12341234
struct DeriveData {
12351235
resolutions: Vec<DeriveResolution>,
12361236
helper_attrs: Vec<(usize, IdentKey, Span)>,
1237+
// if this list keeps getting extended, we could use `bitflags`,
1238+
// something like what [`rustc_type_ir::flags::TypeFlags`] is doing.
12371239
has_derive_copy: bool,
1240+
has_derive_ord: bool,
12381241
}
12391242

12401243
struct MacroData {
@@ -1388,6 +1391,7 @@ pub struct Resolver<'ra, 'tcx> {
13881391
/// Derive macros cannot modify the item themselves and have to store the markers in the global
13891392
/// context, so they attach the markers to derive container IDs using this resolver table.
13901393
containers_deriving_copy: FxHashSet<LocalExpnId> = default::fx_hash_set(),
1394+
containers_deriving_ord: FxHashSet<LocalExpnId> = default::fx_hash_set(),
13911395
/// Parent scopes in which the macros were invoked.
13921396
/// FIXME: `derives` are missing in these parent scopes and need to be taken from elsewhere.
13931397
invocation_parent_scopes: FxHashMap<LocalExpnId, ParentScope<'ra>> = default::fx_hash_map(),

‎compiler/rustc_resolve/src/macros.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,10 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
382382
self.containers_deriving_copy.contains(&expn_id)
383383
}
384384

385+
fn has_derive_ord(&self, expn_id: LocalExpnId) -> bool {
386+
self.containers_deriving_ord.contains(&expn_id)
387+
}
388+
385389
fn resolve_derives(
386390
&mut self,
387391
expn_id: LocalExpnId,
@@ -401,6 +405,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
401405
resolutions: derive_paths(),
402406
helper_attrs: Vec::new(),
403407
has_derive_copy: false,
408+
has_derive_ord: false,
404409
});
405410
let parent_scope = self.invocation_parent_scopes[&expn_id];
406411
for (i, resolution) in entry.resolutions.iter_mut().enumerate() {
@@ -423,6 +428,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
423428
);
424429
}
425430
entry.has_derive_copy |= ext.builtin_name == Some(sym::Copy);
431+
entry.has_derive_ord |= ext.builtin_name == Some(sym::Ord);
426432
ext
427433
}
428434
Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive),
@@ -458,6 +464,12 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
458464
if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) {
459465
self.containers_deriving_copy.insert(expn_id);
460466
}
467+
// Similar to the above `Copy` and `Clone` case, the code generated for
468+
// `derive(PartialOrd)` changes if `derive(Ord)` is also present.
469+
// FIXME(makai410): this also doesn't work with `#[derive(PartialOrd)] #[derive(Ord)]`.
470+
if entry.has_derive_ord || self.has_derive_ord(parent_scope.expansion) {
471+
self.containers_deriving_ord.insert(expn_id);
472+
}
461473
assert!(self.derive_data.is_empty());
462474
self.derive_data = derive_data;
463475
Ok(())

‎tests/mir-opt/pre-codegen/derived_ord_debug.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ pub struct MultiField(char, i16);
1010
// EMIT_MIR derived_ord_debug.{impl#1}-cmp.PreCodegen.after.mir
1111

1212
// CHECK-LABEL: partial_cmp(_1: &MultiField, _2: &MultiField) -> Option<std::cmp::Ordering>
13-
// CHECK: = <char as PartialOrd>::partial_cmp(
14-
// CHECK: = <i16 as PartialOrd>::partial_cmp(
13+
// CHECK: = <MultiField as Ord>::cmp(
14+
// CHECK: = Option::<std::cmp::Ordering>::Some(
1515

1616
// CHECK-LABEL: cmp(_1: &MultiField, _2: &MultiField) -> std::cmp::Ordering
1717
// CHECK: = <char as Ord>::cmp(

‎tests/mir-opt/pre-codegen/derived_ord_debug.{impl#0}-partial_cmp.PreCodegen.after.panic-unwind.mir‎

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -4,49 +4,14 @@ fn <impl at $DIR/derived_ord_debug.rs:6:10: 6:20>::partial_cmp(_1: &MultiField,
44
debug self => _1;
55
debug other => _2;
66
let mut _0: std::option::Option<std::cmp::Ordering>;
7-
let _3: &char;
8-
let _4: &char;
9-
let mut _5: std::option::Option<std::cmp::Ordering>;
10-
let mut _6: isize;
11-
let mut _7: i8;
12-
let _8: &i16;
13-
let _9: &i16;
14-
scope 1 {
15-
debug cmp => _5;
16-
}
7+
let mut _3: std::cmp::Ordering;
178

189
bb0: {
19-
_3 = &((*_1).0: char);
20-
_4 = &((*_2).0: char);
21-
_5 = <char as PartialOrd>::partial_cmp(copy _3, copy _4) -> [return: bb1, unwind continue];
10+
_3 = <MultiField as Ord>::cmp(copy _1, copy _2) -> [return: bb1, unwind continue];
2211
}
2312

2413
bb1: {
25-
_6 = discriminant(_5);
26-
switchInt(move _6) -> [1: bb2, 0: bb4, otherwise: bb6];
27-
}
28-
29-
bb2: {
30-
_7 = discriminant(((_5 as Some).0: std::cmp::Ordering));
31-
switchInt(move _7) -> [0: bb3, otherwise: bb4];
32-
}
33-
34-
bb3: {
35-
_8 = &((*_1).1: i16);
36-
_9 = &((*_2).1: i16);
37-
_0 = <i16 as PartialOrd>::partial_cmp(copy _8, copy _9) -> [return: bb5, unwind continue];
38-
}
39-
40-
bb4: {
41-
_0 = copy _5;
42-
goto -> bb5;
43-
}
44-
45-
bb5: {
14+
_0 = Option::<std::cmp::Ordering>::Some(move _3);
4615
return;
4716
}
48-
49-
bb6: {
50-
unreachable;
51-
}
5217
}

‎tests/ui/derives/deriving-with-repr-packed-move-errors.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@ struct StructA(String);
1818
//~| ERROR: cannot move out of a shared reference [E0507]
1919
//~| ERROR: cannot move out of a shared reference [E0507]
2020
//~| ERROR: cannot move out of a shared reference [E0507]
21-
//~| ERROR: cannot move out of a shared reference [E0507]
22-
//~| ERROR: cannot move out of a shared reference [E0507]
2321

2422

2523
// Unrelated impl: additinal diagnostic should NOT be emitted

‎tests/ui/derives/deriving-with-repr-packed-move-errors.stderr‎

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -29,27 +29,6 @@ LL | struct StructA(String);
2929
= note: `#[derive(PartialEq)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
3030
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
3131

32-
error[E0507]: cannot move out of a shared reference
33-
--> $DIR/deriving-with-repr-packed-move-errors.rs:13:16
34-
|
35-
LL | #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Default)]
36-
| ---------- in this derive macro expansion
37-
LL | struct StructA(String);
38-
| ^^^^^^ move occurs because value has type `String`, which does not implement the `Copy` trait
39-
|
40-
= note: `#[derive(PartialOrd)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
41-
42-
error[E0507]: cannot move out of a shared reference
43-
--> $DIR/deriving-with-repr-packed-move-errors.rs:13:16
44-
|
45-
LL | #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Default)]
46-
| ---------- in this derive macro expansion
47-
LL | struct StructA(String);
48-
| ^^^^^^ move occurs because value has type `String`, which does not implement the `Copy` trait
49-
|
50-
= note: `#[derive(PartialOrd)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
51-
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
52-
5332
error[E0507]: cannot move out of a shared reference
5433
--> $DIR/deriving-with-repr-packed-move-errors.rs:13:16
5534
|
@@ -92,7 +71,7 @@ LL | struct StructA(String);
9271
= note: `#[derive(Clone)]` triggers a move because taking references to the fields of a packed struct is undefined behaviour
9372

9473
error[E0507]: cannot move out of `self` which is behind a shared reference
95-
--> $DIR/deriving-with-repr-packed-move-errors.rs:28:9
74+
--> $DIR/deriving-with-repr-packed-move-errors.rs:26:9
9675
|
9776
LL | self.0
9877
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -103,7 +82,7 @@ LL | self.0.clone()
10382
| ++++++++
10483

10584
error[E0507]: cannot move out of `self` which is behind a shared reference
106-
--> $DIR/deriving-with-repr-packed-move-errors.rs:38:20
85+
--> $DIR/deriving-with-repr-packed-move-errors.rs:36:20
10786
|
10887
LL | let x = &{ self.0 };
10988
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -114,7 +93,7 @@ LL | let x = &{ self.0.clone() };
11493
| ++++++++
11594

11695
error[E0507]: cannot move out of `self` which is behind a shared reference
117-
--> $DIR/deriving-with-repr-packed-move-errors.rs:45:12
96+
--> $DIR/deriving-with-repr-packed-move-errors.rs:43:12
11897
|
11998
LL | ({ self.0 }) == ({ other.0 })
12099
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -125,7 +104,7 @@ LL | ({ self.0.clone() }) == ({ other.0 })
125104
| ++++++++
126105

127106
error[E0507]: cannot move out of `other` which is behind a shared reference
128-
--> $DIR/deriving-with-repr-packed-move-errors.rs:45:28
107+
--> $DIR/deriving-with-repr-packed-move-errors.rs:43:28
129108
|
130109
LL | ({ self.0 }) == ({ other.0 })
131110
| ^^^^^^^ move occurs because `other.0` has type `String`, which does not implement the `Copy` trait
@@ -136,7 +115,7 @@ LL | ({ self.0 }) == ({ other.0.clone() })
136115
| ++++++++
137116

138117
error[E0507]: cannot move out of `self` which is behind a shared reference
139-
--> $DIR/deriving-with-repr-packed-move-errors.rs:53:36
118+
--> $DIR/deriving-with-repr-packed-move-errors.rs:51:36
140119
|
141120
LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0 })
142121
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -147,7 +126,7 @@ LL | PartialOrd::partial_cmp(&{ self.0.clone() }, &{ other.0 })
147126
| ++++++++
148127

149128
error[E0507]: cannot move out of `other` which is behind a shared reference
150-
--> $DIR/deriving-with-repr-packed-move-errors.rs:53:49
129+
--> $DIR/deriving-with-repr-packed-move-errors.rs:51:49
151130
|
152131
LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0 })
153132
| ^^^^^^^ move occurs because `other.0` has type `String`, which does not implement the `Copy` trait
@@ -158,7 +137,7 @@ LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0.clone() })
158137
| ++++++++
159138

160139
error[E0507]: cannot move out of `self` which is behind a shared reference
161-
--> $DIR/deriving-with-repr-packed-move-errors.rs:68:20
140+
--> $DIR/deriving-with-repr-packed-move-errors.rs:66:20
162141
|
163142
LL | let x = &{ self.0 };
164143
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -169,7 +148,7 @@ LL | let x = &{ self.0.clone() };
169148
| ++++++++
170149

171150
error[E0507]: cannot move out of `self` which is behind a shared reference
172-
--> $DIR/deriving-with-repr-packed-move-errors.rs:75:12
151+
--> $DIR/deriving-with-repr-packed-move-errors.rs:73:12
173152
|
174153
LL | ({ self.0 }) == ({ other.0 })
175154
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -180,7 +159,7 @@ LL | ({ self.0.clone() }) == ({ other.0 })
180159
| ++++++++
181160

182161
error[E0507]: cannot move out of `other` which is behind a shared reference
183-
--> $DIR/deriving-with-repr-packed-move-errors.rs:75:28
162+
--> $DIR/deriving-with-repr-packed-move-errors.rs:73:28
184163
|
185164
LL | ({ self.0 }) == ({ other.0 })
186165
| ^^^^^^^ move occurs because `other.0` has type `String`, which does not implement the `Copy` trait
@@ -191,7 +170,7 @@ LL | ({ self.0 }) == ({ other.0.clone() })
191170
| ++++++++
192171

193172
error[E0507]: cannot move out of `self` which is behind a shared reference
194-
--> $DIR/deriving-with-repr-packed-move-errors.rs:83:36
173+
--> $DIR/deriving-with-repr-packed-move-errors.rs:81:36
195174
|
196175
LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0 })
197176
| ^^^^^^ move occurs because `self.0` has type `String`, which does not implement the `Copy` trait
@@ -202,7 +181,7 @@ LL | PartialOrd::partial_cmp(&{ self.0.clone() }, &{ other.0 })
202181
| ++++++++
203182

204183
error[E0507]: cannot move out of `other` which is behind a shared reference
205-
--> $DIR/deriving-with-repr-packed-move-errors.rs:83:49
184+
--> $DIR/deriving-with-repr-packed-move-errors.rs:81:49
206185
|
207186
LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0 })
208187
| ^^^^^^^ move occurs because `other.0` has type `String`, which does not implement the `Copy` trait
@@ -213,7 +192,7 @@ LL | PartialOrd::partial_cmp(&{ self.0 }, &{ other.0.clone() })
213192
| ++++++++
214193

215194
error[E0507]: cannot move out of `arg` which is behind a shared reference
216-
--> $DIR/deriving-with-repr-packed-move-errors.rs:92:5
195+
--> $DIR/deriving-with-repr-packed-move-errors.rs:90:5
217196
|
218197
LL | arg.0
219198
| ^^^^^ move occurs because `arg.0` has type `String`, which does not implement the `Copy` trait
@@ -223,6 +202,6 @@ help: consider cloning the value if the performance cost is acceptable
223202
LL | arg.0.clone()
224203
| ++++++++
225204

226-
error: aborting due to 21 previous errors
205+
error: aborting due to 19 previous errors
227206

228207
For more information about this error, try `rustc --explain E0507`.

‎tests/ui/deriving/deriving-all-codegen.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,21 @@ struct FooCopyAndClone(i32);
235235
#[derive(Clone)]
236236
#[derive(Copy)]
237237
struct FooCloneAndCopy(i32);
238+
239+
#[derive(PartialOrd, Ord)]
240+
struct FooPartialOrdOrd(i32);
241+
242+
#[derive(Ord, PartialOrd)]
243+
struct FooOrdPartialOrd(i32);
244+
245+
#[derive(Ord)]
246+
#[derive(PartialOrd)]
247+
struct FooOrdBeforePartialOrd(i32);
248+
249+
// FIXME: this case should also have a trivial `PartialOrd` impl.
250+
#[derive(PartialOrd)]
251+
#[derive(Ord)]
252+
struct FooPartialOrdBeforeOrd(i32);
253+
254+
#[derive(PartialOrd, Ord)]
255+
struct UnitStruct;

0 commit comments

Comments
 (0)