Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #3633base: master
Are you sure you want to change the base?
[RFC]
core::marker::Freeze
in bounds #3633Changes from 11 commits
af62890
106bc46
902b79d
126f8f0
94ef594
34b9775
3a104b1
6e15d06
c5b3fe8
2b4f996
33ffcc6
b339b0d
30a03fc
c8fa805
492a594
c01e96c
405b322
14abf77
c1fedd5
04e39d4
c7eab79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 onT: Freeze
? I suppose it's taking a&
reference toT
. 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 aT
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
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
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 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
. becauseStuck
is a trait and applies to types, not program execution, it seems different enough to me that it won't be that confusing.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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's far from it, though.
Freeze
isnt just aboutrodata
, 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".
LLVM also uses poison/undef, neither of which we use in Rust (we call it "uninitialized memory" or just "uninit"). LLVM calls it
getelementptr
orptraddr
, we call itoffset
. 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.There was a problem hiding this comment.
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 thatT
cannot be mutated behind a shared reference.There was a problem hiding this comment.
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 implementFreeze
, but can be mutated behind a shared reference. therefore I think usingImmutable
would be highly misleading.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 tractionThough IMO it's fine to actually use a multi-word term like
ShallowImmutable
orCellFree
? We already have got the the two-wordUnwindSafe
and three-wordRefUnwindSafe
auto traits in std. As long as this trait bound is not needed very often (unlikeSend
andSync
andSized
) using a longer but more precise name shouldn't be a problem.There was a problem hiding this comment.
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 theShallow
term.Do we have any opposition to
ShallowImmutable
?There was a problem hiding this comment.
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.)There was a problem hiding this comment.
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 isFreeze
even ifT
isn't. IIUC this would requireFreeze
to be a runtime property rather than a type property (ie, interior mutation never happens), and would require being able to writeunsafe 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
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 toUnsafeCell<T>
not having niches thatT
might have.This is only relevant if such a type also wants to support the functionality enabled by implementing
Freeze
, of course.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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
&[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 ofT
and all of its bytes are initialized, but might not be a bit-validT
." To do this, we experimented with aMaybeValid<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 usingFreeze
as a bound to prove that&[u8] -> &MaybeValid<T>
is sound, it won't work ifT: !Freeze
.As I said, we didn't end up going that route - instead of constructing a
&MaybeValid<T>
, we construct (roughly) aNonNull<T>
(okay, actually aPtr
) and just do the operations manually. The code in question starts at this API if you're curious.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we want to be able to use
MaybeValid<T>
with APIs that are safe and use trait bounds (likeFreeze
) 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 usingunsafe
(namely, useunsafe
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 ofunsafe
blocks) in each API. Decomposition using abstractions likePtr
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 atImmutable
, which is ourFreeze
polyfillThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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
despiteT: ?Freeze
. I'm arguing that, in order to support this use case, we should design the RFC to be forwards-compatible with permitting unsafeimpl<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
FromBytes
(which needs no validity check) and byTryFromBytes
(which performs a validity check after casting)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 ontry_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 requiresSelf: Freeze
, which means thatMaybeValid<Self>: Freeze
. However,try_mut_from
does not requireSelf: Freeze
. Today, the linked code fails to compile thanks to line 114 intry_mut_from
: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 writeunsafe impl<T> Freeze for MaybeValid<T>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
MaybeValid
was alwaysFreeze
, you'd have to extend the safety comments onderef_unchecked
to say "also you must never ever mutate via this shared reference, even ifT
is e.g.Cell<T>
". Anunsafe 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 whatFreeze
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 havingunsafe impl Freeze
, it's just a safety invariant thing. Now you can drop theFreeze
side-condition ontry_cast_into
, since no interior mutability is exposed by exposing aMabeValid
. And then your problem goes away, no?This requires
try_cast_into
to rely on a property ofMaybeValid
, but sinceMaybeValid
appears intry_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 onMaybeValid
.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 ofMaybeValid
even if we allowed you tounsafe impl Freeze
, so you'd not gain anything from that as far as I can see.There was a problem hiding this comment.
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 acceptedT: 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.