ci: add extended-lint as a cargo xtask instead of a Python script - #6
ci: add extended-lint as a cargo xtask instead of a Python script#6jordigilh wants to merge 1 commit into
Conversation
Python doesn't fit a Rust-native codebase: it needs a separate interpreter/toolchain in CI, gets no compiler or clippy coverage of its own, and can't share the repo's regex/anyhow dependencies. Convert forge into a minimal two-member workspace (the existing `forge` binary plus a new `xtask` dev-tooling crate) and reimplement the diff-scoped heuristic checks as `cargo xtask lint-extended`, wired into the same CI job. The repo's shared lint deny-list (unsafe_code, missing_docs_in_private_items, unwrap_used, and ~90 others) moves to `[workspace.lints]` so both members inherit it via `lints.workspace = true`, keeping xtask held to the same bar as the rest of the codebase. `xtask/src/lint_extended.rs` excludes itself from its own diff scan: its doc comments and tests legitimately quote the marker words and comment-like syntax the heuristics look for, which the standalone Python script never triggered on since `*.rs` never matched a `.py` file. Fixes praxis-proxy#5 Signed-off-by: Jordi Gil <jgil@redhat.com>
c179a1f to
82fe8a4
Compare
|
Closing this out without merging. After digging into what actually exists in the Rust ecosystem for this class of check, most of the "extended lint" heuristics either have no reliable off-the-shelf equivalent in any language's tooling (commented-out-code detection, "narrating" comment detection), or only partial overlap that does not justify maintaining bespoke regex/diff-scoped Rust across six repos (e.g. TODO markers are trivially a one-line Decision: we do not want to own and maintain custom heuristic lint logic per-repo. If this capability is worth having, the better path is contributing it upstream to a maintained linter (clippy itself, or a |
|
Follow-up on the reasoning above, now that all six ports are done: implementing this surfaced a concrete illustration of exactly the maintenance burden we did not want to sign up for. Every one of the six independent Rust ports had to hand-write a workaround for the same problem -- the linter's own source (its regex definitions, doc comments, and test fixtures) legitimately contains the words |
Summary
cargo xtask lint-extended, a diff-scoped heuristic checker for low-effort-code patternsclippy/rustfmtcan't catch structurally: leftoverTODO/FIXME/XXX/HACKmarkers and commented-out code (blocking), plus narrating "what the code does" comments, diff-local literal repetition, weak/generic identifier names, and new clippy suppressions (warnings, non-blocking).forgeinto a minimal 2-member Cargo workspace (existingforgebinary + newxtaskdev-tooling binary) rather than a standalone Python script, so the check is built, clippy'd, and tested with the same toolchain as the rest of the repo.extended-lintCI job (nowcargo xtask lint-extendedinstead ofpython3 scripts/extended-lint.py), and adds--workspaceto theclippy/test/docjobs and--alltofmtso the newxtaskmember is covered by every other CI job too.Fixes #5
Why a
cargo xtaskworkspace member instead of a Python script or an in-crate binaryThis repo is Rust-native end to end (
clippy,rustfmt,cargo test,cargo docall gate CI); a Python script sat outside all of that — no compiler or lint coverage of its own, a second interpreter/toolchain dependency in CI, and no access to the crate's own regex/anyhow-style tooling conveniences.Two Rust-native shapes were possible:
[workspace] members = ["xtask"]to the existing rootCargo.toml(which keeps its own[package]table, soforgebecomes the workspace's root package automatically — no need to relocate any existingsrc/).xtaskis a separate package with its ownCargo.toml,anyhow/regexdependencies, and lint config (vialints.workspace = trueagainst a promoted[workspace.lints]table).[[bin]]target inside the existingforgecrate.(1) is the lighter-weight, more idiomatic fit and is what this PR implements, for two concrete reasons:
xtaskneedsanyhowandregex; neither is aforgeruntime dependency. A same-crate[[bin]]would force those intoforge's own[dependencies], and thus into thepraxis-forgerelease binary's build graph, unless carefully feature-gated. A separate workspace member keeps them scoped to the dev-tooling binary only.cargo xtaskpattern (the convention popularized by matklad and used by e.g. rust-analyzer): a workspace member invoked via a.cargo/config.tomlalias (cargo xtask <task>→cargo run --locked --package xtask --), so future dev-tooling tasks have an obvious, low-ceremony home instead of accreting more one-off[[bin]]targets or scripts.The workspace conversion itself is not invasive: the root package keeps its
Cargo.toml,src/layout, and singleCargo.lockexactly as before; the only structural change is one added[workspace]table and promoting the existing[lints.*]tables to[workspace.lints.*](withlints.workspace = trueon both members) soxtaskis held to the same ~90-lint deny bar asforgeitself.Design notes
origin/mainby default,EXTENDED_LINT_BASEenv override, or auto-detectedorigin/$GITHUB_BASE_REFin a PR run) — ported faithfully from the original design.BLOCKfails the job (TODO markers, commented-out code);WARNis printed for human review but does not fail (narrating comments, repeated literals without a named constant, weak identifiers, new#[allow(clippy::...)]/#[expect(clippy::...)]suppressions).xtask/src/lint_extended.rsexcludes itself from its own diff scan (via agit diffpathspec exclusion): its doc comments and unit tests legitimately quote the very marker words and comment-like syntax the heuristics look for (e.g. documenting the TODO check requires writing the word "TODO"). The original Python script never hit this because*.rsnever matched a.pyfile — porting to Rust makes the tool a scan target of itself, so the exclusion had to become explicit.missing_docs_in_private_items,unwrap_used/expect_used(regex compilation uses a single documented#[expect(clippy::unwrap_used, reason = "...")]helper for the handful of compile-time-constant patterns), andprint_stdout/print_stderr(explicitly#[expect]-annotated at the two reporting call sites, following the same pattern already used insrc/main.rs'sreport_error).Test plan
cargo clippy --locked --workspace --all-targets --features test-support -- -D warnings— clean.cargo fmt --all --check— clean.cargo test --locked --workspace --features test-support— all 273 existingforgetests plus 4 newxtaskunit tests pass.cargo doc --locked --workspace --no-deps(withRUSTDOCFLAGS=-D warnings) — clean.cargo xtask lint-extended origin/mainagainst this PR's own diff (clean, no blocking findings), then added a throwaway// TODO: ...line tosrc/lib.rs, confirmedcargo xtask lint-extendedcorrectly reported it as aBLOCKINGfinding and exited non-zero, then discarded the throwaway change before pushing (not included in the diff).extended-lintjob (and the now---workspace-scopedclippy/test/docjobs) on this PR itself.