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] core::marker::Freeze in bounds #3633

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
af62890
Stabilization RFC for `core::marker::Freeze` in bounds
p-avital May 10, 2024
106bc46
Make this a proper RFC
p-avital May 13, 2024
902b79d
Add line wraps for legibility.
p-avital May 13, 2024
126f8f0
Update text/0000-stabilize-marker-freeze.md
p-avital May 13, 2024
94ef594
Update 0000-stabilize-marker-freeze.md
p-avital May 14, 2024
34b9775
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
3a104b1
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
6e15d06
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
c5b3fe8
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
2b4f996
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
33ffcc6
Update text/0000-stabilize-marker-freeze.md
p-avital May 14, 2024
b339b0d
Address a batch of comments with actionnable suggestions
p-avital May 22, 2024
30a03fc
Update remaining questions
p-avital May 22, 2024
c8fa805
Propose Freeze->ShallowImmutable and PhantomNotFreeze
p-avital May 22, 2024
492a594
Update text/0000-stabilize-marker-freeze.md
p-avital May 22, 2024
c01e96c
Update 0000-stabilize-marker-freeze.md
p-avital May 22, 2024
405b322
Add motivation for the trait renaming
p-avital May 25, 2024
14abf77
Update text/0000-stabilize-marker-freeze.md
p-avital May 26, 2024
c1fedd5
Update text/0000-stabilize-marker-freeze.md
p-avital May 31, 2024
04e39d4
Update RFC following the 2024-07-24 design meeting
p-avital Jul 27, 2024
c7eab79
Apply suggestions from code review
p-avital Jul 28, 2024
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
196 changes: 196 additions & 0 deletions text/0000-stabilize-marker-freeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
- Feature Name: `stabilize_marker_freeze`
- Start Date: 2024-05-10
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

- Stabilize `core::marker::Freeze` in trait bounds.
- Rename `core::marker::Freeze` to `core::marker::ShallowImmutable`. This proposition is tentative, the RFC will keep on using the historical `core::marker::Freeze` name.
- Provide a marker type to opt out of `core::marker::Freeze` for the most semver-conscious maintainers. Tentatively named `core::marker::PhantomNotFreeze` (or `core::marker::PhantomNotShallowImmutable` to go with the proposed rename)

# Motivation
[motivation]: #motivation

With 1.78, Rust [changed behavior](https://github.com/rust-lang/rust/issues/121250): previously, `const REF: &T = &expr;` was (accidentally) accepted even when `expr` may contain interior mutability.
Now this requires that the type of `expr` satisfies `T: core::marker::Freeze`, which indicates that `T` doesn't contain any un-indirected `UnsafeCell`, meaning that `T`'s memory cannot be modified through a shared reference.

The purpose of this change was to ensure that interior mutability cannot affect content that may have been static-promoted in read-only memory, which would be a soundness issue.
However, this new requirement also prevents using static-promotion to create constant references to data of generic type. This pattern can be used to approximate "generic `static`s" (with the distinction that static-promotion doesn't guarantee a unique address for the promoted content). An example of this pattern can be found in `stabby` and `equator`'s shared way of constructing v-tables:
However, this new requirement also prevents using static-promotion to allow generics to provide a generic equivalent to `static` (with the distinction that static-promotion doesn't guarantee a unique address for the promoted content). An example of this pattern can be found in `stabby` and `equator`'s shared way of constructing v-tables:
p-avital marked this conversation as resolved.
Show resolved Hide resolved
p-avital marked this conversation as resolved.
Show resolved Hide resolved
```rust
pub trait VTable<'a>: Copy {
const VT: &'a Self;
}
pub struct VtAccumulator<Tail, Head> {
tail: Tail,
head: Head,
}
impl<Tail: VTable<'a>, Head: VTable<'a>> VTable<'a> for VtAccumulator<Tail, Head> {
const VT: &'a Self = &Self {tail: *Tail::VT, head: *Head::VT}; // Doesn't compile since 1.78
}
```

Making `VTable` a subtrait of `core::marker::Freeze` in this example is sufficient to allow this example to compile again, as static-promotion becomes legal again. This is however impossible as of today due to `core::marker::Freeze` being restricted to `nightly`.

Orthogonally to static-promotion, `core::marker::Freeze` can also be used to ensure that transmuting `&T` to a reference to an interior-immutable type (such as `[u8; core::mem::size_of::<T>()]`) is sound (as interior-mutation of a `&T` may lead to UB in code using the transmuted reference, as it expects that reference's pointee to never change). This is notably a safety requirement for `zerocopy` and `bytemuck` which are currently evaluating the use of `core::marker::Freeze` to ensure that requirement; or rolling out their own equivalents (such as zerocopy's `Immutable`) which imposes great maintenance pressure on these crates to ensure they support as many types as possible. They could stand to benefit from `core::marker::Freeze`'s status as an auto-trait, and `zerocopy` intends to replace its bespoke trait with a re-export of `core::marker::Freeze`.

Note that for this latter use-case, `core::marker::Freeze` isn't entirely sufficient, as an additional proof that `T` doesn't contain padding bytes is necessary to allow this transmutation to be safe, as reading one of `T`'s padding bytes as a `u8` would be UB.

Renaming the trait to `core::marker::ShallowImmutable` is desirable because `freeze` is already a term used in `llvm` to refer to an intrinsic which allows to safely read from uninitialized memory. [Another RFC](https://github.com/rust-lang/rfcs/pull/3605) is currently open to expose this intrinsic in Rust.

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

`core::marker::Freeze` is a trait that is implemented for any type whose memory layout doesn't contain any `UnsafeCell`: it indicates that the memory referenced by `&T` is guaranteed not to change while the reference is live.

It is automatically implemented by the compiler for any type that doesn't contain an un-indirected `core::cell::UnsafeCell`.

Notably, a `const` can only store a reference to a value of type `T` if `T: core::marker::Freeze`, in a pattern named "static-promotion".

As `core::marker::Freeze` is an auto-trait, it poses an inherent semver-hazard (which is already exposed through static-promotion): this RFC proposes the simultaneous addition and stabilization of a `core::marker::PhantomNotFreeze` type to provide a stable mean for maintainers to reliably opt out of `Freeze` without forbidding zero-sized types that are currently `!Freeze` due to the conservativeness of `Freeze`'s implementation being locked into remaining `!Freeze`.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation
p-avital marked this conversation as resolved.
Show resolved Hide resolved

## `core::marker::Freeze`

The following documentation is lifted from the current nightly documentation.
```markdown
Used to determine whether a type contains
any `UnsafeCell` internally, but not through an indirection.
This affects, for example, whether a `static` of that type is
placed in read-only static memory or writable static memory.
This can be used to declare that a constant with a generic type
will not contain interior mutability, and subsequently allow
placing the constant behind references.
# Safety
This trait is a core part of the language, it is just expressed as a trait in libcore for
convenience. Do *not* implement it for other types.
```

From a cursory review, the following documentation improvements may be considered:

```markdown
[`Freeze`] marks all types that do not contain any un-indirected interior mutability.
This means that their byte representation cannot change as long as a reference to them exists.

Note that `T: Freeze` is a shallow property: `T` is still allowed to contain interior mutability,
provided that it is behind an indirection (such as `Box<UnsafeCell<U>>`).
Notable `!Freeze` types are [`UnsafeCell`](core::cell::UnsafeCell) and its safe wrappers
such as the types in the [`cell` module](core::cell), [`Mutex`](std::sync::Mutex), and [atomics](core::sync::atomic).
Any type which contains any one of these without indirection is also `!Freeze`.

`T: Freeze` is notably a requirement for static promotion (`const REF: &'a T;`) to be legal.

Note that static promotion doesn't guarantee a single address: if `REF` is assigned to multiple variables,
they may still refer to distinct addresses.

Whether or not `T: Freeze` may also affect whether `static STATIC: T` is placed
in read-only static memory or writeable static memory, or the optimizations that may be performed
in code that holds an immutable reference to `T`.

# Semver hazard
`Freeze` being an auto-trait, it may leak private properties of your types to semver.
Specifically, adding an `UnsafeCell` to a type's layout is a _major_ breaking change,
as it removes a trait implementation from it.

## The ZST caveat
While `UnsafeCell<T>` is currently `!Freeze` regardless of `T`, allowing `UnsafeCell<T>: Freeze` iff `T` is
a Zero-Sized-Type is currently under consideration.

Therefore, the advised way to make your types `!Freeze` regardless of their actual contents is to add a
[`PhantomNotFreeze`](core::marker::PhantomNotFreeze) field to it.

# Safety
This trait is a core part of the language, it is just expressed as a trait in libcore for
convenience. Do *not* implement it for other types.
```

Mention could be added to `UnsafeCell` and atomics that adding one to a previously `Freeze` type without an indirection (such as a `Box`) is a SemVer hazard, as it will revoke its implementation of `Freeze`.

## `core::marker::PhantomNotFreeze`

This ZST is proposed as a means for maintainers to reliably opt out of `Freeze` without constraining currently `!Freeze` ZSTs to remain so. While the RFC author doesn't have the expertise to produce its code,
here's its propsed documentation:

```markdown
[`PhantomNotFreeze`] is type with the following guarantees:
- It is guaranteed not to affect the layout of a type containing it as a field.
- Any type including it in its fields (including nested fields) without indirection is guaranteed to be `!Freeze`.

This latter property is [`PhantomNotFreeze`]'s raison-d'être: while other Zero-Sized-Types may currently be `!Freeze`,
[`PhantomNotFreeze`] is the only ZST that's guaranteed to keep that bound.

Notable types that are currently `!Freeze` but might not remain so in the future are:
- `UnsafeCell<T>` where `core::mem::size_of::<T>() == 0`
- `[T; 0]` where `T: !Freeze`.

Note that `core::marker::PhantomData<T>` is `Freeze` regardless of `T`'s `Freeze`ness.
```

# Drawbacks
[drawbacks]: #drawbacks

- Some people have previously argued that this would be akin to exposing compiler internals.
- The RFC author disagrees, viewing `Freeze` in a similar light as `Send` and `Sync`: a trait that allows soundness requirements to be proven at compile time.
- `Freeze` being an auto-trait, it is, like `Send` and `Sync` a sneaky SemVer hazard.
- Note that this SemVer hazard already exists through the existence of static-promotion, as exemplified by the following example:
```rust
// old version of the crate.
mod v1 {
pub struct S(i32);
impl S {
pub const fn new() -> Self { S(42) }
}
}

// new version of the crate, adding interior mutability.
mod v2 {
use std::cell::Cell;
pub struct S(Cell<i32>);
impl S {
pub const fn new() -> Self { S(Cell::new(42)) }
}
}

// Old version: builds
const C1: &v1::S = &v1::S::new();
// New version: does not build
const C2: &v2::S = &v2::S::new();
```

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

- The benefits of stabilizing `core::mem::Freeze` have been highlighted in [Motivation](#motivation).
- By not stabilizing `core::mem::Freeze` in trait bounds, we are preventing useful and sound code patterns from existing which were previously supported.
- Alternatively, a non-auto sub-trait of `core::mem::Freeze` may be defined:
- While this reduces the SemVer hazard by making its breakage more obvious, this does lose part of the usefulness that `core::mem::Freeze` would provide to projects such as `zerocopy`.
- A "perfect" derive macro should then be introduced to ease the implementation of this trait. A lint may be introduced in `clippy` to inform users of the existence and applicability of this new trait.

# Prior Art
[prior-art]: #prior-art
- This trait has a long history: it existed in ancient times but got [removed](https://github.com/rust-lang/rust/pull/13076) before Rust 1.0.
In 2017 it got [added back](https://github.com/rust-lang/rust/pull/41349) as a way to simplify the implementation of the `interior_unsafe` query, but it was kept private to the standard library.
In 2019, a [request](https://github.com/rust-lang/rust/issues/60715) was filed to publicly expose the trait, but not a lot happened until recently when the issue around static promotion led to it being [exposed unstably](https://github.com/rust-lang/rust/pull/121840).
- The work necessary for this RFC has already been done and merged in [this PR](https://github.com/rust-lang/rust/issues/121675), and a [tracking issue](https://github.com/rust-lang/rust/issues/121675) was opened.
- zerocopy's [`Immutable`](https://docs.rs/zerocopy/0.8.0-alpha.11/zerocopy/trait.Immutable.html) seeks to provide the same guarantees as `core::marker::Freeze`.

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

- [Should the trait be exposed under a different name?](https://github.com/rust-lang/rust/pull/121501#issuecomment-1962900148)
- An appealing proposition is `ShallowImmutable` to avoid collision with `llvm`'s `freeze`, while highlighting that the property is "shallow".

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

- One might later consider whether `core::mem::Freeze` should be allowed to be `unsafe impl`'d like `Send` and `Sync` are, possibly allowing wrappers around interiorly mutable data to hide this interior mutability from constructs that require `Freeze` if the logic surrounding it guarantees that the interior mutability will never be used.
p-avital marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a plausible possibility. If you never use interior mutability, just don't use a type with UnsafeCell. What is the possible use-case for having an UnsafeCell that you will definitely never use for interior mutability? (That would be the only situation where unsafe impl Freeze would be sound.)

Copy link
Contributor

@joshlf joshlf May 22, 2024

Choose a reason for hiding this comment

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

We've considered wanting this for zerocopy. The use case would be to add a wrapper type Frozen<T> (joking - no clue what we'd name it) that is Freeze even if T isn't. IIUC this would require Freeze to be a runtime property rather than a type property (ie, interior mutation never happens), and would require being able to write unsafe impl<T> Freeze for Frozen<T>.

It's not something we've put a ton of thought into, so I wouldn't necessarily advocate for it being supported right now, but IMO it's at least worth keeping this listed as a potential future direction and trying to keep the possibility open of moving to that future w/ whatever design we choose.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the possible use-case for having an UnsafeCell that you will definitely never use for interior mutability?

Speculation: Perhaps a data structure whose contents are computed using interior mutability, then put in a wrapper struct which disables all mutation operations and implements Freeze. The reason to do this using a wrapper rather than transmuting to a non-interior-mutable type would be to avoid layout mismatches due to UnsafeCell<T> not having niches that T might have.

This is only relevant if such a type also wants to support the functionality enabled by implementing Freeze, of course.

Copy link
Member

Choose a reason for hiding this comment

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

We've considered wanting this for zerocopy. The use case would be to add a wrapper type Frozen (joking - no clue what we'd name it) that is Freeze even if T isn't.

What would that type be good for?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've considered wanting this for zerocopy. The use case would be to add a wrapper type Frozen (joking - no clue what we'd name it) that is Freeze even if T isn't.

What would that type be good for?

It's been a while since we considered this design, so the details are hazy in my memory, but roughly we wanted to be able to validate that a &[u8] contained the bytes of a bit-valid &T. We wanted to encode in the type system that we'd already validated size and alignment, so we wanted a type that represented "this has the size and alignment of T and all of its bytes are initialized, but might not be a bit-valid T." To do this, we experimented with a MaybeValid<T> wrapper.

When you're only considering by-value operations, you can just do #[repr(transparent)] struct MaybeValid<T>(MaybeUninit<T>); (MaybeUninit itself isn't good enough because it doesn't promise that its bytes are initialized - the newtype lets you add internal invariants). The problem is that this doesn't work by reference - if you're using Freeze as a bound to prove that &[u8] -> &MaybeValid<T> is sound, it won't work if T: !Freeze.

As I said, we didn't end up going that route - instead of constructing a &MaybeValid<T>, we construct (roughly) a NonNull<T> (okay, actually a Ptr) and just do the operations manually. The code in question starts at this API if you're curious.

Copy link
Contributor

@joshlf joshlf May 22, 2024

Choose a reason for hiding this comment

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

If you're wrapping T inside a private field, why does it matter whether it has interior mutability or not?

Because we want to be able to use MaybeValid<T> with APIs that are safe and use trait bounds (like Freeze) to prove their soundness internally. One of our golden rules for developing zerocopy is to be extremely anal about internal abstraction boundaries [1]. While we could just do what you're suggesting using unsafe (namely, use unsafe to directly do the &[u8] -> &MaybeValid<T> cast), we have existing internal APIs such as this one [2] that permit this conversion safely (ie, the method itself is safe to call), using trait bounds to enforce soundness.

In general, we've found that, as we teach our APIs to handle more and more cases, it quickly becomes very unwieldy to program "directly" using one big unsafe block (or a sequence of unsafe blocks) in each API. Decomposition using abstractions like Ptr has been crucial to us being confident that the code we're writing is actually sound.

[1] See "Abstraction boundaries and unsafe code" in our contributing guidelines

[2] This API is a bit convoluted to follow, but basically the AliasingSafe bound bottoms out at Immutable, which is our Freeze polyfill

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid it is not clear to me how Frozen helps build up internal abstraction barriers, and your existing codebase is way too big for me to be able to extract that kind of information from it with reasonable effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR: We can only make this code compile if MaybeValid<T>: Freeze despite T: ?Freeze. I'm arguing that, in order to support this use case, we should design the RFC to be forwards-compatible with permitting unsafe impl<T> Freeze for MaybeValid<T> even if we don't support it out of the gate.

Here's a minification of the sort of code I'm describing. It has three components that, in reality, would live in unrelated parts of our codebase (they wouldn't be right next to each other like they are here, so we'd want to be able to reason about their behavior orthogonally):

  • MaybeValid<T>
  • try_cast_into<T>(bytes: &[u8]) -> Option<&MaybeValid<T>> where MaybeValid<T>: Freeze
    • This is responsible for checking size and alignment, but not validity; in our codebase, it is used both by FromBytes (which needs no validity check) and by TryFromBytes (which performs a validity check after casting)
    • In our codebase (but not in this example), this does some complicated math to compute pointer metadata for slice DST; it can't easily be inlined into its caller
  • TryFromBytes, with:
    • try_ref_from(bytes: &[u8]) -> Option<&Self> where Self: Freeze
    • try_mut_from(bytes: &mut [u8]) -> Option<&mut Self>

Notice the MaybeValid<T>: Freeze bound on try_cast_into (and the safety comments inside that function). That bound permits us to make the function safe to call (without that bound, we'd need a safety precondition about whether interior mutation ever happens using the argument/returned references).

try_ref_from can satisfy that bound because it already requires Self: Freeze, which means that MaybeValid<Self>: Freeze. However, try_mut_from does not require Self: Freeze. Today, the linked code fails to compile thanks to line 114 in try_mut_from:

let maybe_self = try_cast_into::<Self>(bytes)?;

What we'd like to do is be able to guarantee that interior mutation will never be exercised via &MaybeValid<T>, and thus be able to write unsafe impl<T> Freeze for MaybeValid<T>.

Copy link
Member

@RalfJung RalfJung May 24, 2024

Choose a reason for hiding this comment

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

Notice the MaybeValid: Freeze bound on try_cast_into (and the safety comments inside that function). That bound permits us to make the function safe to call (without that bound, we'd need a safety precondition about whether interior mutation ever happens using the argument/returned references).

What goes wrong if it's not Freeze? There can't even be a proof obligation attached to this since you want it to be trivially true. I think all you'd lose is some optimization potential.

If MaybeValid was always Freeze, you'd have to extend the safety comments on deref_unchecked to say "also you must never ever mutate via this shared reference, even if T is e.g. Cell<T>". An unsafe impl Freeze for MyType type has the responsibility of ensuring that its interior is never mutated through any pointer derived from an &MyType. That's what Freeze means: no mutation through shared references to this type, including all pointers ever derived from any such shared reference.

So henceforth I will assume that MaybeValid has added this requirement to its public API. You can do that without having unsafe impl Freeze, it's just a safety invariant thing. Now you can drop the Freeze side-condition on try_cast_into, since no interior mutability is exposed by exposing a MabeValid. And then your problem goes away, no?

This requires try_cast_into to rely on a property of MaybeValid, but since MaybeValid appears in try_cast_into's signature, there's no new coupling here -- you already depend on the fact that MaybeValid can be soundly constructed for invalid data.

So I still don't see a motivation for impl Freeze here, except if you somehow want more optimizations on MaybeValid.

What we'd like to do is be able to guarantee that interior mutation will never be exercised via &MaybeValid

tl;dr you can already do that, just put these requirements into the public API of MaybeValid. And you'd have to put these requirements in the public API of MaybeValid even if we allowed you to unsafe impl Freeze, so you'd not gain anything from that as far as I can see.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point, and I think that's a compelling argument. I can imagine there being cases where you'd want to pass MaybeValid to a generic API that accepted T: Freeze, in which case you'd still need this, but I don't have a concrete example of this off the top of my head.

I'd still advocate for keeping this listed as a future possibility since doing so doesn't have any effect on the current proposal (ie, the current proposal is already forwards-compatible with this). Obviously we'd need to adjudicate it much more thoroughly if it was ever proposed to actually go through with permitting unsafe impl Freeze, and I assume you'd push back if that ever happened just as you are now, but I think it's at least worth mentioning that it's something that we've considered and that there might be some desire for.

- The current status-quo is that it cannot be implemented manually (experimentally verified with 2024-05-12's nightly).
- The RFC author is unable to tell whether allowing manual implementation may cause the compiler to generate unsound code (even if the wrapper correctly prevents interior mutation), but judges that the gains of allowing these implementations are too small to justify allowing the risk.
p-avital marked this conversation as resolved.
Show resolved Hide resolved
- This consideration is purposedly left out of scope for this RFC to allow the stabilization of its core interest to go more smoothly; these two debates being completely orthogonal.
- Adding a `trait Pure: Freeze` which extends the interior immutability guarantee to indirected data could be valuable:
- This is however likely to be a fool's errand, as indirections could (for example) be hidden behind keys to global collections.
- Providing such a trait could be left to the ecosystem unless we'd want it to be an auto-trait also (unlikely).