fix(cpp): remap recovered definitions to original lines - #949
Conversation
DeusData
added
bug
|
Thanks for the focused C++ parser fix. Triage: parser recovery bug for #946, normal priority. Review focus is ownership of recovered callables, avoiding duplicate definitions across #ifdef branches, and keeping unrelated extraction behavior unchanged. |
|
This is the fix we most wanted from the 0.9.1-rc bug queue — the coverage signal we shipped this week (#963/#968) flagged exactly this class on our own codebase (#961: Two things before merge:
Genuinely excited about this one — with the rebase + the line-coordinate decision it merges. |
56da8cf to
eee5a31
Compare
|
Thanks for the detailed review — both requested items are addressed:
I also added coverage for both preprocessor branches, remap failure, header ownership, call-site ambiguity, and more than 256 recoveries. All CI checks are green. Ready for another look. |
5293205 to
69e4255
Compare
|
An update you should see before investing more here, with an apology attached: while working the twin issue #961 (same root cause — What #1050 does (deliberately minimal): re-runs the defs walk on the existing simplecpp second pass and adopts only defs that (a) intersect a raw-tree ERROR region, (b) have their name visible on the corresponding original source line, and (c) are absent by qualified name. It relies on simplecpp's same-file line alignment rather than explicit mapping. #946's SurfaceFlinger shape verifies fixed with it. Reading your latest revision, two things look like genuine improvements over what landed:
If you're willing, I'd welcome a rebase of exactly those two deltas on top of current main as the follow-up: the line-map hardening (with a case #1050 provably skips — that would be its RED test) plus your fixtures. Smaller PR, clearly additive, and it keeps your authorship on the part that outlives both versions. If you'd rather not re-cut it, say so and we'll handle the salvage ourselves with credit to this PR either way. Sorry again for the collision — and thank you for the careful iteration on this; the fail-closed mapping idea is the right instinct. |
Signed-off-by: Blank_Answer <97771966+blankanswer@users.noreply.github.com>
69e4255 to
99d3919
Compare
blankanswer
changed the title
|
Re-cut as requested on current This is now a single additive follow-up to #1050: explicit expanded→original line/file ownership mapping, fail-closed original-span validation, and the salvaged fixtures. The new include-shift test is the requested case #1050 provably skips: a 40-line resolved header shifts the expanded def outside the raw ERROR coordinates; mapping recovers it at original lines 3–12, excludes header defs, and clears the fully recovered parse range. Also retained the real #946 SurfaceFlinger shape for both branches and a macro-generated-callable rejection guard. |
|
Merged (ef14822). This is exactly the clean additive follow-up I hoped for — explicit expanded→original line/file mapping with fail-closed span validation, layered on top of #1050's recovery rather than replacing it. Verified: my #1050 |
A function-like macro invocation whose argument is a type token — e.g. ALLOC(int, n) — makes tree-sitter's C/C++ grammar emit an ERROR node (it parses the type in expression position), which cbm_collect_error_regions recorded as a parse_partial coverage gap. But the macro is #defined in the same file and the call sits inside an already-extracted function body, so nothing is actually missing from the graph — it's a benign call the grammar can't parse without the preprocessor (DeusData#1071, systematic across allocation-macro-style codebases). Subtract an error region only when it both (a) contains an invocation of a file-defined function-like macro and (b) is fully enclosed by an extracted Function/Method body. Condition (b) keeps a TOP-LEVEL macro invocation that expands to a definition still flagged (DeusData#949: the generated def isn't in the original span), and fails safe — a benign top-level call stays flagged rather than a real gap being hidden. Tests: a type-arg macro call inside a function no longer reports parse_partial; a real in-body syntax error still does; DeusData#949/DeusData#946 preserved. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…eusData#963) src/cli/cli.c reported an error range of 1-13047 — the whole file. The file indexed fine; the report was wrong. Three #ifndef _WIN32 blocks split a brace (two `if` headers, one closing brace), so the raw tree-sitter parse cannot resync at file scope, the root node becomes ERROR, and cbm.c takes its whole-file branch. The pipeline already parses these files a second time after preprocessing, and that parse is clean. The report just never consulted it. Build one byte per original line from the preprocessed pass, then cut each raw error range down to the runs of lines the second parse could not vouch for. Three rules, all found by running it and all load-bearing: - An expanded line only vouches for its original line when it HAS TEXT. The preprocessor emits a blank line where it dropped a branch; treating that blank as proof suppressed every C range in the suite. - Preprocessor directive lines (with backslash continuations) never count as missing code — the preprocessor consumes them, so the second parse can never vouch for one. Without this every #include block reported as a miss. Known cost: a #define the raw parse really dropped no longer shows up on its own. - A TOP-LEVEL macro invocation line never counts as vouched-for even when the expanded line parses clean. The macro can expand to a whole definition that the recovery walker deliberately refuses to adopt (DeusData#949), so a clean second parse there proves nothing. An in-body invocation is the benign DeusData#1071 case and is left to the existing macro subtraction. The order of the three coverage steps is now settled by where each one's evidence lives: recovery subtraction -> before the refinement; its evidence is a whole definition that STARTS inside the range, so it must be asked while the range still matches the construct the refinement -> middle DeusData#1071 macro rule -> after the refinement; its evidence is per-line, so a narrow range points at the call itself Measured on this repo: src/cli/cli.c goes from one whole-file range to 64 ranges over ~9.8% of the file, tests/test_cli.c from 48.6% to ~2.9%, src/cli/activation_transaction.c from 38% to 7.5%. What survives is honest — the biggest remaining ranges in cli.c are genuinely discarded #ifdef _WIN32 and #ifdef CBM_CLI_ENABLE_TEST_API blocks, absent from the graph on this platform. Both percentages above are floors, not measurements: cli.c and test_cli.c now land on exactly 64 ranges, which is CBM_MAX_ERROR_REGIONS. That cap drops regions with no signal, and a follow-up raises it and adds a truncation marker. Five tests, all red before the change: the range narrows to the dropped branch; lines the preprocessor explained are excluded; a range never starts or ends on a directive; real garbage beside a split brace stays flagged; a clean file stays unflagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…eusData#963) src/cli/cli.c reported an error range of 1-13047 — the whole file. The file indexed fine; the report was wrong. Three #ifndef _WIN32 blocks split a brace (two `if` headers, one closing brace), so the raw tree-sitter parse cannot resync at file scope, the root node becomes ERROR, and cbm.c takes its whole-file branch. The pipeline already parses these files a second time after preprocessing, and that parse is clean. The report just never consulted it. Build one byte per original line from the preprocessed pass, then cut each raw error range down to the runs of lines the second parse could not vouch for. Three rules, all found by running it and all load-bearing: - An expanded line only vouches for its original line when it HAS TEXT. The preprocessor emits a blank line where it dropped a branch; treating that blank as proof suppressed every C range in the suite. - Preprocessor directive lines (with backslash continuations) never count as missing code — the preprocessor consumes them, so the second parse can never vouch for one. Without this every #include block reported as a miss. Known cost: a #define the raw parse really dropped no longer shows up on its own. - A TOP-LEVEL macro invocation line never counts as vouched-for even when the expanded line parses clean. The macro can expand to a whole definition that the recovery walker deliberately refuses to adopt (DeusData#949), so a clean second parse there proves nothing. An in-body invocation is the benign DeusData#1071 case and is left to the existing macro subtraction. The order of the three coverage steps is now settled by where each one's evidence lives: recovery subtraction -> before the refinement; its evidence is a whole definition that STARTS inside the range, so it must be asked while the range still matches the construct the refinement -> middle DeusData#1071 macro rule -> after the refinement; its evidence is per-line, so a narrow range points at the call itself Measured on this repo: src/cli/cli.c goes from one whole-file range to 64 ranges over ~9.8% of the file, tests/test_cli.c from 48.6% to ~2.9%, src/cli/activation_transaction.c from 38% to 7.5%. What survives is honest — the biggest remaining ranges in cli.c are genuinely discarded #ifdef _WIN32 and #ifdef CBM_CLI_ENABLE_TEST_API blocks, absent from the graph on this platform. Both percentages above are floors, not measurements: cli.c and test_cli.c now land on exactly 64 ranges, which is CBM_MAX_ERROR_REGIONS. That cap drops regions with no signal, and a follow-up raises it and adds a truncation marker. Five tests, all red before the change: the range narrows to the dropped branch; lines the preprocessor explained are excluded; a range never starts or ends on a directive; real garbage beside a split brace stays flagged; a clean file stays unflagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
…eusData#963) src/cli/cli.c reported an error range of 1-13047 — the whole file. The file indexed fine; the report was wrong. Three #ifndef _WIN32 blocks split a brace (two `if` headers, one closing brace), so the raw tree-sitter parse cannot resync at file scope, the root node becomes ERROR, and cbm.c takes its whole-file branch. The pipeline already parses these files a second time after preprocessing, and that parse is clean. The report just never consulted it. Build one byte per original line from the preprocessed pass, then cut each raw error range down to the runs of lines the second parse could not vouch for. Three rules, all found by running it and all load-bearing: - An expanded line only vouches for its original line when it HAS TEXT. The preprocessor emits a blank line where it dropped a branch; treating that blank as proof suppressed every C range in the suite. - Preprocessor directive lines (with backslash continuations) never count as missing code — the preprocessor consumes them, so the second parse can never vouch for one. Without this every #include block reported as a miss. Known cost: a #define the raw parse really dropped no longer shows up on its own. - A TOP-LEVEL macro invocation line never counts as vouched-for even when the expanded line parses clean. The macro can expand to a whole definition that the recovery walker deliberately refuses to adopt (DeusData#949), so a clean second parse there proves nothing. An in-body invocation is the benign DeusData#1071 case and is left to the existing macro subtraction. The order of the three coverage steps is now settled by where each one's evidence lives: recovery subtraction -> before the refinement; its evidence is a whole definition that STARTS inside the range, so it must be asked while the range still matches the construct the refinement -> middle DeusData#1071 macro rule -> after the refinement; its evidence is per-line, so a narrow range points at the call itself Measured on this repo: src/cli/cli.c goes from one whole-file range to 64 ranges over ~9.8% of the file, tests/test_cli.c from 48.6% to ~2.9%, src/cli/activation_transaction.c from 38% to 7.5%. What survives is honest — the biggest remaining ranges in cli.c are genuinely discarded #ifdef _WIN32 and #ifdef CBM_CLI_ENABLE_TEST_API blocks, absent from the graph on this platform. Both percentages above are floors, not measurements: cli.c and test_cli.c now land on exactly 64 ranges, which is CBM_MAX_ERROR_REGIONS. That cap drops regions with no signal, and a follow-up raises it and adds a truncation marker. Five tests, all red before the change: the range narrows to the dropped branch; lines the preprocessor explained are excluded; a range never starts or ends on a directive; real garbage beside a split brace stays flagged; a clean file stays unflagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
2 participants
Summary
Follow-up to #1050 for #946/#961. The recovery already on
mainremains authoritative; this PR adds explicit simplecpp expanded-to-original line mapping and the broader regression fixtures requested in review.What changed
Validation
./build/c/test-runner extraction: all 215 functional extraction tests passed; the only suite failure was the pre-existing wide-flat wall-clock threshold under this 894 MiB/swap-constrained runner../build/c/test-runner parse_coverage: 9 passed.make -f Makefile.cbm lint-format: passed.git diff --check: passed.The previous six-commit implementation was backed up locally and replaced with this single additive commit on current
main.