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

Const generics: Generic array transmutes do not work #61956

Open
HadrienG2 opened this issue Jun 19, 2019 · 27 comments
Open

Const generics: Generic array transmutes do not work #61956

HadrienG2 opened this issue Jun 19, 2019 · 27 comments
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@HadrienG2
Copy link

HadrienG2 commented Jun 19, 2019

This trivial transmute should work:

#![feature(const_generics)]

fn silly_default<const N: usize>(arr: [u8; N]) -> [u8; N] {
    unsafe { core::mem::transmute::<_, _>(arr) }
}

Unfortunately, it refuses to build with the following error:

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
 --> src/lib.rs:4:14
  |
4 |     unsafe { core::mem::transmute::<_, _>(arr) }
  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `[u8; _]` does not have a fixed size

This means that the array initialization example from the core::mem::MaybeUninit documentation currently cannot be generalized to arrays of arbitrary size, as transmutes from [MaybeUnit<T>; N] to [T; N] won't compile.

A horribly error-prone workaround for generic array transmute is:

// Using &mut as an assertion of unique "ownership"
let ptr = &mut arr as *mut _ as *mut [T; N];
let res = unsafe { ptr.read() };
core::mem::forget(arr);
res
@HadrienG2 HadrienG2 changed the title Const generics: Identity array transmute does not work Const generics: Generic array transmutes do not work Jun 19, 2019
@Centril Centril added A-const-generics Area: const generics (parameters and arguments) T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 19, 2019
@DutchGhost
Copy link
Contributor

Transmuting a MaybeUninit<T> to T is already rejected, and thus transmuting from any wrapper containing a MaybeUninit<T> to a Wrapper<T> is also rejected.

Yes, [MaybeUninit<T>; N] to [T; N] should be okey, but Option<MaybeUninit<T>> to Option<T> isnt in all cases.

@HadrienG2
Copy link
Author

HadrienG2 commented Jun 19, 2019

Aha, I didn't check that generally speaking MaybeUninit<T> -> T is also forbidden in isolation. Thanks for pointing it out!

To be honest, this particular rule seems bogus, given that layout-compatibility between non-wrapped MaybeUninit<T> and T is considered so important that it justified introducing a new meaning of #[repr(transparent)] almost solely for its sake.

Considering the lengths that were taken to ensure maximal layout-compatibility between MaybeUninit<T> and T, I think this transmute, on its own really should be okay. As you rightfully point out, it is only wrapper types with NonNull-style optimizations that muddy the water.

And arrays should not be put in this category, given that each item of [T; N] must have the same layout as T (since you can get an &T out of an &[T; N]) and that the stride of [T; N] should only be a function of the size and alignment of T (which is guaranteed to be the same as that of MaybeUninit<T>), not of the precise definition of T.

Further, not providing a reliable transmute path between [MaybeUninit<T>; N] and [T; N] would mean that the MaybeUninit abstraction is not currently able to replace mem::uninitialized() for the purpose of iterative array initialization in the way that is sketched in its documentation, which is a significant issue given that the latter is scheduled to be soon deprecated in favor of the former.

As an aside, while your observation is highly relevant to the target use case that motivated this issue, I should point out that it is not sufficient to explain why the reduced example from the OP failed to compile, as transmuting from MaybeUninit<u8> to u8 is allowed and transmuting from [MaybeUninit<u8>; SOME_CONST] to [u8; SOME_CONST] is allowed as well. Therefore, there has to also be some const generics-specific trickery at work here.

@DutchGhost
Copy link
Contributor

DutchGhost commented Jun 19, 2019

I totally agree with you.
Not being able to transmute is error prone, because:

  1. You either have to do pointer casts, a ptr::read, and dont forget to stick a mem::forget in there, or your toasted
  2. Or use mem::transmute_copy, but again dont forget to mem::forget. But transmute_copy is just...wildly more unsafe than transmute.

But, going even deeper on transmute

You can't even write this today:

fn useless_transmute<T>(x: T) -> T {
   unsafe { core::mem::transmute(x) }
}

Clearly T is the same size as T, but compiler isn't sure about it. Transmute only seems to work if the types you feed into it are known, and you cant really use it generically.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 20, 2019

