Skip to content

docs(types): record that the codec macros must precede the mod declarations - #303

Open
mehmetkr-31 wants to merge 1 commit into
circlefin:mainfrom
mehmetkr-31:docs/codec-macro-ordering
Open

docs(types): record that the codec macros must precede the mod declarations#303
mehmetkr-31 wants to merge 1 commit into
circlefin:mainfrom
mehmetkr-31:docs/codec-macro-ordering

Conversation

@mehmetkr-31

Copy link
Copy Markdown

network and wal reach impl_versioned_codec! and impl_versioned_codec_with_legacy_fallback! through textual scope — there is no path import and no pub(crate) use re-export on main. Hoisting the pub mod block above the macros, or alphabetising codec/mod.rs, breaks all 11 invocation sites with cannot find macro, and nothing at those sites hints at the dependency.

Two comment lines, one per macro, so moving either one alone still surfaces the constraint. No behaviour change; cargo check -p arc-consensus-types and cargo fmt --all --check clean.

Context: @osr21 raised this while reviewing #232, which proposed removing the imports that made the ordering irrelevant. The v0.8.0 sync (#285) removed them upstream instead, so #232 is superseded — but the fragility it would have introduced now exists on main undocumented, and with a second macro depending on it. Closing #232 in favour of this.

…ations

`network` and `wal` reach `impl_versioned_codec!` and
`impl_versioned_codec_with_legacy_fallback!` through textual scope — there
is no path import and no `pub(crate) use` re-export. Hoisting the `pub mod`
block above the macros, or alphabetising the file, breaks all 11 invocation
sites with `cannot find macro`, and nothing at those sites hints at the
dependency.

Comments only; no behaviour change.
@osr21

osr21 commented Sep 1, 2026

Copy link
Copy Markdown

Verified all of it against main at 66ad2d5 — this records exactly the constraint that exists, with the numbers right.

  • The textual-scope premise holds, repo-wide. impl_versioned_codec is defined at codec/mod.rs:29 and impl_versioned_codec_with_legacy_fallback at :78, with the pub mod block at 129–133 below both. A grep across all of crates/ finds zero use statements or re-exports naming either macro — after the v0.8.0 sync removed the imports, textual scope is the only thing making the 11 sites resolve, and nothing at those sites hints at it, exactly as described.
  • The per-macro comment scoping is precise, not approximate. network.rs invokes the plain macro 8 times and the legacy-fallback macro never; wal.rs invokes the legacy-fallback 3 times and the plain macro never. So each comment names exactly the module and site count that its own macro carries — 8+3 = the 11 in the PR body.
  • The blast radius claim is right-sized too: the same pub mod block also declares error, proto, and versions, none of which touch the macros — so the hoist/alphabetise tidy-up breaks exactly the 11 sites in network/wal and nothing else, and the failure is loud (cannot find macro) but the cause is invisible without this comment. That last property is the whole value here.
  • chore(types): drop redundant macro imports that break CI on Rust 1.92 #232's closure is clean supersession bookkeeping: the sync landed the import removal upstream, so what remained of chore(types): drop redundant macro imports that break CI on Rust 1.92 #232 was precisely its one cost — the undocumented ordering contract — and this PR is that residue, now covering the second macro the sync added.

For the record, the sturdier alternative from the #232 review thread is still available if this ever bites: reinstate a pub(crate) use after each definition and switch the 11 sites to path-form invocation, which makes ordering irrelevant and stays warning-free on Rust 1.92 because the re-export then has users. But that is an 11-site diff to buy robustness against a refactor this comment now makes self-explaining — docs-only is the right-sized fix, and it composes with nothing else in flight (no open PR touches codec/mod.rs).

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.

2 participants