From 53d1b7ba2b8aa385319ee4ba7944b3711a61be11 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Mon, 11 May 2026 18:58:59 +0100 Subject: [PATCH] fix(scope-resolution): require unique narrowing in pickImplicitThisOverload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex PR #1497 review, finding 2: pickImplicitThisOverload returned candidates[0] after narrowOverloadCandidates without checking uniqueness. When two same-name methods on the same class shared identical arity and the call site lacked disambiguating argument-type info, narrowing left both compatible and the resolver emitted a high-confidence CALLS edge whose target depended on registration order rather than a defensible resolution. Tighten the picker: - candidates.length === 1 -> return candidates[0] (unchanged for the unambiguous case) - candidates.length !== 1 (zero or multiple) -> return undefined (call left unresolved; no edge emitted) This mirrors pickUniqueGlobalCallable's existing pattern in the same file. Export pickImplicitThisOverload so a unit test can exercise it with synthetic stubs — PHP cannot produce the multi-overload failure shape (no method overloading in PHP), and C# integration coverage would entangle the unit's contract with the broader C# resolver. The unit test pins five cases: sole overload, narrowing-disambiguated, the ambiguous multi-candidate case (the bug regression), no-match, and no-enclosing-class. Verified: 972/972 across PHP + C# + Python + TypeScript + Go + C resolver suites; no regression in any language. tsc clean. Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U4) --- .../passes/free-call-fallback.ts | 21 ++- .../pick-implicit-this-overload.test.ts | 156 ++++++++++++++++++ 2 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 gitnexus/test/unit/scope-resolution/pick-implicit-this-overload.test.ts diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts index 401b3fe1c..eb6dd71fb 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts @@ -248,10 +248,18 @@ function pickConstructorOrClass( /** Walk up from the call-site scope to the enclosing class scope, * pick a method member by name with overload narrowing on arity + - * argument types. Returns undefined if there's no enclosing class - * or no matching method. Used for implicit-this calls inside a - * class body where multiple overloads share the call name. */ -function pickImplicitThisOverload( + * argument types. Returns undefined if there's no enclosing class, + * no matching method, OR narrowing leaves multiple compatible + * candidates — in the multi-candidate case, picking + * `candidates[0]` would emit a high-confidence CALLS edge whose + * target depends on registration order rather than a defensible + * resolution. Mirrors `pickUniqueGlobalCallable`'s uniqueness check + * in the same file (Codex PR #1497 review, finding 2). + * + * Exported for unit testing — language-agnostic logic, exercised + * via synthetic stubs in `pick-implicit-this-overload.test.ts`. The + * production call site is `applyFreeCallFallback` immediately above. */ +export function pickImplicitThisOverload( site: { readonly inScope: ScopeId; readonly name: string; @@ -284,6 +292,11 @@ function pickImplicitThisOverload( if (overloads.length === 0) return undefined; if (overloads.length === 1) return overloads[0]; + // Narrow on arity + argument types. Require a UNIQUE survivor — + // ambiguous narrowing (multiple compatible candidates with no + // disambiguating signal) leaves the call unresolved rather than + // routing to an arbitrary first overload by registration order. const candidates = narrowOverloadCandidates(overloads, site.arity, site.argumentTypes); + if (candidates.length !== 1) return undefined; return candidates[0]; } diff --git a/gitnexus/test/unit/scope-resolution/pick-implicit-this-overload.test.ts b/gitnexus/test/unit/scope-resolution/pick-implicit-this-overload.test.ts new file mode 100644 index 000000000..26f526382 --- /dev/null +++ b/gitnexus/test/unit/scope-resolution/pick-implicit-this-overload.test.ts @@ -0,0 +1,156 @@ +/** + * Unit tests for `pickImplicitThisOverload` — the implicit-`this` free-call + * resolver in `free-call-fallback.ts`. + * + * Codex PR #1497 review, finding 2: the previous implementation returned + * `candidates[0]` after `narrowOverloadCandidates` regardless of how many + * candidates survived narrowing. When two same-name methods on the same + * class had identical arity and unknown argument types, narrowing left both + * compatible and the resolver emitted a high-confidence CALLS edge whose + * target depended on registration order. The fix tightens the picker to + * require a UNIQUE post-narrowing candidate; otherwise the call is left + * unresolved. + * + * These tests exercise the function via synthetic stubs — no fixtures, no + * pipeline — because the failure shape (two same-arity overloads with + * indistinguishable types) cannot be produced by a PHP integration fixture + * (PHP forbids method overloading) and any C# fixture would entangle this + * unit's contract with the wider C# resolver. + */ + +import { describe, it, expect } from 'vitest'; +import type { Scope, ScopeId, SymbolDefinition } from 'gitnexus-shared'; +import { pickImplicitThisOverload } from '../../../src/core/ingestion/scope-resolution/passes/free-call-fallback.js'; +import type { ScopeResolutionIndexes } from '../../../src/core/ingestion/model/scope-resolution-indexes.js'; +import type { SemanticModel } from '../../../src/core/ingestion/model/semantic-model.js'; +import type { WorkspaceResolutionIndex } from '../../../src/core/ingestion/scope-resolution/workspace-index.js'; + +const CLASS_SCOPE_ID = 'scope:test.cs#1:1-100:1:Class' as ScopeId; +const CLASS_DEF_ID = 'def:test.cs:Foo'; + +const mkMethod = (overrides: Partial & { nodeId: string }): SymbolDefinition => ({ + nodeId: overrides.nodeId, + filePath: 'x.cs', + type: 'Method', + ...overrides, +}); + +const mkClassScope = (): Scope => + ({ + id: CLASS_SCOPE_ID, + parent: null, + kind: 'Class', + range: { startLine: 1, startCol: 1, endLine: 100, endCol: 1 }, + filePath: 'test.cs', + bindings: new Map(), + typeBindings: new Map(), + ownedDefs: [], + }) as unknown as Scope; + +const mkScopes = (scope: Scope): ScopeResolutionIndexes => + ({ + scopeTree: { + getScope: (id: ScopeId) => (id === scope.id ? scope : undefined), + }, + }) as unknown as ScopeResolutionIndexes; + +const mkWorkspaceIndex = (mapping: ReadonlyMap): WorkspaceResolutionIndex => + ({ + classScopeIdToDefId: mapping, + }) as unknown as WorkspaceResolutionIndex; + +const mkModel = ( + overloadsByName: ReadonlyMap, +): SemanticModel => + ({ + methods: { + lookupAllByOwner: (_classDefId: string, name: string) => + overloadsByName.get(name) ?? ([] as readonly SymbolDefinition[]), + }, + }) as unknown as SemanticModel; + +describe('pickImplicitThisOverload — uniqueness guard (Codex #1497 finding 2)', () => { + const site = { + inScope: CLASS_SCOPE_ID, + name: 'save', + arity: 1, + argumentTypes: undefined, + }; + + it('returns the sole overload when only one method exists on the owner', () => { + const sole = mkMethod({ nodeId: 'm:1', parameterCount: 1, requiredParameterCount: 1 }); + const scopes = mkScopes(mkClassScope()); + const workspace = mkWorkspaceIndex(new Map([[CLASS_SCOPE_ID, CLASS_DEF_ID]])); + const model = mkModel(new Map([['save', [sole]]])); + + const result = pickImplicitThisOverload(site, scopes, workspace, model); + + expect(result?.nodeId).toBe('m:1'); + }); + + it('returns the single survivor when narrowing disambiguates by arity', () => { + const save1 = mkMethod({ nodeId: 'm:1', parameterCount: 1, requiredParameterCount: 1 }); + const save2 = mkMethod({ nodeId: 'm:2', parameterCount: 2, requiredParameterCount: 2 }); + const scopes = mkScopes(mkClassScope()); + const workspace = mkWorkspaceIndex(new Map([[CLASS_SCOPE_ID, CLASS_DEF_ID]])); + const model = mkModel(new Map([['save', [save1, save2]]])); + + // site.arity = 1 → only save1 survives narrowing. + const result = pickImplicitThisOverload(site, scopes, workspace, model); + + expect(result?.nodeId).toBe('m:1'); + }); + + it('returns undefined when narrowing leaves two compatible candidates (the bug)', () => { + // Two same-arity, same-required-count overloads with no disambiguating + // parameter-type info on either def. `narrowOverloadCandidates` keeps + // both; pre-fix code returned `candidates[0]` (registration order); + // post-fix code returns undefined. + const save1 = mkMethod({ nodeId: 'm:1', parameterCount: 1, requiredParameterCount: 1 }); + const save2 = mkMethod({ nodeId: 'm:2', parameterCount: 1, requiredParameterCount: 1 }); + const scopes = mkScopes(mkClassScope()); + const workspace = mkWorkspaceIndex(new Map([[CLASS_SCOPE_ID, CLASS_DEF_ID]])); + const model = mkModel(new Map([['save', [save1, save2]]])); + + const result = pickImplicitThisOverload(site, scopes, workspace, model); + + expect(result).toBeUndefined(); + }); + + it('returns undefined when no method on the owner matches the call name', () => { + const scopes = mkScopes(mkClassScope()); + const workspace = mkWorkspaceIndex(new Map([[CLASS_SCOPE_ID, CLASS_DEF_ID]])); + const model = mkModel(new Map()); + + const result = pickImplicitThisOverload(site, scopes, workspace, model); + + expect(result).toBeUndefined(); + }); + + it('returns undefined when the call site is not inside a Class scope', () => { + // Module-scope sites: no enclosing class, so the implicit-this picker + // has nothing to pick from. Different from an empty-narrowing miss. + const moduleScope = { + id: 'scope:test.cs#1:1-100:1:Module' as ScopeId, + parent: null, + kind: 'Module', + range: { startLine: 1, startCol: 1, endLine: 100, endCol: 1 }, + filePath: 'test.cs', + bindings: new Map(), + typeBindings: new Map(), + ownedDefs: [], + } as unknown as Scope; + const scopes = mkScopes(moduleScope); + const workspace = mkWorkspaceIndex(new Map()); + const model = mkModel(new Map()); + + const result = pickImplicitThisOverload( + { ...site, inScope: moduleScope.id }, + scopes, + workspace, + model, + ); + + expect(result).toBeUndefined(); + }); +});