diff --git a/src/domain/graph/builder/call-resolver.ts b/src/domain/graph/builder/call-resolver.ts index 68a5a258..dde9fb23 100644 --- a/src/domain/graph/builder/call-resolver.ts +++ b/src/domain/graph/builder/call-resolver.ts @@ -14,6 +14,7 @@ import { isModuleScopedLanguage, resolveByGlobal, resolveByReceiver, + unwrapTypeEntry, } from '../resolver/strategy.js'; // ── Public interface ───────────────────────────────────────────────────── @@ -60,6 +61,49 @@ export function resolveSameClassQualifiedMethod( .filter((n) => n.kind === 'method'); } +/** + * Shared by both the full-build (build-edges.ts, including its native-engine + * post-pass) and incremental (incremental.ts) `Object.defineProperty` accessor + * fallback: when a function is registered as a getter/setter via + * `Object.defineProperty(obj, "bar", { get: getter })`, calls to `this.X()` + * inside `getter` resolve against `obj` (this === obj when the accessor is + * invoked). + * + * `definePropertyReceivers` maps the getter/setter's own name (`callerName`) + * to the receiver variable name (`obj`). Resolution: + * 1. Look up `obj`'s type in the typeMap and try the qualified `Type.X` + * method in the same file. + * 2. Otherwise, fall back to any same-file definition named `X` — handles + * plain object literals where the method isn't qualified (e.g. + * `const obj = { baz() {} }` defines `baz` directly). + * + * The fallback tier (2) is restricted to `function`/`method` kinds: a + * getter/setter's implementation is always callable code, so an unfiltered + * lookup could otherwise match an unrelated same-named class or variable in + * the same file (issue #1766). Tier (1) is intentionally left unfiltered, + * matching its pre-existing behaviour on all three call sites. + */ +export function resolveDefinePropertyAccessorTarget( + callName: string, + callerName: string, + relPath: string, + typeMap: Map, + lookup: CallNodeLookup, + definePropertyReceivers: ReadonlyMap, +): Array<{ id: number; file: string; kind?: string }> { + const receiverVarName = definePropertyReceivers.get(callerName); + if (!receiverVarName) return []; + + const typeName = unwrapTypeEntry(typeMap.get(receiverVarName)); + if (typeName) { + const qualified = lookup.byNameAndFile(`${typeName}.${callName}`, relPath); + if (qualified.length > 0) return [...qualified]; + } + return lookup + .byNameAndFile(callName, relPath) + .filter((n) => n.kind === 'function' || n.kind === 'method'); +} + // ── Shared resolution functions ────────────────────────────────────────── /** diff --git a/src/domain/graph/builder/incremental.ts b/src/domain/graph/builder/incremental.ts index 809fa7b8..8da7d4ab 100644 --- a/src/domain/graph/builder/incremental.ts +++ b/src/domain/graph/builder/incremental.ts @@ -26,6 +26,7 @@ import { findCaller, isModuleScopedLanguage, resolveCallTargets, + resolveDefinePropertyAccessorTarget, resolveReceiverEdge, resolveSameClassQualifiedMethod, } from './call-resolver.js'; @@ -607,42 +608,6 @@ function buildIncrementalTypeMap(symbols: ExtractorOutput): Map return typeMap; } -/** - * Strategy 2 — Object.defineProperty accessor fallback. - * When a function is registered as a getter/setter via - * `Object.defineProperty(obj, "bar", { get: getter })`, calls to `this.X()` - * inside `getter` resolve against `obj`. Looks up the receiver var in the - * typeMap for its type, then falls back to any same-file definition named - * `callName` with function or method kind. - */ -function resolveDefinePropertyTarget( - callName: string, - callerName: string, - relPath: string, - typeMap: Map, - lookup: CallNodeLookup, - definePropertyReceivers: Map, -): Array<{ id: number; file: string; kind?: string }> { - const receiverVarName = definePropertyReceivers.get(callerName); - if (!receiverVarName) return []; - - const typeEntry = typeMap.get(receiverVarName); - const typeName = typeEntry - ? typeof typeEntry === 'string' - ? typeEntry - : (typeEntry as { type?: string }).type - : null; - if (typeName) { - const qualified = lookup.byNameAndFile(`${typeName}.${callName}`, relPath); - if (qualified.length > 0) return [...qualified]; - } - // Narrow to function/method kinds only to avoid matching unrelated - // variables or classes that share a name in the same file. - return lookup - .byNameAndFile(callName, relPath) - .filter((n) => n.kind === 'function' || n.kind === 'method'); -} - /** * Apply fallback resolution strategies for a single call site when the * primary resolveCallTargets pass returned no targets. @@ -685,9 +650,11 @@ function applyCallFallbacks( if (s2.length > 0) return s2; } - // Strategy 3: Object.defineProperty accessor fallback. + // Strategy 3: Object.defineProperty accessor fallback. Shared with the + // full-build path (stages/build-edges.ts) via call-resolver.ts so both + // paths apply the same function/method kind filter (issue #1766). if (call.receiver === 'this' && callerName != null && definePropertyReceivers) { - return resolveDefinePropertyTarget( + return resolveDefinePropertyAccessorTarget( call.name, callerName, relPath, diff --git a/src/domain/graph/builder/stages/build-edges.ts b/src/domain/graph/builder/stages/build-edges.ts index d12abfc4..fc11f82c 100644 --- a/src/domain/graph/builder/stages/build-edges.ts +++ b/src/domain/graph/builder/stages/build-edges.ts @@ -43,6 +43,7 @@ import { findCaller, isModuleScopedLanguage, resolveCallTargets, + resolveDefinePropertyAccessorTarget, resolveReceiverEdge, resolveSameClassQualifiedMethod, } from '../call-resolver.js'; @@ -712,22 +713,16 @@ function buildDefinePropertyPostPass( ); if (directTargets.length > 0) continue; - // Resolve via receiver type - let targets: ReadonlyArray<{ id: number; file: string }> = []; - const typeEntry = typeMap.get(receiverVarName); - const typeName = typeEntry - ? typeof typeEntry === 'string' - ? typeEntry - : (typeEntry as { type?: string }).type - : null; - if (typeName) { - const qualifiedName = `${typeName}.${call.name}`; - targets = lookup.byNameAndFile(qualifiedName, relPath); - } - // Same-file fallback for plain object-literal methods - if (targets.length === 0) { - targets = lookup.byNameAndFile(call.name, relPath); - } + // Resolve via receiver type, restricted to function/method kinds + // (shared with the WASM-path fallback and incremental.ts — issue #1766). + const targets = resolveDefinePropertyAccessorTarget( + call.name, + caller.callerName, + relPath, + typeMap as Map, + lookup, + definePropertyReceivers, + ); for (const t of targets) { const edgeKey = `${caller.id}|${t.id}`; @@ -1259,6 +1254,11 @@ function resolveReflectionKeyExprFallback( * nothing, try treating `obj` as the receiver and look up `obj.X` in the * typeMap, or fall back to a same-file lookup of any definition named X * that belongs to the object literal or its type. + * + * Checks applicability (this-receiver + known caller + a receiver map to + * consult) then delegates the actual resolution to the shared + * `resolveDefinePropertyAccessorTarget` (call-resolver.ts), which is also + * used by the native-engine post-pass below and by incremental.ts. */ function resolveDefinePropertyAccessorFallback( call: Call, @@ -1269,23 +1269,14 @@ function resolveDefinePropertyAccessorFallback( definePropertyReceivers: Map | undefined, ): Array<{ id: number; file: string; kind?: string }> { if (call.receiver !== 'this' || callerName == null || !definePropertyReceivers) return []; - const receiverVarName = definePropertyReceivers.get(callerName); - if (!receiverVarName) return []; - - const typeName = unwrapTypeEntry(typeMap.get(receiverVarName)); - if (typeName) { - const qualified = lookup.byNameAndFile(`${typeName}.${call.name}`, relPath); - if (qualified.length > 0) return [...qualified]; - } - // If still no targets, search for any definition named `call.name` in - // the same file — handles plain object literals where the method isn't - // qualified (e.g. `const obj = { baz() {} }` defines `baz` directly). - // Note: this is intentionally broad — it matches any same-file definition - // with the called name, not just members of the receiver object. This is - // the same behaviour used by the native post-pass path (buildDefinePropertyPostPass). - const sameFile = lookup.byNameAndFile(call.name, relPath); - if (sameFile.length > 0) return [...sameFile]; - return []; + return resolveDefinePropertyAccessorTarget( + call.name, + callerName, + relPath, + typeMap as Map, + lookup, + definePropertyReceivers, + ); } /** diff --git a/tests/integration/issue-1766-defineproperty-kind-filter-parity.test.ts b/tests/integration/issue-1766-defineproperty-kind-filter-parity.test.ts new file mode 100644 index 00000000..e429f6a7 --- /dev/null +++ b/tests/integration/issue-1766-defineproperty-kind-filter-parity.test.ts @@ -0,0 +1,192 @@ +/** + * Regression test for #1766: the full-build `Object.defineProperty` accessor + * fallback (`resolveDefinePropertyAccessorFallback` in + * `stages/build-edges.ts`, plus its native-engine post-pass + * `buildDefinePropertyPostPass`) and the incremental rebuild path's fallback + * (`applyCallFallbacks` in `incremental.ts`) used to diverge in their final + * same-file fallback tier: full-build returned ANY same-file node named + * `call.name` (unfiltered by kind — could match an unrelated class or + * variable), while incremental filtered to `function`/`method` kinds only. + * + * Both paths now share a single implementation + * (`resolveDefinePropertyAccessorTarget` in `call-resolver.ts`), so a full + * build and an incremental single-file rebuild of the same source must + * produce identical `calls` edges for this pattern. + * + * See `tests/unit/call-resolver.test.ts` ("resolveDefinePropertyAccessorTarget + * — kind filter parity (#1766)") for direct unit coverage of the kind filter + * itself (an unrelated same-named class/variable in the same file must never + * win over the actual function/method). That divergence is only observable + * by calling the fallback function directly: in a full pipeline run, a + * same-file name collision on the *bare* call name is normally already + * resolved — correctly or not, see #1888 (a separate, pre-existing, + * already-shared primary-resolution bug, confirmed identical on both + * engines) — by `resolveCallTargets`'s own unqualified lookup before this + * fallback tier is ever reached. This integration test instead locks in + * end-to-end parity for the overall accessor-fallback feature across + * full-build vs incremental after the consolidation. (Native-engine full + * builds reach this via a different mechanism than the WASM path — Rust's + * own composite-pts-key resolution, not the `definePropertyReceivers` path + * this issue is about — since #1887 tracks that the native orchestrator's + * fast path never runs the JS `definePropertyReceivers` post-pass at all.) + * + * Mirrors the full-build-vs-incremental-rebuild comparison pattern from + * `issue-1765-incremental-same-class-barecall.test.ts`. + */ +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 { getNodeId as getNodeIdQuery, initSchema, openDb } from '../../src/db/index.js'; +import { rebuildFile } from '../../src/domain/graph/builder/incremental.js'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; +import type { EngineMode } from '../../src/types.js'; + +function writeFixture(dir: string, marker: string): void { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'accessor.js'), + `// ${marker} +// Object.defineProperty accessor this-dispatch: getter is registered as a +// get accessor for accessorTarget, so \`this\` inside getter refers to +// accessorTarget. this.baz() -> accessorTarget.baz -> baz. +function baz() { + return 42; +} + +const accessorTarget = { baz }; + +function getter() { + this.baz(); +} + +Object.defineProperty(accessorTarget, 'computed', { get: getter }); + +export function useAccessor() { + return accessorTarget.computed; +} +`, + ); +} + +interface CallEdgeRow { + src: string; + srcKind: string; + tgt: string; + tgtKind: string; + kind: 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, e.kind + 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 makeStmts(db: ReturnType) { + return { + insertNode: db.prepare( + 'INSERT OR IGNORE INTO nodes (name, kind, file, line, end_line) VALUES (?, ?, ?, ?, ?)', + ), + getNodeId: { + get: (name: string, kind: string, file: string, line: number) => { + const id = getNodeIdQuery(db, name, kind, file, line); + return id != null ? { id } : undefined; + }, + }, + insertEdge: db.prepare( + 'INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?, ?, ?, ?, ?)', + ), + countNodes: db.prepare('SELECT COUNT(*) as c FROM nodes WHERE file = ?'), + countEdges: db.prepare( + 'SELECT COUNT(*) as c FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?)', + ), + findNodeInFile: db.prepare( + "SELECT id, kind, file FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant') AND file = ?", + ), + findNodeByName: db.prepare( + "SELECT id, file, kind FROM nodes WHERE name = ? AND kind IN ('function', 'method', 'class', 'interface', 'type', 'struct', 'enum', 'trait', 'record', 'module', 'constant')", + ), + listSymbols: db.prepare("SELECT name, kind, line FROM nodes WHERE file = ? AND kind != 'file'"), + upsertFileHash: db.prepare( + 'INSERT OR REPLACE INTO file_hashes (file, hash, mtime, size) VALUES (?, ?, ?, ?)', + ), + deleteFileHash: db.prepare('DELETE FROM file_hashes WHERE file = ?'), + }; +} + +function runWatchScenario(engine: EngineMode): void { + describe(`codegraph watch (rebuildFile): Object.defineProperty accessor fallback matches full rebuild (#1766) — ${engine}`, () => { + let projDir: string; + let refDir: string; + + beforeAll(async () => { + projDir = fs.mkdtempSync(path.join(os.tmpdir(), `cg-1766-watch-${engine}-`)); + writeFixture(projDir, 'v1'); + await buildGraph(projDir, { engine, incremental: false, skipRegistry: true }); + + // Edit accessor.js, then rebuild ONLY it via the watcher's single-file + // incremental path (the exact code path under test). + writeFixture(projDir, 'v2 edited'); + const dbPath = path.join(projDir, '.codegraph', 'graph.db'); + const db = openDb(dbPath); + try { + initSchema(db); + const stmts = makeStmts(db); + await rebuildFile(db, projDir, path.join(projDir, 'accessor.js'), stmts, { engine }, null); + } finally { + db.close(); + } + + refDir = fs.mkdtempSync(path.join(os.tmpdir(), `cg-1766-watch-ref-${engine}-`)); + writeFixture(refDir, 'v2 edited'); + await buildGraph(refDir, { engine, incremental: false, skipRegistry: true }); + }, 60_000); + + afterAll(() => { + fs.rmSync(projDir, { recursive: true, force: true }); + fs.rmSync(refDir, { recursive: true, force: true }); + }); + + it('resolves the accessor this-dispatch edge to the function, matching a full rebuild', () => { + const incremental = readCallEdges(path.join(projDir, '.codegraph', 'graph.db')); + const reference = readCallEdges(path.join(refDir, '.codegraph', 'graph.db')); + + expect(reference).toContainEqual({ + src: 'getter', + srcKind: 'function', + tgt: 'baz', + tgtKind: 'function', + kind: 'calls', + }); + + expect(incremental).toEqual(reference); + expect(incremental).toContainEqual({ + src: 'getter', + srcKind: 'function', + tgt: 'baz', + tgtKind: 'function', + kind: 'calls', + }); + }); + }); +} + +runWatchScenario('wasm'); +describe.skipIf(!isNativeAvailable())('native engine coverage', () => { + runWatchScenario('native'); +}); diff --git a/tests/unit/call-resolver.test.ts b/tests/unit/call-resolver.test.ts index 2c4d5daa..a50a1045 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, + resolveDefinePropertyAccessorTarget, resolveReceiverEdge, } from '../../src/domain/graph/builder/call-resolver.js'; @@ -288,3 +289,102 @@ describe('resolveReceiverEdge — local function constructor blocks global class expect(result?.receiverId).toBe(2); }); }); + +// ── resolveDefinePropertyAccessorTarget ─────────────────────────────────── + +/** + * Regression tests for #1766: the full-build path's `Object.defineProperty` + * accessor fallback (`resolveDefinePropertyAccessorFallback` in + * stages/build-edges.ts, plus its native-engine post-pass) and the incremental + * path's (`applyCallFallbacks` in incremental.ts) used to diverge in their + * final same-file fallback tier: full-build returned ANY same-file node named + * `call.name` (unfiltered by kind), while incremental filtered to + * function/method kinds only. Both paths now share this single function. + * + * A getter/setter registered via Object.defineProperty always dispatches to + * callable code — never a class or variable — so an unrelated same-named + * class or variable declared in the same file must never win. (In a full + * pipeline run this same-file collision is normally already caught upstream + * by resolveCallTargets's own unqualified lookup before this fallback is + * ever reached; these tests exercise the fallback directly to pin down its + * standalone kind-filtering contract regardless of caller.) + */ +describe('resolveDefinePropertyAccessorTarget — kind filter parity (#1766)', () => { + const receivers = new Map([['getter', 'obj']]); + + it('resolves to the function, ignoring an unrelated same-named class in the same file', () => { + const fn = { id: 1, file: 'a.js', kind: 'function' }; + const unrelatedClass = { id: 2, file: 'a.js', kind: 'class' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedClass, fn] }, {}); + const result = resolveDefinePropertyAccessorTarget( + 'bar', + 'getter', + 'a.js', + new Map(), // no typeMap entry for 'obj' — plain object literal, not a typed instance + lookup, + receivers, + ); + expect(result).toEqual([fn]); + }); + + it('resolves to a method, ignoring an unrelated same-named variable in the same file', () => { + const method = { id: 3, file: 'a.js', kind: 'method' }; + const unrelatedVar = { id: 4, file: 'a.js', kind: 'variable' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedVar, method] }, {}); + const result = resolveDefinePropertyAccessorTarget( + 'bar', + 'getter', + 'a.js', + new Map(), + lookup, + receivers, + ); + expect(result).toEqual([method]); + }); + + it('returns nothing when only a same-named class/variable exists (no function/method)', () => { + const unrelatedClass = { id: 5, file: 'a.js', kind: 'class' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [unrelatedClass] }, {}); + const result = resolveDefinePropertyAccessorTarget( + 'bar', + 'getter', + 'a.js', + new Map(), + lookup, + receivers, + ); + expect(result).toEqual([]); + }); + + it('returns [] when callerName has no entry in definePropertyReceivers', () => { + const fn = { id: 6, file: 'a.js', kind: 'function' }; + const lookup = makeReceiverLookup({ 'bar:a.js': [fn] }, {}); + const result = resolveDefinePropertyAccessorTarget( + 'bar', + 'unregisteredGetter', + 'a.js', + new Map(), + lookup, + receivers, + ); + expect(result).toEqual([]); + }); + + it('prefers the typeName-qualified method when the receiver has a resolvable type', () => { + const qualifiedMethod = { id: 7, file: 'a.js', kind: 'method' }; + const bareFn = { id: 8, file: 'a.js', kind: 'function' }; + const lookup = makeReceiverLookup( + { 'Registry.bar:a.js': [qualifiedMethod], 'bar:a.js': [bareFn] }, + {}, + ); + const result = resolveDefinePropertyAccessorTarget( + 'bar', + 'getter', + 'a.js', + new Map([['obj', 'Registry']]), + lookup, + receivers, + ); + expect(result).toEqual([qualifiedMethod]); + }); +});