Reject derive inputs that capture generated helpers - #3635
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## Gifjr6pbpu4fh6t7qbzuuqczj3odny3ck #3635 +/- ##
==================================================================
Coverage 91.86% 91.86%
==================================================================
Files 20 20
Lines 6097 6097
==================================================================
Hits 5601 5601
Misses 496 496 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
0724207 to
ac19a3c
Compare
e6bf549 to
dd0d485
Compare
|
@codex review Authored by an AI agent acting on Josh Liebow-Feeser's behalf. |
![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: dd0d485d95
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ac19a3c to
219b88e
Compare
dd0d485 to
4de13f3
Compare
|
@codex review Authored by an AI agent acting on Josh Liebow-Feeser's behalf. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4de13f3 to
9ebc60b
Compare
|
@codex review Authored by an AI agent acting on Josh Liebow-Feeser's behalf. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
| #[doc(hidden)] // `#[macro_export]` bypasses this module's `#[doc(hidden)]`. | ||
| #[macro_export] | ||
| macro_rules! enum_padding { | ||
| (@tag_size, $t:ty, $_align:expr, $packed:expr, $disc_size:expr, $([$($ts:ty),*]),*) => {{ |
There was a problem hiding this comment.
Update the documentation on this macro based on this new branch?
There was a problem hiding this comment.
Updated. The enum_padding! documentation now describes the @tag_size branch, its tag-size expression, and why that expression is evaluated in a lexical scope separate from copied field-type tokens. The generated-side PaddingCheck::Enum documentation was updated consistently. The rebased head also regenerates the affected UI expectations and passes the derive/UI/codegen tests and full pre-push hook.
Authored by an AI agent acting on Josh Liebow-Feeser's behalf.
219b88e to
65f60cb
Compare
9ebc60b to
77afe3a
Compare
|
@codex review Please review exact head Authored by an AI agent acting on Josh Liebow-Feeser's behalf. |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77afe3aa86
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reject_uninspectable_field_types(&ctx.ast.data, derive_name)?; | ||
|
|
||
| let reserved = match &ctx.ast.data { | ||
| Data::Struct(_) | Data::Union(_) => &["___Zc", "ẕ"][..], |
There was a problem hiding this comment.
For a non-generic struct or union derived with FromBytes and on_error = "fail", try_gen_trivial_is_bit_valid emits a validator that references only Self, so user field syntax is never placed inside the scope of ___ZcAlignment; the other field-type copies are in impl-level bounds or associated types outside that method generic. Consequently, a previously valid declaration such as type ___ZcAlignment = u8; #[derive(FromBytes)] struct Packet(___ZcAlignment); is now rejected even though no capture is possible. Apply the ___Zc reservation only when the nontrivial validator actually copies user syntax into that method scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Authored by an AI agent acting on Josh Liebow-Feeser's behalf.
Addressed on exact head 0353a20f43189f535dca715da92fa4848672bd89. The reservation now follows the exact try_gen_trivial_is_bit_valid branch: nongeneric, fail-on-error FromBytes structs, unions, and enums may use ___Zc* names because their validator body mentions only Self. Direct TryFromBytes, FromZeros, generic or skip-mode FromBytes, and generated field-marker ẕ* cases remain rejected. The positive struct/union regression and the compact negative matrix pass on MSRV, stable, and nightly; the repository pre-push hook also passes.
77afe3a to
0353a20
Compare
|
@codex review Please review exact head Authored by an AI agent acting on Josh Liebow-Feeser's behalf. |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0353a20f43
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // The derive target is referenced unqualified inside generated helper | ||
| // scopes, so its name must not be capturable either. | ||
| finder.record(&ctx.ast.ident); |
There was a problem hiding this comment.
___Zc on struct and union names
For a struct or union using the nontrivial validator, this records the derive target itself against the ___Zc method-generic prefix, so a valid declaration such as #[derive(TryFromBytes)] struct ___ZcPacket(bool); is newly rejected. The target name is only used in generated impl headers and field-marker helpers outside is_bit_valid; the validator method refers to it through Self, so ___ZcAlignment cannot capture it. Restrict target-name scanning for this prefix to enum paths that actually copy the target name into the validator scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Authored by an AI agent acting on Josh Liebow-Feeser's behalf.
Addressed on exact head 2b77ff6165d6414c74175ff98fce9b9aff428a62. Target-name scanning now has its own generated-prefix set, separate from copied generics and field types. Struct and union validators refer to the target as Self, so targets named ___Zc* are accepted across direct TryFromBytes, FromZeros, generic FromBytes, and skip-on-error FromBytes; copied syntax still rejects actual method-generic capture. Nonempty field helpers continue to reserve ẕ* targets, zero-field structs reserve no nonexistent field-helper namespace, and full enum validators retain all required target prefixes. Positive and true-collision regressions pass on MSRV, stable, and nightly, and the repository pre-push hook passes.
Derive expansions copy user syntax into generated scopes and refer to helper items by unqualified names. Reject uninspectable syntax, contextual `Self`, and identifiers that could change the meaning of copied fields or generated safety checks. Pin the Syn grammar used by this validation and add cross-toolchain regressions for helper capture, copied macros, qualified paths, and enum tag validation. Closes #3633 *Authored by an AI agent acting on Josh Liebow-Feeser's behalf.* gherrit-pr-id: Gkiogbp7b2w7nxwx27s7vk5wesojtk7bn
0353a20 to
2b77ff6
Compare
|
Authored by an AI agent acting on Josh Liebow-Feeser's behalf. @codex review Please review exact head |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Derive expansions copy user syntax into generated scopes and refer to helper
items by unqualified names. Reject uninspectable syntax, contextual
Self,and identifiers that could change the meaning of copied fields or generated
safety checks.
Pin the Syn grammar used by this validation and add cross-toolchain
regressions for helper capture, copied macros, qualified paths, and enum tag
validation.
Closes #3633
Authored by an AI agent acting on Josh Liebow-Feeser's behalf.
Latest Update: v7 — Compare vs v6
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/Gkiogbp7b2w7nxwx27s7vk5wesojtk7bn && git checkout -b pr-Gkiogbp7b2w7nxwx27s7vk5wesojtk7bn FETCH_HEADCheckout
git fetch origin refs/heads/Gkiogbp7b2w7nxwx27s7vk5wesojtk7bn && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gkiogbp7b2w7nxwx27s7vk5wesojtk7bn && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.