feat(cpp): resolve inheritance-lattice member lookup#2077
Conversation
|
@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10752 tests passed 16 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
PR Tri-Review — feat(cpp): resolve inheritance-lattice member lookup
Methods (5 engines, asymmetric independence). GitNexus risk/test-CI lanes + Compound-Engineering personas (ISO-C++ semantics, performance, correctness, adversarial, maintainability) — all Claude — plus two non-Claude engines: Codex (independent review) and CodeQL (SAST gate). Weight Codex/CodeQL + a Claude lane agreeing as strong; agreement among Claude personas alone is "consistent," not independent confirmation.
Credit where due. The hard semantics are largely right and well-tested: virtual-base dominance and linear-chain name-hiding, the virtual-vs-non-virtual diamond grouping for the primary shapes (incl. a mixed virtual/non-virtual edge to the same base), primary name-hiding, and base-clause virtual detection were all verified faithful by the ISO-C++ lane. Common-case cost is O(1) (own-method / no-inheritance early returns) and capture-phase scaling stayed linear (1.162 < budget 1.5). Several suspected bugs were probed and refuted (below) — that validation is a feature.
Merge blockers + one P1 correctness bug
1. member-lookup.ts:390 greedy /<.*>/g — MERGE BLOCKER (CodeQL) + functional bug. The most-corroborated finding here: CodeQL + Codex + ISO-C++ + correctness (two non-Claude engines). CodeQL high-severity alert #683 (js/incomplete-multi-character-sanitization) fires on this line — the HTML-injection sink is a false positive (output feeds C++ base-name matching, not HTML), but the gate keeps the PR BLOCKED until the regex is fixed or the alert is dismissed-with-reason. It is also a real functional defect: A<x>::B<y> → "A" not "B" (the correctness lane executed it), so virtual/using facts on templated nested bases silently fail to match. One fix (split on :: first, then strip <…> per segment — or parse the node structurally) clears both.
2. GITNEXUS_BENCH red — MERGE BLOCKER (missing re-baseline). The new fixture cpp-member-lattice/main.cpp joins the lang-resolution/cpp-* corpus the bench fingerprints (fixture_count 273→274), so the cpp entry in bench/scope-capture/baselines.json must be re-baselined — it wasn't. Log: FAIL: cpp: capture fingerprint drift (got 0abf525f…, expected fd3d3768…). scaling_ratio 1.162 < budget 1.5 → not a perf regression. Regenerate the cpp fingerprint and add a _note (per the #1975/#1990/#1993 precedent).
3. member-lookup.ts:152-159 non-virtual diamond → FALSE edge (P1). The dominance filter (with isAncestor, 398-409) is virtuality-blind. For a non-virtual diamond where an intermediate class overrides the member, it emits a wrong CALLS edge where ISO C++ requires ambiguity — the one outcome this design tries hardest to avoid. Trigger: struct V{void f();}; struct A:V{void f();}; struct B:V{}; struct D:A,B{}; D d; d.f(); resolves to A::f (the V occurrence is dropped because isAncestor(V,A) is true), but C++ deems it ambiguous (A::f hides only A's V-subobject's f, not B's). The plainDiamondCall fixture omits the intermediate override and so masks this. (adversarial lane + coordinator re-trace; the ISO-C++ lane separately confirmed the virtual dominance path is correct, so this is specifically the non-virtual-subobject case.)
Inline findings (P2)
:316(+ capture:357) — same-simple-name bases in one file collide: parents are matched by simple name only (qualifier dropped at capture), so ausing/virtualfact can bind to the wrong base. (Codex + adversarial + ISO-C++):203— the early return incollectInheritedOccurrencesprecedes the member-using merge, so an inner class with both own methods andusing Base::fdrops the using'd overloads → wrong target. (Codex + maintainability):190(performance — emphasized in this review) —resolveCppReceiverMemberruns per call site with no(owner,member)memo; O(occurrences²) dominance (anisAncestorBFS per pair); and the walk enumerates root-to-base paths (exponential on deep non-virtual diamonds). Bounded by realistic inputs today, but latent given this indexer's documented O(n²) history.
Body-level (scope / process / maintainability)
this->member()bypasses the new hook (Codex-only, independent engine).receiver-bound-calls.tsCase 0.5 (376-462) never callsresolveReceiverMember, so an ambiguousthis->f()diamond still emits a leftmost-MRO edge — inconsistent with typed-receivervalue.f(). Appears pre-existing; the new ambiguity detection just isn't wired here.- No resolution-phase benchmark. The bench measures only the capture phase, so a green-after-rebaseline says nothing about the resolution-path cost above — worth a scaling fixture given the O(n²) history.
- Maintainability nits:
buildMrois overridden purely to runpopulateResolvedHierarchyas a side effect (a clearer lifecycle seam exists);CppReceiverMemberResolutionduplicates the contract'sReceiverMemberResolution;simpleNameis re-implemented a third time incpp/; the newresolveReceiverMembercontract param is typedCallsitethough the caller passesReferenceSite(compiles understrict:false; clarity nit);_scopesis unused.
Refuted — excluded (validation is a feature)
simpleName splitting on . is correct (C++ qualifiedName is dot-separated here; adl.ts:188); the Callsite→ReferenceSite change is runtime-safe (bivariant under strict:false; sole implementer typed Callsite); chooseOverload on an emptied candidate set falls through correctly; \0-joined keys can't collide (nodeId/filePath grammar); the worker collect/apply round-trip is complete (clearCppMemberLookupState runs before the apply loop); malformed bases (decltype/CRTP/template-template/anonymous) don't crash; cyclic inheritance terminates.
Test gaps
No coverage for: namespaced bases, cross-file bases, a non-virtual diamond with an override (finding #3), a worker-mode collect/apply round-trip unit test, or same-signature using-hiding.
CI
MERGEABLE but BLOCKED. Pass: typecheck/lint/format, tree-sitter ABI (all OS), packaged-install smoke, Analyze (js-ts) + Analyze (python) (the full CodeQL analysis itself passed), build/push. Fail (code): CodeQL gate (#683) + GITNEXUS_BENCH. Fail (infra): Vercel = deploy-auth. Pending: macOS/windows platform-sensitive, ubuntu coverage.
Coverage note
The GitNexus risk and test-CI persona lanes ended mid-investigation; those domains were synthesized from the cross-stream findings and the coordinator's own CI-log verification.
Automated multi-tool digest (Codex + CodeQL + 5 Claude persona lanes, vetted by an independent synthesis critic). Independence is asymmetric — verify before acting.
| } | ||
|
|
||
| function trailingIdentifier(value: string): string { | ||
| const withoutTemplates = value.replace(/<.*>/g, ''); |
There was a problem hiding this comment.
[MERGE BLOCKER · CodeQL #683 + functional bug · reproduced] Greedy /<.*>/g is an incomplete strip: on a multi-segment templated name like A<x>::B<y> it consumes from the first < to the last >, yielding "A" instead of "B" (the correctness lane executed the regex). Effect: virtual/using facts on templated nested bases silently fail the simpleName(def) === baseName match → over-suppression. Separately, CodeQL fires high-severity js/incomplete-multi-character-sanitization here; the HTML-injection sink is a false positive (this output feeds base-name matching, not HTML) but the gate keeps the PR BLOCKED. Fix once: split('::') first, then strip <…> per segment (or read the qualified_identifier node structurally). Corroborated by CodeQL + Codex + ISO-C++ + correctness (two non-Claude engines).
| @@ -0,0 +1,59 @@ | |||
| struct Left { | |||
There was a problem hiding this comment.
[MERGE BLOCKER · reproduced from CI] Adding this fixture grows the lang-resolution/cpp-* corpus that bench/scope-capture/measure.mjs fingerprints (fixture_count 273→274), so the cpp entry in bench/scope-capture/baselines.json must be re-baselined — it wasn't, so GITNEXUS_BENCH is red:
FAIL: cpp: capture fingerprint drift (got 0abf525f…, expected fd3d3768…)
scaling_ratio 1.162 < budget 1.5, so this is pure fixture-corpus drift, not a perf regression. Regenerate the cpp fingerprint and add a _note documenting the cpp-member-lattice addition (per the #1975/#1990/#1993 precedent). (baselines.json isn't in this diff, so the comment lands on the fixture that triggers the drift.)
| ); | ||
| if (occurrences.length === 0) return undefined; | ||
|
|
||
| const undominated = occurrences.filter( |
There was a problem hiding this comment.
[P1 · false edge · code-read] This dominance filter (via isAncestor, 398-409) is virtuality-blind, so it over-applies the cross-lattice dominance rule ISO C++ reserves for virtual bases. For a non-virtual diamond with an intermediate override it emits a wrong CALLS edge where C++ requires ambiguity. Trigger:
struct V { void f(); };
struct A : V { void f(); }; // non-virtual V
struct B : V {};
struct D : A, B {};
void c(){ D d; d.f(); }The code resolves A::f (the V occurrence is dropped because isAncestor(V,A) is true), but d.f() is ambiguous in C++ — A::f hides only A's V-subobject's f, not the one reached through B. The plainDiamondCall fixture omits the override and masks this. Fix: only let dominance cross a subobject boundary through a virtual edge; keep distinct non-virtual paths as separate groups so the site is suppressed as ambiguous. (The virtual-diamond and linear-chain dominance paths were separately verified correct.)
| nextActive.add(ownerDefId); | ||
|
|
||
| const definitions = model.methods.lookupAllByOwner(ownerDefId, memberName); | ||
| if (definitions.length > 0) { |
There was a problem hiding this comment.
[P2 · code-read] This early return fires before the member-using merge below (207-224), so an inner inherited class that has both its own method(s) and using Base::f; drops the using'd overloads. Trigger:
struct A { void f(int); };
struct B : A { void f(double); using A::f; };
struct D : B {};
void c(){ D d; d.f(1); }Resolves to B::f(double) instead of A::f(int) — a wrong edge (C++ picks the exact int match from the using-introduced overload). Merge own methods + member-using entries into the occurrence's overload set before returning. (Codex + the maintainability lane flagged the same root-vs-inner asymmetry.)
| readonly virtualAnchor?: string; | ||
| } | ||
|
|
||
| function collectInheritedOccurrences( |
There was a problem hiding this comment.
[P2 · performance · code-read] This walk runs per receiver-bound call site with no (ownerDefId, memberName) memo (the lattice is immutable during the pass, so the result is invariant across sites), enumerates root-to-base paths rather than nodes (the per-path new Set(active) at 199 re-walks a node reachable via K paths K times → exponential on deep non-virtual diamonds), and feeds an O(occurrences²) dominance filter that runs a fresh isAncestor BFS per pair. Bounded by realistic C++ today, but given this indexer's documented O(n²)/exponential history on kernel-scale and generated code: add a per-pass memo keyed ${ownerId}\0${memberName} (cache the callsite-independent occurrence/dominance result; run only chooseOverload per site) and precompute an ancestor closure instead of per-pair BFS. No resolution-phase benchmark currently guards this. (performance lane.)
abhigyanpatwari
left a comment
abhigyanpatwari
left a comment
There was a problem hiding this comment.
Hermes Agent Review
Verdict: Approve with suggestions — no critical issues. The algorithm is sound, the integration is clean, and the tests cover the right cases. Three suggestions below.
✅ What Looks Good
The inheritance-lattice algorithm is correct. The core loop in resolveCppReceiverMember (lines 117-181) correctly handles:
- Member-using declarations (
using Base::select) — resolves them first viamemberUsingsByDefId, picks the right overload viachooseOverload. Return-early at line 136 avoids descending into the lattice when using-declarations are in play — this is correct:usingbrings the base member into the derived class scope, so it is found directly. - Direct declarations hide base declarations —
if (ownMethods.length > 0) return undefinedat line 140 correctly lets the shared MRO walk handle the simple case. - DFS-based inherited occurrence collection —
collectInheritedOccurrenceswalksdirectParentsByDefIdto find members in the inheritance lattice. Cycle detection viaactiveset prevents infinite loops. - Non-dominated filter — lines 152-159 correctly prune dominated occurrences (those where the member exists in an ancestor that is in the MRO of another occurrence owner). This handles the
Dominant : Left, Right { void collide(); }case — the derived class declaration dominates both base declarations. - Virtual-diamond deduplication via grouping keys — lines 160-169 group occurrences by
(virtualAnchor, ownerDefId)or(path, ownerDefId). When a shared member comes through two virtual-base paths, both occurrences share the samevirtualAnchor, so they merge into one group →groups.size === 1→ not ambiguous. - Non-virtual diamond ambiguity — when the same declaration is reached through two non-virtual base sub-objects, each occurrence has a different
pathkey →groups.size !== 1→ reported as ambiguous. This matches the C++ standard: you must qualify which base sub-object you mean.
The side-channel pattern is consistent. captureCppMemberLookupFacts extracts baseEdges and memberUsings at capture time; applyCppMemberLookupSideChannel restores on the worker path; populateResolvedHierarchy builds the resolved directParentsByDefId, virtualEdges, and memberUsingsByDefId maps from the captured facts + the graph EXTENDS edges. The integration into collectCppCaptureSideChannel / applyCppCaptureSideChannel follows the established pattern (same as ADL, file-local, inline-namespace, two-phase channels).
The buildMro contract extension is clean. Adding buildMro to ScopeResolver as a per-language override lets C++ build the MRO from its own member-lookup hierarchy. The buildCppMemberLookupMro wrapper delegates to the shared buildMro with defaultLinearize, so it only adds the populateResolvedHierarchy pre-step.
Test coverage is thorough. The fixture cpp-member-lattice/main.cpp covers:
- Unrelated-base ambiguity (
Ambiguous : Left, Right) - Derived hiding (
Dominant : Left, Right { void collide(); }) - Virtual diamond (
VirtualLeft : virtual Root, VirtualRight : virtual Root → VirtualDiamond) - Non-virtual diamond (
PlainLeft : Root, PlainRight : Root → PlainDiamond) - Member-using overloads (
Derived : Base { using Base::select; void select(double); })
And cpp.test.ts has 6 assertions:
- Ambiguous call: 0 CALLS edges
- Dominant call: 1 CALLS edge, targets main.cpp
- Virtual diamond: 1 CALLS edge
- Non-virtual diamond: 0 CALLS edges
- Using-call: resolves to
select(int)overload - Resolution outcomes: both conservatively suppressed with reason
member-lookup-ambiguous
💡 Suggestions
1. isAncestor uses BFS with shift() — O(n²) memory churn (line 398-409)
isAncestor uses queue.shift() in a loop. For deep class hierarchies, this is O(n²) because shift() on a large array re-indexes all elements. Consider using a manual index pointer instead:
function isAncestor(ancestorDefId: string, descendantDefId: string): boolean {
const queue = [...(directParentsByDefId.get(descendantDefId) ?? [])];
const seen = new Set<string>();
let head = 0;
while (head < queue.length) {
const current = queue[head++];
if (current === ancestorDefId) return true;
if (seen.has(current)) continue;
seen.add(current);
const parents = directParentsByDefId.get(current);
if (parents !== undefined) queue.push(...parents);
}
return false;
}This is O(n) and avoids array re-indexing on every iteration. Not a blocker — this is called in the undominated filter at line 152, and C++ class hierarchies are rarely deep enough to matter. But given how clean the rest of the code is, fixing this micro-optimization is worth it.
2. populateResolvedHierarchy silently ignores graphIdToClassDef mismatches (line 327-328)
populateResolvedHierarchy builds defByGraphId and parents from graph.iterRelationshipsByType(EXTENDS). Then when building nextVirtualEdges, it resolves captured baseName by finding the EXTENDS parent whose simpleName matches (line 309-310). If the graph EXTENDS relationship and the captured base-edge describe the same inheritance but with different identifiers, this silently produces no virtual edge. Consider logging a warning when edge.isVirtual && parent === undefined so debugging is easier when resolution fails.
3. Consider caching collectInheritedOccurrences results per (ownerDefId, memberName)
collectInheritedOccurrences re-walks the entire inheritance lattice for every call site with the same (ownerDefId, memberName). In large C++ codebases with many ptr->method() calls on the same type, this is wasted work. A simple Map cache keyed by ${ownerDefId}:${memberName} could help. Not urgent — the current implementation is correct and the walk only hits classes that actually have the member (line 203 early-returns on first definition found).
Risk Assessment
- Correctness risk: LOW. The algorithm correctly implements C++ member-name lookup semantics (direct hiding, virtual diamond deduplication, non-virtual diamond ambiguity).
- Performance risk: LOW. The lattice walk only recurses into base classes that do NOT have the member themselves, and stops at the first occurrence. Only diamond cases cause deeper traversal.
- Integration risk: VERY LOW. The
resolveReceiverMemberhook is opt-in — only C++ sets it, all other languages continue using the shared MRO walk. The side-channel follows the established pattern used by ADL, two-phase lookup, file-local linkage, and inline namespaces.
Reviewed by Hermes Agent via Claude Code analysis
|
Addressed the tri-review findings in commit 9247d8d. Changes:
Regressions added for non-virtual override diamonds, inherited using overloads, qualified same-name bases, nested templated bases, explicit this receivers, cross-file bases, and worker JSON round-trip. Validation:
|
4 participants
Summary
usingfacts through worker side channelsusing Base::memberdeclarations from being treated as namespace importsValidation
npx tsc --noEmitnpx prettier --checkon all changed TypeScript filesnpx vitest run test/unit/scope-resolution/cpp/cpp-imports.test.ts(16/16)npx vitest run test/integration/resolvers/cpp.test.ts(305/305)REGISTRY_PRIMARY_CPP=0 npx vitest run test/integration/resolvers/cpp.test.ts(305/305)upstream/mainworktree (literal-collectorsC# receiver-node assertion and Swift scope-capture timing tripwire)Fixes #1891