repr(transparent): do not consider repr(C) types to be 1-ZST

[RalfJung]

Context: rust-lang/unsafe-code-guidelines#552

This experiments with a suggestion by @RustyYato to stop considering repr(C) types as 1-ZST for the purpose of repr(transparent). If we go with rust-lang/rfcs#3845 (or another approach for fixing repr(C)), they will anyway not be ZST on all targets any more, so this removes a portability hazard. Furthermore, zero-sized repr(C) structs may have to be treated as non-ZST for the win64 ABI (at least that's what gcc/clang do), so allowing them to be ignored in repr(transparent) types is not entirely coherent.

Turns out we already have an FCW for repr(transparent), namely #78586. This extends that lint to also check for repr(C).

TODO: update the lint name and wording to account for its extended scope.

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

[RalfJung]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0de49ae (0de49ae9e81f9a1e7df6f0783824ce94ed18e8a9, parent: a2db9280539229a3b8a084a09886670a57bc7e9c)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-147185 created and queued.
🤖 Automatically detected try build 0de49ae
🔍 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-147185 is now running

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

[craterbot]

🎉 Experiment pr-147185 is completed!
📊 8 regressed and 3 fixed (708741 total)
📊 1547 spurious results on the retry-regessed-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-147185/retry-regressed-list.txt ↩

[RalfJung]

This found

• a few cases of abi_stable-0.9.3, an outdated crate version where the lint probably has been fixed in more recent versions (and I highly doubt there's any repr(C) here, this is going to be about the non-exhaustive part of the lint)
• dynamic_graph-0.1.5, a crate that didn't have any updates in 4 years, here, involving a use of generativity::Id. I don't actually understand why this happens as the Id type is repr(transparent) itself... but nothing here is repr(C) so this is also almost certainly about the non-exhaustive part of the lint, not the repr(C) part.

EDIT: It later got confirmed that these crates indeed already trigger the lint before this PR.

[RalfJung]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: e19b5cc (e19b5cc0f024358aaaecf2776a5291fa5a95b623, parent: e2c96cc06bdbdbc6f59c7551194d6a742260d6ff)

[RalfJung]

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

[craterbot]

👌 Experiment pr-147185-1 created and queued.
🤖 Automatically detected try build e19b5cc
🔍 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-147185-1 is now running

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

[craterbot]

🎉 Experiment pr-147185-1 is completed!
📊 6 regressed and 0 fixed (1534 total)
📊 90 spurious results on the retry-regessed-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-147185-1/retry-regressed-list.txt ↩

[RalfJung]

Sorry, I should have marked this as a draft. This also needs t-lang approval and if we are touching this logic anyway I am considering also ruling out uninhabited types.

[RalfJung]

@bors try

[rust-bors]

💔 Test for 507f846 failed: CI. Failed jobs:

• try - dist-x86_64-linux (web logs, extended logs)

[RalfJung]

So... apparently the hyper build for PGO failed. There are half a dozen backtraces but none of them actually says why the build fails. How can one figure out what is going on...?

Let's see if trying again helps. 🤷
@bors try

[rust-bors]

💔 Test for 91f7b9e failed: CI. Failed jobs:

• try - dist-x86_64-linux (web logs, extended logs)

[RalfJung]

I guess hyper has a problematic transparent type somewhere and the PGO build swallows the error message...

[RalfJung]

@bors try

[rust-bors]

💔 Test for 3f96790 failed: CI. Failed jobs:

• try - dist-x86_64-linux (web logs, extended logs)

[RalfJung]

The odd thing is that when I build hyper locally with the rustc from this branch, it just works. Even when I use the specific version and lockfile from the rustc-perf repo.

EDIT: ah, I have to add --all-targets --all-features.

[RalfJung]

Okay I managed to figure out where hyper fails to build -- I spun off discussion of uninhabited types to #147588; this will not be part of this PR.

[RalfJung]

@rust-lang/lang I am proposing we no longer accept repr(C) 1-ZST as being "trivial" for the purpose of repr(transparent). That is, we should reject code like this:

#[repr(C)]
struct ReprCZst([u8; 0]);

#[repr(transparent)]
struct Wrap(ReprCZst, i32);

The reason for this is two-fold:

• repr(C) is an ABI-relevant marker, but the entire point of repr(transparent) is that its ABI ignores the wrapper type and behaves like the wrapped type. So having repr(C) amongst the types that are entirely ignored is not quite coherent. This causes issues because it is unclear how to handle types like #[repr(transparent)] (ReprCZst, ()): should they behave like ReprCZst or like ()? Our repr(transparent) rules pretty much imply that all 1-ZST must have the same ABI, but ReprCZst does not always have the same ABI as (). (The one known exception is win64, where a () return type must behave like a C function with void return type, but a ReprCZst return type has its return value passed by-pointer to match what MSVC would do for a type that looks like ReprCZst. Also see here.)
• If we adopt something like repr(ordered_fields) rfcs#3845, we will eventually fix repr(C) to produce a layout that actually matches what the C compiler for the target would do. This means ReprCZst would not be a 1-ZST everywhere any more; on MSVC targets, it would get size 1. That introduces a portability hazard where Wrap would get accepted on Linux targets but fail to build on Windows targets.

We did a crater run and found 0 cases of code that actually relied on a repr(C) 1-ZST for the purpose of repr(transparent). So, I propose we introduce an FCW for this.

The way this PR implements the FCW is by extending the existing FCW repr_transparent_external_private_fields (#78586). I did it this way because it is quite unclear how to turn this into two separate checks: there are types that only get rejected when we both account for non_exhaustive and repr(C) during the repr(transparent) check. That means the lint should be renamed as the old name no longer makes sense for its extended scope; I would propose repr_transparent_unsuited_zst but haven't yet implemented that part.

So the questions for you are:

• Should we phase out repr(transparent) ignoring repr(C) 1-ZST fields?
• If yes, should we do this as part of the existing repr_transparent_external_private_fields lint (under some new name)? If it should be a new separate lint, please specify how exactly the two lints should behave. Specifically, which lints should fire for a repr(transparent) type that has a 1-ZST non-exhaustive field, a 1-ZST repr(C) field, and a non-ZST field? That type would only get rejected once we make both the non-exhaustive and the repr(C) restriction into hard errors, but we have no good way to fire a lint only if both lints are enabled, and the logic to disentangle these cases would become quite messy. It just doesn't seem worth it for something that so few people will even see.
• What should the new lint (combined or not) be called?
• Should the lint fire in dependencies immediately? Even for the combined lint, crater found only two rarely-used old crates that would trigger it.

[RalfJung]

The previous wording here made no sense -- it said "this enum contains NonExhaustiveEnum", but what it meant was that NonExhaustiveEnum is an enum. The span points at InternalIndirection<NonExhaustiveEnum> which is not an enum!

So I changed it to just "this field contains ...".