Open scottmcm opened 10 months ago
Also add reference versions to get &'static mut Z and such.
I don't think this is neccessary as one can just do Box::leak(Box::new(Z))
which is safe and does't allocate.
Edit: I guess you'd need it for no_std
no_std
could hypothetically use Box::leak(Box::new_in(Z, AlwaysFailsAllocator))
.
But that's still different because it needs an instance. I suppose one could Box::leak(Box::new(conjure_zst::<Z>()))
, but avoiding incantations like that are part of what I was hoping to do with this API.
(That said, I'm still considering the reference versions out of scope for this ACP, unless libs-api says otherwise.)
The value of including reference-returning versions would be to enable creating ZST refs while communicating clearly to readers that no allocation is happening, and also discoverability; searching std
docs won't show you the Box::leak()
trick. Redundant-but-better-named functions are not novel in the standard library: std::iter::once()
is almost the same as Option::into_iter()
, and drop()
doesn't need to exist at all.
(But perhaps most of the value would come from documenting these tricks in conjure_zst()
's function documentation.)
I'm skeptical that this is something we truly want. While I understand that the docs would certainly say that it has to uphold all safety invariants of the produced type, I feel that a nontrivial number of people will either overlook that requirement, ignore it, or not bother to check what the safety requirements actually are.
a nontrivial number of people will either overlook that requirement, ignore it, or not bother to check what the safety requirements actually are.
I feel like this can be applied to almost every unsafe fn
. I've seen a lot of code using mem::transmute
incorrectly for example by transmuting between repr(Rust)
types.
Having a well documented function gives us where to point when we spot UB.
mem::transmute
is also described in its documentation as an "absolute last resort" and is something that is essentially required for a number of situations.
I feel that a nontrivial number of people will either overlook that requirement, ignore it, or not bother to check what the safety requirements actually are.
I suspect that anyone who writes unsafe { conjure_zst() }
would also not hesitate to write unsafe { MaybeUninit::<Z>::uninit().assume_init() }
. The latter's documentation only mentions being wary of "additional invariants," and some users (e.g. see the linked internals.rust-lang discussion) aren't sure whether ZSTs can have invariants at all since they can't have an invalid bit pattern.
I've seen several cases of people asking about using transmute or other tricks to produce a ZST. The question they manage to think of is “Is this UB?”, which is a good thing to think about, but having to make that journey distracts them from the more important question — “is this correct for the ZST type?” Having an official function for conjuring ZSTs, whose documentation tells them about the actually important question, will be a usability improvement (as well as a readability improvement as discussed in my previous comment).
(On the other hand, I've never seen someone ask about conjuring a ZST where they were intending to do it for a third-party type that it'd be invalid for. So perhaps this isn't important in practice.)
I think I can mostly sum up my position in a single question. If a library wanted to permit downstream users to create a ZST out of thin air, why wouldn't they expose that behavior themself? Uses for within the same crate aren't a concern, given that they can just pub(crate)
the ZST.
I feel that a nontrivial number of people will either overlook that requirement, ignore it, or not bother to check what the safety requirements actually are.
What I would hope is that the mere fact that it's unsafe helps people have the "wait, why isn't it just safe to do that?", rather than just reaching for mem::zeroed::<Z>()
because they "know" that that's sound because they checked the size_of
.
With highly-flexible unsafe methods it's easy to think "well of course it's unsafe because of other cases, but not in this one", whereas with a hyper-specific specific method if it's unsafe
it's more obviously because the operation is fundamentally unsafe.
If a library wanted to permit downstream users to create a ZST out of thin air, why wouldn't they expose that behavior themself?
I wholeheartedly agree that Z::default()
is better when possible.
However, I point to things like the linked vec::IntoIter
example that are dealing in generic types and justifiably know that it's sound to return the Z
instances, but also very much want to handle non-Default
non-Copy
generic types that aren't going to have any safe way to do this.
We discussed this ACP in last week's libs-API meeting. Overall, we were positive on an unsafe function in core::mem for obtaining a value of ZST type, and there were no objections to conjure_zst::<Z>()
as a name for this.
We spent a while debating whether it would be more valuable as a wrapper around transmute::<(), Z>(())
(compile-time error if Z isn't an (inhabited??) ZST) vs around assert
+ zeroed::<Z>()
(runtime panic).
My initial impression was that conjure_zst
has commonality with the array_chunks
precedent where the team decided that allowing array_chunks::<0>
to compile in a not-taken branch was important (see https://github.com/rust-lang/rust/pull/99471#issuecomment-1544685018). The same thing is also identified as being desirable in this ACP under the Alternatives.
if mem::size_of::<T>() == 0 {
conjure_zst::<T>()
} else {
// something else
}
Mara brought up that she has some cases in her codebase where conjure_zst
would be nice to have, and all of them involve a specific zero-sized type, not conjuring an arbitrary generic type if it happens to be zero sized. I am familiar with one case like this in my work codebase involving C++ FFI.
#[derive(Copy, Clone)]
#[non_exhaustive]
pub struct DidInit;
// Call this once at program startup
pub fn init() -> DidInit {
extern "C" {
init(); // expensive
}
unsafe { init(); }
DidInit
}
// Called often in hot code
pub fn do_interesting_thing(_proof: DidInit) {
extern "C" {
fn do_interesting_thing(); // SAFETY: requires init to have been called
}
// No runtime enforcement needed!
unsafe { do_interesting_thing(); }
}
// Most callers will thread a DidInit through from main to all
// the places that need the proof that init has been called. But
// some places (such as, behind certain third-party traits) it's
// just hard to get to and so they want to unsafely assume that
// init has been called. `do_interesting_thing(conjure_zst())`
At this point I rethought and leaned toward having a conjure_zst
that replaces transmute(())
would be more valuable than one that replaces zeroed()
. The cases that have been doing if size_of::<T>() == 0
could continue to use zeroed::<T>()
. That's less bad than making someone decide whether they want to write do_interesting_thing(conjure_zst::<DidInit>())
(to take advantage of the sweet new name, despite losing checking) vs do_interesting_thing(transmute(()))
(in spite of the inferior spelling, to more likely detect problem if the Rust binding is ever refactored to a non-ZST design).
A conjure_zst
that replaces transmute(())
is the one that benefits more from being in the standard library and leveraging transmute-like dedicated diagnostics as an intrinsic. Meanwhile a conjure_zst
that replaces zeroed()
(impl) serves just as a minor convenience. (And if a primary motivation for having conjure_zst
in the first place is to have a place to document why people shouldn't assume that any ZST is always safe to conjure, both ways are equally suitable for that.)
We didn't end up committed to either approach yet. I think we'll bring it up again in another meeting.
After the meeting I read through all the related links:
and still not sure what to think. But supporting if size_of::<T>() == 0 { conjure_zst::<T>() }
seems well-motivated, including in standard library code.
One novel alternative was brought up later on Zulip by @the8472.
// alloc::vec::IntoIter::next, for example
trait IntoIterImpl {
fn do_next<T>(i: &mut IntoIter<T>) -> Option<T>;
}
struct IsZeroSized<const IS: bool>;
impl IntoIterImpl for IsZeroSized<true> {
fn do_next<T>(i: &mut IntoIter<T>) -> Option<T> { ... }
}
impl IntoIterImpl for IsZeroSized<false> {
fn do_next<T>(i: &mut IntoIter<T>) -> Option<T> { ... }
}
IsZeroSized::<{ mem::size_of::<T>() == 0 }>::do_next(self)
If trait impls for Struct<true>
+ Struct<false>
together unify into one impl that's usable as if it were for <const B: bool> Struct<B>
, that might be even better suitable for this stuff in the standard library than if size_of::<T>() == 0 { ... }
would be. @scottmcm pointed out this would save so much MIR generation in things like the slice iterators.
Maybe this is "future work" more so than "alternative", because in that snippet, IsZeroSized<true>::do_next
would most likely still end up containing a conjure_zst<T>()
if it existed.
Most callers will thread a DidInit through from main to all the places that need the proof that init has been called. But some places (such as, behind certain third-party traits) it's just hard to get to and so they want to unsafely assume that init has been called.
do_interesting_thing(conjure_zst())
This particular case seems to me better served by the library adding an unsafe function DidInit::unchecked_did_init()
than by suggesting the caller conjure the ZST. That way, the library can document reasonable usage, and if the invariants change such that particular attention needs to be drawn, choose to remove or rename the function as a breaking change.
if size_of::<T>() == 0 { conjure_zst::<T>() }
Idea: What if conjure_zst()
was statically checked, but there was another function for this case?
/// If `T` has zero size, then return an instance of `T`.
///
/// This function always returns a fully initialized value and cannot
/// cause undefined behavior solely by being called, but it is still unsafe
/// because the type may have particular invariants. ...
pub unsafe fn conjure_if_zst<T>() -> Option<T> {
if size_of::<T>() == 0 {
Some(unsafe { zeroed::<T>() })
} else {
None
}
}
This would serve the run-time-branching cases directly, giving them a useful documented function to use; conjure_if_zst().unwrap_or_else(more_expensive_construction)
. On the other hand, it might be less clear to read than the visible if
condition.
Proposal
Problem statement
People are often unsure about what the rules are for creating a ZST. See, for example, https://internals.rust-lang.org/t/equality-of-zst-values/19815/3?u=scottmcm, where the person made an unsound function. (Thankfully they made the thread to ask about it, rather than just assuming it was fine!)
There's lots of ways that you could make a ZST:
mem::uninitialized::<Z>()
,mem::zeroed::<Z>()
,MaybeUninit::<Z>::uninit().assume_init()
,*NonNull::<Z>::dangling().as_ptr()
,mem::transmute_unchecked::<_, Z>(())
, etc.It would be nice to have one obvious API to point people at that would also serve as a place to put documentation about the safety and validity invariants of ZSTs, and thus the necessary preconditions for calling the method.
Motivating examples or use cases
unsafe
code often has different paths for ZSTs where it wants to make items, such as https://github.com/rust-lang/rust/blob/9bd71afb90c2a6e0348cdd4a2b10a3bf39908f19/library/alloc/src/vec/into_iter.rs#L196-L197This can show up in const generic code making arrays, too, where in an
if N == 0 { return []; }
doesn't work, so a "make a ZST" API could be nice to use there too: https://github.com/rust-lang/rust/blob/4654a910018cf0447db1edb2a46a0cae5f7dff8e/library/core/src/array/mod.rs#L801-L804Solution sketch
The implementation of which would use (directly or indirectly) things like https://doc.rust-lang.org/nightly/std/intrinsics/fn.assert_inhabited.html to give good error messages for hitting
conjure_zst::<!>()
. And it would probably have a runtime trap (ud2
) for it not actually being a ZST, since that's checkable at monomorphization time, and thus would be cheap in practice.The docs would have text like https://github.com/rust-lang/rust/pull/95385/files#diff-02049689ae3bb7ac98af3418ad8fdca219bfa1227cb6cad31afc72bdbae30e27R681-R717
Alternatives
const { assert!(T::IS_ZST) }
. I didn't pick that one because that would keep it from being used inif T::IS_ZST { ... }
blocks, like the array example.&'static mut Z
and such.Links and related work
A few more threads I found: https://internals.rust-lang.org/t/is-synthesizing-zero-sized-values-safe/11506?u=scottmcm https://internals.rust-lang.org/t/static-mutable-references-to-zsts/18330?u=scottmcm
What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution: