Skip to content

Commit f7305a3

Browse files
committed
Auto merge of #135634 - joboet:trivial-clone, r=<try>
stop specializing on `Copy` fixes #132442 `std` specializes on `Copy` to optimize certain library functions such as `clone_from_slice`. This is unsound, however, as the `Copy` implementation may not be always applicable because of lifetime bounds, which specialization does not take into account; the result being that values are copied even though they are not `Copy`. For instance, this code: ```rust struct SometimesCopy<'a>(&'a Cell<bool>); impl<'a> Clone for SometimesCopy<'a> { fn clone(&self) -> Self { self.0.set(true); Self(self.0) } } impl Copy for SometimesCopy<'static> {} let clone_called = Cell::new(false); // As SometimesCopy<'clone_called> is not 'static, this must run `clone`, // setting the value to `true`. let _ = [SometimesCopy(&clone_called)].clone(); assert!(clone_called.get()); ``` should not panic, but does ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6be7a48cad849d8bd064491616fdb43c)). To solve this, this PR introduces a new `unsafe` trait: `TrivialClone`. This trait may be implemented whenever the `Clone` implementation is equivalent to copying the value (so e.g. `fn clone(&self) -> Self { *self }`). Because of lifetime erasure, there is no way for the `Clone` implementation to observe lifetime bounds, meaning that even if the `TrivialClone` has stricter bounds than the `Clone` implementation, its invariant still holds. Therefore, it is sound to specialize on `TrivialClone`. I've changed all `Copy` specializations in the standard library to specialize on `TrivialClone` instead. Unfortunately, the unsound `#[rustc_unsafe_specialization_marker]` attribute on `Copy` cannot be removed in this PR as `hashbrown` still depends on it. I'll make a PR updating `hashbrown` once this lands. With `Copy` no longer being considered for specialization, this change alone would result in the standard library optimizations not being applied for user types unaware of `TrivialClone`. To avoid this and restore the optimizations in most cases, I have changed the expansion of `#[derive(Clone)]`: Currently, whenever both `Clone` and `Copy` are derived, the `clone` method performs a copy of the value. With this PR, the derive macro also adds a `TrivialClone` implementation to make this case observable using specialization. I anticipate that most users will use `#[derive(Clone, Copy)]` whenever both are applicable, so most users will still profit from the library optimizations. Unfortunately, Hyrum's law applies to this PR: there are some popular crates which rely on the precise specialization behaviour of `core` to implement "specialization at home", e.g. [`libAFL`](https://github.com/AFLplusplus/LibAFL/blob/89cff637025c1652c24e8d97a30a2e3d01f187a4/libafl_bolts/src/tuples.rs#L27-L49). I have no remorse for breaking such horrible code, but perhaps we should open other, better ways to satisfy their needs – for example by dropping the `'static` bound on `TypeId::of`...
2 parents 90f5eab + 1509254 commit f7305a3

File tree

41 files changed

+324
-88
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+324
-88
lines changed

compiler/rustc_builtin_macros/src/deriving/bounds.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::MetaItem;
1+
use rustc_ast::{MetaItem, Safety};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_span::Span;
44

@@ -23,6 +23,8 @@ pub(crate) fn expand_deriving_copy(
2323
methods: Vec::new(),
2424
associated_types: Vec::new(),
2525
is_const,
26+
safety: Safety::Default,
27+
document: true,
2628
};
2729

2830
trait_def.expand(cx, mitem, item, push);
@@ -46,6 +48,8 @@ pub(crate) fn expand_deriving_const_param_ty(
4648
methods: Vec::new(),
4749
associated_types: Vec::new(),
4850
is_const,
51+
safety: Safety::Default,
52+
document: true,
4953
};
5054

5155
trait_def.expand(cx, mitem, item, push);
@@ -60,6 +64,8 @@ pub(crate) fn expand_deriving_const_param_ty(
6064
methods: Vec::new(),
6165
associated_types: Vec::new(),
6266
is_const,
67+
safety: Safety::Default,
68+
document: true,
6369
};
6470

6571
trait_def.expand(cx, mitem, item, push);
@@ -83,6 +89,8 @@ pub(crate) fn expand_deriving_unsized_const_param_ty(
8389
methods: Vec::new(),
8490
associated_types: Vec::new(),
8591
is_const,
92+
safety: Safety::Default,
93+
document: true,
8694
};
8795

8896
trait_def.expand(cx, mitem, item, push);

