make repr_transparent_non_zst_fields a hard error

[RalfJung]

View all comments

This lint is about which fields we consider "trivial" for repr(transparent). For repr(transparent) to be valid, there can be at most one non-trivial field. In other words, trivial fields are those that we promise do not affect the layout or ABI of the repr(transparent) type. Historically we considered all types with size 0 and alignment 1 (i.e., all 1-ZST) to be trivial. However we'd like to take some of that back:

• Types might be 1-ZST today but that's not actually meant to be a semver guarantee, so it's bad for downstream code to rely on it. Therefore we should not accept types that have private fields or that are marked #[non_exhaustive] (except when we are in the same crate as that type).
• Types might be 1-ZST but still be relevant for the ABI because they are repr(C) and who knows what the C ABI does. In particular on MSVC a struct whose only field is a 0-length array has size 1. Rust incorrectly gives it size 0. With repr(ordered_fields) rfcs#3845 we can hopefully fix the layout, which would make "is that type a 1-ZST" target-dependent, and we ideally should reject such code on all targets.

This was a deny-by-default FCW since #147185, which landed almost 6 months ago and shipped with Rust 1.93. (If this PR lands now it will ship with 1.97.) Already back then we found hardly any crater impact. The tracking issue has had no new relevant backrefs since that PR landed.

So, I think it is time to make this a hard error.
Fixes #78586 (tracking issue)
Fixes rust-lang/unsafe-code-guidelines#552 because this means the repr(transparent) ABI compatibility rule no longer ever "ignores" repr(C) fields.

@rust-lang/lang What do you think? See here for the crater analysis; the summary is that there's no relevant regressions found in the wild. But some points have been raised by people:

• The ghost crate offers a macro to define PhantomData-like types, and those types involve a repr(C). If someone uses such a type as a marker inside a repr(transparent), that will no longer work. Apparently nobody does that in the code checked by crater. A new version of the crate has been released that fixes this.
• There should be a way to make a type as "stably a 1-ZST" for repr(transparent) purposes #155925 is unresolved: there is currently no way for a crate to say "yes this type has private field but I promise it will remain a 1-ZST" (except via the unstable #[rustc_pub_transparent]). That means it is not possible for a crate to expose a semantically relevant maker type (like GhostToken) that is "trivial" for repr(transparent) purposes. Apparently currently nobody does this in the ecosystem (the parts crater can see, anyway), but it seems like a sensible thing to do. If we are concerned about this, we could limit this PR to only make the repr(C) and #[non_exhaustive] part of the check a hard error, and leave the "has private fields" part as a warning.

Also note that the private field check is technically a bit odd: we literally check "is the type defined in this crate or are all fields public". We do not check if the current module can access those fields. So if a type has private fields then one can rely on it being "trivial" everywhere in the current crate, even outside the module that defined the type. This is not how field privacy usually works. If we want to restrict this to "only modules that can 'see' the fields are allowed to rely on the type being a 1-ZST", that's technically a breaking change. I don't know if this was a deliberate choice or just the easiest thing to implement. @scottmcm do you remember?

[rustbot]

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
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
• compiler expanded to 69 candidates
• Random selection from 13 candidates

[RalfJung]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6757d70 (6757d700f93f6d16c8b39cf79e96b019bd570e7d, parent: 12f35ad39ed3e39df4d953c46d4f6cc6c82adc96)

[RalfJung]

@craterbot check

[craterbot]

👌 Experiment pr-155299 created and queued.
🤖 Automatically detected try build 6757d70
⚠️ Try build based on commit 5b074cf, but latest commit is 3bb6c15. 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

[jdonszelmann]

This looks nice Ralf, indeed let's nominate for lang after this completes

[CAD97]

Note that this also forbids #[repr(transparent)] pub struct Phantomish<T>(PhantomData<T>) in addition to #[repr(C)] and #[repr(Rust)] 1ZSTs. My crate generativity exposes deliberately-PhantomData-like types with subtle safety invariants.

The custom phantom variance markers in core::marker use the internal #[rustc_pub_transparent] to avoid triggering this lint. There should be some option for custom user types before this becomes a hard error.

