Request: Make std::marker::Freeze pub again

[mtak-]

I had heard tell of Freeze but didn't really know what it was until today. swym, a hybrid transactional memory library, has an accidental reimplementation of Freeze using optin_builtin_traits. Unfortunately optin_builtin_traits is the only feature keeping swym on nightly.

The ticket that removed Freeze doesn't have much of an explanation for why it was removed so I'm assuming it was a lack of motivating use cases.

Use Case

swym::tcell::TCell::borrow returns snapshots of data - shallow memcpys - that are guaranteed to not be torn, and be valid for the duration of the transaction. Those snapshots hold on to the lifetime of the TCell in order to act like a true reference, without blocking updates to the TCell from other threads. Other threads promise to not mutate the value that had its snapshot taken until the transaction has finished, but are permitted to move the value in memory.

This works great for a lot of types, but fails miserably when UnsafeCells are directly stored in the type.

let x = TCell::new(Mutex::new("hello there".to_owned()));

// ..  inside a transaction
let shallow_copy = x.borrow(tx, Default::default())?;
// locking a shallow copy of a lock... is not really a lock at all!
*shallow_copy.lock().unwrap() = "uh oh".to_owned();

Even if Mutex internally had a pointer to the "actual" mutex data structure, the above example would still be broken because the String is deallocated through the shallow copy. The String contained in the TCell would point to freed memory.

Note that having TCell::borrow require Sync would still allow the above broken example to compile.

Freeze

If swym::tcell::TCell::borrow could require Freeze then this would not be an issue as the Mutex type is definitely not Freeze.

pub(crate) unsafe auto trait Freeze {}

impl<T: ?Sized> !Freeze for UnsafeCell<T> {}
unsafe impl<T: ?Sized> Freeze for PhantomData<T> {}
unsafe impl<T: ?Sized> Freeze for *const T {}
unsafe impl<T: ?Sized> Freeze for *mut T {}
unsafe impl<T: ?Sized> Freeze for &T {}
unsafe impl<T: ?Sized> Freeze for &mut T {}

Shallow immutability is all that is required for TCell::borrow to work. Sync is only necessary to make TCell Sync.

• TCell<String> - should be permitted.
• TCell<Mutex<String>> - should be forbidden.
• TCell<Box<Mutex<String>>> - should be permitted.

Alternatives

• A manually implemented marker trait could work, but is actually very dangerous in practice. In the below example, assume that the impl of MyFreeze was correct when it was written. Everytime the author of MyType updates their dependency on other_crate they must recheck that OtherType still has no direct interior mutability or risk unsoundness.

struct MyType { value: other_crate::OtherType }
unsafe impl MyFreeze for MyType {}

• Add a T: Copy bound on TCell::<T>::borrow. This would definitely work but leaves a lot of types out.

• Wait for OIBITs to stabilize (assuming it will be stabilized).

• Have TCell store a Box<T> internally, and only work with heap allocated data where interior mutability is of no concern. This would be pretty effective, and if the type is small enough and Copy, the Box could be elided. While not as good as stabilizing Freeze, I think this is the best alternative.

[Centril]

cc @rust-lang/lang and possibly @rust-lang/libs

I'm sorta interested in this direction. However, Freeze is also a lang item used by other parts of the language so we need to tread carefully here and possibly consider exposing a version of Freeze that isn't a lang item.

I would also be interested in seeing more use cases to justify the exposure of Freeze.

[cramertj]

Note that this would make it an api-breaking change to add interior mutability to a type.

[dtolnay]

Adding interior mutability into a type is already an API breaking change.

Even adding trait objects into a type is an API breaking change.

struct S {
    i: usize,
    //j: std::cell::Cell<usize>,
    //k: Box<dyn std::error::Error>,
}

fn f(s: &S) {
    let _ = std::panic::catch_unwind(|| {
        let _s = s;
    });
}

fn main() {}

[mtak-]

I would also be interested in seeing more use cases to justify the exposure of Freeze.

This is essentially the same use case, but crossbeam-epoch could add a non-heap Atomic style type to the API. e.g. A Seq<T> type which internally protects the contained value with a seqlock. On write, Seq defers the drop of the overwritten value. On read, Seq returns a shallow copy which borrows the lifetime of both the Guard and the Seq. Seq would not have to require Copy, but could instead only require Freeze.

Note that this would make it an api-breaking change to add interior mutability to a type.

Fair point! This is already the case for Cell/RefCell which are !Sync. Freeze would add Mutex/RwLock/Atomic*/etc as API breakers.

[Centril]

Adding interior mutability into a type is already an API breaking change.

