Allow #[derive(IntoBytes)] to ignore unused const generic parameters - #3685
barunaniket wants to merge 2 commits into
Conversation
Previously, any generic parameter on a struct -- including a const
parameter that doesn't appear in any field's type, and thus cannot
affect layout -- forced all field types to implement Unaligned. This
made it impossible to derive IntoBytes on, e.g., a repr(C) struct with
an unused const generic tag, even though nothing about its layout
actually depends on that parameter.
Detect structs whose only generic parameters are const parameters not
referenced by any field, and for those, prove the absence of padding
via PaddingCheck::ReprCStruct rather than requiring Unaligned fields.
Struct::Struct's own padding check can't be reused here: its
struct_padding! expansion computes size_of::<Self>() inside an
anonymous const, and Self is generic once const parameters are
present (e.g. Foo<{ N }>), which rustc rejects ("generic `Self` types
are currently not permitted in anonymous constants").
ReprCStruct's repr_c_struct_has_padding! sidesteps this: its expansion
never references its $t argument, only the (concrete) field types, so
Self only appears as a standalone generic argument outside of the
anonymous const, which is permitted. This does mean the fix is
currently limited to repr(C) structs; repr(Rust) structs with unused
const generics still require Unaligned fields.
Fixes google#2723
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3685 +/- ##
=======================================
Coverage 91.90% 91.90%
=======================================
Files 20 20
Lines 6175 6179 +4
=======================================
+ Hits 5675 5679 +4
Misses 500 500 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eca67f9f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| fn contains_ident(tokens: TokenStream, ident: &Ident) -> bool { | ||
| tokens.into_iter().any(|tt| match tt { | ||
| TokenTree::Ident(i) => i == *ident, |
There was a problem hiding this comment.
When a const parameter is declared with raw spelling but referenced ordinarily, as in struct Foo<const r#N: usize>([u8; N], u8), proc_macro2::Ident equality does not consider r#N and N equal. This therefore misclassifies N as unused and emits the anonymous-const padding check, which stable Rust rejects because that check now contains the generic N; the same derive compiled against the parent revision. Compare normalized spellings, for example with the existing to_ident_str helper, rather than using i == *ident.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c48a9c7 — compare identifiers via to_ident_str instead of raw Ident equality, so r#N and N are treated as the same parameter.
| !ast.generics | ||
| .const_params() | ||
| .any(|param| fields.iter().any(|(_, _, ty)| contains_ident(quote!(#ty), ¶m.ident))) |
There was a problem hiding this comment.
A field type macro can reference the surrounding const parameter without placing its identifier in the derive input, for example macro_rules! field { () => { [u8; N] } } followed by struct Foo<const N: usize>(field!(), u8). This scan classifies that struct as having an unused N, but the macro subsequently expands inside repr_c_struct_has_padding!, where stable Rust rejects the generic parameter in the anonymous const; the parent revision instead emitted Unaligned bounds and compiled this input. Conservatively return false whenever a field type contains a macro invocation, including nested invocations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c48a9c7 — the token scan now conservatively treats any macro invocation (any ! token) in a field's type as risky, falling back to the Unaligned bounds.
| // | ||
| // This requires `repr(C)`, which `repr_c_struct_has_padding!` | ||
| // requires of its input. | ||
| (Some(PaddingCheck::ReprCStruct), false, None) |
There was a problem hiding this comment.
For an unused const parameter on a struct such as #[repr(C)] struct Foo<const N: usize>(u8, str), this branch replaces the previous valid IntoBytes + Unaligned bounds with repr_c_struct_has_padding!. That macro only recognizes syntactic slice fields as unsized, so it sends str, a slice alias, or a parenthesized slice to DstLayout::for_unpadded_type<T>(), whose implicit T: Sized bound makes the derive fail; the same str example compiles against the parent revision. Retain the old Unaligned fallback for potentially unsized trailing types or extend the padding check to obtain their DST layout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c48a9c7 — added a guard that bails out of the new branch when the trailing field is str. Noted in the comment that this doesn't cover every unsized type hiding behind a non-slice-syntax alias (a narrower, pre-existing limitation shared with the no-generics case's is_syntactic_dst check), which is now a known, documented gap rather than a silent compile failure.
| // `PaddingCheck::ReprCStruct`'s `repr_c_struct_has_padding!` | ||
| // expansion sidesteps this: it never mentions `$t` in its body, only | ||
| // the (concrete) field types and the `align`/`packed` literals. Its |
There was a problem hiding this comment.
A field type can contain Self without spelling any const parameter, for example #[repr(C)] struct Foo<const N: usize>(PhantomData<Self>, u8). The predicate classifies N as unused, but copying PhantomData<Self> into the padding macro places the generic Self inside the anonymous const and stable Rust rejects the derive; this example compiled with the parent revision's Unaligned bounds. Treat any Self reference in a field type as preventing this optimization and fall back to the previous bounds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c48a9c7 — the token scan now also bails out when a field's type contains Self, since Self is generic over all of the struct's parameters regardless of whether it spells any of their identifiers.
| } else { | ||
| (Some(PaddingCheck::Struct), false, None) | ||
| } | ||
| } else if is_c && generics_are_unused_consts(&ctx.ast, &strct.fields()) { |
There was a problem hiding this comment.
The public IntoBytes derive analysis in zerocopy/src/lib.rs still says that a generic repr(C) struct outside the homogeneous case requires every field to implement Unaligned, which is directly contradicted by this new branch and its u32 regression test. Add the unused-const exception to that analysis so users do not rely on obsolete guidance when interpreting derive failures.
AGENTS.md reference: zerocopy/AGENTS.md:L37-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c48a9c7 — added a bullet to the IntoBytes derive's Analysis section documenting the new exception.
Four edge cases in the unused-const-generic-parameters detection could misclassify a parameter as unused, causing the derive to attempt the new padding check and fail to compile (previously-working code would stop compiling, though never unsoundly): - A const parameter declared with raw syntax (e.g. `r#N`) wasn't recognized as the same identifier as an ordinary reference to it (`N`) in a field type, since token equality doesn't normalize raw identifiers. Compare via the existing `to_ident_str` helper instead. - A field mentioning `Self` implicitly depends on all of the struct's generic parameters (Self is the struct at the same arguments as the impl), even if it doesn't spell any of their identifiers. - A field type built by a macro invocation could expand to something that depends on a const parameter without that parameter's identifier appearing anywhere in the derive input's tokens. - A trailing field written as `str` (or any other unsized type not spelled with literal `[T]` syntax) isn't recognized by repr_c_struct_has_padding!'s slice-detecting macro arm, so it falls to a helper with an implicit `Sized` bound that `str` can't satisfy. This is a narrower, pre-existing limitation of that macro (shared with the no-generics case's `is_syntactic_dst` check) that the new branch would otherwise have newly exposed for generic structs. Fold the first three checks into one conservative token scan and gate the fourth at the call site, since it's about field shape rather than the generic parameters themselves. Also note the new exception in the IntoBytes derive's "Analysis" doc section, which still described the pre-existing Unaligned-for-all-fields requirement unconditionally. Add a regression test for each case, confirming the derive falls back to requiring `Unaligned` fields rather than attempting (and failing) the new padding check.
|
@joshlf whenever you have time please review it and then merge it. |
This branch has not been deployed
2 participants
Summary
Fixes #2723. Previously, deriving
IntoByteson a struct with any generic parameters -- including aconstparameter that doesn't appear in any field's type, and thus can't affect layout -- forced every field type to implementUnaligned. This made code like the issue's repro impossible:This detects structs whose only generic parameters are
constparameters that don't appear anywhere in any field's type (conservatively, by token identity -- see the doc comment ongenerics_are_unused_constsfor the shadowing edge case this over-approximates), and for those, proves the absence of padding viaPaddingCheck::ReprCStructinstead of falling back to requiringUnalignedfields.Why not just treat it like the no-generics case?
That was my first attempt, and it compiles the derive-macro-output-comparison tests fine, but fails on an actual real-world instantiation.
PaddingCheck::Struct'sstruct_padding!expansion computessize_of::<Self>()inside an anonymousconst(thePADDING_BYTESargument toPaddingFree<Self, { .. }>). Once the impl has aconstgeneric parameter,Selfis generic (e.g.Foo<{ N }>), and rustc rejects that:PaddingCheck::ReprCStruct'srepr_c_struct_has_padding!sidesteps this because its expansion never references its$targument at all -- only the (concrete) field types and thealign/packedliterals go into the anonymous const.Selfonly appears as a standalone generic argument toDynamicPaddingFree, outside of the anonymous const, which rustc permits.One limitation as a result: this currently only covers
repr(C)structs, sincerepr_c_struct_has_padding!requires that repr.repr(Rust)structs with unused const generics still requireUnalignedfields (same as before this change). Happy to discuss whether that's worth lifting in a follow-up if there's a similar trick available, but didn't want to hold up the common case on it.Test plan
test_into_bytes_struct_unused_const_generic(derive-output snapshot test) covering the issue's exact shapetest_into_bytes_struct_used_const_generic(derive-output snapshot test) as a negative case, confirming a used const param still requiresUnaligned(guards the "is it actually unused" detection)test_into_bytes_unused_const_generic_deriveinzerocopy's own test suite -- an actual compiling+running end-to-end test (cargo test --features derive), not just a token-stream comparison, since the snapshot tests alone wouldn't have caught the anonymous-const rustc limitation abovecargo test -p zerocopy-derive --lib-- full suite passes (36 tests)cargo test -p zerocopy --lib --features derive-- full suite passes (117 tests)cargo clippy -p zerocopy-derive --lib -- -D warnings-- cleanrustfmt +nightly --checkon all touched files -- cleantests/ui/*.rscompile-fail fixture exercisesIntoBytes+ const generics, so nothing regresses there