fix(metrics/loc): whitespace-only carve-out (#1087) + comment rows counted as PLOC (#1135, #1137) - #1139
Merged
Merged
Conversation
Tcl and iRules are the only grammars here that surface the row terminator as a token child of the root rather than as extra. That token's start row is the row it terminates, so the `_` catch-all in their `Loc::compute` credited every terminated row to PLOC — comment-only rows and whitespace-only rows included. A realistic fourteen-row Tcl file with six comment rows reported `ploc 13` instead of `7`, which also drove `cloc + ploc` past `sloc`. A wholly empty row was unaffected (the LF starting on it is the one tree-sitter collapses at end-of-input); a row carrying trailing whitespace was not. `tcl_cloc` pinned the defect at `ploc == 2` for a two-row file with one comment row; its expectation and snapshot are corrected. `irules_cloc` asserted only cloc/blank, so it never saw the bug — it now asserts sloc and ploc too. Hoists the comment sitting between the `ExprCmd | Command` pattern alternatives above the arm. A comment inside a match pattern makes rustfmt emit the whole match verbatim while `cargo fmt --check` exits 0, so these two matches were outside the formatting gate. The site list in .claude/rules/formatting.md never covered src/metrics/loc/; it is corrected and its detection script now takes a directory. The remaining loc/ bails are #1136. Fixes #1135
#1067 established that a trailing newline is a formatting detail no LOC sub-metric may depend on. Whitespace-only source violates that: most grammars collapse tree-sitter's root to a zero-width node at end-of-input, so `b" "` reports `sloc 1` and `b" \n"` reports `sloc 0`. The behaviour is upstream and the newline-terminated answer is the defensible one, so the resolution is to state the carve-out rather than fabricate rows from a byte count — a second, non-AST source of truth for a metric the AST otherwise owns. Accepting a limitation is only defensible if its shape is known, and it had only been measured for Rust. Sweeping the tree found it is not uniform: twenty grammars collapse the root, five keep the span and are newline-independent already. Both halves are now pinned per language by `whitespace_only_input_is_the_documented_carve_out`, so a grammar bump that moves a language across fails a test instead of silently changing a metric. No behaviour change. Fixes #1087
`Loc for PerlCode` handles `P::Comments`, but the `#` token inside that node fell through to the `_` catch-all. That inserted the comment row into PLOC and, via `check_comment_ends_on_code_line`, reclassified it from comment-only to code-and-comment. Every Perl file with a comment-only row was affected. `$#array` is unaffected: the grammar emits a distinct `$#` token under `length_expression`, not `HASH`. `perl_cloc_line_comments` pinned the defect at `ploc 3` for a three-row fixture whose first row is comment-only; its inline snapshot is corrected to `ploc 2`. Adds `a_comment_row_is_never_counted_as_code`, which sweeps every language and comment spelling in both orders. This is the generalised form of #1135's guard, and it is what found the Perl case — the per-language `*_cloc` tests largely leave `ploc` unpinned, and `unterminated_one_line_file_reports_one_source_line` uses fixtures with no comment row at all. Fixes #1137
`.bca-baseline.toml` had drifted behind the tree: entries still
recorded values for functions that have since improved, and eighteen
carried stale `start_line`s from unrelated commits. Regenerating with
`make self-scan-write-baseline-headroom` tightens the gate without
touching any threshold.
Audited by key rather than by text diff — nothing loosens:
- added: 0 (no new offender is being masked)
- raised: 0 (no recorded value goes up)
- removed: 3 — vcs_command::emit and vcs_jit::emit (nexits),
vcs_command::write_csv (halstead.effort); all now under the
soft limit
- lowered: 3 — c_macro::replace and c_macro::step_normal
(halstead.effort), vcs_command.rs loc.sloc 815 -> 787
- start_line only: 18
Split from the #1137 fix, which patched its one entry by hand so the
metric it moved stayed legible in that commit.
`Cognitive`, `Cyclomatic`, and `Abc` end their per-language matches
with `_ => {}`, so an unconsidered token costs nothing. All 24 `Loc`
implementations instead end with a catch-all that inserts the row into
PLOC — an unrecognised token silently asserts "this row is code", and
through `check_comment_ends_on_code_line` can reclassify a
comment-only row as code-and-comment.
That inverted default is the shared root cause of #1135 and #1137, and
it is non-obvious precisely because the neighbouring metric modules
train you to read `_ =>` as harmless. Recorded in `loc::shared`'s
module doc, next to the helpers those catch-alls call.
Also extends lesson #85 (coverage measures execution, not
discrimination) with the same pair as a second instance: the guards
that should have caught these are the `*_cloc` tests, several of which
assert `cloc` and `blank` and leave `ploc` unasserted — so the
catch-all's own metric was the one no test named, while
`src/metrics/loc/tcl.rs` measured 95% line coverage carrying the bug.
Evaluated as a standalone lesson and rejected: it caused real bugs but
cost no real debugging time, and its test half duplicates #85.
The list said "ten sites, all under `src/metrics/`". The real count is 36, and the largest cluster is somewhere the file never mentioned: `src/getter/`, where 18 of 25 per-language modules bail, plus the JS-family macro body in `src/getter.rs`. Also adds `cyclomatic/` (4) and `abc/` (3). Two mistakes produced the undercount, both mine in a28c886: - The `src/metrics/`-only framing was inherited rather than checked. I swept `src/metrics/loc/` because that is what I was editing and generalised from it. - The detection script probed only the *first* `=> {` per file. The bail is match-scoped, so a module whose first match formats cleanly reports `ok` while a later one bails (`src/getter/c.rs`), and a module with only expression-bodied arms has no `=> {` to probe and reports `SKIP` (`src/getter/go.rs`). Over `src/getter` that is 11 false verdicts out of 18 bailing modules. The replacement script over-indents every arm header and reports how many rustfmt refused to restore. Extracted verbatim from the doc and run: getter 18, cognitive 7, cyclomatic 4, abc 3, loc 2, npa 0. Records the sharpest demonstration of the rule: over-indent `src/getter.rs:142`, run `cargo fmt --all`, and the line stays over-indented while `cargo fmt --all --check` exits 0. Found by code review of #1087. Broadens the scope of #1136.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1139 +/- ##
=======================================
Coverage 98.15% 98.16%
=======================================
Files 276 276
Lines 69223 69312 +89
Branches 68793 68882 +89
=======================================
+ Hits 67949 68038 +89
Misses 873 873
Partials 401 401
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1087 with option 1 (accept and document), and fixes two
metric defects the investigation turned up.
#1087 — the whitespace-only carve-out
#1067 established that a trailing newline is a formatting detail no LOC
sub-metric may depend on. Whitespace-only source violates that: most
grammars collapse tree-sitter's root to a zero-width node at
end-of-input, so
b" "reportssloc 1andb" \n"reportssloc 0.The issue had measured Rust only, so the first step was measuring the
rest — accepting a limitation is only defensible if its shape is known.
It is not uniform:
preproc,ccomment— 5The unterminated side is uniform: every grammar reports the row as
one blank line.
whitespace_only_input_is_the_documented_carve_outpins both sides perlanguage, so a grammar bump that moves a language across fails a test
rather than silently changing a metric. Option 2 (a byte-derived row
fallback) was declined: it adds a second, non-AST source of truth for a
metric the AST otherwise owns, to fix an input class real source does
not contain.
No behaviour change from this part.
#1135, #1137 — comment rows counted as code⚠️ metric-affecting
The sweep's "whitespace is never code" assertion failed for Tcl and
iRules. Both defects let a token reach the
_catch-all endingstats.ploc.lines.insert(start):grammars alone surface as a token child of the root, and whose start
row is the row it terminates. A realistic fourteen-row Tcl file
with six comment rows reported
ploc 13against a true7.Whitespace-only rows (trailing whitespace on a blank line) were also
counted as code, so
blankmoves too.#inside thecommentsnode, whichadditionally reclassified the row from comment-only to
code-and-comment. Every Perl file with a comment row was affected.
$#arrayverified unaffected: the grammar emits a distinct$#token under
length_expression.PLOC,
ploc_average,ploc_min/ploc_max, and (Tcl family)blankmove for Tcl, iRules, and Perl sources. No other language isaffected. MI is unaffected —
mi.rsreadssloc, notploc. Onein-tree snapshot moved (
tcl_cloc, metric-value-only, reviewed beforeacceptance); no integration snapshot moved, as the corpus has no Tcl,
iRules, or Perl files.
Both were invisible to the existing guards: several per-language
*_cloctests assertclocandblankand leaveplocunpinned, sothe catch-all's own metric was the one no test named.
a_comment_row_is_never_counted_as_codenow sweeps every language andcomment spelling, comment-before-code and comment-after-code — it found
the Perl case immediately after being written for the Tcl one.
Verification
else; reverting only the Perl arm fails exactly two, including the
cross-language sweep. Perturbing the carve-out membership list fails
only the carve-out sweep.
comment was the last instance.
make pre-commitgreen. Patch coverage above project coverage(
metrics/loc.rs99.94% lines vs 97.70% project).Also in this branch
chore(baseline)— reclaims accumulated self-scan slack, auditedby key rather than by text diff: 0 entries added, 0 values raised, 3
removed (now under the soft limit), 3 lowered, 18
start_line-only.The fix(metrics/loc): Perl counts comment-only rows as PLOC #1137 commit patches its own single entry by hand so the metric
it moved stays legible there.
docs(metrics/loc)— records the shared root cause inloc::shared's module doc:Cognitive/Cyclomatic/Abcendtheir matches with
_ => {}, but all 24Locimplementations end byinserting into PLOC, so the fallthrough is fail-open. Extends
lesson feat(cli): add --list-metrics command to big-code-analysis-cli #85 with the pair as a second instance. Evaluated as a
standalone lesson and rejected — it caused real bugs but cost no real
debugging time, and its test half duplicates feat(cli): add --list-metrics command to big-code-analysis-cli #85.
docs(rules)— corrects.claude/rules/formatting.md, whichclaimed "ten sites, all under
src/metrics/". The real count is 36,and the largest cluster is
src/getter/(18 of 25 modules). Both thesrc/metrics/-only framing and a detection script that probed onlythe first arm per file were mine, from earlier in this branch; found
by code review. Scope tracked in style: in-pattern comments disable rustfmt for 36 matches (18 in src/getter/) #1136.
Filed, not fixed
"loc.sloc" = 800predatesexclude_tests = truebytwelve days and was derived for test-inclusive SLOC; proposes gating
loc.plocwithloc.slocas a bloat backstop. Notesloc.plocisalready a supported threshold key, so no feature is needed.