Lint against &T to &mut T and &T to &UnsafeCell<T> transmutes

[ChayimFriedman2]

Conversion from & to &mut are and always were immediate UB, and we already lint against them, but until now the lint did not catch the case were the reference was in a field.

Conversion from & to &UnsafeCell is more nuanced: Stacked Borrows makes it immediate UB, but in Tree Borrows it is sound.

However, even in Tree Borrows it is UB to write into that reference (if the original value was Freeze). In all cases crater found where the lint triggered, the reference was written into.

Lints (mutable_transmutes existed before):

The mutable_transmutes lint catches transmuting from &T to &mut T because it is undefined behavior.

Example

unsafe {
    let y = std::mem::transmute::<&i32, &mut i32>(&5);
}

Produces:

error: transmuting &T to &mut T is undefined behavior, even if the reference is unused, consider instead using an UnsafeCell
 --> .\example.rs:5:17
  |
5 |         let y = std::mem::transmute::<&i32, &mut i32>(&5);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `&i32` to `&mut i32`
  = note: `#[deny(mutable_transmutes)]` on by default

Explanation

Certain assumptions are made about aliasing of data, and this transmute
violates those assumptions. Consider using UnsafeCell instead.

---

The unsafe_cell_transmutes lint catches transmuting or casting from &T to &UnsafeCell<T>
because it dangerous and might be undefined behavior.

Example

use std::cell::Cell;

unsafe {
    let x = 5_i32;
    let y = std::mem::transmute::<&i32, &Cell<i32>>(&x);
    y.set(6);

    let z = &*(&x as *const i32 as *const Cell<i32>);
    z.set(7);
}

Produces:

error: transmuting &T to &UnsafeCell<T> is error-prone, rarely intentional and may cause undefined behavior
 --> .\example.rs:6:17
  |
6 |         let y = std::mem::transmute::<&i32, &Cell<i32>>(&x);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `*&i32` to `(*&Cell<i32>).value`
  = note: `#[deny(unsafe_cell_transmutes)]` on by default

error: transmuting &T to &UnsafeCell<T> is error-prone, rarely intentional and may cause undefined behavior
 --> .\example.rs:9:17
  |
9 |         let z = &*(&x as *const i32 as *const Cell<i32>);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: transmute from `i32` to `Cell<i32>.value`

Explanation

Conversion from &T to &UnsafeCell<T> might be immediate undefined behavior, depending on
unspecified details of the aliasing model.

Even if it is not, writing to it will be undefined behavior if there was no UnsafeCell in
the original T, and even if there was, it might be undefined behavior (again, depending
on unspecified details of the aliasing model).

It is also highly dangerous and error-prone, and unlikely to be useful.

Crater summary is below.

cc #111229

r? compiler

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[oli-obk]

@bors try

[bors]

⌛ Testing commit 5c1d66e with merge 94971c2...

[bors]

💔 Test failed - checks-actions

[oli-obk]

@bors try

[bors]

⌛ Trying commit 59c22c3 with merge be92fb7...

[tgross35]

@bors try

[bors]

⌛ Trying commit cc72a92 with merge 0914651...

[bors]

☀️ Try build successful - checks-actions
Build commit: 0914651 (09146510cdb33d15474a35fde290187a88b1cf35)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-128351 created and queued.
🤖 Automatically detected try build 0914651
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-128351 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[RalfJung]

Hm, just makes me wonder if there are any other wrong assumptions encoded in the lint...

[tmandry]

@rfcbot reviewed

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[ChayimFriedman2]

Since we haven't heard from the reviewer...

r? compiler

[nnethercote]

@oli-obk: would you be willing to review? This is covering lots of territory I don't know well, and I imagine there are subtleties that I could easily overlook.

[oli-obk]

😭 that's a lot of new code to review

[nnethercote]

This feels like it's heading in the right direction.

My main observation is about terminology. To me:

• "casting" is a specific thing, done via as or cast
• "transmuting" is a different specific thing, done only via mem::transmute
• "converting" is an imprecise term that possibly covers both "casting" and "transmuting".

Is that correct? It aligns with the Type Conversions chapter of the Rustonomicon. But this PR tends to use "cast" and "transmute" imprecisely and somewhat interchangeably.

• I think mutable_transmutes only triggers on mem::transmute, but unsafe_cell_transmutes triggers on both mem::transmute and casting.
• The crater result mentions unsafe_cell_reference_casting. Is that an old name for unsafe_cell_transmutes?
• The error message only mention transmuting.
• The lint descriptions (in doc comments) mention both transmuting and casting in a way that matches my understanding.
• is_type_cast seems to cover both casting and transmuting.

I feel like this needs to be clarified, and the whole commit needs a once-over to ensure these terms are being used precisely and consistently. Should unsafe_cell_transmutes should be renamed unsafe_cell_conversions?

