Skip to content

Commit 159a353

Browse files
makai410makai410
committed
Implement fast path for derive(PartialOrd) 
1 parent e22c616 commit 159a353

12 files changed

Lines changed: 106 additions & 236 deletions

File tree

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

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
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};
44
use thin_vec::thin_vec;
@@ -41,6 +41,35 @@ pub(crate) fn expand_deriving_partial_ord(
4141
} else {
4242
true
4343
};
44+
let substructure = combine_substructure(Box::new(|cx, span, substr| {
45+
cs_partial_cmp(cx, span, substr, discr_then_data)
46+
}));
47+
let (is_simple, substructure) = match item {
48+
Annotatable::Item(annitem) => match &annitem.kind {
49+
ItemKind::Struct(_, ast::Generics { params, .. }, _)
50+
| ItemKind::Enum(_, ast::Generics { params, .. }, _)
51+
if let container_id = cx.current_expansion.id.expn_data().parent.expect_local()
52+
&& cx.resolver.has_derive_ord(container_id)
53+
&& !params
54+
.iter()
55+
.any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. })) =>
56+
{
57+
(
58+
true,
59+
combine_substructure(Box::new(|cx, span, _| {
60+
cs_partial_cmp_simple(
61+
cx,
62+
span,
63+
cx.expr_ident(span, Ident::new(sym::other, span)),
64+
)
65+
})),
66+
)
67+
}
68+
_ => (false, substructure),
69+
},
70+
_ => (false, substructure),
71+
};
72+
4473
let partial_cmp_def = MethodDef {
4574
name: sym::partial_cmp,
4675
generics: Bounds::empty(),
@@ -49,9 +78,7 @@ pub(crate) fn expand_deriving_partial_ord(
4978
ret_ty,
5079
attributes: thin_vec![cx.attr_word(sym::inline, span)],
5180
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-
})),
81+
combine_substructure: substructure,
5582
};
5683

5784
let trait_def = TraitDef {
@@ -68,7 +95,21 @@ pub(crate) fn expand_deriving_partial_ord(
6895
safety: Safety::Default,
6996
document: true,
7097
};
71-
trait_def.expand(cx, mitem, item, push)
98+
trait_def.expand_ext(cx, mitem, item, push, is_simple)
99+
}
100+
101+
// Special case for the type deriving both `PartialOrd` and `Ord`. Builds:
102+
// ```
103+
// Some(self.cmp(other))
104+
// ```
105+
fn cs_partial_cmp_simple(cx: &ExtCtxt<'_>, span: Span, other_expr: Box<ast::Expr>) -> BlockOrExpr {
106+
let cmp_expr = cx.expr_method_call(
107+
span,
108+
cx.expr_self(span),
109+
Ident::new(sym::cmp, span),
110+
thin_vec![other_expr],
111+
);
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
@@ -1227,7 +1227,10 @@ impl ExternPreludeEntry<'_> {
12271227
struct DeriveData {
12281228
resolutions: Vec<DeriveResolution>,
12291229
helper_attrs: Vec<(usize, IdentKey, Span)>,
1230+
// if this list keeps getting extended, we could use `bitflags`,
1231+
// something like what [`rustc_type_ir::flags::TypeFlags`] is doing.
12301232
has_derive_copy: bool,
1233+
has_derive_ord: bool,
12311234
}
12321235

12331236
struct MacroData {
@@ -1381,6 +1384,7 @@ pub struct Resolver<'ra, 'tcx> {
13811384
/// Derive macros cannot modify the item themselves and have to store the markers in the global
13821385
/// context, so they attach the markers to derive container IDs using this resolver table.
13831386
containers_deriving_copy: FxHashSet<LocalExpnId> = default::fx_hash_set(),
1387+
containers_deriving_ord: FxHashSet<LocalExpnId> = default::fx_hash_set(),
13841388
/// Parent scopes in which the macros were invoked.
13851389
/// FIXME: `derives` are missing in these parent scopes and need to be taken from elsewhere.
13861390
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
@@ -381,6 +381,10 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
381381
self.containers_deriving_copy.contains(&expn_id)
382382
}
383383

384+
fn has_derive_ord(&self, expn_id: LocalExpnId) -> bool {
385+
self.containers_deriving_ord.contains(&expn_id)
386+
}
387+
384388
fn resolve_derives(
385389
&mut self,
386390
expn_id: LocalExpnId,
@@ -400,6 +404,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
400404
resolutions: derive_paths(),
401405
helper_attrs: Vec::new(),
402406
has_derive_copy: false,
407+
has_derive_ord: false,
403408
});
404409
let parent_scope = self.invocation_parent_scopes[&expn_id];
405410
for (i, resolution) in entry.resolutions.iter_mut().enumerate() {
@@ -422,6 +427,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
422427
);
423428
}
424429
entry.has_derive_copy |= ext.builtin_name == Some(sym::Copy);
430+
entry.has_derive_ord |= ext.builtin_name == Some(sym::Ord);
425431
ext
426432
}
427433
Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive),
@@ -457,6 +463,12 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> {
457463
if entry.has_derive_copy || self.has_derive_copy(parent_scope.expansion) {
458464
self.containers_deriving_copy.insert(expn_id);
459465
}
466+
// Similar to the above `Copy` and `Clone` case, the code generated for
467+
// `derive(PartialOrd)` changes if `derive(Ord)` is also present.
468+
// FIXME(makai410): this also doesn't work with `#[derive(PartialOrd)] #[derive(Ord)]`.
469+
if entry.has_derive_ord || self.has_derive_ord(parent_scope.expansion) {
470+
self.containers_deriving_ord.insert(expn_id);
471+
}
460472
assert!(self.derive_data.is_empty());
461473
self.derive_data = derive_data;
462474
Ok(())

‎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`.

0 commit comments

Comments
 (0)