@dtolnay We might want to add a PhantomUnfrozen to give users the ability to not guarantee Freeze then? (Inspired by PhantomPinned)

[cramertj]

@Centril That's equivalent to PhantomData<Cell<()>>.

[glaebhoerl]

Cc #25053 -- using Copy as a workaround here would obviously break if UnsafeCell were made Copy. (Which might provide additional motivation for exposing Freeze, in order to be able to express the intended requirement directly rather than by fortuitous coincidence.)

[RalfJung]

Add a T: Copy bound on TCell::::borrow. This would definitely work but leaves a lot of types out.

We'd very much appreciate if you wouldn't do that. This would paint us into a corner that is impossible to get out of, where the legitimate usecases for a Copy type with interior mutability would be forever excluded from Rust.

[mtak-]

@glaebhoerl Ahh, I didn't realize there was a discussion around that. It's not clear to me that UnsafeCell: Copy would cause unsafety in swym, but it would definitely break the reference semantics of borrow's return type.

That's equivalent to PhantomData<Cell<()>>.

For that to work, the proposed version of Freeze should not have the PhantomData impl core currently has. core would certainly wanna keep their definition of Freeze as it's used for deciding whether to place statics in read-only memory or writable memory and possibly for other optimizations.

[eddyb]

Note that Cell<()> is zero-sized so you can just use it without PhantomData around it.

[danielhenrymantilla]

To opt out of Freeze, using Mutex<()> / PhantomData<Mutex<()>> instead avoids opting out of Sync "by accident".

---

aside

I think that having an explicitely named PhantomXXX (e.g., PhantomUnfrozen, but also PhantomUnsynced, PhantomThreadBound), even when it is just a type alias or a new type wrapper around PhantomData, helps improve the readability of the code: currently code with occurrences of PhantomData very often have a comment next to it to explain what the purpose of the PhantomData is. See this thread regarding the usage of PhantomData for (in)variance

[mtak-]

A separate PhantomUnfrozen type is required. AFAIK there is no Sync ZST with interior mutability.

PhantomData<Mutex<()>>/PhantomData<AtomicUsize> would have the correct opt out behavior, but requires removing the blanket PhantomData Freeze impl. This is a bad idea due to widespread usage of PhantomData for owned pointers. Types like Vec<Cell<u8>> would not be Freeze - even though they should be.

[eddyb]

You can make your own on stable Rust with a wrapper around UnsafeCell<()> that you then unsafe impl Sync on.

Note that we reserve the right to consider everything outside a literal UnsafeCell to be frozen so this shouldn't be used for unsafe tricks, only "unpromising" Freeze on a type.

[pythonesque]

Okay, seeing this issue has convinced me that we really need to push for either opt-in Copy, or Copy for UnsafeCell, or the CopyOwn thing. Because we cannot just wait and expect people not tor rely on people abusing the number of things Copy excludes for other purposes than declaring things to be (implicitly) byte-copyable. People already abuse it for making sure a type has a no-op Drop impl, which is its own headache that is further exacerbated by the fact that UnsafeCell can't implement Copy.

[petertodd]

I would also be interested in seeing more use cases to justify the exposure of Freeze.

Mem-mapping is a use-case I have for Freeze, specifically to prevent accidentally implementing some traits on types with interior mutability. Though given how much unsafe code is involved in that use-case anyway it'd be just a "belt-and-suspenders" measure.

It's also a use-case where exposing the actual compiler Freeze type would be annoying: in my usecase a small subset of types have both interior mutability and implement mem-mapping, with care taken to only actually mutate "dirty" values that are heap allocated. So those types would need Freeze impl's, which is probably inappropriate if they actually affect code generation.

[RalfJung]

@petertodd

It's also a use-case where exposing the actual compiler Freeze type would be annoying: in my usecase a small subset of types have both interior mutability and implement mem-mapping, with care taken to only actually mutate "dirty" values that are heap allocated. So those types would need Freeze impl's, which is probably inappropriate if they actually affect code generation.

I don't entirely follow. Note that Freeze only talks about the value itself, not other memory it points to. So, for example, Box<RefCell<T>> is Freeze.

[petertodd]

On January 1, 2020 12:42:22 PM EST, Ralf Jung ***@***.***> wrote: @petertodd > It's also a use-case where exposing the actual compiler Freeze type would be annoying: in my usecase a small subset of types have both interior mutability and implement mem-mapping, with care taken to only actually mutate "dirty" values that are heap allocated. So those types would need Freeze impl's, which is probably inappropriate if they actually affect code generation. I don't entirely follow. Note that `Freeze` only talks about the value itself, not other memory it points to. So, for example, `Box<RefCell<T>>` is `Freeze`.