compiler/rustc_builtin_macros/src/deriving/clone.rs

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
use rustc_ast::{self as ast, Generics, ItemKind, MetaItem, VariantData};
1+
use rustc_ast::{self as ast, Generics, ItemKind, MetaItem, Safety, VariantData};
22
use rustc_data_structures::fx::FxHashSet;
33
use rustc_expand::base::{Annotatable, ExtCtxt};
4-
use rustc_span::{Ident, Span, kw, sym};
4+
use rustc_span::{DUMMY_SP, Ident, Span, kw, sym};
55
use thin_vec::{ThinVec, thin_vec};
66

77
use crate::deriving::generic::ty::*;
@@ -68,6 +68,28 @@ pub(crate) fn expand_deriving_clone(
6868
_ => cx.dcx().span_bug(span, "`#[derive(Clone)]` on trait item or impl item"),
6969
}
7070

71+
// If the clone method is just copying the value, also mark the type as
72+
// `TrivialClone` to allow some library optimizations.
73+
if is_simple {
74+
let trivial_def = TraitDef {
75+
span,
76+
path: path_std!(clone::TrivialClone),
77+
skip_path_as_bound: false,
78+
needs_copy_as_bound_if_packed: true,
79+
additional_bounds: bounds.clone(),
80+
supports_unions: true,
81+
methods: Vec::new(),
82+
associated_types: Vec::new(),
83+
is_const,
84+
safety: Safety::Unsafe(DUMMY_SP),
85+
// `TrivialClone` is not part of an API guarantee, so it shouldn't
86+
// appear in rustdoc output.
87+
document: false,
88+
};
89+
90+
trivial_def.expand_ext(cx, mitem, item, push, true);
91+
}
92+
7193
let trait_def = TraitDef {
7294
span,
7395
path: path_std!(clone::Clone),
@@ -87,6 +109,8 @@ pub(crate) fn expand_deriving_clone(
87109
}],
88110
associated_types: Vec::new(),
89111
is_const,
112+
safety: Safety::Default,
113+
document: true,
90114
};
91115

92116
trait_def.expand_ext(cx, mitem, item, push, is_simple)

compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::{self as ast, MetaItem};
1+
use rustc_ast::{self as ast, MetaItem, Safety};
22
use rustc_data_structures::fx::FxHashSet;
33
use rustc_expand::base::{Annotatable, ExtCtxt};
44
use rustc_span::{Span, sym};
@@ -43,6 +43,8 @@ pub(crate) fn expand_deriving_eq(
4343
}],
4444
associated_types: Vec::new(),
4545
is_const,
46+
safety: Safety::Default,
47+
document: true,
4648
};
4749
trait_def.expand_ext(cx, mitem, item, push, true)
4850
}

compiler/rustc_builtin_macros/src/deriving/cmp/ord.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::MetaItem;
1+
use rustc_ast::{MetaItem, Safety};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_span::{Ident, Span, sym};
44
use thin_vec::thin_vec;
@@ -34,6 +34,8 @@ pub(crate) fn expand_deriving_ord(
3434
}],
3535
associated_types: Vec::new(),
3636
is_const,
37+
safety: Safety::Default,
38+
document: true,
3739
};
3840

3941
trait_def.expand(cx, mitem, item, push)

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use rustc_ast::ptr::P;
2-
use rustc_ast::{BinOpKind, BorrowKind, Expr, ExprKind, MetaItem, Mutability};
2+
use rustc_ast::{BinOpKind, BorrowKind, Expr, ExprKind, MetaItem, Mutability, Safety};
33
use rustc_expand::base::{Annotatable, ExtCtxt};
44
use rustc_span::{Span, sym};
55
use thin_vec::thin_vec;
@@ -84,6 +84,8 @@ pub(crate) fn expand_deriving_partial_eq(
8484
methods: Vec::new(),
8585
associated_types: Vec::new(),
8686
is_const: false,
87+
safety: Safety::Default,
88+
document: true,
8789
};
8890
structural_trait_def.expand(cx, mitem, item, push);
8991

@@ -110,6 +112,8 @@ pub(crate) fn expand_deriving_partial_eq(
110112
methods,
111113
associated_types: Vec::new(),
112114
is_const,
115+
safety: Safety::Default,
116+
document: true,
113117
};
114118
trait_def.expand(cx, mitem, item, push)
115119
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::{ExprKind, ItemKind, MetaItem, PatKind};
1+
use rustc_ast::{ExprKind, ItemKind, MetaItem, PatKind, Safety};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_span::{Ident, Span, sym};
44
use thin_vec::thin_vec;
@@ -64,6 +64,8 @@ pub(crate) fn expand_deriving_partial_ord(
6464
methods: vec![partial_cmp_def],
6565
associated_types: Vec::new(),
6666
is_const,
67+
safety: Safety::Default,
68+
document: true,
6769
};
6870
trait_def.expand(cx, mitem, item, push)
6971
}

