Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit f14a7ea

Browse files
committedMar 17, 2025
Auto merge of rust-lang#135634 - joboet:trivial-clone, r=<try>
stop specializing on `Copy` fixes rust-lang#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 9c67cec + c5e262c commit f14a7ea

File tree

41 files changed

+276
-87
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

+276
-87
lines changed
 

‎library/alloc/src/rc.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -243,9 +243,9 @@
243243

244244
use core::any::Any;
245245
use core::cell::Cell;
246-
#[cfg(not(no_global_oom_handling))]
247-
use core::clone::CloneToUninit;
248246
use core::clone::UseCloned;
247+
#[cfg(not(no_global_oom_handling))]
248+
use core::clone::{CloneToUninit, TrivialClone};
249249
use core::cmp::Ordering;
250250
use core::hash::{Hash, Hasher};
251251
use core::intrinsics::abort;
@@ -2150,7 +2150,8 @@ impl<T> Rc<[T]> {
21502150

21512151
/// Copy elements from slice into newly allocated `Rc<[T]>`
21522152
///
2153-
/// Unsafe because the caller must either take ownership or bind `T: Copy`
2153+
/// Unsafe because the caller must either take ownership, bind `T: Copy` or
2154+
/// bind `T: TrivialClone`.
21542155
#[cfg(not(no_global_oom_handling))]
21552156
unsafe fn copy_from_slice(v: &[T]) -> Rc<[T]> {
21562157
unsafe {
@@ -2240,9 +2241,11 @@ impl<T: Clone> RcFromSlice<T> for Rc<[T]> {
22402241
}
22412242

22422243
#[cfg(not(no_global_oom_handling))]
2243-
impl<T: Copy> RcFromSlice<T> for Rc<[T]> {
2244+
impl<T: TrivialClone> RcFromSlice<T> for Rc<[T]> {
22442245
#[inline]
22452246
fn from_slice(v: &[T]) -> Self {
2247+
// SAFETY: `T` implements `TrivialClone`, so this is sound and equivalent
2248+
// to the above.
22462249
unsafe { Rc::copy_from_slice(v) }
22472250
}
22482251
}

‎library/alloc/src/slice.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
use core::borrow::{Borrow, BorrowMut};
1313
#[cfg(not(no_global_oom_handling))]
14+
use core::clone::TrivialClone;
15+
#[cfg(not(no_global_oom_handling))]
1416
use core::cmp::Ordering::{self, Less};
1517
#[cfg(not(no_global_oom_handling))]
1618
use core::mem::MaybeUninit;
@@ -440,7 +442,7 @@ impl<T> [T] {
440442
}
441443
}
442444

443-
impl<T: Copy> ConvertVec for T {
445+
impl<T: TrivialClone> ConvertVec for T {
444446
#[inline]
445447
fn to_vec<A: Allocator>(s: &[Self], alloc: A) -> Vec<Self, A> {
446448
let mut v = Vec::with_capacity_in(s.len(), alloc);
@@ -825,7 +827,7 @@ impl<T: Clone, A: Allocator> SpecCloneIntoVec<T, A> for [T] {
825827
}
826828

827829
#[cfg(not(no_global_oom_handling))]
828-
impl<T: Copy, A: Allocator> SpecCloneIntoVec<T, A> for [T] {
830+
impl<T: TrivialClone, A: Allocator> SpecCloneIntoVec<T, A> for [T] {
829831
fn clone_into(&self, target: &mut Vec<T, A>) {
830832
target.clear();
831833
target.extend_from_slice(self);
There was a problem loading the remainder of the diff.

0 commit comments

Comments
 (0)
Failed to load comments.