I'm talking about values in memmapped files. To summarize, I have a trait, ValidateBlob, that validates that a blob of bytes are structurally valid for the type implementing it ([repr(C)], unaligned types, etc). It's designed to work with read-only mappings¹, so interior mutability will simply segfault². Freeze in that context would be a useful lint. But ValidateBlob is of course unsafe, so it'd just be a lint. 1) I'm both willing to live with corruption caused by unintentional modifications to the backing file, *and* in this use case since the backend is meant to be append only, you can use the append-only bit to make that problem much less of an issue. It's basically a database after all. You can also play games with memmapping modes to get similar results, with different trade-offs. 2) Intentional mutation is via copy-on-write, so all intentional mutation will be in heap/stack allocated memory.

[danielhenrymantilla]

Yes it would

[RalfJung]

In #121250 another case came up where a Freeze bound would be very useful.

I am more and more in agreement that we should allow Freeze bounds on stable. We should not allow any Freeze impls though. Do we have any way of marking a trait as accessible on stable only for bounds but not for impls?

[Skgland]

Do we have any way of marking a trait as accessible on stable only for bounds but not for impls?

Don't we already have such a trait with Sized?

[RalfJung]

In the previous RFC for stabilizing Freeze, t-lang said this

However, our most significant concern was around backwards compatibility. This RFC proposes to expose a new auto trait which makes it backwards-incompatible to add a field with interior mutability to an existing type which previously had no interior mutability. This proposal would require library authors who may want to add interior mutability in the future to mark their types with PhantomData<UnsafeCell<()>>. The RFC further addresses this restriction in two places:

However, this is already a breaking change. Various aspects of working with data in const, and static promotion, depend on whether a type is Freeze or not. Not exposing the bound doesn't really avoid that issue. We should figure out if they are willing to re-evaluate their position given what seems like new information.

They also suggested experimenting with this as a library crate, but that doesn't work for issues like #121250.

Don't we already have such a trait with Sized?

The difference is that Sized cannot be implemented anywhere. Freeze should be implementable with a nightly feature but not without.

[petertodd]

Can't we do that by just using the Sealed trait trick to make implementations impossible?

…

On February 22, 2024 11:55:53 AM GMT+01:00, Ralf Jung ***@***.***> wrote: In #121250 another case came up where a `Freeze` bound would be very useful. I am more and more in agreement that we should allow `Freeze` bounds on stable. We should *not* allow any `Freeze` impls though. Do we have any way of marking a trait as accessible on stable only for bounds but not for impls?

[bjorn3]

Why should Freeze be implementable? Implementing it for a type containing an UnsafeCell would cause UB, right? And if it doesn't contain UnsafeCell it will automatically be implemented already.

[RalfJung]

Can't we do that by just using the Sealed trait trick to make implementations impossible?

No, Freeze is an auto trait.

Why should Freeze be implementable? Implementing it for a type containing an UnsafeCell would cause UB, right? And if it doesn't contain UnsafeCell it will automatically be implemented already.

This is the current definition:

#[lang = "freeze"]
pub(crate) unsafe auto trait Freeze {}

impl<T: ?Sized> !Freeze for UnsafeCell<T> {}
marker_impls! {
    unsafe Freeze for
        {T: ?Sized} PhantomData<T>,
        {T: ?Sized} *const T,
        {T: ?Sized} *mut T,
        {T: ?Sized} &T,
        {T: ?Sized} &mut T,
}

If we make it not implementable, we have to completely change how it works.

[bjorn3]

For Sized we have a bunch of builtin impls. We can have the same for Freeze if we were to deny user implementations. Or we could restrict impls to just libcore. For example by renaming Freeze to FreezeInternal and then export a new Freeze trait which requires FreezeInternal and has a blanket impl. This should prevent end users from manually implementing it, right?

[RalfJung]

Various hacks are possible. But TBH just checking a feature flag when there is an impl seems easiest...

[bjorn3]

Could it at least be an internal feature to indicate that it will never be stabilized?

[danielhenrymantilla]

various hacks are possible

Could you justify the term of "hack" regarding @bjorn3's suggestion? It is a bit (too) easy to dismiss an ideä throwing that word around; in this instance we have:

• an internal trait, currently called Freeze, but which I'll call ShallowlyImmutableWhenShared, which checks for the structural presence of UnsafeCell / shared mutability, in a shallow-manner, within a type. It is implemented as an auto-trait, modulo the danger of it being implemented by third-party code. It being kept internal/"private"/sealed thus makes sense.
• the need to expose some trait that allows for static-promotability, which we could call StaticPromotable.

