fix(metrics/loc): guard doc-comment row discount at EOF#1061
Merged
Conversation
A Rust doc comment ending at EOF has no trailing newline for the scanner to consume, so its `LineComment` node ends on its own start row. The unconditional `end - 1` then underflowed: debug builds panicked at the subtraction, release builds wrapped to `usize::MAX` and surfaced far away as a hash-table capacity overflow while inserting comment rows. Inputs as small as `/// x` were affected. Both reported panic sites are one bug. When `start == end == 0` the subtraction underflows directly; when `start == end > 0` it succeeds but drives `end` below `start`, underflowing `add_cloc_lines` instead. A guard of `end > 0` would fix only the first case, so the guard is `end > start` — discount the row only when the node really spans one. Rust's `LineComment` arm is the only caller that adjusts `end` before calling `add_cloc_lines`; the other 24 pass `init`'s span unmodified. `add_cloc_lines` therefore gains a `debug_assert` documenting that precondition, so a future caller reintroducing the inversion fails loudly in tests instead of wrapping in release. The first-party binaries never reached this — `bca`, `bca-web`, and the Python bindings all normalize line endings — but library callers using `analyze` / `Source` directly did, which is the documented entry point and the shape a fuzz harness would use. Values for newline-terminated input are unchanged, so no snapshots move. Tests go through `Source` directly: `check_metrics` trims and re-appends a trailing newline, which makes these inputs unreachable through it and any such test vacuous. Fixes #1051
Records the #1051 test-helper trap as a new sub-example under lesson 7 rather than as a new lesson. Lesson 7 already states the general pattern — an intermediate stage that "filters, normalizes, or short-circuits the input" makes a wrapper-level test pass for the wrong reason — so a standalone entry would have restated it with a different anecdote. What the new case adds is severity, not principle. Lesson 7's existing NaN example is a filter in one test's call path; here the normalization lives in `check_func_space`, the helper every LOC test shares, so it does not make one test vacuous — it makes an entire input class (a node ending at EOF) unreachable for the whole suite, and would have made a regression test written the ordinary way pass against the unfixed code. No renumbering: appended within lesson 7, so the lesson numbers other skills reference by index are unchanged. Follow-up to aaddda9 (#1051).
dekobon
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1061 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 267 267
Lines 65968 66021 +53
Branches 65538 65591 +53
=======================================
+ Hits 64673 64726 +53
Misses 854 854
Partials 441 441
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Code-review follow-ups on the #1051 guard. Order the guard's operands cheap-first: `end > start` is false for every plain line comment, since only the doc-comment scanner consumes the trailing newline, so it short-circuits `is_child`'s FFI child walk away on the common case. Both operands are pure, so this is semantically identical. Correct the `add_cloc_lines` comment, which claimed the `debug_assert` prevents "wrapping to usize::MAX in release". It cannot: for `/// x` the caller's arithmetic wraps before the call, so the helper sees (0, usize::MAX), `end >= start` holds, and the assert passes. Release also disables debug-assertions and overflow-checks outright. The comment now states the narrow scope the assert actually has. Cite `process_line_doc_content` by name instead of a scanner.c line range; the range was already one off against the pinned 0.24.2. Tests: `loc_verbatim` now takes a `LANG` rather than hard-coding Rust behind a language-agnostic name — the untestable-at-EOF class is shared by every Loc submodule — with `rust_loc` as the explicit Rust binding. Add CRLF coverage, the one boundary where the discount must still apply and nothing pinned it. Pin `/// a\n/// b` at EOF, which release under-counted as 1. Add absolute assertions to the parity test so it cannot certify a wrong value on both sides at once.
The #1051 changelog entry was wrong on three counts, all found in review. It claimed the Python bindings normalize line endings. `PyAst::parse` does not — it hands raw bytes to `Source::from_bytes`, unlike `analyze_source` and `Ast.from_path` — so `Ast.parse(..).metrics()` was itself exposed to the crash, not shielded from it. It claimed "metric values for all other inputs are unchanged". Only the row-0 shapes panicked. When the EOF doc comment sat on a later row, release builds did not crash: they silently reported one cloc too few, which also shifted blank and MI's comment percentage. Those values change with the fix, so the entry now says so and tells readers to re-check baselines. It claimed bca-web normalizes, blanket. Three of its seven source-consuming handlers deliberately do not (#640); the conclusion survives only because none of them compute Loc. Also narrow the lessons-learned claim that "every LOC test routes through check_metrics" — the integration suites route through read_file_with_eol instead. They hit the same wall, since that path also appends a trailing newline, which makes the lesson stronger: two independent chokepoints put the input class out of reach.
Owner
Author
|
Ran a high-effort code review (8 finder angles) over the branch. It found The two that matter1. The Python bindings were exposed; I said they weren't. 2. "No metric drift" was wrong. I verified the panic on three inputs
Three input classes change value. The changelog now says so and tells Also fixed
Not fixed — needs its own issue
|
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.
Problem
A Rust doc comment ending at EOF without a trailing newline panicked the
analyzer, from inputs as small as
/// x:/// xattempt to subtract with overflowHash table capacity overflow//! xfn f(){}\n/// xlet x = 1; /// d/// x\n(control)Release builds do not save us:
endwraps tousize::MAX, flows intoadd_only_comment_lines, and blows up inside hashbrown while extending anear-
usize::MAXrange.Reachable from
analyze/Sourcedirectly — the documented library entrypoint, whose own rustdoc example uses exactly that path — and from the
Python
Ast.parse(...).metrics()fast path, which hands raw bytes toSource::from_byteswith nonormalize_eol, unlikeanalyze_sourceandAst.from_path(big-code-analysis-py/src/ast.rs:123,131). ThebcaCLIand every
bca-webendpoint that computes metrics normalize their inputand were unaffected.
Root cause
Locdiscounts the extra row aDocCommentspans, because thetree-sitter-rust scanner consumes the trailing newline as part of the
token. At EOF there is no newline left to consume, so the node ends on its
own start row and the unconditional
end - 1underflows.The two reported panic sites are one bug, and that distinction drove the
fix.
init()returns(start_row, end_row)and tree-sitter guaranteesthe span is non-decreasing, so all 25
add_cloc_linescall sites are safe— except Rust's
LineCommentarm, the only one that adjustsenditself:/// xstart == end == 0loc/rust.rsdirectlyfn f(){}\n/// xstart == end > 0endbelowstart, underflowsloc/shared.rsA guard of
end > 0— the obvious reading of the debug panic — fixes onlythe first row. The correct guard is
end > start, which is alsoexactly the semantics of "exclude the last line": discount the row only
when the node really spans one.
Changes
src/metrics/loc/rust.rs— add theend > startguard, orderedcheap-operand-first so it short-circuits
is_child's FFI child walk onplain line comments. The comment records the EOF exception and cites
process_line_doc_contentby name rather than a line range, which wasalready one off against the pinned tree-sitter-rust 0.24.2.
src/metrics/loc/shared.rs—add_cloc_linesgains adebug_assert!(end >= start)and a documented precondition. It needed nobehavioural change: the precondition is real and every caller satisfies
it once
rust.rsis fixed. Saturating arithmetic here would have tradedwrong crashes for silently wrong comment counts, which is worse in an
analysis tool. The comment states the assert's narrow scope: it fires
only for
end < start, only in debug, and cannot catch a caller whose ownarithmetic already wrapped —
(0, usize::MAX)satisfiesend >= start.src/metrics/loc.rs— four tests plus aloc_verbatim(lang, source)helper and its Rust binding
rust_loc.docs/development/lessons_learned.md— records the test-helper trapas a sub-example under lesson 7 (see below).
This changes metric values
Only the row-0 shapes panicked. When the EOF doc comment sat on any
later row, release builds did not crash — they silently reported one
cloctoo few, which also shiftedblankand MI's comment percentage.Measured against pre-fix code under release arithmetic:
/// x,//! x,let x = 1; /// dcloc=1fn f(){}\n/// xcloc=0(no panic)cloc=1fn f(){}\r\n/// xcloc=0(no panic)cloc=1/// a\n/// b(EOF)cloc=1(no panic)cloc=2So any Rust file whose last line is an unterminated doc comment now reports
one more
cloc(and one fewerblank) than a2.0.1release build did.Downstreams should re-check
.bca-baseline.tomlentries and thresholds.No in-tree snapshots move — every fixture is newline-terminated — confirmed
by a clean
git statusafter the full gate.Testing
The tests deliberately bypass
check_metrics. It routes throughtools::check_func_space, which doestrim_end().trim_matches('\n')thenpush(b'\n')— it strips trailing newlines and appends one, making "a nodeending at EOF" structurally unreachable. The integration suites bypass that
helper but hit the same wall —
read_file_with_eolalso appends a trailingnewline. No test written the ordinary way could have caught this, and a
regression test added that way would have passed against the unfixed
code.
loc_verbatimgoes throughanalyze(Source::new(..))with nonormalization, takes a
LANG(the untestable-at-EOF class is shared byevery
Locsubmodule), and carries a doc comment explaining why it must notbe "simplified" back.
rust_doc_comment_at_eof_does_not_underflow— all five shapes, including/// a\n/// bat EOF, which release under-counted as 1.rust_doc_comment_at_eof_matches_plain_comment— parity with// x,plus absolute pins on the baseline so it cannot certify a wrong value on
both sides at once.
rust_doc_comment_with_trailing_newline_still_discounts_the_row— theno-regression guard, proving the new condition did not become a no-op.
rust_doc_comment_crlf_still_discounts_the_row— the boundary where thediscount is still owed (
\ris ordinary content, so the\nis stillconsumed), plus the lone-trailing-
\rcase where it is not.Test-via-revert per
.claude/rules/testing.md, using a precise inverseedit rather than a file-level restore (which would have wiped the
uncommitted fix): with the guard removed, the bug-pinning tests fail with
the expected panic, while the no-regression tests pass against both
versions — which is their job.
Sibling sweep
end - 1appears in no otherlocmodule; the doc-comment newlineadjustment is unique to Rust. Every other
- 1undersrc/metrics/isinside a test fixture's source string or unrelated
cyclomatic()arithmetic.
Lessons learned
Recorded as a sub-example under lesson 7, not as a new lesson — #7
already states the general pattern (an intermediate stage that "filters,
normalizes, or short-circuits the input") and its prescription. What this
case adds is severity: #7's existing example is a filter in one test's call
path, whereas here two independent chokepoints (
check_func_spacefor unittests,
read_file_with_eolfor the integration suites) put the input classout of reach of the whole suite. Lesson numbering is unchanged, so
index-based references from other skills still resolve.
Validation
make pre-commitgreen — 3,065 lib tests, clippy clean on default and--all-features, markdown lint, self-scan threshold gates, no snapshotdrift, no baseline refresh.
A high-effort code review over this branch found 10 issues, all fixed in
6b30fca0anda0ae4398; three were factual errors in this description,corrected above. See the review comment below for the full list.
Fixes #1051