Feat/improve bitfields macro#65
Open
sagi21805 wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors the bitfield/flag procedural macro by splitting a monolithic implementation into ChangesBitfields Macro Reorganization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
crates/macros/src/bitfields/bitfield.rs (1)
16-16: ⚡ Quick winReturn a proper error instead of panicking.
Using
.expect()can produce less helpful error messages. Return asyn::Errorinstead to provide better diagnostic information to users.♻️ Proposed refactor
- let name = value.ident.as_ref().expect("Field must have a name"); + let name = value.ident.as_ref().ok_or_else(|| { + syn::Error::new_spanned(value, "Field must have a name") + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/bitfields/bitfield.rs` at line 16, Replace the panic-triggering expect call with a proper syn::Error return: instead of let name = value.ident.as_ref().expect("Field must have a name"); use value.ident.as_ref().ok_or_else(|| syn::Error::new_spanned(&value, "Field must have a name"))? so the function returns a syn::Result and produces a diagnostic tied to the field span; update the surrounding function signature to return syn::Result if necessary.crates/macros/src/bitfields.rs (3)
67-67: 💤 Low valueFix typo: "literly" → "literally".
The comment appears in three places with the same typo.
📝 Proposed fix
- // We literly don't shift + // We literally don't shiftAlso applies to: 132-132, 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/bitfields.rs` at line 67, Replace the typo "literly" with "literally" in the three comment occurrences that read "We literly don't shift" inside crates/macros/src/bitfields.rs; search for that exact comment string (appearing around the bitfield handling code) and update each to "We literally don't shift" so all three instances (the ones you saw at lines ~67, ~132, ~188) are corrected.
227-227: ⚡ Quick winRemove redundant conversion.
The value
vis already converted torepr_tyat line 217, so callingtry_from(v).unwrap()again at line 227 is redundant.♻️ Proposed fix
let cleared = val & !(((1 << `#size`) - 1) << `#offset`); - let new = cleared | ((`#repr_ty`::try_from(v).unwrap() as `#struct_type`) `#shift`); + let new = cleared | ((v as `#struct_type`) `#shift`); core::ptr::write_volatile(addr, new);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/bitfields.rs` at line 227, The assignment building `new` calls `#repr_ty::try_from(v).unwrap()` redundantly even though `v` was already converted to the representation type earlier (at the earlier conversion around line 217); update the `new = cleared | ((`#repr_ty`::try_from(v).unwrap() as `#struct_type`) `#shift`)` expression to use the already-converted value instead of calling `#repr_ty::try_from` again (preserve the same cast to `#struct_type`, combine with `cleared` and `#shift` as before) so symbols involved are `v` (the already-converted repr value), `cleared`, `new`, `#struct_type`, and `#shift`.
98-98: 💤 Low valueFix typo: "convery" → "convert".
The error message appears in two places with the same typo.
📝 Proposed fix
- .expect("Can't convery value 'v' into the struct type"); + .expect("Can't convert value 'v' into the struct type");Also applies to: 219-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/bitfields.rs` at line 98, Correct the typo in the error message used in the .expect(...) calls: replace "Can't convery value 'v' into the struct type" with "Can't convert value 'v' into the struct type" in both occurrences (the two .expect calls that produce this string) so the message reads "Can't convert value 'v' into the struct type".crates/macros/src/bitfields/flag_attr.rs (1)
83-83: ⚡ Quick winReplace magic number with a named constant.
The hardcoded
3matches the number of parsing attempts but makes the code fragile if more options are added.♻️ Proposed refactor
+ const OPTION_COUNT: usize = 3; + while !input.is_empty() { let mut error_count = 0; 'next: { // ... parsing attempts ... } - if error_count == 3 { + if error_count == OPTION_COUNT { let unknown: proc_macro2::TokenTree = input.parse()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/bitfields/flag_attr.rs` at line 83, Replace the magic literal 3 used in the condition "if error_count == 3" with a named constant (e.g., PARSE_ATTEMPT_LIMIT or MAX_PARSE_ATTEMPTS) declared near the top of the module or function so it's easy to update if parsing options change; update the comparison to use that constant (e.g., if error_count == PARSE_ATTEMPT_LIMIT) and ensure any related comments/documentation reference the constant to make the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/macros/src/bitfields.rs`:
- Around line 284-298: The generated clear method incorrectly references an
undefined parameter v; update the implementation of the macro-generated function
(`#vis` fn `#fn_name`(&mut self)) to remove the try_from/ok/expect conversion and
any checks that reference v and instead use the constant `#clear_val` directly
when computing new (i.e., compute cleared and then new = cleared | ((`#clear_val`
as `#struct_type`) `#shift`)); keep the volatile read/write and existing bit math
using `#repr_ty`, `#struct_type`, `#size`, `#offset` and `#shift` but drop the undefined v
and its associated checks.
- Line 139: Remove the stray debug eprintln! call left in the macro code:
eliminate the line that prints ty.path.get_ident() (the eprintln!("\n\n\n {:?}
\n\n\n", ty.path.get_ident());) from crates/macros/src/bitfields.rs so macro
expansion no longer writes to stderr; if you need runtime diagnostics instead,
replace it with a proper logging call (e.g., log::debug) or guard it behind a
debug-only cfg (cfg(debug_assertions)) so production builds are clean.
In `@crates/macros/src/bitfields/flag_attr.rs`:
- Around line 128-136: The error diagnostics use the String `permission` as the
span target, which is invalid; parse the Ident into a variable (e.g. `ident =
input.parse::<Ident>()?`), derive `permission` from that ident (e.g.
`ident.to_string().to_lowercase()`), and then pass the original `ident` to
`syn::Error::new_spanned` (instead of `&permission`) so the error spans the
correct token; update the checks that reference `permission` to use the derived
string but keep `ident` for error reporting.
In `@crates/macros/src/bitfields/utils.rs`:
- Around line 59-64: The error construction in the fallback arm uses
syn::Error::new_spanned(size, ...) but size is a usize and has no span; replace
this by creating the Error with a valid span (e.g.,
syn::Error::new(proc_macro2::Span::call_site(), "Bit width must be between 1 and
128")) or pass a token/AST node that carries span info so the error compiles and
reports a proper location; update the code path in the function in utils.rs
where the match falls through to this arm (the branch returning Err(...)) to use
proc_macro2::Span::call_site() or a nearby token's span instead of size.
---
Nitpick comments:
In `@crates/macros/src/bitfields.rs`:
- Line 67: Replace the typo "literly" with "literally" in the three comment
occurrences that read "We literly don't shift" inside
crates/macros/src/bitfields.rs; search for that exact comment string (appearing
around the bitfield handling code) and update each to "We literally don't shift"
so all three instances (the ones you saw at lines ~67, ~132, ~188) are
corrected.
- Line 227: The assignment building `new` calls `#repr_ty::try_from(v).unwrap()`
redundantly even though `v` was already converted to the representation type
earlier (at the earlier conversion around line 217); update the `new = cleared |
((`#repr_ty`::try_from(v).unwrap() as `#struct_type`) `#shift`)` expression to use the
already-converted value instead of calling `#repr_ty::try_from` again (preserve
the same cast to `#struct_type`, combine with `cleared` and `#shift` as before)
so symbols involved are `v` (the already-converted repr value), `cleared`,
`new`, `#struct_type`, and `#shift`.
- Line 98: Correct the typo in the error message used in the .expect(...) calls:
replace "Can't convery value 'v' into the struct type" with "Can't convert value
'v' into the struct type" in both occurrences (the two .expect calls that
produce this string) so the message reads "Can't convert value 'v' into the
struct type".
In `@crates/macros/src/bitfields/bitfield.rs`:
- Line 16: Replace the panic-triggering expect call with a proper syn::Error
return: instead of let name = value.ident.as_ref().expect("Field must have a
name"); use value.ident.as_ref().ok_or_else(|| syn::Error::new_spanned(&value,
"Field must have a name"))? so the function returns a syn::Result and produces a
diagnostic tied to the field span; update the surrounding function signature to
return syn::Result if necessary.
In `@crates/macros/src/bitfields/flag_attr.rs`:
- Line 83: Replace the magic literal 3 used in the condition "if error_count ==
3" with a named constant (e.g., PARSE_ATTEMPT_LIMIT or MAX_PARSE_ATTEMPTS)
declared near the top of the module or function so it's easy to update if
parsing options change; update the comparison to use that constant (e.g., if
error_count == PARSE_ATTEMPT_LIMIT) and ensure any related
comments/documentation reference the constant to make the intent clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 225b1fcc-8b39-4c1e-bdeb-58e396578d16
📒 Files selected for processing (7)
crates/macros/src/bitfields.rscrates/macros/src/bitfields/bitfield.rscrates/macros/src/bitfields/flag_attr.rscrates/macros/src/bitfields/utils.rscrates/macros/src/bitflags.rscrates/macros/src/lib.rstests/src/main.rs
💤 Files with no reviewable changes (1)
- crates/macros/src/bitflags.rs
| quote! { >> #offset } | ||
| }; | ||
|
|
||
| eprintln!("\n\n\n {:?} \n\n\n", ty.path.get_ident()); |
There was a problem hiding this comment.
Remove debug eprintln! from production code.
This debug statement was left in the code and will print to stderr during macro expansion for every field.
🐛 Proposed fix
- eprintln!("\n\n\n {:?} \n\n\n", ty.path.get_ident());
let fn_name = if ty.path.get_ident().is_some_and(|i| i == "bool") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| eprintln!("\n\n\n {:?} \n\n\n", ty.path.get_ident()); | |
| let fn_name = if ty.path.get_ident().is_some_and(|i| i == "bool") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` at line 139, Remove the stray debug eprintln!
call left in the macro code: eliminate the line that prints ty.path.get_ident()
(the eprintln!("\n\n\n {:?} \n\n\n", ty.path.get_ident());) from
crates/macros/src/bitfields.rs so macro expansion no longer writes to stderr; if
you need runtime diagnostics instead, replace it with a proper logging call
(e.g., log::debug) or guard it behind a debug-only cfg (cfg(debug_assertions))
so production builds are clean.
Comment on lines
+284
to
+298
| #vis fn #fn_name(&mut self) { | ||
| let v = #repr_ty::try_from(v) | ||
| .ok() | ||
| .expect("Can't convery value 'v' into the struct type"); | ||
|
|
||
| #checks | ||
|
|
||
| unsafe { | ||
| let addr = self as *const _ as *mut #struct_type; | ||
| let val = core::ptr::read_volatile(addr); | ||
| let cleared = val & !(((1 << #size) - 1) << #offset); | ||
| let new = cleared | ((#clear_val as #struct_type) #shift); | ||
| core::ptr::write_volatile(addr, new); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix undefined variable reference.
The fn_clear method references a variable v at lines 285-287 and 289, but the function signature on line 284 doesn't define any parameter named v. The clear function should use the clear_val constant directly without attempting conversions or checks on an undefined variable.
🐛 Proposed fix
quote! {
#(`#doc_attrs`)*
#[inline]
`#vis` fn `#fn_name`(&mut self) {
- let v = `#repr_ty`::try_from(v)
- .ok()
- .expect("Can't convery value 'v' into the struct type");
-
- `#checks`
-
unsafe {
let addr = self as *const _ as *mut `#struct_type`;
let val = core::ptr::read_volatile(addr);
let cleared = val & !(((1 << `#size`) - 1) << `#offset`);
let new = cleared | ((`#clear_val` as `#struct_type`) `#shift`);
core::ptr::write_volatile(addr, new);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #vis fn #fn_name(&mut self) { | |
| let v = #repr_ty::try_from(v) | |
| .ok() | |
| .expect("Can't convery value 'v' into the struct type"); | |
| #checks | |
| unsafe { | |
| let addr = self as *const _ as *mut #struct_type; | |
| let val = core::ptr::read_volatile(addr); | |
| let cleared = val & !(((1 << #size) - 1) << #offset); | |
| let new = cleared | ((#clear_val as #struct_type) #shift); | |
| core::ptr::write_volatile(addr, new); | |
| } | |
| } | |
| `#vis` fn `#fn_name`(&mut self) { | |
| unsafe { | |
| let addr = self as *const _ as *mut `#struct_type`; | |
| let val = core::ptr::read_volatile(addr); | |
| let cleared = val & !(((1 << `#size`) - 1) << `#offset`); | |
| let new = cleared | ((`#clear_val` as `#struct_type`) `#shift`); | |
| core::ptr::write_volatile(addr, new); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` around lines 284 - 298, The generated clear
method incorrectly references an undefined parameter v; update the
implementation of the macro-generated function (`#vis` fn `#fn_name`(&mut self)) to
remove the try_from/ok/expect conversion and any checks that reference v and
instead use the constant `#clear_val` directly when computing new (i.e., compute
cleared and then new = cleared | ((`#clear_val` as `#struct_type`) `#shift`)); keep
the volatile read/write and existing bit math using `#repr_ty`, `#struct_type`,
`#size`, `#offset` and `#shift` but drop the undefined v and its associated checks.
Comment on lines
+128
to
+136
| let permission = | ||
| input.parse::<Ident>()?.to_string().to_lowercase(); | ||
|
|
||
| if !permission.chars().all(|c| matches!(c, 'r' | 'w' | 'c')) { | ||
| return Err(syn::Error::new_spanned( | ||
| &permission, | ||
| "expected permission string (e.g. `rw`, `r`, `wc(0)`)", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Fix error span to use the original token.
The permission variable is a String, which doesn't implement the required traits for syn::Error::new_spanned. This will produce poor error spans. Use the original Ident instead to provide accurate source location information.
🔧 Proposed fix
- let permission =
- input.parse::<Ident>()?.to_string().to_lowercase();
+ let permission_ident = input.parse::<Ident>()?;
+ let permission = permission_ident.to_string().to_lowercase();
if !permission.chars().all(|c| matches!(c, 'r' | 'w' | 'c')) {
return Err(syn::Error::new_spanned(
- &permission,
+ &permission_ident,
"expected permission string (e.g. `rw`, `r`, `wc(0)`)",
));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let permission = | |
| input.parse::<Ident>()?.to_string().to_lowercase(); | |
| if !permission.chars().all(|c| matches!(c, 'r' | 'w' | 'c')) { | |
| return Err(syn::Error::new_spanned( | |
| &permission, | |
| "expected permission string (e.g. `rw`, `r`, `wc(0)`)", | |
| )); | |
| } | |
| let permission_ident = input.parse::<Ident>()?; | |
| let permission = permission_ident.to_string().to_lowercase(); | |
| if !permission.chars().all(|c| matches!(c, 'r' | 'w' | 'c')) { | |
| return Err(syn::Error::new_spanned( | |
| &permission_ident, | |
| "expected permission string (e.g. `rw`, `r`, `wc(0)`)", | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields/flag_attr.rs` around lines 128 - 136, The error
diagnostics use the String `permission` as the span target, which is invalid;
parse the Ident into a variable (e.g. `ident = input.parse::<Ident>()?`), derive
`permission` from that ident (e.g. `ident.to_string().to_lowercase()`), and then
pass the original `ident` to `syn::Error::new_spanned` (instead of
`&permission`) so the error spans the correct token; update the checks that
reference `permission` to use the derived string but keep `ident` for error
reporting.
Comment on lines
+59
to
+64
| _ => { | ||
| return Err(syn::Error::new_spanned( | ||
| size, | ||
| "Bit width must be between 1 and 128", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Fix error construction to use a valid span.
The code calls syn::Error::new_spanned(size, ...) where size is a usize, which doesn't implement the required traits for span-based errors. This will likely fail to compile or produce incorrect error locations. Use syn::Error::new(proc_macro2::Span::call_site(), ...) instead, or pass a token/type that has span information.
🔧 Proposed fix
_ => {
- return Err(syn::Error::new_spanned(
- size,
+ Err(syn::Error::new(
+ proc_macro2::Span::call_site(),
"Bit width must be between 1 and 128",
- ));
+ ))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ => { | |
| return Err(syn::Error::new_spanned( | |
| size, | |
| "Bit width must be between 1 and 128", | |
| )); | |
| } | |
| _ => { | |
| Err(syn::Error::new( | |
| proc_macro2::Span::call_site(), | |
| "Bit width must be between 1 and 128", | |
| )) | |
| } |
🧰 Tools
🪛 Clippy (1.95.0)
[warning] 60-60: unneeded return statement
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields/utils.rs` around lines 59 - 64, The error
construction in the fallback arm uses syn::Error::new_spanned(size, ...) but
size is a usize and has no span; replace this by creating the Error with a valid
span (e.g., syn::Error::new(proc_macro2::Span::call_site(), "Bit width must be
between 1 and 128")) or pass a token/AST node that carries span info so the
error compiles and reports a proper location; update the code path in the
function in utils.rs where the match falls through to this arm (the branch
returning Err(...)) to use proc_macro2::Span::call_site() or a nearby token's
span instead of size.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/macros/src/bitfields.rs`:
- Around line 153-159: The generated extraction expression uses ((1 << `#size`) -
1) which overflows for full-width fields; replace that inline expression with
the precomputed mask token (e.g. `#mask`) everywhere it appears (notably in the
expression passed to `#ty`::try_from and the other two occurrences referenced) so
the emitted code uses (val & (`#mask` << `#offset`)) `#shift` as `#repr_ty` before
calling `#ty`::try_from(...). Ensure all spots that currently emit ((1 << `#size`) -
1) use the same `#mask` token so full-width B64/B128 fields compile correctly.
- Around line 74-100: The generated setter (const fn `#name`) currently only uses
debug_assert! checks (in the `checks` block) so out-of-range values can still
spill in release; fix by masking the incoming `v` to the field width before
applying/OR-ing it and keep or keep only debug asserts for diagnostics.
Concretely: compute a width mask like `let mask = ((1usize << `#size`) - 1) as
`#repr_ty`;` then if `attr.dont_shift` mask with `v & (mask << `#offset`)`, else
mask then shift with `((v & mask) << `#offset`)` and use that masked value in the
write; you may keep the existing debug_assert! checks in `checks` but ensure the
actual value used is the masked one so oversized inputs cannot escape in
release.
- Around line 92-103: The generated setter (the const fn named by `#name` in the
quote! block) currently does self.0 |= ... which leaves previous bits intact;
change it to clear the target field bits first then OR the new value.
Concretely: compute the field mask from the field width, shift it by the same
shift used in the current expression, AND self.0 with the inverse of that
shifted mask to zero out the field, then OR in the new value cast as
`#struct_type` and shifted as before; keep the existing `#repr_ty`::try_from(v)
check and `#checks` intact and perform the clear-before-OR in the same quote!
block where self.0 is updated.
- Around line 306-324: Filter out write-only fields when building field_args
(only include fields that have a readable accessor) and stop wrapping the getter
result in `#ty`::try_from(...) so the Debug output shows the field value directly;
specifically, in the mapping over self.fields replace the current unconditional
construction with a filter that skips fields lacking a getter (i.e., write-only
ones), compute the accessor name the same way you already do (the getter
variable using f.ty.size and f.attr.flag_type to choose is_*/get_*), and emit
quote! { stringify!(`#name`), self.#getter() } (or formatted value as needed)
instead of quote! { stringify!(`#name`), &#ty::try_from(self.#getter()) } so
debug_impl compiles for structs with write-only fields and prints raw field
values.
In `@tests/src/main.rs`:
- Line 15: The local variable `nested` is created but never used (from the
expression `Nested::new().a(true).b(Test::SomeRandomName)`), triggering a Clippy
unused-binding warning; to fix, change the binding to an underscore-prefixed
name (e.g., `_nested`) or use `let _ = ...` so the macro-generated API is
exercised without leaving an unused variable; update the `let nested =
Nested::new().a(true).b(Test::SomeRandomName);` statement accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f3cec71-ffce-468a-809e-7f6264d469a4
📒 Files selected for processing (4)
crates/macros/src/bitfields.rscrates/macros/src/bitfields/bitfield.rscrates/macros/src/bitfields/utils.rstests/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/macros/src/bitfields/bitfield.rs
Comment on lines
+74
to
+100
| let mut checks = quote! { | ||
| debug_assert!( | ||
| ( | ||
| (v as #struct_type) >> #offset | ||
| ) < (1 << #size) as #struct_type, | ||
| "Value is too large for this bitfield" | ||
| ); | ||
| }; | ||
|
|
||
| if attr.dont_shift { | ||
| checks.extend(quote! { | ||
| debug_assert!( | ||
| v & !((((1 << #size) - 1) as #struct_type) << #offset) == 0, | ||
| "Value overrides flags on positions that are not in bounds of flag", | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| quote! { | ||
| #(#doc_attrs)* | ||
| #[inline] | ||
| #vis const fn #name(mut self, v: #ty) -> Self { | ||
| let v = #repr_ty::try_from(v) | ||
| .ok() | ||
| .expect("Can't convery value 'v' into the struct type"); | ||
|
|
||
| #checks |
There was a problem hiding this comment.
Reject or mask oversized inputs in generated setters/builders.
Line 77 and Line 200 validate width only with debug_assert!, so release builds still accept out-of-range values and the unmasked write can spill into adjacent fields. The current >> #offset`` comparison also under-validates the normal shifted case because v has not been shifted yet.
🔧 Suggested direction
- let mut checks = quote! {
- debug_assert!(
- (
- (v as `#struct_type`) >> `#offset`
- ) < (1 << `#size`) as `#struct_type`,
- "Value is too large for this bitfield"
- );
- };
+ let mut checks = if attr.dont_shift {
+ quote! {
+ assert!(
+ ((v as `#struct_type`) >> `#offset`) < ((1 as `#struct_type`) << `#size`),
+ "Value is too large for this bitfield"
+ );
+ }
+ } else {
+ quote! {
+ assert!(
+ (v as `#struct_type`) < ((1 as `#struct_type`) << `#size`),
+ "Value is too large for this bitfield"
+ );
+ }
+ };You can also mask the value before the final OR so invalid inputs cannot escape the field even in release.
Also applies to: 197-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` around lines 74 - 100, The generated setter
(const fn `#name`) currently only uses debug_assert! checks (in the `checks`
block) so out-of-range values can still spill in release; fix by masking the
incoming `v` to the field width before applying/OR-ing it and keep or keep only
debug asserts for diagnostics. Concretely: compute a width mask like `let mask =
((1usize << `#size`) - 1) as `#repr_ty`;` then if `attr.dont_shift` mask with `v &
(mask << `#offset`)`, else mask then shift with `((v & mask) << `#offset`)` and use
that masked value in the write; you may keep the existing debug_assert! checks
in `checks` but ensure the actual value used is the masked one so oversized
inputs cannot escape in release.
Comment on lines
+92
to
+103
| quote! { | ||
| #(#doc_attrs)* | ||
| #[inline] | ||
| #vis const fn #name(mut self, v: #ty) -> Self { | ||
| let v = #repr_ty::try_from(v) | ||
| .ok() | ||
| .expect("Can't convery value 'v' into the struct type"); | ||
|
|
||
| #checks | ||
|
|
||
| self.0 |= (v as #struct_type) #shift; | ||
| self |
There was a problem hiding this comment.
Clear the field before OR-ing in the builder.
fn_build currently does self.0 |= ..., so calling the same builder twice or starting from a non-zero value keeps stale bits from the previous field value instead of replacing them.
🔧 Suggested direction
- self.0 |= (v as `#struct_type`) `#shift`;
+ let field_mask = (((1 as `#struct_type`) << `#size`) - 1) << `#offset`;
+ self.0 = (self.0 & !field_mask) | (((v as `#struct_type`) `#shift`) & field_mask);
self🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` around lines 92 - 103, The generated setter
(the const fn named by `#name` in the quote! block) currently does self.0 |= ...
which leaves previous bits intact; change it to clear the target field bits
first then OR the new value. Concretely: compute the field mask from the field
width, shift it by the same shift used in the current expression, AND self.0
with the inverse of that shifted mask to zero out the field, then OR in the new
value cast as `#struct_type` and shifted as before; keep the existing
`#repr_ty`::try_from(v) check and `#checks` intact and perform the clear-before-OR
in the same quote! block where self.0 is updated.
Comment on lines
+153
to
+159
| #ty::try_from( | ||
| ( | ||
| ( | ||
| val & (((1 << #size) - 1) << #offset) | ||
| ) #shift | ||
| ) as #repr_ty | ||
| ).expect("Cannot convert bit representation into the given type") |
There was a problem hiding this comment.
Handle full-width fields without 1 << #size``.
Expressions like ((1 << #size) - 1) overflow when the field width matches the backing type width, so valid inputs such as a full-width B64/B128 field will generate code that fails to compile.
🔧 Suggested direction
+ let full_mask = if `#size` == core::mem::size_of::<#struct_type>() * 8 {
+ quote!(`#struct_type`::MAX)
+ } else {
+ quote!(((1 as `#struct_type`) << `#size`) - 1)
+ };Use that precomputed mask token everywhere the generated code currently emits ((1 << #size) - 1).
Also applies to: 228-229, 296-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` around lines 153 - 159, The generated
extraction expression uses ((1 << `#size`) - 1) which overflows for full-width
fields; replace that inline expression with the precomputed mask token (e.g.
`#mask`) everywhere it appears (notably in the expression passed to `#ty`::try_from
and the other two occurrences referenced) so the emitted code uses (val & (`#mask`
<< `#offset`)) `#shift` as `#repr_ty` before calling `#ty`::try_from(...). Ensure all
spots that currently emit ((1 << `#size`) - 1) use the same `#mask` token so
full-width B64/B128 fields compile correctly.
Comment on lines
+306
to
+324
| let field_args: Vec<_> = self | ||
| .fields | ||
| .iter() | ||
| .map(|f| { | ||
| let getter = if f.ty.size == 1 | ||
| && f.attr.flag_type | ||
| .as_ref() | ||
| .is_some_and(|t| t.path | ||
| .get_ident() | ||
| .is_some_and(|i| i == "bool")) | ||
| { | ||
| format_ident!("is_{}", f.name) | ||
| } else { | ||
| format_ident!("get_{}", f.name) | ||
| }; | ||
| let name = f.name; | ||
| let ty = f.attr.flag_type.as_ref().unwrap_or(&f.ty.repr_ty); | ||
| quote! { stringify!(#name), &#ty::try_from(self.#getter()) } | ||
| }) |
There was a problem hiding this comment.
Generate Debug output only from readable fields, and print the value directly.
debug_impl always calls get_*/is_*, so any write-only field makes the generated Debug impl fail to compile. It also wraps each getter in #ty::try_from(...), which prints Ok(...) instead of the field value.
🔧 Suggested direction
let field_args: Vec<_> = self
.fields
.iter()
+ .filter(|f| f.attr.permissions.read)
.map(|f| {
let getter = if f.ty.size == 1
&& f.attr.flag_type
.as_ref()
.is_some_and(|t| t.path
.get_ident()
.is_some_and(|i| i == "bool"))
{
format_ident!("is_{}", f.name)
} else {
format_ident!("get_{}", f.name)
};
let name = f.name;
- let ty = f.attr.flag_type.as_ref().unwrap_or(&f.ty.repr_ty);
- quote! { stringify!(`#name`), &#ty::try_from(self.#getter()) }
+ quote! { stringify!(`#name`), &self.#getter() }
})
.collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let field_args: Vec<_> = self | |
| .fields | |
| .iter() | |
| .map(|f| { | |
| let getter = if f.ty.size == 1 | |
| && f.attr.flag_type | |
| .as_ref() | |
| .is_some_and(|t| t.path | |
| .get_ident() | |
| .is_some_and(|i| i == "bool")) | |
| { | |
| format_ident!("is_{}", f.name) | |
| } else { | |
| format_ident!("get_{}", f.name) | |
| }; | |
| let name = f.name; | |
| let ty = f.attr.flag_type.as_ref().unwrap_or(&f.ty.repr_ty); | |
| quote! { stringify!(#name), &#ty::try_from(self.#getter()) } | |
| }) | |
| let field_args: Vec<_> = self | |
| .fields | |
| .iter() | |
| .filter(|f| f.attr.permissions.read) | |
| .map(|f| { | |
| let getter = if f.ty.size == 1 | |
| && f.attr.flag_type | |
| .as_ref() | |
| .is_some_and(|t| t.path | |
| .get_ident() | |
| .is_some_and(|i| i == "bool")) | |
| { | |
| format_ident!("is_{}", f.name) | |
| } else { | |
| format_ident!("get_{}", f.name) | |
| }; | |
| let name = f.name; | |
| quote! { stringify!(`#name`), &self.#getter() } | |
| }) | |
| .collect(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/bitfields.rs` around lines 306 - 324, Filter out write-only
fields when building field_args (only include fields that have a readable
accessor) and stop wrapping the getter result in `#ty`::try_from(...) so the Debug
output shows the field value directly; specifically, in the mapping over
self.fields replace the current unconditional construction with a filter that
skips fields lacking a getter (i.e., write-only ones), compute the accessor name
the same way you already do (the getter variable using f.ty.size and
f.attr.flag_type to choose is_*/get_*), and emit quote! { stringify!(`#name`),
self.#getter() } (or formatted value as needed) instead of quote! {
stringify!(`#name`), &#ty::try_from(self.#getter()) } so debug_impl compiles for
structs with write-only fields and prints raw field values.
|
|
||
| let mut f = MyFlags::new(); | ||
| let nested = Nested::new().a().b(Test::SomeRandomName); | ||
| let nested = Nested::new().a(true).b(Test::SomeRandomName); |
There was a problem hiding this comment.
Resolve unused nested binding.
Line 15 creates nested but never uses it, which matches the Clippy warning. If this is only to exercise macro-generated API, bind it as _nested (or let _ = ...) to keep warnings clean.
Suggested fix
- let nested = Nested::new().a(true).b(Test::SomeRandomName);
+ let _nested = Nested::new().a(true).b(Test::SomeRandomName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let nested = Nested::new().a(true).b(Test::SomeRandomName); | |
| let _nested = Nested::new().a(true).b(Test::SomeRandomName); |
🧰 Tools
🪛 Clippy (1.95.0)
[warning] 15-15: unused variable
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/main.rs` at line 15, The local variable `nested` is created but
never used (from the expression
`Nested::new().a(true).b(Test::SomeRandomName)`), triggering a Clippy
unused-binding warning; to fix, change the binding to an underscore-prefixed
name (e.g., `_nested`) or use `let _ = ...` so the macro-generated API is
exercised without leaving an unused variable; update the `let nested =
Nested::new().a(true).b(Test::SomeRandomName);` statement accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests