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 1 commit
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
?