Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper by maxdexh · Pull Request #162849 · rust-lang/rust

maxdexh · 2026-09-16T10:54:41Z

Follow-up to #162804

This is only the first step in actually refactoring Box, and is essentially just a rename.
The layout of Box stays as-is for now, due to #162850.

Needs perf run

rustbot · 2026-09-16T17:22:14Z

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustbot · 2026-09-16T17:22:17Z

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 76 candidates
Random selection from 20 candidates

maxdexh · 2026-09-16T17:22:36Z

@hanna-kruppe do you want to take this one too?

r? hanna-kruppe

Reroll libs if not, please (sorry, should have asked before marking the PR as ready)

maxdexh · 2026-09-16T17:23:05Z

ignore rustbot, all changes outside libs were to test files ^^

rustbot · 2026-09-16T17:25:17Z

hanna-kruppe is currently at their maximum review capacity.
They may take a while to respond.

hanna-kruppe · 2026-09-18T20:14:23Z

@bors try @rust-timer queue

Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper

rust-bors · 2026-09-18T21:14:40Z

☀️ Try build successful (CI)
Build commit: 5b25bbf (5b25bbfa1be1e911397b56089384cce001a1a9e2)
Base parent: 420ed2a (420ed2a0c3d7225b1744266fd884d431b4d8cfe0)

rust-timer · 2026-09-18T21:58:10Z

Finished benchmarking commit (5b25bbf): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.3%, 0.5%]	3
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.3%]	2
All ❌✅ (primary)	0.4%	[0.3%, 0.5%]	3

Max RSS (memory usage)

Results (primary -2.9%, secondary 1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.6%, 3.6%]	2
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Cycles

