fix(macro): make invalid #[Overwritten(...)] attributes a compile error - #967
fix(macro): make invalid #[Overwritten(...)] attributes a compile error#967techmech-keeb wants to merge 3 commits into
#[Overwritten(...)] attributes a compile error#967Conversation
7ec93fe to
b139126
Compare
Size Report
|
|
CI caught a real gap in my first push: I matched the attribute by the name Overwritten only, but the documented spelling (FAQ, stm32 examples) is #[Override(...)] — the old code never checked the attribute name at all, so both spellings were in use. That made the stm32 examples' #[Override(chip_config)] stop matching, surfacing as unused-import failures. Fixed: both spellings are matched, bind_interrupt passes validation (its own matching in bind_interrupt.rs is untouched), tests extended, and the three failing examples now build clean locally. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b139126459
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An override attribute that failed to parse - an unknown or miscased variant like `Entry` instead of `entry` - was silently skipped: the `if let Ok(...)` matchers discarded darling's error and fell back to the generated default, and the function itself was dropped from the expansion, so neither the macro nor rustc reported anything. The matchers also required `attrs.len() == 1`, so any extra attribute (doc comments included) silently disabled a correctly spelled override. Validate all override attributes up front in parse_keyboard_mod and parse_split_peripheral_mod, emitting darling's diagnostics (which already suggest the snake_case spelling) as compile errors via write_errors(). Replace the per-site `attrs.len() == 1` + from_meta matching with a shared find_overwritten() helper that locates the attribute by path. Both spellings in real use are accepted: the documented `#[Override(...)]` (docs/faq, stm32 examples) and `#[Overwritten(...)]` (the enum's name) - the old matching ignored the attribute path entirely, so both worked before. `bind_interrupt` is accepted by validation as it appears in existing examples; its own matching in bind_interrupt.rs is unchanged. Add unit tests for the typo, extra-attribute, both-spellings, and valid cases. Fixes rmk-rs#966 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3hnNYKmSK6ow1GZAT34F9
b139126 to
c7b52d4
Compare
An attribute macro receives a module's items before cfg-stripping, so a `#[cfg(...)] #[Override(...)]` function reaches the macro with its cfg attribute intact (verified with a probe macro: an always-disabled fn is still handed to the macro). Matching the override marker by path alone would select or validate a cfg-disabled override, letting it replace the generated default or emit a compile error for a disabled typo (PR rmk-rs#967 review by Codex). Skip a function for override selection and validation when it carries `#[cfg(...)]` or `#[cfg_attr(...)]`; inert attributes that don't gate compilation (doc comments, `#[allow(...)]`, ...) may still accompany the marker. Add tests for cfg/cfg_attr-gated markers, cfg-gated typos, and inert-attribute markers. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3hnNYKmSK6ow1GZAT34F9
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b1cafd36f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Accepted so `#[Override(bind_interrupt)]` (used in existing examples) | ||
| /// passes validation; the bind_interrupt override itself is matched | ||
| /// separately in `bind_interrupt.rs`. | ||
| BindInterrupt, |
There was a problem hiding this comment.
When a user follows the documented #[Override(bind_interrupt)] form, this new enum variant makes validate_overwritten_attrs accept it as a valid marker, but expand_bind_interrupt still only recognizes a bare attribute whose path is bind_interrupt and therefore emits the generated default instead. In that configuration the custom interrupt binding function is dropped from the expansion with no diagnostic, so either this variant should be selected by the bind-interrupt path or the marker should remain rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Sorry for the slow follow-up. Fixed in cabb8d8 — bind_interrupt overrides are now selected through the shared override matcher, so the documented #[Override(bind_interrupt)] form is actually applied after passing validation, with the same semantics as every other override (both spellings, inert attributes allowed, cfg-gated functions skipped). The legacy bare #[bind_interrupt] marker keeps working unchanged.
Wiring it up exposed that the stm32h7 example's custom binding had bit-rotted while it was being silently dropped: missing imports, and missing the EXTI lines async_matrix needs (the override replaces the whole generated block). Fixed the example body too — it now builds with its custom binding actually in effect, and added regression tests for the selection path.
There was a problem hiding this comment.
Can you test it on the real hardware?
The documented `#[Override(bind_interrupt)]` form was accepted by validation but never selected: expand_bind_interrupt only recognized a bare attribute whose path is `bind_interrupt`, so the custom function was silently dropped and the generated default emitted instead (rmk-rs#967 review). Select bind_interrupt overrides through the shared override matcher, so the documented form is applied and behaves like every other override (both spellings, inert attributes allowed, cfg-gated functions skipped). The legacy bare `#[bind_interrupt]` marker — the original syntax — keeps working unchanged. Wiring the override exposed that the stm32h7 example's custom binding had bit-rotted while it was being dropped: it was missing imports and the EXTI lines that `async_matrix` needs (the override replaces the whole generated block). Fix the example body and note that requirement; the example now builds with its custom binding actually in effect. Add selection regression tests: the documented form, the Overwritten spelling, the legacy bare marker, a doc-comment neighbor, cfg gating, and a non-bind_interrupt marker. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Fixes #966
Description
An
#[Overwritten(...)]attribute that failed to parse — an unknown or miscased variant like#[Overwritten(Entry)]instead ofentry— was silently skipped: theif let Ok(...)matchers discarded darling's error and fell back to the generated default, and the function itself was dropped from the expansion, so neither the macro nor rustc reported anything. The matchers also requiredattrs.len() == 1, so any extra attribute (doc comments included) silently disabled a correctly spelled override.Overwrittenattributes up front inparse_keyboard_mod/parse_split_peripheral_mod; parse failures now surface darling's own diagnostic (Unknown field: `Entry`. Did you mean `entry`?) as a compile error viawrite_errors()— the idiomlib.rsalready uses for other attribute parsing.attrs.len() == 1+from_metamatching with a sharedfind_overwritten()helper that finds the attribute by path, so an extra attribute (doc comments included) no longer disables a correctly spelled override.Overwrittenattribute, and valid single-attribute overrides, expand exactly as before.CHANGELOGentry added.Testing
cargo testinrmk-macro: unit tests (incl. 6 new covering the typo, extra-attribute/doc-comment, valid, and no-attribute cases), trybuildcompile_fail, and macrotestexpand— all passing (withcargo expandinstalled).cargo nextest run --no-default-features --features=split,vial,storage,async_matrix,_bleinrmk: 548/548 passed on currentmain(rmk consumes the pathrmk-macro, so this compiles with the change).scripts/format_all.shclean (nightly rustfmt).