-
Notifications
You must be signed in to change notification settings - Fork 1.6k
core::marker::NoCell
in bounds (previously known an Freeze
)
#3633
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
af62890
106bc46
902b79d
126f8f0
94ef594
34b9775
3a104b1
6e15d06
c5b3fe8
2b4f996
33ffcc6
b339b0d
30a03fc
c8fa805
492a594
c01e96c
405b322
14abf77
c1fedd5
04e39d4
c7eab79
84c1aad
65f1c90
b5c089b
9ac85ff
7d91686
ebffacf
ad10530
4225d5c
a274335
4172280
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
- 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. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
With 1.78, Rust [stabilized](https://github.com/rust-lang/rust/issues/121250) the requirement that `T: core::marker::Freeze` for `const REF: &'a T = T::const_new();` (a pattern referred to as "static-promotion" regardless of whether `'a = 'static`) to be legal. `T: core::marker::Freeze` 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 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
|
||
```rust | ||
pub trait VTable<'a>: Copy { | ||
const VT: &'a Self; | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
pub struct VtAccumulator<Tail, Head> { | ||
tail: Tail, | ||
head: Head, | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
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, but this isn't a requirement for them, and explicit derivation is considered suitable (at least to zerocopy). | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
`core::marker::Freeze` is a trait that indicates the shallow lack of interior mutability in a type: it indicates that the memory referenced by `&T` is guaranteed not to change under defined behaviour. | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
It is automatically implemented by the compiler for any type that doesn't shallowly contain a `core::cell::UnsafeCell`. | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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". | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
``` | ||
|
||
The current _Safety_ section may be removed, as manual implementation of this trait is forbidden. | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
From a cursary review, the following documentation improvements may be considered: | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```markdown | ||
[`Freeze`](core::marker::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 interior mutability sources are [`UnsafeCell`](core::cell::UnsafeCell) (and any of its safe wrappers | ||
such the types in the [`cell` module](core::cell) or [`Mutex`](std::sync::Mutex)) and [atomics](core::sync::atomic). | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
`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`. | ||
``` | ||
|
||
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 revoque its implementation of `Freeze`. | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# 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 examplified by the following example: | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
```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(); | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
# 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 | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[prior-art]: #prior-art | ||
|
||
- This feature has been available in `nightly` for 7 years, and is used internally by the compiler. | ||
p-avital marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something that seems troublesome about Unfortunately, I don't have any good ideas for alternatives, especially not non-negated ones as @RalfJung suggested, but I think that stabilizing this under the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least on the theory side, a program being "stuck" has meaning already -- it means that execution can't progress further as the program has left the realm of well-defined executions. I would find this term very confusing. "Frozen" is better than "Freeze" as it's not an action.
Maybe we shouldn't call that "Freeze"? It's not a great name as what the operation really does is turn poison into non-deterministic regular bytes. Rust doesn't have undef-style "bytes that are unstable". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was thinking more of that the value in memory is stuck at whatever bytes are put into
since the LLVM IR op is already called freeze, they've effectively already laid claim to that name, so anyone who's used LLVM IR and tries to figure out rust will be confused if we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's far from it, though. I find "stuck" extremely unintuitive. It conveys negative feelings to me, "we're stuck somewhere or with something (and want to get unstuck again)". I don't think it is a good term for "this memory is immutable".
LLVM also uses poison/undef, neither of which we use in Rust (we call it "uninitialized memory" or just "uninit"). LLVM calls it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arguably any name that doesn't clarify that the property only applies shallowly/not-via-indirection is similarly misleading. If we're worried about that, we'd need to add an adjective like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just invent another term that no one else used. Perhaps But given that the trait has existed for so long under the name Though IMO it's fine to actually use a multi-word term like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me set a record for the most negative name: I like Do we have any opposition to |
||
|
||
# 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 This is only relevant if such a type also wants to support the functionality enabled by implementing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What would that type be good for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 When you're only considering by-value operations, you can just do As I said, we didn't end up going that route - instead of constructing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because we want to be able to use 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 [1] See "Abstraction boundaries and unsafe code" in our contributing guidelines [2] This API is a bit convoluted to follow, but basically the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid it is not clear to me how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TLDR: We can only make this code compile if 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):
Notice the
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 So henceforth I will assume that This requires So I still don't see a motivation for
tl;dr you can already do that, just put these requirements into the public API of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
- 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). |
Uh oh!
There was an error while loading. Please reload this page.