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 5 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
149 changes: 149 additions & 0 deletions text/0000-stabilize-marker-freeze.md
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.
p-avital marked this conversation as resolved.
Show resolved Hide resolved

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)
Copy link

Choose a reason for hiding this comment

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

Something that seems troublesome about Freeze is that it's a trait named using a verb, which is good in isolation, but that verb would usually be an operation you can perform on values of the implementing type. What's the “freeze” operation on T: Freeze? I suppose it's taking a & reference to T. That's reasonable enough, but there is another “freeze” operation that's both already named in LLVM and recently been proposed for addition to Rust: “freezing” uninitialized memory to enable safely reading the bytes of a value that contains padding, or reading a value that may be partially initialized. That's a different thing in the same problem domain (what you can or can't do with the bytes when you have a T or &T) which seems dangerously confusing. And outside of that name collision, “Freeze” doesn't convey much to the naïve reader.

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 Freeze would create a frequent source of confusion among those encountering it for the first time. It might be better to have an awkward complex name than a cryptic simple one. Awkward suggestion: ShallowImmut?

Copy link
Member

Choose a reason for hiding this comment

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

what about Stuck? that is unused afaict so people seeing it would think to look up what it means in Rust. It's also nice and short and reminds users of what it means.

Copy link
Member

@RalfJung RalfJung May 17, 2024

Choose a reason for hiding this comment

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

Unfortunately, I don't have any good ideas for alternatives, especially not rust-lang/rust#121501 (comment), but I think that stabilizing this under the name Freeze would create a frequent source of confusion among those encountering it for the first time.

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

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more of that the value in memory is stuck at whatever bytes are put into .rodata. because Stuck is a trait and applies to types, not program execution, it seems different enough to me that it won't be that confusing.

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

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 freeze to mean something completely different where it could be confused with an operation (since freeze is present-tense, unlike stuck where being past-tense and being applied to types makes it much less likely to be an operation).

Copy link
Member

Choose a reason for hiding this comment

The 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 .rodata

That's far from it, though. Freeze isnt just about rodata, it's about all shared references without interior mutability.

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

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 freeze to mean something completely different where it could be confused with an operation (since freeze is present-tense, unlike stuck where being past-tense and being applied to types makes it much less likely to be an operation).

LLVM also uses poison/undef, neither of which we use in Rust (we call it "uninitialized memory" or just "uninit"). LLVM calls it getelementptr or ptraddr, we call it offset. LLVM calls it load/store, we call it read/write. The list can probably go on for a while. I don't think we should let LLVM dictate our terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

In zerocopy, we went with Immutable, which I quite like (credit to @jswrenn's coworker for suggesting the name). It is an adjective and accurately describes the semantics - T: Immutable means that T cannot be mutated behind a shared reference.

Copy link
Member

Choose a reason for hiding this comment

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

but T can be mutated behind a shared reference if that mutation is hidden in some other block of memory: Box<Cell<u8>> does implement Freeze, but can be mutated behind a shared reference. therefore I think using Immutable would be highly misleading.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 Shallow to the name.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just invent another term that no one else used. Freeze itself is an invented term anyway.

Perhaps Solid (you "freeze" liquid to get "solid" 🤷).
Or Acellular (technically a negated word).

But given that the trait has existed for so long under the name Freeze perhaps it has already gotten traction

Though IMO it's fine to actually use a multi-word term like ShallowImmutable or CellFree? We already have got the the two-word UnwindSafe and three-word RefUnwindSafe auto traits in std. As long as this trait bound is not needed very often (unlike Send and Sync and Sized) using a longer but more precise name shouldn't be a problem.

Copy link
Author

Choose a reason for hiding this comment

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

Let me set a record for the most negative name: UnindirectedUnUnsafeCelled 😄

I like ShallowImmutable a lot, as it's rather descriptive and attracts curiosity thanks to the Shallow term.

Do we have any opposition to ShallowImmutable?


# 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

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