Skip to content
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

Add MaybeUninit method array_assume_init #80600

Merged
merged 6 commits into from
Jan 12, 2021

Conversation

joboet
Copy link
Contributor

@joboet joboet commented Jan 1, 2021

When initialising an array element-by-element, the conversion to the initialised array is done through mem::transmute, which is both ugly and does not work with const generics (see #61956). This PR proposes the associated method array_assume_init, matching the style of slice_assume_init_*:

unsafe fn array_assume_init<T, const N: usize>(array: [MaybeUninit<T>; N]) -> [T; N];

Example:

let mut array: [MaybeUninit<i32>; 3] = MaybeUninit::uninit_array();
array[0].write(0);
array[1].write(1);
array[2].write(2);

// SAFETY: Now safe as we initialised all elements
let array: [i32; 3] = unsafe {
     MaybeUninit::array_assume_init(array)
};

Things I'm unsure about:

  • Should this be a method of array instead?
  • Should the function be const?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Could you please open a tracking issue or pick one of the existing maybeuninit ones to track this under?

@joboet
Copy link
Contributor Author

joboet commented Jan 11, 2021

Here it is: #80908

@dtolnay
Copy link
Member

dtolnay commented Jan 11, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2021

📌 Commit dec8c03 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0308]: mismatched types
   --> library/core/src/mem/maybe_uninit.rs:843:13
    |
242 | impl<T> MaybeUninit<T> {
    |      - this type parameter
...
835 |     pub unsafe fn array_assume_init<const N: usize>(array: [Self; N]) -> [T; N] {
    |                                                                          ------ expected `[T; N]` because of return type
...
843 |             (&array as *const _ as *const T).read()
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected array, found type parameter `T`
    |
    = note:       expected array `[T; N]`
            found type parameter `T`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `core`
error: could not compile `core`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:16

@RalfJung
Copy link
Member

Okay then let's @bors r- until the new implementation is done.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2021
@dtolnay
Copy link
Member

dtolnay commented Jan 12, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 12, 2021

📌 Commit 985071b has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#79757 (Replace tabs earlier in diagnostics)
 - rust-lang#80600 (Add `MaybeUninit` method `array_assume_init`)
 - rust-lang#80880 (Move some tests to more reasonable directories)
 - rust-lang#80897 (driver: Use `atty` instead of rolling our own)
 - rust-lang#80898 (Add another test case for rust-lang#79808)
 - rust-lang#80917 (core/slice: remove doc comment about scoped borrow)
 - rust-lang#80927 (Replace a simple `if let` with the `matches` macro)
 - rust-lang#80930 (fix typo in trait method mutability mismatch help)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit babfdaf into rust-lang:master Jan 12, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 12, 2021
@joboet joboet deleted the maybe_uninit_array_assume_init branch January 14, 2021 10:31
// * MaybeUnint does not drop, so there are no double-frees
// And thus the conversion is safe
unsafe {
intrinsics::assert_inhabited::<T>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assertion really correct?

I tried this code:

#![feature(maybe_uninit_array_assume_init)]

use std::mem::MaybeUninit;

enum Uninhabited {}

fn main() {
    unsafe {
        MaybeUninit::<Uninhabited>::array_assume_init([]);
    }
}
And it panicked with attempted to instantiate uninhabited type `Uninhabited`.
   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.12s
     Running `target/debug/playground`
thread 'main' panicked at 'attempted to instantiate uninhabited type `Uninhabited`', /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/core/src/mem/maybe_uninit.rs:842:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/core/src/panicking.rs:50:5
   3: core::mem::maybe_uninit::MaybeUninit<T>::array_assume_init
             at /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/core/src/mem/maybe_uninit.rs:842:13
   4: playground::main
             at ./src/main.rs:9:9
   5: core::ops::function::FnOnce::call_once
             at /rustc/f4eb5d9f719cd3c849befc8914ad8ce0ddcf34ed/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

But it seems that creating an array of uninhabited type with length of zero is not UB (playground):

#![forbid(unsafe_code)]

#[derive(Debug)]
enum Uninhabited {}

fn main() {
    let not_ub: [Uninhabited; 0] = [];
    dbg!(not_ub);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! This should probably be

if N > 0 { assert_inhabited(); }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit a PR to fix it.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 26, 2021
…ertion, r=m-ou-se

Fix assertion in `MaybeUninit::array_assume_init()` for zero-length arrays

That assertion has a false positive ([playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=63922b8c897b04112adcdf346deb1d0e)):
```rust
#![feature(maybe_uninit_array_assume_init)]

use std::mem::MaybeUninit;

enum Uninhabited {}

fn main() {
    unsafe {
        // thread 'main' panicked at 'attempted to instantiate uninhabited type `Uninhabited`'
        MaybeUninit::<Uninhabited>::array_assume_init([]);
    }
}
```
*Previously reported in rust-lang#80600 (comment)

This PR makes it ignore zero-length arrays.

cc rust-lang#80908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants