Skip to content

fix(cpp): remap recovered definitions to original lines#949

Open
blankanswer wants to merge 1 commit into
DeusData:mainfrom
blankanswer:agent/fix-cpp-preproc-signature-gap
Open

fix(cpp): remap recovered definitions to original lines#949
blankanswer wants to merge 1 commit into
DeusData:mainfrom
blankanswer:agent/fix-cpp-preproc-signature-gap

Conversation

@blankanswer

@blankanswer blankanswer commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #1050 for #946/#961. The recovery already on main remains 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.

@blankanswer blankanswer requested a review from DeusData as a code owner July 8, 2026 04:38
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Jul 8, 2026
@DeusData DeusData mentioned this pull request Jul 8, 2026
2 tasks
@DeusData

DeusData commented Jul 8, 2026

Copy link
Copy Markdown
Owner

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.

@DeusData DeusData added this to the 0.9.1-rc milestone Jul 8, 2026
@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

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: cbm_path_within_root, handle_process_kill — both in-function #ifdef splits), and recovering missing callables from the already-existing preprocessor second pass is the right systemic answer: raw parse stays primary, expanded AST only fills verified gaps, deduped by label+QN. The defs-only walker entry point is a clean seam.

Two things before merge:

  1. Line coordinates of recovered defs. The merged callables carry EXPANDED-source line numbers, but get_code_snippet (and the UI's GitHub deep-links) read the ORIGINAL file at start_line..end_line — a drifted range shows the wrong code for exactly the methods this PR rescues. Please either remap to original coordinates where feasible, or mark recovered defs (e.g. a recovered_from_preprocessed property / reduced confidence) and document the drift — plus one test pinning whatever behavior you choose, so snippet consumers aren't silently wrong.

  2. Rebase onto current main — the end-of-extraction region of internal/cbm/cbm.c changed in feat(index): best-effort parse-coverage signal — flag not-fully-indexed files #968: the parse-coverage signal now computes error regions AFTER extraction and subtracts recovered definitions. Your merge hook lands in the same neighborhood (conflict likely), and there's a pleasant interaction to verify once rebased: callables you recover should automatically UN-flag the corresponding parse_partial ranges (a region fully covered by recovered defs is dropped). The regression test tests/test_parse_coverage.c shows the mechanics; a one-line assertion in your test that the fixture is no longer parse_partial-flagged would prove the two features compose.

Genuinely excited about this one — with the rebase + the line-coordinate decision it merges.

@blankanswer blankanswer force-pushed the agent/fix-cpp-preproc-signature-gap branch 2 times, most recently from 56da8cf to eee5a31 Compare July 10, 2026 05:45
@blankanswer

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review — both requested items are addressed:

  • Rebased onto current main.
  • Recovered callables now map expanded lines back to original main-file coordinates and fail closed when mapping is unreliable.
  • Raw definitions remain primary/fill-only; successfully remapped callables are used for parse-coverage, and the bug for parse c++ #946 regression verifies both coordinates and cleared parse_incomplete/error ranges.

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.

@blankanswer blankanswer force-pushed the agent/fix-cpp-preproc-signature-gap branch from 5293205 to 69e4255 Compare July 12, 2026 08:25
@DeusData

Copy link
Copy Markdown
Owner

An update you should see before investing more here, with an apology attached: while working the twin issue #961 (same root cause — #ifdef-split braces → ERROR region → dropped defs), I landed a minimal version of the recovery on main in #1050 (65a176e) without cross-linking it to this PR first. That overlap is on me — you had the fix candidate in flight for #946 and had just addressed review.

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:

  1. Explicit expanded→original line mapping with fail-closed semantics — my name-on-line check rejects misaligned defs but can't correct them; your mapping potentially recovers defs whose lines shift (e.g. resolved includes above the def), which fix(extract): recover C/C++ defs lost to #ifdef-split brace ERROR regions #1050 silently skips.
  2. Your fixture set — broader than the two shapes fix(extract): recover C/C++ defs lost to #ifdef-split brace ERROR regions #1050 pinned.

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>
@blankanswer blankanswer force-pushed the agent/fix-cpp-preproc-signature-gap branch from 69e4255 to 99d3919 Compare July 13, 2026 05:22
@blankanswer blankanswer changed the title fix(cpp): recover callables after preprocessor-gated signatures fix(cpp): remap recovered definitions to original lines Jul 13, 2026
@blankanswer

Copy link
Copy Markdown
Contributor Author

Re-cut as requested on current main in 99d3919.

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. parse_coverage is 9/9 and all 215 functional extraction tests pass locally; only the unrelated wide-flat wall-clock guard failed under this 894 MiB/swap-constrained environment. Ready for another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants