mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-24 00:51:53 +00:00
fix(cpp): suppress receiver-bound CALLS when integer-width overloads collide (U2)
C++ arity-metadata normalizes int, long, short, unsigned, size_t to 'int' so single-candidate flows like 'process(42L)' match a 'long'- typed parameter via loose matching. But when both 'process(int)' and 'process(long)' coexist as method overloads, they both end up with parameterTypes=['int'] in the registry, and pickOverload's narrowing returns 2 candidates with no way to disambiguate. The previous code picked candidates[0] arbitrarily, emitting a CALLS edge to the wrong overload roughly half the time. Fix: - Add isOverloadAmbiguousAfterNormalization in overload-narrowing.ts that detects >1 candidate sharing identical parameterTypes sequences. - Have pickOverload return a new OVERLOAD_AMBIGUOUS sentinel when this fires. - In the receiver-bound-calls loop, when pickOverload signals ambiguity, suppress the edge AND add the site to handledSites so the late-stage emitReferencesViaLookup pass does not re-emit the pre-resolved reference. Without the handled-mark, the reference index still carries a toDef and emits the same wrong edge. Graph schema has no ambiguous-target edge model, so emitting two edges (one per candidate) would require a separate schema change. Zero-edge is the only safe outcome. Other languages: the ambiguity check is a precondition gate, not a behavior change for normal narrowing. Languages whose normalizers do not collapse distinct types into a single token (verified by grep over *-arity-metadata.ts) will never produce >1 candidate with identical parameterTypes from genuinely distinct declarations, so the branch is effectively C++-only in practice. Test: cpp-overload-int-long fixture asserts exactly .toBe(0) CALLS edges. Count=1 = arbitrary pick (the bug); count>1 = unsupported ambiguous-edge model. Mode-gated to REGISTRY_PRIMARY_CPP=1 — legacy DAG has no OVERLOAD_AMBIGUOUS wiring; backporting is out of scope. All 2105 resolver integration tests pass under registry-primary; all 139 cpp tests pass under both modes (3 negative tests skipped in legacy as documented).
This commit is contained in:
parent
7e600fe8f2
commit
02fe4b2723
7 changed files with 130 additions and 4 deletions
|
|
@ -88,3 +88,48 @@ export function narrowOverloadCandidates(
|
|||
|
||||
return candidates;
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect when >1 candidate share identical `parameterTypes` after the
|
||||
* per-language normalizer has collapsed distinct underlying types. This
|
||||
* signals "the resolver cannot pick the right overload — the
|
||||
* normalization that helps single-candidate flows now hides a real
|
||||
* ambiguity" and lets callers suppress the edge rather than pick
|
||||
* arbitrarily.
|
||||
*
|
||||
* Concrete trigger (PR #1520 review follow-up plan U2, Claude review
|
||||
* Finding 5): the C++ `arity-metadata.ts` normalizer collapses `int`,
|
||||
* `long`, `short`, `unsigned`, and `size_t` to `'int'`. Without this
|
||||
* check, `process(int)` and `process(long)` both end up with
|
||||
* `parameterTypes === ['int']`, and `pickOverload` arbitrarily picks
|
||||
* the first — emitting a false CALLS edge to the wrong overload.
|
||||
*
|
||||
* Returns false when:
|
||||
* - 0 or 1 candidates (no ambiguity to detect)
|
||||
* - any candidate has undefined `parameterTypes` (can't compare)
|
||||
* - candidates differ in arity or in any parameter-type slot
|
||||
*
|
||||
* Other languages: this check is a precondition gate, not a behavior
|
||||
* change for normal narrowing. Languages whose normalizers do not
|
||||
* collapse distinct types (verified by grep over `*-arity-metadata.ts`
|
||||
* — no `int → int` collapse outside C++) will never produce >1
|
||||
* candidate with identical `parameterTypes` from genuinely distinct
|
||||
* declarations, so this returns false for them. The branch is
|
||||
* effectively C++-only in practice.
|
||||
*/
|
||||
export function isOverloadAmbiguousAfterNormalization(
|
||||
candidates: readonly SymbolDefinition[],
|
||||
): boolean {
|
||||
if (candidates.length < 2) return false;
|
||||
const first = candidates[0].parameterTypes;
|
||||
if (first === undefined) return false;
|
||||
for (let i = 1; i < candidates.length; i++) {
|
||||
const p = candidates[i].parameterTypes;
|
||||
if (p === undefined) return false;
|
||||
if (p.length !== first.length) return false;
|
||||
for (let j = 0; j < p.length; j++) {
|
||||
if (p[j] !== first[j]) return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -51,7 +51,7 @@ import {
|
|||
import { tryEmitEdge } from '../graph-bridge/edges.js';
|
||||
import { resolveCompoundReceiverClass } from '../passes/compound-receiver.js';
|
||||
import { resolveDefGraphId } from '../graph-bridge/ids.js';
|
||||
import { narrowOverloadCandidates } from './overload-narrowing.js';
|
||||
import { narrowOverloadCandidates, isOverloadAmbiguousAfterNormalization } from './overload-narrowing.js';
|
||||
|
||||
/** Subset of `ScopeResolver` consumed by this pass. Accepting the
|
||||
* subset rather than the full provider keeps tests and partial
|
||||
|
|
@ -454,9 +454,24 @@ export function emitReceiverBoundCalls(
|
|||
if (ownerDef !== undefined) {
|
||||
const chain = [ownerDef.nodeId, ...scopes.methodDispatch.mroFor(ownerDef.nodeId)];
|
||||
let memberDef: SymbolDefinition | undefined;
|
||||
let ambiguous = false;
|
||||
for (const ownerId of chain) {
|
||||
memberDef = pickOverload(ownerId, memberName, site, model);
|
||||
if (memberDef !== undefined) break;
|
||||
const picked = pickOverload(ownerId, memberName, site, model);
|
||||
if (picked === OVERLOAD_AMBIGUOUS) {
|
||||
ambiguous = true;
|
||||
break;
|
||||
}
|
||||
if (picked !== undefined) {
|
||||
memberDef = picked;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (ambiguous) {
|
||||
// Suppress and mark handled so `emitReferencesViaLookup`
|
||||
// doesn't re-emit the pre-resolved reference. See
|
||||
// OVERLOAD_AMBIGUOUS docstring for the upstream cause.
|
||||
handledSites.add(siteKey);
|
||||
continue;
|
||||
}
|
||||
if (memberDef !== undefined) {
|
||||
// For read/write ACCESSES, mirror the legacy DAG's reason
|
||||
|
|
@ -509,7 +524,7 @@ function pickOverload(
|
|||
memberName: string,
|
||||
site: ParsedFile['referenceSites'][number],
|
||||
model: SemanticModel,
|
||||
): SymbolDefinition | undefined {
|
||||
): SymbolDefinition | typeof OVERLOAD_AMBIGUOUS | undefined {
|
||||
const overloads = model.methods.lookupAllByOwner(ownerId, memberName);
|
||||
if (overloads.length === 0) {
|
||||
// Non-callable member (field / property / variable) — ACCESSES
|
||||
|
|
@ -520,5 +535,22 @@ function pickOverload(
|
|||
if (overloads.length === 1) return overloads[0];
|
||||
|
||||
const candidates = narrowOverloadCandidates(overloads, site.arity, site.argumentTypes);
|
||||
// When narrowing leaves >1 candidate that share identical normalized
|
||||
// parameter-types (e.g., C++ `f(int)` vs `f(long)` both collapsed to
|
||||
// `['int']` by `normalizeCppParamType`), suppress the edge entirely.
|
||||
// The graph schema has no ambiguous-target edge model, so emitting one
|
||||
// would arbitrarily pick a candidate and lie about the call's target.
|
||||
// PR #1520 review follow-up plan U2 / Claude review Finding 5.
|
||||
if (isOverloadAmbiguousAfterNormalization(candidates)) return OVERLOAD_AMBIGUOUS;
|
||||
return candidates[0] ?? overloads[0];
|
||||
}
|
||||
|
||||
/**
|
||||
* Sentinel returned by `pickOverload` when narrowing leaves >1 candidate
|
||||
* sharing identical normalized parameter-types. Callers should suppress
|
||||
* the CALLS edge AND mark the site as handled so `emitReferencesViaLookup`
|
||||
* does not re-emit from the pre-resolved reference index. See
|
||||
* `pickOverload` JSDoc for the upstream cause (per-language normalizer
|
||||
* collapses distinct types in arity-metadata).
|
||||
*/
|
||||
export const OVERLOAD_AMBIGUOUS = Symbol('overload-ambiguous');
|
||||
|
|
|
|||
6
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/caller.cpp
vendored
Normal file
6
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/caller.cpp
vendored
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
#include "service.h"
|
||||
|
||||
void run() {
|
||||
Service s;
|
||||
s.process(42);
|
||||
}
|
||||
4
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/service.cpp
vendored
Normal file
4
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/service.cpp
vendored
Normal file
|
|
@ -0,0 +1,4 @@
|
|||
#include "service.h"
|
||||
|
||||
void Service::process(int x) {}
|
||||
void Service::process(long x) {}
|
||||
7
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/service.h
vendored
Normal file
7
gitnexus/test/fixtures/lang-resolution/cpp-overload-int-long/service.h
vendored
Normal file
|
|
@ -0,0 +1,7 @@
|
|||
#pragma once
|
||||
|
||||
class Service {
|
||||
public:
|
||||
void process(int x);
|
||||
void process(long x);
|
||||
};
|
||||
|
|
@ -1664,3 +1664,28 @@ describe('C++ anonymous namespace symbols visible in same TU', () => {
|
|||
expect(wCalls.length).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// U2: integer-width overload ambiguity suppresses CALLS edge entirely
|
||||
// (PR #1520 review follow-up plan U2; Claude review Finding 5)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('C++ ambiguous integer-width overloads', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'cpp-overload-int-long'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('emits zero CALLS edges when process(int)/process(long) collide after normalization', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const processCalls = calls.filter((c) => c.source === 'run' && c.target === 'process');
|
||||
// Exact .toBe(0): any non-zero count is a regression. count=1 = arbitrary
|
||||
// pick (the bug U2 fixes); count=2+ would require an ambiguous-edge model
|
||||
// GitNexus does not have. The resolver must suppress entirely.
|
||||
expect(processCalls.length).toBe(0);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -101,6 +101,13 @@ const LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES: Readonly<Record<string, Readonly
|
|||
// out of scope.
|
||||
'does NOT resolve unqualified save() to User::save via #include',
|
||||
'does NOT resolve unqualified foo() to ns::foo via #include',
|
||||
// The legacy DAG path lacks the OVERLOAD_AMBIGUOUS suppression
|
||||
// wired through `pickOverload` + `isOverloadAmbiguousAfterNormalization`,
|
||||
// so it arbitrarily picks the first overload when `f(int)` and
|
||||
// `f(long)` collide after C++ integer-width normalization. Scope-
|
||||
// resolver-only correctness win (PR #1520 review follow-up plan U2 /
|
||||
// Claude review Finding 5); backporting to legacy is out of scope.
|
||||
'emits zero CALLS edges when process(int)/process(long) collide after normalization',
|
||||
]),
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue