-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: master
Are you sure you want to change the base?
Conversation
IIRC @joshlf was also asking about exposing Freeze; maybe they can help provide some more motivating examples. Currently there's only really one. (I don't understand the "key of a map" note, and the RFC doesn't explain it in more than 5 words either.) |
The main reason for not stabilising I don't think that just stabilising it for bounds affects this concern. The issue isn't the trait itself being exposed in the library, but APIs having to worry about whether they are |
A bound is exactly what one is worried about here? If one writes a function This is exactly the same as Fundamentally there are some things the compiler only lets you do with (Or did I misunderstand what you mean? It sounded a bit like you're saying just stabilizing the bound means we don't have to worry about the concern. But upon re-reading I am less sure.)
Letting people write |
Sure! If you want to dive deeper, look at uses of On zerocopy stable, we provide traits like However, this is overly-restrictive. The "no Another restriction is that some authors want to use Our solution in the upcoming zerocopy 0.8 is to add a separate * Currently, |
I should clarify, what I meant here was the adding of stuff like Not the ability to make something |
For another "motivating example", One possible alternative that would "work around" the semver issue would be to have Footnotes
|
Yeah that could work -- as you say, the existing |
core::marker::Freeze
in boundscore::marker::Freeze
in bounds
Thanks for writing this up. I appreciate the effort that went into it and the desire to push things forward. But... I think this RFC needs a fair bit of work. After reading it, I'm still left with some very fundamental questions (and I expect these to be answered in the RFC):
|
IIUC, we couldn't do that today since If we decide to keep it a type property - ie, the absence of
|
So if we want to expose the |
@joshlf For the purpose of |
IMO the main advantage is that An additional advantage is that there would be only one trait to encode the invariant.
For my sketch of Footnotes
|
The most important reason is that the compiler can see into the internals of types in the standard library. For example, I filed this extremely silly issue because there's technically no guarantee provided to code outside of the standard library that Besides that, here are some other reasons:
|
Thanks to everyone taking an interest in this RFC. First, a quick apology for it starting up so half-assed, I clearly underestimated the task. I started this RFC as a knee-jerk reaction to 1.78 merging the breaking change the RFC mentions without providing a way for const references to generics to exist. I'm still committed to getting this to move forward, but I have limited availability for the next 2 weeks, so please bear with me as I rewrite this entire thing to the standard I should have started it with. I would really appreciate getting some early feedback on the direction people around here would like to see this RFC take regarding:
Sorry for my high latency for the beginnings of this RFC. I really appreciate all the feedback you can give, and would like to mention that PRs to this PR's branch are very welcome :) |
@p-avital happy to hear that you're not discouraged by all the extra work we're throwing your way. :)
Even if Freeze was recursive, types can use raw pointers or global |
@p-avital It's hard for me to give you good feedback here because I think I lack a fair bit of context. Most of the discussion (and the RFC itself) seems to be very high context here, and it's context that I don't have. With that said, I can say that adding a new auto/marker trait is a Very Big Deal. It's not just that "auto traits = bad," but that adding a new one comes with very significant trade offs. That doesn't mean that adding one is impossible, but it does mean, IMO, that adding one needs to come with significant benefits. Or that it's the only path forward for some feature that most everyone deems to be essential. From the discussion, it sounds like there are alternatives. So those alternatives, at minimum, should be explored. |
@BurntSushi note that the |
@RalfJung Yikes. OK. Thank you for pointing that out. That's exactly the kind of context I was missing. |
I like the name Immutable. Maybe it will help shift people away from using the term "immutable reference" vs "shared reference". Agreed that the shallow vs deep distinction is potentially confusing, particularly in contrast to Send, and agreed that it is orthogonal.
Message ID: ***@***.***>
|
# 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. |
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 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.
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 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.
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 (joking - no clue what we'd name it) that is Freeze even if T isn't.
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.
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.
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.
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
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'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
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 byTryFromBytes
(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
- This is responsible for checking size and alignment, but not validity; in our codebase, it is used both by
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>
.
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.
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.
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 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.
Hello again everyone, I'm back and ready to keep pushing on this. I've already committed the result of every suggestion that seemed immediately actionable to me. It appears to me that 2 remaining roadblocks are:
As an aside to maintainers around here, is there a chance we can slip this in 1.79 if we're quick enough about resolving those questions, or is the release cycle already too far gone? |
One remaining question in my mind. I'm not sure whether this would affect the design, but it might. Do we want to leave open the possibility of implementing struct Foo<T>(UnsafeCell<T>);
static_assertions::assert_not_impl_any!(Foo<u8>: Freeze);
static_assertions::assert_impl_all!(Foo<()>: Freeze); This would make ZST-ness (or "zestiness", as @jswrenn prefers to pronounce it) a semver-visible property, but that's already true for some types (e.g. Obviously a lot of thorny questions to work out - I'm not talking about working them out here, but just making sure we don't foreclose on the future possibility. Maybe add it to the "future possibilities" section of the RFC. |
@joshlf, I believe that possibility is permitted. See the reference level explanation's proposal of
This explanation is phrased in terms of mutability-without-indirection, rather than the presence of absence of That said, the current implementation of |
Yeah, so I think we'd just want to update the proposed doc comment to be more explicit about the fact that |
1.79 is in beta and not going to receive any new features. If an implementation of this got merged before June 7th, it would be part of 1.80. But given t-lang capacity and the 10-day FCP period for the RFC, I don't want to raise any false expectations -- that's very unlikely. If it makes it into 1.81 (which branches on July 19th) that would be extremely fast by RFC standards, but we can possibly used the fact that this fixes a regression as an argument for prioritizing this in t-lang discussions. (I can't make that call, I am not on the lang team.) Rust is moving too fast for some people's liking, but not that fast. |
I think it does. I think a nice way to allow for this to fit the RFC nicely would be to:
|
Bunch of grumps! 🥲
So at least 2 more months of sad |
I'm afraid it's going to be more than 2 months -- 1.81 branches on July 19th, i.e. that's when the 6-week beta period starts. It is going to be released on Sep 5th. I hope this is not too much of a downer, your help here is much appreciated! It's just generally not good idea to rush irreversible decisions that the entire Rust community will have to live with forever. |
Honestly, the downer was 1.78; now I'm just fired up to get that situation resolved! I fully understand wanting to do due diligence on these things, I've been hurt by both ends of the stabilization stick before (looking at you, forever ABI-unstable But just to keep a sense of urgency: not making it to 1.81 would be a downer 😛 As an aside: I've updated the proposal to include |
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Hiya @RalfJung (sorry if the ping is a bit cavalier), do you see any work this RFC still needs before it can enter FCP? :) |
That's a question for t-lang, not for me. :) I have nominated the RFC to be discussed by the lang team. |
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: Gábor Lehel <glaebhoerl@gmail.com>
Hi @RalfJung, sorry for pestering you, any updates on the RFC's status? Let me know if there's a more appropriate place/person to follow up with on this RFC :) |
It's in the lang team's nomination queue. They probably have several dozen items in that queue so we'll have to see when they get around to discussing this RFC.
|
Rendered