transmute: fix check for whether newtypes have equal size

[RalfJung]

View all comments

The transmute check has some logic to be able to tell whether two types that involve generics will always have the same size. That logic has a bug: it ignores the fact that repr(align) can affect the size of a type. This PR fixes that bug by making the check bail out for most repr flags. That rejects some previously accepted code, hence the @rust-lang/lang nomination.

The new logic says that a type is a transparent newtype wrapper for transmute purposes only if

• It is a struct or single-variant enum without any repr flags except for repr(transparent).
• Or it is a two-variant enum without any repr flags except for repr(transparent) that matches the NPO rules. The inner type must be non_zero: true (which holds for references and NonNull), and the wrapper type is then marked as non_zero: false.

The new check is more conservative than it has to be. We do end up rejecting code like this:

#[repr(C)]
pub struct Wrapper<T>(T);

pub fn foo<T: ?Sized>(x: *const T) -> Wrapper<*const T> {
  unsafe { std::mem::transmute::<*const T, Wrapper<*const T>>(x) }
}

We could refine the logic to understand that repr(C) is fine here, but we have to be careful since repr(C) has to be taken into account by the null-pointer-optimization logic in the transmute check. It seemed easier to just bail out on repr(C) entirely, and crater found no regression.

Fixes #155412
Fixes #88290

I manually checked that the playground example linked in #88290 now emits all three expected errors (and we have equivalent cases in the tests, either preexisting or added by this PR, so it didn't seem worth adding that example as well).

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, types
• compiler, types expanded to 72 candidates
• Random selection from 18 candidates

[RalfJung]

FWIW repr(C) is probably sometimes okay, but this code also handles enums with null pointer optimization and there repr(C) is definitely not okay.

View changes since the review

[RalfJung]

@bors try

[RalfJung]

That's strange, I thought I had fixed this and it worked locally...

[theemathas]

@RalfJung IIRC, the target that failed has -Zrandomize-layout turned on in CI.

Edit: Yup. #t-compiler/help > `Span` is 96 bits?

[rust-bors]

☀️ Try build successful (CI)
Build commit: c9eed2c (c9eed2c4c04c13e48841f9c446608a7322ad16d5, parent: 0204aca0663ec534b773f78639b9ebe20b3da001)

[RalfJung]

Ah! There's a "randomize layout" flag.

That should not affect crater though so we can go ahead and
@craterbot check

[craterbot]

👌 Experiment pr-155418 created and queued.
🤖 Automatically detected try build c9eed2c
🔍 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

[RalfJung]

Actually, there's another flag we should ignore, and it's set on Box, which could affect crater.

@craterbot abort
@bors try

[craterbot]

🗑️ Experiment pr-155418 deleted!

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

[JohnTitor]

@rustbot reroll

[rust-bors]

☀️ Try build successful (CI)
Build commit: ad93f57 (ad93f57d532978d6739002a21bc66b45ed22f998, parent: 1b8f2e46e14b08208a53585570edd9206374aae8)

[RalfJung]

@craterbot check

[craterbot]

👌 Experiment pr-155418 created and queued.
🤖 Automatically detected try build ad93f57
⚠️ Try build based on commit 9d897cb, but latest commit is c634619. Did you forget to make a new try build?
🔍 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-155418 is now running

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

[craterbot]

🎉 Experiment pr-155418 is completed!
📊 1 regressed and 3 fixed (896654 total)
📊 5288 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-155418/retry-regressed-list.txt ↩

[RalfJung]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-155418/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-155418-1 created and queued.
🤖 Automatically detected try build ad93f57
⚠️ Try build based on commit 9d897cb, but latest commit is c634619. Did you forget to make a new try build?
🔍 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

[RalfJung]

Yeah that's entirely spurious. :) Nominating for team discussing.

[RalfJung]

We seem to have absolutely no test for the NPO part of the transmute logic...

EDIT: except for a test recently added by @oli-obk , that I took the liberty to merge into this.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.