Skip to content

fix(cpp-lsp): use direct includes to disambiguate receiver targets - #2541

Open
knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:fix/cpp-duplicate-receiver-target
Open

knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:fix/cpp-duplicate-receiver-target

Conversation

@knewstimek

@knewstimek knewstimek commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

C++ member-call fallback can select a same-named receiver from an unrelated module, depending on registration order. This change uses current-module and resolved direct-include evidence to select a unique receiver, and prevents textual fallback only when an unknown C++ receiver has multiple equally ranked candidates that actually provide the requested member.

Forward declarations and homonyms without that member do not compete. Existing direct, alias and base-class lookup remains first, and an ordinary miss on an identified receiver does not become an ambiguity block. Plain C function-pointer calls retain their fallback. Method and field-owner selection share module-boundary and evidence ranking helpers.

The include map uses filename-stem modules for C-family File targets and reaches both the ordinary and prebuilt-registry paths. There is no additional include parser in the LSP. Exact occurrence blocking uses pooled scratch, allocated lazily only for C++/CUDA ambiguity output, and preserves unresolved diagnostics and pending-template handling.

Validation:

  • Windows x64 LLVM-MinGW 20260908 / clang 23.1.1 build succeeds. The C/C++ LSP suite passes 779 tests; nine focused pipeline regressions pass (seven existing controls plus the two new sequential/parallel cases).
  • Pipeline regressions cover direct and mirrored includes, reference/pointer calls, ambiguous duplicates, forward declarations, a sole method-bearing owner, and plain C function-pointer fallback in sequential and parallel extraction.
  • Existing receiver unit coverage for insertion order, namespace qualification and module-prefix boundaries is retained.
  • Changed ranges pass clang-format 20.1.8. Memory-core, no-test-skips and git diff checks pass.
  • Full lint success is not claimed. Diff-scoped clang-tidy reports five diagnostics in existing large/recursive functions: c_lookup_member_depth, c_resolve_calls_in_node_inner, cbm_pxc_build_import_map_ex and cbm_pxc_dispatch_file_with_php_bindings. A control on unmodified current main confirms the C LSP recursion/complexity categories already fail there; this change does not make the full lint gate clean. Their complexity scores are 98/909 with this change versus 71/901 on main. Linux cppcheck and full repository formatting remain CI checks.
  • Linux and sanitizer validation, and the maintainer's 19-project comparison, have not been run locally.

Scope remains heuristic receiver disambiguation using the pipeline's resolved includes; this does not implement compiler-equivalent visibility, transitive include closure or multiple compilation configurations. Existing stored graphs need affected callers reparsed. Issue #2542 remains separate.

@knewstimek
knewstimek requested a review from DeusData as a code owner October 4, 2026 03:56
Prefer a unique receiver consistent with the current module or a direct quoted relative header include. Preserve namespace boundaries and propagate exact-site ambiguity into CALLS materialization. Cover references, pointers, ties and parallel extraction with synthetic regressions.

Signed-off-by: knewstimek <knewstimek@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges labels Oct 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the direct-include disambiguation work and for clearly limiting the C++ visibility claim. I have read the seven-case description and Windows validation notes; the proposal remains pending review, with Linux/sanitizer validation still outstanding.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this, and for the careful write-up -- the reproduction with the mirrored header, the explicit statement of what the change does not cover, and the seven cases including the parallel path made it much easier to evaluate. You also found a real defect we can confirm independently: the short-name fallback in c_lookup_member_depth keeps the LAST registered type with a matching short name, so the target depends on discovery order, and its in-module check has no segment boundary. Your segment-boundary helper and "a tie gives no edge" are both the right idea.

We merged your branch onto a recent main and indexed 19 open-source C++ projects with and without it. It removes about 39,000 call edges net (2.2%), and every project loses edges. Some of what goes is genuinely wrong -- for example std iterator operators that were bound to a project's own iterator class, which is exactly the improvement we want. But the largest group is correct, high-confidence edges, for example:

  • leveldb util/status_test.cc:26, status2.IsNotFound() -> Status::IsNotFound
  • CMake Source/cmMessageCommand.cxx:240, mf.GetBacktrace() -> cmMakefile::GetBacktrace
  • CMake's bundled curl (plain C), Utilities/cmcurl/lib/http2.c:869, data->multi->push_cb(...)

The cause is that ambiguity is raised whenever two distinct same-named types tie, whether or not they declare the member (leveldb alone has five memberless forward declarations of Slice), and the resulting block also applies to plain C. What we would need:

  1. Raise ambiguity only when the receiver type itself was not found AND at least two candidate types actually declare the member. Forward declarations and member-less homonyms must not count; when exactly one candidate declares the member, keep resolving to it.
  2. Limit the blocking to C++ (cpp_mode). Plain C ptr->fn() field calls should keep their edges.
  3. Please reuse what main already has rather than a parallel version: c_qn_in_module, and the ranking used for field owners in c_field_owner_by_short_name (in-module, then directly included, then globally unique; members only; a tie gives NULL).
  4. Include evidence: please take it from the include map the pipeline already passes in (ctx->include_ns_qns) rather than a second quoted-include parser in the LSP, so the sequential and parallel paths always see the same includes. Honest caveat: today that map carries file-form names (".../foo.h.file"), so it never matches a type QN -- which is presumably why you wrote your own. If you would like to convert it to module form as part of this PR (the C-family branch of pxc_import_value_qn), that would be very welcome. If not, it is fine to drop the include rank for now: duplicate types then stay unresolved, which is the safe outcome.
  5. Create the blocking table lazily and only for C/C++, through the existing scratch pool (pxc_scratch_take) instead of a new arena per file. pxc_require_unambiguous_receivers currently runs for every language, including the TypeScript path, and per-file arenas were costly enough on large repositories that the pool was introduced for exactly this reason.
  6. Tests: please add a pipeline-level test to the C test suites that covers both the sequential and the parallel path (the parallel path starts above 50 files). tests/test_cpp_duplicate_calls.py is not run by any of our runners, so as it stands it does not protect the change. Your unit test in tests/test_c_lsp.c is good -- please keep it.
  7. Smaller points: the ambiguity branch jumps past the method_not_found diagnostic and the pending-template registration (goto recurse), and the requires_lsp_resolution doc comment no longer describes its use.
  8. Please rebase onto current main; c_lsp.c and tests/test_c_lsp.c conflict.

Once those are in, we will re-run the same measurement. The bar is simply that the correct edges like the three above stay while the wrong guesses go. Thanks again -- this is a part of the C++ graph that really needs the attention you are giving it.

Consider only method-bearing receiver candidates, preserve identified receivers and plain C fallback, and reuse include evidence and pooled scratch across extraction paths. Add unit and sequential/parallel pipeline regressions.

Signed-off-by: knewstimek <knewstimek@users.noreply.github.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you, knewstimek -- this update addresses every point from the last review, and in exactly the way we hoped. Ambiguity now only counts candidates that actually declare the member, the block is limited to C++ receivers whose type was not found, plain C function-pointer calls keep their edges, the include evidence now comes from the pipeline's own include map in module form (so both extraction paths see the same includes), the blocking table is created lazily from the scratch pool, and the new sequential and parallel pipeline tests run in our C suites -- we checked that CI executed them. Merging main instead of rebasing is perfectly fine for us.

The next step is ours: we will bring the branch up to date with main and re-run the same C++ comparison we described last time, and report the result here. Thanks again for the careful work and for being so clear about what the change does and does not cover.

This branch has not been deployed

No deployments
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants