diff --git a/.config/nextest.toml b/.config/nextest.toml index c9289301..4f144658 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -6,6 +6,17 @@ # the workflow runs those separately via `cargo test --doc`, mirroring the # `test` / `test-doc` split in the Makefile. +# nextest's built-in default is `fail-fast = true`, which would report only +# the first failing test. `cargo test` — what `make test` ran before #1120 — +# reported every failure in the first failing binary, so leaving the default +# in place would have made local runs strictly less informative than the +# command they replaced. That matters most for the bulk-snapshot refreshes +# described in AGENTS.md, where a grammar bump or a metric change is +# *expected* to move hundreds of tests at once and the whole point is to see +# the full set. +[profile.default] +fail-fast = false + [profile.ci] # Collect the full failure set in a single run rather than stopping at the # first failure, so the JUnit report is complete after one invocation. diff --git a/AGENTS.md b/AGENTS.md index 750c703e..bc51aead 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -199,8 +199,12 @@ Before considering a change done, run `make pre-commit` from the repo root. It is the canonical entry point for the full validation gate and runs, in one parallel pass: the cargo trio (`cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings` in both -default-features and `--all-features` flavours, `cargo test ---workspace --all-features`), `cargo doc --no-deps --workspace +default-features and `--all-features` flavours, the full test suite +via `make test` + `make test-doc` — `make test` runs `cargo nextest +run --workspace --all-features` when `cargo-nextest` is on PATH and +falls back to `cargo test --workspace --all-features --lib --bins +--tests` otherwise, and `make test-doc` covers the doctests nextest +cannot run), `cargo doc --no-deps --workspace --all-features` with `RUSTDOCFLAGS="-D warnings"`, `cargo +nightly udeps`, the markdown / TOML / shell / Makefile / GitHub Actions lint families, the man-page @@ -265,8 +269,9 @@ with `make self-scan-write-baseline-headroom` in the commit that moved the metric. A red gate on `main` traces directly to skipping this step. If GNU Make 4 or any of the optional tools (`taplo`, `rumdl`, -`shellcheck`, `shfmt`, `checkmake`, `actionlint`, `ruff`, `mypy`, `pyright`, -`maturin`) are unavailable, fall back to the raw cargo commands: +`shellcheck`, `shfmt`, `checkmake`, `actionlint`, `cargo-nextest`, +`ruff`, `mypy`, `pyright`, `maturin`) are unavailable, fall back to the +raw cargo commands: ```bash cargo fmt --all -- --check diff --git a/CHANGELOG.md b/CHANGELOG.md index a50afc62..0615d76f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,19 @@ for historical reference. ### Changed +- `make test` now runs the suite through `cargo-nextest` when it is + available, matching CI, and falls back to `cargo test` otherwise + (#1120). nextest schedules every binary's tests into one global pool + rather than finishing each test binary before starting the next. Set + `NEXTEST=` to force the fallback, or point it at a specific binary. + nextest's default profile disables fail-fast so a local run still + reports the whole failure set, as `cargo test` did. +- Corpus snapshot tests now assert an exact per-corpus file count, and + separately that each resolved file reached its snapshot assertion + (#1123). A traversal or glob change that silently analyzed fewer files + previously still passed while verifying less than it claimed. A + deliberate corpus bump must update the expected count alongside the + snapshots. - **(breaking)** `FuncSpace`, `Ops`, `AstNode`, `wire::FuncSpace`, and `wire::Ops` now implement `Drop` (#1056), so fields can no longer be moved out of one by value: `let m = space.metrics;` becomes @@ -140,6 +153,27 @@ for historical reference. ### Performance +- `#[cfg(...)]` predicate classification is now linear in the attribute + body rather than `O(len²)` on deeply nested predicates such as + `all(all(all(…test…)))` (#1105). Every operand previously rescanned the + whole remaining tail probing for a top-level comma; commas are now + bucketed by paren depth in one forward pass and each region queries its + own bucket. This path is reached from every Rust attribute under + `--exclude-tests` — which this repository's own `bca.toml` enables — so + a machine-generated or adversarial source file was a denial-of-service + vector. Classification behaviour is unchanged, verified against the + previous implementation over millions of generated predicates. The + depth-50 000 regression test drops from ~73 s to ~0.04 s. +- Dependencies now build at `opt-level = 1` under the dev and test + profiles (#1121). The tree-sitter runtime and every grammar are C/C++ + libraries compiled by `cc-rs`, which forwards Cargo's `OPT_LEVEL`, so + they were previously parsing at roughly half speed in test builds. + Workspace members are unaffected and stay fully debuggable; the cost is + a one-time dependency rebuild and a slower cold build. Deliberately 1 + rather than 2, which measured slower on the unit suite. +- Corpus snapshot tests size their worker pool from + `available_parallelism()` instead of a hardcoded 4 jobs, which had left + three consumer threads analyzing DeepSpeech's 1042 files (#1123). - The ancestor chain now reaches the per-language metric bodies, retiring the last per-node `Node::parent` calls in the walk (#1096). `Getter::get_op_type` (and its `_with_code` variant), `Abc::compute`, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e87549d..5a2ad51a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -49,7 +49,8 @@ gate. In one parallel pass it runs, among other stages: - `cargo fmt --check`. - `cargo clippy --workspace --all-targets -- -D warnings` in both default-features and `--all-features` flavours. -- `cargo test --workspace --all-features`. +- The full test suite via `make test` (`cargo-nextest` when present, + otherwise `cargo test`) plus `make test-doc` for doctests. - `cargo doc --no-deps --workspace --all-features` with `RUSTDOCFLAGS="-D warnings"`. - `cargo +nightly udeps`. diff --git a/Cargo.toml b/Cargo.toml index 983e0c25..44a02a4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -366,6 +366,36 @@ serde_yaml = "^0.9" ciborium = "^0.2" toml = "^1.1" +# Optimize dependencies in dev/test builds. The `"*"` glob excludes +# workspace members, so the crates under `[workspace].members` stay at +# opt-level 0 — quick to rebuild and to step through. It does cover the +# vendored `tree-sitter-*` grammar crates, which are `[workspace].exclude`d +# path dependencies rather than members; that is the point of the stanza. +# +# The motivation is specific to this workspace: the tree-sitter runtime and +# every grammar are C / C++ libraries built by `cc-rs`, which forwards +# Cargo's `OPT_LEVEL` for the package being built as the compiler's +# optimization flag (`-O` on GNU / Clang; cc maps 1 to `/O1` and 2 or 3 +# to `/O2` on MSVC, so the level tuned below is not identical on Windows). +# Left at 0, the parse and the YAML round-trip that together dominate the +# corpus snapshot suites both run roughly twice as slow. +# +# Deliberately 1 rather than 2 (#1121). At 2 the root-crate unit suite — +# thousands of tiny parses, where the extra inlining grows the hot code +# more than the optimization saves — measured consistently slower, while 1 +# left it within run-to-run noise and still captured the corpus win +# (`deepspeech_test` roughly halved). Per-config numbers and their spread +# are recorded in #1121; they are single-machine, so re-measure before +# changing this rather than trusting a remembered figure. +# +# Cost: a one-time full dependency rebuild, and a cold `cargo test +# --no-run` that is markedly slower than at opt-level 0. +[profile.dev.package."*"] +opt-level = 1 + +# A named override beats the `"*"` glob above, so these two keep the +# opt-level 3 insta's own docs recommend for snapshot diffing (added in +# 8ccffeb2). [profile.dev.package.insta] opt-level = 3 diff --git a/Makefile b/Makefile index 6b283073..9fb37ff6 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,35 @@ FIND_EXCLUDE := $(foreach dir,$(EXCLUDE_DIRS),! -path './$(dir)/*') # warnings on `$(2)`, e.g. $(call find-by-ext,md,). find-by-ext = $(if $(FD),$(FD) --extension $(1) $(FD_EXCLUDE) $(2),find . -name "*.$(1)" -type f $(FIND_EXCLUDE)) +# Test runner: prefer cargo-nextest, fall back to `cargo test` (#1120). +# `cargo test` runs the workspace's 72 test binaries strictly one after +# another — parallelism exists only *within* a binary, so the ten root +# integration binaries that hold a single `#[test]` each pin the whole +# machine to one core for their duration. nextest schedules every +# binary's tests into one global pool instead. CI already runs nextest +# (the `test` job in .github/workflows/ci.yml); this is the local half. +# +# The two paths execute an identical target set: nextest's default +# selection is exactly `--lib --bins --tests`, verified by diffing +# `cargo nextest list --workspace --all-features` against the same +# invocation with those flags spelled out (4,731 tests, plus the 10 +# `#[ignore]`d ones both runners skip). +# +# Expect a smaller wall-clock win than the scheduling change implies: +# one test (`cfg_predicate::tests::rust_attr_test_handles_deeply_nested_ +# cfg_without_overflow`, the O(len^2) blowup tracked in #1105) runs for +# ~78s and is now the critical path. The other 4,730 finish in ~17s +# together, so fixing #1105 is what converts this into a ~4x win. +# +# Held in a variable rather than probed inline with `command -v`, and used +# as the command itself, so it overrides the same way `FD` above does: +# `make test NEXTEST=` exercises the fallback without uninstalling +# anything, and `make test NEXTEST=/path/to/cargo-nextest` picks a +# specific build. A cargo subcommand binary takes its own name as the +# first argument, hence `$(NEXTEST) nextest run`. +NEXTEST := $(shell command -v cargo-nextest 2>/dev/null) +TEST_CMD = $(if $(NEXTEST),$(NEXTEST) nextest run --workspace --all-features,cargo test --workspace --all-features --lib --bins --tests) + .PHONY: help check-tools build build-release check test test-doc fmt fmt-check markdown-fmt markdown-lint shellcheck sh-fmt sh-fmt-check toml-fmt toml-fmt-check toml-lint makefile-check actionlint snapshot-anchors grammar-marker-sync grammar-marker-sync-test check-versions check-manpage-assets enums-check enums-codegen-drift enums-codegen-drift-test self-scan self-scan-headroom self-scan-write-baseline self-scan-write-baseline-headroom vcs lint clippy udeps insta-review insta-accept clean distclean install install-cli install-web doc doc-open doc-check doc-check-docsrs book book-serve book-pot book-po-update book-ja book-deploy all pre-commit ci release-check verify-changelog pkg-deb-local pkg-rpm-local dev-env-build dev-env-run dev-env-shell dev-env-rm py-bootstrap py-sync py-relock py-clean py-fmt py-fmt-check py-lint py-typecheck py-test py-stubtest smoke smoke-cli smoke-lib bench bench-scaling bench-walk _check-find _pc-fmt _pc-clippy _pc-test _pc-doc-check _pc-udeps _pc-shellcheck _pc-markdown-lint _pc-toml-lint _pc-makefile-check _pc-actionlint _pc-snapshot-anchors _pc-grammar-marker-sync _pc-grammar-marker-sync-test _pc-check-versions _pc-check-versions-test _pc-check-grammar-crate-test _pc-check-manpage-assets _pc-enums-check _pc-enums-codegen-drift _pc-enums-codegen-drift-test _pc-self-scan _pc-self-scan-headroom _pc-py-fmt _pc-py-typecheck _pc-py-test _pc-py-stubtest _ci-fmt-check _ci-clippy _ci-test _ci-doc-check _ci-build _ci-udeps _ci-shellcheck _ci-markdown-lint _ci-toml-lint _ci-makefile-check _ci-actionlint _ci-snapshot-anchors _ci-grammar-marker-sync _ci-grammar-marker-sync-test _ci-check-versions _ci-check-versions-test _ci-check-grammar-crate-test _ci-check-manpage-assets _ci-enums-check _ci-enums-codegen-drift _ci-enums-codegen-drift-test _ci-enums-codegen-drift-test _ci-self-scan _ci-self-scan-headroom _ci-cargo-pipeline _ci-py-fmt-check _ci-py-lint _ci-py-typecheck _ci-py-test _ci-py-stubtest # Default target @@ -76,7 +105,7 @@ help: @echo " check Run cargo check" @echo "" @echo "Test targets:" - @echo " test Run unit and integration tests" + @echo " test Run unit and integration tests (nextest if present)" @echo " test-doc Run cargo doc tests" @echo " insta-review Review pending insta snapshot diffs" @echo " insta-accept Accept all pending insta snapshots" @@ -189,11 +218,22 @@ check: # Test # --------------------------------------------------------------------------- test: - cargo test --workspace --all-features --lib --bins --tests + @if [ -z "$(NEXTEST)" ]; then \ + echo "cargo-nextest not found or disabled; using 'cargo test' (same tests, slower)."; \ + echo "Install with: cargo install --locked cargo-nextest"; \ + fi + $(TEST_CMD) +# Doctests stay on `cargo test`: nextest cannot run them at all, so this +# is a separate target rather than an oversight (see .config/nextest.toml). test-doc: cargo test --workspace --all-features --doc +# `cargo insta test` shells out to `cargo test`, not $(TEST_CMD): insta's +# nextest integration needs `--test-runner nextest`, and under it insta +# cannot force-pass a failing assertion to collect every pending snapshot +# in one run — which is exactly what review/accept need. Snapshot review +# is an interactive, non-hot-loop operation, so it keeps the slower runner. insta-review: cargo insta test --review @@ -1021,8 +1061,8 @@ _pc-clippy: _pc-fmt cargo clippy --workspace --all-targets --all-features -- -D warnings _pc-test: _pc-clippy - cargo test --workspace --all-features --lib --bins --tests - cargo test --workspace --all-features --doc + $(MAKE) test + $(MAKE) test-doc _pc-doc-check: _pc-test $(MAKE) doc-check @@ -1160,9 +1200,17 @@ _ci-clippy: cargo clippy --workspace --all-targets -- -D warnings cargo clippy --workspace --all-targets --all-features -- -D warnings +# Mirrors the `test` job in .github/workflows/ci.yml (nextest for the +# lib/bin/integration tests, `cargo test --doc` for doctests). The +# workflow additionally passes `--profile ci --locked`. Neither changes +# the executed test set: the `ci` profile differs from `default` only in +# emitting JUnit XML for the PR annotation step (both disable fail-fast, +# see .config/nextest.toml), and `--locked` is a lockfile assertion that +# every `_ci-*` target omits — `make ci` will not catch a `Cargo.lock` +# that CI's `--locked` rejects. _ci-test: - cargo test --workspace --all-features --lib --bins --tests - cargo test --workspace --all-features --doc + $(MAKE) test + $(MAKE) test-doc _ci-doc-check: $(MAKE) doc-check diff --git a/big-code-analysis-book/src/developers/README.md b/big-code-analysis-book/src/developers/README.md index 8fc224bd..9da01ba0 100644 --- a/big-code-analysis-book/src/developers/README.md +++ b/big-code-analysis-book/src/developers/README.md @@ -31,16 +31,17 @@ The repository ships a `Makefile` that wraps every common build, test, lint, format, and docs task. Run `make help` to see the full list of targets, and `make check-tools` to verify which optional tools (`taplo`, `rumdl`, `shellcheck`, `shfmt`, `checkmake`, -`mdbook`, `cargo-insta`, `cargo-udeps`) are present on your machine. +`mdbook`, `cargo-insta`, `cargo-udeps`, `cargo-nextest`) are present on +your machine. The two composite targets you will use most: - `make pre-commit` — the recommended local gate before committing. Runs `cargo fmt --check`, both clippy invocations - (default-features and `--all-features`), `cargo test --workspace - --all-features` (lib + bin + integration + doc), `cargo +nightly - udeps`, and the markdown / TOML / shell / Makefile lint families - in one parallel pass. + (default-features and `--all-features`), the full test suite via + `make test` + `make test-doc` (lib + bin + integration + doc), + `cargo +nightly udeps`, and the markdown / TOML / shell / Makefile + lint families in one parallel pass. - `make ci` — the same checks in the order CI runs them, with no auto-fixing. Use this to reproduce a failing CI run locally. @@ -74,10 +75,31 @@ type-checking during iteration. To verify that all tests pass: ```console -make test # cargo test --workspace --all-features --lib --bins --tests +make test # cargo nextest run --workspace --all-features make test-doc # cargo test --workspace --all-features --doc ``` +`make test` prefers [cargo-nextest](https://nexte.st/), which is also +what CI runs. Plain `cargo test` finishes each test binary before +starting the next, so parallelism exists only within a binary; nextest +schedules every binary's tests into one global pool. Install it with +`cargo install --locked cargo-nextest`. + +Without it, `make test` falls back to `cargo test --workspace +--all-features --lib --bins --tests`, which runs the same set of tests. +The two runners are not quite interchangeable, though: nextest runs each +test in its own process, so anything sharing state across tests within a +binary — a `OnceLock` cache, an environment variable, the working +directory — is isolated under nextest and shared under `cargo test`. +Preferring nextest locally means local runs match CI on that axis. + +Set `NEXTEST=` to force the fallback without uninstalling anything +(`make test NEXTEST=`), or point it at a specific binary +(`make test NEXTEST=/path/to/cargo-nextest`). + +Doctests are a separate target because nextest cannot run them. +`make pre-commit` and `make ci` run both. + If you only want to run the cargo command yourself: ```console diff --git a/src/cfg_predicate.rs b/src/cfg_predicate.rs index 23132fad..6efb8bc4 100644 --- a/src/cfg_predicate.rs +++ b/src/cfg_predicate.rs @@ -12,6 +12,8 @@ //! entry point is [`attribute_marks_test`]; everything else is module- //! private. +use std::{iter, ops::Range}; + /// Return `true` if the Rust attribute body marks the annotated item /// as test-only. /// @@ -68,6 +70,73 @@ fn cfg_inner(body: &str) -> Option<&str> { Some(inner) } +/// Byte offsets of every comma in a cfg predicate, bucketed by the +/// paren nesting depth the comma sits at. +/// +/// A predicate is classified one *region* at a time: first the whole +/// predicate, then the argument list of every `all(...)` / `any(...)` +/// operand found inside it. A comma splits a region into operands +/// exactly when it sits at that region's own nesting depth, and a +/// region's depth is always its nesting level — a region begins right +/// after the `(` of an operand that itself starts at the parent +/// region's depth, so each descent adds exactly one. That makes the +/// depth a comma is recorded at directly comparable to the depth of +/// the region asking about it, so a single forward scan indexes the +/// split points of every region at once. +/// +/// Before issue #1105 each region instead re-scanned its whole +/// interior just to learn whether it held a top-level comma, so a +/// predicate nested `d` levels deep was scanned `d` times over — +/// O(len²) in the attribute body, and a denial-of-service vector for +/// any `exclude_tests` run over machine-generated Rust. +struct CommaIndex { + /// `(depth, offset)` pairs in ascending order, so the commas that + /// split any one region occupy a contiguous run. Ordering by depth + /// before offset is load-bearing — it is what groups a region's + /// split points together for [`CommaIndex::splits`]. + entries: Vec<(usize, usize)>, +} + +impl CommaIndex { + /// Index every comma in `pred` by the paren depth it appears at. + /// + /// Depth is a signed running count, matching the split rule this + /// replaced: an unbalanced `)` drives it negative and a later `(` + /// brings it back up. A comma stranded at a negative depth belongs + /// to no region and is dropped. + fn build(pred: &str) -> Self { + let mut entries = Vec::new(); + let mut depth = 0_isize; + for (offset, byte) in pred.bytes().enumerate() { + match byte { + b'(' => depth += 1, + b')' => depth -= 1, + b',' => { + if let Ok(comma_depth) = usize::try_from(depth) { + entries.push((comma_depth, offset)); + } + } + _ => {} + } + } + entries.sort_unstable(); + Self { entries } + } + + /// Offsets of the commas that split `region`, whose own operands + /// sit at `depth`. + fn splits(&self, region: &Range, depth: usize) -> impl Iterator { + let first = self + .entries + .partition_point(|entry| *entry < (depth, region.start)); + let end = region.end; + self.entries[first..] + .iter() + .take_while(move |(entry_depth, offset)| *entry_depth == depth && *offset < end) + .map(|(_, offset)| *offset) + } +} + /// Return `true` if the cfg predicate `pred` marks the item as /// test-only. /// @@ -80,93 +149,108 @@ fn cfg_inner(body: &str) -> Option<&str> { /// keeps live state on the heap, so nesting depth is bounded by /// available memory rather than the call-frame limit. /// -/// Each operand is classified exactly as the former recursive trio -/// did: bare `test` matches; `not(...)` short-circuits without -/// descending (its contents never mark the item test-only); `all(...)` -/// / `any(...)` push their operands; a bare top-level comma list is -/// treated like `all(...)`; everything else (plain idents, `feature = -/// "test"` and other key=value pairs) neither matches nor descends. +/// Every operand is classified by [`classify_cfg_operand`]; the +/// [`CommaIndex`] turns "where does this region split" into a lookup +/// instead of a rescan, so the whole walk is linear in `pred` up to +/// the index sort (issue #1105). fn cfg_predicate_marks_test(pred: &str) -> bool { - let mut stack = vec![pred]; - while let Some(operand) = stack.pop() { - let trimmed = operand.trim(); - if trimmed == "test" { - return true; - } + let commas = CommaIndex::build(pred); + // Regions still to classify, as `(byte range, nesting depth)`. Both + // this and the index are bounded by `pred`'s length, so a deeper + // predicate costs proportionally more memory, never more per byte. + let mut stack = vec![(0..pred.len(), 0_usize)]; + while let Some((region, depth)) = stack.pop() { // Bare comma-separated predicate lists like `cfg(test, foo)` // — pre-#278 callers relied on this form being treated as - // `cfg(all(test, foo))`. This MUST run before the - // single-operand `not`/`all`/`any` prefix checks below: those - // checks classify by the operand's leading prefix and trailing - // `)`, which only describe a single operand. For a list whose - // first operand is `not(...)` and last ends in `)` — e.g. - // `not(foo), all(test)` — `strip_prefix("not")` leaves - // `(foo), all(test)`, which both starts with `(` and ends with - // `)`, so the `not` short-circuit would otherwise swallow the - // whole list and drop the trailing `test` (regression for #763). - // `cfg_split_top_level_args` respects paren depth, so a comma - // nested inside a predicate's own parens — `not(foo, bar)`, - // `all(test, unix)` — is a single operand and falls through to - // the prefix checks unchanged. - if cfg_split_top_level_args(trimmed).nth(1).is_some() { - stack.extend(cfg_split_top_level_args(trimmed)); - continue; - } - // `not(...)` short-circuits: we do not look inside, because - // `not(test)` excludes the item from test builds. Drop it - // without pushing its contents. - if let Some(rest) = trimmed.strip_prefix("not").map(str::trim_start) - && rest.starts_with('(') - && rest.ends_with(')') - { - continue; - } - // `all(...)` and `any(...)` use the same "contains a `test` - // operand" rule here. Strictly, `any(test, foo)` is over-broad - // (the item is included in production when `foo` holds), but the - // pre-#278 code treated both identically and the issue spec - // preserves that behavior. Push each operand for later inspection. - if let Some(rest) = trimmed - .strip_prefix("all") - .or_else(|| trimmed.strip_prefix("any")) - && let Some(args) = rest.trim_start().strip_prefix('(') - && let Some(args) = args.strip_suffix(')') - { - stack.extend(cfg_split_top_level_args(args)); + // `cfg(all(test, foo))`. Splitting the region MUST happen + // before an operand meets the `not`/`all`/`any` prefix checks: + // those classify by leading prefix and trailing `)`, which only + // describe a single operand. For a list whose first operand is + // `not(...)` and last ends in `)` — e.g. `not(foo), all(test)` + // — `strip_prefix("not")` leaves `(foo), all(test)`, which both + // starts with `(` and ends with `)`, so the `not` short-circuit + // would otherwise swallow the whole list and drop the trailing + // `test` (regression for #763). The index respects paren depth, + // so a comma nested inside a predicate's own parens — + // `not(foo, bar)`, `all(test, unix)` — is not a split point and + // the operand reaches the prefix checks intact. + // + // The region's end acts as a final split point, so the operand + // after the last comma — or the whole region, when there is no + // comma at this depth — goes through the same classification. + let mut operand_start = region.start; + for boundary in commas.splits(®ion, depth).chain(iter::once(region.end)) { + match classify_cfg_operand(pred, operand_start..boundary) { + Operand::Test => return true, + Operand::Args(args) => stack.push((args, depth + 1)), + Operand::Opaque => {} + } + operand_start = boundary + 1; } } false } -/// Iterator over the comma-separated arguments of a cfg predicate -/// body, splitting at top-level commas only (commas inside nested -/// parens belong to a child predicate). Single-pass byte scan. -fn cfg_split_top_level_args(args: &str) -> impl Iterator { - let mut depth = 0_i32; - let mut start = 0_usize; - let mut done = false; - let bytes = args.as_bytes(); - std::iter::from_fn(move || { - if done { - return None; - } - let mut i = start; - while i < bytes.len() { - match bytes[i] { - b'(' => depth += 1, - b')' => depth -= 1, - b',' if depth == 0 => { - let slice = &args[start..i]; - start = i + 1; - return Some(slice); - } - _ => {} - } - i += 1; - } - done = true; - Some(&args[start..]) - }) +/// How one operand of a cfg predicate classifies. +enum Operand { + /// The bare `test` predicate: the item is test-only. + Test, + /// The argument list of an `all(...)` / `any(...)` operand, as a + /// byte range of the predicate, to be walked one level deeper. + Args(Range), + /// Neither matches nor descends: `not(...)`, plain idents, + /// `feature = "test"` and other key/value pairs. + Opaque, +} + +/// Classify one operand of a cfg predicate, given as a byte range of +/// `pred`. +fn classify_cfg_operand(pred: &str, operand: Range) -> Operand { + let raw = &pred[operand.start..operand.end]; + let trimmed = raw.trim(); + if trimmed == "test" { + return Operand::Test; + } + // `not(...)` short-circuits: we do not look inside, because + // `not(test)` excludes the item from test builds (#278). Kept as an + // explicit arm even though it is currently redundant — an operand + // starting with `not` cannot match the `all`/`any` prefixes below, + // so it would fall through to `Opaque` anyway. Deleting it would + // make the #278 rule an emergent property of a prefix test three + // lines down; stating it here keeps the rule visible if that test is + // ever loosened. + if trimmed + .strip_prefix("not") + .map(str::trim_start) + .is_some_and(|rest| rest.starts_with('(') && rest.ends_with(')')) + { + return Operand::Opaque; + } + // `all(...)` and `any(...)` use the same "contains a `test` + // operand" rule here. Strictly, `any(test, foo)` is over-broad (the + // item is included in production when `foo` holds), but the + // pre-#278 code treated both identically and the issue spec + // preserves that behavior. + // + // The three conditions are one shape check, so they read as one + // chain: combinator name, then its opening paren, then a closing + // paren as the operand's *last* byte. That last byte is not + // necessarily the opening paren's match — `all(a)(b)` has argument + // list `a)(b`, and the walk must reproduce that. + if let Some(rest) = trimmed + .strip_prefix("all") + .or_else(|| trimmed.strip_prefix("any")) + && let Some(inside) = rest.trim_start().strip_prefix('(') + && let Some(args) = inside.strip_suffix(')') + { + // `raw.trim_end()` starts where `raw` does, so the trimmed + // operand ends at `operand.start + raw.trim_end().len()`. + // `inside` is a suffix of that, and `args` a prefix of `inside`, + // so both map back onto `pred` by length alone. + let args_start = operand.start + raw.trim_end().len() - inside.len(); + return Operand::Args(args_start..args_start + args.len()); + } + Operand::Opaque } #[cfg(test)] @@ -335,6 +419,188 @@ mod tests { ); } + #[test] + fn cfg_predicate_classification_matches_pre_1105_walker() { + // Issue #1105 replaced the pop-and-rescan predicate walker with + // a comma index plus one classification pass. Every expectation + // below was produced by running the *pre-#1105* walker over the + // input, then transcribed here, so the table pins the exact + // behaviour the rewrite had to preserve — including the corners + // no hand-written test covered: unbalanced parens, empty and + // whitespace-only operands, `test` as a substring, and the + // long-standing blind spot that parens and commas inside string + // literals are counted as structure. + // + // The rewrite was additionally checked against the old walker + // over millions of generated predicates with zero disagreements; + // this table is the cheap, checked-in residue of that run. Note + // the generator alphabet has to be able to spell `test`: + // `trimmed == "test"` is the only check in either implementation + // that can return `true`, so a sweep over an alphabet without + // `e` and `s` agrees trivially on every input and proves nothing + // about the bug class that matters (a missed match). + let cases: &[(&str, bool)] = &[ + // Unbalanced or truncated parens: the `all(...)` shape check + // needs the operand's *last* byte to be `)`, so trailing + // junk or a missing paren drops the whole operand. + ("all(test", false), + ("all(test))", false), + ("all((test)", false), + // `all(test)(x)` is the load-bearing member of this pair: + // under matching-paren semantics it would be `true`. Its + // neighbour classifies the same either way — keep both, but + // do not drop this one. + ("all(test)(x)", false), + ("all(a)(test)", false), + ("all(test)x", false), + (")test", false), + ("test)", false), + ("(test", false), + // A stray `)` drives the depth counter negative. A comma at + // negative depth belongs to no region and is dropped, so it + // splits nothing: `a),test` is one dead operand rather than + // two. Clamping the depth at 0 instead would make that comma + // a top-level split and wrongly surface the trailing `test`, + // so this row is the end-to-end guard on the signed counter. + ("a),test", false), + // These two are *not* negative-depth cases despite the stray + // `)`: the following `(` restores depth to 0, so the comma + // does split. They are false because the second operand ends + // in a trailing paren. Kept for the last-byte rule, not the + // depth rule. + ("all(a))(b, test)", false), + ("all(a))(b, all(test))", false), + ("any(test", false), + // Empty and whitespace-only operands. + ("", false), + (" ", false), + ("all()", false), + ("any()", false), + ("not()", false), + ("all( )", false), + ("all(,)", false), + (",", false), + (",,", false), + ("all(,test)", true), + ("all(test,)", true), + ("all(test,,)", true), + // `test` as a substring of another identifier must not match. + ("testing", false), + ("not_test", false), + ("x_test", false), + ("alltest", false), + ("nottest", false), + ("anytest", false), + ("all(testing)", false), + ("all(x_test, testing)", false), + // String literals are not lexed: a `(` or `,` inside one + // still moves the depth counter. `all(v = "(", test)` is + // therefore read as one operand and misses the `test`. This + // is pre-existing behaviour, pinned here so a future lexer + // change is a deliberate decision rather than a surprise. + ("feature = \"test\"", false), + ("all(feature = \"test\")", false), + ("any(v = \"a,b\")", false), + ("all(v = \"(\", test)", false), + ("all(v = \"r#\\\"test(\\\"#\")", false), + // Only `all` / `any` descend; every other combinator, `cfg` + // and `cfg_attr` included, is an opaque operand. + ("cfg_attr(test, derive(Debug))", false), + ("cfg(test)", false), + ("all(cfg(test))", false), + // `not(...)` short-circuits at any depth, even around a + // combinator that would otherwise match. + ("not(all(test))", false), + ("not(any(test))", false), + ("not(not(test))", false), + ("all(not(test), test)", true), + ("any(not(test), unix)", false), + // Whitespace between the combinator and its parens, and + // around operands, is tolerated. + (" all ( test ) ", true), + ("all\t(test)", true), + ("all\n(\ntest\n)", true), + ("not (test)", false), + ("all( unix , test )", true), + // Non-ASCII operands neither match nor break byte offsets. + ("all(é, test)", true), + ("all(日本語)", false), + ("тест", false), + ("all(тест, test)", true), + // Ordinary nesting. + ("all(all(all(test)))", true), + ("any(all(any(test)))", true), + ("all(any(unix), test)", true), + ("all(a, b, c, test)", true), + ("all(a, b, c, unix)", false), + // Sibling regions at the same depth, where the *earlier* + // sibling contains a comma. Nothing else in this table has + // that shape, and without it the lower bound of the index + // lookup is never varied: dropping it leaves every other row + // passing while these panic on an inverted slice range. + ("any(all(unix, test), all(windows, foo))", true), + ("all(all(a,b), all(c,d))", false), + // A top-level comma list whose `test` follows a nested + // region. The index must be ordered for the trailing operand + // to be reached at all. + ("a,all(b,c),test", true), + ]; + for &(pred, expected) in cases { + assert_eq!( + cfg_predicate_marks_test(pred), + expected, + "predicate {pred:?} must classify as {expected}" + ); + } + } + + #[test] + fn comma_index_buckets_by_paren_depth() { + // The index is what makes classification linear: a region asks + // for the commas at its own nesting depth instead of rescanning + // its interior. Seed a predicate whose commas sit at three + // different depths so each bucket is distinguishable from the + // others and from an empty one. + let pred = "a,all(b,c),any(d,all(e,f))"; + let index = CommaIndex::build(pred); + + let depth0: Vec = index.splits(&(0..pred.len()), 0).collect(); + assert_eq!(depth0, vec![1, 10], "commas outside any parens"); + // `all(b,c)` spans 2..10; its argument list 6..9 holds one + // depth-1 comma, and the depth-1 comma of `any(...)` is outside + // that range and must not leak in. + let args: Vec = index.splits(&(6..9), 1).collect(); + assert_eq!(args, vec![7], "only the commas inside this region"); + let depth1: Vec = index.splits(&(0..pred.len()), 1).collect(); + assert_eq!(depth1, vec![7, 16], "both depth-1 commas"); + let depth2: Vec = index.splits(&(0..pred.len()), 2).collect(); + assert_eq!(depth2, vec![22], "the comma inside the inner all()"); + assert!( + index.splits(&(0..pred.len()), 3).next().is_none(), + "no region nests three deep here" + ); + + // A `)` with no opener drives the depth counter negative, so the + // comma that follows splits nothing and is dropped entirely — + // the behaviour the former `cfg_split_top_level_args` had, and + // what makes `a),test` a single dead operand. + // + // Seeded with a depth-0 comma *before* the stray `)` so the + // assertion distinguishes "dropped the negative-depth comma" + // from "recorded no commas at all"; against an empty index both + // readings look identical (.claude/rules/testing.md). + let stray = CommaIndex::build("x,y),z"); + assert_eq!( + stray.entries, + vec![(0, 1)], + "a comma at negative depth belongs to no region" + ); + // The following `(` brings the counter back to zero, restoring + // the comma as a top-level split point. + let restored = CommaIndex::build("a)(b,c"); + assert_eq!(restored.entries, vec![(0, 4)]); + } + #[test] fn strip_whitespace_preserves_non_ascii_utf8() { // Regression test for #312. The slow path previously rebuilt diff --git a/tests/README.md b/tests/README.md index 8c319667..4e2d9f49 100644 --- a/tests/README.md +++ b/tests/README.md @@ -79,15 +79,25 @@ accepted YAML snapshots in `tests/repositories/big-code-analysis-output/snapshots//`. All five share the same driver: `tests/common/mod.rs::act_on_file`, -called concurrently via `ConcurrentRunner` (4 jobs) and dispatched by +called concurrently via `ConcurrentRunner` with one consumer thread per +core (sized from `available_parallelism()`, #1123), and dispatched by one of: -- `compare_rca_output_with_files(repo, include, exclude)` — corpus is - a sibling of `big-code-analysis-output/` under `tests/repositories/` -- `compare_rca_output_with_files_under(source_root, repo, …)` — corpus - lives *inside* the `big-code-analysis-output` submodule (needed for - C# and PHP so snapshot paths don't pick up the submodule directory - as an extra component) +- `compare_rca_output_with_files(repo, include, exclude, expected_files)` + — corpus is a sibling of `big-code-analysis-output/` under + `tests/repositories/` +- `compare_rca_output_with_files_under(source_root, repo, …, expected_files)` + — corpus lives *inside* the `big-code-analysis-output` submodule + (needed for C# and PHP so snapshot paths don't pick up the submodule + directory as an extra component) + +`expected_files` is the exact number of files the corpus must resolve +to, asserted twice: once against the resolved path list and once +against the number of snapshot assertions actually executed. The two +differ when a file resolves but `act_on_file` returns early on it, so +both are needed to make a silent coverage loss loud. **After a +deliberate corpus bump, update this count alongside the snapshots** — +a legitimate corpus growth is expected to fail these assertions first. Floats are rounded to 3 decimal places before comparison (machine portability) and the `name` field is redacted to `[filepath]` (path diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3f0cc4c6..6a6b7cca 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -10,8 +10,11 @@ clippy::too_many_lines )] +use std::num::NonZeroUsize; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; use globset::{Glob, GlobSet, GlobSetBuilder}; use walkdir::{DirEntry, WalkDir}; @@ -43,6 +46,14 @@ const SNAPSHOT_PATH: &str = concat!( struct Config { language: Option, source_root: PathBuf, + /// Files that reached the snapshot assertion. + /// + /// Counting resolved paths is not the same as counting assertions: + /// `act_on_file` returns `Ok(())` early for a file too short to read + /// and for one whose language cannot be guessed, so a resolved file + /// can still silently skip its snapshot. Shared across the runner's + /// consumer threads, hence the atomic. + snapshotted: Arc, } fn act_on_file(path: PathBuf, cfg: &Config) -> std::io::Result<()> { @@ -76,6 +87,8 @@ fn act_on_file(path: PathBuf, cfg: &Config) -> std::io::Result<()> { ) .expect("analyze returned Err for fixture; the parser may have rejected the source"); + cfg.snapshotted.fetch_add(1, Ordering::Relaxed); + insta::with_settings!({snapshot_path => Path::new(SNAPSHOT_PATH) .join(path.strip_prefix(&cfg.source_root).unwrap()) .parent() @@ -102,9 +115,23 @@ fn act_on_file(path: PathBuf, cfg: &Config) -> std::io::Result<()> { } /// Produces metrics runtime and compares them with previously generated json files +/// +/// `expected_files` is the number of files the corpus root must resolve to. +/// Asserting it keeps a shrinking corpus from reading as a passing test. #[allow(dead_code)] -pub fn compare_rca_output_with_files(repo_name: &str, include: &[&str], exclude: &[&str]) { - compare_rca_output_with_files_under(Path::new(REPO), repo_name, include, exclude); +pub fn compare_rca_output_with_files( + repo_name: &str, + include: &[&str], + exclude: &[&str], + expected_files: usize, +) { + compare_rca_output_with_files_under( + Path::new(REPO), + repo_name, + include, + exclude, + expected_files, + ); } /// Same as [`compare_rca_output_with_files`] but with an explicit source root. @@ -115,18 +142,38 @@ pub fn compare_rca_output_with_files(repo_name: &str, include: &[&str], exclude: /// `big-code-analysis-output` submodule (as for the synthetic PHP corpus) so /// snapshots land at `snapshots//...` rather than picking up the /// submodule directory as an extra path component. +/// +/// `expected_files` is the number of files the corpus root must resolve to. +/// Asserting it keeps a shrinking corpus from reading as a passing test. #[allow(dead_code)] pub fn compare_rca_output_with_files_under( source_root: &Path, repo_name: &str, include: &[&str], exclude: &[&str], + expected_files: usize, ) { - let num_jobs = 4; + // `ConcurrentRunner` reserves one job for the producer thread + // (`max(2, n) - 1` consumers), so ask for one more than the machine's + // parallelism to get one consumer per core. The previous hardcoded `4` + // left 3 consumers analyzing DeepSpeech's 1042 files while the rest of + // the box idled. + // + // Several corpus tests can run concurrently under nextest, each with + // its own runner, so this nominally oversubscribes. Measured, it does + // not cost anything: only DeepSpeech runs longer than ~1.2s, so the + // overlap window is short, and the small corpora leave most of their + // consumers parked on an empty queue. + // Written as `consumers + 1` so the fallback reads as a consumer count + // too: an unavailable parallelism figure keeps the previous hardcoded + // behaviour of 3 consumers rather than silently assuming 4 cores. + let num_jobs = std::thread::available_parallelism().map_or(3, NonZeroUsize::get) + 1; + let snapshotted = Arc::new(AtomicUsize::new(0)); let cfg = Config { language: None, source_root: source_root.to_path_buf(), + snapshotted: Arc::clone(&snapshotted), }; let mut gsbi = GlobSetBuilder::new(); @@ -149,17 +196,26 @@ pub fn compare_rca_output_with_files_under( let corpus_root = source_root.join(repo_name); let paths = resolve_corpus_files(&corpus_root, &include, &exclude); - // A corpus that resolves to zero files makes the runner return - // `Ok(())` without ever invoking `act_on_file`, so every snapshot - // assertion is silently skipped and the test passes having verified - // nothing (#938). The corpus lives in git submodules; an - // uninitialized submodule leaves the directory empty. Fail loudly - // with the likely cause rather than reporting false success. - assert!( - !paths.is_empty(), - "no files matched under {}; the integration corpus is empty or \ - missing. Initialize the submodules with `git submodule update \ - --init --recursive` before running corpus comparison tests.", + // A corpus that resolves to *fewer* files than expected makes the + // runner skip the missing files' snapshot assertions while still + // returning `Ok(())`, so the test passes having verified less than it + // claims. Zero files is the degenerate case (#938): an uninitialized + // submodule leaves the directory empty and every assertion is skipped. + // + // The corpora are submodules pinned to a fixed SHA, so the resolved + // count is deterministic for a given checkout. Asserting it exactly + // turns any silent change in corpus coverage — an over-eager exclude + // glob, a partially-initialized submodule, a change to the traversal + // filters — into a loud failure instead of a quiet coverage loss. A + // deliberate corpus bump updates this number alongside the snapshots. + assert_eq!( + paths.len(), + expected_files, + "unexpected corpus file count under {}. If it resolved 0 files the \ + integration corpus is empty or missing — initialize the submodules \ + with `git submodule update --init --recursive`. Otherwise the \ + corpus or the include/exclude globs changed; update the expected \ + count alongside the snapshots.", corpus_root.display(), ); @@ -171,6 +227,19 @@ pub fn compare_rca_output_with_files_under( // the binary's tests produce their own diagnostics. panic!("ConcurrentRunner failed: {e:?}"); } + + // Resolving a file is not the same as asserting its snapshot: the two + // early returns in `act_on_file` let a resolved file slip through + // without one. Close that gap so "the corpus shrank" and "a file + // stopped being analyzed" are both loud. + assert_eq!( + snapshotted.load(Ordering::Relaxed), + expected_files, + "resolved {expected_files} files under {} but only asserted a \ + snapshot for some of them; a file became unreadable or its \ + language stopped being guessable.", + corpus_root.display(), + ); } fn is_hidden(entry: &DirEntry) -> bool { diff --git a/tests/csharp_test.rs b/tests/csharp_test.rs index a5f07dd3..d199e19f 100644 --- a/tests/csharp_test.rs +++ b/tests/csharp_test.rs @@ -12,5 +12,5 @@ fn test_csharp() { .join("repositories") .join("big-code-analysis-output"); - compare_rca_output_with_files_under(&source_root, "csharp", &["*.cs"], &[]); + compare_rca_output_with_files_under(&source_root, "csharp", &["*.cs"], &[], 6); } diff --git a/tests/deepspeech_test.rs b/tests/deepspeech_test.rs index 0f5958ae..254e3a9d 100644 --- a/tests/deepspeech_test.rs +++ b/tests/deepspeech_test.rs @@ -27,5 +27,10 @@ fn test_deepspeech() { "**/DeepSpeech/kenlm/**", ]; - compare_rca_output_with_files("DeepSpeech", &["*.cc", "*.cpp", "*.h", "*.hh"], exclude); + compare_rca_output_with_files( + "DeepSpeech", + &["*.cc", "*.cpp", "*.h", "*.hh"], + exclude, + 1042, + ); } diff --git a/tests/pdf_js_test.rs b/tests/pdf_js_test.rs index 427dcf71..fdc8fd13 100644 --- a/tests/pdf_js_test.rs +++ b/tests/pdf_js_test.rs @@ -139,5 +139,5 @@ fn test_pdfjs() { "**/pdf.js/web/pdf_sidebar.js", ]; - compare_rca_output_with_files("pdf.js", &["*.js"], exclude); + compare_rca_output_with_files("pdf.js", &["*.js"], exclude, 266); } diff --git a/tests/php_test.rs b/tests/php_test.rs index 8d3d2131..392f89f1 100644 --- a/tests/php_test.rs +++ b/tests/php_test.rs @@ -12,5 +12,5 @@ fn test_php() { .join("repositories") .join("big-code-analysis-output"); - compare_rca_output_with_files_under(&source_root, "php", &["*.php"], &[]); + compare_rca_output_with_files_under(&source_root, "php", &["*.php"], &[], 6); } diff --git a/tests/serde_test.rs b/tests/serde_test.rs index b46a1a8c..5671ba41 100644 --- a/tests/serde_test.rs +++ b/tests/serde_test.rs @@ -5,5 +5,5 @@ use common::compare_rca_output_with_files; #[test] fn test_serde() { - compare_rca_output_with_files("serde", &["*.rs"], &[]); + compare_rca_output_with_files("serde", &["*.rs"], &[], 172); } diff --git a/utils/check-tools.sh b/utils/check-tools.sh index 8c872138..f0fbb294 100755 --- a/utils/check-tools.sh +++ b/utils/check-tools.sh @@ -113,6 +113,21 @@ else shfmt_missing=1 fi +echo "" +echo "Optional Tools (Test runner):" +echo " (optional because 'make test' falls back to 'cargo test', which runs" +echo " the same tests one binary at a time instead of in one global pool)" + +nextest_missing=0 +if command -v cargo-nextest >/dev/null 2>&1; then + nextest_version=$(cargo-nextest nextest --version 2>/dev/null | awk 'NR==1{print $2; exit}' || true) + nextest_version=${nextest_version:-unknown} + echo " ✓ cargo-nextest (version: $nextest_version)" +else + echo " ✗ cargo-nextest (not found)" + nextest_missing=1 +fi + echo "" echo "Optional Tools (GitHub Actions linting):" @@ -187,7 +202,7 @@ fi echo "" core_missing=$((cargo_missing + nightly_missing + udeps_missing + insta_missing + checkmake_missing)) -optional_missing=$((rumdl_missing + fd_missing + taplo_missing + shellcheck_missing + shfmt_missing + actionlint_missing + mdbook_missing + uv_missing + ruff_missing + mypy_missing + pyright_missing + maturin_py_missing)) +optional_missing=$((rumdl_missing + fd_missing + taplo_missing + shellcheck_missing + shfmt_missing + nextest_missing + actionlint_missing + mdbook_missing + uv_missing + ruff_missing + mypy_missing + pyright_missing + maturin_py_missing)) if [ "$core_missing" -gt 0 ]; then echo "Missing core tools:" @@ -228,6 +243,10 @@ if [ "$optional_missing" -gt 0 ]; then if [ "$shfmt_missing" -eq 1 ]; then echo " - shfmt: Install from https://github.com/mvdan/sh/releases" fi + if [ "$nextest_missing" -eq 1 ]; then + echo " - cargo-nextest: Install with: cargo install --locked cargo-nextest" + echo " ('make test' still works without it, just slower)" + fi if [ "$actionlint_missing" -eq 1 ]; then echo " - actionlint: Download from https://github.com/rhysd/actionlint/releases or run 'go install github.com/rhysd/actionlint/cmd/actionlint@latest'" fi