---

Beyond that, mutable_transmutes.rs has most of the new code. I reviewed it and it seems reasonable, but that kind of traversal is not my forte so it's possible I missed stuff. All the breadcrumb code is quite complicated, and I do I wonder if something like that already exists elsewhere in the compiler for some other kind of error message.

[nnethercote]

Why is this test removed?

[nnethercote]

Maybe because there's more comprehensive testing in another file now?

[nnethercote]

The tests/ui/lint/mutable_transmutes directory name is slightly misleading because it contains tests that cover both mutable_transmutes and unsafe_cell_transmutes. I'm not sure how to fix it, though, and whether it's worth fixing.

[nnethercote]

Why does this one have a Builtin prefix but UnsafeCellTransmutes does not?

[nnethercote]

I read this comment several times, but I still don't really understand it. Explain some more, perhaps? Also, the comment talks about "from"/"to" but the fields are "before"/"after", which is confusing.

[nnethercote]

Suggested change

	//! This module check that we are not transmuting `&T` to `&mut T` or `&UnsafeCell<T>`.
	//! This module checks that we are not transmuting `&T` to `&mut T` or `&UnsafeCell<T>`.

[nnethercote]

Does it check casting as well?

[nnethercote]

These two lines are shared with the if block and can be factored out.

[nnethercote]

Suggested change

	// Then, for any entry that we overlap with (there can be many as our size can be bigger than one,
	// Then, for any entry that we overlap with (there can be many because our size can be bigger than one,

(I had to read this sentence twice to understand it. This is a pet peeve of mine, "because" is almost always clearer than "as".)

[nnethercote]

That's a cast, not a transmute...

[traviscross]

For the reasons described here, I would actually call this a transmute. It's using a cast to effect a transmutation. So it's a transmute via a cast.

[nnethercote]

This one only mentions transmuting...

[nnethercote]

This one mentions transmuting or casting...

[traviscross]

My main observation is about terminology. To me:

• "casting" is a specific thing, done via as or cast
• "transmuting" is a different specific thing, done only via mem::transmute
• "converting" is an imprecise term that possibly covers both "casting" and "transmuting".

Is that correct?

I'd take a somewhat more broad view of transmuting. Probably I think of that as essentially synonymous with reinterpreting some sequence of bytes as some other type without the presence of any automatic mechanism to reject invalid reinterpretations or to adjust the bytes as needed.

So whether we do that with transmute or with union or by casting pointers and then dereferencing them, it's still a transmutation.

[nnethercote]

I'd take a somewhat more broad view of transmuting

I find it bizarre to say that transmuting covers anything other than mem::transmute (such as casts), and the Reference and the Rustonomicon appear to disagree with your definition. Having an umbrella term ("conversions") and then specific kinds of conversions (coercions, casts, transmute) is more precise and less likely to cause confusion.

But even if we accept your definition, this PR lacks internal consistency in its terminology. Sometimes it talks about casts, sometimes about transmuting, sometimes it mentions both. @ChayimFriedman2, can you talk about your understanding of these words and how you have used them?

[traviscross]

But even if we accept your definition, this PR lacks internal consistency in its terminology...

Agreed definitely that this PR should be more consistent on this point -- I should have mentioned that. So +1 for catching that in review.

I find it bizarre to say that transmuting covers anything other than mem::transmute (such as casts), and the Reference and the Rustonomicon appear to disagree with your definition.

I don't know. I reviewed the Reference text, and I think my definition aligns with it. E.g., it says:

Unions have no notion of an "active field". Instead, every union access transmutes parts of the content of the union to the type of the accessed field. Since transmutes can cause unexpected or undefined behaviour, unsafe is
required to read from a union field.

That's directly in line with my remark that:

So whether we do that with transmute or with union or by casting pointers and then dereferencing them, it's still a transmutation.

Elsewhere, it says:

Despite pointers and references being similar to usizes in the machine code emitted on most platforms, the semantics of transmuting a reference or pointer type to a non-pointer type is currently undecided. Thus, it may not be valid to transmute a pointer or reference type, P, to a [u8; size_of::<P>()].

If we were to narrowly define transmute as "it's not a transmute unless you call mem::transmute", then this paragraph would clearly need to be broadened.

Similarly, in the Nomicon, at the bottom of the page about transmutes, it says:

Also of course you can get all of the functionality of these functions using raw pointer casts or unions, but without any of the lints or other basic sanity checks. Raw pointer casts and unions do not magically avoid the above rules.

If we were to say that it's only a "transmute" if you call mem::transmute, that would seem to render the word largely useless, as we'd nearly always need to qualify it with "or semantically equivalent operations using mem::transmute_copy, union, or pointer casts".