chore(types): drop redundant macro imports that break CI on Rust 1.92

[mehmetkr-31]

Fixes #234.

Summary

On Rust 1.92 the CI lint command fails on arc-consensus-types:

$ cargo clippy --all-targets --all-features -- -D warnings
error: unused import: `crate::codec::impl_versioned_codec`
  --> crates/types/src/codec/network.rs:26:5
error: unused import: `crate::codec::impl_versioned_codec`
  --> crates/types/src/codec/wal.rs:22:5
error: could not compile `arc-consensus-types` (lib) due to 2 previous errors

main is green today only because rust-toolchain.toml pins 1.91.1. Since the Rust Lint job runs with -D warnings, bumping the toolchain to 1.92 or later turns these into hard CI failures.

Why the imports are redundant

impl_versioned_codec is a macro_rules! macro defined in codec/mod.rs above the pub mod network; and pub mod wal; declarations. Textual macro scoping already puts it in scope for both child modules, so use crate::codec::impl_versioned_codec; never resolved to anything the modules didn't already have. Rust 1.92 is simply the first release whose unused_imports lint reports it.

Removing the two imports leaves the pub(crate) use impl_versioned_codec; re-export in codec/mod.rs with no remaining users, and it warns in turn, so this drops that line as well. Nothing outside the codec module referenced the macro by path.

Net effect is 4 deleted lines and no behaviour change: textual macro scoping is long-stable and version-independent, so impl_versioned_codec! resolves identically on 1.91.1 and on 1.92.

Testing

I do not have the pinned 1.91.1 toolchain available locally, so my verification ran on 1.92.0:

• cargo clippy -p arc-consensus-types --all-targets -- -D warnings — clean (fails on main with the two errors above).
• cargo test -p arc-consensus-types — 192 tests pass.
• cargo fmt --all --check — clean.

CI is the authority for the pinned toolchain, and this PR running against 1.91.1 is the check that matters — if the macro somehow did not resolve without the import, the build would fail outright rather than subtly. Happy to adjust if you would rather keep the imports and silence the lint another way (e.g. an #[allow]), though removing dead lines seemed preferable to carrying an allow for them.

[osr21]

Verified against main before reviewing: impl_versioned_codec is defined at codec/mod.rs:29, above the pub mod network; / pub mod wal; declarations at lines 82–86, so textual macro scoping does put it in scope for both children and the two use statements never resolved to anything new. The toolchain is indeed pinned at 1.91.1, so the "green today, hard-fails on the next toolchain bump" framing is accurate, and filing #234 separately from the fix means the constraint is documented even if this PR's approach were rejected. All 11 macro invocations across the two modules are unaffected. Clean.

One structural note worth a line of code or at least awareness: this change makes declaration order in mod.rs load-bearing. Textual scoping only covers the child modules because macro_rules! impl_versioned_codec sits above pub mod network; and pub mod wal;. Before this PR, the path imports meant a future refactor could reorder mod.rs freely — the pub(crate) use re-export made the macro path-addressable regardless of position. After it, someone alphabetizing the file or hoisting the pub mod block above the macro (both common tidy-ups) gets 11 cannot find macro errors with no obvious cause, since nothing at the use sites hints at the ordering dependence. Two cheap mitigations, either sufficient: a one-line comment above the macro ("must precede the pub mod declarations — child modules rely on textual scope"), or keeping the pub(crate) use and switching the 11 invocation sites to nothing at all — they already work — while documenting why. I'd take the comment; it's the only part of this ordering contract that's currently invisible.

On the 1.92-only local verification: the reasoning holds and is worth spelling out for the record — the failure mode if textual scoping didn't cover the modules is a compile error at the 11 invocation sites, not a silent behavior change, so this PR's own CI run on the pinned 1.91.1 is a complete check. Macro resolution here has been stable since pub(crate) use macro re-exports landed in 2018-edition Rust; there is no version in between where the removal compiles but resolves differently.

Removing dead lines over #[allow]-ing them is the right instinct — an allow would have carried the lint suppression forever to preserve imports whose only function was making order not matter, which is better achieved with the one-line comment.

Approving; with the ordering comment added this is a complete, future-proof close of #234.

[mehmetkr-31]

Closing the verification gap I flagged in the description — I now have access to a Linux box with rustup, so the pinned toolchain actually applies rather than being approximated.

$ rustup show active-toolchain
1.91.1-x86_64-unknown-linux-gnu (overridden by 'rust-toolchain.toml')

Two runs on that toolchain:

main — cargo check -p arc-consensus-types emits zero unused import warnings. That is the direct confirmation that CI is green today only because of the pin, which was the premise of this PR rather than something I could previously demonstrate.

This branch — same command, compiles clean:

Finished `dev` profile [optimized + debuginfo] target(s) in 1.12s

So the textual-scoping argument holds in practice on 1.91.1, not just in theory: removing the two use statements and the now-unused re-export changes nothing for the pinned toolchain, and removes the failure on 1.92. Both directions are now measured rather than inferred.

One incidental finding from the same session, since it is relevant to #231/#233: the E0433 from #233 reproduces on 1.91.1 too — cargo clippy -p arc-consensus-types --all-targets --all-features fails there exactly as it does on 1.92. That bug is not toolchain-specific; it was only ever hidden by workspace feature unification, which is the point #240 is built on.

[osr21]

Good — that closes the loop properly. Both directions measured on the pinned toolchain is exactly the state my approval assumed CI would establish, and now it's established twice.

Two notes on the incidental #233 finding, since it's the more consequential part of your comment (static verification on my side, per our usual split):

The 1.91.1 repro of the E0433 is expected, and the mechanism is worth stating precisely. The arbitrary bug is a missing feature declaration (arbitrary/derive + alloy-primitives/arbitrary), not lint behavior — so no toolchain would hide it. What hides it is package selection: since resolver v2, cargo unifies features across the packages selected in the invocation. --workspace --all-features pulls in arc-consensus-db / arc-node-consensus, whose deps supply the missing features; -p arc-consensus-types --all-features selects one crate, nothing donates, E0433 fires. Your run confirms the two failure axes are fully orthogonal: #232/#234 is toolchain-sensitive and scope-insensitive, #233 is scope-sensitive and toolchain-insensitive. That's the cleanest empirical support yet for #240's premise — a toolchain bump alone would have surfaced one bug class and left the other invisible; only per-crate feature checks catch #233's class.

One consequence for #240's CI job worth capturing there: since the #233 failure reproduces on 1.91.1, the cargo hack --each-feature job needs no toolchain caveat — it would go red on today's pin the moment it lands (until #231 merges), which is the correct behavior and a nice built-in proof the check works. I'll fold your 1.91.1 data point into #240.

On this PR itself: your main-is-clean-on-1.91.1 measurement also confirms the change is invisible to current CI in both directions — it purely unblocks the future bump, so #234 can close on merge with nothing left behind. The only open item from my review is the one-line comment above macro_rules! impl_versioned_codec documenting that it must precede the pub mod declarations (the head is still c1a1852, so declaration order remains a silent load-bearing contract). Non-blocking, as before — my approval stands either way.

[mehmetkr-31]

Added the ordering comment — head is now bdf34bf. You raised it twice and you were right to; I skimmed past it the first time because it was framed as non-blocking, but the fragility it describes is the one real cost this PR introduces and it deserved closing rather than noting.

/// Must stay above the `pub mod` declarations at the bottom of this file:
/// `network` and `wal` reach this macro through textual scope, not through a
/// path, so moving the module declarations above it — or moving this below
/// them — breaks all 11 invocation sites with `cannot find macro`.

Confirmed the count while writing it: 8 invocations in network.rs, 3 in wal.rs, matching your figure. cargo check -p arc-consensus-types and cargo fmt --all --check both clean on the pinned 1.91.1.

Your framing of the two failure axes is sharper than mine and worth keeping as the canonical statement of it: #232/#234 is toolchain-sensitive and scope-insensitive, #233 is scope-sensitive and toolchain-insensitive. That is exactly why a toolchain bump alone would have surfaced one class and left the other invisible — and it means #240's job earns its place independently of any bump, which is a stronger argument for it than the one I made on that issue.

Agreed that #234 closes cleanly on merge with nothing left behind.

[osr21]

Verified bdf34bf on a fresh clone: the comment sits directly above macro_rules! impl_versioned_codec, below the parameter docs where a reader lands right before the ordering matters, and I re-counted the invocation sites — 8 in network.rs, 3 in wal.rs, so the "all 11" claim in the comment is accurate as written.

The wording earns a specific compliment: it states the failure in both directions ("moving the module declarations above it — or moving this below them") and names the exact symptom (cannot find macro). That's the difference between a comment that prevents the mistake and one that merely explains it after two hours of confusion — whoever hits it greps the error string and lands on the answer.

One tiny mechanical note, take or leave: the comment is written as /// doc syntax, continuing the existing doc block. For a private macro_rules! that's inert today (nothing renders it), but if the macro is ever exported the ordering warning would ship into public rustdoc, where it's arguably more useful, not less — so I'd leave it exactly as is. Just flagging that the choice was load-bearing in a good way.

With that, every item from my review is closed: premise measured on the pin in both directions, the four-line diff verified against all 11 call sites, and the one real cost this PR introduces — a silent ordering contract — now documented at the only place it can bite. Nothing further from me; this is ready for a maintainer. And agreed on the axes framing being the durable artifact of this thread — between this PR, #231, and #240, the repo ends up with both bug classes fixed and the CI shape that prevents their recurrence, which is a better outcome than any of the three pieces alone.