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)

[zachs18]

(This is mostly unrelated to this "regression", and is more just questions about transmute/transmute_copy's guarantees in general).

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

I agree that relying on a specific layout is not allowed, so I guess it makes sense that it's okay for transmute to not work when T is unknown (since it could affect the layout), but does this also mean that it is unsound to transmute_copy RwLockReadGuard<'a, T> to RwLockReadGuard<'b, T> when T is a concrete type? because I was under the impression that lifetimes could not affect codegen (including type layout), so Struct<'a> and Struct<'b> would be guaranteed to have the same (unspecified) layout (in one compilation), regardless of any explicit repr? (especially since types whose lifetime parameter is not invariant can be coerced to a different lifetime).

(Although, the type layout page you linked does not mention lifetimes at all, so maybe I was misinterpreting something from somewhere else.)

Although also, the docs for std::mem::transmute explicitly state that one of the uses of transmute is for "Extending a lifetime, or shortening an invariant lifetime", with an example using a repr(Rust) struct. (Maybe it is only intended to apply for types defined in the current crate? Or maybe only for types that (are known to) only contain references?)

// from https://doc.rust-lang.org/std/mem/fn.transmute.html
// implicitly repr(Rust)
struct R<'a>(&'a i32);
unsafe fn extend_lifetime<'b>(r: R<'b>) -> R<'static> {
    std::mem::transmute::<R<'b>, R<'static>>(r)
}
// ...

---

Even if "transmuting away lifetimes" is not allowed, presumably it should always be valid to transmute a type to itself regardless of layout? (or using transmute_copy, where transmute can't guarantee the size).

use std::sync::RwLockReadGuard;
fn test1<'a, T: ?Sized>(guard: RwLockReadGuard<'a, T>) -> RwLockReadGuard<'a, T> {
    // unsafe { std::mem::transmute(guard) } // this has the same problem as in my original comment
    // But. would transmute_copy (and forget) be unsound here?
    unsafe {
      let guard = std::mem::ManuallyDrop(guard);
      std::mem::transmute_copy(&from)
    }
}

---

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.

This would probably be good, but I think it would [need to be/benefit from being] somewhat recursive (since e.g.#[repr(C)] Struct(SomeReprRustType); would presumably be unsound to transmute if SomeReprRustType is unsound to transmute? (except maybe to another repr(C) struct with that same SomeReprRustType-typed field in the same place?)). Also, it should probably not lint/error on transmutes between a default/repr(Rust) type and a repr(transparent) wrapper of that same type.

[RalfJung]

Yes, indeed you are not supposed to transmute Vec. You should never assume you can transmute any type from another crate unless the type explicitly says so. This is true even if they have a repr annotation, as not all repr annotations are stable guarantees. See e.g. #90435 #66401.

[m-ou-se]

Whether transmuting between types that only differ in lifetimes should work or not is not really a library issue, but more a question for the language team. One could argue it's a bug if std::mem::transmute<Something<'a>, Something<'b>> doesn't compile, which is not new to 1.63.

[m-ou-se]

So to clarify, the question for the language team:

• Should std::mem::transmute<Something<'a>, Something<'b>>(..) always be accepted (even if Something<'_> isn't repr(anything))? It's not uncommon to use transmute to unsafely change a lifetime, but today that sometimes results in an error, depending on some details of Something: see above.

[nikomatsakis]

Per discussion in rust-lang/lang meeting today, we are re-nominating this for @rust-lang/types consideration.

[compiler-errors]

searched nightlies: from nightly-2022-06-01 to nightly-2022-09-06
regressed nightly: nightly-2022-06-26
searched commit range: fdca237...20a6f3a
regressed commit: 00ce472

bisected with cargo-bisect-rustc v0.6.3

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2022-06-01