Results (secondary 0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[4.2%, 4.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.8%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.3%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.5%, -0.1%]	44
Improvements ✅ (secondary)	-0.4%	[-0.8%, -0.2%]	14
All ❌✅ (primary)	-0.3%	[-1.5%, -0.1%]	44

Bootstrap: 498.333s -> 498.695s (0.07%)
Artifact size: 408.93 MiB -> 408.97 MiB (0.01%)

Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper

RalfJung · 2026-09-20T14:09:33Z

The answer is no, the 2nd try run cancels the first.

rust-bors · 2026-09-20T15:01:22Z

☀️ Try build successful (CI)
Build commit: ee81c60 (ee81c60a6e1754205bd38a27d0af4d502a4e1b42)
Base parent: fc7358c (fc7358c9223bbf6b30741438fc8588dad7e4671c)

rust-timer · 2026-09-20T15:45:10Z

Finished benchmarking commit (ee81c60): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.6%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	9
All ❌✅ (primary)	0.4%	[0.2%, 0.6%]	4

Max RSS (memory usage)

Results (primary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.6%, 3.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.8%	[-3.9%, -2.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-3.9%, 3.1%]	6

Cycles

Results (secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.7%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.4%]	64
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	48
Improvements ✅ (primary)	-0.3%	[-1.3%, -0.0%]	23
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.0%]	9
All ❌✅ (primary)	0.0%	[-1.3%, 0.4%]	87

Bootstrap: 500.549s -> 509.726s (1.83%)
Artifact size: 408.70 MiB -> 408.81 MiB (0.03%)

maxdexh · 2026-09-20T16:02:55Z

I guess that's better? It's not really what I expected to happen but okay

maxdexh · 2026-09-20T16:03:17Z

@bors try jobs=test-aarch64-apple-1,test-x86_64-msvc-1

Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper try-job: test-aarch64-apple-1 try-job: test-x86_64-msvc-1

rust-bors · 2026-09-20T19:01:11Z

☀️ Try build successful (CI)
Build commit: 76a8f8a (76a8f8ab565a4f05ad35f3fb67dfca69022a93e6)
Base parent: fc7358c (fc7358c9223bbf6b30741438fc8588dad7e4671c)

maxdexh · 2026-09-20T19:14:15Z

Awesome, I think this is ready then ^^

rustbot · 2026-09-25T15:43:04Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rust-bors · 2026-10-02T05:01:58Z

☔ The latest upstream changes (presumably #163609) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

hanna-kruppe

Left a bunch of nits, thinking-out-loud, and questions.

Overall, looking at the size of the test diffs, parts of those diffs being unexpected, and the not-totally-clean perf run, I think it would be better if this PR did much less drive-by refactoring. Let's first land BoxRaw with identical API to Unique, so that all Box code remains the same w.r.t. source code structure, MIR scopes, MIR optimizations, etc. -- the diff is large enough with just this renaming. Any simplification enabled by having the dedicated type in the same module are better done in follow-up PRs where the effects of each change can be isolated.

@rustbot author

View changes since this review

hanna-kruppe · 2026-10-03T10:09:45Z

+}
+impl<T: ?Sized> Clone for BoxRaw<T> {
+    #[inline]
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl<T: ?Sized> Copy for BoxRaw<T> {}


nit: having all of this without any blank lines looks cramped and unconventional to me. Probably the one-line marker trait impls can be smushed together, but I'd put blank lines between struct / impl Clone and between impl Clone / marker impls.

I just noticed, the Clone/Copy impls are actually unused I think 😅

hanna-kruppe · 2026-10-03T10:20:40Z

+unsafe impl<T: ?Sized + Send> Send for BoxRaw<T> {}
+unsafe impl<T: ?Sized + Sync> Sync for BoxRaw<T> {}
+impl<T: ?Sized + core::panic::UnwindSafe> core::panic::UnwindSafe for BoxRaw<T> {}
+impl<T: ?Sized, U: ?Sized> CoerceUnsized<BoxRaw<U>> for BoxRaw<T> where T: Unsize<U> {}
+impl<T: ?Sized, U: ?Sized> DispatchFromDyn<BoxRaw<U>> for BoxRaw<T> where T: Unsize<U> {}


I notice that Unique has a slightly weaker bound on the corresponding impls (PointeeSized as opposed to ?Sized which I believe means MetaSized). I don't think this makes a difference for Box, but I've been surprised before.

Box has the same bound (?Sized), so I don't think this makes a difference. I'll throw the weaker bound on there to check if it makes a difference for compile times (since new-solver is apparently hyper-sensitive to everything)

hanna-kruppe · 2026-10-03T10:27:16Z

+        // - Casting `[u8]` to `str` is correct
+        // - The empty byte slice is valid UTF-8
+        unsafe {
+            let ptr: *mut [u8] = NonNull::<[u8; 0]>::dangling().as_ptr();


(Note: this comment applies to a refactoring I'd rather not do in this PR, but keeping it here for the sake of follow-up PRs.)

Why the round-trip NonNull -> raw -> NonNull? Unfortunately NonNull::cast doesn't work for [u8] -> str, but it seems simpler to start with ptr::dangling().

ptr::dangling does the same thing (call NonNull::dangling). I didn't want to regress this impl again by introducing a call... I suspect this will always be ugly

hanna-kruppe · 2026-10-03T10:31:02Z

+        unsafe {
+            let ptr: *mut [u8] = NonNull::<[u8; 0]>::dangling().as_ptr();


(Note: this comment applies to a refactoring I'd rather not do in this PR, but keeping it here for the sake of follow-up PRs.)

I think this unsafe block has too large scope. The let ptr ... part isn't unsafe at all. But the NonNull::new_unchecked technically has a precondition not mentioned in the safety comment. I'd suggest splitting this up into two steps:

Construct the NonNull<str> (only new_unchecked needs unsafe)

Construct the Box(BoxRaw { ... }) where most of the current safety comments apply

ideally this would just use Box<[u8]> default and use the boxed from utf8 unchecked function, but that involves a lot of unnecessary round trips that regress debug mode

hanna-kruppe · 2026-10-03T12:26:28Z

-          _4 = const std::ptr::Unique::<[bool; 0]> {{ pointer: NonNull::<[bool; 0]> {{ pointer: {0x1 as *const [bool; 0]} is !null }}, _marker: PhantomData::<[bool; 0]> }};
+          _4 = const NonNull::<[bool]> {{ pointer: Indirect { alloc_id: ALLOC0, offset: Size(0 bytes) }: pattern_type!(*const [bool] is !null) }};


Hm, this change is a bit unexpected to me. I think it's because the change to impl Default for Box<[T]> makes the unsizing happen earlier:

Before: NonNull<[T; 0]>::dangling() is used in Unique<[T; 0]>::dangling() is coerced to Unique<[T]>

Now: NonNull<[T; 0]> is coerced to NonNull<[T]> because it's used in BoxRaw struct literal with expected type BoxRaw<[T]>

I'm not sure if that's better or worse. I think it's probably marginally better to unsize later, but mostly I'd like to make sure I understand what's happening. Can you try changing impl Default to stick closer to the original version (create BoxRaw<[T; 0]> and then unsize) to check this theory?

Hm, that's interesting, I intended to match the old function as closely as possible. I'll try to make it match more closely and rerun perf

hanna-kruppe · 2026-10-03T12:44:24Z

+        goto -> bb4;
+    }
+
+    bb4: {
+        StorageDead(_2);


Needing an extra block here seems unfortunate. I guess it's a consequence of the corresponding StorageLive statements not being in the same block any more? But I don't know why they aren't. Any ideas?

I guess mir-opt prefers the old code, which accessed the nonnull field twice, instead of my ahead of time ptr = self.0.pointer? strange, will see if separate accesses give better codegen

hanna-kruppe · 2026-10-03T12:56:37Z

        }
        scope 18 (inlined <Enumerate<std::slice::Iter<'_, T>> as Iterator>::next) {
-            let mut _22: std::option::Option<std::convert::Infallible>;
+            let mut _22: std::option::Option<!>;


I don't understand how this PR possibly could've caused this change, that's concerning.

those tests weren't getting run (was fixed recently), this should fix itself when i rebase