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?

[RalfJung]

That actually seems like a pretty large number of spurious results? Not sure what is "normal" these days.
@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-155299/retry-regressed-list.txt p=1

[craterbot]

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

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

[craterbot]

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

[RalfJung]

Nothing new there :)

[traviscross]

This makes sense. @RalfJung, you also raise some good points, which we discussed in the lang call today, about what we should keep in mind for future work, in terms of 1) allowing people to stably commit to a type being a 1-ZST while having private fields and 2) clawing back the space needed to make field privacy work in the expected way here.

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• ensure-we-dont-break-generativity (make repr_transparent_non_zst_fields a hard error #155299 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

I'm not too worried about there being too much breakage around the repr(C) structs because we've had a deny-by-default warning for a long time, and it's hard to see how someone would be relying on this.

For the crate-level publicity thing, summarizing my understanding: We need need a way to publicly say that a struct will always be a 1-ZST before we could realistically introduce a module-level check here, because 1-ZSTness is a useful property to depend on within a crate even if a struct has private fields.

[joshtriplett]

👍 for this change.

I would also be enthusiastic about having a stable version of rustc_pub_transparent, e.g. repr(pub transparent). But I don't think we need to block on that.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[traviscross]

Thanks @CAD97 for raising the matter about generativity and @aapoalas for raising this in the lang meeting. Let's confirm we're not breaking that or use of it.

@rfcbot concern ensure-we-dont-break-generativity

[RalfJung]

Thanks @CAD97 for #155299 (comment) the matter about generativity and @aapoalas for raising this in the lang meeting. Let's confirm we're not breaking that or use of it.

That concern is exactly #155925. I am confused by the mixed messaging here regarding that issue.^^

[traviscross]

That concern is exactly #155925. I am confused by the mixed messaging here regarding that issue.

Fair enough — you're right it wouldn't make any sense without the context of the lang call. We were going through this somewhat quickly as we had many items to catch up on. We didn't see the generativity connection initially. We saw that this would be missing some expressivity; that didn't seem to break anything, and it seemed we could add it later. I proposed FCP merge and people checked boxes. Then, as we were moving on to the next item, we noticed that @aapoalas had pointed out in our chat that it might break generativity. We weren't immediately sure, without checking, whether there were any viable options for generativity to achieve what it needs to do if we were to make this a hard error (looking at #155925 now, would the workaround proposed by @hanna-kruppe work for it?). That seemed the sort of thing we should be sure about, so we talked about it briefly and I filed a concern so we could look into this.

What do you think? How much does this concern you? And what are your thoughts on @Jules-Bertholet's point, which I read as, essentially, that today's repr(C) is repr(ordered_fields)/repr(linear), and so if our plan is to add a new repr(C) that really means "the C representation", then maybe we shouldn't give repr(C) the full set of C representation drawbacks until then. (I suppose it might depend a bit on the migration strategy we end up adopting.)

[RalfJung]

Regarding the ordered_fields point, my thinking is that it'd be nice to be able to do this step first. It'll make the later migration easier since otherwise switching from repr(C) to repr(C#2027) might break downstream crates.

Regarding generativity (we're talking about the crate, not the concept -- that confused me when first reading the messages above), crater found no such use of these types. But if we're worried about closing that door / code not covered by crater, I can make this PR only turn the repr(C) and #[non_exhaustive] parts into a hard error and leave the "private fieds" part as a warning, indefinitely blocked until someone designs and implements a solution for #155925. Your call.