compiler/rustc_builtin_macros/src/deriving/debug.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::{self as ast, EnumDef, MetaItem};
1+
use rustc_ast::{self as ast, EnumDef, MetaItem, Safety};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_session::config::FmtDebug;
44
use rustc_span::{Ident, Span, Symbol, sym};
@@ -41,6 +41,8 @@ pub(crate) fn expand_deriving_debug(
4141
}],
4242
associated_types: Vec::new(),
4343
is_const,
44+
safety: Safety::Default,
45+
document: true,
4446
};
4547
trait_def.expand(cx, mitem, item, push)
4648
}

compiler/rustc_builtin_macros/src/deriving/default.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
use core::ops::ControlFlow;
22

3-
use rustc_ast as ast;
43
use rustc_ast::visit::visit_opt;
5-
use rustc_ast::{EnumDef, VariantData, attr};
4+
use rustc_ast::{self as ast, EnumDef, Safety, VariantData, attr};
65
use rustc_expand::base::{Annotatable, DummyResult, ExtCtxt};
76
use rustc_span::{ErrorGuaranteed, Ident, Span, kw, sym};
87
use smallvec::SmallVec;
@@ -51,6 +50,8 @@ pub(crate) fn expand_deriving_default(
5150
}],
5251
associated_types: Vec::new(),
5352
is_const,
53+
safety: Safety::Default,
54+
document: true,
5455
};
5556
trait_def.expand(cx, mitem, item, push)
5657
}

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ pub(crate) use SubstructureFields::*;
183183
use rustc_ast::ptr::P;
184184
use rustc_ast::{
185185
self as ast, AnonConst, BindingMode, ByRef, EnumDef, Expr, GenericArg, GenericParamKind,
186-
Generics, Mutability, PatKind, VariantData,
186+
Generics, Mutability, PatKind, Safety, VariantData,
187187
};
188188
use rustc_attr_parsing::{AttributeKind, AttributeParser, ReprPacked};
189189
use rustc_expand::base::{Annotatable, ExtCtxt};
@@ -221,6 +221,12 @@ pub(crate) struct TraitDef<'a> {
221221
pub associated_types: Vec<(Ident, Ty)>,
222222

223223
pub is_const: bool,
224+
225+
/// The safety of the `impl`.
226+
pub safety: Safety,
227+
228+
/// Whether the added `impl` should appear in rustdoc output.
229+
pub document: bool,
224230
}
225231

226232
pub(crate) struct MethodDef<'a> {
@@ -784,15 +790,19 @@ impl<'a> TraitDef<'a> {
784790
let path = cx.path_all(self.span, false, vec![type_ident], self_params);
785791
let self_type = cx.ty_path(path);
786792

787-
let attrs = thin_vec![cx.attr_word(sym::automatically_derived, self.span),];
793+
let mut attrs = thin_vec![cx.attr_word(sym::automatically_derived, self.span),];
794+
if !self.document {
795+
attrs.push(cx.attr_nested_word(sym::doc, sym::hidden, self.span));
796+
}
797+
788798
let opt_trait_ref = Some(trait_ref);
789799

790800
cx.item(
791801
self.span,
792802
Ident::empty(),
793803
attrs,
794804
ast::ItemKind::Impl(Box::new(ast::Impl {
795-
safety: ast::Safety::Default,
805+
safety: self.safety,
796806
polarity: ast::ImplPolarity::Positive,
797807
defaultness: ast::Defaultness::Final,
798808
constness: if self.is_const { ast::Const::Yes(DUMMY_SP) } else { ast::Const::No },

compiler/rustc_builtin_macros/src/deriving/hash.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rustc_ast::{MetaItem, Mutability};
1+
use rustc_ast::{MetaItem, Mutability, Safety};
22
use rustc_expand::base::{Annotatable, ExtCtxt};
33
use rustc_span::{Span, sym};
44
use thin_vec::thin_vec;
@@ -41,6 +41,8 @@ pub(crate) fn expand_deriving_hash(
4141
}],
4242
associated_types: Vec::new(),
4343
is_const,
44+
safety: Safety::Default,
45+
document: true,
4446
};
4547

4648
hash_trait_def.expand(cx, mitem, item, push);

0 commit comments

Comments
 (0)