Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mem::conjure_zst for creating ZSTs out of nothing #95385

Closed
wants to merge 3 commits into from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Mar 27, 2022

This adds a new unsafe fn to mem which can create instances of ZSTs (and only ZSTs).

I think this is valuable for a few reasons:

  1. It avoids wondering whether it's best to do this with zeroed() or uninitialized() or new_uninit().assume_init() or transmute_copy or ... when you're writing code. (See, for example, this PR conversation Add internal collect_into_array[_unchecked] to remove duplicate code #82098 (comment) )
  2. It makes it clearer to the reader of the code what's happening, rather than making them get distracted wondering "wait, why is zero-init okay here?" or similar.
  3. It gives a good place to describe the soundness restrictions on this operation, since it's easy to mistakenly think "oh, it's a ZST, I'm sure it's sound to just create one no matter what". (cc @RalfJung , in case you'd like to check whether what I wrote is sufficient.)

TL/DR: I think this is a useful piece of "good evil".

The PR also uses it in a few places in the library I quickly spotted where we need to create instances after size_of::<T>() == 0 runtime checks, if you'd like to see usage examples.

Naming

To conjure is to summon by or as if by invocation or incantation.

Given that we have transmutation already, I thought that conjuration fit well for this operation -- the method is creating it "from nothing", as opposed to the "from something else" of mem::transmute. (This seems a pretty pervasive use, here's a game example of conjuration to add to the previous book one.)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 27, 2022
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2022
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

The docs LGTM.

Currently the function states "size_of::<T>() must be zero" as a safety precondition but also actually checks this condition at runtime. If the intention is that this must be ensured be the caller (i.e., if this truly is a safety precondition), then it might be good to just add an explicit comment in the code saying that the assert! is "because we can" but not a guarantee exposed to clients.

This could be made guaranteed in future because it's UB to fail it right now, but the other way would introduce unsoundness, so this is the best way to add it for now.
Apply suggestions from code review.

Co-authored-by: kennytm <kennytm@gmail.com>
@scottmcm
Copy link
Member Author

scottmcm commented Apr 2, 2022

One could also think of this method as part of a future set for

(Standard “Zero-Sized-Types get to cheat and lie” caveats apply, although it may be desirable to give them their own API just to make that 100% clear.)

as mentioned in https://doc.rust-lang.org/nightly/std/ptr/fn.invalid.html

For example, there could be unsafe fn conjure_zst_ref<'a, T>() -> &'a T (and _mut) but I'm not going to include that in this PR.

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 6, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 20, 2022
@bors
Copy link
Contributor

bors commented Jun 25, 2022

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

@JohnCSimon
Copy link
Member

Ping from triage:
@scottmcm - can you address the merge conflict

@scottmcm
Copy link
Member Author

scottmcm commented Oct 1, 2022

I probably need to make an ACP for this now, so I'll just close the PR and decide whether to try again later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants