Do not allocate for ZST ThinBox (attempt 2 using const_allocate)

[stepancheg]

There's PR #123184 which avoids allocation for ZST ThinBox.

That PR has an issue with unsoundness with padding in MaybeUninit (see comments in that PR). Also that PR relies on Freeze trait.

This PR is much simpler implementation which does not have this problem, but it uses const_allocate feature.

@oli-obk suggested that const_allocate should not be used for that feature. But I like how easy it to do this feature with const_allocate. Maybe it's OK to use const_allocate while ThinBox is unstable? Or, well, we can abandon this PR.

r? @oli-obk

[workingjubilee]

I would strongly prefer that we not have an implementation that is only justifiable by the surface being unstable as well. It's one thing to have it be somewhat experimental.

[workingjubilee]

...on second thought, I took a look at the other PR and it seems to be so radioactive its like I saw the Elephant's Foot. If you can sell oli on this one, then carry on.

[RalfJung]

Maybe it's OK to use const_allocate while ThinBox is unstable? Or, well, we can abandon this PR.

Only if you make sure that ThinBox will not be stabilized in this form, and promise that you are okay with removing this optimization again without hesitation if we change const_allocate in ways that make it not work any more.

It is nice to see experimentation with const_allocate and that it can be useful even in its current form. But I want to be sure we are not locked-in to its current form either.

[stepancheg]

@RalfJung

Only if you make sure that ThinBox will not be stabilized in this form, and promise that you are okay with removing this optimization again without hesitation if we change const_allocate in ways that make it not work any more.

If it comes to stabilization, I'd prefer to not stabilize ThinBox without this optimization: it should either work optimally, or not work at all.

But my opinion has little weight.

It is nice to see experimentation with const_allocate and that it can be useful even in its current form.

Yes, I think this is useful as

• illustration of this feature
• a test for the feature

[oli-obk]

yea, this seems fine to me.

Things to do in this PR

• The logic should live in a WithHeader method though, as this breaks abstraction boundaries.
• Add miri tests
• add a blocker to the thinbox tracking issue to have made enough progress on const_allocate to justify its correct handling by the rest of the compiler and the fact that we'll always have it or an equivalent to it.

Things to do in follow-up PRs

• move WithHeader into its own module to keep all the finicky logic in one place.

[stepancheg]

The logic should live in a WithHeader method though, as this breaks abstraction boundaries.

Done.

Add miri tests

Give pointers where how to do that please?

add a blocker to the thinbox tracking issue

Is that something I can do?

move WithHeader into its own module to keep all the finicky logic in one place

Like submodule mod with_header? Or a separate file next to thin_box.rs?

[oli-obk]

Is that something I can do?

I added it

Like submodule mod with_header? Or a separate file next to thin_box.rs?

if a submodule works out nicely with visibility, that's prefereable imo

@bors r+ rollup

[bors]

📌 Commit f539134 has been approved by oli-obk

It is now in the queue for this repository.