Regression transmuting `RwLockReadGuard<T: ?Sized>`.

[zachs18]

RwLockReadGuard<T> is now not transmutable when T is (dependent on) a generic parameter and ?Sized, when it was previously

(I don't necessarily think this is a significant enough problem to be worth "fixing", but I guess it is technically a regression).

Code

I tried this code:

use std::sync::RwLockReadGuard;
// the lifetimes aren't important
fn test1<'to, 'from: 'to, T: ?Sized>(guard: RwLockReadGuard<'from, T>) -> RwLockReadGuard<'to, T> {
    unsafe { std::mem::transmute(guard) }
}

I expected to see this happen: compilation success

Instead, this happened: compilation error

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
 --> src/main.rs:7:14
  |
7 |     unsafe { std::mem::transmute(guard) }
  |              ^^^^^^^^^^^^^^^^^^^
  |
  = note: `RwLockReadGuard<T>` does not have a fixed size

Version it worked on

It most recently worked on: Rust 1.63.0

Version with regression

rustc --version --verbose:

rustc 1.64.0-beta.3 (82bf34178 2022-08-18)
binary: rustc
commit-hash: 82bf34178fb14562d158c4611bdab35d4d39b31c
commit-date: 2022-08-18
host: x86_64-unknown-linux-gnu
release: 1.64.0-beta.3
LLVM version: 14.0.6

Backtrace

(no backtrace, compiler didn't crash)

Why

This commit changed the definition of RwLockReadGuard from

pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
    lock: &'a RwLock<T>,
}

to

pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
    inner_lock: &'a sys::MovableRwLock,
    data: &'a T,
}

(The following is (AFAICT) why this change caused this regression; It is not itself a regression, and it applies to all (recent) versions of rustc (i.e. since at least 1.41.0))

AFICT, rustc currently only supports transmuting a subset of Sized types that it conservatively can guarantee are the same size. For generic struct types with ?Sized parameters, i think rustc can only guarantee the size if the whole struct's size does not depend on the parameters, or if the struct's only non-zst field is a (transitive transparent wrapper of a) pointer type depending on a generic parameter (rustc_middle::ty::layout::SizeSkeleton only has Known(usize) and Pointer { .. } variants, so can only express completely known and only-a-pointer sizes), e.g.

pub struct Works<T: ?Sized> { // SizeSkeleton::compute returns Ok(Pointer { .. }) for this when T: ?Sized
    inner: *mut T,
}
pub unsafe fn works<T: ?Sized>(from: Works<T>) -> Works<T> {
    std::mem::transmute(from) // works
}

#[repr(align(32))]
struct OverAlignZST;

pub struct AlsoWorks<T: ?Sized> { // SizeSkeleton::compute returns Ok(Pointer { .. }) for this when T: ?Sized
    inner: *mut T,
    other: OverAlignZST,
}
pub unsafe fn also_works<T: ?Sized>(from: AlsoWorks<T>) -> AlsoWorks<T> {
    std::mem::transmute(from) // works
}

pub struct DoesntWork<T: ?Sized> { // SizeSkeleton::compute returns Err(Unknown(T)) for this when T: ?Sized
    inner: *mut T,
    other: u8,
}
pub unsafe fn doesnt_work<T: ?Sized>(from: DoesntWork<T>) -> DoesntWork<T> {
    std::mem::transmute(from) // DoesntWork<T> does not have a fixed size
}

(See also: #101084 )

Moved to #101084

(See also: #101084) Unrelated note: I think this size-calculation ignoring all ZSTs (not just 1-ZSTs) when there is a pointer field can cause `transmute` to (incorrectly?) allow transmuting between differently-sized types, e.g. this compiles and runs normally, but ICEs under miri:

#[repr(align(32))]
struct OverAlignZST;

pub struct AlsoWorks<T: ?Sized> { // SizeSkeleton::compute returns Ok(Pointer { .. }) for this when T: ?Sized
    inner: *mut T,
    other: OverAlignZST,
}
// This does not compile if you remove ?Sized
pub unsafe fn also_works<T: ?Sized>(from: *mut T) -> AlsoWorks<T> {
    std::mem::transmute(from) // works
}
fn main() {
    let mut x = vec![0; 16];
    let x = &mut x[..];
    unsafe {
        let x = also_works(x);
      }
 }

$ cargo miri run
...
thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `Size(16 bytes)`,
 right: `Size(32 bytes)`
...
query stack during panic:
end of query stack

Miri caused an ICE during evaluation. Here's the interpreter backtrace at the time of the panic:
note: the place in the program where the ICE was triggered
  --> src/main.rs:57:5
   |
57 |     std::mem::transmute(from) // works
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: inside `also_works3::<[i32]>` 
...

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

[saethlin]

This is not a regression, but not for the reason you say. Quoting the reference: https://doc.rust-lang.org/stable/reference/type-layout.html

Type layout can be changed with each compilation. Instead of trying to document exactly what is done, we only document what is guaranteed today.

Nominal types without a repr attribute have the default representation. Informally, this representation is also called the rust representation.

There are no guarantees of data layout made by this representation.

RwLockReadGuard does not have a repr attribute, so you're not allowed to rely on its layout.

[Mark-Simulacrum]

Nominating to get sign-off on acceptable breakage from libs-api; I expect this is an obvious case.

I think this is another case where we should consider making the compiler lint on transmutes of types that aren't marked with a repr, and maybe even hard error for std types.

[saethlin]

maybe even hard error for std types

While I would be personally in favor of this, such a change will cause breakage in the ecosystem, which I suspect others would conclude is too much.

[Mark-Simulacrum]

Yeah, I think it's a difficult tradeoff. We obviously can't catch everything (e.g., pointer casting is probably too far), but I think a warning is going to do 90% of the work for us and is hopefully relatively obviously OK.

[5225225]

I think this is another case where we should consider making the compiler lint on transmutes of types that aren't marked with a repr, and maybe even hard error for std types.

One thought I had was having transmute Kinda do the job of randomize_layout, in the sense that it goes "Okay, you're transmuting from (u32, u32) to u64. Now, the stdlib controls the layout of that tuple, and could legally insert padding there. So let's pretend it did, and then emit a warning (and later, ideally, a hard error)". Since we can just pretend that we laid out the type like that, and as long as we halt compilation because of it, I don't think anyone can notice to prove us wrong (there's no #[cfg(type_size)] feature)

You need to not forbid things like Cell<(u32, u32)> -> (u32, u32), but that shouldn't be too difficult?

I do think we've had enough issues where people are transmuting stdlib types (see: trying to change the layout of the std::net socket type) to justify being harsh here. Should I open a separate issue to try and hash out the details of how we do it there?

[BoxyUwU]

RwLockReadGuard does not have a repr attribute, so you're not allowed to rely on its layout

this is like forbidding transmuting Vec<T> -> Vec<T> though. The only thing differing here is the lifetime and I think it's a pretty reasonable assumption that types are never going to be able to change layout based on lifetime information. (The possibility of this happening is why specialization has basically been dead and considered unsound)