Macros Code Review

Macros Code Review

Review Workflow

1. Check Cargo.toml -- Note Rust edition (2024 reserves gen keyword, affecting macro output), proc-macro crate dependencies (syn, quote, proc-macro2), and feature flags (e.g., syn with minimal features)
2. Check macro type -- Determine if reviewing declarative (macro_rules!), function-like proc macro, attribute macro, or derive macro
3. Check if a macro is needed -- If the transformation is type-based, generics are better. Macros are for structural/repetitive code generation that generics cannot express
4. Scan macro definitions -- Read full macro bodies including all match arms, not just the invocation site
5. Check each category -- Work through the checklist below, loading references as needed
6. Gates -- Complete Gates below before reporting; do not substitute informal “I verified.”

Gates (before reporting findings)

Complete in order. Do not emit findings until Gate 4 passes for each issue.

Gate 1 — Crate context (on disk)
PASS when: You opened the reviewed crate’s Cargo.toml (workspace member path if applicable) and recorded Rust edition, whether the crate is proc-macro = true, and relevant proc-macro dependencies or syn / quote feature flags.
Blocks rationalization: Edition 2024 findings (gen, unsafe extern, generated unsafe bodies) and syn “full” vs minimal flags require this — do not flag edition-specific macro output without matching edition from the file.

Gate 2 — Macro definitions read
PASS when: For every macro you critique, you read the full definition (all macro_rules! arms, or the proc-macro entry plus helpers you rely on), not only call sites or partial expansions.
Artifact: At least one path per macro to the defining .rs file(s) you used.

Gate 3 — Per-finding evidence
PASS when: Each planned issue has [FILE:LINE] from the current tree for the macro definition, attribute/derive site, or generated code location you are discussing (not from memory, docs-only, or another branch).

Gate 4 — Pre-report protocol
PASS when: You loaded and applied the review-verification-protocol skill, including Macro-Specific Verification for hygiene, fragment type, and proc-macro performance claims. Then add findings.

Output Format

Report findings as:

[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.

Quick Reference

Review Checklist

Declarative Macros (macro_rules!)

• Correct fragment types used (:expr vs :tt vs :ident -- wrong choice causes unexpected parsing)
• Repetition separators match intended syntax (, vs ; vs none, * vs +)
• Trailing comma/semicolon handled (add $(,)? or $(;)? at end of repetition)
• Matchers ordered from most specific to least specific (first match wins)
• No ambiguous expansions -- each metavariable appears in the correct repetition depth in the transcriber
• Variables defined in the macro use macro-internal names (hygiene protects variables, not types/modules/functions)
• Exported macros (#[macro_export]) use $crate:: for crate-internal paths, never crate:: or self::
• Standard library paths use ::core:: and ::alloc:: (not ::std::) for no_std compatibility
• compile_error! used for meaningful error messages on invalid input patterns
• Macro placement respects textual scoping (defined before use) unless #[macro_export]

Procedural Macros

• syn features minimized (don't enable full when derive suffices -- reduces compile time)
• Spans propagated from input tokens to output tokens (errors point to user code, not macro internals)
• Span::mixed_site() is the default for introduced helper variables — call_site only when intentionally pointing at user code; def_site is nightly-only on proc_macro::Span
• Error reporting uses syn::Error::new_spanned(node, msg) with the offending AST node, never panic!
• Multiple errors collected and reported together via syn::Error::combine (good UX vs first-failure)
• parse_quote! (not quote!) used when the result needs to be a syn::T for further AST manipulation; syn::parse2(quote!{...}) for fallible cases
• Public types re-exported from proc_macro2, not proc_macro (compatibility with downstream consumers)
• proc-macro2 used for testing (testable outside of compiler context)
• Generated code volume is proportionate -- proc macros that emit large amounts of code bloat compile times

Derive Macros

• Derivation is obvious -- a developer could guess what it does from the trait name alone
• Helper attributes (#[serde(skip)] style) are documented
• Trait implementation is correct for all variant shapes (unit, tuple, struct variants)
• Generated impl blocks use fully qualified paths (::core::, $crate::)

Attribute Macros

• Input item is preserved or intentionally transformed (not accidentally dropped)
• Attribute arguments are validated with clear error messages
• Test generation patterns (#[test_case] style) produce unique test names
• Framework annotations document what code they generate

Edition 2024 Awareness

• Macro output does not use gen as an identifier (reserved keyword -- use r#gen or rename)
• Generated unsafe fn bodies use explicit unsafe {} blocks around unsafe ops
• Generated extern blocks use unsafe extern

Generics vs Macros

Flag a macro when the same result is achievable with generics or trait bounds. Macros are appropriate when:

• The generated code varies structurally (not just by type)
• Repetitive trait impls for many concrete types
• Test batteries with configuration variants
• Compile-time computation that const fn cannot express

Severity Calibration

Critical (Block Merge)

• Macro generates unsound unsafe code
• Hygiene violation in macro that outputs unsafe blocks (caller's variables leak into unsafe context)
• Proc macro panics instead of returning compile_error! (crashes the compiler)
• Derive macro generates incorrect trait implementation (violates trait contract)

Major (Should Fix)

• Exported macro uses crate:: or self:: instead of $crate:: (breaks for downstream users)
• Exported macro uses ::std:: instead of ::core::/::alloc:: (breaks no_std users)
• Wrong fragment type causing unexpected parsing (:expr where :tt needed, or vice versa)
• Proc macro enables syn full features unnecessarily (compile time cost)
• Missing span propagation (errors point to macro definition, not invocation)
• No error handling in proc macro (panics on bad input instead of compile_error!)

Minor (Consider Fixing)

• Missing trailing comma/semicolon tolerance in repetition patterns
• Matcher arms not ordered most-specific-first
• Macro used where generics would be clearer and equally expressive
• Missing compile_error! fallback arm for invalid patterns
• Helper attributes undocumented

Informational (Note Only)

• Suggestions to split complex macro_rules! into a proc macro
• Suggestions to reduce generated code volume
• TT munching or push-down accumulation patterns that could be simplified

Valid Patterns (Do NOT Flag)

• macro_rules! for test batteries -- Generating repetitive test modules from a list of types/configs
• macro_rules! for trait impls -- Implementing a trait for many concrete types with identical bodies
• TT munching -- Valid advanced pattern for recursive token processing
• Push-down accumulation -- Valid pattern for building output incrementally across recursive calls
• #[macro_export] with $crate -- Correct way to make macros usable outside the defining crate
• Span::call_site() for generated functions -- Intentionally making generated items visible to callers
• syn::Error::to_compile_error() -- Correct error reporting pattern in proc macros
• trybuild tests for proc macros -- Standard compile-fail testing approach
• Attribute macros on test functions -- Common pattern for test setup/teardown
• compile_error! in impossible match arms -- Good practice for catching invalid macro input