Repository navigation
fix(cpp-lsp): use direct includes to disambiguate receiver targets - #2541
knewstimek wants to merge 2 commits into
Conversation
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>
07cd40e to
7d3e09b
Compare
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
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
left a comment
There was a problem hiding this comment.
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:
- 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.
- Limit the blocking to C++ (cpp_mode). Plain C ptr->fn() field calls should keep their edges.
- 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).
- 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.
- 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.
- 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.
- 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.
- 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>
|
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. |
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:
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.