Macros Code Review

SKILL.md

---
name: macros-code-review
description: "Reviews Rust macro code for hygiene issues, fragment misuse, compile-time impact, and procedural macro patterns. Use when reviewing macro_rules! definitions, procedural macros, derive macros, or attribute macros."
---

# 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](../review-verification-protocol/SKILL.md) skill, including **Macro-Specific Verification** for hygiene, fragment type, and proc-macro performance claims. **Then** add findings.

## Output Format

Report findings as:

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

## Quick Reference

| Issue Type | Reference |
|------------|-----------|
| Fragment types, repetition, hygiene boundaries (vars vs types), `$crate` paths, TT-muncher pattern + recursion limits, fragment-matcher follow restrictions, decl-vs-proc decision tree | [references/declarative-macros.md](references/declarative-macros.md) |
| Proc macro types, syn/quote, span hygiene (`call_site` vs `def_site` vs `mixed_site`), `syn::Error::new_spanned` + combine, `parse_quote!` vs `quote!`, `syn` feature audit, trybuild UI tests | [references/procedural-macros.md](references/procedural-macros.md) |

## 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