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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
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.

@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.

@scottmcm scottmcm marked this pull request as draft December 11, 2024 18:54
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2024
@scottmcm
Copy link
Member Author

(Picking this back up now that the ACP was approved; will need some corresponding updates.)

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 11, 2024
@scottmcm scottmcm removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 11, 2024
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants