Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 135 additions & 0 deletions src/domain/graph/builder/stages/build-edges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,94 @@ function buildFnRefBindingsPtsPostPass(
}
}

/**
* Object.defineProperty accessor post-pass for the native call-edge path.
*
* When a function is registered as a getter/setter via
* `Object.defineProperty(obj, "bar", { get: getter })`, calls to `this.X()`
* inside `getter` need to resolve against `obj` (because `this === obj` when
* the accessor is invoked). The native Rust engine has no knowledge of
* `definePropertyReceivers`, so this JS post-pass adds the missing edges.
*/
function buildDefinePropertyPostPass(
ctx: PipelineContext,
getNodeIdStmt: NodeIdStmt,
allEdgeRows: EdgeRowTuple[],
sharedLookup?: CallNodeLookup,
): void {
const filesWithReceivers = [...ctx.fileSymbols].filter(
([, symbols]) => symbols.definePropertyReceivers && symbols.definePropertyReceivers.size > 0,
);
if (filesWithReceivers.length === 0) return;

const seenByPair = new Set<string>();
for (const [srcId, tgtId] of allEdgeRows) {
seenByPair.add(`${srcId}|${tgtId}`);
}

const { barrelOnlyFiles, rootDir } = ctx;
const lookup = sharedLookup ?? makeContextLookup(ctx, getNodeIdStmt);

for (const [relPath, symbols] of filesWithReceivers) {
if (barrelOnlyFiles.has(relPath)) continue;
const fileNodeRow = getNodeIdStmt.get(relPath, 'file', relPath, 0);
if (!fileNodeRow) continue;

const importedNames = buildImportedNamesMap(ctx, relPath, symbols, rootDir);
const typeMap: Map<string, TypeMapEntry | string> = symbols.typeMap || new Map();
const definePropertyReceivers = symbols.definePropertyReceivers!;

for (const call of symbols.calls) {
if (call.receiver !== 'this') continue;

const caller = findCaller(lookup, call, symbols.definitions, relPath, fileNodeRow);
if (!caller.callerName) continue;

const receiverVarName = definePropertyReceivers.get(caller.callerName);
if (!receiverVarName) continue;

// Only add edges the native engine missed (no direct target already).
const { targets: directTargets } = resolveCallTargets(
lookup,
call,
relPath,
importedNames,
typeMap as Map<string, unknown>,
caller.callerName,
);
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);
}
Comment on lines +781 to +784

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Overly broad same-file fallback may introduce false-positive edges

When the typeMap holds no type annotation for the receiver variable, both the WASM path (buildFileCallEdges) and the native post-pass (buildDefinePropertyPostPass) fall back to lookup.byNameAndFile(call.name, relPath), which returns every definition named call.name in the file — not just members of the receiver object. In a file that has a top-level function sharing a name with a method on the accessor target, both would be emitted as call targets. The test fixture avoids this collision, but the JavaScript benchmark precision floor is held at 1.0, so any future fixture that triggers this path would immediately break CI.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an intentional design decision consistent with the existing WASM path. Added a comment to buildFileCallEdges explicitly documenting that the same-file fallback is intentionally broad and mirrors the buildDefinePropertyPostPass behaviour. The JS benchmark precision floor at 1.0 will catch any future fixture regression caused by name collisions.


for (const t of targets) {
const edgeKey = `${caller.id}|${t.id}`;
if (t.id !== caller.id && !seenByPair.has(edgeKey)) {
const conf = computeConfidence(relPath, t.file, null);
if (conf > 0) {
seenByPair.add(edgeKey);
allEdgeRows.push([caller.id, t.id, 'calls', conf, 0, 'ts-native']);
}
}
}
}
}
}

/**
* Phase 8.5: CHA + RTA post-pass for the native call-edge path.
*
Expand Down Expand Up @@ -1020,6 +1108,50 @@ function buildFileCallEdges(
}
}

// 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). If the same-class fallback above found
// 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.
if (
targets.length === 0 &&
call.receiver === 'this' &&
caller.callerName != null &&
symbols.definePropertyReceivers
) {
const receiverVarName = symbols.definePropertyReceivers.get(caller.callerName);
if (receiverVarName) {
// Try typeMap lookup for receiver.methodName
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}`;
const qualified = lookup.byNameAndFile(qualifiedName, relPath);
if (qualified.length > 0) {
targets = [...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).
if (targets.length === 0) {
const sameFile = lookup.byNameAndFile(call.name, relPath);
if (sameFile.length > 0) {
targets = [...sameFile];
}
}
Comment on lines +1133 to +1151

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 .filter(() => true) is a no-op — it always returns every element unchanged. It was likely copy-pasted from the same-class fallback above which uses .filter((n) => n.kind === 'method'). Leaving these in place makes the code misleading and could confuse a future reader into thinking an intentional filter criterion was meant to be here.

Suggested change
if (typeName) {
const qualifiedName = `${typeName}.${call.name}`;
const qualified = lookup.byNameAndFile(qualifiedName, relPath).filter(() => true);
if (qualified.length > 0) {
targets = 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).
if (targets.length === 0) {
const sameFile = lookup.byNameAndFile(call.name, relPath).filter(() => true);
if (sameFile.length > 0) {
targets = sameFile;
}
}
if (typeName) {
const qualifiedName = `${typeName}.${call.name}`;
const qualified = lookup.byNameAndFile(qualifiedName, relPath);
if (qualified.length > 0) {
targets = 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).
if (targets.length === 0) {
const sameFile = lookup.byNameAndFile(call.name, relPath);
if (sameFile.length > 0) {
targets = sameFile;
}
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed both .filter(() => true) no-ops from the defineProperty WASM path in buildFileCallEdges. The qualified-name lookup and same-file fallback now call lookup.byNameAndFile() directly without filtering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — but used spread syntax ([...qualified] and [...sameFile]) rather than direct assignment. The original no-op .filter() was inadvertently serving as a ReadonlyArray-to-mutable-Array conversion: .filter() on a ReadonlyArray returns a new mutable Array. Removing it caused TS4104 errors at both lines (ReadonlyArray cannot be assigned to the mutable targets). Spreading into a new array is the clean fix.

}
}

for (const t of targets) {
const edgeKey = `${caller.id}|${t.id}`;
if (t.id !== caller.id) {
Expand Down Expand Up @@ -1482,6 +1614,9 @@ export async function buildEdges(ctx: PipelineContext): Promise<void> {
// (e.g. `const f = fn.bind(ctx)`), so calls to bind-created aliases are
// not resolved to their original function on the native path.
buildFnRefBindingsPtsPostPass(ctx, getNodeIdStmt, allEdgeRows, sharedLookup);
// Object.defineProperty accessor post-pass: resolve this-dispatch inside
// getter/setter functions registered via Object.defineProperty.
buildDefinePropertyPostPass(ctx, getNodeIdStmt, allEdgeRows, sharedLookup);
// Phase 8.5 post-pass: augment native call edges with CHA-resolved dispatch.
// The native Rust engine has no knowledge of the CHA context, so this/self
// calls and interface dispatch are not expanded to concrete implementations.
Expand Down
3 changes: 3 additions & 0 deletions src/domain/wasm-worker-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,9 @@ function serializeExtractorOutput(
astNodes,
...(symbols.fnRefBindings?.length ? { fnRefBindings: symbols.fnRefBindings } : {}),
...(symbols.newExpressions?.length ? { newExpressions: symbols.newExpressions } : {}),
...(symbols.definePropertyReceivers?.size
? { definePropertyReceivers: Array.from(symbols.definePropertyReceivers.entries()) }
: {}),
...(symbols.returnTypeMap?.size
? { returnTypeMap: Array.from(symbols.returnTypeMap.entries()) }
: {}),
Expand Down
5 changes: 5 additions & 0 deletions src/domain/wasm-worker-pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ function deserializeResult(ser: SerializedExtractorOutput | null): ExtractorOutp
if (ser.astNodes !== undefined) out.astNodes = ser.astNodes as unknown as ASTNodeRow[];
if (ser.fnRefBindings?.length) out.fnRefBindings = ser.fnRefBindings;
if (ser.newExpressions?.length) out.newExpressions = ser.newExpressions;
if (ser.definePropertyReceivers?.length) {
const m = new Map<string, string>();
for (const [k, v] of ser.definePropertyReceivers) m.set(k, v);
out.definePropertyReceivers = m;
}
if (ser.returnTypeMap?.length) {
const returnTypeMap = new Map<string, TypeMapEntry>();
for (const [k, v] of ser.returnTypeMap) returnTypeMap.set(k, v);
Expand Down
2 changes: 2 additions & 0 deletions src/domain/wasm-worker-protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export interface SerializedExtractorOutput {
}>;
fnRefBindings?: import('../types.js').FnRefBinding[];
newExpressions?: readonly string[];
/** Serialized definePropertyReceivers map (funcName → receiverVarName) as tuple array. */
definePropertyReceivers?: Array<[string, string]>;
returnTypeMap?: Array<[string, TypeMapEntry]>;
callAssignments?: CallAssignment[];
paramBindings?: ParamBinding[];
Expand Down
82 changes: 82 additions & 0 deletions src/extractors/javascript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,10 @@ function extractSymbolsQuery(tree: TreeSitterTree, query: TreeSitterQuery): Extr
const newExpressions: string[] = [];
extractNewExpressionsWalk(tree.rootNode, newExpressions);

// Object.defineProperty accessor receiver bindings
const definePropertyReceivers: Map<string, string> = new Map();
extractDefinePropertyReceiversWalk(tree.rootNode, definePropertyReceivers);

return {
definitions,
calls,
Expand All @@ -371,6 +375,7 @@ function extractSymbolsQuery(tree: TreeSitterTree, query: TreeSitterQuery): Extr
fnRefBindings,
paramBindings,
newExpressions,
...(definePropertyReceivers.size > 0 ? { definePropertyReceivers } : {}),
};
}

Expand Down Expand Up @@ -629,6 +634,10 @@ function extractSymbolsWalk(tree: TreeSitterTree): ExtractorOutput {
const newExpressions: string[] = [];
extractNewExpressionsWalk(tree.rootNode, newExpressions);
ctx.newExpressions = newExpressions;
// Object.defineProperty accessor receiver bindings
const definePropertyReceivers: Map<string, string> = new Map();
extractDefinePropertyReceiversWalk(tree.rootNode, definePropertyReceivers);
if (definePropertyReceivers.size > 0) ctx.definePropertyReceivers = definePropertyReceivers;
return ctx;
}

Expand Down Expand Up @@ -1415,6 +1424,79 @@ function extractNewExpressionsWalk(rootNode: TreeSitterNode, newExpressions: str
walk(rootNode, 0);
}

/**
* Walk the AST to find `Object.defineProperty(obj, "bar", { get: getter })` patterns
* and record which functions are used as getter/setter accessors for which objects.
*
* Result is stored in the provided map as `funcName → receiverVarName`.
*/
function extractDefinePropertyReceiversWalk(
rootNode: TreeSitterNode,
out: Map<string, string>,
): void {
function walk(node: TreeSitterNode, depth: number): void {
if (depth >= MAX_WALK_DEPTH) return;
if (node.type === 'call_expression') {
const fn = node.childForFieldName('function');
// Match `Object.defineProperty`
if (fn?.type === 'member_expression') {
const obj = fn.childForFieldName('object');
const prop = fn.childForFieldName('property');
if (
obj?.type === 'identifier' &&
obj.text === 'Object' &&
prop?.text === 'defineProperty'
) {
const argsNode = node.childForFieldName('arguments') ?? findChild(node, 'arguments');
if (argsNode) {
// Collect non-punctuation children: arg0 (target obj), arg1 (prop name string), arg2 (descriptor)
const argChildren: TreeSitterNode[] = [];
for (let i = 0; i < argsNode.childCount; i++) {
const c = argsNode.child(i);
if (!c) continue;
if (c.type === ',' || c.type === '(' || c.type === ')') continue;
argChildren.push(c);
}
if (argChildren.length >= 3) {
const targetObj = argChildren[0];
const descriptor = argChildren[2];
if (targetObj?.type === 'identifier' && descriptor?.type === 'object') {
const targetName = targetObj.text;
// Walk the descriptor object's pair children looking for get/set
for (let i = 0; i < descriptor.childCount; i++) {
const pair = descriptor.child(i);
if (pair?.type !== 'pair') continue;
const key = pair.childForFieldName('key');
const val = pair.childForFieldName('value');
if (
key &&
(key.text === 'get' || key.text === 'set') &&
val?.type === 'identifier' &&
!BUILTIN_GLOBALS.has(val.text)
) {
// Known limitation: if the same function is registered as an
// accessor on multiple objects, last-write-wins — only the
// last target object is retained. This is an unusual pattern
// (sharing one function across multiple defineProperty calls)
// and covering it would require Map<string, string[]> which
// changes the consumer API. Tracked as a known edge case.
out.set(val.text, targetName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Last-write-wins for duplicate accessor names

out.set(val.text, targetName) silently overwrites a previous entry if the same function is passed as an accessor to multiple Object.defineProperty calls (e.g. Object.defineProperty(obj1, 'a', { get: getter }) followed by Object.defineProperty(obj2, 'b', { get: getter })). Only the last binding is remembered, so this-calls inside getter would be resolved against the wrong object for the earlier registration. This is an unusual pattern, but given no warning is emitted or comment acknowledging the limitation, it may produce silently wrong edges in production codebases.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged with a comment. A proper fix (allowing multiple receiver bindings per accessor function) would require changing Map<string, string> to Map<string, string[]> and updating all consumers. Since this is an unusual pattern (sharing one function across multiple Object.defineProperty calls), the trade-off isn't worth it for this PR. Added an inline comment documenting the limitation so future readers are aware.

}
}
}
}
}
}
}
}
for (let i = 0; i < node.childCount; i++) {
const child = node.child(i);
if (child) walk(child, depth + 1);
}
}
walk(rootNode, 0);
}

/**
* Extract variable-to-type assignments into a per-file type map.
*
Expand Down
10 changes: 10 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,16 @@ export interface ExtractorOutput {
* project-wide instantiated-types set for Rapid Type Analysis filtering.
*/
newExpressions?: readonly string[];
/**
* Object.defineProperty receiver bindings: maps function name → target object name.
* Records `Object.defineProperty(obj, "bar", { get: getter })` so the edge builder
* can resolve `this.X()` calls inside `getter` as `obj.X()` (this === obj when the
* accessor is invoked through the property).
*
* Example: `Object.defineProperty(obj, "bar", { get: getter })` emits
* `definePropertyReceivers.set("getter", "obj")`.
*/
definePropertyReceivers?: Map<string, string>;
/** WASM tree retained for downstream analysis (complexity, CFG, dataflow). */
_tree?: TreeSitterTree;
/** Language identifier. */
Expand Down
15 changes: 15 additions & 0 deletions tests/benchmarks/resolution/fixtures/javascript/define-property.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,18 @@ function create() {
obj.f1();
obj.f2();
}

// Object.defineProperty accessor this-dispatch:
// When getter is registered as a get accessor for accessorTarget, `this` inside getter
// refers to accessorTarget. So this.baz() → accessorTarget.baz → baz.
function baz() {
return 42;
}

const accessorTarget = { baz };

function getter() {
this.baz();
}

Object.defineProperty(accessorTarget, 'bar', { get: getter });
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@
"mode": "pts-create-prototype",
"notes": "obj.f2() — resolved via Object.create({ f1, f2 })"
},
{
"source": { "name": "getter", "file": "define-property.js" },
"target": { "name": "baz", "file": "define-property.js" },
"kind": "calls",
"mode": "define-property",
"notes": "this.baz() inside getter — this === accessorTarget (registered via Object.defineProperty)"
},
{
"source": { "name": "runBind", "file": "bind-call-apply.js" },
"target": { "name": "greet", "file": "bind-call-apply.js" },
Expand Down
3 changes: 3 additions & 0 deletions tests/benchmarks/resolution/resolution-benchmark.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ const TECHNIQUE_MAP: Record<string, string> = {
'points-to': 'points-to',
'pts-define-property': 'points-to',
'pts-create-prototype': 'points-to',
'define-property': 'ts-native',
};

// ── Configuration ────────────────────────────────────────────────────────
Expand All @@ -118,6 +119,8 @@ const THRESHOLDS: Record<string, { precision: number; recall: number }> = {
// (5 new edges in define-property.js) + Phase 8.5 adds class-inheritance and prototype edges
// (inheritance.js, prototypes.js, prototypes2.js), lifting total expected to 30. Phase 8.3f
// adds bind/call/apply resolution (3 new edges in bind-call-apply.js), total expected now 33.
// Phase 8.3g adds Object.defineProperty accessor this-dispatch (1 new edge in define-property.js),
// total expected now 34.
javascript: { precision: 1.0, recall: 0.9 },
// TS 0.72: Phase 8.3e adds this.method() same-class resolution (Shape.describe → Shape.area),
// lifting recall from 69.4% to 72.2%. Remaining gap (interface-dispatch, CHA) is tracked
Expand Down
Loading