Repository navigation
repr(transparent) accepts too many types as ZST (repr_transparent_non_zst_fields, formerly repr_transparent_external_private_fields) #78586
Description
This tracks eventually removing support for repr(transparent) types ignoring ZST that might not actually remain ZST in the future or on other targets.
Types that can gain more fields
In crate A:
#[non_exhaustive]
pub struct UnitType {}In crate B:
#[repr(transparent)]
pub struct TransparentThing(i32, UnitType);I expected that to fail in crate B, because the reason crate A put #[non_exhaustive] on the type was to allow it to get additional fields in the future, which would no longer allow it to work in repr(transparent).
But this actually compiles today, creating what I would consider an accidental semver hazard.
repr(C) types
#[repr(C)]
struct CZst([u8; 0]);
#[repr(transparent)]
pub struct TransparentThing(i32, CZst);This should fail since CZst actually is not a ZST for all C compilers, and since the type can be relevant for the ABI.
Open questions
Reacted by Kornel
Reacted by Esper Thomson
Activity
Mark-Simulacrum commentedon Oct 30, 2020
I'm going to nominate for lang, because I think (perhaps with a future compat lint) we should not look at foreign types' size unless that type is repr(transparent) for the purposes of checking here.
scottmcm commentedon Nov 2, 2020
Note that this also applies to the way this was done before
non_exhaustive:As that private field also keeps others from constructing or exhaustively matching it, but it would still be accepted as 1-ZST by
repr(transparent).We discussed this in the lang team meeting today. The consensus was that this is an accidental semver hazard that we'd like to do something about.
@rustbot modify labels: +E-help-wanted +E-medium
It'd be great to get a PR we could use to determine the impact of this. Ideally this isn't happening in the wild and can just be an error (maybe for one case or the other, if one is common and the other isn't) but it'll probably need at least a future imcompatibility lint.
jsomedon commentedon Dec 6, 2020
Hello folks, can I take this issue? I am kinda new to the community but I want to start contributing. :-)
scottmcm commentedon Dec 7, 2020
@jsomedon Absolutely! Unfortunately I don't really know the code here, so don't have suggestions for where to start, but you could always try tracing through the code around
non_exhaustiveand dropping by zulip if you run into any trouble.jsomedon commentedon Dec 8, 2020
Should a
non-exhaustivestruct be fine to stay inrepr(transparent)in its own crate? (My understanding is that it's okay? As you can do whatever u want with thenon-exhaustivein its own defining crate?)scottmcm commentedon Dec 8, 2020
@jsomedon Yes, I think that's ok, like how one can exhaustively match it inside its own defining crate.
horse-princess commentedon Jan 23, 2021
@jsomedon do you plan to continue working on this?
64 remaining items
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_…
Unrolled build for #155299
Rollup merge of #155299 - RalfJung:repr_transparent_non_zst_fields, r…
Lint `repr_transparent_non_zst_fields` has been removed
Rollup merge of rust-lang#157973 - Jules-Bertholet:distinguish-c-zst-…
Unrolled build for #157973 in rollup 163680
Rollup merge of #157973 - Jules-Bertholet:distinguish-c-zst-docs, r=R…
Rollup merge of #157973 - Jules-Bertholet:distinguish-c-zst-docs, r=R…
Rollup merge of #157973 - Jules-Bertholet:distinguish-c-zst-docs, r=R…