diff --git a/gitnexus/src/core/ingestion/call-processor.ts b/gitnexus/src/core/ingestion/call-processor.ts index 6a37acc2e..a739c8441 100644 --- a/gitnexus/src/core/ingestion/call-processor.ts +++ b/gitnexus/src/core/ingestion/call-processor.ts @@ -1416,15 +1416,31 @@ const resolveCallTarget = ( // from scratch via `ctx.resolve`, which ignores that narrowing and // could pick a homonymous class from the wrong file. Fall through to // D1-D4 which respects the alias-filtered candidate pool. - const hasActiveModuleAlias = - !!call.receiverName && ctx.moduleAliasMap?.get(currentFile)?.has(call.receiverName) === true; - if (!overloadHints && !preComputedArgTypes && !hasActiveModuleAlias) { + // D0 skip for overload disambiguation: only fires when the name actually + // has multiple candidates in the tiered pool. The sequential path sets + // `overloadHints` for every call regardless of whether the method is + // overloaded — skipping D0 unconditionally would make this fast path + // dead code for the sequential pipeline. By gating on + // `filteredCandidates.length > 1`, we preserve the original intent + // (let D1-D4+E pick the right overload when there are multiple) while + // allowing D0 to fire for the common single-candidate case. + const hasOverloadConcern = + (!!overloadHints || !!preComputedArgTypes) && filteredCandidates.length > 1; + // D0 skip for active module alias: only fires when the alias block above + // actually narrowed filteredCandidates. In Python, a local variable can + // shadow an imported module name (e.g. `from models.c import C; c = C()` + // creates both a module alias `c → models/c.py` AND a typed local `c`). + // Checking `aliasNarrowed` rather than `ctx.moduleAliasMap.has(receiverName)` + // ensures D0 still runs when the method isn't in the aliased module — + // which means the receiver is a typed local variable, not a module reference. + if (!hasOverloadConcern && !aliasNarrowed) { const memberResult = resolveMemberCall( call.receiverTypeName, call.calledName, currentFile, ctx, heritageMap, + call.argCount, ); if (memberResult) return memberResult; } @@ -1475,6 +1491,23 @@ const resolveCallTarget = ( if (disambiguated) return toResolveResult(disambiguated, tiered.tier); return null; } + + // Zero-match null-route: we committed to receiver narrowing (D1 succeeded) + // but both file-based (D3) and owner-based (D4) filters produced zero + // matches. The lone candidate in `filteredCandidates` does not belong to + // this receiver type — refuse to emit a CALLS edge rather than fall + // through to the permissive single-candidate tail return. + // + // Addresses Codex review finding R3 (PR #744): member calls where + // fuzzy fallback picked a globally-matching symbol that has no + // relationship to the receiver's class hierarchy were silently + // producing false-positive edges. Example: Rust `c.trait_only()` where + // `trait_only` is captured as a Function node with no ownerId — it + // matches the name but fails both file and owner narrowing, so the + // old tail return would pick it incorrectly. + if (fileFiltered.length === 0 && ownerFiltered.length === 0) { + return null; + } } } @@ -1704,6 +1737,7 @@ const resolveMethodByOwner = ( filePath: string, ctx: ResolutionContext, heritageMap?: HeritageMap, + argCount?: number, ): { def: SymbolDefinition; tier: ResolutionTier } | undefined => { const typeResolved = ctx.resolve(receiverTypeName, filePath); if (!typeResolved) return undefined; @@ -1722,13 +1756,24 @@ const resolveMethodByOwner = ( // firstDef === undefined → owner-scoped resolution found nothing // firstDef && !ambiguous → unambiguous answer // ambiguous → genuine homonym ambiguity — refuse to pick + // + // argCount is threaded through so arity-differing overloads + // (e.g. C++ `greet()` vs `greet(string)`) are disambiguated inside the + // owner-scoped lookup rather than collapsing to an arbitrary first pick. let firstDef: SymbolDefinition | undefined; let ambiguous = false; for (const candidate of typeResolved.candidates) { if (!CLASS_LIKE_TYPES.has(candidate.type)) continue; const def = canWalkMRO - ? lookupMethodByOwnerWithMRO(candidate.nodeId, methodName, heritageMap, ctx.symbols, language) - : ctx.symbols.lookupMethodByOwner(candidate.nodeId, methodName); + ? lookupMethodByOwnerWithMRO( + candidate.nodeId, + methodName, + heritageMap, + ctx.symbols, + language, + argCount, + ) + : ctx.symbols.lookupMethodByOwner(candidate.nodeId, methodName, argCount); if (!def) continue; if (!firstDef) { firstDef = def; @@ -1784,8 +1829,16 @@ export const resolveMemberCall = ( currentFile: string, ctx: ResolutionContext, heritageMap?: HeritageMap, + argCount?: number, ): ResolveResult | null => { - const resolved = resolveMethodByOwner(ownerType, methodName, currentFile, ctx, heritageMap); + const resolved = resolveMethodByOwner( + ownerType, + methodName, + currentFile, + ctx, + heritageMap, + argCount, + ); if (!resolved) return null; return toResolveResult(resolved.def, resolved.tier); }; @@ -1880,9 +1933,12 @@ export const lookupMethodByOwnerWithMRO = ( heritageMap: HeritageMap, symbols: SymbolTable, language: SupportedLanguages, + argCount?: number, ): SymbolDefinition | undefined => { - // Direct lookup first (child override — no walk needed) - const direct = symbols.lookupMethodByOwner(ownerNodeId, methodName); + // Direct lookup first (child override — no walk needed). + // argCount is threaded through so arity-differing overloads on the direct + // owner can be disambiguated before the MRO walk starts. + const direct = symbols.lookupMethodByOwner(ownerNodeId, methodName, argCount); if (direct) return direct; const strategy = getProvider(language).mroStrategy; @@ -1909,9 +1965,10 @@ export const lookupMethodByOwnerWithMRO = ( ancestors = heritageMap.getAncestors(ownerNodeId); } - // Walk ancestors in MRO order — first match wins + // Walk ancestors in MRO order — first match wins. + // argCount narrows overloaded ancestors the same way as the direct lookup. for (const ancestorId of ancestors) { - const method = symbols.lookupMethodByOwner(ancestorId, methodName); + const method = symbols.lookupMethodByOwner(ancestorId, methodName, argCount); if (method) return method; } diff --git a/gitnexus/src/core/ingestion/symbol-table.ts b/gitnexus/src/core/ingestion/symbol-table.ts index 086148f2f..b8d39170c 100644 --- a/gitnexus/src/core/ingestion/symbol-table.ts +++ b/gitnexus/src/core/ingestion/symbol-table.ts @@ -1,6 +1,18 @@ import type { NodeLabel } from 'gitnexus-shared'; -export const CLASS_TYPES = new Set(['Class', 'Struct', 'Interface', 'Enum', 'Record']); +export const CLASS_TYPES = new Set([ + 'Class', + 'Struct', + 'Interface', + 'Enum', + 'Record', + // Traits are class-like for heritage resolution: PHP `use Trait;`, Rust + // `impl Trait for Struct`, and Scala traits all contribute methods to the + // hierarchy of their using/implementing type. Including Trait here lets + // buildHeritageMap resolve `h.parentName` to a Trait nodeId so the MRO + // walker can visit the trait and find its methods. + 'Trait', +]); export interface SymbolDefinition { nodeId: string; @@ -93,7 +105,24 @@ export interface SymbolTable { * overloads share the same returnType, undefined when return types differ (ambiguous). * Used by walkMixedChain for deterministic cross-class chain resolution. */ - lookupMethodByOwner: (ownerNodeId: string, methodName: string) => SymbolDefinition | undefined; + /** + * Lookup a method by owner class + name, optionally filtered by arity. + * + * When `argCount` is provided, overloads whose parameter count doesn't + * accommodate the call's argument count are filtered out before the + * returnType dedup runs. This lets D0 (`resolveMemberCall`) disambiguate + * arity-differing overloads (e.g. C++ `greet()` vs `greet(string)`) that + * would otherwise collide on the shared `ownerId + methodName` key. + * + * Same-arity, same-returnType overloads (e.g. `save(int)` vs `save(String)`, + * both returning `void`) still collapse to the first match — callers must + * gate D0 on overload concern before invoking this function for that case. + */ + lookupMethodByOwner: ( + ownerNodeId: string, + methodName: string, + argCount?: number, + ) => SymbolDefinition | undefined; /** * Look up class-like definitions (Class, Struct, Interface, Enum, Record) by name. @@ -225,9 +254,16 @@ export const createSymbolTable = (): SymbolTable => { } globalIndex.get(name)!.push(def); - // C2. Methods and constructors with ownerId go to methodByOwner index - // (in addition to globalIndex). - if ((type === 'Method' || type === 'Constructor') && metadata?.ownerId) { + // C2. Methods, constructors, and ownerId-bound Functions go to + // methodByOwner index (in addition to globalIndex). + // + // Some language extractors emit class methods as `Function` with an + // `ownerId` — notably Python (`def method(self):` inside a class body), + // Rust trait methods, and Kotlin object/companion methods. Treating + // `Function` with ownerId the same as `Method` here makes D0 + // (`resolveMemberCall`) work uniformly across all supported languages + // instead of silently falling through to D1-D4 fuzzy widening. + if ((type === 'Method' || type === 'Constructor' || type === 'Function') && metadata?.ownerId) { const key = `${metadata.ownerId}\0${name}`; const existing = methodByOwner.get(key); if (existing) { @@ -303,18 +339,42 @@ export const createSymbolTable = (): SymbolTable => { const lookupMethodByOwner = ( ownerNodeId: string, methodName: string, + argCount?: number, ): SymbolDefinition | undefined => { const defs = methodByOwner.get(`${ownerNodeId}\0${methodName}`); if (!defs || defs.length === 0) return undefined; - if (defs.length === 1) return defs[0]; - // Multiple overloads: return first if all share the same defined returnType (safe for chain resolution). - // Return undefined if return types differ or are absent (truly ambiguous — can't determine which overload). - const firstReturnType = defs[0].returnType; - if (firstReturnType === undefined) return undefined; - for (let i = 1; i < defs.length; i++) { - if (defs[i].returnType !== firstReturnType) return undefined; + + // Arity narrowing: when an argCount is provided and there are multiple + // overloads, keep only those whose parameterCount can accommodate the + // call. This resolves arity-differing overloads (e.g. C++ `greet()` vs + // `greet(string)`) that share the same `ownerId + methodName` key. + // + // Candidates with `parameterCount === undefined` (extractor didn't + // populate the count — typically variadic or unknown) are retained + // conservatively so that legitimate variadic matches still resolve. + let pool = defs; + if (argCount !== undefined && defs.length > 1) { + const arityMatched = defs.filter((d) => { + if (d.parameterCount === undefined) return true; + const min = d.requiredParameterCount ?? d.parameterCount; + return argCount >= min && argCount <= d.parameterCount; + }); + // Only adopt the arity-narrowed pool when it found matches; if arity + // rules out every candidate, fall back to the unfiltered set so the + // caller's fuzzy path still has something to work with. + if (arityMatched.length > 0) pool = arityMatched; } - return defs[0]; + + if (pool.length === 1) return pool[0]; + // Multiple overloads after arity narrowing: return first if all share + // the same defined returnType (safe for chain resolution), undefined if + // return types differ (truly ambiguous — can't determine which overload). + const firstReturnType = pool[0].returnType; + if (firstReturnType === undefined) return undefined; + for (let i = 1; i < pool.length; i++) { + if (pool[i].returnType !== firstReturnType) return undefined; + } + return pool[0]; }; const lookupClassByName = (name: string): SymbolDefinition[] => { diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/A.h b/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/A.h index 2c45b4518..314c3556f 100644 --- a/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/A.h +++ b/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/A.h @@ -1,5 +1,10 @@ #pragma once #include "Base.h" -class A : public Base { +// Virtual inheritance: together with `B : virtual public Base`, this creates +// a single shared `Base` subobject under `Derived`, so `d.method()` is an +// unambiguous call in real C++. Without the `virtual` keyword, a non-virtual +// diamond would produce two separate `Base` subobjects and the call would +// be ambiguous, requiring `d.A::method()` or `d.B::method()` to disambiguate. +class A : virtual public Base { }; diff --git a/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/B.h b/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/B.h index 3c7524ffd..7a9b90fff 100644 --- a/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/B.h +++ b/gitnexus/test/fixtures/lang-resolution/cpp-diamond-inheritance/src/B.h @@ -1,5 +1,7 @@ #pragma once #include "Base.h" -class B : public Base { +// See the comment in A.h — both sides of the diamond use virtual inheritance +// so there is exactly one `Base` subobject under `Derived`. +class B : virtual public Base { }; diff --git a/gitnexus/test/fixtures/lang-resolution/csharp-interface-default-method/App.cs b/gitnexus/test/fixtures/lang-resolution/csharp-interface-default-method/App.cs index dfb955e17..2e516fd93 100644 --- a/gitnexus/test/fixtures/lang-resolution/csharp-interface-default-method/App.cs +++ b/gitnexus/test/fixtures/lang-resolution/csharp-interface-default-method/App.cs @@ -4,7 +4,12 @@ public class App { public static void Run() { - User user = new User("alice"); + // Default interface methods in C# 8.0+ are reachable ONLY through + // the interface type, not as inherited class members. Declaring the + // variable as IValidator is the idiomatic way to invoke Validate(). + // `User user = new User(...); user.Validate();` would be a compile + // error because User does not expose Validate as a class member. + IValidator user = new User("alice"); user.Validate(); } } diff --git a/gitnexus/test/integration/resolvers/rust.test.ts b/gitnexus/test/integration/resolvers/rust.test.ts index d1c8c0fd2..5e03da39b 100644 --- a/gitnexus/test/integration/resolvers/rust.test.ts +++ b/gitnexus/test/integration/resolvers/rust.test.ts @@ -1859,23 +1859,26 @@ describe('Rust abstract dispatch (Repository trait)', () => { }); // --------------------------------------------------------------------------- -// SM-11: Rust Child struct — direct impl method resolution via D0 +// SM-11: Rust Child extends Parent — qualified-syntax MRO // // Companion integration test for the unit-level Rust qualified-syntax tests -// in symbol-table.test.ts. Validates end-to-end that Rust direct-impl methods -// resolve through the owner-scoped D0 path (`resolveMemberCall`). +// in symbol-table.test.ts. Validates end-to-end that: // -// NOTE on trait-inherited methods: Rust's qualified-syntax MRO strategy in -// `lookupMethodByOwnerWithMRO` correctly returns null for trait-inherited -// methods at the unit level. However, in the current pipeline, Rust trait -// default methods are captured as `Function` nodes (not `Method` with -// ownerId), so the owner-scoped index does not contain them. This means -// direct `obj.trait_method()` calls currently fall through to D1-D4 fuzzy -// widening rather than being correctly null-routed. Rust trait capture as -// Method-with-ownerId is a Phase 5 (SM-16) fix — out of SM-11 scope. +// 1. Direct `impl` methods on a struct resolve through the D0 owner-scoped +// path (`resolveMemberCall`) — the positive control. +// +// 2. Trait-inherited default methods are NOT reachable via direct +// `obj.trait_method()` syntax. Rust requires the trait to be in scope +// and uses qualified syntax for trait dispatch; the resolver correctly +// treats direct member calls as opaque to trait ancestry. +// +// Previously this case emitted a false-positive CALLS edge via the +// permissive tail-return in resolveCallTarget — Codex review finding +// R3 (PR #744). The tail-return is now null-routed when D1-D4 receiver +// filtering produces zero matches on both file and owner dimensions. // --------------------------------------------------------------------------- -describe('Rust Child direct-impl method resolution (SM-11)', () => { +describe('Rust Child extends Parent — qualified-syntax MRO (SM-11)', () => { let result: PipelineResult; beforeAll(async () => { @@ -1899,4 +1902,21 @@ describe('Rust Child direct-impl method resolution (SM-11)', () => { ); expect(ownCall).toBeDefined(); }); + + it('does NOT resolve c.trait_only() to Parent::trait_only via direct member call', () => { + // Qualified-syntax MRO: direct member calls on structs do not walk trait + // ancestry. `c.trait_only()` must null-route because `trait_only` is + // defined on the trait, not on the Child struct. + // + // The resolveCallTarget tail-return tightening (R3) is what makes this + // assertion testable: before the fix, resolveCallTarget would fall + // through D1-D4 (zero file matches, zero owner matches) and silently + // pick the single fuzzy candidate as a false-positive edge. + const calls = getRelationships(result, 'CALLS'); + const traitCall = calls.find( + (c) => + c.target === 'trait_only' && c.source === 'run' && c.targetFilePath.includes('parent.rs'), + ); + expect(traitCall).toBeUndefined(); + }); }); diff --git a/gitnexus/test/unit/call-processor.test.ts b/gitnexus/test/unit/call-processor.test.ts index 81dc9f042..b0c2b04eb 100644 --- a/gitnexus/test/unit/call-processor.test.ts +++ b/gitnexus/test/unit/call-processor.test.ts @@ -1716,7 +1716,24 @@ describe('processCalls — D0 MRO fast path (SM-10)', () => { expect(doWorkCalls).toHaveLength(1); }); - it('D0 skipped: same scenario still resolves via D1-D4 when heritageMap is undefined', async () => { + it('no heritageMap: inherited methods are unresolvable (null-routed, not false-positive)', async () => { + // Without a HeritageMap, the resolver cannot know that Parent.parentMethod + // belongs to Child's ancestry. The old D1-D4 tail-return would silently + // pick the lone fuzzy candidate and emit a CALLS edge — but that was an + // accidental match that happened to line up because `parentMethod` + // was unique in the global index. + // + // After the R3 tail-return tightening (PR #744 Codex review), member + // calls whose D1-D4 narrowing produces zero file-matched and zero + // owner-matched candidates null-route instead of falling through. + // The test now asserts the honest answer: without heritage information, + // we cannot attribute `c.parentMethod()` to `Parent` and therefore + // emit no edge. + // + // In the real ingestion pipeline, heritageMap is always threaded + // through, so this scenario is only reachable in tests that explicitly + // omit it. Keeping the test confirms the null-route behavior and + // documents the invariant "no heritage → no inherited-method edges". const { parentMethodId, appFile, parentFile, childFile } = setupChildParent(); await processCalls( @@ -1739,13 +1756,14 @@ describe('processCalls — D0 MRO fast path (SM-10)', () => { ], createASTCache(), ctx, - // no heritageMap — D0 fast path must be skipped, D1-D4 must still resolve + // no heritageMap — D0 MRO walk is unavailable, D1-D4 receiver filtering + // also cannot link c.parentMethod() to Parent, so no edge is emitted. ); const parentMethodCalls = graph.relationships.filter( (r) => r.type === 'CALLS' && r.targetId === parentMethodId, ); - expect(parentMethodCalls).toHaveLength(1); + expect(parentMethodCalls).toHaveLength(0); }); it('overloadHints guard: D0 skipped so literal-inferred overload disambiguation picks the right overload', async () => { diff --git a/gitnexus/test/unit/symbol-table.test.ts b/gitnexus/test/unit/symbol-table.test.ts index df30b2300..1ad33417a 100644 --- a/gitnexus/test/unit/symbol-table.test.ts +++ b/gitnexus/test/unit/symbol-table.test.ts @@ -798,8 +798,19 @@ describe('SymbolTable', () => { expect(table.lookupClassByName('Qux')).toEqual([]); }); + it('includes Trait in the class set (PHP use, Rust impl, Scala traits)', () => { + // Traits are class-like for heritage resolution — they contribute + // methods to the using/implementing type's hierarchy. buildHeritageMap + // relies on this to resolve `use Trait;` edges in PHP, `impl Trait for + // Struct` in Rust, etc. Added as part of PR #744 (SM-11 Codex review + // fixes) after the PHP HasTimestamps trait walk gap was discovered. + table.add('src/a.rs', 'Writer', 'trait:Writer', 'Trait'); + const results = table.lookupClassByName('Writer'); + expect(results).toHaveLength(1); + expect(results[0].nodeId).toBe('trait:Writer'); + }); + it('does NOT include other type-like labels outside the allowed class set', () => { - table.add('src/a.rs', 'User', 'trait:User', 'Trait'); table.add('src/a.ts', 'User', 'type:User', 'Type'); expect(table.lookupClassByName('User')).toEqual([]); });