Skip to content

fix: detect serde attributes inside macros#543

Merged
Boshen merged 6 commits into
mainfrom
test/misplaced-ignored
Jul 15, 2026
Merged

fix: detect serde attributes inside macros#543
Boshen merged 6 commits into
mainfrom
test/misplaced-ignored

Conversation

@Boshen

@Boshen Boshen commented Jul 15, 2026

Copy link
Copy Markdown
Owner

A #[serde(with = "...")] inside a macro body is invisible to the parser, so the dependency is reported as unused:

macro_rules! config {
    ($name:ident) => {
        pub struct $name {
            #[serde(with = "humantime_serde")]   // never seen
            pub timeout: std::time::Duration,
        }
    };
}
config!(Config);

It gets worse if the crate also uses the dependency in a dev target. The macro-generated usage is the lib's only visible one, so the dev usage looks like the only usage and cargo-shear escalates to an error:

× misplaced dependency `humantime-serde`
  help: move this dependency to `[dev-dependencies]`

--fix applies that, moving a dependency the lib genuinely needs and breaking the build.

The fix

A macro body keeps its attributes as plain tokens, so they never become Attr nodes and never reach collect_meta_imports, which is the only thing wired to collect_serde_attribute. collect_token_tree already recovers :: paths from macro bodies for exactly this reason, so the serde scan belongs there too.

It's gated on a serde ident being present in the token stream, so unrelated attribute macros are unaffected. An attribute's own path sits outside its token tree, so #[serde(...)] isn't scanned twice and existing behaviour is untouched.

serde_attributes_inside_macro fails without the fix — imports come back empty instead of {"foo"}. It covers every key in is_serde_attribute_key (with, deserialize_with, serialize_with, crate, remote) across both a macro_rules! definition and a macro invocation. crate earns its line: it's a keyword rather than an ident, so it takes a separate branch in the key check.

Unit-only is deliberate — the defect is in the parser, and everything downstream is generic and already covered by misplaced, clean, and unused_naming_hyphen.

Refs #542

This fixes the unused → misplaced escalation reported there. The redundant_ignore step doesn't reproduce on main and I think the report telescopes two versions: that step requires the misplaced condition to be false while misplaced_dependency requires it true, and when it holds #367 already suppresses the ignore. It most likely predates #367 landing in v1.10.0.

Left as Refs rather than Closes — worth confirming with robjtede that their serde(with = "...") is macro-generated first.

Boshen added 3 commits July 15, 2026 20:02
Adds a fixture for the scenario in #542: a dependency in `[dependencies]`
that is only detected in a dev target, with an `ignored` entry covering it.
The ignore suppresses `misplaced_dependency`, so it must not be reported as
a redundant ignore.

This was fixed in #367 but only covered incidentally, through `slab` in the
`complex` fixture. Reverting the suppression branch from #367 makes the new
test fail with the `misplaced_dependency` error from the report.
`humantime-serde` is never imported; the `serde(with = "...")` attribute is
its only usage. This is the position from #542, where being flagged unused is
what pushes users toward adding an ignore they don't need.

Covered at the parser level in `src/tests.rs`, but not end-to-end against a
real dependency. Dropping `with` from `is_serde_attribute_key` makes the new
test fail with `unused dependency humantime-serde`.
A macro body keeps its attributes as plain tokens, so a
`#[serde(with = "humantime_serde")]` inside one never reaches
`collect_meta_imports` and the dependency looks unused.

Worse, if the crate also uses it in a dev target, the only *visible* usage is
the dev one, so it is reported as a misplaced dependency at error level.
Following that advice, or running `--fix`, moves it to `[dev-dependencies]`
and breaks the lib. That matches the escalation reported in #542.

`collect_token_tree` already recovers `::` paths from macro bodies for this
reason; run the serde scan there too, gated on a `serde` ident so unrelated
attribute macros are unaffected.
@Boshen Boshen changed the title test: cover ignore that suppresses a misplaced dependency fix: detect serde attributes inside macro bodies Jul 15, 2026
Boshen added 3 commits July 15, 2026 21:24
`serde_attributes_inside_macro` covered only `with` and `deserialize_with`;
extend it to every key in `is_serde_attribute_key`. `crate` is a keyword
rather than an ident, so it takes a separate branch in the key check.

Drop `serde` and the derive from both serde fixtures. Nothing compiles them —
cargo-shear only runs `cargo metadata` and parses — so the attribute alone
exercises the parser. `humantime-serde` still pulls `serde` in transitively,
so the lockfiles are unchanged in size.
The behaviour it guards is already covered: reverting #367's suppression
branch fails complex_detection and complex_fix, so a dedicated fixture adds
no protection, and it is unrelated to the serde fix in this branch.
The defect is in the parser, which this repo covers with unit tests in
src/tests.rs. serde_attributes_inside_macro pins it directly and fails
without the fix.

The fixtures could not fail unless that unit test also failed: everything
downstream of the parser is generic and already covered by misplaced, clean,
and unused_naming_hyphen. They cost two 91-line lockfiles and fetching
humantime-serde in CI for no added protection.
@Boshen
Boshen marked this pull request as ready for review July 15, 2026 13:53
@Boshen Boshen changed the title fix: detect serde attributes inside macro bodies fix: detect serde attributes inside macros Jul 15, 2026
@Boshen
Boshen merged commit 61952e2 into main Jul 15, 2026
3 checks passed
@Boshen
Boshen deleted the test/misplaced-ignored branch July 15, 2026 13:56
Boshen added a commit that referenced this pull request Jul 16, 2026
Adds a regression test for a `serde` attribute forwarded through another
attribute, as
[confik](https://docs.rs/confik/0.15.12/confik/#forwarding-attributes)
does:

```rust
struct Foo {
    #[confik(forward(serde(with = "humantime_serde")))]
    timeout: Duration,
}
```

This is the shape reported in #542. It is not a macro body — confik is a
derive macro, so the attribute is a plain `Attr` node. It failed because
`collect_meta_imports` only calls `collect_serde_attribute` when the
attribute's own path is literally `serde`, and here it is `confik`.

#543 already fixes it: the token scan added in `collect_token_tree` runs
on every attribute's token tree and matches on a `serde` ident anywhere
in the stream, so the nested `serde(...)` is picked up regardless of the
outer path. The existing `serde_attributes_inside_macro` test does not
cover this path — it would keep passing if the scan were ever narrowed
to the attribute path or scoped to macro bodies, silently regressing
confik. This test pins that behaviour: it fails with the #543 hunk
reverted (`left: {}, right: {"foo"}`).
Boshen added a commit that referenced this pull request Jul 16, 2026
## 🤖 New release

* `cargo-shear`: 1.13.1 -> 1.13.2 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[1.13.2](v1.13.1...v1.13.2)
- 2026-07-16

### <!-- 1 -->🐛 Bug Fixes
- detect serde attributes inside macros
([#543](#543)) (by @Boshen)

### <!-- 3 -->📚 Documentation
- add security policy
([#537](#537)) (by @Boshen)

### <!-- 6 -->🧪 Testing
- cover serde attributes forwarded through another attribute
([#546](#546)) (by @Boshen)

### Contributors

* @renovate[bot]
* @Boshen
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
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.

1 participant