Repository navigation
fix: batch 2026-11-03-2 (8 issues) - #1804
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1804 +/- ##
========================================
Coverage 98.38% 98.38%
========================================
Files 392 394 +2
Lines 117262 118063 +801
Branches 116827 117628 +801
========================================
+ Hits 115368 116158 +790
- Misses 1188 1194 +6
- Partials 706 711 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dekobon
left a comment
dekobon
left a comment
There was a problem hiding this comment.
Review of f48a2c80 (11 commits over c1506be3), with the R11 lens
Three reviewers: my own read, @astra-review (role-table lens), @sol-review (correctness / process). Every finding below was re-verified on a fresh worktree of the branch before being written down; the two P2s each come with the command that shows them.
Gates on the branch (my run, fresh worktree after make worktree-setup)
make pre-commit → BCA_GATE: pass (gate=pre-commit), 0 gpg failed to sign. make check-test-lang-gates-compare COMPARE_REF=origin/main OK. role-census 0 pending; role-coverage 5,998, observation ok; role-bypass 0 baselined sites across 168 files (the branch adds six allow exemptions and no baseline entry — the R11 contract holds). role-parity-corpus pass. All 11 signatures G. No .snap.new / .pending-snap. Baseline refreshes sit in the commits that moved the metric (86cf847b, 1bd43dc7, 288e28f5), not in the trailing changelog commit.
P2 — 1bd43dc7 (#1445): a leading bare \ row is credited as code even when it splices into a comment or a blank
src/metrics/loc/shared.rs:372-376 says so itself ("an approximation for the two rare shapes … which bash and tclsh read as no code at all") and credit() at :522-537 always takes the ploc branch when the row continues nothing. Measured, both fixtures bash -n-valid:
\ → branch {sloc 3, ploc 2, cloc 1, blank 0}
# note main {sloc 3, ploc 1, cloc 1, blank 1}
echo done
\ → branch {sloc 3, ploc 2, cloc 0, blank 1}
main {sloc 3, ploc 1, cloc 0, blank 2}
echo done
main has it right for these two by accident (the row read as blank); the branch regresses them to ploc 2 while fixing the common case. The fix shape: when previous_bare is None and the row continues nothing before it, look at what it splices into — the next physical row — and credit comment / blank / code accordingly, the mirror of continues_comment. The existing leading-row fixtures (loc.rs:8714, :8748) only spell \ + code; add the two shapes above for Bash, Tcl and iRules. (grammar-dispatch.md §6: this is valid input, so it gets a test; the C-family / Ruby / Elixir FIXME(#1801) pin is correctly labelled known-wrong, not endorsed.)
P2 — 33320180 (#1719): the shared Ruby body walk restates the Call row under an allow(NullSafe)
src/metrics/npa/ruby_body.rs:392-396:
// role-bypass: allow(NullSafe) — the visibility macro calls and the decorator-style ones …
Call | Call2 | Call3 | Call4 => RubyBodyShape::Call,That list is roles::ruby::call's owner list (roles/ruby.rs:134, Call { owns [Call, Call2, Call3, Call4] }), and the question the arm asks is "is this a call" — membership, not a consumer subset. The exemption names NullSafe because those kinds are also that row's holds_payer owners, which is what lets the bypass gate accept it; but the lint's own doc (check-role-bypass.py:41-50) reserves allow for naming, traversal, or "a subset of a row's owners that the row cannot express", and this is the whole row. This is the exact "two encodings of one fact" shape lesson 115 names, and it is the one place on the branch where it recurs: a Call5 alias on a future grammar bump lands on the row and both member walkers silently stop reading it. The Call row is ungated, so the fix is a no-op on output: _ if roles::ruby::call::lists_kind(kind) => RubyBodyShape::Call (or owns_kind with the chain, as the func_opener::is_callable line above it does), and drop the exemption. Zero-drift; the branch's own 1,707-file stdlib comparison is the check.
The other five allow lines are genuine traversal / naming / subset sites with the reason on the line (reads_through names the wrappers read for their statements, not whether they branch; is_statement_of is clause-vs-body position; SymbolArray is a literal read for the names it carries; SingletonMethod is the bucket). And checker/ruby.rs's ruby_documented_core is not a third copy of reads_through: it projects a doc target through IfModifier / UnlessModifier / BodyStatement / BlockBody, a different question with a different (and disjoint) list.
P3 — 4b4df7b4 (#1765): the new B1 summary in src/metrics/cognitive.rs:40-42 says break NUMBER / continue NUMBER increment, while php_numeric_break_not_counted (:~9906) and cognitive/php.rs:74-82 pin +0 citing #435 only
The PR body says item 2 stays open; the source does not. A reader of the test will "fix" it. One sentence beside the PHP arm and the test: "deliberate, unresolved deviation from v1.7 B1 — #1765 §2 / #1800 decide it" — the same marker R11 put beside the ?? / Elvis sequence rule.
Nothing found (per commit)
089e0584perturb: 26 self-tests pass; the single-target R11 form (cargo test -p … --all-features --lib -- <filters>) is accepted;timeout/env/nice/rustup runeach have a test;-qpacluster parsing stops at the first value-taking option.5a1730cerumdl lockstep: 36 self-tests;make actionlintpasses; repo root fromPath(__file__);_pc-*and_ci-*both wired, mirroring the ruff gate.2af96515/842abca1(#1718): Cpp and Mozcpp tested (npa.rs:736, 807;npm.rs:5186); the qualified-name loop descends strictly throughname; 20,000-level / 512 KiB-stack fixtures on both grammars. No row kind restated.86cf847b(#1649): 655 of 659 moved lines unchanged (git diff -M); olddoc_comment::paths kept through crate-private re-exports. Astra notes the issue records snapshot equality and the move diff rather than adrift-oracle.pyall-views run; for a pure move with re-exports that is sufficient, but a one-linedrift-oracle --lang <the 19 doc-gated languages>capture would put it on the series' footing — optional.e3794ca0(#1719): attribute targets askMember::ownswith the target's own chain; the 9 gained attributes are each explained on the issue with the command and revisions; perturbations recorded. Thenpa/ruby-nested-if-attrsprobe discriminates (1.10 vs 1.98 with unknown ancestry) — it prices nestedifs, not class nesting, which the PR discloses as unchanged-quadratic.288e28f5(#1307): scoped std threads +AtomicUsizework-stealing, worker-local vectors,--jobs 1spawns nothing, total order by (stripped path, seed rank, raw spelling);report_skipped_is_independent_of_job_count_and_keeps_seed_ordercompares--jobs 1vs8byte-for-byte including both overlapping-seed orders; nounwrap/expect/to_string_lossyin production; thehyperfinecommand and tree generator are on #1307.f48a2c80changelog: one entry per user-visible issue under the right heading; #1718/#1719 wording matches the code.
Two things to carry into the follow-up plan, not blockers here
- Ruby
npaquadratic in class nesting (disclosed): theChildAncestorschange it needs is ast-crate work; file it so it is not lost in a PR body. - The corpus spells no bare
\row in any.sh/.tclfile (146 files checked), so "no integration snapshot moved" for #1445 is plausible but is not evidence the new path ran on the corpus; the unit fixtures and the three validated scaling probes are the coverage.
Verdict: request changes for the two P2s (both small: one credit branch + fixtures; one lists_kind call and a dropped exemption). The P3 is a comment.
f48a2c8 to
0612e31
Compare
|
All three findings are fixed and folded into the commits they belong to. The branch is now P2: a leading bare
|
| Grammar | Fixture | Branch before | Now | Oracle |
|---|---|---|---|---|
| Bash | \ / # note / echo done |
3, 2, 1, 0 | 3, 1, 2, 0 | bash runs only echo |
| Bash | \ / blank / echo done |
3, 2, 0, 1 | 3, 1, 0, 2 | same |
| Bash | \ / # note (indented); \ / \ / # note; echo a / \ at EOF; # c \ / \ / blank |
code | comment, comment, blank, blank | same |
| Tcl | \ / # note; \ / blank; \ / # c \ / \ / blank |
code | comment, blank, comment | tclsh runs only puts |
| iRules | \ / # note; \ / blank; the comment form inside when HTTP_REQUEST { … } |
code | comment, blank, comment | the Tcl twins |
Every Bash fixture passes bash -n.
Perturbations, run as single edits against --lib metrics::loc:
- Classifying the leading run as code fails the Bash, Tcl and both iRules tests.
- Reverting the Bash comment-
\arm fails only the Bash test. - Never matching the comment fails all four.
- Never matching blank fails the Bash, Tcl and iRules table tests.
make bench-scaling puts all 110 probes within bound: loc/bash-nested-continued-fn 1.08, loc/tcl-wide-continued-comment 0.95, loc/tcl-unclosed-continued-comments 1.10. The shared.rs comment that called this an approximation, the book paragraph and the CHANGELOG entry are corrected.
P2: the body walk asks the Call row instead of restating it (d8187648, #1719)
RubyBodyState::shape now reads call::lists_kind(kind), and the allow(NullSafe) exemption is gone. make role-bypass reports OK — 0 baselined site(s) across 168 files.
This changes no output:
bca metrics --metrics npm,npa -p /usr/lib/ruby -O json -o out-<side>.json
old: f48a2c80 dev build (build inputs 10c7f6ee…)
new: 0612e31e dev build (build inputs 73bb084a…)
corpus: ruby 3.0.2, 1,707 .rb files
result: cmp byte-identical (5,888,954 bytes, both exit 0, no stderr)
P3: the PHP break N +0 is marked as a deliberate deviation (1d84960d, #1765)
The arm in cognitive/php.rs and the comment on php_numeric_break_not_counted now say that the +0 departs from v1.7 §B1, which lists a jump to a NUMBER as an increment. They also say #1765 item 2 decides it, so the citation alone should not prompt a "fix". It is cited as #1765 only: #1800 covers just the ??/Elvis question.
Gates on 0612e31e
make pre-commit:BCA_GATE: pass (gate=pre-commit), nogpg failed to sign.make check-test-lang-gates-compare COMPARE_REF=origin/mainandmake self-scan-baseline-fresh: both clean.- Clippy (
-D warnings) passes on every commit, and no baseline refresh was needed. - All 11 commits are signed
G.
The Ruby class-nesting quadratic will be filed as an ast-crate issue. On the corpus point: agreed. The unit fixtures and the three probes are the coverage for the bare-row path, not the integration snapshots.
0612e31 to
82a40c7
Compare
perturb.py documented the testing.md rule that a cargo argv naming more than one test target must carry --no-fail-fast, but did not enforce it. Without the flag, cargo stops at the first failing target, so a guard in any later target reads as dead rather than as a harness error. sweep() now refuses such an argv with exit 2 before any git read, backup or mutation. A `cargo test` argv (program basename cargo or cargo.exe, an optional +toolchain, global flags skipped, subcommand test or t) is accepted with --no-fail-fast anywhere before a bare `--`, or with exactly one package selector, exactly one of --lib, --doc, --test, --bin, --example, --bench, and at most one --target triple, since each triple builds test executables of its own. A selector value holding a glob character is refused, since cargo expands it to many targets. Short-option clusters are read as cargo reads them (`-qpa` is `-q -p a`), and a wrapper in front of cargo (timeout, env, nice, rustup run) is seen through. An argv with no cargo program in it, such as the self-test's python stub or a `sh -c` string, is unchecked. The flag is never injected: the manifest should say what it runs. Fixes #1787
82a40c7 to
e006715
Compare
Test audit of the PR's tests: 16 fixes, now
|
| Area | Fixed |
|---|---|
| perturb self-test (#1787) | The multi-target cases were built from perturb.MULTI_TARGET_FLAGS itself, so dropping --workspace passed the suite and let cargo test -p a --lib --workspace through. The flags are now spelled out. Added cargo --config X test, cargo -C dir test, and the -F/-j/-m value-letter rows. |
| rumdl self-test (#1779) | Two assertions matched text that other errors (or the footer) also print. Each now matches its own message. |
| loc (#1445) | Indented bare rows; two non-adjacent bare rows of different classes; per-space checks for a bare row between two functions; an exact-reading test for the three bare-row probes (each passed with its bare row removed); the raw-string row relabelled, since a leaf has no gaps. |
| npa (#1719) | while/protected → public, and the shadowed private moved to where the flag is public. Neither row could change the result before. The three-class test asserts per class, because the totals let two errors cancel. Added the private-flag half of "an ordinary call's argument takes the flag", and an npa-level pin of the documented @x-assignment scope. All values are checked against ruby 3.0.2. |
| CLI (#1307) | A unit test for the ./-stripped merge key. The non-UTF-8 test now uses two names that decode to the same lossy string, so a lossy merge would count one. The parallel test no longer claims to catch a double-claimed directory, which the merge dedupe makes unobservable. |
New issues, each pinned with a FIXME
- upstream(bash): heredoc body after a trailing |/&&/|| parses as commands #1806: tree-sitter-bash parses heredoc body rows after a trailing
|/&&/||as commands. This pins thecloc 1rows in the heredoc test. - fix(npa/npm/cpp): two member-pointer spellings misread through grammar recovery #1807: two C++ member-pointer spellings misread through grammar recovery. Pinned one spelling per class for C++ and Mozcpp, because together the class totals come out right.
- grammar(tcl, bash): a bare \ row joins the previous command, not the next row #1808: the Tcl and Bash grammars join a bare
\row to the previous command instead of the next row. Tclputs a/\/# creadsloc(3, 3, 0, 0) wheretclshgives (3, 1, 2, 0), andabc.branchesreads 1 instead of 2 in both grammars.
Not changed
holder_of's three fallbacks: no fixture or malformed probe reaches them. They return safe answers rather than asserting, because a zero-width recovery token could reach one; the source now says so.opens_comment's descent: it runs (on grammar(tcl, bash): a bare \ row joins the previous command, not the next row #1808's shape) but has not yet found a comment below a node.--jobsplumbing: no test checks that--jobsreaches the measurement. It's a one-argument call whose failure is "slower", so a bench would catch it, not a test.
Gates on e0067158
make pre-commit:BCA_GATE: pass.make check-test-lang-gates-compare COMPARE_REF=origin/main: clean.- Clippy and the baseline refresh pass on each rewritten commit; all 11 commits are signed.
The previous head's coverage job failed with No space left on device on the runner, so its Codecov patch result was empty; this push re-runs it.
The coverage job ran the runner out of disk on two consecutive runs of PR #1804 ("No space left on device" during the instrumented test step), so Codecov received no report. nextest runs each test in its own process, and cargo-llvm-cov's default profile name carries %p, so every process wrote its own .profraw: 8,085 files and 15 GB for the workspace suite, on top of a 6 GB instrumented build. Set LLVM_PROFILE_FILE_NAME=big-code-analysis-%4m.profraw for that step. Without %p, the processes of one test binary merge on-line into a pool of four files, which LLVM locks and adds counters into, so no count is lost. Measured locally with the job's own commands: 101 files and 62 MB, and all 409 files' covered-line and region counts identical to the default naming (totals 123,149 / 126,121 lines).
fa13a9e to
3838cfa
Compare
The coverage job ran the runner out of disk on two consecutive runs of PR #1804 ("No space left on device" during the instrumented test step), so Codecov received no report. nextest runs each test in its own process, and cargo-llvm-cov's default profile name carries %p, so every process wrote its own .profraw: 8,085 files and 15 GB for the workspace suite, on top of a 6 GB instrumented build. Set LLVM_PROFILE_FILE_NAME=big-code-analysis-%4m.profraw for that step. Without %p, the processes of one test binary merge on-line into a pool of four files, which LLVM locks and adds counters into, so no count is lost. Measured locally with the job's own commands: 101 files and 62 MB, and all 409 files' covered-line and region counts identical to the default naming (totals 123,149 / 126,121 lines).
Round 2 review: five findings fixed, one withdrawn. New tip
|
rumdl is pinned in mise.toml, the CI lint job's RUMDL_VERSION / RUMDL_SHA256, and the Dockerfile's ENV RUMDL_VERSION plus per-arch SHA256 table, and only comments kept them in step. #1653's bump nearly left the Dockerfile behind, which would have linted the dev container on a different rumdl than CI. Add utils/check-rumdl-lockstep.py and its self-test, modelled on check-ruff-lockstep. It fails when the three versions disagree, naming the odd file out (or all three when no two agree), and when the Dockerfile's x86_64 hash differs from CI's RUMDL_SHA256, since both verify the same tarball. The arm64 hash has no offline counterpart and stays covered by the image build's sha256sum --check. ci.yml and the Dockerfile are read by line, each anchored to a shape that must occur exactly once (the Dockerfile block is the heredoc RUN that interpolates RUMDL_VERSION, the arm the one naming the x86_64 triple), and any other shape is a hard error. Matching versions and matching hashes still pass a bump that moved all three versions and kept the old release's hash, because the stale hash agrees with itself. .rumdl-lockstep-baseline.txt records the version and x86_64 hash last accepted; the gate fails by name when the version moved and the hash did not (or the reverse), and fails a bump that moved both until `--update` records the new pair in the same commit, so the record never lags. `--update` refuses a pair that moved only one, and refuses when nothing moved. rumdl-only by decision: it is the only tool pinned in all three files. taplo, shfmt and shellcheck reach CI through install-action with no hash, and checkmake's arm64 container path is a different fork. Wired like ruff's gate: make targets plus -test, _pc-/_ci- pairs, the _pc-all / _ci-all / lint aggregates, pre-commit hooks, and explicit twin steps in the CI lint job. Fixes #1779
The white paper's URL is unversioned and its rules were reworded under it (v1.5 made the multi-level break/continue increment explicit), so a bare "§B1" could not be checked. Record v1.7, 29 August 2023, beside the link in the book and summarise its Appendix B (B1 increments, B2 nesting level, B3 nesting increments) in the cognitive module header. Correct the citations that the summary exposes. The five flat-+1 null-coalescing sites now cite B1 for the increment and B3 for the absent nesting increment. They also say that scoring `??` / Elvis at all is this crate's choice, because the paper's prose ignores null-coalescing operators. Ruby's redo/retry +1 is "by analogy with" B1, which names only goto and labelled or numbered break/continue. The PHP `break N` / `continue N` arm and its test keep +0 and now say that it departs from B1 deliberately, pending item 2 of the issue, so the citation alone does not read as a bug to fix. Comment and doc only; no metric value moves. Refs #1765
Both C++ grammars spell the templated member pointer `S::*fp` as a `qualified_identifier` whose `name` holds a `pointer_type_declarator`. `cpp_declares_function` recognised neither half of it: - inside parentheses the indirection was read as "still a function", so `template<class T> static int (S::*fp)(T);` counted under npm instead of npa; - outside parentheses the function under it was never seen, so the methods `template<class T> int (*S::*getPp())(T);` and `template<class T> static int S::*getPd();` counted under npa. `cpp_member_pointer` follows `name` through nested scopes to the pointer, and both arms apply the existing indirection rule to it: a `function_declarator` underneath keeps the member a method, a name makes it data. The method returning a member-function pointer, `template<class T> int (S::*get())(T);`, therefore stays a method. The walk is a loop, not recursion: every qualified type a member declaration holds is asked too, and a recursive walk overflowed the stack on an `A::A::...::B` type some 11,000 levels deep, which every other metric reads. A test pins 20,000 levels on a 512 KiB stack. The parenthesised non-templated `int (S::*pmf)(int);` was already right: both grammars recover it into an `ERROR` over `S::` beside a plain `pointer_declarator`. The attribute counter gains a matching arm so that whatever the predicate declines it counts, though no `field_declaration` reaches it under the pinned grammars. Two older recoveries still misread and are documented beside the arms: the unparenthesised `int S::*getPd();` reads as data, and `static constexpr int (S::*fp)(int) = nullptr;` as a method. Cpp and Mozcpp measured identically: the 13-member fixture moves from npa 4/4, npm 8/6 to npa 7/6, npm 5/4. Fixes #1718
Both C++ grammars read a member function template whose every
parameter parses as an expression as a variable with a parenthesised
initialiser: `template<class T> void connect(T* obj,
void (T::*slot)());` is init_declarator(connect, argument_list(...)).
cpp_declares_function recursed into the init_declarator, found no
function_declarator under it, and npa counted the method as an
attribute.
A member-declarator takes only a brace-or-equal-initializer
([class.mem.general]), so at class scope, the only scope these
helpers read, an argument_list value is never an initialiser: it
marks a method. Values spelled `= T(1)` or `{1}` stay data.
The fixture (connect and post, with data controls x and y) read
npa 4 / npm 0 before and npa 2 / npm 2 after, on Cpp and Mozcpp
alike; g++ -std=c++20 -fsyntax-only accepts it. The misread predates
the branch and was found reviewing the member-pointer fix.
Refs #1718
doc_comment.rs held two halves with one dependency edge: the binding
walk over Node and Ancestors, and the per-tool marker tests that read
only a comment's bytes. Move the 15 fn(&[u8]) classifiers, their
private helpers and their 12 tests into lang_helpers/doc_text.rs,
together with the module doc that describes them. The walk, the
DocBinding / KeptDocs types, node_bytes, never_joins and the run
helpers stay.
doc_comment re-exports the classifiers pub(crate), so every caller,
including the role rows, keeps its `doc_comment::` path. The public
`pub use doc_comment::{...}` walker re-exports are unchanged and
doc_text is crate-private, so nothing published moves. The move is
byte-identical: no snapshot moves, and the four gated walker tests stay
where they were.
The roles module doc's pointer to the marker tests now names
doc_text.rs.
The baseline loses doc_comment.rs's file-level loc.ploc row; the four
function-level rows keep their values.
Fixes #1649
Move the statement inventory of a Ruby class body out of npm into npa/ruby_body.rs: which wrappers run their statements at class scope (reads_through), which children of each are statements (is_statement_of), the explicit-stack traversal through wrappers and call argument lists, and the visibility state those statements set - the body-wide flag, bare-marker reading and the local-shadow table. The walk hands a consumer four events: a statement, an argument with the call it belongs to, and leaving a call once its arguments are read, the point at which npm re-files what `private :foo` named. npm's ledger keeps its declarations and retroactive re-filing and now implements the consumer. A call is whatever the Ruby `Call` row lists, asked through the row's kind table rather than restated beside it. No output change: the lib suite passes unchanged, and bca metrics --metrics npm,npa over the ruby 3.0.2 stdlib (1,707 files) plus the in-tree Ruby corpus is byte-identical before and after. The attribute walk adopts the driver in the next commit. Refs #1719
The Ruby attribute walk read the class body's direct children only, so an attr_* call or an @x = ... assignment under a conditional, a loop, a begin, a case arm, a statement modifier or a call's argument list was not counted. It also read a bare visibility marker only as a direct child and had no shadow table, so after `private = false` a bare `private` demoted every later attribute while npm, reading the same body, left its methods public. npa now drives the class-body walk npm uses (npa/ruby_body.rs). The changes in npa's output, in full: - attrs and ivar/cvar assignments under wrappers and in call arguments count; - a marker is read wherever the shared walk reads one (inside a wrapper, `x = private`, `foo(private)`), and a local of the same name shadows it, direct children included; - `private attr_accessor :x` is counted once, by the argument event that carries the enclosing keyword; ruby_wrapped_attr_count goes. The retroactive `private :a` stays unmodelled, and def bodies, blocks and nested class/module bodies are still not entered. ruby_wrapped_is_public takes the wrapping keyword as an Option, so npm and npa make one call for wrapped and unwrapped declarations, and RubyBodyConsumer::leave_call gets an empty default for a consumer that gathers nothing from a call's arguments. The module doc records what the walk still approximates, each shared with npm: a keyword wrapping an ordinary call or a statement wrapper leaves the attribute on the body flag; a multiple, operator, for or rescue assignment to @x is not counted; and a local bound outside the statements the walk reads does not shadow a later marker. Over the ruby 3.0.2 stdlib (1,707 files) npm is byte-identical and npa moves in 6 files, +9 attributes and +7 public, each an ivar or attr_* under a modifier, if/else or begin/rescue/else. Every new test value was checked against ruby 3.0.2. Adds the npa/ruby-nested-if-attrs scaling probe. Fixes #1719
Bash, Tcl and iRules splice `\` + newline away in the lexer, so a row holding only the backslash has no node, and blank = sloc - ploc - cloc claimed it: `echo a | \` + `\` + ` grep b` read ploc 2, blank 1. credit_bare_continuation_rows (loc/shared.rs) reads only the padding between a node's children. A bare row has no token, so it sits in the gap of exactly one node: the deepest one spanning it, whose stats are the innermost space's. Nested spaces get their own rows, and parents get them through Stats::merge. Each padding byte belongs to one node, so the scan is linear. A per-space scan of each space's whole range would cost bytes x depth. The new loc/bash-nested-continued-fn probe reads 1.09 and goes to 1.93 under that perturbation. The root also owns the padding before its first token and after its last, to EOF. Tcl and iRules continue a comment with `\`, but their comment token stops at the row's end. A bare row continuing a comment is therefore CLOC, decided by a tree lookup on the previous row's unescaped `\`; where a range arm already made the row code (a quoted word around the command substitution holding the comment) it is a comment on a code row, so blank does not lose it twice. The lookup starts at the child before the row's gap rather than the gap's owner: an unclosed opener per unit leaves every unit under one flat ERROR that tree-sitter never balances, and descendant_for_byte_range scans each level's children from the first. loc/tcl-wide-continued-comment prices the lookup per bare row, and loc/tcl-unclosed-continued-comments reads 1.10 against 2.09 from the owner. Bash never continues a comment, and tclsh, bash -n and bash confirm each fixture. A bare row that continues nothing starts a logical line and is what that line becomes once joined to the row after the run: a comment (`\` + `# note`), blank (an empty row, or EOF), or code. That row's first token is the gap's next child, so no lookup is needed. A Bash comment's trailing `\` is comment text, so a bare row after one starts a line too; bash and tclsh confirm each shape. The heredoc_redirect arm keeps main's whole-interior range. Narrowing it to the rows after the command prefix assumes a blank or comment-only prefix row is a bash syntax error, which is false after a trailing |, && or ||: tree-sitter-bash parses the first body rows as the operator's right-hand command, so `cat <<EOT |` + blank + `body` + `EOT` + `grep x` would lose a literal row. A test pins four such valid shapes at main's values. C, C++, Mozcpp, Objective-C, Ruby and Elixir splice the row the same way but are not wired yet; a FIXME test pins their bare row as blank (#1801). Where CRLF reaches the Tcl and iRules parsers unchanged (the library's analyze, Python's Ast.parse), an all-CRLF bare row is still lexed as a word (#1802). Fixes #1445
`bca check` re-lists every walked directory to count what ignore rules dropped (#1055). That pass ran serially on the main thread after the parallel walk joined. On a 65k-directory tree it cost more wall time than the walk and the analysis combined. Run it on the walk's own `threads` budget: per-directory jobs claimed through an atomic index by scoped workers (the calling thread included, so `--jobs 1` spawns nothing). Each worker returns its own rank-tagged results through its join handle, which re-raises a panicking worker's own payload. The merge sorts on (stripped path, seed rank, raw spelling), a total order, so overlapping seeds keep the first-named seed's spelling however the workers finished. `classify_dropped_child` now returns its verdict instead of pushing into a shared accumulator, and canonicalizes a dropped file only after the extension and glob checks that turn most away. Measured with hyperfine (15 runs, 16 cores, warm cache) over a synthetic 65,720-directory tree: `bca check` 339.7 ms -> 142.9 ms, with byte-identical --report-skipped output at -j 1 and -j 16. `-j 1` is unchanged (688.0 vs 691.9 ms), and so is this repo's analysis-bound run (1.800 vs 1.811 s). The new CLI tests build their expected paths with Path::join, and the non-UTF-8 fixture is skipped where the filesystem refuses the name. Part 1 of the issue (attributing via `IncrementalIgnore`) is not done. Under this walker configuration it agrees with the subtractive classification on every deterministic input and costs ~9 ms more; the issue comment has the measurements. Fixes #1307
The coverage job ran the runner out of disk on two consecutive runs of PR #1804 ("No space left on device" during the instrumented test step), so Codecov received no report. nextest runs each test in its own process, and cargo-llvm-cov's default profile name carries %p, so every process wrote its own .profraw: 8,085 files and 15 GB for the workspace suite, on top of a 6 GB instrumented build. Set LLVM_PROFILE_FILE_NAME=big-code-analysis-%4m.profraw for that step. Without %p, the processes of one test binary merge on-line into a pool of four files, which LLVM locks and adds counters into, so no count is lost. Measured locally with the job's own commands: 101 files and 62 MB, and all 409 files' covered-line and region counts identical to the default naming (totals 123,149 / 126,121 lines).
3838cfa to
07822c6
Compare
Follow-up to finding 1: an honest rumdl bump now fails until recorded. New tip
|
dekobon
left a comment
dekobon
left a comment
There was a problem hiding this comment.
Round 3 review of 07822c69 (12 commits over c1506be3; the 11 rewritten + fix(ci) on top)
The interdiff from e0067158 is exactly the five accepted findings plus the CI commit (9 files, +259/−15). Each verified on a fresh worktree; the round-2 finding 6 (bca-web CRLF) was withdrawn — every handler calls normalize_eol first (web/server/handlers.rs:261, 363, 409, 436), so the CHANGELOG and the book were right and the premise was mine.
Finding 1 (P2) — rumdl lockstep now catches the find-and-replace bump
Reproduced both directions at 07822c69, originals restored, tree clean after each:
- all three versions →
0.3.3, hashes untouched:error: rumdl version bumped to 0.3.3 but the x86_64-unknown-linux-gnu hash is still 0.3.2's; download the new release and paste its sha256 into .github/workflows/ci.yml and the Dockerfile.— exit 1. - versions and hashes moved, baseline not updated:
error: rumdl bumped to 0.3.3 (SHA256 000000000000...) but .rumdl-lockstep-baseline.txt still records 0.3.2; run … --update …— exit 1; after--updatethe baseline records the new pair and the gate is OK. So an honest bump cannot land without the baseline in the same commit, which is the ruff-lockstep shape. - clean tree:
rumdl lockstep OK (rumdl 0.3.2, … 3 sites checked).
Self-tests OK; the hook's files: filter includes the baseline and the script; _pc- / _ci- wiring unchanged; make actionlint passes; CHANGELOG #1779 entry describes the baseline.
Findings 2–5 — each as asked
- Book
metrics.md: PHP bullet in the deviations list, worded "deliberate … and an undecided one: #1765 (item 2)", matching the arm and the test. shapes.rsloc/tcl-wide-continued-comment: rationale now records the perturbed exponent (2.00 against 1.01 at widths 1 000–4 000), with the 20 s note kept for the probe's own sizes.npa.rs: the ratio is measured, not asserted — overflow at ~2,600 levels on the 512 KiB test stack (10,800 at 2 MiB, 43,500 at 8 MiB), soDEPTH = 20_000is ~7.7× past it. That is a tighter margin than my round-2 guess of ~28× (which assumed the release profile); the comment's instruction to keepDEPTHwell above the overflow depth for the stack it sets is the right pin.- #1718 issue: addendum present, with the SIGABRT output and why
perturb.pyreportsHARNESS-ERROR.
07822c69 fix(ci): pool coverage profiles per test binary
Not a review finding but a real one: the coverage job ran out of disk twice on this PR (8,085 .profraw, 15 GB, because nextest's one-process-per-test × cargo-llvm-cov's default %p name). LLVM_PROFILE_FILE_NAME=big-code-analysis-%4m.profraw drops %p so processes of one binary merge on-line into a 4-file pool; I confirmed cargo-llvm-cov 0.9.1 reads that variable explicitly (main.rs:210, and rejects a non-.profraw extension). The commit records a local measurement with the job's own commands: 101 files / 62 MB, every file's covered-line and region counts identical to the default naming. actionlint passes. The one thing to watch is the first green Codecov run on this PR, since that is the measurement that matters.
Gates on 07822c69 (my run)
make pre-commit → BCA_GATE: pass, 0 gpg failed to sign; make actionlint OK; check-test-lang-gates-compare COMPARE_REF=origin/main OK; role-bypass 0/168; role-coverage 5,998, observation ok; role-parity-corpus pass; 12/12 G; tree clean.
Verdict: approve. Nothing open on my side. Ready to fast-forward once CI and Codecov report on this tip.
Batch fix of eight issues, one commit per issue. Review fixes are folded into the commits they correct.
Commits
utils/perturb.pyrefuses acargo testargv that can run several targets without--no-fail-fast. It sees through wrappers (timeout,env,rustup run), splits short-option clusters (-qpa), and refuses a second--target.utils/check-rumdl-lockstep.pyholds themise.toml, CI and Dockerfile rumdl pins (and the x86_64 hash) in step, wired like the ruff gate.??/Elvis divergence from Sonar.template<class T> void connect(T* obj, void (T::*slot)());) is now a method, not an attribute.lang_helpers/doc_text.rs. Nothing published moves.fix(npa/ruby): read attributes through wrappers
npaandnpmshare one class-body walk. Over the ruby 3.0.2 stdlib (1,707 files),npmis byte-identical andnpagains 9 attributes.\counts as code, or as a comment where it continues a Tcl/iRules comment. The heredoc range keepsmain's whole interior.bca check's ignored-file measurement runs in parallel: 339.7 ms → 142.9 ms on a 65,720-directory tree, with byte-identical--report-skippedoutput. Part 1 (attribution viaIncrementalIgnore) was measured as a no-op and dropped; see the issue comment.Validation
make pre-commit:BCA_GATE: pass.make check-test-lang-gates-compare COMPARE_REF=origin/mainandmake self-scan-profile-parity: both clean.--workspace --all-targets -D warnings) passes on every commit, and each commit carries its own baseline refresh.loc/bash-nested-continued-fn,loc/tcl-wide-continued-comment,loc/tcl-unclosed-continued-comments,npa/ruby-nested-if-attrs.Known gaps
\row #1801: C, C++, Mozcpp, Objective-C, Ruby and Elixir still count a bare\row as blank. AFIXME(#1801)test pins this.\-continued comment parses its next row as code #1802: an all-CRLF bare row in Tcl/iRules is lexed as a word. This only matters on library paths that keep CR;bcaconverts line endings to LF first.npais quadratic in class nesting depth, asnpmalready was onmain. Fixing it needs aChildAncestorschange in the ast crate.Fixes #1787
Fixes #1779
Refs #1765
Fixes #1718
Fixes #1649
Fixes #1719
Fixes #1445
Fixes #1307