fix(resolver): scope this.prop typeMap key to enclosing class to prevent false edges in multi-class files - #1382
Conversation
Sorry, something went wrong.
|
Claude finished @carlos-alm's task —— View job PR Review
Phase 1: Root Cause AnalysisProblem: In multi-class files, handlePropWriteTypeMap stored this.prop = new Ctor() under the bare key this.prop. With setTypeMapEntry's equal-confidence-wins-first policy, the second class's identical property name was silently discarded from the typeMap, causing all this.service.method() calls in that class to resolve against the wrong type. Approach is correct and non-hiding: The fix changes the root data — scoping the typeMap key to ClassName.prop — rather than patching a symptom. The resolver's symmetric fallback mirrors how the extractor now stores keys, which is the right way to close the loop. No config relaxations found. No test expectations lowered. Clean. Phase 2: Configuration Change DetectionNo biome rules relaxed, no TypeScript flags changed, no test timeouts or coverage thresholds modified. ✓ Phase 3: Test IntegrityTests are strengthened, not weakened:
No red flags. Phase 4: Code QualityExtractor (src/extractors/javascript.ts)
Resolver (src/domain/graph/builder/call-resolver.ts)
Benchmark / fixture
Phase 5: Backlog Compliance
Critical ConcernsMedium (non-blocking, worth a follow-up): The extractTypeMapWalk.walk function does not mirror extractReturnTypeMapWalk.walk's explicit currentClass → null reset when entering method_definition or function_declaration bodies. For the current use case (only this.prop = new Ctor() reads currentClass) this is correct, but the divergence from the established sibling pattern is a maintenance hazard. Recommend opening a follow-up to align the two walks, or add a comment explaining why the reset is intentionally omitted here. Low: The class (anonymous class expression) case produces a null className and silently falls back to this.prop — same as pre-fix. A comment noting this would help future reviewers. Final Assessment## Root Cause Analysis
Problem Being Solved: typeMap collision between same-named this.prop across classes in one file
causing false call edges (wrong type attributed to method calls)
Why This Approach: Scoping the typeMap key to ClassName.prop eliminates the root data
collision; resolver fallback closes the lookup loop symmetrically
Risk Assessment: Low. Backward-compatible, incrementally applied, well-tested.
## Backlog Compliance
- Zero-dep: ✓
- Foundation-aligned: ✓
- Problem-fit: 5 — directly prevents false edges (core product value)
- Breaking: No
- Tier: 1
## Critical Concerns
- Medium: extractTypeMapWalk.walk doesn't reset currentClass on function/method
boundaries unlike its sibling extractReturnTypeMapWalk — harmless today but a
maintainability divergence worth tracking.
## Final Recommendation
- Rating: ⭐⭐⭐⭐ (4/5)
- Action: APPROVE
- Reasoning: Correct fix, right architecture, additive tests, no regressions.
The one structural concern (missing currentClass reset pattern) does not affect
correctness today but should be tracked as a follow-up.
|
Sorry, something went wrong.
Greptile SummaryThis PR fixes false call-graph edges in multi-class files by scoping this.prop = new Ctor() typeMap keys to the enclosing class name (ClassName.prop) instead of the shared this.prop key, and adds a symmetric fallback lookup in the resolver so calls on this.X receivers find the class-scoped entry.
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to this.prop = new Ctor() key generation and its corresponding resolver lookup, with no effect on any other resolution path. The extractor change only affects this.prop = new Ctor() writes inside class declarations; all other entry types are untouched. The resolver addition is an append-only fourth-stage fallback guarded by !typeEntry and call.receiver.startsWith('this.') so it cannot fire when the three existing lookups already succeed. The test suite adds specific regression coverage for all new branches plus four new end-to-end benchmark edges. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as walk(node, depth, currentClass)
participant E as handlePropWriteTypeMap
participant TM as typeMap
Note over W: class_declaration ClassA
W->>W: "currentClass = ClassA"
W->>E: "this.service = new ServiceA()"
E->>TM: "set ClassA.service = ServiceA"
Note over W: class_declaration ClassB
W->>W: "currentClass = ClassB"
W->>E: "this.service = new ServiceB()"
E->>TM: "set ClassB.service = ServiceB"
Note over TM: Both entries coexist
participant R as resolveByMethodOrGlobal
Note over R: receiver=this.service callerName=ClassA.runA
R->>TM: get(service) miss
R->>TM: get(this.service) miss
R->>TM: get(ClassA.runA::service) miss
R->>TM: "get(ClassA.service) = ServiceA"
R-->>R: resolves to ServiceA.doA
Reviews (9): Last reviewed commit: "fix: resolve merge conflicts with main" | Re-trigger Greptile |
Sorry, something went wrong.
| if (!typeEntry && call.receiver.startsWith('this.') && callerName) { | ||
| const dotIdx = callerName.lastIndexOf('.'); | ||
| if (dotIdx > -1) { | ||
| const callerClass = callerName.slice(0, dotIdx); | ||
| typeEntry = typeMap.get(`${callerClass}.${effectiveReceiver}`); | ||
| } |
There was a problem hiding this comment.
Qualifier mismatch for deeply-nested callerNames
The extractor stores the bare class name from nameNode.text (e.g. ClassA), so the typeMap key is always ClassA.prop. The resolver extracts the class portion via callerName.lastIndexOf('.'), which returns the full prefix for any callerName with more than one dot segment — e.g. callerName = 'Namespace.ClassA.method' yields callerClass = 'Namespace.ClassA', and the lookup Namespace.ClassA.prop will never find the extractor's ClassA.prop entry. The same lastIndexOf pattern is already used for same-class method dispatch at line 184, so this is a pre-existing limitation of the naming scheme rather than a new regression, but it is worth noting for future work if qualified class names appear in the graph.
Sorry, something went wrong.
There was a problem hiding this comment.
Acknowledged. This is a pre-existing limitation of the lastIndexOf('.') naming scheme that also exists at line 184 for same-class method dispatch. Tracked as #1385 for future resolution.
Sorry, something went wrong.
Codegraph Impact Analysis14 functions changed → 33 callers affected across 8 files
|
Sorry, something went wrong.
…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
|
Commit message amended to fix the two commitlint violations (header was 105 chars, body had long lines — both reduced to ≤100 chars). Force-pushed. Follow-up issues filed for the two non-blocking concerns raised in review:
|
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
Claude finished @carlos-alm's task —— View job Re-Review
Commit MessageThe amended commit message is clean. Header is fix(resolver): scope this.prop typeMap key to class to prevent false edges in multi-class files — well within the 100-char limit. The body stays ≤100 chars per line. Commitlint violations are resolved. ✓ Follow-up IssuesI wasn't able to fetch #1385 and #1386 directly (tool restrictions), but the comments show both were filed by @carlos-alm before tagging me. Taken at face value. ✓ Code VerificationextractTypeMapWalk (javascript.ts:1648–1672) — reviewed the current state:
resolveByMethodOrGlobal (call-resolver.ts:78–84) — reviewed the current state:
Fixture and test integrity:
AssessmentNo new concerns since the initial review. The two non-blocking findings have been properly tracked as issues rather than swept under the rug. The code is correct, well-tested, and backward-compatible. Rating: ⭐⭐⭐⭐ (4/5) — unchanged Action: APPROVE The half-star gap remains the currentClass reset pattern divergence from extractReturnTypeMapWalk (#1386) — a future-maintenance concern, not a correctness issue. Nothing blocks merge. |
Sorry, something went wrong.
… 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.
|
Fixed the named class expression silent regression flagged by Greptile in the summary review. For const Foo = class Bar { ... }, the extractor was storing the key as Bar.x (using the expression-internal name), but the resolver derives callerClass from the binding name Foo — so Foo.x missed Bar.x. Before this PR the entry was stored as this.x and found by the second fallback lookup. Fix: only class_declaration and abstract_class_declaration receive the class-scoped key treatment. The class expression case now walks children with null, preserving the this.prop fallback that the second lookup in resolveByMethodOrGlobal still finds. A unit test asserting this.x is stored (not Bar.x) for named class expressions was added. Committed in 501d620. |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
Claude finished @carlos-alm's task —— View job Re-Review (latest commit: 501d620)
Named Class Expression Fix (501d620)What changed: extractTypeMapWalk.walk now handles class (expression) nodes separately from class_declaration / abstract_class_declaration. Class expressions walk their children with null, preserving the pre-fix this.prop fallback that the second lookup in resolveByMethodOrGlobal finds. Verification of the fix: src/extractors/javascript.ts:1651–1668 — the split is clean:
The comment at lines 1659–1662 correctly explains the reasoning: the expression-internal name (Bar in class Bar) is never the name the resolver derives callerClass from (which comes from the binding Foo), so using it would create an unreachable key. Storing as this.prop is the right regression-safe choice. New unit test at tests/parsers/javascript.test.ts:370–381: it('uses this.prop fallback for named class expressions ...')
No new concerns. The fix is minimal, correctly reasoned, and fully covered by the new test. It also doesn't disturb the class_declaration path that was working correctly. Final Assessment## Summary of Changes Across All Commits - Core fix: ClassName.prop key scoping for class_declaration/abstract_class_declaration - Resolver: symmetric CallerClass.prop fallback lookup - Regression fix: class expression nodes walk with null (this.prop fallback preserved) - Tests: 4 unit tests + benchmark fixture + driver ## Backlog Compliance - Zero-dep: ✓ - Foundation-aligned: ✓ - Problem-fit: 5 — directly prevents false call edges - Breaking: No - Tier: 1 ## Open Tracking - #1385: qualified callerName mismatch (lastIndexOf pattern, pre-existing) - #1386: currentClass reset pattern divergence (harmless, maintainability) ## Final Recommendation - Rating: ⭐⭐⭐⭐½ (4.5/5) - Action: APPROVE - Reasoning: All three review rounds resolved cleanly. Root cause fixed, regression patched and tested, both non-blocking findings tracked as issues. No correctness concerns remain. |
Sorry, something went wrong.
Sorry, something went wrong.
Merge origin/main (jelly-micro fixtures + Phase 8.3f rest-param scoping). call-resolver.ts: combine both fallbacks — callerName::effectiveReceiver for Phase 8.3f rest-param collision fix (#1358) and callerClass.prop for class-scoped this.prop resolution (#1323). Both lookups target distinct keyspaces and are additive.
Sorry, something went wrong.
Summary
Test plan
Closes #1323