To be honest, this particular rule seems bogus, given that layout-compatibility between non-wrapped MaybeUninit and T is considered so important that it justified introducing a new meaning of #[repr(transparent)] almost solely for its sake.

This is incorrect: repr(transparent) was introduced for ABI compatibility, not layout compatibility.

but Option<MaybeUninit<T>> to Option<T> isnt in all cases.

This is incorrect: MaybeUninit<T> and T are layout compatible, which means that Option<MaybeUninit<T>> and Option<T> are layout compatible for all Ts as well.

--

cc @eddyb I think the transmute<T, U> check is being overly conservative here. If both T and U have the same size, transmuting should work. Proving this for generic types is hard, but for this particular case (from union with one non-zero-sized field to type of the non-zero-sized field), it should be possible.

@HadrienG2
Copy link
Author

HadrienG2 commented Jun 20, 2019

This is incorrect: repr(transparent) was introduced for ABI compatibility, not layout compatibility.

Isn't ABI compatibility a superset of layout compatibility?

If, for example, the fields of a struct S are layed out differently than those of a previously initialized MaybeUninit<S>, then I cannot see how a precompiled library built to accept binary values of type S can correctly process (initialized) binary values of type MaybeUninit<S>. So it seems to me that ABI compatibility implies layout compatibility.

On the other hand, I can see how ABI compatibility could encompass other things in addition to layout compatibility, such as a guarantee that MaybeUninit<u32> is passed to methods via registers like u32 is for example.

This is incorrect: MaybeUninit<T> and T are layout compatible, which means that Option<MaybeUninit<T>> and Option<T> are layout compatible for all Ts as well.

Are you sure about this? My understanding so far was that for example, although Option<NonNull<_>> has a null pointer pointer optimization, Option<MaybeUninit<NonNull<_>>> should not have it as the possibly-uninitialized NonNull<_> inner value might be in an invalid null state. So the latter will get a discriminant field that the former doesn't have.

@DutchGhost
Copy link
Contributor

Option<MaybeUninit<&'static T>> has a size of 16, while Option<&'static T> has a size of 8, thus transmuting the first into the latter would be UB.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 20, 2019

I think you are right. For some reason I thought that this issue had already been resolved to allow this, but that has not happened yet. rust-lang/unsafe-code-guidelines#73

The TL;DR: is that, if have an union U { a: WithNiche, b: NoNiche }, where both types have the same size such that they fully overlap, then U cannot have a niche. However, if you have an union V { a: (), b: WithNiche }, then depending on how the validity of the union is defined (which is still WIP), the union could end up having a niche, and that allows Option<V> to have the exact same size as V.

@HadrienG2
Copy link
Author

To quote @RalfJung 5 minutes ago on the topic you linked to:

One thing that everyone seems to agree on though (including the above definition) is that if the union has a field of size 0 (such as is the case for MaybeUninit), then it may contain any value and thus there can be no layout optimizations.

I would propose continuing this part of the discussion there.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 20, 2019

Yep, that kind of followed from this Zulip discussion that we just had: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/validity.20of.20unions Feel free to also chime in there. Sorry for the noise, I completely misremembered what we wanted to do there.

@RalfJung
Copy link
Member

Yes, [MaybeUninit; N] to [T; N] should be okey

The hope is to eventually have APIs for Box-of-MaybeUninit and array/slice-of-MaybeUninit that handle such cases, so no transmute should be necessary. But at least for the array part, last time I tried writing down the APIs we wanted was not possible. But as const generics matures, we are getting closer and closer to that. :)

Also, to state the obvious, the transmute you mentioned has the same restrictions and MaybeUninit::assert_initialized.

@eddyb
Copy link
Member

eddyb commented Jun 25, 2019

@gnzlbg We do have some special-cases already, e.g. you can transmute between Box<T> and Rc<T> (not that it would be useful, given Rc's need for a prefix) even when T isn't known to be Sized, via:

rust/src/librustc/ty/layout.rs

Lines 1586 to 1604 in 10deeae

/// Type size "skeleton", i.e., the only information determining a type's size.
/// While this is conservative, (aside from constant sizes, only pointers,
/// newtypes thereof and null pointer optimized enums are allowed), it is
/// enough to statically check common use cases of transmute.
#[derive(Copy, Clone, Debug)]
pub enum SizeSkeleton<'tcx> {
/// Any statically computable Layout.
Known(Size),
/// A potentially-fat pointer.
Pointer {
/// If true, this pointer is never null.
non_zero: bool,
/// The type which determines the unsized metadata, if any,
/// of this pointer. Either a type parameter or a projection
/// depending on one, with regions erased.
tail: Ty<'tcx>
}
}

We could add a general case, to replace LayoutError::Unknown for the purposes of computing a SizeSkeleton, which mostly handles removing any wrapping that doesn't change the size.
This can be tricky to compute correctly, because of things like alignment, which can increase the size.

I'd argue it's kind of broken already, this should check that no #[repr(align)] is present:

rust/src/librustc/ty/layout.rs

Lines 1643 to 1647 in 10deeae

