From 6b721d731fb1f2d56fb2d335cc1add3293668928 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 3 Aug 2026 10:37:43 +0200 Subject: [PATCH] chore(tools): Remove doc pass from `cargo_check` The `cargo doc` pass added in #900 dominates the cost of every check. Timing a one-line whitespace edit in `jp_config` with a warm cache: 163s and 190s on two runs with the pass enabled, against 16s and 18s with it disabled. rustdoc re-runs for the edited crate and everything downstream of it and has no incremental mode, so a change low in the dependency graph re-documents most of the workspace. That is the wrong price for a signal only needed before work leaves the machine. `cargo_check` is called many times per session, and the doc lints stay covered by `just docs-ci` on CI and by bacon's `doc` job locally. Removes the `docs` option along with the pass, returning the tool config to its pre-#900 shape. Signed-off-by: Jean Mertz --- .config/jp/tools/src/cargo.rs | 9 +- .config/jp/tools/src/cargo/check.rs | 128 +--------- .config/jp/tools/src/cargo/check_tests.rs | 297 +++------------------- .jp/mcp/tools/cargo/check.toml | 9 +- 4 files changed, 41 insertions(+), 402 deletions(-) diff --git a/.config/jp/tools/src/cargo.rs b/.config/jp/tools/src/cargo.rs index 5a9ad38c..e6b5e88d 100644 --- a/.config/jp/tools/src/cargo.rs +++ b/.config/jp/tools/src/cargo.rs @@ -30,14 +30,7 @@ pub async fn run(ctx: Context, t: Tool) -> ToolResult { let checksum_freshness = t.option_or("checksum_freshness", false); match t.name.trim_start_matches("cargo_") { - "check" => { - // Documentation lints fire on doc comments rather than on code, so - // clippy never sees them. Checking them here keeps them from - // reaching CI, at the cost of a `cargo doc` pass; set - // `options.docs = false` to trade that back. - let docs = t.option_or("docs", true); - cargo_check(&ctx, t.opt("package")?, checksum_freshness, docs).await - } + "check" => cargo_check(&ctx, t.opt("package")?, checksum_freshness).await, "expand" => cargo_expand(&ctx, t.req("item")?, t.opt("package")?, checksum_freshness).await, "test" => { cargo_test( diff --git a/.config/jp/tools/src/cargo/check.rs b/.config/jp/tools/src/cargo/check.rs index 5cc53bc6..580e7e70 100644 --- a/.config/jp/tools/src/cargo/check.rs +++ b/.config/jp/tools/src/cargo/check.rs @@ -9,32 +9,15 @@ use crate::util::{ truncate, }; -/// Rustdoc lints that are denied, kept in lockstep with `just docs-ci`. -/// -/// These fire on documentation content rather than on code, so `cargo clippy` -/// and `cargo check` never see them and they otherwise surface only on CI. -const RUSTDOC_LINTS: &[&str] = &[ - "-D rustdoc::broken-intra-doc-links", - "-D rustdoc::private-intra-doc-links", - "-D rustdoc::invalid-codeblock-attributes", - "-D rustdoc::invalid-html-tags", - "-D rustdoc::invalid-rust-codeblocks", - "-D rustdoc::bare-urls", - "-D rustdoc::unescaped-backticks", - "-D rustdoc::redundant-explicit-links", -]; - pub(crate) async fn cargo_check( ctx: &Context, package: Option, checksum_freshness: bool, - docs: bool, ) -> ToolResult { cargo_check_impl( ctx, package.as_deref(), checksum_freshness, - docs, &DuctProcessRunner, ) } @@ -43,7 +26,6 @@ fn cargo_check_impl( ctx: &Context, package: Option<&str>, checksum_freshness: bool, - docs: bool, runner: &R, ) -> ToolResult { let clippy_scope = package.map_or("--workspace".to_owned(), |v| format!("--package={v}")); @@ -85,18 +67,6 @@ fn cargo_check_impl( let clippy = strip_ansi_escapes::strip_str(stderr); let clippy = truncate(clippy.trim(), MAX_DIAGNOSTIC_BYTES); - let doc_note = match doc_check(ctx, package, checksum_freshness, docs, runner)? { - DocCheck::Skipped | DocCheck::Clean => None, - // Deliberately silent on *why* it failed: exit 101 also covers cargo - // errors, `cfg(doc)` compile errors and rustdoc crashes, and nothing in - // the exit status distinguishes those from a denied lint. The - // diagnostics below say which it was. - DocCheck::Failed(diagnostics) => Some(format!( - "`cargo doc` failed. This pass runs the documentation lints CI denies (`just \ - docs-ci`), which clippy does not report:\n\n```\n{diagnostics}\n```" - )), - }; - let comfort_note = match comfort_check(ctx, package, runner)? { ComfortCheck::Clean => None, ComfortCheck::Drift(note) => Some(note), @@ -108,11 +78,7 @@ fn cargo_check_impl( } }; - // Hardest-to-ignore first: a failed doc pass blocks CI, comfort drift is - // auto-fixable. - let extra: Vec = doc_note.into_iter().chain(comfort_note).collect(); - - if extra.is_empty() { + let Some(note) = comfort_note else { return Ok(if clippy.is_empty() { "Check succeeded. No warnings or errors found." .to_owned() @@ -120,101 +86,17 @@ fn cargo_check_impl( } else { format!("```\n{clippy}\n```\n").into() }); - } + }; - // Something below failed, so the header is scoped to what clippy alone - // found. A bare "Check succeeded" would contradict the sections that follow. + // The header is scoped to what clippy alone found. A bare "Check succeeded" + // would contradict the drift note that follows. let header = if clippy.is_empty() { "`cargo clippy` found no warnings or errors.".to_owned() } else { format!("```\n{clippy}\n```") }; - let mut sections = vec![header]; - sections.extend(extra); - Ok(sections.join("\n\n").into()) -} - -enum DocCheck { - /// The caller opted out of the documentation pass. - Skipped, - /// `cargo doc` succeeded, so no denied lint fired. - Clean, - /// `cargo doc` exited non-zero; carries whatever it reported. - /// - /// A denied lint is the expected cause, but the exit status alone cannot - /// rule out a cargo error, a `cfg(doc)` compile error or a rustdoc crash, - /// so this variant does not claim to know which. - Failed(String), -} - -/// Run `cargo doc` with the rustdoc lints CI denies. -/// -/// `--document-private-items` is required, not cosmetic: without it -/// `private-intra-doc-links` cannot fire at all, which is the lint that catches -/// a public doc comment linking to a private item. -/// -/// Shares cargo's default profile and feature set with the clippy pass above, -/// rather than the `docs` profile CI runs under: that pass already holds the -/// profile's build lock (`target//.cargo-lock`) and has already built -/// every dependency unit, so this leaves rustdoc over the workspace crates as -/// the only new work and adds no contention the same invocation wasn't causing -/// already. -/// Profile choice does not affect which documentation lints fire. -fn doc_check( - ctx: &Context, - package: Option<&str>, - checksum_freshness: bool, - enabled: bool, - runner: &R, -) -> Result { - if !enabled { - return Ok(DocCheck::Skipped); - } - - let scope = package.map_or("--workspace".to_owned(), |v| format!("--package={v}")); - let rustdocflags = RUSTDOC_LINTS.join(" "); - - let mut env = vec![("RUSTDOCFLAGS", rustdocflags.as_str())]; - if checksum_freshness { - env.push(("CARGO_UNSTABLE_CHECKSUM_FRESHNESS", "true")); - } - - let ProcessOutput { stderr, status, .. } = runner.run_with_env( - "cargo", - &[ - "doc", - "--color=never", - &scope, - "--quiet", - // `just docs-ci` documents every feature. Without this, code behind - // an optional feature is never compiled, so its doc comments go - // unlinted here and fail on CI instead. - "--all-features", - "--no-deps", - "--document-private-items", - // Report every crate's diagnostics in one pass rather than stopping - // at the first failing crate. - "--keep-going", - ], - &ctx.root, - &env, - )?; - - if status.is_success() { - return Ok(DocCheck::Clean); - } - - let diagnostics = strip_ansi_escapes::strip_str(stderr); - let diagnostics = diagnostics.trim(); - - // An empty stderr leaves nothing to report but the status, which is still - // worth surfacing. - Ok(DocCheck::Failed(if diagnostics.is_empty() { - format!("`cargo doc` failed with exit status {status} and no diagnostics.") - } else { - truncate(diagnostics, MAX_DIAGNOSTIC_BYTES) - })) + Ok(format!("{header}\n\n{note}").into()) } enum ComfortCheck { diff --git a/.config/jp/tools/src/cargo/check_tests.rs b/.config/jp/tools/src/cargo/check_tests.rs index fb8ca58b..037d01de 100644 --- a/.config/jp/tools/src/cargo/check_tests.rs +++ b/.config/jp/tools/src/cargo/check_tests.rs @@ -50,7 +50,7 @@ fn test_cargo_check_with_warnings() { .expect("comfort") .returns_success(""); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); assert_eq!(result.into_content().unwrap(), indoc::indoc! {r#" ``` @@ -80,7 +80,7 @@ fn test_cargo_check_no_warnings() { .expect("comfort") .returns_success(""); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); assert_eq!( result.into_content().unwrap(), @@ -103,7 +103,7 @@ fn clean_clippy_with_comfort_drift_appends_note() { status: ExitCode::from_code(1), }); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); // The header is clippy-scoped, not a blanket "Check succeeded", so it does // not contradict the drift note below it. @@ -134,7 +134,7 @@ fn clippy_warnings_and_comfort_drift_are_both_reported() { status: ExitCode::from_code(1), }); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); assert_eq!(result.into_content().unwrap(), indoc::indoc! {" ``` @@ -163,7 +163,7 @@ fn comfort_drift_listing_is_bounded() { status: ExitCode::from_code(1), }); - let content = cargo_check_impl(&ctx, None, false, false, &runner) + let content = cargo_check_impl(&ctx, None, false, &runner) .unwrap() .unwrap_content(); @@ -193,7 +193,7 @@ fn comfort_real_failure_is_reported_as_error() { status: ExitCode::from_code(2), }); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); match result { Outcome::Error { message, .. } => { assert_eq!(message, "comfort failed: comfort: parse error"); @@ -214,7 +214,7 @@ fn clippy_failure_short_circuits_before_running_comfort() { status: ExitCode::from_code(101), }); - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, None, false, &runner).unwrap(); match result { Outcome::Error { message, .. } => { assert_eq!(message, "Cargo command failed: error: build failed"); @@ -252,222 +252,44 @@ fn package_scope_is_passed_through_to_both_tools() { ]) .returns_success(""); - let result = cargo_check_impl(&ctx, Some("my_pkg"), false, false, &runner).unwrap(); + let result = cargo_check_impl(&ctx, Some("my_pkg"), false, &runner).unwrap(); assert_eq!( result.into_content().unwrap(), "Check succeeded. No warnings or errors found." ); } -/// Rustdoc lints are denied on CI but invisible to clippy, so a clean clippy -/// run must still surface them. -#[test] -fn doc_lints_are_reported_alongside_a_clean_clippy_run() { - let (_dir, ctx) = ctx(); - - let doc_stderr = indoc::indoc! {" - error: public documentation for `estimate_overhead_chars` links to private item `OVERHEAD_FACTOR` - --> crates/jp_llm/src/window.rs:42:35 - error: could not document `jp_llm` - "}; - - let runner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("cargo") - .returns(ProcessOutput { - stdout: String::new(), - stderr: doc_stderr.to_owned(), - status: ExitCode::from_code(101), - }) - .expect("comfort") - .returns_success(""); - - let result = cargo_check_impl(&ctx, None, false, true, &runner).unwrap(); - - assert_eq!(result.into_content().unwrap(), indoc::indoc! {" - `cargo clippy` found no warnings or errors. - - `cargo doc` failed. This pass runs the documentation lints CI denies (`just docs-ci`), which clippy does not report: - - ``` - error: public documentation for `estimate_overhead_chars` links to private item `OVERHEAD_FACTOR` - --> crates/jp_llm/src/window.rs:42:35 - error: could not document `jp_llm` - ```"}); -} - -#[test] -fn a_clean_doc_run_adds_nothing_to_the_output() { - let (_dir, ctx) = ctx(); - - let runner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("cargo") - .returns_success("") - .expect("comfort") - .returns_success(""); - - let result = cargo_check_impl(&ctx, None, false, true, &runner).unwrap(); - - assert_eq!( - result.into_content().unwrap(), - "Check succeeded. No warnings or errors found." - ); -} - -/// Doc lints come before the comfort note: the first fails CI, the second is -/// auto-fixable. -#[test] -fn doc_lints_and_comfort_drift_are_both_reported() { - let (_dir, ctx) = ctx(); - let comfort_stdout = format!("{root}/src/lib.rs", root = ctx.root); - - let runner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("cargo") - .returns(ProcessOutput { - stdout: String::new(), - stderr: "error: unresolved link to `Nope`".to_owned(), - status: ExitCode::from_code(101), - }) - .expect("comfort") - .returns(ProcessOutput { - stdout: comfort_stdout, - stderr: String::new(), - status: ExitCode::from_code(1), - }); - - let result = cargo_check_impl(&ctx, None, false, true, &runner).unwrap(); - - assert_eq!(result.into_content().unwrap(), indoc::indoc! {" - `cargo clippy` found no warnings or errors. - - `cargo doc` failed. This pass runs the documentation lints CI denies (`just docs-ci`), which clippy does not report: - - ``` - error: unresolved link to `Nope` - ``` - - Doc comments in the following files are badly formatted. Run `cargo_fmt` to auto-fix them: - - src/lib.rs"}); -} - -/// `--document-private-items` is what makes `private-intra-doc-links` fire, -/// `--all-features` is what makes feature-gated doc comments visible at all, -/// and the package scope has to reach rustdoc too. -#[test] -fn doc_run_denies_the_ci_lints_and_honours_the_package_scope() { - let (_dir, ctx) = ctx(); - - let runner = MockProcessRunner::builder() - .expect("cargo") - .args(&[ - "clippy", - "--color=never", - "--package=my_pkg", - "--quiet", - "--all-targets", - "--all-features", - ]) - .returns_success("") - .expect("cargo") - .args(&[ - "doc", - "--color=never", - "--package=my_pkg", - "--quiet", - "--all-features", - "--no-deps", - "--document-private-items", - "--keep-going", - ]) - .returns_success("") - .expect("comfort") - .returns_success(""); - - let result = cargo_check_impl(&ctx, Some("my_pkg"), false, true, &runner).unwrap(); - assert_eq!( - result.into_content().unwrap(), - "Check succeeded. No warnings or errors found." - ); -} - -/// The denied lints only take effect if they reach rustdoc. -/// -/// `MockProcessRunner` validates the program and args of each call but ignores -/// the environment, so without this the whole `RUSTDOCFLAGS` plumbing could be -/// deleted and every other test here would still pass. -#[test] -fn doc_run_passes_the_denied_lints_to_rustdoc() { - let (_dir, ctx) = ctx(); - - let runner: CallCapturingRunner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("cargo") - .returns_success("") - .expect("comfort") - .returns_success("") - .into(); - - cargo_check_impl(&ctx, None, false, true, &runner).unwrap(); - - let doc = runner - .call_with_arg("doc") - .expect("the doc pass must have run"); - - let expected = RUSTDOC_LINTS.join(" "); - assert_eq!( - doc.env - .iter() - .find(|(key, _)| key == "RUSTDOCFLAGS") - .map(|(_, value)| value.as_str()), - Some(expected.as_str()), - ); - - // The lint that caught the failure this pass was added for. - assert!(expected.contains("-D rustdoc::private-intra-doc-links")); -} - -/// `CARGO_UNSTABLE_CHECKSUM_FRESHNESS` has to reach both cargo passes. +/// `CARGO_UNSTABLE_CHECKSUM_FRESHNESS` has to reach cargo. /// /// Sibling git worktrees share a target directory, and mtime-based freshness /// lets one checkout serve the other's stale artifacts; content checksums are /// what prevent that. /// `MockProcessRunner` validates args but ignores the environment, so nothing -/// else here would notice the variable going missing from either pass. +/// else here would notice the variable going missing. #[test] -fn checksum_freshness_reaches_both_cargo_passes() { +fn checksum_freshness_reaches_cargo() { let (_dir, ctx) = ctx(); let runner: CallCapturingRunner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") .expect("cargo") .returns_success("") .expect("comfort") .returns_success("") .into(); - cargo_check_impl(&ctx, None, true, true, &runner).unwrap(); + cargo_check_impl(&ctx, None, true, &runner).unwrap(); - for pass in ["clippy", "doc"] { - let call = runner - .call_with_arg(pass) - .unwrap_or_else(|| panic!("the {pass} pass must have run")); + let call = runner + .call_with_arg("clippy") + .expect("the clippy pass must have run"); - assert_eq!( - call.env - .iter() - .find(|(key, _)| key == "CARGO_UNSTABLE_CHECKSUM_FRESHNESS") - .map(|(_, value)| value.as_str()), - Some("true"), - "the {pass} pass must opt into checksum-based freshness", - ); - } + assert_eq!( + call.env + .iter() + .find(|(key, _)| key == "CARGO_UNSTABLE_CHECKSUM_FRESHNESS") + .map(|(_, value)| value.as_str()), + Some("true"), + ); } /// Off unless opted into, so the tools also work on stable cargo. @@ -476,29 +298,25 @@ fn checksum_freshness_is_absent_unless_opted_into() { let (_dir, ctx) = ctx(); let runner: CallCapturingRunner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") .expect("cargo") .returns_success("") .expect("comfort") .returns_success("") .into(); - cargo_check_impl(&ctx, None, false, true, &runner).unwrap(); + cargo_check_impl(&ctx, None, false, &runner).unwrap(); - for pass in ["clippy", "doc"] { - let call = runner - .call_with_arg(pass) - .unwrap_or_else(|| panic!("the {pass} pass must have run")); + let call = runner + .call_with_arg("clippy") + .expect("the clippy pass must have run"); - assert!( - !call - .env - .iter() - .any(|(key, _)| key == "CARGO_UNSTABLE_CHECKSUM_FRESHNESS"), - "the {pass} pass must not require nightly cargo unless asked to", - ); - } + assert!( + !call + .env + .iter() + .any(|(key, _)| key == "CARGO_UNSTABLE_CHECKSUM_FRESHNESS"), + "the clippy pass must not require nightly cargo unless asked to", + ); } /// One recorded subprocess invocation. @@ -510,7 +328,7 @@ struct CapturedCall { /// A runner that records every call's args and environment. /// /// `EnvCapturingRunner` in `cargo/test_tests.rs` keeps only the most recent -/// call's environment; `cargo_check` makes three, so the doc pass has to be +/// call's environment; `cargo_check` makes two, so the clippy pass has to be /// picked out rather than assumed to be last. struct CallCapturingRunner { inner: MockProcessRunner, @@ -561,50 +379,3 @@ impl ProcessRunner for CallCapturingRunner { self.inner.run_with_opts(program, args, working_dir, opts) } } - -/// A `cargo doc` failure with no diagnostics still has to say something. -#[test] -fn a_silent_doc_failure_reports_its_status() { - let (_dir, ctx) = ctx(); - - let runner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("cargo") - .returns(ProcessOutput { - stdout: String::new(), - stderr: String::new(), - status: ExitCode::from_code(101), - }) - .expect("comfort") - .returns_success(""); - - let content = cargo_check_impl(&ctx, None, false, true, &runner) - .unwrap() - .unwrap_content(); - - assert!( - content.contains("`cargo doc` failed with exit status 101"), - "got: {content}" - ); -} - -/// Disabling the doc pass must not leave a stray `cargo doc` invocation behind: -/// `MockProcessRunner` panics on drop if an expectation goes unused, so the -/// two-expectation setup here is the assertion. -#[test] -fn docs_disabled_skips_the_doc_run() { - let (_dir, ctx) = ctx(); - - let runner = MockProcessRunner::builder() - .expect("cargo") - .returns_success("") - .expect("comfort") - .returns_success(""); - - let result = cargo_check_impl(&ctx, None, false, false, &runner).unwrap(); - assert_eq!( - result.into_content().unwrap(), - "Check succeeded. No warnings or errors found." - ); -} diff --git a/.jp/mcp/tools/cargo/check.toml b/.jp/mcp/tools/cargo/check.toml index 0d90b4eb..e19dbc4e 100644 --- a/.jp/mcp/tools/cargo/check.toml +++ b/.jp/mcp/tools/cargo/check.toml @@ -3,7 +3,7 @@ enable = false run = "unattended" source = "local" command = "just serve-tools {{context}} {{tool}}" -summary = "Run `cargo check` for the given package, validating if the code compiles. Also runs the documentation lints CI denies (e.g. a public doc comment linking to a private item), and reports doc comments that are badly formatted; run `cargo_fmt` to auto-fix the formatting." +summary = "Run `cargo check` for the given package, validating if the code compiles. Also reports doc comments that are badly formatted; run `cargo_fmt` to auto-fix them." examples = """ ```json @@ -22,13 +22,6 @@ When no package is specified, all workspace packages will be checked. # defaults to off so the tool also works on stable Rust. options.checksum_freshness = true -# Also run `cargo doc` with the rustdoc lints CI denies. These fire on doc -# comment content, so clippy never sees them and they otherwise surface only on -# CI. Shares the clippy pass's profile and feature set, so the only new work is -# rustdoc over the workspace crates; set to `false` to trade the coverage back -# for speed. -options.docs = true - [conversation.tools.cargo_check.style] inline_results = "full" results_file_link = "off"