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%)

hanna-kruppe

Much closer to what I had in mind, but there's still one change that I'd rather not have in this PR.

View changes since this review

hanna-kruppe · 2026-10-10T16:44:25Z

            // of this box and `layout` would fit that allocation. We also are the only ones
            // responsible for doing this deallocation and know that the pointer must be valid.
            unsafe {
-                self.1.deallocate(From::from(ptr.cast()), layout);
+                self.1.deallocate(ptr.cast().as_non_null_ptr(), layout);


Test diff would be even smaller if this line was unchanged and the From impl used as_non_null_ptr (like the impl From<Unique> for NonNull does).

maxdexh · 2026-10-10T16:50:26Z

Ah right, I did this initially but then I got rid of it. When I impl From<BoxRaw> for NonNull, I get

error: implementation has missing stability attribute
   --> library/alloc/src/boxed.rs:283:1
    |
283 | impl<T: ?Sized> From<BoxRaw<T>> for NonNull<T> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

But when I do that, I get

error: an `#[unstable]` annotation here has no effect
   --> library/alloc/src/boxed.rs:283:1
    |
283 | #[unstable(feature = "box_internals", issue = "none")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information
    = note: `#[deny(ineffective_unstable_trait_impl)]` on by default

I think this is just broken

maxdexh · 2026-10-10T16:53:31Z

#[inline]
fn non_null_from_box_raw<T: ?Sized>(ptr: BoxRaw<T>) -> NonNull<T> {
    ptr.as_non_null_ptr()
}

I can do this, but that seems kinda silly

RalfJung · 2026-10-10T17:01:54Z

Yeah that seems broken, please file a bug for the stability attribute problem.

hanna-kruppe · 2026-10-10T17:03:17Z

I see a few other impls involving unstable types #[expect(..)] the lint. Maybe a workaround for the bug, but should also work here, right?

maxdexh · 2026-10-10T17:10:50Z

I see exactly one such place, but sure

@rustbot ready

hanna-kruppe · 2026-10-10T17:11:35Z

You're gonna have to bless a bunch of tests again (miri and mir-opt at least)

maxdexh · 2026-10-10T17:11:47Z

😭

hanna-kruppe · 2026-10-10T18:02:22Z

Great, thanks! I don't see anything worrying in the test diffs any more, and the last perf run was neutral except for two tiny rustdoc regressions that I'm inclined to believe are noise or otherwise negligible consequence of having a new type at all.

@bors r+ rollup=iffy

rust-bors · 2026-10-10T18:02:25Z

📌 Commit 5d260fd has been tentatively approved by hanna-kruppe

It will be put into the queue for this repository once PR CI succeeds.

mejrs · 2026-10-10T18:33:12Z

+#[expect(ineffective_unstable_trait_impl, reason = "See #164109")]
+#[unstable(feature = "box_internals", issue = "none")]
+impl<T: ?Sized> From<BoxRaw<T>> for NonNull<T> {
+    fn from(value: BoxRaw<T>) -> Self {
+        value.as_non_null_ptr()
+    }
+}


Maybe this is just me missing something (haven't read all the linked discussion) but I don't understand why this implementation needs to exist. It is easier to just use as_non_null_ptr in Box's drop impl.

View changes since the review

We found that this code is so sensitive to minimal changes that Hanna suggested I just copy the structure of the code 1-to-1. This can be changed in a follow-up

This impl and a bunch of other API surface on BoxRaw isn't expected to remain for long. But doing that kind of cleanup at the same time as the main point of this PR caused more & less obvious test diffs as well as more rustc-perf effects. So I suggested landing this like this first (which might take a while too -- I would be surprised if this lands cleanly on first try), and do the cleanup in one or several follow up PRs.

edit: too slow :)

I actually did use as_non_null_ptr before this comment: #162849 (review)

My next 2 PRs, which I'll write in parallel, will be to remove BoxRaw (flattening the fields of Box) and Unique. The former will be pretty invasive (and might end up just not working reasonably well), so yay

…uwer Rollup of 3 pull requests Successful merges: - #164092 (miri subtree update) - #162849 (Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper) - #163900 (Disable default cc optimizations in shell.nix)

Rollup merge of #162849 - maxdexh:nuke-unique-in-box, r=hanna-kruppe Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper Follow-up to #162804 See [zulip](https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Can.20we.20nuke.20.60Unique.60.3F/with/624141418). 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

rust-timer · 2026-10-11T06:06:42Z

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (9404582): comparison URL.

Overall result: ❌ regressions - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

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

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.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.5%]	3

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

This perf run didn't have relevant results for this metric.

Binary size

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)	0.1%	[0.1%, 0.1%]	12
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	12

Artifact size: 407.22 MiB -> 406.59 MiB (-0.15%)

Kobzol · 2026-10-11T15:36:30Z

Tiny doc regressions on hyper and syn. Changes around Box almost always cause perf. perturbations, I don't think that there is more to do here.

@rustbot label: +perf-regression-triaged

…uwer Rollup of 3 pull requests Successful merges: - rust-lang/rust#164092 (miri subtree update) - rust-lang/rust#162849 (Replace `Unique` in `Box` with a `(NonNull, PhantomData)` wrapper) - rust-lang/rust#163900 (Disable default cc optimizations in shell.nix)