Skip to content

feat(rules): add macro correctness checks to Rust review#475

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
sisantaChhatoi:rules/rust-macro-checks
Jul 25, 2026
Merged

feat(rules): add macro correctness checks to Rust review#475
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
sisantaChhatoi:rules/rust-macro-checks

Conversation

@sisantaChhatoi

@sisantaChhatoi sisantaChhatoi commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What & why

internal/config/rules/rule_docs/rust.md is otherwise thorough — ownership/lifetimes, unsafe boundaries, async cancellation, atomics ordering — but has no coverage of macros, a high-bug-density corner of Rust. This adds a scoped #### Macros and Metaprogramming section for the well-known macro_rules! / procedural-macro footguns:

  • an $x:expr fragment interpolated more than once → the caller's expression (and its side effects) runs multiple times
  • exported macros referencing items without $crate:: → name resolution breaks when invoked from another crate
  • token-tree ($t:tt) fragments re-emitted without parentheses → operator precedence can silently change meaning
  • procedural macros that panic!/unwrap() on malformed input instead of emitting syn::Error / compile_error!
  • hygiene assumptions that break: call-site name capture, or collisions when a macro is invoked more than once

The section is guarded — "only flag when the diff actually defines a macro_rules! or procedural macro" — mirroring how the existing async/concurrency sections gate themselves, so it preserves the doc's precision-over-recall bias and does not fire on ordinary macro invocations.

No wiring changes: **/*.rs already maps to rust.md in system_rules.json, and .rs is already in supported_file_types.json.

Why review has to catch these (the compiler doesn't)

Verified against rustc:

  • Double evaluation — compiles clean; the side effect simply runs twice at runtime. No diagnostic.
  • Missing $crate:: — the defining crate compiles with no warning at all; a consumer crate then fails with error[E0433]: failed to resolve: use of unresolved module or unlinked crate. The breakage is displaced into someone else's build, which is exactly why it survives local CI.

Does it change reviewer behavior? (indicative A/B eval)

The section was injected via the existing --rule merge mechanism as the only variable between arms, same model on both arms, against a fixture of planted macro defects:

Model Baseline (stock rust.md) With this section
Claude Haiku 4.5 0 / 6 runs flagged 5 / 6 runs flagged
Claude Opus 4.6 1 / 4 runs flagged 3 / 3 runs flagged

Small N — indicative, not statistical.

Disclosure: the fixture originally planted three defects, but follow-up verification with rustc showed one of them (operator precedence on an :expr expansion) is not a real Rust defect — macro expansion is AST-level, so :expr fragments and whole expansions are atomic. That rule bullet has been narrowed to tt-fragment interpolation (see the comment below), and the eval's signal should be read as resting on the two independently verified defects above.

go test ./internal/config/rules/... passes.

@CLAassistant

CLAassistant commented Jul 23, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: No supported files changed.

@sisantaChhatoi
sisantaChhatoi force-pushed the rules/rust-macro-checks branch from c125b41 to 917aa41 Compare July 23, 2026 20:42
@lizhengfeng101
lizhengfeng101 requested a review from MuoDoo July 24, 2026 00:35
@lizhengfeng101

Copy link
Copy Markdown
Collaborator

cc @MuoDoo

@sisantaChhatoi
sisantaChhatoi force-pushed the rules/rust-macro-checks branch from 917aa41 to 857a0f5 Compare July 24, 2026 03:01
@sisantaChhatoi

Copy link
Copy Markdown
Contributor Author

Self-review follow-up — correcting one item in this PR.

While verifying each rule item against rustc, I found that my original precedence bullet imported C-preprocessor semantics that don't apply to Rust. macro_rules! expands at the AST level, so an :expr fragment and the expansion as a whole are atomic:

macro_rules! add    { ($a:expr, $b:expr) => { $a + $b }; }
macro_rules! square { ($x:expr)          => { $x * $x }; }

add!(1, 2) * 3   // == 9, not 7
square!(1 + 1)   // == 4, not 3

Precedence leaking is only reachable through token-tree interpolation:

macro_rules! dbl_tt { ($($t:tt)*) => { $($t)* * 2 }; }
dbl_tt!(1 + 1)   // == 3  <- leaks

I've narrowed that bullet to $t:tt fragments and pushed the correction, and added a disclosure to the description since the original eval fixture included the now-retracted case.

The other two items are verified as genuinely compiler-invisible:

  • double evaluation — compiles clean, side effect runs twice at runtime;
  • missing $crate:: — the defining crate compiles with no warning, while a consumer crate fails with error[E0433]: failed to resolve: use of unresolved module or unlinked crate. Since the failure lands in a downstream build, local CI stays green — which is precisely the case review needs to cover.

Happy to adjust scope or wording further.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Rust macro correctness guidance to the system Rust review rules documentation, covering common macro_rules! and procedural-macro footguns to improve defect detection during reviews.

Changes:

  • Introduces a new “Macros and Metaprogramming” section with checks for double-evaluation, $crate:: pathing, tt precedence hazards, proc-macro error reporting, and hygiene/collision pitfalls.
  • Adds a guard instruction to only flag these items when the diff defines a macro_rules! or procedural macro (not for ordinary macro invocations).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/config/rules/rule_docs/rust.md Outdated
Only flag when the diff actually defines a `macro_rules!` or procedural macro; do not report on ordinary macro invocations.
- An `$x:expr` fragment interpolated more than once in the expansion, so the caller's expression — and any side effects — runs multiple times; bind it to a `let` once inside the expansion
- Exported or publicly used macros that reference items without `$crate::`, so name resolution breaks or binds the wrong item when the macro is invoked from another crate
- Token-tree (`$t:tt`) fragments re-emitted without parentheses, where operator precedence can silently change the intended meaning; `:expr` fragments and the expansion as a whole are already atomic, so this applies to token-level interpolation only

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — rephrased to drop the overloaded term. The bullet now reads “only to token-level (tt) interpolation, since an :expr fragment and a whole expansion are each parsed as one complete expression,” which also avoids colliding with the atomic/atomics usage in the Concurrency section. Pushed.

rust.md covered ownership, unsafe, async and atomics but had no
coverage of macros — a high-bug-density area of Rust. Add a scoped
"Macros and Metaprogramming" section for the well-known macro_rules!/
proc-macro footguns: $expr double-evaluation, missing $crate::,
unparenthesized expansion precedence, and proc-macros panicking
instead of emitting compile_error!/syn::Error. Guarded to fire only
when a macro is defined, preserving the doc's precision-over-recall
bias so it does not flag ordinary macro invocations.

No wiring changes: **/*.rs already resolves to rust.md and .rs is
already in the extension allowlist.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@MuoDoo MuoDoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very clean change.

@lizhengfeng101
lizhengfeng101 merged commit c9b1456 into alibaba:main Jul 25, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants