[derive] Unify handling of #[allow] attributes#2882
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
from
16b4ff3 to
9d48376
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid refactoring that unifies the handling of #[allow] attributes on generated code by introducing a const_block helper function. This improves maintainability by centralizing lint suppression. The changes are consistent and well-executed. I've identified a couple of places where this new pattern could be applied to further improve consistency, specifically in the derive implementations for Hash and Eq.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2882 +/- ##
=======================================
Coverage 91.90% 91.90%
=======================================
Files 20 20
Lines 5878 5878
=======================================
Hits 5402 5402
Misses 476 476 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
from
9d48376 to
83a1309
Compare
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
2 times, most recently
from
97f557a to
185b513
Compare
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
2 times, most recently
from
f2d9ba6 to
80fa702
Compare
|
Sorry to jump in. I noticed this was approved, but validating it against #2880 still gives an error. Threw the code in a repo to make it easier to test: https://github.com/kristof-mattei/zerocopy-non-ascii-error |
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
from
80fa702 to
4463bc6
Compare
Introduce a single `const_block` utility function which generates a
`const _: () = { ... };` block annotated with all relevant
`#[allow(...)]` attributes. Remove `#[allow(...)]` attributes from most
other places in the codebase.
Add `#[allow(non_ascii_idents)]` to this list.
Makes progress on #2880
gherrit-pr-id: G51354cfb7f9717a2e63a11c83c74dacce29366f2
joshlf
force-pushed
the
G51354cfb7f9717a2e63a11c83c74dacce29366f2
branch
from
4463bc6 to
ea01b6d
Compare
|
Updated this so it won't close #2880 when merged. It's still worth merging since it cleans up our handling of |
Linux is built with `-Dnon_ascii_idents`. However, `zerocopy-derive`
uses a non-ASCII character (`ẕ`) internally, which in turn triggers
the lint when attempting to use derives like `FromBytes`:
error: identifier contains non-ASCII characters
--> rust/kernel/lib.rs:153:9
|
153 | a: u32,
| ^
|
= note: requested on the command line with `-D non-ascii-idents`
This was already noticed by another project using
`#![deny(non_ascii_idents)]` [1]. `zerocopy` added an
`#[allow(non_ascii_idents)]` [2], but it does not work since, at the
moment, the `non_ascii_idents` lint is a `crate_level_only` one, and thus
`allow`s only work at the crate root level.
Due to this, an issue about relaxing this restriction was created in
upstream Rust [3]. So far, there have been no replies.
Thus work around it here by using another prefix. The likelihood of a
collision is very small for us, since we control the callers, and this
will hopefully be fixed soon at either the `zerocopy` or the Rust level.
Cc: Joshua Liebow-Feeser <joshlf@google.com>
Cc: Jack Wrenn <jswrenn@amazon.com>
Link: google/zerocopy#2880 [1]
Link: google/zerocopy#2882 [2]
Link: rust-lang/rust#151025 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Linux is built with `-Dnon_ascii_idents`. However, `zerocopy-derive`
uses a non-ASCII character (`ẕ`) internally, which in turn triggers
the lint when attempting to use derives like `FromBytes`:
error: identifier contains non-ASCII characters
--> rust/kernel/lib.rs:153:9
|
153 | a: u32,
| ^
|
= note: requested on the command line with `-D non-ascii-idents`
This was already noticed by another project using
`#![deny(non_ascii_idents)]` [1]. `zerocopy` added an
`#[allow(non_ascii_idents)]` [2], but it does not work since, at the
moment, the `non_ascii_idents` lint is a `crate_level_only` one, and thus
`allow`s only work at the crate root level.
Due to this, an issue about relaxing this restriction was created in
upstream Rust [3]. So far, there have been no replies.
Thus work around it here by using another prefix. The likelihood of a
collision is very small for us, since we control the callers, and this
will hopefully be fixed soon at either the `zerocopy` or the Rust level.
Cc: Joshua Liebow-Feeser <joshlf@google.com>
Cc: Jack Wrenn <jswrenn@amazon.com>
Link: google/zerocopy#2880 [1]
Link: google/zerocopy#2882 [2]
Link: rust-lang/rust#151025 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Linux is built with `-Dnon_ascii_idents`. However, `zerocopy-derive`
uses a non-ASCII character (`ẕ`) internally, which in turn triggers
the lint when attempting to use derives like `FromBytes`:
error: identifier contains non-ASCII characters
--> rust/kernel/lib.rs:153:9
|
153 | a: u32,
| ^
|
= note: requested on the command line with `-D non-ascii-idents`
This was already noticed by another project using
`#![deny(non_ascii_idents)]` [1]. `zerocopy` added an
`#[allow(non_ascii_idents)]` [2], but it does not work since, at the
moment, the `non_ascii_idents` lint is a `crate_level_only` one, and thus
`allow`s only work at the crate root level.
Due to this, an issue about relaxing this restriction was created in
upstream Rust [3]. So far, there have been no replies.
Thus work around it here by using another prefix. The likelihood of a
collision is very small for us, since we control the callers, and this
will hopefully be fixed soon at either the `zerocopy` or the Rust level.
Cc: Joshua Liebow-Feeser <joshlf@google.com>
Cc: Jack Wrenn <jswrenn@google.com>
Link: google/zerocopy#2880 [1]
Link: google/zerocopy#2882 [2]
Link: rust-lang/rust#151025 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Linux is built with `-Dnon_ascii_idents`. However, `zerocopy-derive`
uses a non-ASCII character (`ẕ`) internally, which in turn triggers
the lint when attempting to use derives like `FromBytes`:
error: identifier contains non-ASCII characters
--> rust/kernel/lib.rs:153:9
|
153 | a: u32,
| ^
|
= note: requested on the command line with `-D non-ascii-idents`
This was already noticed by another project using
`#![deny(non_ascii_idents)]` [1]. `zerocopy` added an
`#[allow(non_ascii_idents)]` [2], but it does not work since, at the
moment, the `non_ascii_idents` lint is a `crate_level_only` one, and thus
`allow`s only work at the crate root level.
Due to this, an issue about relaxing this restriction was created in
upstream Rust [3] some months ago.
Thus work around it here by using another prefix. The likelihood of a
collision is very small for us, since we control the callers, and this
will hopefully be fixed soon at either the `zerocopy` or the Rust level.
I filed an issue [4] about it with upstream `zerocopy` as requested
and we discussed this with upstream Rust and `zerocopy`: the Rust issue
got nominated and a PR [5] to relax the restriction was submitted by
Joshua. Upstream `zerocopy` prefers that approach, so if Rust merges it,
then it means we will be able to remove the workaround when we bump the
MSRV, thus likely late 2027, since we follow Debian Stable.
Cc: Joshua Liebow-Feeser <joshlf@google.com>
Cc: Jack Wrenn <jswrenn@google.com>
Link: google/zerocopy#2880 [1]
Link: google/zerocopy#2882 [2]
Link: rust-lang/rust#151025 [3]
Link: google/zerocopy#3427 [4]
Link: rust-lang/rust#157497 [5]
Link: https://patch.msgid.link/20260608141439.182634-16-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Linux is built with `-Dnon_ascii_idents`. However, `zerocopy-derive`
uses a non-ASCII character (`ẕ`) internally, which in turn triggers
the lint when attempting to use derives like `FromBytes`:
error: identifier contains non-ASCII characters
--> rust/kernel/lib.rs:153:9
|
153 | a: u32,
| ^
|
= note: requested on the command line with `-D non-ascii-idents`
This was already noticed by another project using
`#![deny(non_ascii_idents)]` [1]. `zerocopy` added an
`#[allow(non_ascii_idents)]` [2], but it does not work since, at the
moment, the `non_ascii_idents` lint is a `crate_level_only` one, and thus
`allow`s only work at the crate root level.
Due to this, an issue about relaxing this restriction was created in
upstream Rust [3] some months ago.
Thus work around it here by using another prefix. The likelihood of a
collision is very small for us, since we control the callers, and this
will hopefully be fixed soon at either the `zerocopy` or the Rust level.
I filed an issue [4] about it with upstream `zerocopy` as requested
and we discussed this with upstream Rust and `zerocopy`: the Rust issue
got nominated and a PR [5] to relax the restriction was submitted by
Joshua. Upstream `zerocopy` prefers that approach, so if Rust merges it,
then it means we will be able to remove the workaround when we bump the
MSRV, thus likely late 2027, since we follow Debian Stable.
Cc: Joshua Liebow-Feeser <joshlf@google.com>
Cc: Jack Wrenn <jswrenn@google.com>
Link: google/zerocopy#2880 [1]
Link: google/zerocopy#2882 [2]
Link: rust-lang/rust#151025 [3]
Link: google/zerocopy#3427 [4]
Link: rust-lang/rust#157497 [5]
Link: https://patch.msgid.link/20260608141439.182634-16-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
4 participants
Introduce a single
const_blockutility function which generates aconst _: () = { ... };block annotated with all relevant#[allow(...)]attributes. Remove#[allow(...)]attributes from mostother places in the codebase.
Add
#[allow(non_ascii_idents)]to this list.Makes progress on #2880
Latest Update: v9 — Compare vs v8
📚 Full Patch History
Links show the diff between the row version and the column version.