See also CAD97/generativity#13 where this was first reported as an issue with my crate. I didn't say anything at that point as I presumed it wouldn't be made into a hard error without some way to opt-in to being a trivial type for #[repr(transparent)]'s purposes.

[RalfJung]

Uh, that's interesting that you would assume that when the error very clearly states what our plans are and nothing in the tracking issue indicates other plans either... why did you wait years until the literal last moment before raising this concern upstream? You could have saved me a bunch of work by posting this any time in the last 3 years. :(

[Jules-Bertholet]

Note that this also forbids #[repr(transparent)] pub struct Phantomish<T>(PhantomData<T>) in addition to #[repr(C)] and #[repr(Rust)] 1ZSTs.

WDYM? This compiles without warning on stable:

use std::marker::PhantomData;

#[repr(transparent)]
pub struct Phantomish<T>(PhantomData<T>);

#[repr(transparent)]
pub struct Transparent<T>(u8, Phantomish<T>);

Edit: ah, I see, you need two crates…

[CAD97]

FWIW I don't think it's necessary to block turning this into a hard error on some way to make 1ZST with private members. I just want to ensure that this knowingly effects #[repr(transparent)] of only PhantomData, and that providing a way of defining such is a known desire.

It's ultimately quite minor.

[RalfJung]

@CAD97 is there an issue tracking that? It'd be nice to have a good self-contained writeup for what you think is currently missing.

[RalfJung]

FWIW I'd also be fine with only making the repr(C) part of this a hard error, and letting the part that is motivated by semver concerns bake for longer. But there hasn't been any movement on that semver side nor even any suggestions for how the rules should be relaxed so that's not very actionable.

Let's see what crater says. Last time we tried this, we found only 2 regressions due to the semver rule; your crate did not show up. So either crater bugged out a bit (spurious failure in the baseline build?) or nothing covered by crater uses your crate with this pattern you mention.

[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.

[RalfJung]

Cc @obi1kenobi as this is about removing a currently-existing semver hazard 🎉

[RalfJung]

@CAD97 I think I finally understood your concern and made an issue: #155925.

[Jules-Bertholet]

Another breakage: the FCW currently triggers when one tries to use a PhantomData-like type defined using the ghost crate as an "extra" field in repr(tranparent).

[RalfJung]

That's exactly #155925 isn't it?

[craterbot]

🚧 Experiment pr-155299 is now running

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

[Jules-Bertholet]

That's exactly #155925 isn't it?

Not just, repr(C) is used as well

[RalfJung]

That sounds like a bug in that crate then, repr(C) cannot be used for types that should be "ignored" by repr(transparent). That's the entire point.

[Jules-Bertholet]

IIUC, ghost is not using #[repr(C)] in the "compatible with C sense", but instead in the "predictable and stable layout" sense. They were actually trying to do the right thing according to our current documentation by not relying on unstable repr(Rust)

[RalfJung]

Yeah, I can see how they got there, but sadly it doesn't work... I filed an issue: dtolnay/ghost#41.

[Jules-Bertholet]

This is why I think we should hold the hard error until the repr(ordered_fields) RFC is merged. Maybe we want only the new repr(C) to actually correspond to C and be a hard error.

[craterbot]

🎉 Experiment pr-155299 is completed!
📊 17 regressed and 0 fixed (892996 total)
📊 5258 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-155299/retry-regressed-list.txt ↩

[RalfJung]

Regression analysis:

• abi_stable-0.9.3 -- macro-generated code inside the #[sabi_trait] attribute, not entirely sure what's happening. But there's a repr(C) type involved. This crate version is ancient, v0.10 has been released over 4 years ago. Shows up in two reverse dependencies: eavesdropper_cli-0.1.0, eavesdropper_pcap_parser-0.2.0. Both of these saw no release in over 4 years.
• 3 GH projects and 7 crate versions fail because they deny the renamed_and_removed_lints lint... 🤷
• The rest is spurious.

[RalfJung]

I updated the PR description and nominated this for lang team discussion.