From 6a2dc14ff0d57d3d497608619875bf327f186cf9 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 12 Sep 2026 18:39:28 -0700 Subject: [PATCH 01/12] test(metrics/loc): pin the multiline-string ploc branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three lines in `src/metrics/loc/shared.rs` could be perturbed without failing any of the ~3,330 lib tests: `add_multiline_string_ploc`'s parent gate in either direction, and `add_string_interior_ploc`'s `end_line().saturating_sub(1)` upper bound. None of the three is observable end-to-end. `LineSet::insert` and `check_comment_ends_on_code_line` are idempotent, and every language routing a literal through the helper ends its match with an unconditional catch-all that has already credited the literal's start row, so crediting it again changes nothing and skipping it drops nothing the catch-all does not restore. The bound differs from the raw end row only for a node whose end column is 0. The three new tests therefore call the helpers directly, seeding a row the assertions can be measured against rather than starting from a default-empty `Stats`. The bound test uses a Bash `heredoc_body` — the one literal here that ends at column 0 — at helper level rather than through `bca metrics`, so a change to what `bash.rs` routes (#1412) cannot silently disarm it. Verified by perturbation: always-credit fails only the parent-started test, never-credit fails only the parent-did-not-start test, flipping the comparison fails both, and swapping the bound for `end_row()` fails only the interior test. A control perturbation elsewhere in the same file failed 276 tests, so the harness discriminates. Fixes #1414 --- src/metrics/loc.rs | 179 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 179 insertions(+) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 41dd74c2..639d5f1e 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -8715,6 +8715,185 @@ $y = 10 + match ($x) { 1 => 2, default => 0 };", assert_eq!(loc.blank(), 0); } + /// The #1414 fixture for [`add_multiline_string_ploc`]'s parent gate: + /// two multi-row Rust literals, the first opening on row 1 under an + /// `array_expression` that started on row 0, the second opening on + /// row 4 alongside the `const_item` that owns it. + /// + /// Returned as a [`crate::Ast`] rather than as a pair of [`Node`]s + /// because the nodes borrow the held tree. + #[cfg(feature = "rust")] + fn multiline_string_gate_fixture() -> crate::Ast { + crate::test_support::parse_named( + LANG::Rust, + "lib.rs", + "const A: [&str; 1] = [\n \"a\nb\",\n];\nconst B: &str = \"c\nd\";\n", + ) + } + + /// Returns the `index`-th `string_literal` of + /// [`multiline_string_gate_fixture`], asserting the fixture still + /// holds both and that this one still spans `rows`. + /// + /// The span check is the fixture's own guard: a grammar bump that + /// re-shaped the literal would otherwise turn the row assertions + /// below into claims about some other node. + #[cfg(feature = "rust")] + fn multiline_string_gate_literal<'a>( + root: &Node<'a>, + index: usize, + rows: (usize, usize), + ) -> Node<'a> { + let literals = root.descendants_by_kind(&["string_literal"]); + assert_eq!( + literals.len(), + 2, + "fixture drift: expected exactly two string literals" + ); + let literal = literals[index]; + assert_eq!( + (literal.start_row(), literal.end_row()), + rows, + "fixture drift: literal {index} no longer spans the expected rows" + ); + literal + } + + /// [`add_multiline_string_ploc`] credits the opening row when the + /// parent started earlier — the branch #1414 measured as unguarded. + /// + /// End-to-end neither half of that gate is observable, which is why + /// this test is at the helper's level rather than through a + /// fixture. Every language routing a literal through this helper + /// ends its match with an unconditional catch-all that has already + /// inserted the literal's start row, and both `LineSet::insert` and + /// `check_comment_ends_on_code_line` are idempotent — so crediting + /// the row twice reads the same as crediting it once, and skipping + /// it drops nothing the catch-all does not put back. Before these + /// tests, removing the gate and inverting it each failed zero of + /// the ~3,330 lib tests. Only a call that reaches the helper with + /// the row still absent separates them. + #[cfg(feature = "rust")] + #[test] + fn multiline_string_credits_an_opening_row_its_parent_did_not_start() { + let ast = multiline_string_gate_fixture(); + let root = ast.root_node(); + let literal = multiline_string_gate_literal(&root, 0, (1, 2)); + + let mut stats = Stats::default(); + // Seed the row a real caller's catch-all would already hold, so + // nothing below can pass against a default-empty set. + stats.ploc.lines.insert(0); + + add_multiline_string_ploc( + &literal, + Ancestors::unknown(), + &mut stats, + literal.start_row(), + ); + + assert!(stats.ploc.lines.contains(0), "the seeded row must survive"); + assert!( + stats.ploc.lines.contains(1), + "the `array_expression` parent starts on row 0, so nothing but \ + this helper covers the literal's opening row" + ); + assert!( + stats.ploc.lines.contains(2), + "the literal's closing row is interior to it" + ); + assert!( + !stats.ploc.lines.contains(3), + "row 3 is `];`, past the literal" + ); + } + + /// The gate's other side: [`add_multiline_string_ploc`] leaves the + /// opening row alone when the parent started on it, and still runs + /// the interior insertion. See the sibling above for why this is + /// invisible end-to-end. + #[cfg(feature = "rust")] + #[test] + fn multiline_string_leaves_an_opening_row_its_parent_started() { + let ast = multiline_string_gate_fixture(); + let root = ast.root_node(); + let literal = multiline_string_gate_literal(&root, 1, (4, 5)); + + let mut stats = Stats::default(); + stats.ploc.lines.insert(0); + + add_multiline_string_ploc( + &literal, + Ancestors::unknown(), + &mut stats, + literal.start_row(), + ); + + assert!(stats.ploc.lines.contains(0), "the seeded row must survive"); + assert!( + !stats.ploc.lines.contains(4), + "the `const_item` parent starts on row 4 and already covers it, \ + so the gate must skip it" + ); + assert!( + stats.ploc.lines.contains(5), + "the interior insertion runs whichever way the gate goes" + ); + } + + /// [`add_string_interior_ploc`] stops at the last row the literal + /// *occupies*, not at its raw end row — the third #1414 branch, and + /// the one that helper's own doc comment argues for. + /// + /// The two spellings differ only for a node whose end column is 0, + /// and the one string literal known to have that shape here is a + /// Bash `heredoc_body`: it absorbs the newline after its last + /// content row and so ends at column 0 of the terminator's row. + /// Everywhere else a literal stops just past its closing delimiter, + /// where `end_line() - 1` and `end_row()` agree. + /// + /// The call is direct rather than through `bca metrics` + /// deliberately — which node `bash.rs` routes is a separate + /// question (#1412), and an end-to-end fixture would stop + /// discriminating the moment that answer changed. + #[cfg(feature = "bash")] + #[test] + fn string_interior_stops_at_the_last_row_the_literal_occupies() { + // rows: 0 `cat < Date: Sat, 12 Sep 2026 19:15:30 -0700 Subject: [PATCH 02/12] fix(metrics/loc): route Bash heredocs by their wrapper `heredoc_body`'s span starts at the first body row that has text and collapses to zero width when the body has none, so a leading empty row sat inside no node at all and `blank = sloc - ploc - cloc` claimed it: `cat < 7554` and `blank 795 -> 792` -- a clean swap with `sloc` and `cloc` unchanged. The two cross-language sweeps that are the home for this property used `line1\nline2\nline3`, which passes whether or not a textless interior row is credited; that is why they saw neither #1396 nor #1412. Their interior row is now empty (C excepted -- its multi-row literal is a backslash continuation, so it cannot express one), each gained the PHP rows it never had, quoted and heredoc alike, and both gained the `#[cfg(any(feature = ...))]` union that makes them absent rather than panicking under a feature subset (#1285). Fixes #1412 --- CHANGELOG.md | 18 +++ src/metrics/loc.rs | 269 +++++++++++++++++++++++++++++++++++----- src/metrics/loc/bash.rs | 68 +++++++--- 3 files changed, 311 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 237583f7..d94b1f66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,24 @@ for historical reference. ### Fixed +- **A Bash heredoc whose body is empty at the top, or empty + throughout, no longer reports those rows as blank** (#1412). + `heredoc_body`'s span begins at the first body row that *has* text + and collapses to zero width when there is none, so a leading empty + row sat inside no node at all and `blank = sloc - ploc - cloc` + claimed it: `cat < 0`, `assert_fixtures_present`) without diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 639d5f1e..813a2242 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -4755,6 +4755,27 @@ line3\";", ); } + // The union of the fixture languages below, so a feature subset that + // enables none of them makes this test *absent* rather than failing + // in a way that reads as a defect in whatever is being changed + // (`.claude/rules/testing.md`). Every call site carries its own + // narrower gate, so if the test exists at least one row runs and it + // cannot go vacuous. + #[cfg(any( + feature = "bash", + feature = "c", + feature = "elixir", + feature = "go", + feature = "irules", + feature = "kotlin", + feature = "mozcpp", + feature = "mozjs", + feature = "perl", + feature = "php", + feature = "python", + feature = "ruby", + feature = "tcl", + ))] #[test] fn multiline_string_ploc_consistent_across_languages() { // Cross-language parity for issue #778: the SAME 3-line string @@ -4764,6 +4785,33 @@ line3\";", // `check_metrics` takes a plain `fn(CodeMetrics)`, so the shared // assertion is a named function rather than a capturing closure and // must take its argument by value to match that pointer type. + // + // The middle row is **empty**, and that is the point (#1412). + // Until then every fixture here read `line1\nline2\nline3`, which + // a language passes whether or not it credits an interior row + // with no text — so the table could not see either #1396 (PHP) or + // #1412 (Bash), the two defects it is the cross-language home + // for. C is the one language that cannot express the shape: its + // multi-row literal is a backslash-newline continuation, and an + // unescaped empty row ends the string, so its row keeps `line2`. + // + // Gated on its own callers rather than on the test's union: Go is + // in the union and is the one row with different expectations, so + // an ungated helper is dead code in a Go-only build. + #[cfg(any( + feature = "bash", + feature = "c", + feature = "elixir", + feature = "irules", + feature = "kotlin", + feature = "mozcpp", + feature = "mozjs", + feature = "perl", + feature = "php", + feature = "python", + feature = "ruby", + feature = "tcl", + ))] #[allow(clippy::needless_pass_by_value)] fn assert_three_code_rows(metric: crate::CodeMetrics) { assert_eq!(metric.loc.sloc(), 3); @@ -4771,29 +4819,39 @@ line3\";", assert_eq!(metric.loc.cloc(), 0); assert_eq!(metric.loc.blank(), 0); } + // The heredoc forms carry their opener and terminator on rows of + // their own, so they are four rows rather than three. + #[cfg(any(feature = "bash", feature = "php"))] + #[allow(clippy::needless_pass_by_value)] + fn assert_four_code_rows(metric: crate::CodeMetrics) { + assert_eq!(metric.loc.sloc(), 4); + assert_eq!(metric.loc.ploc(), 4); + assert_eq!(metric.loc.cloc(), 0); + assert_eq!(metric.loc.blank(), 0); + } + #[cfg(feature = "python")] check_metrics::( - "s = \"\"\"line1\nline2\nline3\"\"\"", + "s = \"\"\"line1\n\nline3\"\"\"", "foo.py", assert_three_code_rows, ); + #[cfg(feature = "perl")] check_metrics::( - "my $s = \"line1\nline2\nline3\";", + "my $s = \"line1\n\nline3\";", "foo.pl", assert_three_code_rows, ); - check_metrics::( - "s = \"line1\nline2\nline3\"", - "foo.rb", - assert_three_code_rows, - ); + #[cfg(feature = "ruby")] + check_metrics::("s = \"line1\n\nline3\"", "foo.rb", assert_three_code_rows); // Go, Kotlin, and Mozilla-C++ reach the same shared // `add_multiline_string_ploc` helper through their own // raw-string kinds (`raw_string_literal`, // `multiline_string_literal`, `raw_string_literal`), and were // the three call sites of it that no test exercised. Each needs // its own syntax, so they cannot reuse the quoted form above. + #[cfg(feature = "go")] check_metrics::( - "package p\n\nvar s = `line1\nline2\nline3`", + "package p\n\nvar s = `line1\n\nline3`", "foo.go", |metric| { // Two extra code rows for `package p` and the blank @@ -4804,28 +4862,34 @@ line3\";", assert_eq!(metric.loc.blank(), 1); }, ); + #[cfg(feature = "kotlin")] check_metrics::( - "val s = \"\"\"line1\nline2\nline3\"\"\"", + "val s = \"\"\"line1\n\nline3\"\"\"", "foo.kt", assert_three_code_rows, ); + #[cfg(feature = "mozcpp")] check_metrics::( - "const char* s = R\"(line1\nline2\nline3)\";", + "const char* s = R\"(line1\n\nline3)\";", "foo.cpp", assert_three_code_rows, ); // C has no raw string; tree-sitter-c folds a backslash-newline - // continuation into one `string_literal` spanning every row. + // continuation into one `string_literal` spanning every row, and + // that continuation is why the C row below is the one that keeps + // a non-empty middle line. // Mozjs carries the same `template_string` arm as its upstream // JS siblings, but only `.jsm` routes to it, so the JavaScript // and TypeScript template tests never reach this copy. + #[cfg(feature = "c")] check_metrics::( "const char* s = \"line1\\\nline2\\\nline3\";", "foo.c", assert_three_code_rows, ); + #[cfg(feature = "mozjs")] check_metrics::( - "const s = `line1\nline2\nline3`;", + "const s = `line1\n\nline3`;", "foo.jsm", assert_three_code_rows, ); @@ -4833,24 +4897,53 @@ line3\";", // the single-quoted `raw_string`; Tcl and its iRules dialect spell // the literal `quoted_word`; Elixir routes the `quoted_content` // every one of its string forms wraps. - check_metrics::("s='line1\nline2\nline3'", "foo.sh", assert_three_code_rows); + #[cfg(feature = "bash")] + check_metrics::("s='line1\n\nline3'", "foo.sh", assert_three_code_rows); + #[cfg(feature = "tcl")] check_metrics::( - "set s \"line1\nline2\nline3\"", + "set s \"line1\n\nline3\"", "foo.tcl", assert_three_code_rows, ); + #[cfg(feature = "irules")] check_metrics::( - "set s \"line1\nline2\nline3\"", + "set s \"line1\n\nline3\"", "foo.irule", assert_three_code_rows, ); - check_metrics::( - "s = \"line1\nline2\nline3\"", - "foo.ex", + #[cfg(feature = "elixir")] + check_metrics::("s = \"line1\n\nline3\"", "foo.ex", assert_three_code_rows); + // PHP was absent from this table entirely — including for the + // quoted forms #778 routed — which is half of why #1396 survived + // #778. + #[cfg(feature = "php")] + check_metrics::( + "( + "("cat < Stats { + metrics_verbatim( + crate::LANG::Bash, + source, + crate::MetricsOptions::default().with_only(&[crate::Metric::Loc]), + ) + .loc + } + + /// #1412: a heredoc body that is empty at the top, or empty + /// throughout, read as blank. `heredoc_body`'s span begins at the + /// first body row that *has* text and collapses to zero width when + /// there is none, so the leading empty rows sat inside no node at + /// all and `blank = sloc - ploc - cloc` claimed them. + /// + /// #1260 routed the body, which fixed only the interior of a body + /// that has text — the shape all of its fixtures had. The arm now + /// routes the `heredoc_redirect` wrapper, the node present for every + /// spelling (`.claude/rules/grammar-dispatch.md` section 6), which + /// is what #1396 did for PHP. + #[cfg(feature = "bash")] + #[test] + fn bash_heredoc_bodies_that_open_or_stay_empty_are_not_blank() { + // `(source, rows the file has, blank reported before #1412)`. + // The last column is what makes each row a regression test + // rather than a restatement of the fixed behaviour. + // + // `sloc` is the second axis the fixtures are anchored on + // (`.claude/rules/testing.md`): trimming the heredoc out of any + // of them leaves a bare `cat` — or, in the last row, a bare + // function — which fails the `sloc` assertion before the `blank` + // one can go vacuous. + const FIXTURES: &[(&[u8], u64, u64)] = &[ + // The issue's four reproductions. The fourth is the control: + // it is the shape #1260's own fixtures had and was already + // correct, so a "fix" that simply stopped counting body rows + // moves it off `ploc == rows`. + (b"cat < { + // `ploc 0, blank 1`. Re-derived for `heredoc_redirect`: its + // parent `redirected_statement` starts on the same row, so the + // gate would skip there too — harmless only because the `<<` + // token is a leaf on that row and the catch-all picks it up. + // Depending on that is a second rule for no gain, so the + // wrapper takes the unconditional form its siblings do. + String | RawString | AnsiCString | HeredocRedirect => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); add_string_interior_ploc(node, stats, start); @@ -73,8 +114,7 @@ impl Loc for BashCode { // (`X=1 cmd`). The unsuffixed `VariableAssignment` below is // never emitted, so listing only it scored `a=1` zero // logical lines (`.claude/rules/grammar-dispatch.md` §1); - // it stays as a defensive arm rather than being swapped, - // the same shape as the `HeredocBody` pair above. + // it stays as a defensive arm rather than being swapped. // // The last two positions need the parent gate. Both // `declaration_command` and `command` are counted as From 43595c52b60111f83281090cc9105e9c43602a44 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 12 Sep 2026 19:36:59 -0700 Subject: [PATCH 03/12] fix(metrics/loc): bound PreprocArg rows by end_line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `PreprocArg` arm in the C, C++, `mozcpp` and Objective-C `Loc` impls credited rows up to tree-sitter's raw `end_row()`. That over-reads by one whenever a node ends at column 0 — the shape a macro body whose last continuation is a dangling backslash produces, since the node finishes at the *start* of the row below rather than inside it. `Node::end_line` exists to encode that rule and every other multi-row PLOC path already used it. All four arms now call `add_string_interior_ploc`, of which they were an open-coded copy with the wrong bound. That fixes the bound, drops a redundant second `kind_id().into()` (the match scrutinee is bound instead), and replaces a per-row loop with one bitmap-span write. The defect is live, not latent: DeepSpeech's `left_test.cc` has a `TEXT_TEST` macro of exactly this shape and moves from `ploc 344, blank 35` to `ploc 343, blank 36`. The issue's "not currently observable" sweep measured the `ploc <= sloc` contract, which the over-credit never violated, rather than the snapshot values. Python's `String` arm was the last remaining open-coded copy of the pattern and is folded onto `add_multiline_string_ploc`. The bound change is unreachable there — a Python `string` node always closes on a `string_end` quote, so its end column is never 0, and an unterminated literal parses to a bare `string_start` under an `ERROR` rather than to a `string` node — so the fold is for reuse. C# has a `PreprocArg` and no arm for it. `tree-sitter-c-sharp` accepts a backslash continuation the C# specification does not, so the multi-row shape is grammar-reachable and language-invalid; left alone with a `FIXME` and filed as #1430 rather than guessed at. Fixes #1423 --- CHANGELOG.md | 13 ++++ src/metrics/loc.rs | 66 +++++++++++++++++++++ src/metrics/loc/c.rs | 18 +++--- src/metrics/loc/cpp.rs | 18 +++--- src/metrics/loc/csharp.rs | 10 ++++ src/metrics/loc/mozcpp.rs | 18 +++--- src/metrics/loc/objc.rs | 13 ++-- src/metrics/loc/python.rs | 20 ++++--- src/metrics/loc/shared.rs | 6 ++ tests/repositories/big-code-analysis-output | 2 +- 10 files changed, 150 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d94b1f66..a1acca00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,19 @@ for historical reference. ### Fixed +- **A C-family macro body ending on a dangling backslash no longer + credits the blank row below it as code** (#1423). The + `PreprocArg` arm in the C, C++, `mozcpp` and Objective-C `Loc` impls + bounded its row range with tree-sitter's raw end row, which over-reads + by one whenever a node ends at column 0 — the shape a trailing `\` + with nothing after it produces. All four arms now go through + `add_string_interior_ploc`, the helper every other multi-row PLOC path + already used, which derives the last row from `Node::end_line`. + `ploc` falls by one and `blank` rises by one for each such macro; + DeepSpeech's `left_test.cc` is one real instance. Python's `String` + arm, the last remaining open-coded copy of the same pattern, is folded + onto `add_multiline_string_ploc` — a no-op there, since a Python + `string` node always closes on a quote and so never ends at column 0. - **A Bash heredoc whose body is empty at the top, or empty throughout, no longer reports those rows as blank** (#1412). `heredoc_body`'s span begins at the first body row that *has* text diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 813a2242..94ea2957 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -11499,6 +11499,12 @@ class A { /// being the macro's last continuation line. `sloc` is 5 (three /// macro rows, one blank, one `main`) and `lloc` is 1 — the single /// `return` statement — since a `#define` declares no statement. + /// + /// This fixture pins how *many* rows the arm credits, not where it + /// stops: its body closes on `(b))`, so the node's end column is + /// above 0 and the raw end row and `Node::end_line() - 1` name the + /// same last row. The bound is pinned separately by + /// [`a_dangling_macro_continuation_does_not_credit_the_row_below`]. #[test] fn a_continued_macro_body_counts_every_row_it_spans() { // Rows: 0-2 are the macro, 3 is blank, 4 is `main`. @@ -11524,6 +11530,66 @@ class A { } } + /// Where the `PreprocArg` arm stops: a macro body whose last + /// continuation is a *dangling* backslash produces a node ending at + /// column 0 of the row below, and that row is not part of the span. + /// + /// `Node::end_line` is what encodes the rule, so the arm derives its + /// last row from `add_string_interior_ploc` rather than from the raw + /// `end_row()`, which over-reads by one on exactly this shape + /// (#1423). Every other multi-row PLOC path already went through the + /// helper; these four arms were the last open-coded copies. + /// + /// Measured with `bca dump`, not read off the source text: the + /// `preproc_arg` runs from row 0 column 10 to row 2 column 0. Rows + /// are 0 `#define`, 1 the continuation, 2 the blank row the trailing + /// backslash runs into, 3 `int x;`. So `ploc` is {0, 1, 3} = 3 and + /// `blank` is 1. The raw end row credited row 2 as well, reporting + /// `ploc 4, blank 0`. + /// + /// Two parts of the fixture are load-bearing rather than scenery: + /// + /// - **`int x;` after the macro.** `clamp_line_sets_to_span` already + /// drops a PLOC row past the space's own last row, so a fixture + /// ending on the macro reports the corrected numbers under either + /// spelling and discriminates nothing. + /// - **The ` 2 \` continuation row.** Nothing but the arm's + /// interior insert credits row 1 — `preproc_def` contributes only + /// its own start row — so `ploc == 3` fails if that row is ever + /// trimmed out of the fixture, per the fixture-decay rule in + /// `.claude/rules/testing.md`. The `blank == 1` half has no such + /// anchor available: deleting the dangling backslash also stops + /// row 2 being credited, so a revert of the bound is the only + /// evidence that half discriminates, and it was run. + #[test] + fn a_dangling_macro_continuation_does_not_credit_the_row_below() { + // Rows: 0-1 are the macro, 2 is the blank row its trailing + // backslash runs into, 3 is `int x;`. + const DANGLING_CONTINUATION: &[u8] = b"#define A 1 \\\n 2 \\\n\nint x;\n"; + + for lang in [ + crate::LANG::C, + crate::LANG::Cpp, + crate::LANG::Mozcpp, + crate::LANG::Objc, + ] { + let loc = metrics_verbatim(lang, DANGLING_CONTINUATION, MetricsOptions::default()).loc; + assert_eq!( + loc.ploc(), + 3, + "{lang:?}: the row a dangling continuation ends *at* is not code" + ); + assert_eq!( + loc.blank(), + 1, + "{lang:?}: that row is blank, not part of the macro body" + ); + assert_eq!(loc.sloc(), 4, "{lang:?} sloc"); + assert_eq!(loc.lloc(), 1, "{lang:?} lloc"); + assert_eq!(loc.cloc(), 0, "{lang:?} cloc"); + } + } + /// Whether the last line ends in a newline is a formatting detail, not /// a property of the code — no LOC sub-metric, and therefore no MI /// value, may depend on it. This is the invariant #1067 violated, and diff --git a/src/metrics/loc/c.rs b/src/metrics/loc/c.rs index 42870b1a..7d25adec 100644 --- a/src/metrics/loc/c.rs +++ b/src/metrics/loc/c.rs @@ -52,17 +52,21 @@ impl Loc for CCode { stats.lloc.count_logical_line(); } } - _ => { + kind => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); // As reported here: https://github.com/tree-sitter/tree-sitter-cpp/issues/276 - // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. - // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content - if let PreprocArg = node.kind_id().into() { - (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { - stats.ploc.lines.insert(line); - }); + // `tree-sitter-cpp` doesn't expand macros, providing a single + // `PreprocArg` node for the entire macro argument, so every row + // that node spans is PLOC rather than blank. + // + // Bounded by `add_string_interior_ploc`, and therefore by + // `Node::end_line` rather than the raw end row: a body whose + // last continuation is a dangling backslash ends at column 0 of + // the row below, which the node does not occupy (#1423). + if let PreprocArg = kind { + add_string_interior_ploc(node, stats, start); } } } diff --git a/src/metrics/loc/cpp.rs b/src/metrics/loc/cpp.rs index 5fe04ee9..5ea26a99 100644 --- a/src/metrics/loc/cpp.rs +++ b/src/metrics/loc/cpp.rs @@ -51,17 +51,21 @@ impl Loc for CppCode { stats.lloc.count_logical_line(); } } - _ => { + kind => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); // As reported here: https://github.com/tree-sitter/tree-sitter-cpp/issues/276 - // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. - // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content - if let PreprocArg = node.kind_id().into() { - (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { - stats.ploc.lines.insert(line); - }); + // `tree-sitter-cpp` doesn't expand macros, providing a single + // `PreprocArg` node for the entire macro argument, so every row + // that node spans is PLOC rather than blank. + // + // Bounded by `add_string_interior_ploc`, and therefore by + // `Node::end_line` rather than the raw end row: a body whose + // last continuation is a dangling backslash ends at column 0 of + // the row below, which the node does not occupy (#1423). + if let PreprocArg = kind { + add_string_interior_ploc(node, stats, start); } } } diff --git a/src/metrics/loc/csharp.rs b/src/metrics/loc/csharp.rs index 35b23236..c82cfa47 100644 --- a/src/metrics/loc/csharp.rs +++ b/src/metrics/loc/csharp.rs @@ -53,6 +53,16 @@ impl Loc for CsharpCode { _ => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); + + // FIXME(#1430): C# has a `PreprocArg` (185) and no arm for + // it, unlike its four C-family siblings, so a multi-row + // `preproc_arg` credits only its first row and the rest + // fall through to `blank`. `tree-sitter-c-sharp` accepts a + // backslash continuation in a directive (`#region Big \`), + // but the C# specification terminates a directive at the + // newline, so the shape is grammar-reachable and + // language-invalid. Deliberately left alone by #1423 rather + // than guessed at. } } } diff --git a/src/metrics/loc/mozcpp.rs b/src/metrics/loc/mozcpp.rs index 211601d7..fed6fd77 100644 --- a/src/metrics/loc/mozcpp.rs +++ b/src/metrics/loc/mozcpp.rs @@ -51,17 +51,21 @@ impl Loc for MozcppCode { stats.lloc.count_logical_line(); } } - _ => { + kind => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); // As reported here: https://github.com/tree-sitter/tree-sitter-cpp/issues/276 - // `tree-sitter-cpp` doesn't expand macros, providing a single `PreprocArg` node for the entire macro argument. - // Therefore, all lines from `start_row` to `end_row` must be added to PLOC to account for the unexpanded macro content - if let PreprocArg = node.kind_id().into() { - (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { - stats.ploc.lines.insert(line); - }); + // `tree-sitter-cpp` doesn't expand macros, providing a single + // `PreprocArg` node for the entire macro argument, so every row + // that node spans is PLOC rather than blank. + // + // Bounded by `add_string_interior_ploc`, and therefore by + // `Node::end_line` rather than the raw end row: a body whose + // last continuation is a dangling backslash ends at column 0 of + // the row below, which the node does not occupy (#1423). + if let PreprocArg = kind { + add_string_interior_ploc(node, stats, start); } } } diff --git a/src/metrics/loc/objc.rs b/src/metrics/loc/objc.rs index be1ee74c..2c957798 100644 --- a/src/metrics/loc/objc.rs +++ b/src/metrics/loc/objc.rs @@ -76,17 +76,20 @@ impl Loc for ObjcCode { stats.lloc.count_logical_line(); } } - _ => { + kind => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); // tree-sitter-objc inherits tree-sitter-cpp's unexpanded // macro handling: a single `PreprocArg` node spans the // whole macro argument, so every line it covers is PLOC. - if let PreprocArg = node.kind_id().into() { - (node.start_row().saturating_add(1)..=node.end_row()).for_each(|line| { - stats.ploc.lines.insert(line); - }); + // + // Bounded by `add_string_interior_ploc`, and therefore by + // `Node::end_line` rather than the raw end row: a body whose + // last continuation is a dangling backslash ends at column 0 of + // the row below, which the node does not occupy (#1423). + if let PreprocArg = kind { + add_string_interior_ploc(node, stats, start); } } } diff --git a/src/metrics/loc/python.rs b/src/metrics/loc/python.rs index c6343034..2d59c1f0 100644 --- a/src/metrics/loc/python.rs +++ b/src/metrics/loc/python.rs @@ -50,13 +50,19 @@ impl Loc for PythonCode { // blank lines (#415). The opening row is inserted only when // the parent statement begins on an earlier row, otherwise // that row is already attributed to the enclosing statement. - if parent.start_row() != start { - check_comment_ends_on_code_line(stats, start); - stats.ploc.lines.insert(start); - } - (start.saturating_add(1)..=end).for_each(|line| { - stats.ploc.lines.insert(line); - }); + // + // This arm is the rule `add_multiline_string_ploc` was + // extracted from (#778), and stayed open-coded until #1423 + // swept the last copies of the pattern onto the helper. The + // parent gate is unchanged — `parent` is `Some` here, so + // the helper's `is_none_or` reduces to the same test — and + // the interior bound moves from the raw end row to + // `Node::end_line`. That is a no-op in Python today: a + // `string` node always closes on a `string_end` quote, so + // its end column is never 0, and an unterminated literal + // parses to a bare `string_start` under an `ERROR` rather + // than to a `string` node at all. + add_multiline_string_ploc(node, ancestors, stats, start); } } Statement diff --git a/src/metrics/loc/shared.rs b/src/metrics/loc/shared.rs index f8134717..585c6509 100644 --- a/src/metrics/loc/shared.rs +++ b/src/metrics/loc/shared.rs @@ -229,6 +229,12 @@ pub(crate) fn add_multiline_string_ploc( // `insert_range` rather than a row-at-a-time loop: the range is one // bitmap span, and Bash's heredoc bodies push thousands of rows through // here where the loop paid a reserve and a bounds check per row. +// +// The name records where the rule came from, not the only node it +// applies to: #1423 routed the four C-family `PreprocArg` arms here +// too, since an unexpanded macro body is the same question — which rows +// does this multi-row node cover — and had the same off-by-one bound +// open-coded. #[inline] pub(crate) fn add_string_interior_ploc(node: &Node, stats: &mut Stats, start: usize) { // Inclusive, and `insert_range` no-ops on an inverted span, so a diff --git a/tests/repositories/big-code-analysis-output b/tests/repositories/big-code-analysis-output index c03ffa97..98c48d2c 160000 --- a/tests/repositories/big-code-analysis-output +++ b/tests/repositories/big-code-analysis-output @@ -1 +1 @@ -Subproject commit c03ffa977062d691ecc54cf66eeb21ed1f1a3a12 +Subproject commit 98c48d2cb9e1209edc6204ec868eef1e068d0e2f From 642959d5d6a82739932078bec2f2e7d64d80a55b Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 12 Sep 2026 20:06:01 -0700 Subject: [PATCH 04/12] fix(metrics/loc): exclude only the rows a prune removes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Sloc` accumulated each `exclude_tests`-pruned subtree's whole row span as a scalar count, on the assumption its own doc comment stated: that rustfmt gives every Rust item dedicated rows, so a pruned span shares no physical line with a retained sibling and the counts add without an interval merge. Anything hand-written, generated, minified or concatenated breaks it. `fn a() {} #[cfg(test)] mod t { … }` reported `sloc 0, ploc 1` — one row of code in a zero-row file — and fed that zero to MI's `ln(sloc)` term, the #1051 shape. `excluded_lines: usize` becomes `excluded_rows: LineSet`, the single source of truth: no cached count that can fall out of step with it, which is the trade `ploc`/`cloc` already made in #1109. A new `LineSet::subtract` clears `other`'s bits in place — destructive rather than a `difference_len` counter, because `Sloc::merge` folds the residual upward and the identity of the surviving rows has to outlive the call. `Stats::settle_excluded_rows` runs it against `ploc.lines` and both `cloc` sets once per space at finalization, between the #1398 clamp and `compute_minmax`, so the subtrahend is settled before anything reads `sloc()`. `Sloc::merge` unions instead of adding, which also stops two pruned siblings on one row costing that row twice. Not the cheaper clamp of the count to `span - |retained rows|`. That under-reports whenever the space has a blank row to absorb the phantom subtraction, which is not a corner: on `fn a() {} #[cfg(test)] mod t {}` / blank / `fn b() {}` the truth is `sloc 3, blank 1`, the bug gives `sloc 2, blank 0`, and the clamp gives `min(1, 3 - 2) = 1`, so `sloc 2, blank 0` — no improvement at all. It buys `ploc <= sloc` by moving the error into `blank`. Measured against the built binary, and pinned by a regression test that fails under the clamp. With `P`, `O`, `C` and the pruned set all clamped inside the span, the settled residual is disjoint from `P ∪ O ∪ C`, so `sloc() >= max(ploc(), cloc())` follows with no dependence on grammar shape or walk order. `clamp_line_sets_to_span` gains a fourth `retain_range` to establish that premise, and the tightened pair of `debug_assert!`s moves to the new pass — running on every space of every walk, which is how the fix is evidenced across all twenty languages rather than only on the named fixtures. The clamp keeps its own `<= span` pair, which localises a clamp failure to the clamp. Ordinary rustfmt-shaped input is byte-identical: the seven `exclude_tests_rust` LOC fixtures and `exclude_tests_pruning_composes_with_the_unit_anchor` are unchanged, because their excluded and retained rows are already disjoint. Two observable changes to `Sloc`, both on private state reached through derives: `Debug` prints the rows, and `PartialEq` strengthens. `big-code-analysis-bench` gains `loc/wide-cfg-test-mod`, the only scaling probe on which the *number* of pruned nodes grows with the size parameter — the two `nom/*-attributed-fn` probes use `#[inline]` precisely so the walk does not prune, so the prune arm's own row bookkeeping was unpriced. It measures 1.21 against a 1.50 bound. The remaining over-count is separate and filed as #1431: a `#[cfg(test)]` attribute is an `AttributeItem` sibling of the item, so it is walked normally and its row stays in PLOC. A 100%-test file still reports `ploc 1`. Fixes #1417 --- CHANGELOG.md | 21 ++ big-code-analysis-bench/src/shapes.rs | 56 ++++++ src/metrics/loc.rs | 280 ++++++++++++++++++++++---- src/metrics/loc/line_set.rs | 85 +++++++- src/spaces/compute.rs | 38 +++- src/spaces_tests.rs | 46 ++++- 6 files changed, 474 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1acca00..c3d9672f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,27 @@ for historical reference. ### Fixed +- **`--exclude-tests` no longer drops rows a pruned item shares with + retained code**, which made `sloc` fall below `ploc` (#1417). + `Sloc` accumulated each pruned subtree's whole row span as a + *count*, on the assumption — written into its `exclude_span` doc + comment — that rustfmt gives every Rust item its own rows. Anything + hand-written, generated, minified, or concatenated breaks it: + `fn a() {} #[cfg(test)] mod t { … }` reported `sloc 0, ploc 1`, one + row of code in a zero-row file, and MI's `ln(sloc)` term saw the + same zero. The count is now a row *set*, from which the space's + retained code and comment rows are subtracted at finalization, so + only rows the prune genuinely removed are subtracted; two pruned + siblings on one row also now cost that row once instead of twice. + `sloc` rises for affected spaces and `blank` rises with it; ordinary + rustfmt-shaped input is byte-identical, and the shipped + `--exclude-tests`-off default is untouched. `Loc`'s per-space + invariant `ploc <= sloc` (and `cloc <= sloc`) is now asserted in + debug builds on every space of every walk. Two observable + side effects on the `Sloc` sub-struct: its `Debug` rendering prints + the excluded rows (`excluded_rows: {6, 7, 8}`) where it printed a + count, and its `PartialEq` strengthens — two `Sloc`s that exclude + the same *number* of different rows no longer compare equal. - **A C-family macro body ending on a dangling backslash no longer credits the blank row below it as code** (#1423). The `PreprocArg` arm in the C, C++, `mozcpp` and Objective-C `Loc` impls diff --git a/big-code-analysis-bench/src/shapes.rs b/big-code-analysis-bench/src/shapes.rs index 661c188d..8fc07ab9 100644 --- a/big-code-analysis-bench/src/shapes.rs +++ b/big-code-analysis-bench/src/shapes.rs @@ -294,6 +294,34 @@ pub fn wide_attributed_fns(width: usize) -> String { format!("{}\n", "#[inline] fn f() {} ".repeat(width)) } +/// Rust: `#[cfg(test)]\nmod m {}\n` repeated at file scope. +/// +/// The shape [`nested_attributed_fns`] and [`wide_attributed_fns`] +/// cannot be: both use `#[inline]` precisely so the walk does *not* +/// prune, which leaves the number of pruned nodes at zero on every +/// existing probe and the prune's own bookkeeping unpriced (#1417). +/// Here every item is pruned, so the count grows with the size +/// parameter. +/// +/// Two rows per item, with the attribute on its own row, is what makes +/// the reading move: the attribute is an `AttributeItem` *sibling* of +/// the item, so it is walked normally and its row survives, while the +/// `mod` row does not. `sloc` is therefore `width` against a `2 * +/// width` file, and a prune that stopped recording rows would read as +/// `2 * width`. +/// +/// The cost this prices is `Sloc::excluded_rows`: one `insert_range` +/// per pruned node during the walk, and one `retain_range` plus three +/// `subtract`s over the accumulated bitset at finalization. All four +/// are word-wise passes, so the shape is linear — but the bitset grows +/// downward with a `Vec::splice` (`LineSet::reserve`), which would be +/// quadratic in words if rows ever arrived descending. A flat file +/// visits them ascending; this probe is what keeps that measured. +#[must_use] +pub fn wide_cfg_test_mods(width: usize) -> String { + "#[cfg(test)]\nmod m {}\n".repeat(width) +} + /// Rust: `fn f000000() { let v000000 = 000000; } fn f000001() { … }`, /// all at file scope. /// @@ -1073,6 +1101,34 @@ pub const PROBES: &[Probe] = &[ against the node's depth, and this probe is what \ keeps the shallow-wide half of that trade measured.", }, + Probe { + name: "loc/wide-cfg-test-mod", + lang: LANG::Rust, + axis: Axis::Width, + workload: Workload::Metrics { + exclude_tests: true, + selection: &[Metric::Loc], + reading: |m| m.loc.sloc(), + }, + render: wide_cfg_test_mods, + sizes: LINEAR_WIDTHS, + max_exponent: LINEAR_BOUND, + rationale: "#1417: the only probe on which the number of *pruned* \ + nodes grows with the size parameter. The two \ + `nom/*-attributed-fn` probes deliberately use \ + `#[inline]`, because a test attribute prunes the \ + outermost item and stops the walk — so before this \ + one, `exclude_tests` was exercised only through its \ + attribute scan and never through the row bookkeeping \ + the prune arm itself does. #1417 replaced that \ + bookkeeping's running count with a `LineSet`, adding \ + an `insert_range` per pruned node and a \ + `retain_range` plus three `subtract`s per space. All \ + are word-wise, but `LineSet::reserve` grows downward \ + with an `O(len)` splice, so a caller that fed rows \ + descending would be quadratic in words. This is what \ + prices that.", + }, Probe { name: "nom/nested-cfg-predicate", lang: LANG::Rust, diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 94ea2957..2e648cce 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -97,15 +97,22 @@ pub struct Sloc { // Storing the resolved line rather than the raw end row plus its // column keeps the "does the final row count" rule in one place. end_line: usize, - // Physical lines removed from this space's span by `exclude_tests` + // Physical rows removed from this space's span by `exclude_tests` // pruning. `sloc` is the lone loc sub-metric computed by span // subtraction rather than node-by-node accumulation, so a pruned // subtree (which a `continue` in the walk suppresses for every - // accumulated metric) leaves the span untouched. We accumulate the - // inclusive row count of each pruned subtree here and subtract it - // in `sloc()` so SLOC drops in step with `ploc`/`cloc`/`lloc` + // accumulated metric) leaves the span untouched. We record each + // pruned subtree's rows here and subtract the cardinality in + // `sloc()` so SLOC drops in step with `ploc`/`cloc`/`lloc` // (issue #722). - excluded_lines: usize, + // + // A set rather than the counter this was until #1417, because rows + // shared with retained code have to be identifiable and removed + // before the subtraction — see `Stats::settle_excluded_rows`. It is + // the single source of truth; there is no cached count to fall out + // of step with it, matching what `ploc`/`cloc` already gave up + // when they became bitsets (#1109). + excluded_rows: LineSet, sloc_min: usize, sloc_max: usize, } @@ -115,7 +122,7 @@ impl Default for Sloc { Self { start: 0, end_line: 0, - excluded_lines: 0, + excluded_rows: LineSet::default(), sloc_min: usize::MAX, sloc_max: 0, } @@ -130,32 +137,53 @@ impl Sloc { // This metric counts the number of physical lines this space // occupies, including blanks and comments. let span = span_rows(self.start, self.end_line); - // Subtract the lines belonging to `exclude_tests`-pruned subtrees - // (issue #722). `saturating_sub` is defensive: `excluded_lines` - // can never exceed the span (each pruned subtree is contained in - // it), but a future caller that double-records a span must not + // Subtract the rows belonging to `exclude_tests`-pruned subtrees + // (issue #722). `saturating_sub` is defensive: the set is + // clamped to the span and its rows are a subset of it, but a + // future caller that records a row outside the span must not // wrap to `u64::MAX`. - span.saturating_sub(self.excluded_lines) as u64 + span.saturating_sub(self.excluded_rows.len()) as u64 } - /// Records a pruned (`exclude_tests`) subtree's row span so that + /// Records the rows of a pruned (`exclude_tests`) subtree so that /// `sloc()` drops in step with the node-accumulated loc sub-metrics. /// The arguments are the pruned node's own start row and - /// `Node::end_line`; its row count follows the same rule the - /// enclosing span was measured with, so the subtraction cannot - /// overshoot. + /// `Node::end_line`, so the rows recorded follow the same rule the + /// enclosing span was measured with. /// - /// Pruned subtrees are whole Rust items (`mod`/`fn`/`impl`/…) that - /// rustfmt places on dedicated rows, so they share no physical line - /// with a retained sibling and their spans are pairwise disjoint (the - /// walk `continue`s on a pruned node, never descending, so a nested - /// pruned item is never recorded twice). The counts therefore add - /// without an interval merge (issue #722). + /// A *set* rather than the running count this kept until #1417. A + /// pruned item can share its first or last physical row with a + /// retained sibling — `fn a() {} #[cfg(test)] mod t { … }` is one + /// row, of which nothing is removed — and a count cannot tell that + /// row from one the prune really took, so `sloc()` fell below + /// `ploc()`. The set also makes two pruned siblings on one row + /// subtract that row once instead of twice. + /// + /// The rows recorded here are the pruned node's *whole* span; + /// [`Stats::settle_excluded_rows`] is what takes the shared ones + /// back out, once the retained sets for this space are complete. #[inline] pub(crate) fn exclude_span(&mut self, start_row: usize, end_line: usize) { - self.excluded_lines = self - .excluded_lines - .saturating_add(span_rows(start_row, end_line)); + // `end_line` is the 1-based inclusive last row, so the last + // 0-based row is `end_line - 1`. Guarding here rather than + // leaning on `insert_range`'s inverted-span return, which would + // need `end_line - 1` computed first: at `end_line == 0` that + // saturates to row 0 and records a row the node does not + // occupy, and an empty span is not an inverted one. + if end_line <= start_row { + debug_assert!( + end_line == start_row, + "exclude_span: end_line {end_line} < start_row {start_row}" + ); + return; + } + // Exact under the guard — `end_line > start_row >= 0` gives + // `end_line >= 1` — and saturating for the module's + // `arithmetic_side_effects` warning, which cannot see that. + // The row count inserted is `span_rows(start_row, end_line)`, + // byte-identical to the count this used to add. + self.excluded_rows + .insert_range(start_row, end_line.saturating_sub(1)); } /// The `Sloc` metric minimum value. See `min_or_zero` for the @@ -174,7 +202,7 @@ impl Sloc { } /// Folds `other` into `self`, updating the min/max accumulators and - /// accumulating the child's `exclude_tests`-pruned line count. + /// unioning the child's `exclude_tests`-pruned rows. #[inline] pub fn merge(&mut self, other: &Sloc) { // Fold the child's own min/max (not its aggregate `sloc()`), so the @@ -184,8 +212,8 @@ impl Sloc { self.sloc_min = self.sloc_min.min(other.sloc_min); self.sloc_max = self.sloc_max.max(other.sloc_max); - // Propagate the child's pruned line count upward so an ancestor's - // span-based `sloc()` drops by the same lines, mirroring how `Ploc` + // Propagate the child's pruned rows upward so an ancestor's + // span-based `sloc()` drops by the same rows, mirroring how `Ploc` // unions its line-set upward (`Ploc::merge`). The prune hook records // each pruned subtree's span only on its innermost enclosing // func-space; without this fold a `#[test] fn` inside a retained @@ -193,11 +221,20 @@ impl Sloc { // leaving every enclosing space (including the unit, which feeds // MI's SLOC term) inflated (issue #741, #722 follow-up). Each // ancestor's span already includes the pruned rows exactly once, so - // subtracting the accumulated count once per level cannot - // double-count: pruned subtrees never descend, so a nested pruned - // item is recorded on a single space and folded up one altitude at - // a time. - self.excluded_lines = self.excluded_lines.saturating_add(other.excluded_lines); + // subtracting them once per level cannot double-count: pruned + // subtrees never descend, so a nested pruned item is recorded on a + // single space and folded up one altitude at a time. + // + // Union rather than the `saturating_add` this was until #1417. + // Addition double-counted two pruned siblings sharing one + // physical row, and — the reason the fold is safe to leave + // exactly as it is — the child arrives here already settled + // against its own retained rows, which the parent's later + // `settle_excluded_rows` can only narrow further: + // `child_retained ⊆ parent_retained`, so + // `(X \ child_retained) \ parent_retained == X \ parent_retained`. + // The order the two settle in therefore does not matter. + self.excluded_rows.union_with(&other.excluded_rows); } #[inline] @@ -865,22 +902,28 @@ impl Stats { self.ploc.lines.retain_range(first, last); self.cloc.only_comment_line_starts.retain_range(first, last); self.cloc.code_comment_line_starts.retain_range(first, last); + // The fourth set is the `exclude_tests`-pruned rows, and it is + // clamped for the same reason as the other three rather than + // because anything is known to put a row outside the span + // there: a pruned node lies inside the space that recorded it, + // so this is expected to be a no-op. What it buys is the + // premise `X ⊆ span` that makes `settle_excluded_rows`'s + // `ploc() <= sloc()` a theorem rather than an observation — + // without it a stray row would turn `sloc()` negative-by- + // saturation and fire an assertion in the wrong pass (#1417). + self.sloc.excluded_rows.retain_range(first, last); // The invariant the clamp establishes, asserted on the path // every walk takes for every space rather than only on the // fixtures the regression tests name — 821 of the workspace's // tests reach it. // - // Against the span and not against `sloc()`, which is weaker on - // purpose. `sloc()` subtracts the rows of `exclude_tests`-pruned - // subtrees, and `Sloc::exclude_span` counts each pruned span - // whole — including a row a retained sibling also occupies, a - // case its #722 comment excludes by assuming rustfmt's layout. - // Hand-written one-liners break that assumption, so - // `fn a() {} #[cfg(test)] mod t { … }` reports `sloc 0, ploc 1` - // under `--exclude-tests` today. That is #1417, a different - // cause from the phantom row above, and asserting `sloc()` here - // would fire on it. Tighten this to `sloc()` once #1417 lands. + // Against the span, deliberately, and kept that way now that + // [`Stats::settle_excluded_rows`] asserts the stronger + // `ploc() <= sloc()` form (#1417). The two say different + // things: this pair localises a failure to the clamp itself, + // where `sloc()`'s subtrahend is not yet settled and could not + // be blamed. // // `ploc()` and `cloc()` popcount their word arrays (#1109): // O(words) per space, the same order as the `compute_minmax` that @@ -898,6 +941,57 @@ impl Stats { self.cloc() ); } + + /// Removes from the `exclude_tests`-pruned row set every row that + /// retained code or comments also occupy, so `sloc()` subtracts only + /// the rows the prune genuinely took (#1417). + /// + /// `Sloc::exclude_span` records a pruned subtree's whole span, and + /// its first or last physical row can carry a retained sibling — + /// `fn a() {} #[cfg(test)] mod t { … }` is one row that stays. The + /// walk cannot know that when it prunes, because the retained rows + /// of the enclosing space are still arriving; this pass runs once + /// per space at finalization, when they are all in. + /// + /// All three retained sets are subtracted. + /// `code_comment_line_starts` is very probably a subset of + /// `ploc.lines` — a row with both code and a comment is a code row + /// — but nothing enforces that, and the cost of not relying on it + /// is one word-wise `&= !` over an array that is empty for every + /// space that pruned nothing. + /// + /// Ordered after [`Stats::clamp_line_sets_to_span`], which + /// establishes the premise: with `P`, `O`, `C` and the pruned set + /// `X` all inside the span `S`, the residual `X' = X \ (P ∪ O ∪ C)` + /// is disjoint from `P ∪ O ∪ C`, so `|X'| + |P ∪ O ∪ C| <= |S|` and + /// `sloc() = |S| - |X'| >= max(ploc(), cloc())`. That is what the + /// assertions below pin — on every space of every walk, so they are + /// the real evidence the fix holds across all twenty languages + /// rather than only on the fixtures the regression tests name. + /// + /// [`Stats::blank`] keeps its `saturating_sub`: `P` and `O ∪ C` may + /// overlap on a code-and-comment row, so no lower bound on + /// `sloc - ploc - |O|` follows from the above. + pub(crate) fn settle_excluded_rows(&mut self) { + // Disjoint field paths, so the shorthand borrows cleanly. + let excluded = &mut self.sloc.excluded_rows; + excluded.subtract(&self.ploc.lines); + excluded.subtract(&self.cloc.only_comment_line_starts); + excluded.subtract(&self.cloc.code_comment_line_starts); + + debug_assert!( + self.ploc() <= self.sloc(), + "ploc {} exceeds sloc {} after settling", + self.ploc(), + self.sloc() + ); + debug_assert!( + self.cloc() <= self.sloc(), + "cloc {} exceeds sloc {} after settling", + self.cloc(), + self.sloc() + ); + } } #[doc(hidden)] @@ -11912,6 +12006,108 @@ class A { ); } + /// Analyses `source` as Rust with `exclude_tests` on, byte-for-byte. + /// `check_metrics` cannot express this: it trims and re-appends the + /// trailing newline (destroying the row structure these fixtures are + /// about) and its macro hard-binds `MetricsOptions`. + fn rust_loc_pruned(source: &[u8]) -> Stats { + metrics_verbatim( + crate::LANG::Rust, + source, + crate::MetricsOptions::default().with_exclude_tests(true), + ) + .loc + } + + /// #1417: `Sloc::exclude_span` recorded each pruned subtree's whole + /// span as a *count*, so a row a retained sibling also occupies was + /// subtracted anyway and `sloc` fell below `ploc`. Both one-liner + /// spellings from the issue reported `sloc 0, ploc 1` — one row of + /// code in a zero-row file. + /// + /// The pruned and unpruned readings are identical here, and that is + /// the clearest statement of why: the whole row survives the prune, + /// because `fn a()` is on it. `sloc 1` for a file that is more than + /// half test code reads as under-subtraction and is not — nothing + /// on that row can be removed without removing `fn a()` with it. + /// + /// `ploc 1` has a second, separate cause worth not confusing with + /// this one: `should_skip_subtree` matches the *item*, and a + /// `#[cfg(test)]` / `#[test]` attribute is an `AttributeItem` + /// sibling of it, so the attribute is walked normally and Rust's + /// catch-all credits its start row to PLOC. A file that is 100% + /// test code therefore still reports `ploc 1`. That is a different + /// defect, tracked as #1431. + #[test] + fn a_pruned_item_sharing_a_row_with_retained_code_keeps_the_row() { + for source in [ + &b"fn a() {} #[cfg(test)] mod t { #[test] fn x() {} }\n"[..], + &b"#[cfg(test)] mod t { #[test] fn x() {} } fn a() {}\n"[..], + ] { + let text = String::from_utf8_lossy(source); + let pruned = rust_loc_pruned(source); + assert_eq!( + (pruned.sloc(), pruned.ploc(), pruned.cloc(), pruned.blank()), + (1, 1, 0, 0), + "pruned {text:?}" + ); + + let kept = rust_loc(source); + assert_eq!( + (kept.sloc(), kept.ploc(), kept.cloc(), kept.blank()), + (1, 1, 0, 0), + "unpruned {text:?} — the prune can remove no row here" + ); + } + } + + /// The discriminator against the alternative fix #1417 rejected: + /// clamping the excluded *count* to `span - |retained rows|`. + /// + /// On `shared_row` the truth is `sloc 3, ploc 2, blank 1`; before + /// the fix it read `sloc 2, blank 0`; and the clamp reads + /// `min(1, 3 - 2) = 1`, so `sloc 2, blank 0` — no improvement at + /// all. It buys `ploc <= sloc` by moving the error into `blank`, + /// which is the outcome #1398's rationale warns against. Measured + /// against the built binary before the set-based fix landed, not + /// reasoned about. Without this fixture the fix is indistinguishable + /// from that alternative. + /// + /// `own_rows` is the control, in the same test because the headline + /// numbers must match: it reaches `(3, 2, 0, 1)` by genuinely + /// excluding one row where `shared_row` excludes none, so a fix + /// cannot buy the invariant by under-subtracting in the ordinary + /// rustfmt layout. + #[test] + fn a_blank_row_does_not_absorb_a_phantom_exclusion() { + // Row 0 `fn a` and the pruned `mod t`, row 1 blank, row 2 `fn b`. + let shared_row = rust_loc_pruned(b"fn a() {} #[cfg(test)] mod t {}\n\nfn b() {}\n"); + assert_eq!( + ( + shared_row.sloc(), + shared_row.ploc(), + shared_row.cloc(), + shared_row.blank() + ), + (3, 2, 0, 1), + "nothing is excluded: row 0 carries `fn a`" + ); + + // Row 0 `fn a`, row 1 blank, row 2 `#[cfg(test)]`, row 3 the + // pruned `mod t` — the only row the prune can take. + let own_rows = rust_loc_pruned(b"fn a() {}\n\n#[cfg(test)]\nmod t { #[test] fn x() {} }\n"); + assert_eq!( + ( + own_rows.sloc(), + own_rows.ploc(), + own_rows.cloc(), + own_rows.blank() + ), + (3, 2, 0, 1), + "one of four rows excluded; the attribute row stays" + ); + } + /// The non-unit half of the same off-by-one. tree-sitter-perl's /// `function_definition` swallows the newline after the closing brace /// of a file's **last** `sub`, so that node's span ends at column 0 of diff --git a/src/metrics/loc/line_set.rs b/src/metrics/loc/line_set.rs index 1b5d5fce..8aef5034 100644 --- a/src/metrics/loc/line_set.rs +++ b/src/metrics/loc/line_set.rs @@ -65,7 +65,11 @@ // use rather than by a property of the input — `reserve` runs before // each subtraction in `insert`/`insert_range`/`union_with`, `slot` and // `word` already use `checked_sub`, and `insert_range` returns early on -// an inverted span. +// an inverted span. `subtract` and `intersection_len` are the two sites +// with no `reserve` to lean on: both clip to the overlap of the two +// arrays first, so `start..end` is empty rather than inverted when the +// sets are disjoint, and every index inside it is in bounds for both +// operands by that construction. // // Making these saturating would be actively worse than leaving them // checked. `self.words[word - self.first_word]` saturating to index 0 @@ -251,6 +255,30 @@ impl LineSet { } } + /// Removes every row of `other` from `self`. + /// + /// The destructive counterpart of [`LineSet::intersection_len`], and + /// the reason it is destructive rather than a `difference_len` + /// counter: `Sloc` folds its residual upward through + /// [`LineSet::union_with`], so the *identity* of the surviving rows + /// has to outlive the call (#1417). + /// + /// Never allocates. Clearing a bit can only touch a word `self` + /// already holds, so — unlike `union_with` — there is no `reserve` + /// here and rows of `other` outside `self`'s covered interval are + /// simply not there to clear. `words.len()` and its capacity are + /// unchanged on every path. + pub(super) fn subtract(&mut self, other: &Self) { + let start = self.first_word.max(other.first_word); + let end = (self.first_word + self.words.len()).min(other.first_word + other.words.len()); + // Empty rather than inverted when the two intervals are + // disjoint, exactly as in `intersection_len`; inside it both + // indices are in bounds for their own array by construction. + for word in start..end { + self.words[word - self.first_word] &= !other.words[word - other.first_word]; + } + } + /// The rows in the set, ascending. /// /// Scans a full word per populated word, so it is for [`fmt::Debug`] @@ -574,6 +602,61 @@ mod tests { assert_eq!(only_comments.union_len(&LineSet::default()), 3); } + /// The live shape in `Stats::settle_excluded_rows`: the pruned rows + /// a retained sibling also occupies go, and the rows it solely + /// occupies stay. Overlap on both a shared word and a word only one + /// side holds. + #[test] + fn subtract_removes_only_the_shared_rows() { + let mut excluded = set_of(&[3, 64, 65, 300]); + excluded.subtract(&set_of(&[3, 65, 4_000])); + assert_eq!(rows_of(&excluded), vec![64, 300]); + } + + /// Disjoint in either direction is a no-op, which is the case every + /// space that pruned nothing takes. + #[test] + fn subtract_disjoint_sets_changes_nothing() { + let mut low = set_of(&[1, 2]); + low.subtract(&set_of(&[1_000, 1_001])); + assert_eq!(rows_of(&low), vec![1, 2]); + + let mut high = set_of(&[1_000, 1_001]); + high.subtract(&set_of(&[1, 2])); + assert_eq!(rows_of(&high), vec![1_000, 1_001]); + + let mut untouched = set_of(&[7]); + untouched.subtract(&LineSet::default()); + assert_eq!(rows_of(&untouched), vec![7]); + } + + /// Subtracting *from* a never-written set must not seed one. This is + /// the path every space with `exclude_tests` off takes, three times + /// per finalization. + #[test] + fn subtract_from_an_unallocated_set_allocates_nothing() { + let mut empty = LineSet::default(); + empty.subtract(&set_of(&[0, 5_000])); + assert_eq!(empty.len(), 0); + assert_eq!(empty.words.capacity(), 0, "an unused set must not allocate"); + } + + /// The load-bearing performance claim, and what distinguishes + /// `subtract` from `union_with`: clearing bits can never need a word + /// `self` does not already hold, so neither the length nor the + /// capacity of `words` may move for rows outside its interval. + #[test] + fn subtract_never_grows_the_word_array() { + let mut set = set_of(&[BITS_PER_WORD, BITS_PER_WORD + 1]); + let (len, capacity) = (set.words.len(), set.words.capacity()); + + set.subtract(&set_of(&[0, BITS_PER_WORD + 1, 10_000])); + + assert_eq!(rows_of(&set), vec![BITS_PER_WORD]); + assert_eq!(set.words.len(), len, "subtract must not resize"); + assert_eq!(set.words.capacity(), capacity, "subtract must not allocate"); + } + /// Equality is over rows, so two sets that reached the same rows /// through different offsets and different amounts of zero padding /// compare equal. `Loc::Stats` derives `PartialEq`. diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 4c9d41a3..64a03a84 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -232,6 +232,28 @@ fn clamp_loc_line_sets(state: &mut State, selected: MetricSet) { } } +/// Drops from the `exclude_tests`-pruned row set the rows that retained +/// code or comments also occupy, so `sloc()` subtracts only what the +/// prune took (#1417). +/// +/// Ordered after [`clamp_loc_line_sets`], which supplies the premise its +/// `ploc <= sloc` assertion rests on, and before `compute_minmax` and +/// `compute_halstead_and_mi`, both of which read the settled `sloc()` — +/// the former into `sloc_min`/`sloc_max`, the latter into MI's `ln(sloc)` +/// term. +/// +/// Runs per space and unconditionally, like the clamp: a space that +/// pruned nothing holds an empty, unallocated set, so the three +/// subtractions visit no words. The gate is the same +/// `selected.contains(Metric::Loc)`, so deselecting loc keeps the walk's +/// work identical. +#[inline] +fn settle_loc_excluded_rows(state: &mut State, selected: MetricSet) { + if selected.contains(Metric::Loc) { + state.space.metrics.loc.settle_excluded_rows(); + } +} + /// Runs the per-space finalization passes (unit-span anchoring, line-set /// clamping, min/max, sum, Halstead, MI, WMC, averages) on a single /// [`State`]. Shared by both the single-element and pop arms of @@ -242,7 +264,10 @@ fn clamp_loc_line_sets(state: &mut State, selected: MetricSet) { /// [`anchor_unit_sloc_span`] runs first because everything after it reads /// the span it fixes: `compute_minmax` folds `sloc` into the unit's /// `sloc_min`/`sloc_max`, and `compute_halstead_and_mi` feeds it into MI's -/// `ln(sloc)` term. +/// `ln(sloc)` term. [`settle_loc_excluded_rows`] sits between +/// [`clamp_loc_line_sets`] and `compute_minmax` for both halves of that +/// same reason: it needs the clamped line sets as its input, and it moves +/// `sloc()`, which those two later passes consume (#1417). /// /// [`finalize`]'s pop arm additionally calls [`compute_wmc`] on the /// *parent* before each child merges into it, because `wmc::Stats::merge` @@ -254,6 +279,7 @@ fn clamp_loc_line_sets(state: &mut State, selected: MetricSet) { fn finalize_state(state: &mut State, selected: MetricSet) { anchor_unit_sloc_span(state, selected); clamp_loc_line_sets(state, selected); + settle_loc_excluded_rows(state, selected); compute_minmax(state, selected); compute_sum(state, selected); compute_halstead_and_mi::(state, selected); @@ -778,13 +804,19 @@ pub(crate) fn metrics_inner( if options.exclude_tests && T::Checker::should_skip_subtree(&node, code, ancestors) { // `sloc` is span-based, not node-accumulated, so unlike every // other loc sub-metric it does not shrink just because we - // skip the subtree. Record the pruned node's row span on the + // skip the subtree. Record the pruned node's rows on the // innermost enclosing func-space so its `sloc` drops in step - // (#722); `Sloc::merge` then folds that count upward so every + // (#722); `Sloc::merge` then unions those rows upward so every // enclosing space — including the unit, which feeds MI's SLOC // term — drops too, even when the test item is nested in a // retained `impl`/`trait`/closure (#741). Gated on the `Loc` // selection so deselecting loc keeps the walk's work identical. + // + // The whole span goes in, shared rows included; the retained + // rows of this space are still arriving, so nothing here can + // tell a row the prune took from one it only touched. + // `Stats::settle_excluded_rows` subtracts the shared ones at + // finalization, when the retained sets are complete (#1417). if selected.contains(Metric::Loc) && let Some(state) = state_stack.last_mut() { diff --git a/src/spaces_tests.rs b/src/spaces_tests.rs index ad01fefe..9b98eb0f 100644 --- a/src/spaces_tests.rs +++ b/src/spaces_tests.rs @@ -1034,10 +1034,10 @@ impl Foo { assert_eq!(baseline_impl.metrics.loc.sloc(), 5); assert_eq!(pruned_impl.metrics.loc.sloc(), 4); - // Unit-root propagation (the #741 fix): the pruned line count - // folds upward through `Sloc::merge`, so the unit's `sloc` drops - // by the same one row. Before the fix this stayed at the - // baseline value because only the impl's `excluded_lines` grew. + // Unit-root propagation (the #741 fix): the pruned rows fold + // upward through `Sloc::merge`, so the unit's `sloc` drops by + // the same one row. Before the fix this stayed at the baseline + // value because only the impl's `excluded_rows` grew. assert_eq!(baseline.metrics.loc.sloc(), 5); assert_eq!(pruned.metrics.loc.sloc(), 4); } @@ -1096,8 +1096,8 @@ fn make() { } // A non-test `impl` with no test items must be unaffected by the - // upward-propagation fold: with nothing pruned, `excluded_lines` - // stays zero at every level, so pruned and baseline `sloc` agree. + // upward-propagation fold: with nothing pruned, `excluded_rows` + // stays empty at every level, so pruned and baseline `sloc` agree. #[test] fn non_test_impl_sloc_unaffected_by_pruning() { let source = "\ @@ -1117,6 +1117,40 @@ impl Calc { pruned.spaces[0].metrics.loc.sloc() ); } + + // #1417's aggregation pin. The pruned `#[test] fn t()` shares row 2 + // with the retained `fn prod`, so the prune removes no row at all + // and `sloc` must stay at the impl's three rows. Before the fix the + // impl reported `sloc 2` against its own `ploc 3`, and `Sloc::merge` + // carried that straight up: the unit read `sloc 2, ploc 3` too. Both + // altitudes are asserted because the child is where the row is + // recorded and the parent is where the fold could still lose it. + #[test] + fn a_pruned_method_sharing_a_row_shrinks_no_space() { + let source = "\ +impl Foo { + fn prod(&self) {} #[test] fn t() {} +} +"; + let baseline = analyse(source, false); + let pruned = analyse(source, true); + + // The `t` function space is gone; only the `impl` remains under + // the unit, and only `prod` under the impl. + assert_eq!(pruned.spaces.len(), 1); + assert_eq!(pruned.spaces[0].spaces.len(), 1); + assert_eq!(baseline.spaces[0].spaces.len(), 2); + + for space in [&pruned, &pruned.spaces[0]] { + assert_eq!( + (space.metrics.loc.sloc(), space.metrics.loc.ploc()), + (3, 3), + "{:?}: three rows, all code, none removable", + space.kind + ); + } + assert_eq!(baseline.metrics.loc.sloc(), pruned.metrics.loc.sloc()); + } } // Non-Rust languages must ignore `exclude_tests = true` because From dfab86cc044afb5d1008985dbbfbece3486b2c2e Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sat, 12 Sep 2026 20:38:28 -0700 Subject: [PATCH 05/12] fix(metrics/loc): keep the wrapper-less heredoc body #1412 replaced Bash's HeredocBody / HeredocBody2 arms with the heredoc_redirect wrapper, on the grounds that node-types.json lists heredoc_body as a child of heredoc_redirect and of nothing else. That describes the well-formed grammar. Under error recovery tree-sitter-bash emits an orphan body with no wrapper at all: a single-line compound carrying a heredoc parses to an {ERROR} root whose direct child is the heredoc_body. The leaf-gated catch-all then credits only its start row and the terminator row falls to blank -- #1412's own symptom, on three valid and executable spellings: f() { cat < 2, default => 0 };", /// the one that helper's own doc comment argues for. /// /// The two spellings differ only for a node whose end column is 0, - /// and the one string literal known to have that shape here is a + /// and the one *string literal* known to have that shape here is a /// Bash `heredoc_body`: it absorbs the newline after its last /// content row and so ends at column 0 of the terminator's row. /// Everywhere else a literal stops just past its closing delimiter, - /// where `end_line() - 1` and `end_row()` agree. + /// where `end_line() - 1` and `end_row()` agree. Since #1423 the + /// C-family `preproc_arg` is a second caller of this helper that + /// reaches column 0 — a macro body ending on a dangling backslash — + /// and `a_dangling_macro_continuation_does_not_credit_the_row_below` + /// covers that one end-to-end. /// /// The call is direct rather than through `bca metrics` /// deliberately — which node `bash.rs` routes is a separate @@ -11970,13 +12013,13 @@ class A { assert_eq!(nested.metrics.loc.blank(), 0, "the function has no blanks"); } - /// `Sloc::exclude_span` subtracts each pruned subtree's row count from - /// the enclosing span, so widening that span at the top could in - /// principle desynchronise the two. It cannot: the rows the anchor - /// adds are above the first token, and no pruned subtree can overlap - /// them. Pinned rather than argued, since the failure mode is a - /// silent `saturating_sub` clamp to 0 rather than a panic (#722, - /// #1247). + /// `Sloc::exclude_span` records each pruned subtree's rows and + /// `sloc()` subtracts their cardinality from the enclosing span, so + /// widening that span at the top could in principle desynchronise + /// the two. It cannot: the rows the anchor adds are above the first + /// token, and no pruned subtree can overlap them. Pinned rather than + /// argued, since the failure mode is a silent `saturating_sub` clamp + /// to 0 rather than a panic (#722, #1247, #1417). #[test] fn exclude_tests_pruning_composes_with_the_unit_anchor() { // Rows 1-3 blank, 4 `fn a`, 5 blank, 6 `#[test]`, 7-9 `fn t`. @@ -12108,6 +12151,31 @@ class A { ); } + /// The *comment* half of the settle, which the fixtures above cannot + /// reach because every one of them has `cloc 0`. + /// + /// `Stats::settle_excluded_rows` subtracts three sets from the + /// pruned rows — `ploc.lines` and both `Cloc` sets — and only the + /// first was covered: deleting **both** comment subtractions failed + /// 0 of 3,388 lib tests, while deleting the `ploc` one failed 3. A + /// comment sharing a row with a pruned item is retained text on that + /// row exactly as code would be, so the row must survive the prune. + #[test] + fn a_comment_sharing_a_pruned_items_row_keeps_that_row() { + // Row 0 `#[cfg(test)]`, row 1 the pruned `mod t` and a trailing + // comment. Row 1 holds no code once the mod is pruned, but the + // comment is still there, so the row is not the prune's to take. + let loc = rust_loc_pruned(b"#[cfg(test)]\nmod t {} // note\n"); + assert_eq!(loc.sloc(), 2, "the comment row survives the prune"); + assert_eq!(loc.cloc(), 1, "the trailing comment is still counted"); + assert!( + loc.ploc() <= loc.sloc(), + "ploc {} must not exceed sloc {}", + loc.ploc(), + loc.sloc() + ); + } + /// The non-unit half of the same off-by-one. tree-sitter-perl's /// `function_definition` swallows the newline after the closing brace /// of a file's **last** `sub`, so that node's span ends at column 0 of diff --git a/src/metrics/loc/bash.rs b/src/metrics/loc/bash.rs index dd172a9b..21a78bab 100644 --- a/src/metrics/loc/bash.rs +++ b/src/metrics/loc/bash.rs @@ -55,22 +55,33 @@ impl Loc for BashCode { // {heredoc_end:156} from (3, 1) to (3, 4) // // The body node is on the *terminator's* row and empty; row 2 is - // covered only by the wrapper. `HeredocBody` / `HeredocBody2` - // are dropped rather than kept alongside it, because the wrapper - // is a strict superset in every spelling: tree-sitter-bash - // 0.25.1's `node-types.json` lists `heredoc_body` as a child of - // `heredoc_redirect` and of nothing else, and the wrapper runs - // from `<<` on the command row to the end of `heredoc_end`, so - // its interior range already covers every body row and the - // terminator. Dumped and confirmed for `<<`, `<<-`, quoted - // (`<<'EOT'`), empty, unterminated, and heredocs inside a - // function, subshell, pipeline, command substitution and - // `&&` list. + // covered only by the wrapper. // - // Routing the wrapper also fixes a body that *has* text: - // `heredoc_content` is itself multi-row (`(64, 1)` to - // `(65, 24)` in the corpus's `generate-pc.sh`), so the - // leaf-gated catch-all credited only its first row. + // `HeredocBody` / `HeredocBody2` stay listed **alongside** the + // wrapper rather than being replaced by it — section 6's "narrow + // with a gate, never by deletion". `node-types.json` does list + // `heredoc_body` only as a child of `heredoc_redirect`, but that + // describes the well-formed grammar and says nothing about error + // recovery, where tree-sitter-bash emits an **orphan** body with + // no wrapper anywhere. A single-line compound carrying a heredoc + // is the shape, and all three spellings are valid, executable + // Bash: + // + // f() { cat < { + String | RawString | AnsiCString | HeredocRedirect | HeredocBody | HeredocBody2 => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); add_string_interior_ploc(node, stats, start); From f895224af70cd0a8ea9fb4bed807575f614245d3 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 06:02:15 -0700 Subject: [PATCH 06/12] refactor(metrics/loc): clip both word walks in one place `subtract` and `intersection_len` are the two `LineSet` methods with no `reserve` to lean on for their index arithmetic, so each open-coded the same clip to the overlap of the two word arrays, and the module header's `arithmetic_side_effects` carve-out had to state the bounds argument for both at once. Extract `overlapping_words`, which carries that argument once: the range is empty rather than inverted when the sets are disjoint, and every index in it is in bounds for both operands by construction. Behaviour is identical. --- src/metrics/loc/line_set.rs | 40 +++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/metrics/loc/line_set.rs b/src/metrics/loc/line_set.rs index 8aef5034..76d1a8f6 100644 --- a/src/metrics/loc/line_set.rs +++ b/src/metrics/loc/line_set.rs @@ -66,10 +66,10 @@ // each subtraction in `insert`/`insert_range`/`union_with`, `slot` and // `word` already use `checked_sub`, and `insert_range` returns early on // an inverted span. `subtract` and `intersection_len` are the two sites -// with no `reserve` to lean on: both clip to the overlap of the two -// arrays first, so `start..end` is empty rather than inverted when the -// sets are disjoint, and every index inside it is in bounds for both -// operands by that construction. +// with no `reserve` to lean on: both clip to `overlapping_words` first, +// which is empty rather than inverted when the sets are disjoint and +// whose every index is in bounds for both operands by that +// construction. // // Making these saturating would be actively worse than leaving them // checked. `self.words[word - self.first_word]` saturating to index 0 @@ -269,12 +269,7 @@ impl LineSet { /// simply not there to clear. `words.len()` and its capacity are /// unchanged on every path. pub(super) fn subtract(&mut self, other: &Self) { - let start = self.first_word.max(other.first_word); - let end = (self.first_word + self.words.len()).min(other.first_word + other.words.len()); - // Empty rather than inverted when the two intervals are - // disjoint, exactly as in `intersection_len`; inside it both - // indices are in bounds for their own array by construction. - for word in start..end { + for word in self.overlapping_words(other) { self.words[word - self.first_word] &= !other.words[word - other.first_word]; } } @@ -316,15 +311,26 @@ impl LineSet { .unwrap_or(0) } - /// Number of rows in `self ∩ other`. - fn intersection_len(&self, other: &Self) -> usize { + /// Absolute word indices both arrays cover, ascending. + /// + /// The bounds premise [`LineSet::intersection_len`] and + /// [`LineSet::subtract`] lean on in place of a `reserve`, in one + /// place because both need it stated the same way: the range is + /// empty rather than inverted when the two intervals are disjoint + /// (`start > end` yields no iterations rather than panicking), and + /// every index inside it is in bounds for `self` *and* for `other` + /// by construction — so an offset that drifted panics on the index + /// rather than silently reading the wrong row's word. + #[inline] + fn overlapping_words(&self, other: &Self) -> std::ops::Range { let start = self.first_word.max(other.first_word); let end = (self.first_word + self.words.len()).min(other.first_word + other.words.len()); - // Empty when the two spans are disjoint: `start > end` yields an - // empty range rather than panicking. Inside it both indices are - // in bounds by construction, so a bound that drifted would panic - // here rather than silently miscount. - (start..end) + start..end + } + + /// Number of rows in `self ∩ other`. + fn intersection_len(&self, other: &Self) -> usize { + self.overlapping_words(other) .map(|word| { (self.words[word - self.first_word] & other.words[word - other.first_word]) .count_ones() as usize From 4ccd07af06e2105df30446b32ff7d36da6d1c0a1 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 06:26:57 -0700 Subject: [PATCH 07/12] test(metrics/loc): make two unbacked coverage claims honest A perturbation sweep over this branch found one production line no test could fail on and one fixture with no decay anchor. `settle_excluded_rows`'s third subtraction, over `code_comment_line_starts`, failed 0 of 3,390 lib tests when deleted, where dropping either of the other two fails 3 and 1. It is redundant by construction: the two `add_cloc_lines` writers test `ploc.lines.contains(start)` first, and every per-language call site of `check_comment_ends_on_code_line` inserts that same row into `ploc.lines` on the next statement. A `debug_assert_eq!` now pins that premise on every space of every walk, so the line is defence with a live guard rather than unreachable weight. Verified to fire by dropping the precondition from `add_cloc_lines`. `a_blank_row_does_not_absorb_a_phantom_exclusion`'s `shared_row` keeps all four numbers when the pruned `mod t {}` is trimmed out of it, and no axis can change that: an excluded item sharing its only row with retained code is what "metrically invisible" means. Recorded in the doc comment rather than papered over or overstated, per `.claude/rules/testing.md`. Its `own_rows` sibling is anchored in the ordinary way -- trimming that `mod` fails `sloc 3`. --- src/metrics/loc.rs | 53 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index d3ffb6a9..da9383b5 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -953,12 +953,22 @@ impl Stats { /// of the enclosing space are still arriving; this pass runs once /// per space at finalization, when they are all in. /// - /// All three retained sets are subtracted. - /// `code_comment_line_starts` is very probably a subset of - /// `ploc.lines` — a row with both code and a comment is a code row - /// — but nothing enforces that, and the cost of not relying on it - /// is one word-wise `&= !` over an array that is empty for every - /// space that pruned nothing. + /// All three retained sets are subtracted, but only two of them can + /// move the answer, and that is worth stating rather than hedging: + /// `code_comment_line_starts` is a subset of `ploc.lines` by + /// construction. Its only writers are the two `add_cloc_lines` arms, + /// both gated on `ploc.lines.contains(start)` already being true, + /// and `check_comment_ends_on_code_line`, whose per-language call + /// sites every one insert that same row into `ploc.lines` on the + /// next statement. `Stats::merge` unions both sets and + /// [`Stats::clamp_line_sets_to_span`] retains both over one range, + /// so no later pass can separate them. Deleting the third + /// subtraction therefore fails no test and *cannot* — measured at 0 + /// of 3,390 lib tests, where dropping either of the other two fails + /// 3 and 1. It stays as defence against a future call site that + /// forgets the PLOC insert, and the `debug_assert_eq!` opening the + /// body is what keeps that defence honest: without it the line is + /// unreachable weight whose loss nothing would ever notice. /// /// Ordered after [`Stats::clamp_line_sets_to_span`], which /// establishes the premise: with `P`, `O`, `C` and the pruned set @@ -973,6 +983,18 @@ impl Stats { /// overlap on a code-and-comment row, so no lower bound on /// `sloc - ploc - |O|` follows from the above. pub(crate) fn settle_excluded_rows(&mut self) { + // The premise that makes the third subtraction below redundant, + // pinned on every space of every walk because it is a property + // of all the `check_comment_ends_on_code_line` call sites at + // once and no single fixture can reach them. + debug_assert_eq!( + self.ploc + .lines + .union_len(&self.cloc.code_comment_line_starts), + self.ploc.lines.len(), + "a row carrying both code and a comment must also be a code row" + ); + // Disjoint field paths, so the shorthand borrows cleanly. let excluded = &mut self.sloc.excluded_rows; excluded.subtract(&self.ploc.lines); @@ -12121,6 +12143,25 @@ class A { /// excluding one row where `shared_row` excludes none, so a fix /// cannot buy the invariant by under-subtracting in the ordinary /// rustfmt layout. + /// + /// **`shared_row` has no fixture-decay anchor, and none exists.** + /// The rule in `.claude/rules/testing.md` asks for a second axis + /// only the excluded construct contributes to, so that trimming it + /// out of the fixture fails this test rather than quietly hollowing + /// it. Deleting `#[cfg(test)] mod t {}` from row 0 leaves all four + /// numbers at `(3, 2, 0, 1)` — measured, 0 of 3,390 lib tests fail + /// — and that is not an oversight but the claim restated: a pruned + /// item whose only row a retained sibling also occupies is *by + /// definition* metrically indistinguishable from not being there. + /// Every candidate axis collapses the same way. The unpruned + /// reading is also `(3, 2, 0, 1)`, because `mod t {}` adds no row + /// of its own; the space tree matches too, because the prune + /// removes the space the construct would have opened. The coverage + /// here is the production perturbation, which was run: this test is + /// among the failures for dropping the `ploc` subtraction, for + /// widening `exclude_span`'s bound by one, and for both `subtract` + /// mutations. `own_rows` is anchored in the ordinary way — trim its + /// pruned `mod` and `sloc 3` fails. #[test] fn a_blank_row_does_not_absorb_a_phantom_exclusion() { // Row 0 `fn a` and the pruned `mod t`, row 1 blank, row 2 `fn b`. From bcd62812454811888b01223aa4a1fdcb73abc94c Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 07:16:13 -0700 Subject: [PATCH 08/12] fix(ast): prune a test item's attribute row (#1431) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An outer attribute is an `AttributeItem` sibling of the item it marks, not a child, so `--exclude-tests` pruning the item never reached it and Rust's `Loc` catch-all credited its start row to PLOC. A file that was nothing but test code reported `sloc 1, ploc 1`, and every `#[cfg(test)] mod tests` cost its file one phantom row per attribute row. `should_skip_subtree` now also answers for an `AttributeItem`, looking ahead over its run to the item it decorates and pruning only when that item is one the prune removes anyway — so the attribute on a `use` or a `struct` is untouched. Answering before the row is recorded, rather than retracting it afterwards, is what keeps #1417's shared-row case correct: a row the attribute happens to share with retained code is still inserted by that code. The lookahead dispatches on the same depth-scaled budget as the backward run reading, for #1100's reason with the directions swapped. Complexity class is unchanged on all 28 scaling probes (`nom/wide-attributed-fn` 1.02 to 1.04, `nom/nested-attributed-fn` 1.00 to 1.02, `loc/wide-cfg-test-mod` 1.13 to 1.09). The constant is not free: a generated 20_000-item `#[inline] fn f() {}` file goes 245 ms to 305 ms in release, while this repository's own 90 kloc of Rust does not move off 59 ms. `loc/wide-cfg-test-mod` gains a retained `fn p()` per item — its reading was the surviving attribute row, which is now zero. Metric drift: `loc.ploc`, `loc.sloc` and the MI values derived from them fall for `--exclude-tests` runs over Rust with test items. The default (`--exclude-tests` off) is byte-identical, and the six `loc.ploc` baseline entries that moved are refreshed here. Fixes #1431 --- .bca-baseline.toml | 14 +- CHANGELOG.md | 19 +++ big-code-analysis-ast/src/checker.rs | 163 ++++++++++++++++--- big-code-analysis-ast/src/checker/rust.rs | 28 ++-- big-code-analysis-ast/src/node.rs | 19 +++ big-code-analysis-bench/src/shapes.rs | 20 +-- src/metrics/loc.rs | 58 ++++--- src/spaces_tests.rs | 185 ++++++++++++++++++---- 8 files changed, 400 insertions(+), 106 deletions(-) diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 68e8e095..86835f9e 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -15,7 +15,7 @@ headroom = 0.95 path = "big-code-analysis-ast/src/alterator.rs" qualified = "" metric = "loc.ploc" -value = 561.0 +value = 550.0 [[entry]] path = "big-code-analysis-ast/src/alterator.rs" @@ -267,7 +267,7 @@ value = 120805.48245794396 path = "big-code-analysis-ast/src/node.rs" qualified = "Node<'a>" metric = "nom" -value = 35.0 +value = 36.0 [[entry]] path = "big-code-analysis-ast/src/parser.rs" @@ -561,7 +561,7 @@ value = 59351.805921378844 path = "big-code-analysis-cli/src/markdown_report.rs" qualified = "" metric = "loc.ploc" -value = 588.0 +value = 575.0 [[entry]] path = "big-code-analysis-cli/src/markdown_report.rs" @@ -591,7 +591,7 @@ value = 6.0 path = "big-code-analysis-cli/src/markdown_report/hotspot.rs" qualified = "" metric = "loc.ploc" -value = 696.0 +value = 694.0 [[entry]] path = "big-code-analysis-cli/src/markdown_report/hotspot.rs" @@ -627,7 +627,7 @@ value = 66737.04848699087 path = "big-code-analysis-cli/src/thresholds.rs" qualified = "" metric = "loc.ploc" -value = 562.0 +value = 549.0 [[entry]] path = "big-code-analysis-cli/src/thresholds.rs" @@ -639,7 +639,7 @@ value = 5.0 path = "big-code-analysis-cli/src/vcs_command.rs" qualified = "" metric = "loc.ploc" -value = 532.0 +value = 531.0 [[entry]] path = "big-code-analysis-cli/src/vcs_command.rs" @@ -819,7 +819,7 @@ value = 6.0 path = "big-code-analysis-py/src/types_codegen.rs" qualified = "" metric = "loc.ploc" -value = 748.0 +value = 747.0 [[entry]] path = "big-code-analysis-py/src/vcs.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c124904..51b1f2dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,25 @@ for historical reference. ### Fixed +- **`--exclude-tests` now prunes the `#[cfg(test)]` / `#[test]` + attribute along with the item it marks** (#1431). An outer attribute + is an `AttributeItem` *sibling* of its item, not a child, so pruning + the item never reached it and Rust's `Loc` catch-all credited its + start row to PLOC. A file that was nothing but test code reported + `sloc 1, ploc 1` — one row of code in a file with no production code + — and every `#[cfg(test)] mod tests` at the foot of a production file + inflated that file by one row, a stacked attribute run by one row per + attribute. `Checker::should_skip_subtree` now also answers for an + attribute, looking ahead over its run to the item it decorates and + pruning only when that item is one the prune removes anyway; the + attribute on a `use`, a `struct`, or any other kind `--exclude-tests` + keeps is untouched. **Metric drift:** `loc.ploc`, `loc.sloc` and the + MI values derived from them fall for `--exclude-tests` runs over Rust + containing test items, by one row per attribute row that is not + shared with retained code; `--exclude-tests` off — the default — is + byte-identical. A wholly test-only file now measures `sloc 0`, which + scores MI `0.0` on all three formulas through the existing + empty-input guard. - **`--exclude-tests` no longer drops rows a pruned item shares with retained code**, which made `sloc` fall below `ploc` (#1417). `Sloc` accumulated each pruned subtree's whole row span as a diff --git a/big-code-analysis-ast/src/checker.rs b/big-code-analysis-ast/src/checker.rs index 4f2f5af5..b65169ba 100644 --- a/big-code-analysis-ast/src/checker.rs +++ b/big-code-analysis-ast/src/checker.rs @@ -728,6 +728,99 @@ fn rust_item_is_test_only<'a>(node: &Node<'a>, code: &[u8], ancestors: Ancestors rust_outer_attr_marks_test(node, code, ancestors) || rust_inner_attr_marks_test(node, code) } +/// The item kinds the `exclude_tests` prune removes, given a test-only +/// verdict. +/// +/// `RustCode::should_skip_subtree` and +/// [`rust_attribute_run_marks_test_item`] both consult this, so an +/// attribute run and the item it decorates can never disagree about +/// whether that item is prunable: `#[cfg(test)] use foo;` keeps both +/// rows, because a `use_declaration` is not on this list (#1431). +fn rust_prunable_item(node: &Node) -> bool { + matches!( + node.kind_id().into(), + Rust::ModItem + | Rust::FunctionItem + | Rust::ImplItem + | Rust::TraitItem + | Rust::ConstItem + | Rust::StaticItem + ) +} + +/// Whether the outer-attribute run `node` belongs to decorates an item +/// the prune removes — which makes `node`'s own row test code too. +/// +/// The whole run is asked about the *item*, not about itself, so every +/// row of a stacked `#[cfg(test)]\n#[allow(dead_code)]\nmod t` goes: +/// the second attribute says nothing about tests, and +/// [`rust_item_is_test_only`] reads the run from the item end either +/// way (#1431). +fn rust_attribute_run_marks_test_item<'a>( + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, +) -> bool { + rust_attributed_item(node, ancestors).is_some_and(|item| { + // `item` is `node`'s sibling, so the chain that describes one + // describes the other — parent and depth alike. + rust_prunable_item(&item) && rust_item_is_test_only(&item, code, ancestors) + }) +} + +/// The item an outer-attribute run decorates: the first sibling after +/// `node` that is not itself an `AttributeItem`. +/// +/// Dispatched on [`forward_attribute_scan_budget`] for the reason the +/// backward reading of the same run is (#1100), with the directions +/// swapped: a cursor pass over a narrow parent's children is `O(width)` +/// and depth-free, while [`Node::next_sibling`] descends from the root +/// per step. Reading forward unconditionally would be `O(width)` *per +/// attribute*, which is quadratic on the flat `#[derive]`-per-item file +/// `bindgen` emits — the exact shape `nom/wide-attributed-fn` prices. +/// +/// A `parent` that is not `node`'s parent leaves the first pass with +/// nothing to skip to and answers `None`, i.e. "nothing is pruned" — +/// the same conservative reading `rust_attribute_run_under` takes, and +/// a caller error either way (`Ancestors::checked`, #1122). +/// +/// # What this costs +/// +/// An attribute now pays a lookahead plus the run reading the item +/// pays, so a file of nothing but attributed items resolves about 2.5x +/// the siblings it did. Measured in release: a generated 20 000-item +/// `#[inline] fn f() {}` file went 245 ms to 305 ms and a 3 000-deep +/// nest 33 ms to 35 ms, while this repository's own 90 kloc of Rust did +/// not move off 59 ms — real code is a few percent attributed, not all +/// of it. Every scaling probe kept its exponent (`nom/wide-attributed- +/// fn` 1.02 to 1.04, `nom/nested-attributed-fn` 1.00 to 1.02), which is +/// the property that matters: the constant is the price of asking, and +/// the class is what #1100 was about. +/// +/// Reusing [`rust_item_is_test_only`] rather than fusing the two walks +/// into one pass is deliberate at that price. A fused scan would save +/// one of the three sibling resolutions and would be a third reading of +/// the same run, free to drift from the other two — the failure mode +/// `.claude/rules/grammar-dispatch.md` §7 is about. +fn rust_attributed_item<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>) -> Option> { + match ancestors.parent(node) { + Some(parent) if parent.child_count() <= forward_attribute_scan_budget(ancestors) => parent + .children() + .skip_while(|child| child.id() != node.id()) + .find(|child| child.kind_id() != Rust::AttributeItem), + _ => { + let mut sibling = node.next_sibling(); + while let Some(candidate) = sibling { + if candidate.kind_id() != Rust::AttributeItem { + return Some(candidate); + } + sibling = candidate.next_sibling(); + } + None + } + } +} + /// Children a parent may have before reading its child list costs more /// than resolving the sibling from `node` itself. /// @@ -1413,24 +1506,32 @@ mod tests { /// The same equivalence one level up, on the hook the walker calls. /// /// `rust_item_is_test_only` folds the inner-attribute scan in, and - /// `should_skip_subtree` adds the item-kind filter, so pinning it - /// here is what carries the predicate-level agreement above through - /// to the set of subtrees `exclude_tests` actually prunes. + /// `should_skip_subtree` adds the item-kind filter and the #1431 + /// attribute arm, so pinning it here is what carries the + /// predicate-level agreement above through to the set of subtrees + /// `exclude_tests` actually prunes. + /// + /// The oracle reads every run backward and every lookahead forward + /// with the raw sibling accessors — no depth-against-width budget — + /// so it disagrees with production the moment either dispatch arm + /// answers differently from the other. The fixture spans both: + /// `source_file` is wide enough at depth 1 to take the sibling + /// arm, and `mod narrow`'s body is not. #[test] fn rust_should_skip_subtree_matches_the_backward_reading() { let source = "#[cfg(test)]\nmod tests {\nfn a() {}\n}\n\ #[allow(dead_code)]\nstatic S: i32 = 1;\n\ mod inner {\n#![cfg(test)]\nconst C: i32 = 1;\n}\n\ - #[rstest]\nfn b() {}\nfn c() {}\n"; + #[rstest]\nfn b() {}\nfn c() {}\n\ + #[cfg(test)]\nuse std::fmt;\n\ + #[cfg(test)]\n#[allow(dead_code)]\nfn d() {}\n\ + mod narrow {\n#[cfg(test)]\nfn e() {}\n}\n"; let code = source.as_bytes(); - let mut pruned = 0_usize; - let visited = for_each_node_with_chain::(code, |node, chain| { - let reference = - rust_attribute_run_before(node, code) || rust_inner_attr_marks_test(node, code); - // Spelled out rather than reused from the production hook: - // the point is to pin which kinds the prune considers, so - // borrowing the production `matches!` would assert nothing. - let is_item = matches!( + // Spelled out rather than reused from the production predicate: + // the point is to pin which kinds the prune considers, so + // borrowing `rust_prunable_item` would assert nothing. + let is_item = |node: &Node| { + matches!( node.kind_id().into(), Rust::ModItem | Rust::FunctionItem @@ -1438,11 +1539,28 @@ mod tests { | Rust::TraitItem | Rust::ConstItem | Rust::StaticItem - ); + ) + }; + let is_test_only = |node: &Node| { + rust_attribute_run_before(node, code) || rust_inner_attr_marks_test(node, code) + }; + let mut pruned = 0_usize; + let visited = for_each_node_with_chain::(code, |node, chain| { + let reference = if node.kind_id() == Rust::AttributeItem { + // Walk to the item this run decorates with the raw + // forward accessor, independent of the budget dispatch. + let mut following = node.next_sibling(); + while following.is_some_and(|s| s.kind_id() == Rust::AttributeItem) { + following = following.and_then(|s| s.next_sibling()); + } + following.is_some_and(|item| is_item(&item) && is_test_only(&item)) + } else { + is_item(node) && is_test_only(node) + }; let skipped = RustCode::should_skip_subtree(node, code, Ancestors::known(chain)); assert_eq!( skipped, - is_item && reference, + reference, "prune decision moved on {} at row {}", node.kind(), node.start_row(), @@ -1450,11 +1568,18 @@ mod tests { pruned += usize::from(skipped); }); assert!(visited > 0, "fixture must have nodes to compare"); - // `mod tests`, `mod inner`, and `fn b` — the three test-only - // items, and no more: a hook that pruned everything would - // satisfy the equality above only if the oracle agreed, but - // this pins the count the fixture was written for. - assert_eq!(pruned, 3, "expected exactly the three test-only items"); + // Five test-only items — `mod tests`, `mod inner`, `fn b`, + // `fn d`, `fn e` — plus the five `#[…]` rows that mark four of + // them (`fn d` carries two, `mod inner` an inner attribute that + // its own subtree covers). Neither `#[allow(dead_code)]` on the + // production `static`, nor `#[cfg(test)]` on the `use`, is the + // prune's to take. A hook that pruned everything would satisfy + // the equality above only if the oracle agreed; this pins the + // count the fixture was written for. + assert_eq!( + pruned, 10, + "expected the five items and their five attributes" + ); } #[test] diff --git a/big-code-analysis-ast/src/checker/rust.rs b/big-code-analysis-ast/src/checker/rust.rs index 2ae4fc78..bc6debdb 100644 --- a/big-code-analysis-ast/src/checker/rust.rs +++ b/big-code-analysis-ast/src/checker/rust.rs @@ -102,22 +102,26 @@ impl Checker for RustCode { /// Skip the subtree when `node` is a `mod`, `fn`, `impl`, /// `trait`, `const`, or `static` item marked test-only by an /// outer or inner attribute (`#[test]`, `#[cfg(test)]`, - /// `#[tokio::test]`, `#![cfg(test)]`, …). The runtime guard + /// `#[tokio::test]`, `#![cfg(test)]`, …), or is one of the + /// `#[…]` siblings that mark it. The runtime guard /// in `spaces::metrics_with_options` only consults this hook /// when the caller opts in via `MetricsOptions::exclude_tests`, /// so the default `metrics()` entry point is unaffected. fn should_skip_subtree<'a>(node: &Node<'a>, code: &[u8], ancestors: Ancestors<'a, '_>) -> bool { - if !matches!( - node.kind_id().into(), - Rust::ModItem - | Rust::FunctionItem - | Rust::ImplItem - | Rust::TraitItem - | Rust::ConstItem - | Rust::StaticItem - ) { - return false; + if rust_prunable_item(node) { + return rust_item_is_test_only(node, code, ancestors); } - rust_item_is_test_only(node, code, ancestors) + // An outer attribute is a *sibling* of the item it marks, not a + // child, so pruning the item never reached it and Rust's `Loc` + // catch-all credited its start row to PLOC — one phantom row per + // pruned item, and a file that is nothing but a `#[cfg(test)] + // mod` still reading `ploc 1` (#1431). Answering here, before the + // row is ever recorded, is what makes the shared-row case behave: + // a row the attribute happens to share with retained code is + // still inserted by that code, and `Stats::settle_excluded_rows` + // keeps it (#1417). Retracting the row afterwards could not tell + // the two apart. + node.kind_id() == Rust::AttributeItem + && rust_attribute_run_marks_test_item(node, code, ancestors) } } diff --git a/big-code-analysis-ast/src/node.rs b/big-code-analysis-ast/src/node.rs index 08c1bb3d..63c8aedd 100644 --- a/big-code-analysis-ast/src/node.rs +++ b/big-code-analysis-ast/src/node.rs @@ -346,6 +346,25 @@ impl<'a> Node<'a> { self.0.prev_sibling().map(Node) } + /// The sibling immediately after this node. + /// + /// **`O(depth)`, not `O(1)`**, for [`previous_sibling`]'s reason: + /// `ts_node_next_sibling` opens with `ts_node_parent`. There is no + /// [`Ancestors`] counterpart because the only caller wants exactly + /// this cost — the `exclude_tests` attribute lookahead reads forward + /// over the parent's children whenever the parent is narrow enough, + /// and falls back here precisely for the wide-and-shallow parent a + /// cursor pass would make `O(width)` per node (#1431). Anything on a + /// walk that does not budget the two against each other should be + /// reading the child list instead. + /// + /// [`previous_sibling`]: Self::previous_sibling + #[inline] + pub(crate) fn next_sibling(&self) -> Option> { + node_resolved_sibling_lookups::record(); + self.0.next_sibling().map(Node) + } + /// Returns `true` if any direct child has the given grammar /// `kind_id`. See #217 for the motivating perf finding from the /// JS/TS template-literal hot path. diff --git a/big-code-analysis-bench/src/shapes.rs b/big-code-analysis-bench/src/shapes.rs index 8fc07ab9..b9132baa 100644 --- a/big-code-analysis-bench/src/shapes.rs +++ b/big-code-analysis-bench/src/shapes.rs @@ -294,21 +294,23 @@ pub fn wide_attributed_fns(width: usize) -> String { format!("{}\n", "#[inline] fn f() {} ".repeat(width)) } -/// Rust: `#[cfg(test)]\nmod m {}\n` repeated at file scope. +/// Rust: `fn p() {}\n#[cfg(test)]\nmod m {}\n` repeated at file scope. /// /// The shape [`nested_attributed_fns`] and [`wide_attributed_fns`] /// cannot be: both use `#[inline]` precisely so the walk does *not* /// prune, which leaves the number of pruned nodes at zero on every /// existing probe and the prune's own bookkeeping unpriced (#1417). -/// Here every item is pruned, so the count grows with the size +/// Here two nodes per item are pruned — the `mod` and, since #1431, the +/// `#[cfg(test)]` sibling marking it — so the count grows with the size /// parameter. /// -/// Two rows per item, with the attribute on its own row, is what makes -/// the reading move: the attribute is an `AttributeItem` *sibling* of -/// the item, so it is walked normally and its row survives, while the -/// `mod` row does not. `sloc` is therefore `width` against a `2 * -/// width` file, and a prune that stopped recording rows would read as -/// `2 * width`. +/// The retained `fn p()` is what keeps the reading non-zero and moving +/// with the size: `sloc` is `width` against a `3 * width` file, so a +/// prune that stopped recording rows would read `3 * width` and one +/// that took the production row with them would read `0`. Before #1431 +/// the surviving attribute row played that part, and the shape was two +/// rows per item with no production code at all — which now measures +/// zero and prices nothing. /// /// The cost this prices is `Sloc::excluded_rows`: one `insert_range` /// per pruned node during the walk, and one `retain_range` plus three @@ -319,7 +321,7 @@ pub fn wide_attributed_fns(width: usize) -> String { /// visits them ascending; this probe is what keeps that measured. #[must_use] pub fn wide_cfg_test_mods(width: usize) -> String { - "#[cfg(test)]\nmod m {}\n".repeat(width) + "fn p() {}\n#[cfg(test)]\nmod m {}\n".repeat(width) } /// Rust: `fn f000000() { let v000000 = 000000; } fn f000001() { … }`, diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index da9383b5..aa2edd27 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -12052,15 +12052,14 @@ class A { MetricsOptions::default().with_exclude_tests(true), ) .loc; - // The pruned node is the `fn t` item, rows 7-9; its `#[test]` - // attribute is a sibling and stays, which is #722's shape and not - // something the anchor changes. What the anchor decides is the - // other end: `blank` is 4 rather than 1, because rows 1-3 are now - // inside the span the pruning subtracts from. + // The prune takes the `fn t` item's rows 7-9 and, since #1431, + // the `#[test]` sibling on row 6 that marks it. What the anchor + // decides is the other end: `blank` is 4 rather than 1, because + // rows 1-3 are now inside the span the pruning subtracts from. assert_eq!( (kept.sloc(), kept.ploc(), kept.cloc(), kept.blank()), - (6, 2, 0, 4), - "the three pruned rows leave; the three leading blanks stay" + (5, 1, 0, 4), + "the four pruned rows leave; the three leading blanks stay" ); let unpruned = rust_loc(source); @@ -12096,13 +12095,15 @@ class A { /// half test code reads as under-subtraction and is not — nothing /// on that row can be removed without removing `fn a()` with it. /// - /// `ploc 1` has a second, separate cause worth not confusing with - /// this one: `should_skip_subtree` matches the *item*, and a - /// `#[cfg(test)]` / `#[test]` attribute is an `AttributeItem` - /// sibling of it, so the attribute is walked normally and Rust's - /// catch-all credits its start row to PLOC. A file that is 100% - /// test code therefore still reports `ploc 1`. That is a different - /// defect, tracked as #1431. + /// A second, separate cause used to add a row here and is worth not + /// confusing with this one: `should_skip_subtree` matched only the + /// *item*, and a `#[cfg(test)]` / `#[test]` attribute is an + /// `AttributeItem` sibling of it, so the attribute was walked + /// normally and Rust's catch-all credited its start row to PLOC — a + /// file that was 100% test code still reported `ploc 1` (#1431, now + /// fixed). These fixtures put the attribute on the shared row, so + /// that fix changes neither reading: what holds row 0 here is + /// `fn a()`, not the attribute. #[test] fn a_pruned_item_sharing_a_row_with_retained_code_keeps_the_row() { for source in [ @@ -12138,11 +12139,11 @@ class A { /// reasoned about. Without this fixture the fix is indistinguishable /// from that alternative. /// - /// `own_rows` is the control, in the same test because the headline - /// numbers must match: it reaches `(3, 2, 0, 1)` by genuinely - /// excluding one row where `shared_row` excludes none, so a fix - /// cannot buy the invariant by under-subtracting in the ordinary - /// rustfmt layout. + /// `own_rows` is the control, in the same test because the two must + /// not be confusable: it genuinely excludes rows — both of the + /// pruned item's own, since #1431 took the attribute with it — where + /// `shared_row` excludes none, so a fix cannot buy the invariant by + /// under-subtracting in the ordinary rustfmt layout. /// /// **`shared_row` has no fixture-decay anchor, and none exists.** /// The rule in `.claude/rules/testing.md` asks for a second axis @@ -12161,7 +12162,7 @@ class A { /// among the failures for dropping the `ploc` subtraction, for /// widening `exclude_span`'s bound by one, and for both `subtract` /// mutations. `own_rows` is anchored in the ordinary way — trim its - /// pruned `mod` and `sloc 3` fails. + /// pruned `mod` and `sloc 2` fails. #[test] fn a_blank_row_does_not_absorb_a_phantom_exclusion() { // Row 0 `fn a` and the pruned `mod t`, row 1 blank, row 2 `fn b`. @@ -12178,7 +12179,8 @@ class A { ); // Row 0 `fn a`, row 1 blank, row 2 `#[cfg(test)]`, row 3 the - // pruned `mod t` — the only row the prune can take. + // pruned `mod t` — rows 2 and 3 are both the prune's to take, + // the attribute along with the item it marks (#1431). let own_rows = rust_loc_pruned(b"fn a() {}\n\n#[cfg(test)]\nmod t { #[test] fn x() {} }\n"); assert_eq!( ( @@ -12187,8 +12189,8 @@ class A { own_rows.cloc(), own_rows.blank() ), - (3, 2, 0, 1), - "one of four rows excluded; the attribute row stays" + (2, 1, 0, 1), + "two of four rows excluded; only `fn a` and the blank stay" ); } @@ -12204,11 +12206,15 @@ class A { #[test] fn a_comment_sharing_a_pruned_items_row_keeps_that_row() { // Row 0 `#[cfg(test)]`, row 1 the pruned `mod t` and a trailing - // comment. Row 1 holds no code once the mod is pruned, but the - // comment is still there, so the row is not the prune's to take. + // comment. Both rows are pruned — the attribute since #1431 — + // but row 1 holds retained text, so only row 0 is the prune's to + // take. `sloc 1` against `sloc 0` is exactly the comment + // subtraction: without it both rows go and a file with a + // surviving comment reports no lines at all. let loc = rust_loc_pruned(b"#[cfg(test)]\nmod t {} // note\n"); - assert_eq!(loc.sloc(), 2, "the comment row survives the prune"); + assert_eq!(loc.sloc(), 1, "the comment row survives the prune"); assert_eq!(loc.cloc(), 1, "the trailing comment is still counted"); + assert_eq!(loc.ploc(), 0, "no production code is left on either row"); assert!( loc.ploc() <= loc.sloc(), "ploc {} must not exceed sloc {}", diff --git a/src/spaces_tests.rs b/src/spaces_tests.rs index 9b98eb0f..f0c59a42 100644 --- a/src/spaces_tests.rs +++ b/src/spaces_tests.rs @@ -902,13 +902,13 @@ mod tests { // step with the pruned test module. // // Layout (0-based rows): prod body rows 0..=3, blank row 4, - // the `#[cfg(test)]` attribute (a sibling of `mod_item`, NOT - // pruned) row 5, and the pruned `mod tests { … }` rows 6..=11. - // Baseline `sloc` is the full 12-row span; pruned drops the six - // module rows to 6, which equals the retained `ploc 5` (prod's - // four lines + the surviving attribute line) plus the single - // real `blank` line. Pre-fix, pruned `sloc` stayed 12 and - // `blank` reported 7 — six phantom blanks from the elided module. + // the `#[cfg(test)]` attribute row 5, and the `mod tests { … }` + // it marks rows 6..=11 — both pruned since #1431. + // Baseline `sloc` is the full 12-row span; pruned drops those + // seven rows to 5, which equals the retained `ploc 4` (prod's + // four lines) plus the single real `blank` line. Pre-#722, + // pruned `sloc` stayed 12 and `blank` reported 7 — six phantom + // blanks from the elided module. #[test] fn sloc_drops_with_pruned_cfg_test_mod() { let source = "\ @@ -932,8 +932,8 @@ mod tests { assert_eq!(baseline.metrics.loc.ploc(), 11); // The headline fix: `sloc` falls in step with `ploc`. - assert_eq!(pruned.metrics.loc.sloc(), 6); - assert_eq!(pruned.metrics.loc.ploc(), 5); + assert_eq!(pruned.metrics.loc.sloc(), 5); + assert_eq!(pruned.metrics.loc.ploc(), 4); // Internal consistency restored: one real blank line, not the // pre-fix seven. assert_eq!(pruned.metrics.loc.blank(), 1); @@ -943,9 +943,9 @@ mod tests { // blocks. Their spans are disjoint, so the excluded line counts // simply add (no interval merge). Rows (0-based): prod row 0, // attr row 1, `mod a` rows 2..=5, attr row 6, `mod b` rows - // 7..=10 — a 11-row span. Pruning removes both four-row modules, - // leaving rows 0/1/6 (prod + the two surviving sibling - // attributes) → `sloc 3`, matching `ploc 3` with zero blanks. + // 7..=10 — a 11-row span. Pruning removes both four-row modules + // and the attribute row marking each (#1431), leaving row 0 → + // `sloc 1`, matching `ploc 1` with zero blanks. #[test] fn sloc_drops_for_adjacent_test_modules() { let source = "\ @@ -965,8 +965,8 @@ mod b { let pruned = analyse(source, true); assert_eq!(baseline.metrics.loc.sloc(), 11); - assert_eq!(pruned.metrics.loc.sloc(), 3); - assert_eq!(pruned.metrics.loc.ploc(), 3); + assert_eq!(pruned.metrics.loc.sloc(), 1); + assert_eq!(pruned.metrics.loc.ploc(), 1); assert_eq!(pruned.metrics.loc.blank(), 0); } @@ -975,7 +975,8 @@ mod b { // never descends, so `inner`'s span is folded into `outer`'s and // never double-counted. Rows (0-based): prod row 0, attr row 1, // `mod outer` rows 2..=7 (an 8-row span). Pruning removes the - // six outer rows, leaving rows 0/1 → `sloc 2`, matching `ploc 2`. + // six outer rows plus the attribute row that marks them (#1431), + // leaving row 0 → `sloc 1`, matching `ploc 1`. #[test] fn sloc_drops_for_nested_test_modules() { let source = "\ @@ -992,8 +993,8 @@ mod outer { let pruned = analyse(source, true); assert_eq!(baseline.metrics.loc.sloc(), 8); - assert_eq!(pruned.metrics.loc.sloc(), 2); - assert_eq!(pruned.metrics.loc.ploc(), 2); + assert_eq!(pruned.metrics.loc.sloc(), 1); + assert_eq!(pruned.metrics.loc.ploc(), 1); assert_eq!(pruned.metrics.loc.blank(), 0); } @@ -1004,11 +1005,10 @@ mod outer { // `Sloc::merge` folds the pruned line count upward so the unit's // span-based `sloc` drops in step — mirroring how `Ploc` unions its // line-set upward (issue #741, a #722 follow-up). Rows (0-based): - // `impl Foo {` row 0, `fn prod` row 1, the `#[test]` attribute (a - // sibling, retained) row 2, the pruned single-line `fn t() {}` row - // 3, `}` row 4 — a five-row impl span inside a six-row unit span. - // Pruning removes the one test-fn row, so both the impl-level and - // the unit-level `sloc` drop by exactly one. + // `impl Foo {` row 0, `fn prod` row 1, the `#[test]` attribute row + // 2, the single-line `fn t() {}` it marks row 3, `}` row 4 — a + // five-row impl span. Pruning removes rows 2 and 3 (#1431), so both + // the impl-level and the unit-level `sloc` drop by exactly two. #[test] fn sloc_drops_for_test_fn_nested_in_impl() { let source = "\ @@ -1032,23 +1032,24 @@ impl Foo { let baseline_impl = &baseline.spaces[0]; let pruned_impl = &pruned.spaces[0]; assert_eq!(baseline_impl.metrics.loc.sloc(), 5); - assert_eq!(pruned_impl.metrics.loc.sloc(), 4); + assert_eq!(pruned_impl.metrics.loc.sloc(), 3); // Unit-root propagation (the #741 fix): the pruned rows fold // upward through `Sloc::merge`, so the unit's `sloc` drops by - // the same one row. Before the fix this stayed at the baseline + // the same two rows. Before the fix this stayed at the baseline // value because only the impl's `excluded_rows` grew. assert_eq!(baseline.metrics.loc.sloc(), 5); - assert_eq!(pruned.metrics.loc.sloc(), 4); + assert_eq!(pruned.metrics.loc.sloc(), 3); } // A `#[test] fn` directly inside a production `impl` (no separate // `#[cfg(test)] mod`): the unit's span-based `sloc` must still drop // by the pruned test-fn rows. Rows (0-based): `impl Calc {` row 0, // `fn add` rows 1..=3 (a retained production method), `#[test]` row - // 4, `fn t` rows 5..=7 (pruned), `}` row 8 — a nine-row unit span. - // Pruning removes the three test-fn rows (5..=7), so the unit `sloc` - // drops from 9 to 6, matching `ploc`. + // 4, `fn t` rows 5..=7, `}` row 8 — a nine-row unit span. + // Pruning removes the three test-fn rows (5..=7) and the attribute + // row 4 that marks them (#1431), so the unit `sloc` drops from 9 to + // 5, matching `ploc`. #[test] fn sloc_drops_for_test_fn_in_production_impl() { let source = "\ @@ -1066,7 +1067,7 @@ impl Calc { let pruned = analyse(source, true); assert_eq!(baseline.metrics.loc.sloc(), 9); - assert_eq!(pruned.metrics.loc.sloc(), 6); + assert_eq!(pruned.metrics.loc.sloc(), 5); assert_eq!(pruned.metrics.loc.ploc(), pruned.metrics.loc.sloc()); } @@ -1075,9 +1076,9 @@ impl Calc { // prune hook records the span on the closure, not the unit. The fix // must still propagate the count up to the unit. Rows (0-based): // `fn make() {` row 0, `let f = || {` row 1, `#[test]` row 2, - // `fn t() {}` row 3 (pruned), `};` row 4, `}` row 5 — a six-row unit - // span. Pruning removes the one test-fn row, so the unit `sloc` - // drops from 6 to 5. + // `fn t() {}` row 3, `};` row 4, `}` row 5 — a six-row unit + // span. Pruning removes the test fn and its attribute (#1431), so + // the unit `sloc` drops from 6 to 4. #[test] fn sloc_drops_for_test_fn_nested_in_closure() { let source = "\ @@ -1092,7 +1093,7 @@ fn make() { let pruned = analyse(source, true); assert_eq!(baseline.metrics.loc.sloc(), 6); - assert_eq!(pruned.metrics.loc.sloc(), 5); + assert_eq!(pruned.metrics.loc.sloc(), 4); } // A non-test `impl` with no test items must be unaffected by the @@ -1151,6 +1152,124 @@ impl Foo { } assert_eq!(baseline.metrics.loc.sloc(), pruned.metrics.loc.sloc()); } + + // #1431. An outer attribute is an `AttributeItem` *sibling* of the + // item it marks, so pruning the item never reached it and Rust's + // `Loc` catch-all credited its start row to PLOC. A file that is + // nothing but test code read `sloc 1, ploc 1` — one row of code in a + // file with no production code — and every `#[cfg(test)] mod tests` + // at the foot of a production file cost that file one phantom row. + // + // `mi` is asserted to 0.0 on all three formulas because `sloc 0` is + // where `Mi::inputs_are_empty` takes over, and the issue raised that + // as a hazard: it is not one — a file with nothing in it scores + // zero, deliberately and for every formula. + #[test] + fn an_all_test_file_measures_zero_rows() { + for source in [ + // The issue's reproducer: attribute, item, and a nested + // `#[test] fn` for good measure. + "#[cfg(test)]\nmod t {\n #[test] fn x() {}\n}\n", + // A stacked run — *every* row of it goes, not just the one + // adjacent to the item. `#[allow(dead_code)]` says nothing + // about tests on its own, so a rule that read each attribute + // rather than the item it marks would leave row 1 behind. + "#[cfg(test)]\n#[allow(dead_code)]\nmod t {\n fn x() {}\n}\n", + // Attribute and item on one row. + "#[test] fn x() {}\n", + // Seven children at depth 1, past the six-child budget, so + // the lookahead resolves siblings from the node instead of + // reading the parent's child list. The two arms answer the + // same thing by construction and nothing else here reaches + // the second one (#1100's dispatch, #1431's use of it). The + // leading run is stacked so the arm has to walk *over* an + // attribute to find the item, not just take the next node. + "#[cfg(test)]\n#[allow(dead_code)]\nfn a() {}\n\ + #[test]\nfn b() {}\n#[test]\nfn c() {}\n", + ] { + let pruned = analyse(source, true); + let loc = &pruned.metrics.loc; + assert_eq!( + (loc.sloc(), loc.ploc(), loc.cloc(), loc.blank()), + (0, 0, 0, 0), + "{source:?}" + ); + assert!( + analyse(source, false).metrics.loc.ploc() > 0, + "{source:?}: the unpruned reading must be non-zero, or the \ + fixture proves nothing" + ); + + let mi = &pruned.metrics.mi; + assert_eq!( + (mi.original(), mi.sei(), mi.visual_studio()), + (0.0, 0.0, 0.0), + "{source:?}: `sloc 0` must reach the empty-input guard" + ); + } + } + + // The other half of #1431: an attribute run is pruned for the item + // it marks, never for itself. Each row here is an attribute the + // prune must leave alone, and `ploc` is asserted equal to the + // unpruned reading so a rule that pruned every `#[…]` — or every + // one whose text mentions `test` — fails on it. + #[test] + fn a_non_test_attribute_run_survives_pruning() { + for source in [ + // Nothing test-related at all. + "#[inline]\nfn a() {}\n", + // A stacked non-test run on a prunable item kind. + "#[inline]\n#[allow(dead_code)]\nfn a() {}\n", + // A test attribute on an item kind the prune does not + // remove: `use_declaration` is not in `rust_prunable_item`, + // so pruning its attribute would leave the file reporting + // fewer rows than it has retained code on. + "#[cfg(test)] use foo;\n", + "#[cfg(test)]\nstruct S;\n", + // `#[cfg(not(test))]` marks production code, and the + // attribute lookahead must inherit that reading from + // `rust_item_is_test_only` rather than re-deriving it. + "#[cfg(not(test))]\nfn a() {}\n", + // The same, past the six-child budget: the sibling-resolving + // arm of the lookahead must be as conservative as the + // child-list one. + "#[inline]\nfn a() {}\n#[inline]\nfn b() {}\n\ + #[inline]\nfn c() {}\n#[inline]\nfn d() {}\n", + ] { + let baseline = analyse(source, false); + let pruned = analyse(source, true); + assert_eq!( + ( + pruned.metrics.loc.sloc(), + pruned.metrics.loc.ploc(), + pruned.metrics.loc.cloc() + ), + ( + baseline.metrics.loc.sloc(), + baseline.metrics.loc.ploc(), + baseline.metrics.loc.cloc() + ), + "{source:?}: nothing here is the prune's to take" + ); + assert!(pruned.metrics.loc.ploc() > 0, "{source:?}"); + } + } + + // An *inner* attribute (`mod tests { #![cfg(test)] … }`) already + // sits inside the subtree the item's prune removes, so there is no + // outer run to reach and #1431 changes nothing about it. Pinned + // because the attribute arm now fires on `AttributeItem` generally, + // and an arm that also matched `InnerAttributeItem` would be + // pruning a node its enclosing item already covers. + #[test] + fn an_inner_cfg_test_attribute_still_elides_its_module() { + let source = "fn prod() {}\nmod tests {\n #![cfg(test)]\n fn a() {}\n}\n"; + let pruned = analyse(source, true); + assert_eq!(analyse(source, false).metrics.loc.sloc(), 5); + assert_eq!(pruned.metrics.loc.sloc(), 1); + assert_eq!(pruned.metrics.loc.ploc(), 1); + } } // Non-Rust languages must ignore `exclude_tests = true` because From 9a36d39d506b5eaa121a975b81216aec2f63b33d Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 07:52:26 -0700 Subject: [PATCH 09/12] fix(metrics/loc): bound a heredoc to its own rows heredoc_redirect spans the command-line prefix as well as the literal, and the grammar lets that prefix cross rows -- a pipeline is one of its children. Crediting the wrapper's whole interior, as the sibling arm does for a literal that is its own span, therefore billed every prefix row as code: a blank row read as code, and a comment-only row was reclassified from comment to code-and-comment. Start the range at the first body row instead, derived from the last row any non-body child occupies. For the single-row prefix every runnable heredoc has, that is start + 1 -- the same arithmetic as before -- so no valid input moves. The body node's own start row cannot serve here: per #1412 its span begins at the first body row carrying text and collapses to zero width for an empty body, so it cannot say where the body begins. The multi-row form is a bash syntax error. bash begins the body on the line after the one carrying <<, so `cat < { + String | RawString | AnsiCString | HeredocBody | HeredocBody2 => { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); add_string_interior_ploc(node, stats, start); } + // The wrapper, whose interior starts where the *body* does and + // not one row below the `<<` (#1443). + // + // `heredoc_redirect` spans the command-line prefix as well as + // the literal, and the grammar lets that prefix cross rows: a + // `pipeline` is one of its children. Crediting from + // `start + 1`, as the arm above does for a literal that *is* + // its own span, then bills every prefix row as code — + // including a blank or comment-only one. + // + // The first body row is one past the last row any non-body + // child occupies, which collapses to `start + 1` for the + // ordinary single-row prefix, so this is the same arithmetic + // everywhere except the shape it exists for. + // + // **Upstream divergence, deliberately not worked around.** + // Every input reaching the multi-row form is a bash syntax + // error: bash begins the body on the line after the one + // carrying `<<`, so `cat < { + check_comment_ends_on_code_line(stats, start); + stats.ploc.lines.insert(start); + stats.ploc.lines.insert_range( + heredoc_body_first_row(node, start), + node.end_line().saturating_sub(1), + ); + } // An assignment standing as a statement of its own is one // logical line — but only then. // @@ -159,3 +194,47 @@ impl Loc for BashCode { } } } + +/// The first row of `redirect`'s heredoc body: one past the last row any +/// of its non-body children occupies. +/// +/// `heredoc_redirect` covers the command-line prefix (`<<`, the marker, +/// and whatever the grammar hangs off the rest of the line) as well as +/// the literal, so the literal's own rows start below all of them. The +/// prefix is single-row in every runnable spelling, where this returns +/// `start + 1` and the caller behaves exactly as the sibling arm does. +/// +/// Reading the *body* node's start row instead would be wrong for the +/// shape #1412 is about: `heredoc_body`'s span begins at the first body +/// row that has text and collapses to zero width when the body is empty +/// throughout, so it cannot say where the body *begins*. The prefix can, +/// because it is bounded by the row the marker sits on. +/// +/// The floor keeps the range clear of the opening row the caller has +/// already credited, and it is **inert today**: every `heredoc_redirect` +/// the grammar emits carries the `<<` token, which starts on `start` at +/// a column above 0 and so ends at `start + 1` or later. Deleting the +/// floor fails no test — measured, 0 of 3,397 — which is why the +/// `debug_assert!` is here rather than a comment claiming the shape +/// cannot arise. It checks the premise on every heredoc of every walk, +/// so a grammar that ever emits a body-only wrapper reports that +/// directly instead of silently crediting rows above the literal. +fn heredoc_body_first_row(redirect: &Node, start: usize) -> usize { + let after_prefix = redirect + .children() + .filter(|child| { + !matches!( + child.kind_id().into(), + Bash::HeredocBody | Bash::HeredocBody2 | Bash::HeredocContent | Bash::HeredocEnd + ) + }) + .map(|child| child.end_line()) + .max(); + + debug_assert!( + after_prefix.is_some_and(|row| row > start), + "a heredoc_redirect at row {start} has no non-body child below its opening row" + ); + + after_prefix.unwrap_or(0).max(start.saturating_add(1)) +} From fcf8c741bf3fb627d55293ca34ba04cf67f3826a Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 08:26:24 -0700 Subject: [PATCH 10/12] docs(metrics/loc): record why C# has no PreprocArg arm C# has a preproc_arg and its four C-family siblings all route theirs through add_string_interior_ploc, so the absence here reads as an oversight. It is a decision, and #1423 left it as a FIXME rather than settling it. Settle it as a gap, documented at the catch-all. The C# specification terminates a pp-directive at the new-line and defines no line continuation, so `#region Big \` ends there and the row below is a syntax error rather than an argument row. tree-sitter-c-sharp is over-permissive relative to the language, accepting a backslash continuation the way the C grammar legitimately does. A preproc_arg in runnable C# is therefore always single-row, and on a single row the arm would no-op -- so adding it could only change the number reported for source that does not compile, where a line count has no correct answer. The note records the measured reproducer, the parse it produces, why this is the opposite call to #1443 (there the range's *meaning* was wrong and correcting it was free on valid input; here the meaning is already right and only invalid input separates the two behaviours), and what would reopen the question. No test: pinning the reported numbers for invalid C# would encode the gap as a contract and invert into a bug-lock the moment the grammar tightens. The decision rests on a specification fact, not on behaviour. Comment-only; metric output is unchanged. Fixes #1430 --- src/metrics/loc/csharp.rs | 54 ++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/src/metrics/loc/csharp.rs b/src/metrics/loc/csharp.rs index c82cfa47..0abe42c7 100644 --- a/src/metrics/loc/csharp.rs +++ b/src/metrics/loc/csharp.rs @@ -54,15 +54,51 @@ impl Loc for CsharpCode { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); - // FIXME(#1430): C# has a `PreprocArg` (185) and no arm for - // it, unlike its four C-family siblings, so a multi-row - // `preproc_arg` credits only its first row and the rest - // fall through to `blank`. `tree-sitter-c-sharp` accepts a - // backslash continuation in a directive (`#region Big \`), - // but the C# specification terminates a directive at the - // newline, so the shape is grammar-reachable and - // language-invalid. Deliberately left alone by #1423 rather - // than guessed at. + // **No `PreprocArg` arm here, unlike the four C-family + // siblings — a decided gap, not an oversight (#1430).** + // + // C# has a `preproc_arg` (`Csharp::PreprocArg`, 185, no + // numeric-suffix aliases) and `c.rs` / `cpp.rs` / + // `mozcpp.rs` / `objc.rs` all route theirs through + // `add_string_interior_ploc` so a multi-row macro body + // reaches PLOC. Without the arm a multi-row `preproc_arg` + // credits only its start row and the rest fall to + // `blank = sloc - ploc - cloc`. Measured: + // + // #region Big \ + // section + // #endregion + // class C {} + // + // parses `{preproc_arg:185} from (1, 9) to (2, 10)` and + // reports `sloc 4, ploc 3, blank 1` — row 1 is text scored + // as blank. + // + // The arm is absent because **no valid C# can reach the + // shape**. The C# specification terminates a `pp-directive` + // at the new-line and defines no line continuation, so + // `#region Big \` ends at the newline and ` section` below + // it is a syntax error, not an argument row. + // `tree-sitter-c-sharp` is over-permissive relative to the + // language here, accepting a backslash continuation the way + // the C grammar legitimately does. A `preproc_arg` in + // runnable C# is always single-row, and on a single row the + // arm would no-op — so adding it could only change the + // number reported for source that does not compile, where a + // line count has no correct answer to be right about. + // + // This is the opposite call to #1443, which fixed an + // equally unreachable Bash shape. The difference is what + // the arm would buy: there, the range was wrong in a way + // that also made the *meaning* wrong ("the literal's rows" + // included rows outside the literal), and correcting it was + // free on valid input. Here the meaning is already right + // and only invalid input can tell the two behaviours apart. + // + // If `tree-sitter-c-sharp` ever tightens to match the + // specification, this note and #1430 both become moot. If + // C# ever *gains* a continuation, add the arm — it is two + // lines, and its four siblings are the template. } } } From 9c3712da61a979e65bf4b1610ffbad045349bf8d Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 09:09:32 -0700 Subject: [PATCH 11/12] docs: correct the reachability claim behind #1443 #1443's rationale said every multi-row heredoc prefix is a bash syntax error, "verified with bash -n on five spellings". False. The five spellings all happened to carry a blank or comment row; a continuation that carries content is valid Bash and produces a genuinely multi-row heredoc_redirect: cat <` on the file, or a `git diff --stat` that shows what you expect. +**For a compiled subject the rebuild must be inside the measurement +step.** `rg` on the source proves the *source* changed; a sweep that +shells out to `target/release/bca` is measuring the last binary someone +built. During #1412 a corpus sweep reported "0 of 438 files changed" — +a perfectly plausible result — because the restore ran without a +rebuild, so the before and after runs used the same stale binary. Put +`cargo build` in the same step as the run, and assert a known fixture +whose answer differs between the two states immediately after each +build; that guard is what caught it on the rewrite. + **The result *parser* is the other half of the subject.** During #1238 a sweep drove three perturbations of one match arm and reported zero Rust failures for all three, while the Python leg of the same sweep reported @@ -212,6 +222,29 @@ Where the construct contributes to no axis once excluded — a Lua anchor on, and the revert test is the only coverage available. Say so in a comment, so the missing anchor is not read as an oversight. +### Never let the measured value *be* the defect's output + +A fixture whose discriminating quantity is *produced by the bug* stops +measuring anything the moment the bug is fixed. It is the inverse of the +decay above, it bites benchmark probes hardest, and it fails as "the +workload scored zero on its own shape" — which reads like a broken +fixture rather than like the fix working. + +`loc/wide-cfg-test-mod` (`big-code-analysis-bench/src/shapes.rs`) read +`sloc` under `exclude_tests` on a file of nothing but `#[cfg(test)] +mod m {}` repeated. Its only non-zero row was the phantom attribute row +#1431 then removed, so the probe scored zero and tripped +`probe_workload_is_exercised`. Left unnoticed it would have timed the +walk's fixed overhead and reported an excellent exponent forever. The +repair was to render a retained `fn p() {}` per item, so the reading +survives the fix. + +When a change alters what a shape measures, re-check every probe or +fixture reading *that* metric on *that* shape before comparing a +before/after result. Build the measured quantity out of something the +fix does not touch — for a metric fix, usually retained content +alongside the construct under test. + ## Coverage measures execution, not discrimination A coverage report answers "did any test run this line?" It never answers diff --git a/CHANGELOG.md b/CHANGELOG.md index b2b0aa2c..370c5134 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,14 +142,17 @@ for historical reference. interior read a blank prefix row as code, and reclassified a comment-only one as code-and-comment. The credited range now starts at the first body row, derived from the last row any non-body child - occupies; that collapses to the previous arithmetic for the single-row - prefix every runnable heredoc has, so no valid input changes. The - multi-row form is in fact a bash *syntax error* — bash begins the body - on the line after the `<<`, so the terminator is never found, and - `bash -n` rejects all five spellings tried — which makes this a - `tree-sitter-bash` divergence rather than a miscount on runnable - input. Fixed rather than worked around because `bca` is still asked to - measure malformed trees, the argument #1398 rests on. + occupies. A *blank or comment-only* prefix row — the shape this fixes + — is unreachable in runnable Bash, since bash begins the body on the + line after the `<<` and a `\` continuation splices the rows rather + than leaving one empty; `bash -n` rejects all four spellings. A prefix + that crosses rows with content is valid and unaffected, its + continuation rows carrying leaves the catch-all credits. Fixed rather + than left as a `tree-sitter-bash` divergence because `bca` is still + asked to measure malformed trees, the argument #1398 rests on. One + valid shape does move: a continuation row holding only `\` has no leaf + to credit it and now reads blank, a general Bash gap this arm's old + blanket range happened to mask in one position (#1445). - **`--exclude-tests` now prunes the `#[cfg(test)]` / `#[test]` attribute along with the item it marks** (#1431). An outer attribute is an `AttributeItem` *sibling* of its item, not a child, so pruning diff --git a/docs/development/lessons_learned.md b/docs/development/lessons_learned.md index c5d8ab77..4230fd5a 100644 --- a/docs/development/lessons_learned.md +++ b/docs/development/lessons_learned.md @@ -137,6 +137,7 @@ number and the higher number stays as a redirect. | [91](#91-a-gate-can-filter-out-its-own-subject-before-the-check-runs) | A gate can filter out its own subject before the check runs | | [92](#92-an-optimizations-rationale-can-encode-the-waste-it-optimizes-for) | An optimization's rationale can encode the waste it optimizes for | | [93](#93-a-gate-that-reads-a-typed-accessor-is-invisible-to-the-front-ends-that-read-the-wire) | A gate that reads a typed accessor is invisible to the front-ends that read the wire | +| [94](#94-a-sweep-that-finds-nothing-only-rules-out-what-its-predicate-can-express) | A sweep that finds nothing only rules out what its predicate can express | --- @@ -3185,7 +3186,9 @@ when written and rot silently. When a claim is expensive to verify or cannot be pinned, write what was measured and under which conditions rather than the generalisation it suggests. Be most suspicious of all of a comment saying a fix is *already in place*: it ends the search that -would have found the gap. +would have found the gap — as does a comment that **declines** to make a +claim ("probably X, but nothing enforces it"), which reads as candour and +so is never measured. No gate checks any of it. `cargo test` does not read prose, clippy does not evaluate it, and a reviewer's eye slides over a plausible sentence — @@ -3236,6 +3239,14 @@ confirmation and moved on. The fix pinned the half that *was* true (`c_family_char_literal_is_not_a_string`, two-sided per language) so it fails loudly if it stops being true, and made the other half true. +**A hedge is the same failure worn as honesty** (#1417). One of the three +sets `Stats::settle_excluded_rows` subtracts was documented as "very +probably a subset of `ploc.lines` … but nothing enforces that". Deleting +that subtraction failed **0 of 3,390** lib tests, where dropping either +sibling fails 3 and 1. It survived two fresh-context reviews *because* it +admitted the uncertainty: a hedge reads as a gap someone has already +logged, not as a claim to test. The remedy was a `debug_assert_eq!` on the walk, not a fixture. + --- ## 85. Coverage measures execution, not discrimination @@ -3713,3 +3724,43 @@ field now disagreed — and stopped there; the binding that reads that field is the consumer the note needed to name. --- + +## 94. A sweep that finds nothing only rules out what its predicate can express + +**Lesson:** When a measurement reports no violations, state the +*predicate* it evaluated, not the conclusion it suggests. "No file +breaches `ploc <= sloc`" is not "no file is miscounted", and the gap +between them is invisible because the sweep's own output looks +exhaustive — a big denominator reads as thoroughness regardless of what +was asked. Before trusting a negative result to size a fix, write down +the defect's expected signature and check the predicate can represent +it: a defect that moves two quantities by the same amount in opposite +directions cannot violate an inequality between them, and one confined +to a language the corpus does not cover cannot appear at any denominator +at all. Where the predicate cannot express the signature, say what was +measured and what it therefore does not cover. + +A negative result is load-bearing in a way a positive one is not. It +sets the test bar, the snapshot expectation, the changelog wording and +whether the change is scheduled at all, and each of those decisions +inherits the unstated scope. Nothing rechecks it later: the sweep is +usually run once, during triage, and the number is quoted from the issue +body from then on. + +**234,791 spaces, and the wrong question** (#1423). The C-family +`PreprocArg` arm credited a macro body's rows by the raw end row rather +than by `Node::end_line`, over-crediting one row whenever the body ends +at column 0. The issue was filed "not currently observable", on a sweep +finding 0 of 234,791 spaces across 14,450 files violating `ploc <= sloc` +or `cloc <= sloc`. Both inequalities are blind to this defect by +construction: it moves one row from `blank` into `ploc`, so their sum is +unchanged and neither bound can be violated. The first run of the fix moved +a DeepSpeech snapshot — `left_test.cc`, `ploc 344 → 343` — which the +"latent, not live" framing had said to expect none of. + +The second way a sweep's scope goes unstated is a corpus that cannot +contain the subject: the sibling fix predicted snapshot churn "for any +Bash corpus file with a heredoc", and not one of the submodule's 1,610 +snapshots is shell-derived (#1412, and lesson 74 for the mechanism). + +--- diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index 062279a8..fe69d705 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -7418,12 +7418,16 @@ EOF /// A single fixture asserting `ploc` alone would keep passing if the /// prefix row were trimmed out of it. /// - /// Note both inputs are bash *syntax errors* — bash starts the body - /// on the line after the `<<`, so it never finds the terminator, and - /// `bash -n` rejects every spelling of this shape. They are here - /// because `bca` measures malformed trees too, which is the same - /// argument #1398 rests on, and because the arm's range should mean - /// "the literal's rows" rather than being incidentally right. + /// Note both inputs are bash *syntax errors*: bash starts the body + /// on the line after the `<<`, so a blank or comment-only prefix row + /// is unreachable in runnable Bash — `bash -n` rejects all four + /// spellings, bare and `\`-continued. A prefix that crosses rows + /// *with content* is valid (`cat < { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); @@ -214,7 +229,7 @@ impl Loc for BashCode { /// already credited, and it is **inert today**: every `heredoc_redirect` /// the grammar emits carries the `<<` token, which starts on `start` at /// a column above 0 and so ends at `start + 1` or later. Deleting the -/// floor fails no test — measured, 0 of 3,397 — which is why the +/// floor fails no test — measured, 0 of 3,394 — which is why the /// `debug_assert!` is here rather than a comment claiming the shape /// cannot arise. It checks the premise on every heredoc of every walk, /// so a grammar that ever emits a body-only wrapper reports that diff --git a/src/metrics/loc/csharp.rs b/src/metrics/loc/csharp.rs index 0abe42c7..16b029a3 100644 --- a/src/metrics/loc/csharp.rs +++ b/src/metrics/loc/csharp.rs @@ -87,13 +87,14 @@ impl Loc for CsharpCode { // number reported for source that does not compile, where a // line count has no correct answer to be right about. // - // This is the opposite call to #1443, which fixed an - // equally unreachable Bash shape. The difference is what - // the arm would buy: there, the range was wrong in a way - // that also made the *meaning* wrong ("the literal's rows" - // included rows outside the literal), and correcting it was - // free on valid input. Here the meaning is already right - // and only invalid input can tell the two behaviours apart. + // This is the opposite call to #1443, which fixed a Bash + // shape that was also unreachable — a blank prefix row in a + // heredoc. The difference is what the arm would buy: there, + // the range was wrong in a way that made the *meaning* wrong + // ("the literal's rows" included command-prefix rows), and + // the valid multi-row prefixes were unaffected. Here the + // meaning is already right, and only source that does not + // compile can tell the two behaviours apart. // // If `tree-sitter-c-sharp` ever tightens to match the // specification, this note and #1430 both become moot. If From d4479cdf87a0ce2dc63d0fce1e7ca171fd17e0b8 Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 13 Sep 2026 12:27:57 -0700 Subject: [PATCH 12/12] fix: address the PR #1446 review findings Five findings, one HIGH and four MEDIUM. **The attribute lookahead was quadratic** (checker.rs). #1431 made should_skip_subtree answer for every AttributeItem, and each ask re-read the whole run -- once in rust_attributed_item's skip_while from child 0, once again inside rust_item_is_test_only. D nested fns with 3*D attributes on one item took 0.08 / 0.34 / 1.21 / 5.18 s for D = 250/500/1000/2000, against 0.05 s with --exclude-tests off. Both readings were the same waste: every attribute in a run decorates the same item, so all L answers are equal. A verdict now reports how far it reaches -- should_skip_subtree returns SubtreeSkip rather than bool, Rust's attribute arm sets the reach to the decorated item's start byte, and the walk reuses the last reaching verdict. Pre-order visits start_byte non-decreasing, so the test is O(1) and a passed reach cannot be re-entered. One ask per run. The same input now runs in 0.00 / 0.01 / 0.02 / 0.07 s. forward_attribute_scan_budget is unchanged: on the diagonal the forward pass is still the cheaper of the two readings, and the budget was never what made this quadratic. rust_item_is_test_only is still reused verbatim, so no third reading of the run was added (grammar-dispatch section 7). The new nom/deep-attribute-run probe grows depth and one run together -- the axis the depth-only and width-only probes cannot see. It reads nom.total() of the *retained* wrappers, not anything the prune produces, so a later change to what is pruned cannot zero it. Watched failing: 2.01 and over bound with the reach disabled, 1.06 with it. **A valid Bash continuation row became blank** (bash.rs). #1443 narrowed the heredoc wrapper's range to the literal's own rows. What that fixed needs bash -n to reject the file; what it broke runs: a continuation row holding only `\` carries no leaf for the leaf-gated catch-all, so the blanket range was the only thing covering it, and `cat < bool { .is_match(code) } +/// A [`Checker::should_skip_subtree`] verdict, plus the following +/// nodes it also answers for. +/// +/// Most verdicts are about one node and carry no reach. The exception +/// is a verdict a classifier can only reach by reading a *run* of +/// siblings: Rust's `#[…]` rows all decorate the same item, so they all +/// get the same answer, and re-deriving it per row is `O(run)` work +/// done `run` times. A run of `3 * depth` attributes under a parent +/// wide enough to stay inside the forward attribute-scan budget made +/// that quadratic — 5.2 s at depth 2 000, against 0.05 s with +/// `exclude_tests` off (#1446). Reporting the reach lets the walker ask +/// the classifier once per run. +/// +/// # Soundness +/// +/// The reach is a byte offset, and the walk visits nodes in +/// non-decreasing `start_byte` order, so "starts before it" is a state +/// the walker can test in `O(1)` and can never re-enter. A claim is +/// sound when every node the walk *visits* inside the reach genuinely +/// has the reported verdict. +/// +/// "Visits" is the load-bearing word, and it cuts the obligation in +/// two. A `SKIP` reach need only hold for the nodes it skips, since a +/// skipped node's descendants are never pushed. A `RETAIN` reach +/// carries the heavier claim: the members' descendants *are* walked, so +/// it must be right for them too. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct SubtreeSkip { + skipped: bool, + covers_through_byte: Option, +} + +impl SubtreeSkip { + /// Keep this node. Answers for nothing else. + pub const RETAIN: Self = Self { + skipped: false, + covers_through_byte: None, + }; + + /// Elide this node and its descendants. Answers for nothing else. + pub const SKIP: Self = Self { + skipped: true, + covers_through_byte: None, + }; + + /// [`Self::SKIP`] or [`Self::RETAIN`], from a predicate's answer. + #[inline] + #[must_use] + pub const fn of(skipped: bool) -> Self { + if skipped { Self::SKIP } else { Self::RETAIN } + } + + /// Extend this verdict to every node that starts before `byte`. + /// + /// The caller is asserting that it read far enough to answer for + /// all of them — see the soundness note on the type. + #[inline] + #[must_use] + pub const fn covering_through(self, byte: usize) -> Self { + Self { + covers_through_byte: Some(byte), + ..self + } + } + + /// Whether this node and its descendants are elided. + #[inline] + #[must_use] + pub const fn is_skipped(self) -> bool { + self.skipped + } + + /// Whether this verdict already answers for a node starting at + /// `start_byte`, so the classifier need not be asked again. + #[inline] + #[must_use] + pub const fn answers_for(self, start_byte: usize) -> bool { + match self.covers_through_byte { + Some(through) => start_byte < through, + None => false, + } + } +} + /// Per-language AST classification predicates the metric walkers use to /// recognize comments, function spaces, calls, strings, branches, and so /// on by node kind. @@ -493,27 +577,32 @@ pub trait Checker { node.has_error() } - /// Return `true` to elide this node and all its descendants from - /// every metric. Used by language modules to filter - /// test-only / generated / preprocessor-disabled subtrees. + /// Answer [`SubtreeSkip::SKIP`] to elide this node and all its + /// descendants from every metric. Used by language modules to + /// filter test-only / generated / preprocessor-disabled subtrees. /// - /// The default returns `false` for every node, preserving the - /// pre-#182 behavior. Language overrides drive opt-in skips - /// (currently: `RustCode` filters `#[cfg(test)]` items, gated - /// by the runtime `MetricsOptions::exclude_tests` flag). + /// The default retains every node, preserving the pre-#182 + /// behavior. Language overrides drive opt-in skips (currently: + /// `RustCode` filters `#[cfg(test)]` items, gated by the runtime + /// `MetricsOptions::exclude_tests` flag). /// /// `ancestors` is the chain the caller descended through. Rust's /// override needs the parent to read the run of `#[…]` siblings /// before an item, and resolving siblings from the node alone /// costs `O(depth)` per step (#1100). + /// + /// A verdict that took a *sibling-run* to reach should say so with + /// [`SubtreeSkip::covering_through`], so the walker asks once per + /// run rather than once per member. See that method for what makes + /// such a claim sound. #[inline] #[must_use] fn should_skip_subtree<'a>( _node: &Node<'a>, _code: &[u8], _ancestors: Ancestors<'a, '_>, - ) -> bool { - false + ) -> SubtreeSkip { + SubtreeSkip::RETAIN } /// Source-aware variant of [`is_func_space`](Self::is_func_space). The default forwards @@ -756,16 +845,45 @@ fn rust_prunable_item(node: &Node) -> bool { /// the second attribute says nothing about tests, and /// [`rust_item_is_test_only`] reads the run from the item end either /// way (#1431). -fn rust_attribute_run_marks_test_item<'a>( +/// +/// That is also why the answer is reported as covering the rest of the +/// run. Every attribute from `node` up to the item resolves the same +/// item and therefore the same verdict, so a walker that re-asks per +/// row pays the run twice over per row: once in +/// [`rust_attributed_item`]'s lookahead and once in the backward +/// reading [`rust_item_is_test_only`] takes. Both are `O(run)` under a +/// parent inside [`forward_attribute_scan_budget`], and a run of +/// `3 * depth` attributes is inside it by construction — the diagonal +/// #1446 measured at 5.2 s / depth 2 000. One ask per run makes it one +/// pass, not `run` passes. +fn rust_attribute_run_verdict<'a>( node: &Node<'a>, code: &[u8], ancestors: Ancestors<'a, '_>, -) -> bool { - rust_attributed_item(node, ancestors).is_some_and(|item| { - // `item` is `node`'s sibling, so the chain that describes one - // describes the other — parent and depth alike. - rust_prunable_item(&item) && rust_item_is_test_only(&item, code, ancestors) - }) +) -> SubtreeSkip { + let Some(item) = rust_attributed_item(node, ancestors) else { + // Attributes all the way to the end of the parent: nothing is + // decorated, so nothing is pruned — and no later member of this + // run will find an item either, so say so once. Without a + // parent to bound the claim there is nothing to say, which + // costs only a re-ask. + return match ancestors.parent(node) { + Some(parent) => SubtreeSkip::RETAIN.covering_through(parent.end_byte()), + None => SubtreeSkip::RETAIN, + }; + }; + // `item` is `node`'s sibling, so the chain that describes one + // describes the other — parent and depth alike. + let skipped = rust_prunable_item(&item) && rust_item_is_test_only(&item, code, ancestors); + // Everything strictly between `node` and `item` is an + // `AttributeItem` — that is what `rust_attributed_item` searched + // for — so the reach holds for the run's remaining members. It + // holds for *their* descendants too, which the walk visits whenever + // the run is retained: an attribute's children are the `#`, `[`, + // `]` tokens and the `attribute` body, none of them a + // `rust_prunable_item` or an `AttributeItem`, so `RETAIN` is their + // answer as well. + SubtreeSkip::of(skipped).covering_through(item.start_byte()) } /// The item an outer-attribute run decorates: the first sibling after @@ -801,7 +919,22 @@ fn rust_attribute_run_marks_test_item<'a>( /// into one pass is deliberate at that price. A fused scan would save /// one of the three sibling resolutions and would be a third reading of /// the same run, free to drift from the other two — the failure mode -/// `.claude/rules/grammar-dispatch.md` §7 is about. +/// `.claude/rules/grammar-dispatch.md` §7 is about. #1446 kept that +/// choice: what was quadratic there was asking `run` times, not the +/// number of passes each ask makes, and +/// [`rust_attribute_run_verdict`]'s reach removes the repetition +/// without adding a reading. +/// +/// # Why the same budget as the backward reading +/// +/// The dispatch here weighs the same two costs #1100 did, and #1446 — +/// which found the diagonal `width ~= 3 * depth` shape this branch is +/// slowest on — did not move it. `parent.child_count() <= 3 * depth` +/// picks the forward pass exactly when `width * 35 ns` beats +/// `depth * 120 ns`, and on that diagonal it is still the cheaper of +/// the two: the sibling walk would be `O(run)` *and* `O(depth)` per +/// step. The budget was never what made the shape quadratic — asking +/// once per attribute was. fn rust_attributed_item<'a>(node: &Node<'a>, ancestors: Ancestors<'a, '_>) -> Option> { match ancestors.parent(node) { Some(parent) if parent.child_count() <= forward_attribute_scan_budget(ancestors) => parent @@ -1517,6 +1650,14 @@ mod tests { /// answers differently from the other. The fixture spans both: /// `source_file` is wide enough at depth 1 to take the sibling /// arm, and `mod narrow`'s body is not. + /// + /// It also pins the [`SubtreeSkip`] reach, by replaying exactly what + /// the walker does with it: carry the last covering verdict, and on + /// every node it claims to answer for, check the claim against what + /// the classifier says when asked directly. An over-broad reach — + /// one byte past the item, say, or a run start taken as the run end + /// — is a *silently wrong prune* in production, because the walker + /// never asks again (#1446). #[test] fn rust_should_skip_subtree_matches_the_backward_reading() { let source = "#[cfg(test)]\nmod tests {\nfn a() {}\n}\n\ @@ -1525,7 +1666,10 @@ mod tests { #[rstest]\nfn b() {}\nfn c() {}\n\ #[cfg(test)]\nuse std::fmt;\n\ #[cfg(test)]\n#[allow(dead_code)]\nfn d() {}\n\ - mod narrow {\n#[cfg(test)]\nfn e() {}\n}\n"; + #[inline]\n#[allow(dead_code)]\n#[must_use]\n\ + fn f() -> i32 {\n#[cfg(test)]\nfn inner() {}\n1\n}\n\ + mod narrow {\n#[cfg(test)]\nfn e() {}\n}\n\ + #[inline]\n#[allow(dead_code)]\n"; let code = source.as_bytes(); // Spelled out rather than reused from the production predicate: // the point is to pin which kinds the prune considers, so @@ -1545,6 +1689,14 @@ mod tests { rust_attribute_run_before(node, code) || rust_inner_attr_marks_test(node, code) }; let mut pruned = 0_usize; + // The walker's own state, replayed: the last verdict that + // claimed to answer for the nodes after it, and the end of the + // deepest subtree pruned so far. The second is what makes the + // replay a replay — the walker never pushes a skipped node's + // children, so a reach owes them nothing and asserting over + // them would fail on correct code. + let mut reach = SubtreeSkip::RETAIN; + let (mut pruned_through, mut reused) = (0_usize, 0_usize); let visited = for_each_node_with_chain::(code, |node, chain| { let reference = if node.kind_id() == Rust::AttributeItem { // Walk to the item this run decorates with the raw @@ -1557,7 +1709,8 @@ mod tests { } else { is_item(node) && is_test_only(node) }; - let skipped = RustCode::should_skip_subtree(node, code, Ancestors::known(chain)); + let verdict = RustCode::should_skip_subtree(node, code, Ancestors::known(chain)); + let skipped = verdict.is_skipped(); assert_eq!( skipped, reference, @@ -1565,20 +1718,98 @@ mod tests { node.kind(), node.start_row(), ); + if node.start_byte() >= pruned_through { + if reach.answers_for(node.start_byte()) { + reused += 1; + assert_eq!( + reach.is_skipped(), + reference, + "a reach answered {} for {} at row {}", + reach.is_skipped(), + node.kind(), + node.start_row(), + ); + } else { + reach = verdict; + } + if skipped { + pruned_through = pruned_through.max(node.end_byte()); + } + } pruned += usize::from(skipped); }); assert!(visited > 0, "fixture must have nodes to compare"); - // Five test-only items — `mod tests`, `mod inner`, `fn b`, - // `fn d`, `fn e` — plus the five `#[…]` rows that mark four of - // them (`fn d` carries two, `mod inner` an inner attribute that - // its own subtree covers). Neither `#[allow(dead_code)]` on the - // production `static`, nor `#[cfg(test)]` on the `use`, is the - // prune's to take. A hook that pruned everything would satisfy - // the equality above only if the oracle agreed; this pins the - // count the fixture was written for. + // Both halves of the reach have to fire, or the assertion above + // is decoration: a run whose members are pruned (`fn d`'s two + // rows) and one whose members are kept (`fn f`'s three, plus + // the trailing pair that decorates nothing at all), the latter + // also carrying each attribute's `#` / `[` / `]` / `attribute` + // children through the same reach. + // + // `fn f`'s body is what makes an over-broad reach fail rather + // than agree by luck. A reach is *always* right about the item + // it stops at — the run's verdict is that item's verdict, by + // construction — so a reach extended one node too far reads as + // correct. Extended over the item's *body*, it swallows the + // nested `#[cfg(test)] fn inner`, and the two answers part + // company (verified by perturbation: `item.start_byte()` to + // `item.end_byte()` fails this assertion, and without `fn f`'s + // body it fails nothing). + assert!( + reused > 4, + "only {reused} nodes were answered by a reach; the fixture \ + must carry runs long enough for the walker to reuse one" + ); + // Seven test-only items — `mod tests`, `mod inner`, `fn b`, + // `fn d`, `fn inner`, `fn e`, and `fn a` inside `mod tests` — + // plus the six `#[…]` rows that mark five of them (`fn d` + // carries two, `mod inner` an inner attribute that its own + // subtree covers). Neither `#[allow(dead_code)]` on the + // production `static`, nor `#[cfg(test)]` on the `use`, nor the + // trailing run that decorates nothing, is the prune's to take. + // A hook that pruned everything would satisfy the equality + // above only if the oracle agreed; this pins the count the + // fixture was written for. assert_eq!( - pruned, 10, - "expected the five items and their five attributes" + pruned, 12, + "expected the test-only items and the attributes marking them" + ); + } + + /// A run that decorates nothing still answers for the rest of + /// itself. + /// + /// `#[inline]` with no item after it parses cleanly — no ERROR node, + /// just a `source_file` whose last children are attributes — so the + /// lookahead runs off the end and there is no item to take the reach + /// from. Answering per row there is *correct*, which is exactly why + /// [`rust_should_skip_subtree_matches_the_backward_reading`] cannot + /// see it: dropping this reach costs only time, and a trailing run + /// is unbounded in length like any other. + #[cfg(feature = "rust")] + #[test] + fn a_trailing_attribute_run_answers_for_its_whole_run() { + let source = "fn a() {}\n#[inline]\n#[allow(dead_code)]\n"; + let code = source.as_bytes(); + let mut attributes = Vec::new(); + for_each_node_with_chain::(code, |node, chain| { + if node.kind_id() == Rust::AttributeItem { + attributes.push(( + node.start_byte(), + RustCode::should_skip_subtree(node, code, Ancestors::known(chain)), + )); + } + }); + let [(_, first), (second_start, second)] = attributes[..] else { + panic!("fixture must hold exactly two attributes, got {attributes:?}"); + }; + assert!( + !first.is_skipped() && !second.is_skipped(), + "a run decorating nothing prunes nothing" + ); + assert!( + first.answers_for(second_start), + "the first row's verdict must answer for the rest of the run" ); } diff --git a/big-code-analysis-ast/src/checker/rust.rs b/big-code-analysis-ast/src/checker/rust.rs index bc6debdb..ecf9e5f6 100644 --- a/big-code-analysis-ast/src/checker/rust.rs +++ b/big-code-analysis-ast/src/checker/rust.rs @@ -107,9 +107,13 @@ impl Checker for RustCode { /// in `spaces::metrics_with_options` only consults this hook /// when the caller opts in via `MetricsOptions::exclude_tests`, /// so the default `metrics()` entry point is unaffected. - fn should_skip_subtree<'a>(node: &Node<'a>, code: &[u8], ancestors: Ancestors<'a, '_>) -> bool { + fn should_skip_subtree<'a>( + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, + ) -> SubtreeSkip { if rust_prunable_item(node) { - return rust_item_is_test_only(node, code, ancestors); + return SubtreeSkip::of(rust_item_is_test_only(node, code, ancestors)); } // An outer attribute is a *sibling* of the item it marks, not a // child, so pruning the item never reached it and Rust's `Loc` @@ -121,7 +125,15 @@ impl Checker for RustCode { // still inserted by that code, and `Stats::settle_excluded_rows` // keeps it (#1417). Retracting the row afterwards could not tell // the two apart. - node.kind_id() == Rust::AttributeItem - && rust_attribute_run_marks_test_item(node, code, ancestors) + // + // The verdict is the whole run's, not this row's, so it is + // reported as reaching the rest of the run: the walker then + // asks once per run instead of once per row, which is what + // keeps a deep file carrying one long run out of `O(run^2)` + // (#1446). + if node.kind_id() == Rust::AttributeItem { + return rust_attribute_run_verdict(node, code, ancestors); + } + SubtreeSkip::RETAIN } } diff --git a/big-code-analysis-bench/src/shapes.rs b/big-code-analysis-bench/src/shapes.rs index b9132baa..331519e6 100644 --- a/big-code-analysis-bench/src/shapes.rs +++ b/big-code-analysis-bench/src/shapes.rs @@ -294,6 +294,43 @@ pub fn wide_attributed_fns(width: usize) -> String { format!("{}\n", "#[inline] fn f() {} ".repeat(width)) } +/// Rust: `depth` nested `fn f`, then one run of `3 * depth` +/// `#[cfg(test)]` rows on a single innermost `fn t`. +/// +/// The diagonal the two probes above cannot reach, and the shape #1446 +/// was quadratic on. Each grows one axis with the other pinned: +/// [`nested_attributed_fns`] carries one attribute per level, and +/// [`wide_attributed_fns`] carries thousands of one-attribute items at +/// depth 1. Neither produces a *long run*, and the prune's cost is per +/// run, not per attribute. +/// +/// The `3 *` is what makes the shape adversarial rather than merely +/// large. `forward_attribute_scan_budget` allows a parent +/// `3 * depth` children wide before the reading flips to the +/// depth-priced sibling walk, so a run sized to that budget is +/// guaranteed to take the forward, `O(children)` branch — the budget +/// selects *into* the expensive reading exactly here. Asking once per +/// attribute then read the run `run` times: 0.08 / 0.34 / 1.21 / 5.18 s +/// at depths 250 / 500 / 1 000 / 2 000, against 0.05 s for the deepest +/// with `exclude_tests` off. +/// +/// The reading is `nom.total()`, which counts the `depth` retained +/// wrappers and not the pruned `fn t`. That is deliberate: a reading +/// taken from the *pruned* item would be this probe's own defect +/// output, and a future change to what the prune removes would zero it +/// and hand the gate a flattering exponent forever. The wrappers are +/// ordinary production code, so the reading is `depth` whether or not +/// the prune fires at all. +#[must_use] +pub fn nested_fns_with_attribute_run(depth: usize) -> String { + format!( + "{}{}fn t() {{}}\nlet x = 1;\n{}", + "fn f() {\n".repeat(depth), + "#[cfg(test)]\n".repeat(3 * depth), + "}\n".repeat(depth), + ) +} + /// Rust: `fn p() {}\n#[cfg(test)]\nmod m {}\n` repeated at file scope. /// /// The shape [`nested_attributed_fns`] and [`wide_attributed_fns`] @@ -599,6 +636,25 @@ const LINEAR_WIDTHS: [usize; 3] = [500, 1_000, 2_000]; /// than abandoning the probe as over budget. const SPACE_MERGE_WIDTHS: [usize; 3] = [4_000, 8_000, 16_000]; +/// Depths for `nom/deep-attribute-run`, whose shape grows *two* axes +/// with one parameter. +/// +/// Half [`LINEAR_DEPTHS`] at every rung, and reasoned rather than +/// harmonised. The shape renders `4 * depth` rows to a depth probe's +/// `depth`, so depth 2 000 here is already a 100 KB file with a +/// 6 000-attribute run — comfortably past where #1446's quadratic +/// showed (3.9x per doubling, ~2.0 fitted, 5.2 s at the top rung), +/// which is all the ladder has to reach. +/// +/// Going one rung higher costs accuracy rather than buying coverage. +/// A 4 000-deep cell walks a 200 KB tree under an 8 000-entry ancestor +/// chain, and its per-byte cost runs 1.7x the shallowest cell's against +/// the ~1.25x [`LINEAR_BOUND`] is set from — enough cache drift on its +/// own to fit 1.37 with the walk linear. That is a bound with 0.13 of +/// headroom on a shared runner, and the ladder below fits 1.0-1.1 +/// instead. +const ATTRIBUTE_RUN_DEPTHS: [usize; 3] = [500, 1_000, 2_000]; + /// Bound for a probe expected to be linear in its size parameter. /// /// Set from measurement, not from theory. A genuinely linear walk does @@ -1103,6 +1159,35 @@ pub const PROBES: &[Probe] = &[ against the node's depth, and this probe is what \ keeps the shallow-wide half of that trade measured.", }, + Probe { + name: "nom/deep-attribute-run", + lang: LANG::Rust, + axis: Axis::Depth, + workload: Workload::Metrics { + exclude_tests: true, + selection: &[Metric::Nom], + reading: |m| m.nom.total(), + }, + render: nested_fns_with_attribute_run, + sizes: ATTRIBUTE_RUN_DEPTHS, + max_exponent: LINEAR_BOUND, + rationale: "#1446: the diagonal neither `nom/nested-attributed-fn` \ + (one attribute per level) nor `nom/wide-attributed-fn` \ + (thousands of one-attribute items at depth 1) can \ + reach — depth and one attribute *run* growing \ + together. #1431 made the prune answer for every \ + `#[…]` row and not only the item, and each answer \ + re-derived the whole run, so the run was read `run` \ + times: 5.18 s at depth 2 000 against 0.05 s with \ + `exclude_tests` off, fitting ~2.0. The run's verdict \ + is now taken once and reused across its members \ + (`SubtreeSkip`), which is what this probe holds in \ + place. Note the sizing: the run is `3 * depth` \ + because that is exactly `forward_attribute_scan_ \ + budget`, so the shape is guaranteed to take the \ + forward `O(children)` reading rather than sampling \ + whichever branch a round number happened to select.", + }, Probe { name: "loc/wide-cfg-test-mod", lang: LANG::Rust, diff --git a/docs/development/benchmarking.md b/docs/development/benchmarking.md index 03cf562c..240b185d 100644 --- a/docs/development/benchmarking.md +++ b/docs/development/benchmarking.md @@ -149,6 +149,7 @@ Read it as follows. | `loc/nested-quote` | Elixir | depth | `loc`'s Elixir catch-all arm (#1096) | linear | | `nom/nested-attributed-fn` | Rust | depth | the `exclude_tests` outer-attribute scan (#1100) | linear | | `nom/wide-attributed-fn` | Rust | width | the same scan on the width axis (#1100) | linear | +| `nom/deep-attribute-run` | Rust | depth | the same scan on the diagonal: depth and one attribute run together (#1446) | linear | | `nom/nested-cfg-predicate` | Rust | depth | the `cfg(...)` predicate classifier (#1105) | linear | | `halstead/wide-distinct-fn` | Rust | width | per-child work at the space-merge boundary (#1106) | linear | @@ -261,6 +262,38 @@ the 94x #1100 measured — while every depth probe stayed inside its of widening) among them at 1.06. That is the evidence that the probe covers what it claims and that the depth probes do not. +**Two axes are not a plane** (#1446). Both probes above pin one axis +with the other held still, and the scan's cost is a product of the two, +so a shape that grows them *together* is outside the region either +covers. [#1431][attribute-row] made the prune answer for every `#[…]` +row rather than only for the item, and each answer re-derived the whole +run — which is `O(run)` done `run` times. It is invisible to +`nom/nested-attributed-fn`, where every run is one attribute long, and +to `nom/wide-attributed-fn`, where depth 1 keeps the budget at six +children and the reading on the sibling walk. + +`nom/deep-attribute-run` renders the diagonal: `depth` nested `fn f`, +then one run of `3 * depth` `#[cfg(test)]` rows on a single innermost +item. The `3 *` is the point rather than a round number — it is exactly +`forward_attribute_scan_budget`, so the parent is guaranteed to stay +inside the budget and take the forward `O(children)` reading. The +budget that exists to bound that scan selects *into* it on this shape. +Measured at 319 / 1 288 / 5 172 ms across the ladder, fitting **2.01**, +against 0.05 s for the deepest cell with `exclude_tests` off. The run's +verdict is now taken once and reused across its members +(`SubtreeSkip`, `big-code-analysis-ast/src/checker.rs`), which reads +1.5 / 3.1 / 6.4 ms and fits 1.06. The budget itself did not move: on +this diagonal the forward pass is still the cheaper of the two +readings, and what was quadratic was asking once per attribute. + +Its ladder is `ATTRIBUTE_RUN_DEPTHS` — half `LINEAR_DEPTHS` at every +rung — because the shape renders four rows per unit of depth. A +4 000-deep cell is a 200 KB tree under an 8 000-entry ancestor chain, +and its per-byte cost drifts 1.7x up the ladder against the ~1.25x +`LINEAR_BOUND` is set from: enough to fit 1.37 with the walk linear, +which is 0.13 of headroom on a shared runner. The shorter ladder still +reaches a 6 000-attribute run, well past where the quadratic shows. + The unit suite still pins the *dispatch* separately: `the_exclude_tests_prune_reads_forward_up_to_its_depth_scaled_budget` in `big-code-analysis-ast/src/node.rs` asserts which arm each boundary shape takes, so @@ -401,6 +434,7 @@ a walk's chain bookkeeping, not just around a change to its cost. The [halstead-climbs]: https://github.com/dekobon/big-code-analysis/issues/1096 [attribute-scan]: https://github.com/dekobon/big-code-analysis/issues/1100 [cfg-predicate]: https://github.com/dekobon/big-code-analysis/issues/1105 +[attribute-row]: https://github.com/dekobon/big-code-analysis/issues/1431 [space-merge]: https://github.com/dekobon/big-code-analysis/issues/1106 The ten control probes are what make the other readings mean diff --git a/src/metrics/loc.rs b/src/metrics/loc.rs index fe69d705..89dec441 100644 --- a/src/metrics/loc.rs +++ b/src/metrics/loc.rs @@ -7403,54 +7403,50 @@ EOF assert_eq!(loc.blank(), 1); } - /// The bound on the *other* side of the wrapper (#1443). + /// A `\`-continuation row inside a heredoc's command prefix is code + /// (#1443, #1445). /// - /// The test above pins the rows after the terminator; nothing pinned - /// the rows before the body, and `heredoc_redirect` spans the - /// command-line prefix as well as the literal. The grammar lets that - /// prefix cross rows — a `pipeline` is one of its children — so - /// crediting the wrapper's whole interior billed a blank or - /// comment-only prefix row as code. + /// `heredoc_redirect` spans the command-line prefix as well as the + /// literal, so crediting its whole interior over-credits a blank or + /// comment-only prefix row. #1443 narrowed the range to the literal's + /// own rows to fix that, and the narrowing was reverted: the shape it + /// fixed needs `bash -n` to reject the file, while the shape it broke + /// — this one — runs. /// - /// Two fixtures because the defect shows on two different axes, and - /// each is the only thing its own axis can come from: the blank row - /// is the file's only `blank`, and the comment row its only `cloc`. - /// A single fixture asserting `ploc` alone would keep passing if the - /// prefix row were trimmed out of it. - /// - /// Note both inputs are bash *syntax errors*: bash starts the body - /// on the line after the `<<`, so a blank or comment-only prefix row - /// is unreachable in runnable Bash — `bash -n` rejects all four - /// spellings, bare and `\`-continued. A prefix that crosses rows - /// *with content* is valid (`cat < Stats { metrics_verbatim( crate::LANG::Rust, @@ -12154,6 +12177,7 @@ class A { /// fixed). These fixtures put the attribute on the shared row, so /// that fix changes neither reading: what holds row 0 here is /// `fn a()`, not the attribute. + #[cfg(feature = "rust")] #[test] fn a_pruned_item_sharing_a_row_with_retained_code_keeps_the_row() { for source in [ @@ -12213,6 +12237,7 @@ class A { /// widening `exclude_span`'s bound by one, and for both `subtract` /// mutations. `own_rows` is anchored in the ordinary way — trim its /// pruned `mod` and `sloc 2` fails. + #[cfg(feature = "rust")] #[test] fn a_blank_row_does_not_absorb_a_phantom_exclusion() { // Row 0 `fn a` and the pruned `mod t`, row 1 blank, row 2 `fn b`. @@ -12253,6 +12278,7 @@ class A { /// 0 of 3,388 lib tests, while deleting the `ploc` one failed 3. A /// comment sharing a row with a pruned item is retained text on that /// row exactly as code would be, so the row must survive the prune. + #[cfg(feature = "rust")] #[test] fn a_comment_sharing_a_pruned_items_row_keeps_that_row() { // Row 0 `#[cfg(test)]`, row 1 the pruned `mod t` and a trailing diff --git a/src/metrics/loc/bash.rs b/src/metrics/loc/bash.rs index 9f8468a4..b4240545 100644 --- a/src/metrics/loc/bash.rs +++ b/src/metrics/loc/bash.rs @@ -113,55 +113,38 @@ impl Loc for BashCode { stats.ploc.lines.insert(start); add_string_interior_ploc(node, stats, start); } - // The wrapper, whose interior starts where the *body* does and - // not one row below the `<<` (#1443). - // - // `heredoc_redirect` spans the command-line prefix as well as - // the literal, and the grammar lets that prefix cross rows: a - // `pipeline` is one of its children. Crediting from - // `start + 1`, as the arm above does for a literal that *is* - // its own span, then bills every prefix row as code — - // including a blank or comment-only one. - // - // The first body row is one past the last row any non-body - // child occupies, which collapses to `start + 1` for the - // ordinary single-row prefix, so this is the same arithmetic - // everywhere except the shape it exists for. - // - // **What is and is not reachable here — measured, because - // the first reading of it was wrong.** A multi-row prefix - // *is* valid Bash whenever the continuation carries content: - // `cat < { check_comment_ends_on_code_line(stats, start); stats.ploc.lines.insert(start); - stats.ploc.lines.insert_range( - heredoc_body_first_row(node, start), - node.end_line().saturating_sub(1), - ); + add_string_interior_ploc(node, stats, start); } // An assignment standing as a statement of its own is one // logical line — but only then. @@ -209,47 +192,3 @@ impl Loc for BashCode { } } } - -/// The first row of `redirect`'s heredoc body: one past the last row any -/// of its non-body children occupies. -/// -/// `heredoc_redirect` covers the command-line prefix (`<<`, the marker, -/// and whatever the grammar hangs off the rest of the line) as well as -/// the literal, so the literal's own rows start below all of them. The -/// prefix is single-row in every runnable spelling, where this returns -/// `start + 1` and the caller behaves exactly as the sibling arm does. -/// -/// Reading the *body* node's start row instead would be wrong for the -/// shape #1412 is about: `heredoc_body`'s span begins at the first body -/// row that has text and collapses to zero width when the body is empty -/// throughout, so it cannot say where the body *begins*. The prefix can, -/// because it is bounded by the row the marker sits on. -/// -/// The floor keeps the range clear of the opening row the caller has -/// already credited, and it is **inert today**: every `heredoc_redirect` -/// the grammar emits carries the `<<` token, which starts on `start` at -/// a column above 0 and so ends at `start + 1` or later. Deleting the -/// floor fails no test — measured, 0 of 3,394 — which is why the -/// `debug_assert!` is here rather than a comment claiming the shape -/// cannot arise. It checks the premise on every heredoc of every walk, -/// so a grammar that ever emits a body-only wrapper reports that -/// directly instead of silently crediting rows above the literal. -fn heredoc_body_first_row(redirect: &Node, start: usize) -> usize { - let after_prefix = redirect - .children() - .filter(|child| { - !matches!( - child.kind_id().into(), - Bash::HeredocBody | Bash::HeredocBody2 | Bash::HeredocContent | Bash::HeredocEnd - ) - }) - .map(|child| child.end_line()) - .max(); - - debug_assert!( - after_prefix.is_some_and(|row| row > start), - "a heredoc_redirect at row {start} has no non-body child below its opening row" - ); - - after_prefix.unwrap_or(0).max(start.saturating_add(1)) -} diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index 64a03a84..2b07944b 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -8,6 +8,7 @@ use super::*; use crate::MetricSuite; +use crate::SubtreeSkip; use crate::diag::warn; // Walks that ended with a cognitive nesting slot still live. Freeing @@ -565,7 +566,11 @@ fn apply_comment_suppression( /// Propagating the flag down the traversal computes the same predicate in /// `O(1)` per node: a node is inside a comment iff its parent was, or the /// node itself is a comment. -#[derive(Clone, Copy)] +/// `Default` is the root's tag, and the only place one is built from +/// nothing: level 0, depth 0, and outside any comment are what "no +/// ancestors yet" means for all three fields at once. Every other tag +/// is derived from a parent's. +#[derive(Clone, Copy, Default)] struct Walk { /// Nesting level, used to close func-spaces on the way back up. level: usize, @@ -693,6 +698,35 @@ pub(crate) fn push_children<'a, 's, Tag: Copy>( &stack[first..] } +/// Whether `exclude_tests` elides `node` and its descendants, reusing +/// `run_verdict` when the classifier's last answer already reached this +/// far. +/// +/// The reach exists because some verdicts are about a *run* of +/// siblings rather than one node: Rust's `#[…]` rows all decorate the +/// same item, so a classifier that read the run to answer for the first +/// row has answered for the rest of them too. Asking per row instead +/// re-derived the run per row, which is `O(run^2)` on a file deep +/// enough to keep the run inside the forward-scan budget — 5.2 s at +/// depth 2 000 (#1446). +/// +/// Pre-order visits `start_byte` non-decreasing, so a reach the walk +/// has passed can never be re-entered and the stale-answer case does +/// not arise. `SubtreeSkip::RETAIN` reaches nothing, which is what the +/// walk starts from and what every classifier that answers per node +/// keeps returning. +fn prunes_subtree<'a, T: MetricSuite>( + node: &Node<'a>, + code: &[u8], + ancestors: Ancestors<'a, '_>, + run_verdict: &mut SubtreeSkip, +) -> bool { + if !run_verdict.answers_for(node.start_byte()) { + *run_verdict = T::Checker::should_skip_subtree(node, code, ancestors); + } + run_verdict.is_skipped() +} + pub(crate) fn metrics_inner( parser: &T, name: Option, @@ -752,16 +786,14 @@ pub(crate) fn metrics_inner( // #289). The root `Unit` state — always at index 0 once the walk // has visited the AST root — owns file-scoped markers. + // A classifier verdict that answered for a whole run of siblings, + // carried past the member it was asked about. See `prunes_subtree` + // (#1446). + let mut run_verdict = SubtreeSkip::RETAIN; + push_synthetic_unit_root::(&mut state_stack, &node, code, selected); - stack.push(( - node, - Walk { - level: 0, - depth: 0, - in_comment: false, - }, - )); + stack.push((node, Walk::default())); while let Some(( node, @@ -801,7 +833,7 @@ pub(crate) fn metrics_inner( // The hook is gated on `exclude_tests` so the default // `metrics()` entry point keeps emitting the pre-#182 // numbers byte-for-byte. - if options.exclude_tests && T::Checker::should_skip_subtree(&node, code, ancestors) { + if options.exclude_tests && prunes_subtree::(&node, code, ancestors, &mut run_verdict) { // `sloc` is span-based, not node-accumulated, so unlike every // other loc sub-metric it does not shrink just because we // skip the subtree. Record the pruned node's rows on the diff --git a/src/spaces_tests.rs b/src/spaces_tests.rs index f0c59a42..027edb9f 100644 --- a/src/spaces_tests.rs +++ b/src/spaces_tests.rs @@ -656,6 +656,13 @@ fn file_suppression_empty_stack_is_silent_noop() { // `cognitive_sum`, `n_operators`) rather than float magnitudes, // because Halstead floats are bit-brittle (lessons_learned.md). +// Every fixture below is Rust, and `RustParser::new` on a build +// without the language raises `LanguageDisabled(Rust)` — so without +// this the whole module fails, loudly and for a reason that has +// nothing to do with the change under test, on any feature subset that +// omits `rust` (`--features go` is the reproducer). Gating the module +// makes the tests *absent* there instead (#1446). +#[cfg(feature = "rust")] mod exclude_tests_rust { use crate::spaces::metrics_inner; use crate::{MetricsOptions, ParserTrait, RustParser}; @@ -1270,6 +1277,41 @@ impl Foo { assert_eq!(pruned.metrics.loc.sloc(), 1); assert_eq!(pruned.metrics.loc.ploc(), 1); } + + // #1446's shape, small enough to assert on: nesting deep enough + // that the attribute lookahead reads the parent's child list + // forward, and one attribute run long enough that re-deriving it + // per row was quadratic. The prune's answer is now taken once for + // the run and reused, so this pins that the *reused* answer is the + // same one every row used to compute for itself. + // + // Only the innermost `fn t` is test code, so a reach that ran past + // the item it stops at would take `let keep = 3;` — the row that + // makes `ploc` differ from the depth — with it. + #[test] + fn a_long_attribute_run_inside_nesting_prunes_exactly_its_own_rows() { + let attributes = "#[cfg(test)]\n#[allow(dead_code)]\n#[allow(unused)]\n\ + #[allow(clippy::all)]\n#[must_use]\n"; + let source = format!( + "fn a() {{\nfn b() {{\nfn c() {{\n{attributes}fn t() -> i32 {{ 1 }}\nlet keep = 3;\n}}\n}}\n}}\n" + ); + // Seven production rows — three `fn` headers, `let keep`, and + // three closing braces — against a thirteen-row file: the five + // attributes and `fn t` are the six the prune takes. + assert_eq!(analyse(&source, false).metrics.loc.ploc(), 13); + let pruned = analyse(&source, true); + assert_eq!( + (pruned.metrics.loc.ploc(), pruned.metrics.loc.sloc()), + (7, 7), + "the five attribute rows and the item they mark are the \ + prune's, and nothing else is" + ); + assert_eq!( + pruned.metrics.nom.functions_sum() as usize, + 3, + "`fn t` goes; `fn a` / `fn b` / `fn c` stay" + ); + } } // Non-Rust languages must ignore `exclude_tests = true` because