Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upDeprecate uninitialized in favor of a new MaybeUninit type #1892
Conversation
canndrew
added some commits
Feb 9, 2017
This comment has been minimized.
This comment has been minimized.
|
cc @nikomatsakis, @arielb1, @eddyb. I know y'all have some thoughts on what to do with all this. |
This comment has been minimized.
This comment has been minimized.
|
Calling |
This comment has been minimized.
This comment has been minimized.
|
@ubsan, what about the Without this, any (otherwise correct) code that uses |
This comment has been minimized.
This comment has been minimized.
|
The compiler bug happened mostly because previously the |
nagisa
reviewed
Feb 9, 2017
|
|
||
| This trait is automatically implemented for all inhabited types. | ||
|
|
||
| Change the type of `uninitialized` to: |
This comment has been minimized.
This comment has been minimized.
nagisa
Feb 9, 2017
Contributor
This conflicts somewhat with Inhabited being an auto-trait (a-la Sized), or rather T is already Inhabited unless specified otherwise (i.e. T: ?Inhabited)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
canndrew
Feb 10, 2017
Author
Contributor
@mark-i-m Yes, size_of::<!>() == 0, More generally though, for any given size every element of ! has that size. Similarly, any two elements of ! have the same size. These are both trivially true since ! has no elements.
This comment has been minimized.
This comment has been minimized.
mark-i-m
Feb 10, 2017
Contributor
So we are defining size_of::<T>() == n iff for all values v of T that v takes n bits to express, and since there are no values of T = !, this vacuously true?
This comment has been minimized.
This comment has been minimized.
canndrew
Feb 10, 2017
Author
Contributor
Basically, yeah. There's two subtly different definitions you could give of Sized but ! satisfies both of them vacuously.
nagisa
reviewed
Feb 9, 2017
| This could be a rather large breaking change depending on how many people are | ||
| currently calling `uninitialized::<T>` with a generic `T`. However all such | ||
| code is already somewhat future-incompatible as it will malfunction (or panic) | ||
| if used with `!`. |
This comment has been minimized.
This comment has been minimized.
nagisa
Feb 9, 2017
Contributor
If Inhabited is auto-trait, like Sized, I do not see how that’s problematic in any way.
nagisa
reviewed
Feb 9, 2017
| The author of the crate may expect this change to be private and its effects | ||
| contained to within the crate. But in making this change they've also stopped | ||
| exporting the `Inhabited` impl, causing potential breakages for downstream | ||
| users. |
This comment has been minimized.
This comment has been minimized.
nagisa
Feb 9, 2017
Contributor
Again, that’s pretty much the same story with Sized. I.e. you cannot change pub struct Sized { a: [u8; 42] } to pub struct Sized { a: [u8] }.
This comment has been minimized.
This comment has been minimized.
|
We pretty much decided in the design sprint to deprecate |
This comment has been minimized.
This comment has been minimized.
|
@arielb1 Ah okay. I figured that might too radical for now so I intended this RFC as a (possibly temporary) middle ground. |
This comment has been minimized.
This comment has been minimized.
|
@nagisa Sorry if my wording is confusing. |
This comment has been minimized.
This comment has been minimized.
That’s not true. fn main() {
let x: *const ! = &0 as *const _ as *const _; // imagine this comes as an generic argument from somewhere
let z: ! = unsafe {
::std::mem::read(x) // boom bam blamma
};
} |
This comment has been minimized.
This comment has been minimized.
|
Ah true, I hadn't thought of I still think |
ubsan
reviewed
Feb 9, 2017
| match std::panic::catch_unwind(|| { | ||
| let val = f(); | ||
| unsafe { | ||
| (*foo_ref).value = val; |
This comment has been minimized.
This comment has been minimized.
ubsan
Feb 9, 2017
Contributor
This line is broken for types with Drop impls. It should be ptr::write(&mut (*foo_ref).value, val)
This comment has been minimized.
This comment has been minimized.
canndrew
Feb 10, 2017
Author
Contributor
Hmm, I thought we settled on different rules for overwriting union fields for some reason. Thanks for the catch!
aturon
added
the
T-lang
label
Feb 9, 2017
aturon
assigned
nikomatsakis
Feb 9, 2017
mark-i-m
reviewed
Feb 10, 2017
| ``` | ||
| Yet calling this function does not diverge! It just breaks everything then eats | ||
| your laundry instead. |
This comment has been minimized.
This comment has been minimized.
mark-i-m
Feb 10, 2017
Contributor
Somehow, this seems preferable to folding my laundry at the moment...
mark-i-m
reviewed
Feb 10, 2017
| if used with `!`. | ||
|
|
||
| Another drawback is that the `Inhabited` trait leaks private information about | ||
| types. Consider a type with the following definition: |
This comment has been minimized.
This comment has been minimized.
mark-i-m
Feb 10, 2017
Contributor
I am not convinced this is more serious than already-existing leaks... for example, you can already find out the size of a type. Is there any fundamental difference with this?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mark-i-m
reviewed
Feb 10, 2017
| Ideally, Rust's type system should have a way of talking about initializedness | ||
| statically. In the past there have been proposals for new pointer types which | ||
| could safely handle uninitialized data. We should seriously consider pursuing | ||
| one of these proposals. |
This comment has been minimized.
This comment has been minimized.
mark-i-m
Feb 10, 2017
Contributor
static muts much more powerful while still being safe.
This comment has been minimized.
This comment has been minimized.
|
@canndrew could you add the alternative from the design sprint? (add I find it compelling, as it removes some magic from the compiler (no special checks or automatic traits for size/inhabitedness) and lets the type system just do "its thing". |
This comment has been minimized.
This comment has been minimized.
|
Sorry, I just realised the option of completely deprecating |
This comment has been minimized.
This comment has been minimized.
|
Are there any plans to do the same to |
This comment has been minimized.
This comment has been minimized.
whataloadofwhat
commented
Feb 10, 2017
I agree with what you're saying about I'd be (more) okay with Myself, I think the best route is:
|
This comment has been minimized.
This comment has been minimized.
djzin
commented
Feb 12, 2017
|
Could this issue be solved with |
This comment has been minimized.
This comment has been minimized.
|
I think the definition of the size of a type discussed in one of the
earlier comments is nicer in that it gracefully handles !
On Feb 12, 2017 11:02 AM, "djzin" <notifications@github.com> wrote:
Could this issue be solved with ! having no size? If the size of a type is
defined to be ceil(log(n)) where n is the number of possible
representations, then ! should have undefined size, or, if you like,
"negative infinity" size. The main issue with this is that empty enums are
already defined as being sized... and also "negative infinity" is not
representable in a usize... but just a thought I had ;)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1892 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIazwIFjJbuaydpz5kxDgk8hWjqJSt9Xks5rbzsGgaJpZM4L8U6F>
.
|
This comment has been minimized.
This comment has been minimized.
newpavlov
commented
Aug 11, 2018
•
I think we either should do this (honestly I still do not understand why creating In other words we will have the following code: // io::Read, this change is backwards compatible with the existing signature
fn read_exact(&mut self, buf: &out [u8]) -> io::Result<()>;
// user code, note that we have only one unsafe line
let mut buffer: MaybeUninit<[u8; 4096]> = MaybeUninit::uninitialized();
// `get_out` returns `&out [u8; 4096]`
file.read_exact(buffer.get_out())?;
// we are sure that buffer is fully initialized
let buffer = unsafe { buffer.into_inner() };
parse(&buffer); |
This comment has been minimized.
This comment has been minimized.
|
I think it's reasonable to say "an fn foo1(x: &i32) { // not UB to pass &uninit
}
fn foo2(x: &mut i32) { // not UB to pass &mut uninit
ptr::write(x, 0);
}
fn foo3(x: &i32) {
*x; // maybe UB if one passes &uninit?
// (we may choose to guarantee that only the lvalue -> value conversion reads the memory)
}
fn foo4(x: &i32) {
*x == 0; // definitely UB if one passes &uninit, imo
}Where this gets interesting is something like fn foo(x: &&i32, y: bool) -> i32 {
*x;
if (y) {
**x // are we allowed to speculatively load **x ahead of time?
} else {
0
}
} |
This comment has been minimized.
This comment has been minimized.
pftbest
commented
Aug 11, 2018
|
@newpavlov Having an &out parameters is a neat idea. Maybe if we would have something like this in Rust, then there will be no need for a hacks like prepare_uninitialized_buffer to exist. |
This comment has been minimized.
This comment has been minimized.
|
The Rust reference says:
Surprisingly, that is what most Rust users actually need to know about references: Also, per definition,
It just follows from those two very simple rules. The memory you get from I find it almost unbelievable how simple these two rules are, and how much follows from them: I don't see how we could allow That can be a pretty big time bomb considering how people interpret IMO the current basic definition of |
This comment has been minimized.
This comment has been minimized.
Thomasdezeeuw
commented
Aug 11, 2018
|
For the reading: maybe |
This comment has been minimized.
This comment has been minimized.
newpavlov
commented
Aug 11, 2018
•
It's just that in my understanding, real UB here is creation of I guess it's about trade-off between higher level rules which are easier to track and verify, but with annoying false-positives (as in the example with Probably the most optimal solution will be to introduce |
This comment has been minimized.
This comment has been minimized.
This is correct, doing so violates the contract of
I think this is incorrect. As long as you don't violate any of Rust contracts, you have no undefined behavior. That The following contract is not written anywhere AFAIK, and I am not a memory model person so there might be a million things wrong about the following contract, but I'd expect every Rust memory model to somehow specify it in one way or another:
Copying uninitialized memory from one memory location to another does not violate this contract because it does not make the execution of the program depend on the content of the memory. Using the memory in any way that affects the execution of the program does, however, invoke undefined behavior. I agree with you that creating a Rust has many contracts, and all of them must hold for a program to have defined behavior. These are / could just be two of them. |
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg I would argue that code like @newpavlov showed is common enough that we should think about not making it UB - there should be a distinction here between what is UB, and what one is allowed to assume. |
This comment has been minimized.
This comment has been minimized.
newpavlov
commented
Aug 11, 2018
•
But before it you can safely create
I think that this quote is mostly about branching dependent on UB. With "don't read from uninitialized memory" rule this quote is still true, because program execution can't depend on something which is never read, but doing copy from uninitialized memory becomes UB. |
This comment has been minimized.
This comment has been minimized.
I agree, we could do that, but I am unconvinced on whether that is worth it the increased complexity.
Yes. If you write uninitialized memory via a pointer somewhere, and that breaks the contract of a reference, that is instant undefined behavior in my mental model: let mut x = 0_i32;
let y= &mut x;
let z = y as *mut i32;
drop(y); // not dropping y here would be UB
ptr::write(z, uninitialized()); |
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg It seems to me that writing an |
This comment has been minimized.
This comment has been minimized.
|
@ubsan That makes sense. I guess one would need to drop the object instead, the storage after the |
XOSplicer
referenced this pull request
Aug 16, 2018
Open
Show a constant's virtual memory on validation errors #53325
This comment has been minimized.
This comment has been minimized.
Well, there are other people saying the exact opposite. ;) We have to make a choice either way, but for both cases we have people arguing that this is clearly the more intuitive option.
No, there would be no incompatible change. As @ubsan mentioned, we have two invariants at play here: Which assumptions the compiler can make for its optimizations, and which assumptions safe code can make for values it sees. There is no a priori reason to think these are the same, and in fact I think it is rather impossible to make them the same. I have a blogpost upcoming for this that should hopefully be done no later than Monday... But just one example: A safe higher-order function which takes an argument There are other problems as well. For example, the invariant that may be assumed by safe code is impossible to check for because it is frequently not computable (as in, would require solving the halting problem). I think we should have a definition of UB that can, at least in principle, be checked. |
japaric
added a commit
to japaric/heapless
that referenced
this pull request
Aug 19, 2018
bors bot
added a commit
to japaric/heapless
that referenced
this pull request
Aug 19, 2018
japaric
referenced this pull request
Aug 19, 2018
Closed
Fix broken nightly: Const fn union workaround #52
rfcbot
added
finished-final-comment-period
and removed
final-comment-period
labels
Aug 19, 2018
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Aug 19, 2018
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
Centril
referenced this pull request
Aug 19, 2018
Open
Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type" #53491
Centril
merged commit 21f887f
into
rust-lang:master
Aug 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Huzzah! This RFC has been merged! Tracking issue: rust-lang/rust#53491 |
canndrew commentedFeb 9, 2017
•
edited by Centril
My thoughts on what to do with
uninitializedand!.Rendered
Tracking issue
Edit: This has been updated to instead recommend deprecating
uninitializedentirely. The oldInhabitedtrait proposal is now listed as an alternative.Edit: FCP Proposal is #1892 (comment) (in the collapsed-by-default part).