Generalize {Rc,Arc}::make_mut() to unsized types.

[kpreid]

• {Rc,Arc}::make_mut() now accept any type implementing the new unstable trait core::clone::CloneToUninit.
• CloneToUninit is implemented for T: Clone and for [T] where T: Clone.
• CloneToUninit is a generalization of the existing internal trait alloc::alloc::WriteCloneIntoRaw.
• New feature gate: clone_to_uninit

This allows performing make_mut() on Rc<[T]> and Arc<[T]>, which was not previously possible.

---

Previous PR description, now obsolete:

Add {Rc, Arc}::make_mut_slice()

These functions behave identically to make_mut(), but operate on Arc<[T]> instead of Arc<T>.

This allows performing the operation on slices, which was not previously possible because make_mut() requires T: Clone (and slices, being !Sized, do not and currently cannot implement Clone).

Feature gate: make_mut_slice

try-job: test-various

[rustbot]

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

[kpreid]

@rustbot label +T-libs-api

[thomcc]

This is insta-stable, so it needs libs-api FCP.

r? @rust-lang/libs-api

[thomcc]

Rerolling because @BurntSushi doesn't do libs reviews.

r? rust-lang/libs-api

[dtolnay]

How about adjusting the trait bound on make_mut instead? It can be implemented the current way for T: Clone and separately for [T].

impl<T:CanMakeMut,A:Allocator + Clone>Arc<T,A>{pubfnmake_mut(this:&mutSelf) -> &mutT{ ...}}impl<T:Clone>CanMakeMutforT{ ... }impl<T:Clone>CanMakeMutfor[T]{ ... }

[kpreid]

This is insta-stable, so it needs libs-api FCP.

Is it? The current commit only adds unstable inherent methods. There's no trait implementation involved.

How about adjusting the trait bound on make_mut instead?

I like that idea much better since it doesn't add a new name. (Though it would be insta-stable.) I'm not familiar with how introducing a trait like CanMakeMut should work in std; should the trait be (perma?-)unstable or public-in-private sealed?

Actually, the trait could be of more broad use — it seems like it could be a general "clone ?Sized into uninit", which suggests it should be an unstable trait not tied to Rc/Arc in particular. So maybe it becomes core::mem::CloneUnsized or something like that? Is there any existing work on unsized rvalues that has such a trait?

[thomcc]

Oh, it's totally not insta-stable, my bad (sorry, been a draining couple of weeks for me).

Still, I'll leave the review to the assignee at this point.

[dtolnay]

The core::clone module has been looking pretty bare for a decade. 🥳

I would definitely be on board with a trait that allows cloning into a pre-allocated &mut MaybeUninit<Self>. I think it would need to be unsafe trait, right? The calling code needs to be able to do assume_init afterwards without the possibility that the trait impl didn't actually initialize the thing.

I am not familiar with the Rc and Arc code but if such a trait existed, would it be straightforward for their make_mut to allocate a value of size header + mem::size_of_val(&**self), cast the spot for the value to &mut MaybeUninit<T>, and call the CloneUnsized to get the value cloned into there?

[kpreid]

Observation: The new CloneToUninit trait turns out to be identical in signature to the internal trait alloc::alloc::WriteCloneIntoRaw, except for the ?Sizedness. That's a good sign, I think — I'm refining something that already proved useful rather than making up an ad-hoc solution.

[kpreid]

Observation: It's not possible to define these operations in terms of MaybeUninit<T> (as Rc currently does inside maek_mut(), and a previous comment suggested), because MaybeUninit<T> requires T: Sized, so you cannot have a MaybeUninit<T> where T might be [U]. I wonder if changing that would help all maybe-unsized code — but presumably it's difficult, and certainly out of scope for this PR.

[kpreid]

It was quite an adventure, but I believe I've got it all set now. I had to introduce additional helper types at both the CloneToUninit layer and the make_mut() layer in order to avoid leaks on panic.

Note that make_mut()'s enlarged receiver type bounds are insta-stable. Additionally, you can tell that CloneToUninit is specialized in the documentation, but I assume that's fine because it doesn't affect which programs will compile.

[kpreid]

Updated PR description with the new design based on CloneToUninit.

[matthiaskrgr]

@bors r-
failed in #126821

[kennytm]

Suggested change

	use core::sync::atomic::{AtomicUsize,Ordering::Relaxed};
	#[cfg(panic = "unwind")]
	use core::sync::atomic::{AtomicUsize,Ordering::Relaxed};

(or move the use inside test_clone_to_uninit_slice_drops_on_panic())

 error: unused imports: `AtomicUsize` and `Ordering::Relaxed`
--> library/core/tests/clone.rs:3:26
|
3 | use core::sync::atomic::{AtomicUsize, Ordering::Relaxed};
| ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^
|
= note: `-D unused-imports` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(unused_imports)]`

[kpreid]

Fixed, I hope.

Is there a way I could check this locally? I tried ./x test library/core --target=wasm32-unknown-unknown but it fails on profiler_builtins before getting to emitting warnings for core.

[dtolnay]

@bors r+

[bors]

📌 Commit 88c3db5 has been approved by dtolnay

It is now in the queue for this repository.

[bors]

⌛ Testing commit 88c3db5 with merge f944afe...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing f944afe to master...

[rust-timer]

Finished benchmarking commit (f944afe): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.3%, 2.2%]	13
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.3%, 2.2%]	13

Max RSS (memory usage)

Results (primary -5.7%, secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.7%	[-5.7%, -5.7%]	1
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-5.7%	[-5.7%, -5.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.1%, secondary 0.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.3%]	8
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.3%]	15

Bootstrap: 694.577s -> 694.429s (-0.02%)
Artifact size: 327.03 MiB -> 326.84 MiB (-0.06%)

[Mark-Simulacrum]

Regressions are mostly in doc benchmarks, seem likely to be just new docs due to extra stuff in the standard library.