Skip to content

Commit fd59f88

Browse files
committed
fix(resolver): scope this.prop typeMap key to enclosing class to prevent false edges in multi-class files (#1382)
* fix(resolver): scope this.prop typeMap key to class to prevent false edges in multi-class files When a file defines two or more classes that both assign `this.service = new X()` in their constructors, `handlePropWriteTypeMap` previously stored all entries under the unqualified key `this.service`. `setTypeMapEntry` discards equal-confidence duplicates, so only one class's entry survived, causing the other class's `this.service.method()` calls to resolve against the wrong type and producing spurious call-graph edges. Fix: extractTypeMapWalk now threads a `currentClass: string | null` parameter through its inner walk. When a `class_declaration` node is encountered, the class name is extracted and passed down to all children. `handlePropWriteTypeMap` uses this to key `this.prop = new Ctor()` writes as `ClassName.prop` (e.g. `ClassA.service`) instead of `this.prop`, so two classes with the same property name no longer collide in the typeMap. The call resolver gains a symmetric fallback: when a `this.X` receiver isn't found via `effectiveReceiver` / `this.X` lookups, it also tries `CallerClass.X` where the caller class is extracted from `callerName` (e.g. `ClassA.runA` → `ClassA`). This correctly resolves the class-scoped key while leaving all other resolution paths untouched. Also adds: `multi-class.js` fixture, four expected edges, driver exercise, and three unit tests covering the new scoping, fallback when there is no enclosing class, and the absence of false entries. Closes #1323 * fix(extractor): use this.prop fallback for named class expressions in typeMap Named class expressions (`const Foo = class Bar { ... }`) store the key as `Bar.x` using the expression-internal name, but the resolver derives callerClass from the binding name `Foo`, so it looks up `Foo.x` and misses `Bar.x`. Walk class expression children with null so the pre-fix `this.prop` fallback is preserved and the second lookup in resolveByMethodOrGlobal still finds the entry. Only class_declaration and abstract_class_declaration receive the class-scoped key treatment. Add a unit test asserting `this.x` is stored (not `Bar.x`) for named class expressions. Fixes a silent regression identified by Greptile in PR #1382 review.
1 parent 19bec64 commit fd59f88

6 files changed

Lines changed: 158 additions & 10 deletions

File tree

src/domain/graph/builder/call-resolver.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,22 @@ export function resolveByMethodOrGlobal(
7272
const effectiveReceiver = call.receiver.startsWith('this.')
7373
? call.receiver.slice('this.'.length)
7474
: call.receiver;
75-
const typeEntry =
75+
// For this.prop receivers, also try the class-scoped key (ClassName.prop) seeded by
76+
// handlePropWriteTypeMap — prevents false edges when multiple classes define the same
77+
// property name (issue #1323).
78+
let typeEntry =
7679
typeMap.get(effectiveReceiver) ??
7780
typeMap.get(call.receiver) ??
81+
// Phase 8.3f: callee-scoped rest-param key (`callee::restName`) to avoid
82+
// same-name rest-binding collision across functions in the same file (#1358).
7883
(callerName ? typeMap.get(`${callerName}::${effectiveReceiver}`) : undefined);
84+
if (!typeEntry && call.receiver.startsWith('this.') && callerName) {
85+
const dotIdx = callerName.lastIndexOf('.');
86+
if (dotIdx > -1) {
87+
const callerClass = callerName.slice(0, dotIdx);
88+
typeEntry = typeMap.get(`${callerClass}.${effectiveReceiver}`);
89+
}
90+
}
7991
let typeName = typeEntry
8092
? typeof typeEntry === 'string'
8193
? typeEntry

src/extractors/javascript.ts

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,23 +1741,41 @@ function extractTypeMapWalk(
17411741
callAssignments?: CallAssignment[],
17421742
fnRefBindings?: FnRefBinding[],
17431743
): void {
1744-
function walk(node: TreeSitterNode, depth: number): void {
1744+
function walk(node: TreeSitterNode, depth: number, currentClass: string | null): void {
17451745
if (depth >= MAX_WALK_DEPTH) return;
17461746
const t = node.type;
1747+
if (t === 'class_declaration' || t === 'abstract_class_declaration') {
1748+
const nameNode = node.childForFieldName('name');
1749+
const className = nameNode?.text ?? null;
1750+
for (let i = 0; i < node.childCount; i++) {
1751+
walk(node.child(i)!, depth + 1, className);
1752+
}
1753+
return;
1754+
}
1755+
// Class expressions (e.g. `const Foo = class Bar { ... }`): the expression-internal
1756+
// name (`Bar`) is never visible to the resolver, which derives callerClass from the
1757+
// binding name (`Foo`). Walking with null preserves the pre-fix `this.prop` fallback
1758+
// so the second lookup in resolveByMethodOrGlobal still finds the entry.
1759+
if (t === 'class') {
1760+
for (let i = 0; i < node.childCount; i++) {
1761+
walk(node.child(i)!, depth + 1, null);
1762+
}
1763+
return;
1764+
}
17471765
if (t === 'variable_declarator') {
17481766
handleVarDeclaratorTypeMap(node, typeMap, returnTypeMap, callAssignments, fnRefBindings);
17491767
} else if (t === 'required_parameter' || t === 'optional_parameter') {
17501768
handleParamTypeMap(node, typeMap);
17511769
} else if (t === 'assignment_expression') {
1752-
handlePropWriteTypeMap(node, typeMap);
1770+
handlePropWriteTypeMap(node, typeMap, currentClass);
17531771
} else if (t === 'call_expression') {
17541772
handleDefinePropertyTypeMap(node, typeMap);
17551773
}
17561774
for (let i = 0; i < node.childCount; i++) {
1757-
walk(node.child(i)!, depth + 1);
1775+
walk(node.child(i)!, depth + 1, currentClass);
17581776
}
17591777
}
1760-
walk(rootNode, 0);
1778+
walk(rootNode, 0, null);
17611779
}
17621780

17631781
/** Extract type info from a variable_declarator: type annotation, constructor, or factory. */
@@ -1957,12 +1975,17 @@ function handleParamTypeMap(node: TreeSitterNode, typeMap: Map<string, TypeMapEn
19571975
* Phase 8.3d: seed the pts map from object property writes.
19581976
*
19591977
* `handlers.auth = authMiddleware` → typeMap.set('handlers.auth', { type: 'authMiddleware', confidence: 0.85 })
1960-
* `this.logger = new Logger(...)` → typeMap.set('this.logger', { type: 'Logger', confidence: 1.0 })
1978+
* `this.logger = new Logger(...)` → typeMap.set('UserService.logger', { type: 'Logger', confidence: 1.0 })
1979+
* (keyed as ClassName.prop when currentClass is known, to avoid collisions across classes)
19611980
*
19621981
* Only simple `obj.prop = identifier` and `this.prop = new Ctor()` writes are tracked
19631982
* (not chained `a.b.c = x`). BUILTIN_GLOBALS are skipped (e.g. `console.log = fn`).
19641983
*/
1965-
function handlePropWriteTypeMap(node: TreeSitterNode, typeMap: Map<string, TypeMapEntry>): void {
1984+
function handlePropWriteTypeMap(
1985+
node: TreeSitterNode,
1986+
typeMap: Map<string, TypeMapEntry>,
1987+
currentClass: string | null,
1988+
): void {
19661989
const lhsN = node.childForFieldName('left');
19671990
const rhsN = node.childForFieldName('right');
19681991
if (!lhsN || !rhsN) return;
@@ -1975,10 +1998,15 @@ function handlePropWriteTypeMap(node: TreeSitterNode, typeMap: Map<string, TypeM
19751998
// computed subscript expressions — consistent with the adjacent fnRefBindings block.
19761999
if (prop.type !== 'property_identifier' && prop.type !== 'identifier') return;
19772000

1978-
// this.prop = new ClassName(...) — constructor-assigned property type
2001+
// this.prop = new ClassName(...) — constructor-assigned property type.
2002+
// Key as ClassName.prop (class-scoped) so two classes with identically-named
2003+
// properties don't overwrite each other's typeMap entry.
19792004
if (obj.type === 'this' && rhsN.type === 'new_expression') {
19802005
const ctorType = extractNewExprTypeName(rhsN);
1981-
if (ctorType) setTypeMapEntry(typeMap, `this.${prop.text}`, ctorType, 1.0);
2006+
if (ctorType) {
2007+
const key = currentClass ? `${currentClass}.${prop.text}` : `this.${prop.text}`;
2008+
setTypeMapEntry(typeMap, key, ctorType, 1.0);
2009+
}
19822010
return;
19832011
}
19842012

tests/benchmarks/resolution/fixtures/javascript/driver.mjs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import { directInstantiation, main } from './index.js';
1414
import { Logger } from './logger.js';
15+
import { ClassA, ClassB } from './multi-class.js';
1516
import { buildService } from './service.js';
1617
import { normalize, validate } from './validators.js';
1718

@@ -39,6 +40,10 @@ try {
3940
svc.createUser({ name: 'Direct' });
4041
svc.deleteUser(99);
4142

43+
// Multi-class fixture — exercises class-scoped this.prop typeMap (issue #1323)
44+
new ClassA().runA();
45+
new ClassB().runB();
46+
4247
globalThis.__tracer.popCall();
4348
} catch {
4449
// Swallow errors — we only care about call edges

tests/benchmarks/resolution/fixtures/javascript/expected-edges.json

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,34 @@
247247
"kind": "calls",
248248
"mode": "defineProperty-accessor",
249249
"notes": "this.accessMethod() inside accessorGetter — this === accessorTarget (get accessor via Object.defineProperty); accessorTarget.accessMethod is the qualified node name for the arrow function property of accessorTarget"
250+
},
251+
{
252+
"source": { "name": "ClassA.constructor", "file": "multi-class.js" },
253+
"target": { "name": "ServiceA", "file": "multi-class.js" },
254+
"kind": "calls",
255+
"mode": "constructor",
256+
"notes": "new ServiceA() — constructor call in ClassA.constructor"
257+
},
258+
{
259+
"source": { "name": "ClassB.constructor", "file": "multi-class.js" },
260+
"target": { "name": "ServiceB", "file": "multi-class.js" },
261+
"kind": "calls",
262+
"mode": "constructor",
263+
"notes": "new ServiceB() — constructor call in ClassB.constructor"
264+
},
265+
{
266+
"source": { "name": "ClassA.runA", "file": "multi-class.js" },
267+
"target": { "name": "ServiceA.doA", "file": "multi-class.js" },
268+
"kind": "calls",
269+
"mode": "receiver-typed",
270+
"notes": "this.service.doA() — receiver-typed via ClassA.service = new ServiceA() (class-scoped typeMap key prevents collision with ClassB.service)"
271+
},
272+
{
273+
"source": { "name": "ClassB.runB", "file": "multi-class.js" },
274+
"target": { "name": "ServiceB.doB", "file": "multi-class.js" },
275+
"kind": "calls",
276+
"mode": "receiver-typed",
277+
"notes": "this.service.doB() — receiver-typed via ClassB.service = new ServiceB() (class-scoped typeMap key prevents collision with ClassA.service)"
250278
}
251279
]
252280
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* Fixture: two classes in the same file that both use `this.service`,
3+
* but assign different types. Before the fix, the second class's
4+
* typeMap entry overwrote the first, causing one class to resolve
5+
* `this.service.method()` against the wrong type (false edge).
6+
*/
7+
8+
export class ServiceA {
9+
doA() {}
10+
}
11+
12+
export class ServiceB {
13+
doB() {}
14+
}
15+
16+
export class ClassA {
17+
constructor() {
18+
this.service = new ServiceA();
19+
}
20+
21+
runA() {
22+
this.service.doA();
23+
}
24+
}
25+
26+
export class ClassB {
27+
constructor() {
28+
this.service = new ServiceB();
29+
}
30+
31+
runB() {
32+
this.service.doB();
33+
}
34+
}

tests/parsers/javascript.test.ts

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,24 +360,65 @@ describe('JavaScript parser', () => {
360360
expect(symbols.typeMap.has('b.c')).toBe(false);
361361
});
362362

363-
it('seeds typeMap for this.prop = new ClassName() with confidence 1.0', () => {
363+
it('seeds typeMap for this.prop = new ClassName() using class-scoped key', () => {
364364
const symbols = parseJS(`
365365
class UserService {
366366
constructor() {
367367
this.logger = new Logger('UserService');
368368
}
369369
}
370370
`);
371+
expect(symbols.typeMap.get('UserService.logger')).toEqual({
372+
type: 'Logger',
373+
confidence: 1.0,
374+
});
375+
expect(symbols.typeMap.has('this.logger')).toBe(false);
376+
});
377+
378+
it('uses this.prop key when no enclosing class is present', () => {
379+
const symbols = parseJS(`
380+
function setup() {
381+
this.logger = new Logger();
382+
}
383+
`);
371384
expect(symbols.typeMap.get('this.logger')).toEqual({ type: 'Logger', confidence: 1.0 });
372385
});
373386

387+
it('scopes this.prop typeMap key to enclosing class — no collision across classes', () => {
388+
const symbols = parseJS(`
389+
class ClassA {
390+
constructor() { this.service = new ServiceA(); }
391+
}
392+
class ClassB {
393+
constructor() { this.service = new ServiceB(); }
394+
}
395+
`);
396+
expect(symbols.typeMap.get('ClassA.service')).toEqual({ type: 'ServiceA', confidence: 1.0 });
397+
expect(symbols.typeMap.get('ClassB.service')).toEqual({ type: 'ServiceB', confidence: 1.0 });
398+
expect(symbols.typeMap.has('this.service')).toBe(false);
399+
});
400+
401+
it('uses this.prop fallback for named class expressions (expression name not resolver-visible)', () => {
402+
// `const Foo = class Bar { ... }` — the resolver derives callerClass from the
403+
// binding name `Foo`, never from the expression name `Bar`. Storing as `Bar.x`
404+
// would produce an unreachable key, so we fall back to `this.x` instead.
405+
const symbols = parseJS(`
406+
const Foo = class Bar {
407+
constructor() { this.x = new X(); }
408+
};
409+
`);
410+
expect(symbols.typeMap.get('this.x')).toEqual({ type: 'X', confidence: 1.0 });
411+
expect(symbols.typeMap.has('Bar.x')).toBe(false);
412+
});
413+
374414
it('does not seed typeMap for this.prop = identifier (only new expressions)', () => {
375415
const symbols = parseJS(`
376416
class Foo {
377417
init(logger) { this.logger = logger; }
378418
}
379419
`);
380420
expect(symbols.typeMap.has('this.logger')).toBe(false);
421+
expect(symbols.typeMap.has('Foo.logger')).toBe(false);
381422
});
382423

383424
it('ignores non-identifier RHS (a.prop = obj.method)', () => {

0 commit comments

Comments
 (0)