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

RFC: Unsafe Set Enum Discriminants #3727

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
320 changes: 320 additions & 0 deletions text/3727-unsafe-set-discriminant.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,320 @@
- Feature Name: unsafe_set_discriminant
- Start Date: 2024-11-08
- RFC PR: [rust-lang/rfcs#3727](https://github.com/rust-lang/rfcs/pull/3727)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

This RFC proposes a way to write the discriminant of an enum when building it "from scratch". This introduces two new library components, an unsafe `set_discriminant` function, and a `discriminant_of!` macro, which can be used with any enum, regardless of `repr` or any other details.

# Motivation
[motivation]: #motivation

At the moment, enums are the only "plain old data" type that cannot be initialized in-place. This is discussed in [this blog post][pods_from_scratch] in more detail, and was [discussed on Zulip]. This RFC is aimed at removing that gap.

In-place construction is desirable for creating objects at their destination, often "by parts" (one piece or sub-component at a time) removing the need for potential on-stack construction, which can cause issues (particularly for large objects) with stack overflows, increased stack usage, or additional copies that cause performance impact.

In-place construction is useful for a number of niche applications, including in-place deserialization, construction of items on the heap, pinned initialization (including [in the Linux kernel]) or when using "outptr" patterns (for FFI or otherwise).

**Simple data types** (integers, booleans, etc.) can be created with `MaybeUninit::uninit` and initialized using pointer operations.

**Composite data types** (structs, tuples) can also be created with `MaybeUninit::uninit`, and `offset_of!` can be used to initialize each of the subcomponents.

With [this feature][feature_offset_of_enum] (currently proposed for FCP merge at the time of writing), it is possible to also use `offset_of!` to **initialize the variant** of an enum in-place, as with structs.

However, there is no stable way to set the discriminant of an enum in the general case, without fully creating the variant and writing it by-value to the destination. This means that any enum value must first be created on the stack, and written through to the final destination, even if the potentially large enum variant could be initialized in place.

For example, TODAY if we had the type `Option<[u8; 1024 * 1024]>`, and would like to initialize it as `Some([0u8; 1024 * 1024])`, this would require creation of the 1MiB array on the stack, before it is written to its destination. Although there are various workarounds to *hopefully* have the compiler elide this copy, they are not guaranteed.

With this RFC, it would be possible to zero-initialize the variant (meaning no 1MiB stack creation), and then set the discriminant to `Some`.

[in the Linux kernel]: https://docs.rs/pinned-init/latest/pinned_init/
[pods_from_scratch]: https://onevariable.com/blog/pods-from-scratch/
[discussed on Zulip]: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Prior.20discussions.20for.20manually.20building.20an.20enum.3F
[feature_offset_of_enum]: https://github.com/rust-lang/rust/issues/120141

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

This introduces two new library components, a `discriminant_of!` macro, and an unsafe `set_discriminant` function.

## The `discriminant_of!` macro

For manual construction of an enum, it is necessary to obtain the discriminant of an enum, without necessarily having an instance of that enum.

This RFC proposes a macro that allows for creation of a [`Discriminant<T>`](https://doc.rust-lang.org/stable/std/mem/struct.Discriminant.html), similar to [`discriminant()`](https://doc.rust-lang.org/stable/std/mem/fn.discriminant.html), but without requiring a reference to the enum.

This would have the following form[^1]:

```rust
// macro: discriminant_of!(Type, Variant Name);

// Usable in normal let bindings
let discriminant: Discriminant<Option<u32>>
= discriminant_of!(Option<u32>, Some);
// ^^^^^^^^^^^ --------> Type
// ^^^^ --> Variant Name

// Also available in const context
const DISCRIMINANT: Discriminant<Result<u32, String>>
= discriminant_of!(Result<u32, String>, Ok);
// ^^^^^^^^^^^^^^^^^^^ ------> Type
// ^^ --> Variant Name
```

If the provided **Type** is not an enum, or the provided **Variant Name** is not present in the list of variants of **Type**, then a compile time error will occur.

```rust
discriminant_of!(u32, Some); // Not allowed
discriminant_of!(Option<u32>, Ok); // Not allowed
```

Compiler errors could look like this:

```sh
discriminant_of!(u32, Some);
^^^ -> ERROR: "u32" is not an enum

discriminant_of!(Option<u32>, Ok);
^^ -> ERROR: type Option<u32> does not contain
the variant "Ok". Variants are "Some" or
"None".
```

[^1]: The syntax of [enum variant offsets][feature_offset_of_enum] is still in discussion, this RFC would adopt whatever syntax decisions made by that feature for consistency. The currently described syntax of that feature is used in this RFC as a placeholder.

## The `set_discriminant` function

This function is used for setting the discriminant of an enum, and has the following signature:

```rust
pub unsafe fn set_discriminant<T: ?Sized>(
*mut T, // The enum being constructed
Discriminant<T>, // The discriminant being set
);
```

This function MUST be called AFTER fully initializing the values of the variant associated with this discriminant, and when called, MUST be called BEFORE use of the enum, for example calling `assume_init` on a `MaybeUninit<T>` or creation of a reference from a pointer to the uninitialized enum.

This could be used as follows:

```rust
let mut out: MaybeUninit<Option<[u32; 1024]>> = MaybeUninit::uninit();
// Make a decision on runtime values
let init_some: Option<u32> = get_user_input();

if let Some(val) = init_some {
let opt_ptr: *mut Option<[u32; 1024]> = out.as_mut_ptr();
// Tracking issue #120141 for enum variant offset_of! definition
let val_offset: usize = offset_of!(Option<[u32; 1024]>, Some.0);
let arr_ptr: *mut [u32; 1024] = opt_ptr.byte_add(val_offset).cast();
let item_ptr: *mut u32 = arr_ptr.cast();
// Initialize all items in the array to the user provided value
for i in 0..1024 {
// SAFETY: The variant body is always well aligned and valid for
// the size of the type, uninit fields are only written.
unsafe {
item_ptr.add(i).write(val);
}
}
// Obtain the discriminant
let discrim = discriminant_of!(Option<[u32; 1024]>, Some);
// Set the discriminant
//
// SAFETY: We have initialized all fields for this variant, and
// this discriminant is correct for the type we are writing to.
unsafe {
set_discriminant(out.as_mut_ptr(), discrim);
}
} else {
// No value to write, just set the discriminant, leaving the
// rest of the value uninitialized
//
// SAFETY: We have initialized all fields for this variant
// (which is 'no fields'), and this discriminant is correct
// for the type we are writing to.
unsafe {
// We can also use discriminant_of! without binding it
set_discriminant(
out.as_mut_ptr(),
discriminant_of!(Option<[u32; 1024]>, None),
);
}
}

// This is now sound. We could also use `assume_init_ref` or
// `assume_init_mut` if we are explicitly avoiding potential
// copies by value, and the allocation is not local.
let out: Option<[u32; 1024]> = unsafe { out.assume_init() };
assert_eq!(out.is_some(), init_some.is_some());
if let Some(val) = init_some {
assert!(out.as_ref().unwrap().iter().all(|x| *x == val));
}
```

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

## The `discriminant_of!` macro

This macro is the "valueless" version of `std::mem::discriminant`. It needs to be a macro, as we cannot otherwise name the variant.

Unlike the `discriminant` function, it is possible at compile time to detect if this is called on a type that is not an `enum`, meaning that it is possible for it to be a compiler error in this case, rather than "unspecified but not UB" to use this with a non-enum type.

This macro returns by value the same opaque `Discriminant<T>` used by `std::mem::discriminant`.

This RFC does not propose allowing obtaining the discriminant of "nested" fields, and will only work with a "top level" item, but does not preclude adding this capability in the future. This addition could be specified in a later RFC. For now users are recommended to handle "one level at a time".

```rust
discriminant_of!(Option<Option<u32>>, Some); // Allowed
discriminant_of!(Option<Option<u32>>, Some.0.Some); // Not Allowed
```

## The `set_discriminant` function

This function must be unsafe, as setting an incorrect discriminant could lead to undefined behavior by allowing safe code to observe incorrectly or incompletely initialized values.

This function takes an `*mut T` and has multiple requirements necessary for safety:

* The pointer `*mut T` must be non-null
* The enum `T` and the selected variant's fields must be well-aligned for reading and writing
* This function is allowed to write and read-back both the discriminant and body (whether they each exist or not, and whether the discriminant and body are separate or not). This function also may do this in an unsynchronized manner (not requiring locks or atomic operations), which means exclusive access is required

If the `T` used for `*mut T` or `Discriminant<T>` when calling this function is NOT an enum, then this function is specified as a no-op. This is not undefined behavior, but later calls to `assume_init` or usage of the value `T` may be undefined behavior if the `T` was not properly initialized. This is also allowed (but not required) to cause a debug assertion failure to aid in testing (implementor's choice).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust does have other types that have discriminants, namely the generators that are created by lowering async. So we have to be a bit careful in how this is worded.


When this function is called, it MUST be called AFTER fully initializing the variant of the enum completely, as in some cases it may be necessary to read back these values. This is discussed more in the next section.

Semantically, `set_discriminant` is specified to optionally write the discriminant (when necessary), and read-back the discriminant. If the read-back discriminant does not match the expected value, then the behavior is undefined.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung I would appreciate if you could let me know if I got this right! I don't totally understand the "mandatory readback", so if this could be stated better, please feel free to propose alternate wording.

Copy link
Contributor

@traviscross traviscross Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good also for this (or the following) section to be extended with more details about why this is the correct semantic. I think I see the case for and agree that setting a MaybeUninit<Option<&u8>> to Some with set_discriminant should represent an assertion that the relevant bits aren't zero at the point of that call, but it's natural to wonder about this, and so it'd be good to describe it in some detail.

Copy link
Member

@RalfJung RalfJung Nov 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds about right, though I would use some of the terms a bit differently. Specifically, set_discriminant will never read or write anything but the discriminant. However, enum layout is unspecified, and the discriminant can overlap with arbitrary values of the body.

It may be worth spelling out exactly what the safety requirements are: when writing the discriminant of variant V, then all fields of V that overlap with the discriminant must be set to values that satisfy the language invariant of their type. In practice, we don't guarantee which fields overlap with the discriminant, so all fields must be properly initialized.

This can replace the discussion of "read back" etc, that is just an implementation detail. Likewise, you can keep the example setting None, but I would frame it differently: because None has no fields, the requirement is trivially met. So the instruction is not "responsible for also initializing the whole body of the variant", all it really has to ensure is that future reads of the discriminant behave correctly.

It'd be good also for this (or the following) section to be extended with more details about why this is the correct semantic.

The reason we introduced the readback is to make it so that

SetDiscriminant(place, variant_idx);
_x = GetDiscriminant(place);

can be optimized to

SetDiscriminant(place, variant_idx);
_x = variant_idx;

This seems like an obviously desirable transformation, so we must ensure that e.g. writing the Some discriminant can only return successfully if the next GetDiscriminant indeed would return Some.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung Would the optimization you have in mind be impeded if we merely say that any enum value which is not wrapped in MaybeUninit is valid? Ie, that initialization of the variant and fields may happen in any order, but that initialization must be complete by the time that MaybeUninit::assume_init is called? That would permit set_discriminant to not need to constrain the existing content of the enum since it would basically just be a fancy ptr::write, and it would make it easier to reason about for callers. I assume there's a reason that this simpler approach isn't viable, but I'm curious what that reason is.

Copy link
Member

@RalfJung RalfJung Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the question. It seems you are proposing to just not do the optimization. But we think we should reserve the possibility to do that optimization, and then it follows immediately that we need the UB.

EDIT: Ah, I misread "impeded". Yes, it would be heavily impeded, the optimization just becomes impossible.

IMO it'd also be rather strange to say that setting the discriminant of Option<&T> to Some on an all-null value is just a NOP and permitted. The programmer asked us to do something nonsensical.


### Alternate forms of `set_discriminant`

This RFC only specifies one form of `set_discriminant`, which takes an `*mut T` to the enum `T`. It is expected that additional versions of this function will also be added for convenience in the future, for example taking other pointer types like:

* `unsafe fn set_discriminant_mu(&mut MaybeUninit<T>, Discriminant<T>);`
* `unsafe fn set_discriminant_nn(NonNull<T>, Discriminant<T>);`

These forms are not specified as part of this RFC, and can be added without an additional RFC in the future. Alternate forms are expected to match the semantics of the pointer-based version of `set_discriminant`.

## Interactions with niche-packed types

This RFC is intended to work with ANY enum types, including those with niche representations. This includes types like `Option<&u32>` or `Option<NonZeroUsize>`, where excluded values (like `null`) are used for the `None` variant.

In these cases, the discriminant and variant body are fully overlapping, rather than being independent memory locations.

This RFC recommends users to explicitly call `set_discriminant`, even if the act of setting the value also sets the discriminant implicitly, for example by writing `null` to the body of `Option<&u32>` via pointer methods or casting.

```rust
let refr: &u32 = &123u32;
let mut out: MaybeUninit<Option<&u32>> = MaybeUninit::uninit();

let opt_ptr: *mut Option<&u32> = out.as_mut_ptr();
// Tracking issue #120141 for enum variant offset_of! definition
let val_offset: usize = offset_of!(Option<&u32>, Some.0);
let val_ptr: *mut &u32 = opt_ptr.byte_add(val_offset).cast();

unsafe {
// Sets the value of the body
val_ptr.write(refr);
// Does not affect the contents of `out`
set_discriminant(
out.as_mut_ptr(),
discriminant_of!(Option<&u32>, Some),
);
}
let out: Option<&u32> = unsafe { out.assume_init() };
assert_eq!(out, Some(refr));
```

In the case of the niche variant, `set_discriminant` would be responsible for also initializing the whole body of the variant. For example, this would be sufficient initialization:

```rust
let mut out: MaybeUninit<Option<&u32>> = MaybeUninit::uninit();
unsafe {
set_discriminant(
out.as_mut_ptr(),
discriminant_of!(Option<&u32>, None),
);
}
let out: Option<&u32> = unsafe { out.assume_init() };
assert_eq!(out, None);
```

## Specifically known types vs unknown types

This RFC does not invalidate any currently-accepted ways of initializing enums manually without these new functions. For example, enums with a [primitive representation], or niche represented enums with [discriminant elision], can be soundly created today.

When specific types with these qualities are used, it is not required, but still allowed, to use the `set_discriminant` function to set the discriminant.

However, when authoring generic or macro code, which may potentially accept types that do not have these qualities, it is necessary to call `set_discriminant` to fully initialize the enum in the general case.

For example, if a macro was used to populate an `Option<U>` value, and users could chose `U: u32` (which does not have a niche repr), OR `U: &u32` (which does have a niche repr), then the author of the macro should call `set_discriminant` to soundly initialize the `Option<U>` in all cases.
Comment on lines +243 to +251
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolreader18 (replying in inline to point to the relevant part of the RFC where this is clarified)

What's the current legality of initializing niche-optimized enums?

let mut x: Option<NonNull<u8>> = None;
(&raw mut x).cast::<NonNull<u8>>().write(NonNull::dangling()); // UB?

For Option<NonNull<u8>> specifically, this is allowed and fully defined. For "null-pointer-optimized"/"discriminant-elision-guaranteed" types T, Option<T> is guaranteed to have the same size, alignment, and function call ABI as T, and the same representation for Some. NonNull is one such type.

The documentation explicitly calls out mem::transmute::<T, Option<T>> as being allowed, but (&raw mut x).cast::<T>().write(t); is effectively the same as x = std::mem::transmute::<T, Option<T>>(t); (with the exception of not dropping the old value of x, which is not relevant for Option<NonNull>), so ptr::writeing a t: T into an Option<T> will give Some(t) for "null-pointer-optimized" T.

(These guarantees also apply to some instantiations of Result; IIUC the guarantees currently do not formally extend to all "Option-like" enums, only to core::option::Option and core::result::Result specifically).


For general enums without this guarantee, or for Option<OtherTypes>, you do need some way to explicitly write the discriminant, even if it is functionally a NOP. For #[repr(Int and/or C)] enums, this can be done without additional language support because the layout and value of the discriminant/tag is guaranteed, but for repr(Rust) enums there is no explicit layout guarantee, so to be fully sound1 and general, set_discriminant is needed.

Footnotes

  1. you could argue that if size_of::<T>() == size_of::<MyEnum<T>>() where MyEnum has a variant ThatVariant { only_field: T }, then that enum must have an elided discriminant, and thus writing a T: t into it writes ThatVariant(t). This is probably sound logic, but since it is based on non-guaranteed behavior it could break if in a compiler update size_of::<T>() != size_of::<MyEnum<T>>() and IMO should not be relied upon unless guarantees are established.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the guarantee was extended to "Option-like" enums recently and there are some in progress documentation updates.


[primitive representation]: https://doc.rust-lang.org/reference/items/enumerations.html#pointer-casting
[discriminant elision]: https://rust-lang.github.io/unsafe-code-guidelines/layout/enums.html#discriminant-elision-on-option-like-enums

# Drawbacks
[drawbacks]: #drawbacks

* This RFC increases the API surface of the standard library
* This RFC adds unsafe methods which must be considered, documented, and tested
* The abilities of this RFC could technically already be done today, either for carefully selected subsets of enums (specific reprs, hand-built enums using explicit discriminant and `union`'d value fields), or be done today less efficiently with explicit value creations and copies
* The features added to this RFC generally only benefit "power users", likely library authors already doing unsafe things

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

This RFC is necessary to "complete" the ability to soundly create enums in-place.

Without this capability, downstream library authors are required to make a choice between:

* Suboptimal stack usage and performance, in the case of by-value initialization
* Restriction of functionality to a subset of enum reprs, or bespoke proxy types that emulate enums to allow for in-place construction

This is necessary to be part of the language and/or standard library as enum representations (at least for the default `repr(Rust)`) are unspecified, and particularly with concepts like niche-packed enums, it is not possible to soundly handle this in the general case of user defined enum types.

I am not aware of any other proposed designs for this functionality.

# Prior art
[prior-art]: #prior-art

The `rkyv` crate defines [proxy enums] with explict `#[repr(N)]` (where N is an unsigned integer) types, in order to allow in-place construction and access

[proxy enums]: https://rkyv.org/format.html

Syntax for `discriminant_of!` is inspired by the in-progress [offset_of_enum feature][feature_offset_of_enum].

# Unresolved questions
[unresolved-questions]: #unresolved-questions

## Should we provide alternate forms of `set_discriminant`?

There was some discussion when writing this RFC what the type of the first argument to `set_discriminant` should be:

* `&mut MaybeUninit<T>`
* `NonNull<T>`
* `*mut T`

`*mut T` was chosen as the most general option, however it is likely desirable to accept other forms as well for convenience.
Comment on lines +290 to +298
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolved questions are things that are intended to be required to be answered before stabilization. In this case, the other forms could always be added later, so it may be worth moving this to a future possibility instead.

That is, unless you mean for there to be an open question about whether we'd want the *mut T form at all, but I think @RalfJung sufficiently addressed that in the Zulip thread about how it would be inconsistent with our many other such functions for there to not be a *mut T form of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original draft, I had proposed using &mut MaybeUninit<T>, rather than *mut T. When I swapped that over I left this section, but refactored it. I probably could remove it, but there is still some open discussion about what API should there be on day one.

I'll likely remove this in the near future, as I am somewhat convinced that if there were to be only one interface, it should be on *mut T.


# Future possibilities
[future-possibilities]: #future-possibilities

## Should `discriminant_of!` support nested usage?

Discussed above, we said this was not allowed:

```rust
discriminant_of!(Option<Option<u32>>, Some); // Allowed
discriminant_of!(Option<Option<u32>>, Some.0.Some); // Not Allowed
```

Should we allow this in the future?

## Should we support construction of unaligned enums?

If the user is writing to a packed format, they could potentially want the ability to set the discriminant in cases where the enum discriminant or fields are not well-aligned.

This could require the creation of a `set_discriminant_unaligned` function, that relaxes the well-aligned safety requirements of the proposed `set_discriminant`.
jamesmunns marked this conversation as resolved.
Show resolved Hide resolved

There is also currently no way to read the discriminant of an unaligned enum, so it may also be necessary to add unaligned versions of the `discriminant()` function as well.