diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs index 74b4d650..4131ca9d 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs @@ -947,14 +947,26 @@ fn find_enclosing_caller<'a>(defs: &[DefWithId<'a>], call_line: u32, file_node_i /// confidence (e.g. several files at the same directory depth from the /// caller) is genuinely ambiguous and returns nothing, letting the caller /// fall through to the narrower same-class-sibling fallback. +/// +/// A `this`/`self`/`super` call is additionally restricted to callable kinds +/// (`is_callable_kind`): such a call is logically "invoke a member of the +/// current instance", which a class/interface/struct/etc. declaration can +/// never satisfy, so an unrelated same-named type declaration must never +/// substitute for a real callable target just because no other candidate +/// exists (#1888). A genuinely bare call (no receiver at all) is left +/// unfiltered — at this layer it is indistinguishable from a `new +/// ClassName()` constructor invocation, which legitimately targets a +/// class-kind definition. fn resolve_exact_global_match<'a>( ctx: &EdgeContext<'a>, call_name: &str, rel_path: &str, + receiver: Option<&str>, ) -> Vec<&'a NodeInfo> { let scored: Vec<(&'a NodeInfo, f64)> = ctx.nodes_by_name .get(call_name) .map(|v| v.iter() + .filter(|&&n| receiver.is_none() || is_callable_kind(&n.kind)) .map(|&n| (n, resolve::compute_confidence(rel_path, &n.file, None))) .filter(|&(_, confidence)| confidence >= 0.5) .collect()) @@ -1024,10 +1036,27 @@ fn resolve_call_targets<'a>( if !pre_qualified.is_empty() { return pre_qualified; } } - // 2. Same-file resolution - let targets = ctx.nodes_by_name_and_file + // 2. Same-file resolution. A receiver — concrete (`obj.x()`) or + // `this`/`self`/`super` — means the call is logically "invoke a member of + // some instance", which a class/interface/struct/etc. declaration can + // never satisfy; restrict those to definitively callable kinds + // (`is_callable_kind`) so an unrelated same-file type declaration that + // merely shares the call's name can never pre-empt a legitimate target + // that a more specific resolution tier (receiver typing, the + // Object.defineProperty accessor fallback, etc.) would otherwise find. A + // genuinely bare call (no receiver at all) is left unfiltered: at this + // layer it is indistinguishable from a `new ClassName()` constructor + // invocation, which legitimately targets a class-kind definition — + // kind-filtering it would break constructor-call resolution (#1888). + // Mirrors resolveCallTargets in call-resolver.ts. + let bare_matches = ctx.nodes_by_name_and_file .get(&(call.name.as_str(), rel_path)) .cloned().unwrap_or_default(); + let targets: Vec<&NodeInfo> = if call.receiver.is_some() { + bare_matches.into_iter().filter(|n| is_callable_kind(&n.kind)).collect() + } else { + bare_matches + }; if !targets.is_empty() { return targets; } // 3. Type-aware resolution via receiver → type map. @@ -1212,7 +1241,7 @@ fn resolve_call_targets<'a>( } // First try exact name match (e.g. an unqualified function named "area"). - let exact = resolve_exact_global_match(ctx, call.name.as_str(), rel_path); + let exact = resolve_exact_global_match(ctx, call.name.as_str(), rel_path, call.receiver.as_deref()); if !exact.is_empty() { return exact; } // Class-scoped exact lookup: prefer `ClassName.method` when the caller is a qualified diff --git a/src/domain/graph/builder/call-resolver.ts b/src/domain/graph/builder/call-resolver.ts index 674b20f6..d6861772 100644 --- a/src/domain/graph/builder/call-resolver.ts +++ b/src/domain/graph/builder/call-resolver.ts @@ -9,6 +9,7 @@ * `resolveByMethodOrGlobal` delegates its two branches to strategy helpers * in `../resolver/strategy.ts` to keep per-strategy complexity manageable. */ +import { CALLABLE_SYMBOL_KINDS } from '../../../shared/kinds.js'; import { computeConfidence, isSameLanguageFamily } from '../resolve.js'; import { isModuleScopedLanguage, @@ -113,14 +114,6 @@ export function resolveDefinePropertyAccessorTarget( // ── Shared resolution functions ────────────────────────────────────────── -/** - * Callable definition kinds — variable/constant bindings are NOT callable - * in the function-as-enclosing-scope sense (they are local declarations, not - * function bodies). Top-level variable bindings (e.g. Haskell `main = do …`) - * are handled separately as a fallback tier. - */ -const CALLABLE_KINDS = new Set(['function', 'method']); - /** * Variable-like binding kinds that may act as top-level callers when no * enclosing function/method exists (e.g. Haskell top-level `main` is a @@ -145,7 +138,7 @@ function findEnclosingCallable( let best: CallerMatch = null; let bestSpan = Infinity; for (const def of definitions) { - if (!CALLABLE_KINDS.has(def.kind)) continue; + if (!CALLABLE_SYMBOL_KINDS.has(def.kind)) continue; if (def.line > callLine) continue; const end = def.endLine ?? Infinity; if (callLine > end) continue; @@ -300,7 +293,23 @@ export function resolveCallTargets( } if (!targets || targets.length === 0) { - targets = lookup.byNameAndFile(call.name, relPath); + // Same-file bare-name lookup. A receiver — concrete (`obj.x()`) or + // `this`/`self`/`super` — means the call is logically "invoke a member of + // some instance", which a class/interface/struct/etc. declaration can + // never satisfy; restrict those to definitively callable kinds so an + // unrelated same-file type declaration that merely shares the call's name + // can never pre-empt a legitimate target that a more specific resolution + // tier (receiver typing, the Object.defineProperty accessor fallback, + // etc.) would otherwise find. A genuinely bare call (no receiver at all) + // is left unfiltered: at this layer it is indistinguishable from a `new + // ClassName()` constructor invocation, which legitimately targets a + // class-kind definition — kind-filtering it would break constructor-call + // resolution (#1888). + const bareMatches = lookup.byNameAndFile(call.name, relPath); + targets = call.receiver + ? bareMatches.filter((n) => CALLABLE_SYMBOL_KINDS.has(n.kind ?? '')) + : bareMatches; + if (targets.length === 0) { targets = resolveByMethodOrGlobal( lookup, diff --git a/src/domain/graph/resolver/strategy.ts b/src/domain/graph/resolver/strategy.ts index 4648575e..2d6a09f4 100644 --- a/src/domain/graph/resolver/strategy.ts +++ b/src/domain/graph/resolver/strategy.ts @@ -12,6 +12,7 @@ * a circular dependency. The StrategyLookup interface mirrors CallNodeLookup structurally * (TypeScript structural typing ensures compatibility without an explicit import). */ +import { CALLABLE_SYMBOL_KINDS } from '../../../shared/kinds.js'; import { computeConfidence } from '../resolve.js'; // ── Lookup adapter (structural mirror of CallNodeLookup) ────────────────────── @@ -357,14 +358,25 @@ function resolveViaSameClassSibling( * ambiguous: there is no principled way to prefer one over another, so * the match is dropped rather than fanning out, letting the caller fall * through to the narrower resolveViaSameClassSibling fallback. + * + * A `this`/`self`/`super` call is additionally restricted to + * `CALLABLE_SYMBOL_KINDS`: such a call is logically "invoke a member of the + * current instance", which a class/interface/struct/etc. declaration can + * never satisfy, so an unrelated same-named type declaration must never + * substitute for a real callable target just because no other candidate + * exists (#1888). A genuinely bare call (no receiver at all) is left + * unfiltered — at this layer it is indistinguishable from a `new + * ClassName()` constructor invocation, which legitimately targets a + * class-kind definition. */ function resolveExactGlobalMatch( lookup: StrategyLookup, - call: { name: string }, + call: { name: string; receiver?: string | null }, relPath: string, ): ReadonlyArray<{ id: number; file: string }> { const scored = lookup .byName(call.name) + .filter((target) => !call.receiver || CALLABLE_SYMBOL_KINDS.has(target.kind ?? '')) .map((target) => ({ target, confidence: computeConfidence(relPath, target.file, null) })) .filter((candidate) => candidate.confidence >= 0.5); if (scored.length === 0) return []; diff --git a/src/shared/kinds.ts b/src/shared/kinds.ts index 0eb86b38..1b644659 100644 --- a/src/shared/kinds.ts +++ b/src/shared/kinds.ts @@ -42,6 +42,21 @@ export const EVERY_SYMBOL_KIND: readonly SymbolKind[] = [ ...EXTENDED_SYMBOL_KINDS, ]; +/** + * Symbol kinds that represent an actual invocable definition. A call site can + * legitimately target one of these — a class, interface, struct, or plain + * variable/constant binding cannot, in the general case, and must never win a + * same-name lookup that has no other type/receiver information to narrow it. + * + * Guards the "no other signal" tiers of call resolution — the same-file + * bare-name lookup in `resolveCallTargets` and `resolveByGlobal`'s exact + * global-name match (`call-resolver.ts`, `resolver/strategy.ts`) — so an + * unrelated same-named class/interface/variable never masquerades as a real + * callable target purely because those lookups otherwise carry no kind filter + * (#1888). + */ +export const CALLABLE_SYMBOL_KINDS: ReadonlySet = new Set(['function', 'method']); + // Backward compat: ALL_SYMBOL_KINDS stays as the core 10 export const ALL_SYMBOL_KINDS: readonly CoreSymbolKind[] = CORE_SYMBOL_KINDS; diff --git a/tests/integration/issue-1888-same-file-bare-name-kind-filter.test.ts b/tests/integration/issue-1888-same-file-bare-name-kind-filter.test.ts new file mode 100644 index 00000000..d959e995 --- /dev/null +++ b/tests/integration/issue-1888-same-file-bare-name-kind-filter.test.ts @@ -0,0 +1,131 @@ +/** + * Regression test for #1888: the same-file bare-name lookup in + * `resolveCallTargets` (`lookup.byNameAndFile(call.name, relPath)`) ran + * unconditionally for every call and was unfiltered by symbol kind. A call + * with a receiver — `this.x()`, `obj.x()` — is logically "invoke a member of + * some instance", so an unrelated same-file class/interface/etc. that merely + * shared the call's bare name won outright, before any more specific + * resolution tier (receiver typing, the Object.defineProperty accessor + * fallback, etc.) ever got a chance to run. The exact same defect existed in + * the mirrored Rust `resolve_call_targets` / `resolve_exact_global_match` in + * `build_edges.rs` — confirmed identical on both engines before the fix. + * + * Repro (from the issue): `this.bar()` inside a plain function `getter`, + * registered as a get-accessor for `obj` (an instance of `Registry`, which + * defines `bar()`) via `Object.defineProperty`, plus an unrelated + * `class bar {}` declared later in the same file. + * + * class Registry { bar() { return 1; } } + * const obj = new Registry(); + * function getter() { this.bar(); } + * Object.defineProperty(obj, 'x', { get: getter }); + * class bar {} + * + * Before the fix: `getter -> bar` (kind: class) — the coincidentally-named + * class pre-empted the correctly-typed `Registry.bar` method, which was only + * reachable via a later, more specific fallback tier that never got a + * chance to run. + * + * After the fix: `getter -> Registry.bar` (kind: method), and the unrelated + * `bar` class no longer receives a `calls` edge at all. + * + * The `new Registry()` constructor call (`obj -> Registry`) is also asserted + * to still resolve — that call has no receiver at all, so it is + * indistinguishable, at this resolution layer, from a plain bare call; kind + * must NOT be filtered for it, or constructor-call resolution regresses. + */ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; +import type { EngineMode } from '../../src/types.js'; + +const FIXTURE = ` +class Registry { + bar() { + return 1; + } +} + +const obj = new Registry(); + +function getter() { + this.bar(); +} + +Object.defineProperty(obj, 'x', { get: getter }); + +class bar {} +`; + +interface CallEdgeRow { + src: string; + srcKind: string; + tgt: string; + tgtKind: string; +} + +function readCallEdges(dbPath: string): CallEdgeRow[] { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.name AS src, n1.kind AS srcKind, n2.name AS tgt, n2.kind AS tgtKind + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind = 'calls' + ORDER BY n1.name, n2.name`, + ) + .all() as CallEdgeRow[]; + } finally { + db.close(); + } +} + +function runScenario(engine: EngineMode): void { + describe(`same-file bare-name lookup kind filter (#1888) — ${engine}`, () => { + let dir: string; + + beforeAll(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), `cg-1888-${engine}-`)); + fs.writeFileSync(path.join(dir, 'repro.js'), FIXTURE); + await buildGraph(dir, { engine, incremental: false, skipRegistry: true }); + }, 30_000); + + afterAll(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('resolves this.bar() to Registry.bar, not the unrelated class bar', () => { + const edges = readCallEdges(path.join(dir, '.codegraph', 'graph.db')); + + expect(edges).toContainEqual({ + src: 'getter', + srcKind: 'function', + tgt: 'Registry.bar', + tgtKind: 'method', + }); + expect(edges.some((e) => e.tgt === 'bar' && e.tgtKind === 'class')).toBe(false); + }); + + it('still resolves the new Registry() constructor call (bare call, no receiver)', () => { + const edges = readCallEdges(path.join(dir, '.codegraph', 'graph.db')); + + expect(edges).toContainEqual({ + src: 'obj', + srcKind: 'constant', + tgt: 'Registry', + tgtKind: 'class', + }); + }); + }); +} + +runScenario('wasm'); +describe.skipIf(!isNativeAvailable())('native engine coverage', () => { + runScenario('native'); +}); diff --git a/tests/unit/call-resolver.test.ts b/tests/unit/call-resolver.test.ts index 6e298a5b..1747abc9 100644 --- a/tests/unit/call-resolver.test.ts +++ b/tests/unit/call-resolver.test.ts @@ -18,6 +18,7 @@ import { describe, expect, it } from 'vitest'; import type { CallNodeLookup } from '../../src/domain/graph/builder/call-resolver.js'; import { resolveByMethodOrGlobal, + resolveCallTargets, resolveDefinePropertyAccessorTarget, resolveReceiverEdge, } from '../../src/domain/graph/builder/call-resolver.js'; @@ -325,6 +326,54 @@ describe('resolveByMethodOrGlobal — resolveByGlobal exact-name fan-out (#1863) }); }); +describe('resolveByMethodOrGlobal — resolveByGlobal exact-name kind filter (#1888)', () => { + // this.bar() is logically "invoke a member of the current instance" — a + // class declaration can never satisfy that, so an unrelated same-named + // class must not win the exact-global-match tier just because no + // function/method candidate exists. + it('does not resolve this.bar() to a same-named global class', () => { + const cls = { id: 1, file: 'a.js', kind: 'class' }; + const lookup = makeLookup({ bar: [cls] }); + const result = resolveByMethodOrGlobal( + lookup, + { name: 'bar', receiver: 'this' }, + 'a.js', + new Map(), + 'getter', + ); + expect(result).toEqual([]); + }); + + it('still resolves this.bar() to a same-named global function/method', () => { + const fn = { id: 2, file: 'a.js', kind: 'function' }; + const lookup = makeLookup({ bar: [fn] }); + const result = resolveByMethodOrGlobal( + lookup, + { name: 'bar', receiver: 'this' }, + 'a.js', + new Map(), + 'getter', + ); + expect(result).toEqual([fn]); + }); + + it('still resolves a genuinely bare call (no receiver at all) to a same-named class', () => { + // A bare `Registry()` call is indistinguishable, at this layer, from a + // `new Registry()` constructor invocation — kind-filtering it would break + // constructor-call resolution, so it stays unfiltered. + const cls = { id: 3, file: 'a.js', kind: 'class' }; + const lookup = makeLookup({ Registry: [cls] }); + const result = resolveByMethodOrGlobal( + lookup, + { name: 'Registry', receiver: null }, + 'a.js', + new Map(), + null, + ); + expect(result).toEqual([cls]); + }); +}); + // ── resolveReceiverEdge ────────────────────────────────────────────────────── /** @@ -587,3 +636,86 @@ describe('resolveByMethodOrGlobal — self.field receiver parity with this.field expect(result).toEqual([tsMethod]); }); }); + +// ── resolveCallTargets ───────────────────────────────────────────────────── + +/** + * Regression tests for #1888: the same-file bare-name lookup + * (`lookup.byNameAndFile(call.name, relPath)`) in `resolveCallTargets` ran + * unconditionally for every call, unfiltered by kind. A call with a receiver + * (`this.bar()`, `obj.bar()`) is logically "invoke a member of some + * instance" — a same-file class/interface/struct/etc. that merely shares the + * call's bare name could win outright, before any more specific resolution + * tier (receiver typing, the Object.defineProperty accessor fallback, etc.) + * ever got a chance to run. + * + * The literal repro from the issue: `this.bar()` inside a function `getter` + * resolved to a same-file `class bar {}` instead of the correctly-typed + * `Registry.bar` method reachable via the Object.defineProperty accessor + * fallback (see `issue-1888-same-file-bare-name-kind-filter.test.ts` for the + * full end-to-end reproduction on both engines). + * + * A genuinely bare call (no receiver at all) must stay unfiltered: at this + * layer it is indistinguishable from a `new ClassName()` constructor + * invocation, which legitimately targets a class-kind definition. + */ +describe('resolveCallTargets — same-file bare-name lookup kind filter (#1888)', () => { + it('does not resolve this.bar() to an unrelated same-file class', () => { + const unrelatedClass = { id: 1, file: 'a.js', kind: 'class' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedClass] }, {}); + const { targets } = resolveCallTargets( + lookup, + { name: 'bar', receiver: 'this' }, + 'a.js', + new Map(), + new Map(), + 'getter', + ); + expect(targets).toEqual([]); + }); + + it('still resolves this.bar() to a same-file function/method when both exist', () => { + const unrelatedClass = { id: 1, file: 'a.js', kind: 'class' }; + const fn = { id: 2, file: 'a.js', kind: 'function' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedClass, fn] }, {}); + const { targets } = resolveCallTargets( + lookup, + { name: 'bar', receiver: 'this' }, + 'a.js', + new Map(), + new Map(), + 'getter', + ); + expect(targets).toEqual([fn]); + }); + + it('does not resolve obj.bar() to an unrelated same-file class', () => { + const unrelatedClass = { id: 3, file: 'a.js', kind: 'class' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedClass] }, {}); + const { targets } = resolveCallTargets( + lookup, + { name: 'bar', receiver: 'obj' }, + 'a.js', + new Map(), + new Map(), + null, + ); + expect(targets).toEqual([]); + }); + + it('still resolves a genuinely bare call (no receiver) to a same-file class — constructor calls', () => { + // `new Registry()` is captured as a bare call with no receiver, so it + // must still be able to resolve to the class definition. + const cls = { id: 4, file: 'a.js', kind: 'class' }; + const lookup = makeReceiverLookup({ 'Registry:a.js': [cls] }, {}); + const { targets } = resolveCallTargets( + lookup, + { name: 'Registry', receiver: undefined }, + 'a.js', + new Map(), + new Map(), + null, + ); + expect(targets).toEqual([cls]); + }); +});