mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(scope-resolution): require unique narrowing in pickImplicitThisOverload
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)
This commit is contained in:
parent
4ffe5449ac
commit
53d1b7ba2b
2 changed files with 173 additions and 4 deletions
|
|
@ -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];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<SymbolDefinition> & { 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<ScopeId, string>): WorkspaceResolutionIndex =>
|
||||
({
|
||||
classScopeIdToDefId: mapping,
|
||||
}) as unknown as WorkspaceResolutionIndex;
|
||||
|
||||
const mkModel = (
|
||||
overloadsByName: ReadonlyMap<string, readonly SymbolDefinition[]>,
|
||||
): 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();
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue