-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ref-wrapping #3382
Ref-wrapping #3382
Conversation
Alternative: Safe |
This is actually slightly superior to safe transmute because it allows the wrapping type to maintain any extra invariants by having checks within the construction function. eg: |
text/0000-ref-wrapping.md
Outdated
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
As far as this RFC is concerned, there really isn't much prior art for this. Discussions have been had about using `as`-casting for this purpose, although as mentioned in the alternatives section, that has downsides. |
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.
Prior Art: the ref_cast
crate
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 we're listing crates we could mention bytemuck::TransparentWrapper
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.
Also aliri_braid
which is designed specifically for strongly-typed string wrappers.
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.
Also trapper
I like the general idea (and have indeed done the unsafe version by hand before), but the proposed syntax feels too loosey-goosey to me. I don't have much of a deep explanation as to why, however. I really want there to be a keyword in there because I don't want the conversion to happen "by accident" (ignoring whatever the probably of that occurring would actually be). I'd be happy if there was some kind of "visibility-limited |
I suppose unsized types like |
let hmm = &Path( unsize *s ); |
I'm not really sure what you're going for here. In terms of that second example, that actually would not be allowed, since |
That's fair, although one counter to this is the fact that
So, I did briefly discuss this in the RFC, and I brought up the point that it subtly creates the additional zero-sized fields, which can be a bit weird semantically. Even though conjuring (Apologies if you did already read that part; I'm mostly mentioning it because I also have a tendancy to skim RFCs and miss stuff like that.) |
I meant all unsized types work, so
and
but extern types remain a question. |
Oh, I see what you mean now. I don't believe there would be any problems with |
Is this change backwards-compatible? For a somewhat contrived example, consider this: // Can only be (safely) constructed via `Inner::new` and
// `Outer::new` because `x` is private.
pub struct Inner { x: u32, }
impl Inner {
pub fn new(x: u32) -> &'static Self {
Box::leak(Box::new(Self { x }))
}
}
// Can only be (safely) constructed via `Outer::new` because
// `Inner` is only exposed via immutable references.
#[repr(transparent)]
pub struct Outer {
// `x` is guaranteed to be even.
pub inner: Inner,
}
impl Outer {
pub fn new(x: u32) -> &'static Self {
assert!(x % 2 == 0);
Box::leak(Box::new(Self { inner: Inner { x } }))
}
} Right now, unless I'm missing something, this code can assume that instances of That will no longer be the case with this RFC, IIUC, which could be problematic, for example if The key items that interact here are that we have a |
@TimNN that code example is kind of breaking my mind, and I don't have a good answer for it. My gut response is that assuming that things are impossible because the APIs don't exist feels like something you can't do in a backwards-compatible way, but I don't know enough of the details to be sure. Since |
They may be able to rely on the layout being the same, but I don't think that says anything about validity. Consider This is discussed a bit in rust-lang/compiler-team#411 ("Lang Item for Transmutability") when describing why the
I believe such a restriction would be needed here as well. |
The example reminded me that there's no language-level distinction between " The example demonstrates that this RFC needs not just "field is public implies |
Going back to the proposed syntax for a moment, I'm also not that happy with it as it is, because it would be a surprise for me to see a function returning a local reference being accepted by the borrow checker. The reason why that would be allowed is not local to the function, so that would require investigation. Also, it it confusing for people learning a language when things have rules with special casing... |
I think I think the only serous syntax criticism would be if this somehow diverged pedagogically from existing |
I think this is a big issue. These additional concerns are the reasons why such casts nowadays require some level of |
As an alternative, what if there was a trait thay looked something like:
That could be derived for transparent structs. Or maybe even have a way to derive |
But with a trait, you lose the "only public if the field is public" aspect, and thus the "I can preserve a logical invariant" aspect. |
If there was a "private impl" feature, that would go away. Although I'm not sure if it would be worth blocking this on a different feature. |
I finally got around to updating the RFC to include the existing crates & the privacy concerns first mentioned by @TimNN. I do want to more formally address that last point in the RFC, but for now at least everything is mentioned in the text of the RFC itself. |
@TimNN I think you demonstrated that this operation must, in the general case, be unsafe - it can violate validity invariants, after all. However one could create an unsafe marker trait that roughly says "doing the ref wrapping thing isn't unsafe" and then it would be available for use in safe code. |
An unsafe repr attribute is less overhead than a marker trait, no? Ala |
Again, I'm not sure why it would have to be unsafe -- the issue here is that it would be a semver-breaking change, and thus has to be opt-in since |
The Imo, Perhaps a larger question is that this syntax obviously cannot work for Because of this, we need either not to use a normal literal constructor, or some way to specifically say that we're giving the constructor a place and not the value read from that place. |
Soundness of unsafe code is generally reliant on privacy, and unrestricted transmutes could work around privacy by building structs that can't normally be built with safe code, which could break invariants of unsafe code that depends on the struct not being buildable. Because of that, the author of the unsafe code must explicitly allow this transmute to happen. An unsafe marker trait was an idea, the unsafe attribute that @burdges proposed was another, but there must be some way for the author of the code to signal that safe transmuting is okay. |
I'm curious @CAD97 where else does |
Footnotes
|
Thank you for adding that clarification. I was struggling to see how such a semantic difference would be observable, since most of the time the distinction between mutating the temporary and the result seems like it wouldn't matter much. As a very bad (imo) syntax for getting around the limitations we could require wrapping the expression in a custom macro, but I don't like this for hopefully obvious reasons. Once I have more time to elaborate on this I'll take another look through the proposal and see if I can make it more resilient to this, somehow. Might involve a separate repr than |
This syntax worries me, since it's making the whole struct literal a weird sort of place, and I don't know how to feel about that. In particular, if I change one of the examples from use std::marker::PhantomData;
pub struct WeirdType<T> {
marker: PhantomData<T>,
id: u32,
}
impl<T> WeirdType<T> {
pub fn from_str(s: &u32) -> &WeirdType<T> {
&WeirdType {
marker: PhantomData,
id: *s,
}
}
} then how is the compiler supposed to know that it's supposed to do this place trick, rather than make a reference to a temporary (because the Or what would be the syntax if I want to get a view like this of a local variable without moving it? Do I have to write I think it should use some form that makes it easier to distinguish what's happening. Maybe TBH, much as I dislike |
Yeah, no matter what I think that the chosen solution should use the visibility of the constructor to decide who's allowed to perform the cast. That way, the standard privacy rules will still apply, invariants can be upheld, etc. |
Alright, but intuitively "zero cost" strongly suggests "places cannot change unless required to". In particular in I suppose the example by @TimNN suggests why this intuition breaks down for constructors. |
Thinking more about this, I think that it would be possible to have a situation where we semantically separate "value transparency" and "referential transparency," where the latter is a stronger version of the former. Right now, we can think of Value transparency can still be used for some future version of safe transmutes, but only for values. Meanwhile, referential transparency allows the kinds of operations detailed in this RFC. I'll update the text later to reflect this, and potentially we could allow a In terms of the issues with |
i think that's a good idea but I don't like referential transparency naming, since to me that implies something kinda like you can substitute the wrapped type for the wrapper type and vis versa and the program's behavior won't change, like a |
Yeah, to be fair, I wrote that when I was sleep-deprived and agree with you. "Borrow transparency" is probably a way better name. |
I'm going to close this since I initially thought this would be a pretty cut-and-dry feature, which is why I didn't go through any of the non-RFC forums before just writing this up, but it's clear now that this will need more discussion before the final version is proposed. I could just write up my own version and propose that, but that seems less and less like the path forward nowadays. Please feel free to build off of the existing work when creating any future proposals, though, as this is still something I would love to see. |
For
#[repr(transparent)]
structs and variants, allow&Wrapper(*reference)
as a safe alternative to transmutation.Rendered