repr(ordered_fields)#3845
Conversation
|
Not to add too many extra colours to the list, but (Note: those three things should cover every case I've seen that uses Whereas Also, while it may be more technical than most users need to understand, it would be helpful if the RFC reiterated the current issues with |
|
Just as a small point of style the Guide Level Explanation is usually "what would be written in the rust tutorial book", and the Reference Level Explanation is "what could be written into the Rust Reference". This isn't a strict requirement, but personally I'd like to see the Reference Level part written out. Using the present tense, as if the RFC was accepted and implemented. |
add more unresolved questions
Rework struct layout description.
| # Guide-level explanation | ||
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| `repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in memory layout. |
There was a problem hiding this comment.
| `repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in memory layout. | |
| `repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in-memory layout. |
"cross-platform" -- the layout will differ when there are different layouts for struct members' types, in particular primitive types can have different alignments which changes the amount of padding.
e.g., #[repr(ordered_fields)] struct S(u8, f64); doesn't have the same layout on x86_64 and i686
There was a problem hiding this comment.
Good point, this will need to be documented as a hazard in the ordered_fields docs. However, the repr itself will be cross-platform. For example, #[repr(ordered_fields)] struct Cross([u8; 3], SomeEnum); will be truly cross-platform (given that SomeEnum is!).
Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
Just voicing support for |
|
Nominating this so that we can do a preliminary vibe-check on it in a lang triage meeting. |
| Currently `repr(C)` serves two roles | ||
| 1. Provide a consistent, cross-platform, predictable layout for a given type | ||
| 2. Match the target C compiler's struct/union layout algorithm and ABI |
There was a problem hiding this comment.
Big fan of doing this split, especially for structs. (It's less obvious what choices to make for other things, IMHO, but at least for structs this is something I've wanted for ages, so that for example Layout::extend can talk about it instead of C.)
Pondering the bikeshed: declaration_order or something could also be used to directly say what you're getting.
(This could be contrasted with other potential reprs that I wouldn't expect this RFC to add, but could consider as future work, like a deterministic_by_size_and_alignment where some restricted set of optimizations are allowed but you can be sure that usize and NonNull<String> can be mixed between different types while still getting the "same" field offsets, for example.)
There was a problem hiding this comment.
I think this is also useful for unions, so we don't need to rely on repr(C) to ensure that all fields of a union are at offset 0.
This could be contrasted with other potential reprs that I wouldn't expect this RFC to add...
This also works as an argument against names like repr(consistent), since there are multiple consistent and useful repr, making it not descriptive enough.
There was a problem hiding this comment.
I do think that declaration_order or ordered_fields is a bit weird on a union, because of course they're not really in any "order".
It makes me ponder whether we should just have repr(offset_zero) for unions to be explicit about it, or something.
(Which makes me think of other things like addressing rust-lang/unsafe-code-guidelines#494 by having a different constructs for "bag of maybeuninit stuff that overlap" vs "distinct options with an active-variant rule for enum-but-without-stored-discriminant". But those are definitely not this RFC.)
There was a problem hiding this comment.
I don't mind spelling it as repr(offset_zero) for unions if that helps get this RFC accepted 😄. However, I have a sneaking suspicion that this isn't the contentious part of this RFC.
I know the name isn't optimal (intentionally). This can be hashed out after the RFC is accepted (or even give a different name for all of struct, union, and enum).
The most important bit for me is just that we do the split (for all of struct, union, and enum, to be consistent).
|
I've updated how enums's tags are specified, now they just defer to whatever |
|
|
||
| `repr(C)` in edition <= 2024 is an alias for `repr(ordered_fields)` and in all other editions, it matches the default C compiler for the given target for structs, unions, and field-less enums. Enums with fields will be laid out as if they are a union of structs with the corresponding fields. | ||
|
|
||
| Using `repr(C)` in editions <= 2024 triggers a lint to use `repr(ordered_fields)` as a future compatibility lint with a machine-applicable fix. If you are using `repr(C)` for FFI, then you may silence this lint. If you are using `repr(C)` for anything else, please switch over to `repr(ordered_fields)` so updating to future editions doesn't change the meaning of your code. |
There was a problem hiding this comment.
I think this is too noisy. Most code out there using repr(C) is probably fine - IIUC, if you're not targeting Windows or AIX, maybe definitely fine? - and having a bunch of allow(...) across a bunch of projects seems unfortunate.
Maybe we can either (a) only enable the lint for migration, i.e., the next edition's cargo fix would add allows for you or (b) we find some new name... C2 for the existing repr(C) usage to avoid allows. But (b) also seems too noisy to me.
There was a problem hiding this comment.
Maybe it could just be an optional edition compatibility lint, so if someone enables e.g. rust_20xx_compatibility it shows up but otherwise not.
There was a problem hiding this comment.
(a) only enable the lint for migration
That was the intention, hence the name edition_2024_repr_c. I'll make this more clear, that this is intended to be a migration lint.
Rustfix would update to #[repr(ordered_fields)] to preserve the current behavior. For the FFI crates, #![allow(edition_2024_repr_c)] at the top of lib.rs would suffice. If you have a mix of FFI and non-FFI uses of repr(C), then you'll have to do the work to figure out which is which, no matter what option is chosen to update repr(C) - even adding repr(C2), since then the FFI use case would need to update all their reprs to repr(C2).
Overall, I think this scheme only significantly burdens those who have a mix of FFI and non-FFI uses of repr(C). But they were going to be burdened no matter what option was chosen.
There was a problem hiding this comment.
Is the new wording/lints too noisy still?
There was a problem hiding this comment.
I think the new wording is still too noisy. We shouldn't assume that most people using repr(C) are using it for ordering rather than FFI.
There was a problem hiding this comment.
That wasn't my intention, but I don't see another way to do all of the following in the next edition:
- make
repr(C)mean - same layout/ABI as what the standard C compiler does - make
repr(ordered_fields)- the same algorithm that's listed forrepr(C)in the Rust reference - ensure that everyone who upgrades to the next edition gets the layout they need (as long as they read the warnings and follow the given advice)
- make it as painless as possible for people who don't mix FFI and stable ordering cases (which I suspect is the vast majority of people). In other words, each crate currently uses
repr(C)either exclusively for FFI or exclusively for some stable layout. - for people who do mix FFI and stable ordering cases in one crate, at least the warning should give them all the places they need to double-check, and they can silence the warning on a case-by-case basis.
I'm open to suggestions on how to handle the diagnostics. Within these constraints, I think my solution is the only real option we have. If there are some objections to these constraints, I would like to hear those too, maybe I missed the mark with these constraints, and missed a potential solution because of it.
There was a problem hiding this comment.
@DemiMarie said this in a comment:
Would it be possible to rename repr(C) to something else? That way repr(C) becomes a compile-time error rather than silently changing behavior.
I laid out my design axioms above. I believe that making a new repr and deprecating repr(C) is too costly. Especially when the most prevalent use-case of repr(C) is for FFI. A use-case which would benefit from bug fixes that could happen after this RFC.
|
What is the plan for when is depending on the exact details of C struct layout? Should one add explicit compile-time assertions in this case? |
|
I'm not sure what you are asking. If you are asking, when is it ok to depend on the layout being compatible with the target's |
There are cases where one needs to not only be compatible with the target C compiler, but also make assumptions about what that layout is. For instance, that is needed when a protocol is defined by a C struct. A target with a different C struct layout could not be supported. In this case, could one use |
|
If you want to make assumptions about the C layout you'll have to talk to the vendor responsible for that. I don't know what kind of an answer you are expecting here, Rust obviously can't make any promises. Obviously you can use all the usual Rust ways of inspecting the layout, just like for |
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
…fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang#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](rust-lang#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang#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](rust-lang#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang#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?
Rollup merge of #155299 - RalfJung:repr_transparent_non_zst_fields, r=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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](#78586) 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](#155299 (comment)) 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`](https://github.com/dtolnay/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. - #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?
…=jdonszelmann make repr_transparent_non_zst_fields a hard error 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 rust-lang/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 rust-lang/rust#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](rust-lang/rust#78586) has had no new relevant backrefs since that PR landed. So, I think it is time to make this a hard error. Fixes rust-lang/rust#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](rust-lang/rust#155299 (comment)) 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`](https://github.com/dtolnay/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. - rust-lang/rust#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?
|
The RFC should be clarified to indicate how each kind of zero-sized type within a A zero-sized #[repr(C#editionNext)]
struct Slice<'a, T> {
p: *const T,
len: usize,
_marker: PhantomData<&'a ()> // zero-sized `repr(Rust)` type
}An // Whether this is a ZST depends on the target, right?
// (The C compiler may pad a struct that only contains
// ZSTs so that it has at least size 1, as there are no ZSTs
// in ISO C.)
#[repr(C#editionNext)]
struct PhantomCNext<T>(PhantomData<T>);
#[repr(C#editionNext)]
struct ProbablyNotWhatYouWantedSlice<'a, T> {
p: *const T,
len: usize,
_marker: PhantomCNext <&'a ()> // not necessarily zero-sized.
}Here is another thing I have seen in real-world code. This does what one expects with #[repr(C#editionCurr)]
struct AtLeastNCurr<T, const N: usize> {
// `hd` is guaranteed to have size `size_of::<T>() * N`.
hd: [T; N],
tl: [T],
}
#[repr(C#editionNext)]
struct AtLeastNNextBroken<T, const N: usize> {
// `hd` is guaranteed to have size `size_of::<T>() * N` ***EXCEPT*** when `N` is zero.
hd: [T; N],
tl: [T],
} |
|
Those are good points. I'm not sure we should allow unsized types in
Yes, this is a good point. I'll make a note that 1-ZST fields are ignored for the purpose of layout in
Yes, this is surprising, but unavoidable. |
I am not sure that "1-ZST" is the right term to use. Is |
|
With rust-lang/rust#157973, you could say "fields whose types have trivial ABI". |
|
The reason I'm hesitant is I don't want to overpromise anything for
I think you meant
It is the easiest case, since they never have an effect on layout (in all versions of Rust up to now). Since they are zero sized and have the minimal alignment, it is always safe to ignore them for the purpose of layout. A more aligned ZST is complicated, since they likely should have an effect on the offsets of later fields, but since I'm really not sure how to handle them in non-trivial cases. #[repr(C#editionNext)]
struct Foo {
a: u8,
b: [u64; 0],
c: u8,
}What offset should
That would probably be better. |
|
Note that a zero-length array field is the current recommendation for representing C flexible array members in Rust. https://rust-lang.github.io/rust-bindgen/using-fam.html |
|
Just to clarify: How zero-length array fields affect layout in But, most (all?) other 1-ZST types should be ignored. But, again |
There could be a carve out for that use-case, given the recommendation. But I'm still wary of adding more guarantees in general.
Yes, I never proposed banning them outright, esp. since they are used so heavily on the Rust side of FFI (like in flexible array members and And as the RFC specifies,
Yes, after adding that 1-ZSTs (or types have trivial ABI) are ignored, then this just follows from the RFC. |
|
Consider: #[repr(transparent)]
struct ZeroBytesTransparent([u8; 0]);
#[repr(C#editionNext)]
struct Foo {
a: u8,
b: ZeroBytesTransparent,
c: u8,
}What is the layout of the above, when the C ABI gives zero-length arrays non-zero length? That is, do the field layout rules consider Consider: struct ZeroBytesRust([u8; 0]);
#[repr(C#editionNext)]
struct Foo {
a: u8,
b: ZeroBytesRust,
c: u8,
}I argue here that field |
|
rust-lang/rust#157973 accounts for all of that. |
I was expecting from the RFC that in the new |
|
No. See https://rust-lang.github.io/rust-bindgen/using-fam.html#using-dynamically-sized-types, for example.
How they are handled in Rust would be determined by the ABI / C compiler, right? The issue isn't with C compilers that treat them the same, but where C compilers might not treat them the same. For example, a C compiler could flat-out reject |
| * field-less `enum`s as an implementation defined integer | ||
| * general `enum`s as a struct of a tag and a union. Where each field of the union is a struct containing each field of the variants. (NOTE: this is how they are handled in `repr(C)` in current editions) | ||
|
|
||
| `repr(C#editionNext)` will be defined as the same as what the `C` compiler of the given target would do (both in terms of layout and calling convention). |
There was a problem hiding this comment.
It seems like the intended process for this RFC is to have the RFC make the minimal changes that will allow #[repr(ordered_fields)] to be added, while everything about #[repr(C#editionNext)] would be specified externally. But, as comments showed, there are still some universal things that need to be specified for #[repr(C#editionNext)]:
- Is a
#[repr(C#editionCurr)]field allowed in a#[repr(C#editionNext)]type? - Now to handle non-array
#[repr(Rust)]ZST fields in a#[repr(C#editionNext)]type? - What is the syntactic mapping between Rust and C for flexible array members?
Is it desired to resolve these issues in this RFC, or is there another place (where) to have that discussion?
There was a problem hiding this comment.
I understand wanting to specify as much as possible, especially since FFI is such a tricky topic already.
Is a
#[repr(C#editionCurr)]field allowed in a#[repr(C#editionNext)]type?
Yes, and in cases where they agree there will be no issues. In cases where they disagree...
Now to handle non-array#[repr(Rust)] ZST fields in a #[repr(C#editionNext)] type?
If they are 1-ZSTs they can be ignored entirely, if they have a non-trivial alignment, then this can be treated like a T field[0] field in C (provided T as the same alignment as the ZST. I think these two should be equivalent on any reasonable C compiler (and all current C compilers). But correct me if I'm wrong.
But I don't think we need to specify non-trivial aligned ZSTs. Since they don't exists in C. If there is sufficient motivation for them, they can be specified later.
What is the syntactic mapping between Rust and C for flexible array members?
I think following current recommendations is probably best, so a trailing field of type [T; 0]. I understand that you like the idea of using [T], but slices are very different from flexible length array members. Slices have a well defined length, and can only be pointed to by a fat pointer. Flexible length arrays have an unknown length, and all pointers to then are thin (since all pointers are thin in C). These differences are significant enough, that I don't think we should conflate the two.
It may be a good idea to introduce a unsized type specifically for flexible length array members, but that's a different discussion entirely. For this RFC, following current norms is sufficient.
No. See https://rust-lang.github.io/rust-bindgen/using-fam.html#using-dynamically-sized-types, for example.
I don't think this says what you think it says. It's still using [T; 0] to represent flexible length array members for FFI, but trying to provide a cleaner interface when you are on the Rust side.
That page doesn't contradict anything I've written?
If the C compiler simply rejects one of the two, we can just match the one it doesn't reject. There's only an problem if it accepts both but with different layout/ABI. And in that case, it's almost certainly just a bug in the C compiler, like the Clang issue I linked where it doesn't match GCC. Yes, a hypothetical C compiler could theoretically choose to deliberately distinguish them, but there's a lot of cursed stuff that a sufficiently adversarial C compiler could do (e.g. make |
I can't make sense of this -- one cannot even pass a |
View all comments
Add
repr(ordered_fields)and provide a migration path to switch users fromrepr(C)torepr(ordered_fields), then change the meaning ofrepr(C)in the next edition.This RFC is meant to be an MVP, and any extensions (for example, adding more
reprs) are not in scope. This is done to make it as easy as possible to accept this RFC and make progress on the issue ofrepr(C)serving two opposing roles.Rendered
To avoid endless bikeshedding, I'll make a poll if this RFC is accepted with all the potential names for the new
repr. If you have a new name, I'll add it to the list of names in the unresolved questions section, and will include it in the poll.