It turns out that being ShallowlyImmutableWhenShared is sufficient to be StaticPromotable:

unsafe
impl<T> StaticPromotable for T
where
    T : ShallowlyImmutableWhenShared,
{}

And conservatively, the stdlib could make it necessary as well, to avoid user-provided impls:

pub unsafe trait StaticPromotable : ShallowlyImmutableWhenShared {}

So, while the two notions are quite entangled given the equivalence property that the super-bound + blanket impl entail, it still looks legitimate to consider there being two interfaces, with one focusing on the raw byte-level properties of a type, and the other, on the usability that such a property offers.

---

For context, consider the error we currently get with:

trait Trait<T: 'static> {
   const VALUE: T;
   const VALUE_REF: &'static T = &Self::VALUE;
}

it complains of const_refs_to_cells or something like that, when the user has written no Cell whatsoever.

• Moreover, say this code is intending to have an immutable reference to the value / is not intending to think of shared mutability shenanigans. So even with const_refs_to_cells, the code would not be featuring the desired API.

If, however, the compiler required StaticPromotable, the error would be saying:

error[E0277]: `T` cannot be promoted to static storage
 --> src/lib.rs:3:42
  |
3 |     const VALUE_REF: &'static T = &{ Self::VALUE };
  |                                    ^^^^^^^^^^^^^^^ `T` cannot be promoted to static storage.
  |
  = help: the trait `StaticPromotable` is not implemented for `T`
  = note: static promotion can only be applied to values whose type implements `StaticPromotable`

Which would be way more similar to other such errors in Rust (e.g., a static global whose type is missing Sync).

It is a name (modulo bikeshed) which expresses the property the user is most likely interested in, rather than hearing of Freeze or Cells.

• Then, within the documentation of StaticPromotable, it would be mentioned that shared (shallow-)mutability is the reason a type loses its otherwise default static-promotability.

---

The fact is, there are cases where users are direly in need of just knowing, talking, and often actually using the StaticPromotable property (as I had already illustrated on Zulip), and preventing that functionality for non-nightly users (or, currently, even nightly users) based on the æsthetics of a "frontend and backend trait split" (otherwise a rather common pattern) seems ill-advised.

[RalfJung]

Could you justify the term of "hack" regarding @bjorn3's suggestion? It is a bit (too) easy to dismiss an ideä throwing that word around; in this instance we have:

I was interpreting it as just a somewhat roundabout way to achieve the goal of "having a trait that we can use as a bound but not implement". There was no further motivation given. The goal of separating "static promotability" from "no interior mutability" was not articulated here before (or if it was then I missed that), so you can't expect me to take it into account.

I can see that if we want to separate these two things, then (your framing of) @bjorn3's proposal makes sense. But why should we want to have that split in the first place?

[RalfJung]

Also note that StaticPromotable is a misnomer in this context. For something to be promotable, it also needs to have no drop glue. (And then further the expression computing it must be "pure", i.e. must not panic or cause UB or loop infinitely. But if we have a const value of a type, then this concern does not apply.)

const VALUE_REF: &'static T = &{ Self::VALUE }; can work even when Self::VALUE is not promotable, consider e.g.

const C: &'static Vec<i32> = &{ Vec::new() };

This is not promotion, this is the "outer scope" rule applied top-level items. (The fact that this rule applies here is causing pain and bugs and confusion for years now, but it's way too late to take this back.)

[joshtriplett]

Nominating for lang discussion based on @RalfJung's comments at #60715 (comment) and #60715 (comment) .

[RalfJung]

@joshtriplett it may make more sense to nominate #121501, which is a concrete proposal. Or at least you should consider that PR as part of your discussion. :)

[joshtriplett]

@RalfJung Fair enough; I can link to this one for context.

[p-avital]

Would the Freeze trait allow the following Code to compile?

trait Trait<T: Freeze + 'static> {
   const VALUE: T;
   const VALUE_REF: &'static T = &Self::VALUE;
}

Without the Freeze bound it currently fails with error[E0492]: constants cannot refer to interior mutable data, because T could of course have interior mutability.

If I understand correctly, Freeze types cannot contain any UnsafeCells, and thus cannot have interior mutability.

This is actually key to stabby's ABI stable trait objects: I was able to trick pre-nighty versions of the compiler into accepting this by not actually having a VALUE const, only the VALUE_REF one. Nightly sees through that trick however, I've been looking for a way to tell Rust I do not intend for anything interiorly mutable to make it in the vtables...