Generalise (most) impls on Box

[nia-e]

View all comments

Resolves the trait impl concern for #156882; Default wasn't touched since changing that would be breaking, but hopefully all of this should be fine - I'll do a crater run to be sure. Certain impls that were for one reason or another Annoying to generalise (e.g. Clone for Box<Path>) were also untouched since I believe we can add those in the future, as they're on concrete types.

r? clarfonthey

[nia-e]

also should do a perf run since monomorphisation.

@bors try @rust-timer queue

[clarfonthey]

r=me pending crater + perf

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0c3b2c2 (0c3b2c226322b623e493b7611e4909ee6bc6a48d)
Base parent: 344f790 (344f7902949345394fa40a5d7dda31f012ccbc0d)

[nia-e]

@craterbot check

[craterbot]

👌 Experiment pr-161946 created and queued.
🤖 Automatically detected try build 0c3b2c2
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (0c3b2c2): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Max RSS (memory usage)

Results (primary 3.2%, secondary -0.3%)

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

	mean	range	count
Regressions ❌ (primary)	9.0%	[9.0%, 9.0%]	1
Regressions ❌ (secondary)	4.1%	[1.6%, 8.7%]	5
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-4.6%	[-5.7%, -2.4%]	5
All ❌✅ (primary)	3.2%	[-2.6%, 9.0%]	2

Cycles

Results (primary 2.7%, secondary -11.8%)

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

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-11.8%	[-20.6%, -3.9%]	7
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Binary size

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

Bootstrap: 475.673s -> 475.454s (-0.05%)
Artifact size: 403.60 MiB -> 402.86 MiB (-0.18%)

[nia-e]

that's a nothingburger, nice. time to just wait a zillion years for crater lol

[theemathas]

Is there a reason impl<T> From<T> for Box<T> wasn't generalized?

And how is changing the Default impl breaking?

[zachs18]

I'm think this is the issue Nia's referring to: the Default impl would break type inference in some cases (playground). The From impl could also have the same issue (playground). So, those impls would probably need at least a crater run.

cc https://faultlore.com/blah/defaults-affect-inference/ which discusses the same issue for why Default for Vec<T, A> wasn't generalized (yet ™️ ).

[clarfonthey]

Is there a reason impl<T> From<T> for Box<T> wasn't generalized?

And how is changing the Default impl breaking?

Inference basically. It becomes ambiguous in allocator what a default box might mean, or a box converted from another type.

[theemathas]

@craterbot cancel

See #162233

[craterbot]

🗑️ Experiment pr-161946 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[theemathas]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-162233-crater-rollup/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-161946 created and queued.
🤖 Automatically detected try build 0c3b2c2
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[nia-e]

looked at the regression list on the rollup and i think this pr was entirely harmless? if so yay

[craterbot]

🚧 Experiment pr-161946 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-161946 is completed!
📊 0 regressed and 0 fixed (7794 total)
📊 773 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-161946/retry-regressed-list.txt ↩

[clarfonthey]

Needs rebase for stable API timestamp (and thus has to be rollup=never) but otherwise good

[clarfonthey]

Side note: confused myself into thinking this needed FCP but that's included as part of the allocator FCP

[nia-e]

yeah this is not gonna need rebase dw. per the above,

@bors r=clarfonthey

[rust-bors]

📌 Commit f01082b has been approved by clarfonthey

It is now in the queue for this repository.

[clarfonthey]

Right, I guess this doesn't add new stable API.

[lcnr]

even if this didn't cause any breakage on crater, generalizing these impls does cause potential inference breakage and might have been worth an FCP, does it not? Marking it as relnotes at least :>

[clarfonthey]

Note: these are part of the allocator FCP which is currently happening FWIW.