Skip to content

docs: fix rustdoc diagnostics and guard them in CI - #258

Open
mehmetkr-31 wants to merge 2 commits into
circlefin:mainfrom
mehmetkr-31:docs/fix-rustdoc-broken-links
Open

docs: fix rustdoc diagnostics and guard them in CI#258
mehmetkr-31 wants to merge 2 commits into
circlefin:mainfrom
mehmetkr-31:docs/fix-rustdoc-broken-links

Conversation

@mehmetkr-31

Copy link
Copy Markdown

Closes #257.

Summary

cargo doc runs nowhere in this repo, so rustdoc diagnostics have accumulated. On main the command this PR adds to CI reports 31 across 23 files:

$ RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features --locked
error: could not document `arc-consensus-types`   (and 22 more 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. ExecutionPayload in evm-node/src/engine.rs is a prose reference to a family of types, not one importable item.

Public docs pointing at private items → keep the name, drop the link. PersistedBlockMeter referenced the private SUBSCRIPTION_STATUS_* constants and the private subscription_status field; fetch_all_metrics referenced MAX_CONCURRENT_FETCHES; parse_perf_metrics_delta referenced display_name_for_scrape; inspect_frame_init referenced ArcEvm::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_block is a method on the public PersistenceMeter trait, 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 in quake/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

-/// Creates a new [`FixedBytes`] where all bytes are set to `byte`.
+/// Creates a new [`Address`] where all bytes are set to `byte`.
 pub const fn repeat_byte(byte: u8) -> Self {

Address::repeat_byte returns Self, not a FixedBytes — the wording reads as carried over from alloy's docs for its own FixedBytes::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

- name: Build docs
  run: cargo doc --workspace --no-deps --all-features --locked
  env:
    RUSTDOCFLAGS: -D warnings

Anchored after rust-fmt, deliberately away from the areas #241, #247 and #250 touch in ci.yml, so it does not race them. Own cache key (rust-docs) for the same reason as #241 — a doc build would otherwise churn the rust-build cache 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 on main).
  • cargo fmt --all --check — clean.
  • Outside ci.yml the diff is doc comments only; verified no non-comment line changed in any .rs file. The two touched files that contain fenced blocks (evm/src/evm.rs, quake/src/tests/types.rs) use ```text and ```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:8545 examples elsewhere are untouched because rustdoc does not flag them and rewriting them would be churn.

`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 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@mehmetkr-31

Copy link
Copy Markdown
Author

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 sed pattern using \S*, which BSD sed does not support. It matched empty, so the substitution ended right after https:// and left the rest of the URL as prose.

The part worth recording is why my own acceptance test did not catch it. <https://> is a syntactically valid autolink, and the trailing github.com/... has no scheme, so it does not trip rustdoc::bare_urls either. RUSTDOCFLAGS="-D warnings" cargo doc exits 0 on both the broken and the correct form. So the check I cited as proof can demonstrate the absence of diagnostics but not that a link points anywhere — and I presented it as if it covered both. A structural check is what actually catches this class:

$ grep -rn "<https://>" --include="*.rs" crates
(no matches)
$ grep -rhno "<https://[^>]*>" --include="*.rs" crates | wc -l
17

Re-ran RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --locked over the three affected packages after the fix: exit 0, zero diagnostics.

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 osr21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
17

All 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.

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.

docs: cargo doc is not run in CI; 31 rustdoc diagnostics have accumulated

2 participants