mutable_transmutes lint should catch transmutes from a type without interior mutability to one with

[glandium]

I'm filing this as a regression, although the summary is worded in a feature request-ish way.

So, the regression itself is this: code that transmutes from a type without interior mutability to one with interior mutability leads to really bad outcomes after the bump to LLVM 16, because of the changes in https://github.com/llvm/llvm-project/commit/01859da84bad95fd51d6a03b08b60c660e642a4f. This is a pattern that happens in the wild, probably mostly around FFI. At least, that's how it happens in the Firefox codebase.

Code

Here is a reduced testcase using a similar pattern to what Firefox is using:

pub struct Foo(std::cell::UnsafeCell<usize>);

pub struct Bar([u8; 0]);

pub fn foo(f: &Bar) {
    unsafe {
        let f = std::mem::transmute::<&Bar, &Foo>(f);
        *(f.0.get()) += 1;
    }
}

With rustc up to 1.69.0 in --release mode, this produces the following IR:

define void @_ZN10playground3foo17hc0e352349f95f9ecE(ptr noalias nocapture noundef nonnull readonly align 1 %f) unnamed_addr #0 {
start:
  %0 = load i64, ptr %f, align 8, !noundef !2
  %1 = add i64 %0, 1
  store i64 %1, ptr %f, align 8
  ret void
}

With rustc 1.70.0-beta.2 in --release mode, this produces the following IR:

define void @_ZN10playground3foo17h2141d3a0b5fe8d73E(ptr noalias nocapture noundef nonnull readonly align 1 %f) unnamed_addr #0 {
start:
  ret void
}

Note how everything is gone because the input pointer is marked as noalias readonly, and thus the code is not expected to change what it points to, so it's all removed. This is the typical example of undefined behavior leading to the optimizer doing unexpected things. I'm not arguing that there isn't undefined behavior. The undefined behavior existed before. But with LLVM 16, now the undefined behavior is actively dangerous.

Now, to come back to the feature-request-y summary I wrote, the following code does not compile:

pub struct Bar([u8; 0]);

pub fn foo(f: &Bar) {
    unsafe {
        let _ = std::mem::transmute::<&Bar, &mut Bar>(f);
    }
}

The produced error is:

error: transmuting &T to &mut T is undefined behavior, even if the reference is unused, consider instead using an UnsafeCell

I would argue that the code that is now compiled to nothing should also produce a similar error, and that error should be shipped in 1.70.0.

Cc: @pcwalton, @nikic

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high +T-compiler +A-LLVM +regression-from-stable-to-beta

[ChayimFriedman2]

@rustbot claim

[glandium]

FWIW, we've hit yet another case of such a transmute that a lint would have made obvious in the first place...

[glandium]

While trying to find workarounds, I also figured out that the same footgun can be triggered with "manual" transmutes too... e.g.

let f = std::ptr::NonNull::from(f).cast::<Foo>().as_ref();

or

let f = (f as *const Bar as *const Foo).as_ref().unwrap();

I don't know if they exist in the wild, but a lint would sure be useful for those too.

[emilio]

This lint should be quite a big priority IMO. Code like this is busted right now if Base doesn't have an UnsafeCell:

#[repr(C)]
struct Derived {
    base: Base,
    something: RefCell<...>,
}

impl Base {
    fn as_derived(&self) -> Option<&Derived> {
        if !self.is_derived() { // Assume some sort of bit check here.
            return None;
        }
        Some(unsafe { std::mem::transmute(self) })
    }
}

It's totally non-obvious that this code is broken, and totally non-obvious that LLVM will mis-compile your code unless Base has an UnsafeCell or some other interior mutability. This is code that is reasonable (IMO) and has been working forever (Servo has used this kind of pattern for a long time, see https://github.com/servo/servo/blob/c86faae371f1319d136425e2ffcd80def48132fd/components/script/dom/bindings/inheritance.rs#L16)

[glandium]

an UnsafeCell or some other interior mutability

AIUI this actually boils down to UnsafeCell. Any other interior mutability that counts for this problem will have an UnsafeCell hidden somewhere.

[glandium]

BTW, PhantomData<UnsafeCell<...>> doesn't work around the problem either. (maybe it should?)

[sylvestre]

@samueltardieu do you think it could be added as a clippy checker?

[sagudev]

@ChayimFriedman2 any progress on this?

[ChayimFriedman2]

@sagudev I'm working on it (slowly). It is not as simple, because to handle the example in the OP also requires looking for nested fields, which the current code does not do.

[sagudev]

@sagudev I'm working on it (slowly). It is not as simple, because to handle the example in the OP also requires looking for nested fields, which the current code does not do.

Good luck.

Clippy has is_interior_mut_ty which might help.

[ChayimFriedman2]

Under Tree Borrows transmuting a & to &UnsafeCell works as long as there is no actual mutable access to the value, and there is even a test for that in Miri. What am I supposed to do with this lint? See also #118446.

[emilio]

@ChayimFriedman2 I think warn-by-default would be a better state than the current status quo.