ty::Adt(def, substs) => {
// Only newtypes and enums w/ nullable pointer optimization.
if def.is_union() || def.variants.is_empty() || def.variants.len() > 2 {
return Err(err);
}

And this should check for alignments larger than 1 (which can make a ZST have an effect on size):

rust/src/librustc/ty/layout.rs

Lines 1659 to 1660 in 10deeae

SizeSkeleton::Known(size) => {
if size.bytes() > 0 {

cc @oli-obk @nagisa

@maplant
Copy link

maplant commented Jun 27, 2019

This should be aloud. My experimental const generic crate aljabar is blocked by this issue and currently has to rely on mem::uninitialized to move out of arrays with const generic params.

What is especially annoying about this is that size_of, a const function, is totally valid for arrays with const generic params. So the compiler has the ability to determine the size of both arrays at compile time. (edit: probably in a different step of the compiler, I'm not making that comment because I assume this'll be easy to fix)

@HadrienG2
Copy link
Author

HadrienG2 commented Jun 27, 2019

@maplant Note that as was figured out by @DutchGhost, the error message for arrays is actually a misleading red herring. Even a plain identity transmute from T to T doesn't work.

/// Fails to build with a weird message about type size issues
fn identity<T: Sized>(x: T) -> T { unsafe { core::mem::transmute(x) } }

Yields

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
--> src/lib.rs:2:45
  |
2 | fn identity<T: Sized>(x: T) -> T { unsafe { core::mem::transmute(x) } }
  |                                             ^^^^^^^^^^^^^^^^^^^^
  | = note: `T` does not have a fixed size

Playground

I also added a pointer-based workaround for the lack of array transmute in the opening post if it can be of any use to you.

@RalfJung
Copy link
Member

transmute_copy is another thing you can use when the size check is in your way. Use with caution though, that beast is even more dangerous than transmute...

@maplant
Copy link

maplant commented Jun 27, 2019

@HadrienG2 right, as another person pointed out to me pointers can easily transmute between arrays. Thanks for the help.

@eddyb
Copy link
Member

eddyb commented Jul 1, 2019

@maplant size_of::<T> doesn't take any arguments, and is completely deterministic, so of course it can be computed at compile-time - however, it requires knowing the concrete type (from monomorphization).

The transmute check is performed on the generic definition, like all the type-checking in Rust.

If you emulate it with assert_eq!(size_of::<T>(), size_of::<U>()), that will compile into either nothing (if they have equal sizes) or an unconditional panic (otherwise), based on the T and U.
However, that panic will only trigger at runtime, it can't stop the compilation.

@joboet
Copy link
Member

joboet commented Dec 29, 2020

There is another way to convert (using unions), e.g. like this.

I would be in favour of adding a new assume_init function for arrays of MaybeUninit, that way one can reduce the amount of unsafe code needed to initialise an array to just one single line.

@RalfJung
Copy link
Member

I would be in favour of adding a new assume_init function for arrays of MaybeUninit

What would be the signature of that function?

@joboet
Copy link
Member

joboet commented Dec 29, 2020

Something like:

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

Matching the style of MaybeUninit::slice_assume_init_*(...)

@RalfJung
Copy link
Member

Yeah, there are definitely some const generic functions that should be added. Const generics weren't ready for that yet when MaybeUninit was added.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 12, 2021
…me_init, r=dtolnay

Add `MaybeUninit` method `array_assume_init`

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 rust-lang#61956). This PR proposes the associated method `array_assume_init`, matching the style of `slice_assume_init_*`:

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

Example:
```rust
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?
@Mark-Simulacrum Mark-Simulacrum removed the requires-nightly This issue requires a nightly compiler in some way. label Feb 20, 2021
@scottmcm
Copy link
Member

I believe this is by-design right now from a lang perspective (see #86281 (comment)).

The answer for the "array initialization example" is MaybeUninit::array_assume_init, tracked in #80908

@eddyb
Copy link
Member

eddyb commented Aug 24, 2021

I was looking back on #61956 (comment) and and ended up opening #88290 for that aspect - which is what I should've done back then, my bad.

@lukaslihotzki
Copy link

Alternatively, the generic transmute workaround can be written as a single expression:

(&*(&MaybeUninit::new(arr) as *const _ as *const MaybeUninit<_>)).assume_init_read()

This version may be less error-prone because you can't forget to mem::forget. Also, it does not require arr to be mutable and it seems to produce less code without optimization (optimized, it's the same).

matthieu-m pushed a commit to matthieu-m/ghost-cell that referenced this issue May 27, 2022
…ferences

* Changes:

- Add implementation of GhostBorrow for arrays of references to GhostCell
- Add implementation of GhostBorrowMut for arrays of references to GhostCell.
- Various comments and documentation nits.

* Motivation:

More flexibility.

* Implementation Note:

Due to `mem::transmute` not working well with const generics (as per rust-lang/rust#61956), `ptr::read` is used instead of `mem::transmute`. This is expected to be safe, if verbose.
@lcnr lcnr removed the F-const_generics `#![feature(const_generics)]` label Jun 28, 2022
@matheus-consoli
Copy link

matheus-consoli commented Nov 17, 2022

I think the same problem also applies to tuples

Giving a generic tuple (MaybeUninit<T>, MaybeUninit<U>), it's not possible to transmute it to (T, U), resulting in the same cannot transmute between types of different sizes, or dependently-sized types. The concrete version works, (MaybeUninit<ConcreteTypeA>, MaybeUninit<ConcreteTypeB>) -> (ConcreteTypeA, ConcreteTypeB)`

Is this mapped or even correlated?

edit: playground

@memoryruins
Copy link
Contributor

memoryruins commented Nov 19, 2022

The example [u8; N] -> [u8; N] in the original post currently compiles on 1.66-beta.1/1.67-nightly, as well as the example in #61956 (comment) of T -> T. [MaybeUninit<T>; N] -> [T; N] continues to error.

Was this lifted by #101520?

@scottmcm
Copy link
Member

scottmcm commented Nov 19, 2022

This version may be less error-prone because you can't forget to mem::forget

I know this is a comment from ages ago, but remember you can use ManuallyDrop to also not be able to forget the forget if you need a not-fixed-size transmute:

mem::transmute_copy(&ManuallyDrop::new(arr))

VarLad added a commit to VarLad/Yggdrasil that referenced this issue Dec 17, 2022
Will hopefully enable a lot of these platforms back in future Rust versions
rust-lang/rust#61956
giordano pushed a commit to JuliaPackaging/Yggdrasil that referenced this issue Dec 18, 2022
* Add recipe for libwinit

* Fix typo

* Disable some platforms

Will hopefully enable a lot of these platforms back in future Rust versions
rust-lang/rust#61956
@workingjubilee workingjubilee added the A-array Area: `[T; N]` label Mar 7, 2023
@isikkema
Copy link

I used this relatively readable workaround for const generic arrays.

let arr: [T; N] = arr.map(|elem: MaybeUninit<T>| unsafe { elem.assume_init() });

I haven't checked into how optimized it is, but I'd imagine it's likely slower than every other solution here. Still, the readability is a big plus for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.