docs: fix rustdoc diagnostics and guard them in CI - #258
Conversation
`cargo doc` is not run anywhere — not in CI, not in the Makefile — so
rustdoc diagnostics have accumulated unnoticed. On `main`:
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features
31 diagnostics across 23 files
Three classes, all of which render as broken or misleading API docs:
- 13 unresolved intra-doc links. Some are prose the parser mistook for
links (`[<validator>:<voting_power>]`, `[node_groups]`, `#[quake_test]`),
others reference types not in scope (`ExecutionPayload`, `FixedBytes`).
- 7 public items documented with links to private items, e.g.
`PersistedBlockMeter` pointing at the private `SUBSCRIPTION_STATUS_*`
constants.
- 11 bare URLs and unclosed HTML tags: `<timestamp>` and `<subnet>`
placeholders were parsed as HTML, and the upstream Reth fork
references did not render as links.
Each resolution follows what the site actually meant: prose gets
backticks, private references keep the name but lose the link, bare URLs
become autolinks, and `wait_for_persisted_block` gets a real target via
the trait path, `[`PersistenceMeter::wait_for_persisted_block`]`.
One was a documentation error rather than a link error:
`Address::repeat_byte` was documented as "Creates a new [`FixedBytes`]"
though it returns `Self` — wording that reads as carried over from
alloy's docs. It now says `Address`.
Adds a `rust-docs` job so the drift cannot recur silently. Outside the
workflow file this is comments only; no code changed.
Closes circlefin#257
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
osr21
left a comment
There was a problem hiding this comment.
I verified this PR hunk-by-hunk against a fresh clone of main (full details on #257 — every diagnostic the issue cites is real, and I've confirmed this diff touches doc comments only, plus the one CI hunk; no code or Cargo.lock changes needed since nothing becomes pub). The strategy is right throughout: private-item links demoted to code spans instead of widening visibility, wait_for_persisted_block upgraded to the resolvable PersistenceMeter:: path where a real target exists, and the repeat_byte doc corrected to [Address] rather than merely silenced. The rust-docs job mirrors the existing Rust jobs' conventions exactly (same SHA-pinned checkout, same setup-rust-toolchain@v1 the other five jobs use, same apt line as rust-lint).
One class of defect needs fixing before merge: four of the nine URL wraps are malformed. The autolink bracket closes right after the scheme instead of after the URL:
crates/eth-engine/src/engine.rs: <https://>github.com/ethereum/execution-apis/...
crates/eth-engine/src/rpc/errors.rs: <https://>github.com/paradigmxyz/reth/.../error.rs
crates/evm/src/executor.rs: <https://>github.com/alloy-rs/evm/.../block.rs
crates/execution-txpool/src/pool.rs: <https://>github.com/paradigmxyz/reth/...#L435-L509
The other five (e.g. evm-node/src/{engine,node,payload}.rs, execution-payload/src/payload.rs) are correct — <https://github.com/...> wrapping the whole URL. In the four broken ones, rustdoc sees an autolink whose content is just https:// and the rest of the URL becomes plain prose, so these will either still warn or render as a dead https:// link followed by unlinked text — worse than the bare URL they replace. Looks like a search/replace that anchored on https:// and inserted > immediately after it; worth grepping the branch for <https://> to catch all instances at once. Since the PR's own acceptance test is RUSTDOCFLAGS="-D warnings" cargo doc exiting 0, I'd double-check that run happened on the current head — I can't run rustdoc in my environment (disclosed on #257; my verification is static), but <https://> should not survive a clean -D warnings build if rustdoc flags the orphaned tag, and if it does pass, the rendered output is still wrong.
One correction to my #257 comment: I flagged trailing whitespace on the split persistence_meter.rs line. That was an artifact of my own truncated diff output — the actual head has no trailing whitespace (grep '^+.* $' over the full diff comes back empty). Withdrawn; apologies for the noise.
With the four <https://> wraps fixed to enclose the full URL, this is a clean, well-executed PR that permanently closes off a whole class of doc drift.
Four of the nine URL wraps in the previous commit closed the autolink
immediately after the scheme:
<https://>github.com/paradigmxyz/reth/...
leaving the rest of the URL as plain prose. The cause was a `sed`
pattern using `\S*`, which BSD sed does not support — it matched empty,
so the substitution ended at `https://`.
This is worse than the bare URL it replaced: `<https://>` renders as a
dead link followed by unlinked text. Reported by @osr21 on circlefin#258.
Worth recording why the acceptance test missed it: `<https://>` is a
syntactically valid autolink and the trailing `github.com/...` has no
scheme, so it does not trip `rustdoc::bare_urls` either. The
`RUSTDOCFLAGS="-D warnings"` run exits 0 on both the broken and the
correct form — it can prove the absence of diagnostics but not that a
link points anywhere. A structural check is what catches this:
$ grep -rn "<https://>" --include="*.rs" crates
(no matches)
$ grep -rhno "<https://[^>]*>" --include="*.rs" crates | wc -l
17
All 17 autolinks now enclose their full URL.
|
You're right, and thank you — four of the nine were malformed exactly as you describe. Fixed in the head commit; all 17 autolinks now enclose their full URL. The cause was what you guessed: a The part worth recording is why my own acceptance test did not catch it. $ grep -rn "<https://>" --include="*.rs" crates
(no matches)
$ grep -rhno "<https://[^>]*>" --include="*.rs" crates | wc -l
17Re-ran Noted on the withdrawn whitespace flag — no noise caused, and raising it was the right call given you were reading a truncated diff. That one cost nothing to check. |
osr21
left a comment
There was a problem hiding this comment.
Independently re-verified c7e446b on a fresh clone of your branch:
$ grep -rn "<https://>" --include="*.rs" crates | wc -l
0
$ grep -rhno "<https://[^>]*>" --include="*.rs" crates | wc -l
17All four previously-malformed sites now enclose the full URL (engine.rs:110, rpc/errors.rs:25, executor.rs:90, pool.rs:35), and spot-checking the other 13 shows no regressions from the fix commit. Your structural greps and my run agree exactly — and this time both sides executed, so nothing rests on inference.
The BSD-sed \S* root cause is a satisfying one: \S silently matching empty is precisely the kind of failure that produces valid-looking output, which connects to your bigger point. The lesson you extracted is the right one and worth restating for anyone who lands here later: -D warnings proves the absence of diagnostics, not the presence of meaning. <https://> is a well-formed autolink and schemeless trailing prose trips no lint, so the acceptance test was structurally blind to this defect class. Your grep -c "<https://>" check is the correct complement — cheap, exact, and it fails loudly on the only malformation sed could have produced here. If the rust-docs CI job ever grows a second step, that one-liner would be a reasonable candidate, though with the sed pattern gone it's guarding against a generator that no longer exists, so I wouldn't block anything on it.
With the four autolinks fixed, my original review's only defect class is resolved: the 31 diagnostics are correctly addressed, the private-item demotions avoid visibility widening, repeat_byte's doc now tells the truth, and the CI job pins and conventions match the other five Rust jobs. Approving. This plus #257 makes a clean pair: issue documents the debt, PR retires it and locks the door behind it.
Closes #257.
Summary
cargo docruns nowhere in this repo, so rustdoc diagnostics have accumulated. Onmainthe command this PR adds to CI reports 31 across 23 files:After this PR: exit 0.
What changed and why each way
Prose the parser read as a link → backticks.
[<validator>:<voting_power>],[node_groups],#[quake_test],#[non_exhaustive],[COPY],[failures],[default]were never meant to be links.Types not in scope → plain code spans.
ExecutionPayloadinevm-node/src/engine.rsis a prose reference to a family of types, not one importable item.Public docs pointing at private items → keep the name, drop the link.
PersistedBlockMeterreferenced the privateSUBSCRIPTION_STATUS_*constants and the privatesubscription_statusfield;fetch_all_metricsreferencedMAX_CONCURRENT_FETCHES;parse_perf_metrics_deltareferenceddisplay_name_for_scrape;inspect_frame_initreferencedArcEvm::inspect_frame_init_impl. I did not make any of these public — they are implementation details the prose legitimately names, so the fix is to stop pretending they are navigable.One got a real target instead.
wait_for_persisted_blockis a method on the publicPersistenceMetertrait, so[PersistenceMeter::wait_for_persisted_block]resolves and is more useful than a code span.Bare URLs → autolinks. The upstream Reth/alloy fork references in
evm-node/src/{engine,node,payload}.rs,execution-payload/src/payload.rs,execution-txpool/src/pool.rs,evm/src/executor.rs, plus the cloudping source note inquake/src/latency.rs, now render as links.Placeholders → code spans.
<timestamp>,<subnet>,<container>,<node>,<mode>were being parsed as unclosed HTML tags.One documentation error, not a link error
Address::repeat_bytereturnsSelf, not aFixedBytes— the wording reads as carried over from alloy's docs for its ownFixedBytes::repeat_byte. This is the only change that alters what the documentation claims rather than how it renders; flagging it separately so it gets read rather than skimmed with the mechanical ones.The CI job
Anchored after
rust-fmt, deliberately away from the areas #241, #247 and #250 touch inci.yml, so it does not race them. Own cache key (rust-docs) for the same reason as #241 — a doc build would otherwise churn therust-buildcache the compile jobs depend on. Happy to share the key instead if you prefer the reuse.Testing
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features --locked— exit 0 (fails onmain).cargo fmt --all --check— clean.ci.ymlthe diff is doc comments only; verified no non-comment line changed in any.rsfile. The two touched files that contain fenced blocks (evm/src/evm.rs,quake/src/tests/types.rs) use```textand```ignore, and the edits are outside those fences, so no doctest is affected.Scope note
I kept this to diagnostics rustdoc actually reports, rather than sweeping every URL-shaped string in the tree — several
http://localhost:8545examples elsewhere are untouched because rustdoc does not flag them and rewriting them would be churn.