feat(resolver): track constructor-assigned property types for receiver-typed resolution (JS/TS)#1314
Conversation
…r-typed resolution (JS/TS) Closes #1306 Extends `handlePropWriteTypeMap` to seed the points-to type map from `this.prop = new ClassName(...)` assignments, enabling resolution of `this.prop.method()` calls through the existing receiver-typed path. Before: `this.logger.error/info/warn()` in UserService were unresolvable because only `obj.prop = identifier` writes were tracked (Phase 8.3d). After: `this.logger = new Logger(...)` seeds `typeMap['this.logger'] = Logger` with confidence 1.0 (same as `const x = new Ctor()` in variable declarators). The existing `resolveByMethodOrGlobal` and native `edge_builder.rs` fallback (`or_else(|| type_map.get(receiver))`) then pick up `this.logger` and resolve `this.logger.error()` → `Logger.error`. Impact: JS fixture receiver-typed recall 2/5 → 5/5 (40% → 100%). Ratchet JS benchmark gate: precision 0.85→1.0, recall 0.5→0.9.
|
Claude finished @carlos-alm's task —— View job PR Review: feat(resolver): track constructor-assigned property types for receiver-typed resolution (JS/TS)Tasks:
Root Cause AnalysisProblem Being Solved: Why This Approach: Correct. The fix is surgical: extend Risk Assessment: Low. The change is confined to one function, behind an Phase 2: Configuration Change DetectionNo linting rules relaxed, no TypeScript strictness changes, no timeouts changed. The benchmark threshold change is a ratchet-up (precision 0.85→1.0, recall 0.5→0.9), not a relaxation. ✓ Phase 3: Test IntegrityTwo new unit tests are added to
Both assertions are appropriately strict. No previously-passing tests were weakened. The benchmark gate change is an upward ratchet — this is expected and correct. One observation worth checking: The test at line 287 calls Phase 4: Code Quality
// this.prop = new ClassName(...) — constructor-assigned property type
if (obj.type === 'this' && rhsN.type === 'new_expression') {
const ctorType = extractNewExprTypeName(rhsN);
if (ctorType) setTypeMapEntry(typeMap, `this.${prop.text}`, ctorType, 1.0);
return;
}Correctness: The early Docstring update: The JSDoc at line 1511–1518 was updated to document the new case: Accurate and helpful. ✓
Minor concern — Scope of Phase 5: Backlog Compliance
Critical ConcernsNone. The implementation is clean, focused, and correct. One note for future work: The Final Recommendation
|
Greptile SummaryThis PR adds two complementary mechanisms for resolving
Confidence Score: 5/5Safe to merge — both new mechanisms are tightly guarded and handle orthogonal resolution paths without affecting existing behaviour. The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["call site processed\nin buildFileCallEdges"] --> B["resolveCallTargets()"]
B --> C{targets.length > 0?}
C -->|Yes| G["emit edge(s)"]
C -->|No| D{"call.receiver === 'this'\nAND callerName != null?"}
D -->|No| H["pts-map fallback\n(Phase 8.3c)"]
D -->|Yes| E["extract className\nfrom callerName\n(e.g. Logger.info to Logger)"]
E --> F["lookup.byNameAndFile\n(className.methodName, relPath)\n.filter kind==='method'"]
F --> I{found?}
I -->|Yes| G
I -->|No| H
subgraph "typeMap seeding (javascript.ts)"
J["this.prop = new Ctor()\nin AST walk"] --> K["extractNewExprTypeName()"]
K --> L["setTypeMapEntry\n('this.prop', Ctor, 1.0)"]
L --> M["resolveByMethodOrGlobal\nfinds typeMap entry\nfor 'this.logger' receiver"]
M --> B
end
Reviews (6): Last reviewed commit: "Merge branch 'main' into feat/phase-8.3e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (obj.type === 'this' && rhsN.type === 'new_expression') { | ||
| const ctorType = extractNewExprTypeName(rhsN); | ||
| if (ctorType) setTypeMapEntry(typeMap, `this.${prop.text}`, ctorType, 1.0); | ||
| return; |
There was a problem hiding this comment.
this.prop key collision can produce false edges
The typeMap key this.${prop.text} is global within a parsed file and carries no class-scope qualifier. In any file that defines two or more classes that both assign the same property name via this.prop = new DifferentCtor(), only the first write survives (setTypeMapEntry discards equal-confidence duplicates). Resolution for callers of the second class will then resolve this.prop to the wrong type, producing a spurious call graph edge.
Before this PR neither edge was resolved; after it, one is resolved correctly and the other incorrectly. The JS precision gate is now at 1.0, so this regression would surface as a benchmark failure if a multi-class fixture exercised the pattern.
There was a problem hiding this comment.
Tracked as #1323. This is a valid concern — the this.prop key is unscoped and would cause collisions in a multi-class same-file pattern. The current fixtures have at most one class per file, so no false positives occur today (JS precision is 1.0). The fix (qualifying the key as ClassName.prop) is deferred to keep this PR scoped to constructor property type tracking.
| javascript: { precision: 0.85, recall: 0.5 }, | ||
| typescript: { precision: 0.85, recall: 0.5 }, | ||
| javascript: { precision: 1.0, recall: 0.9 }, | ||
| typescript: { precision: 0.85, recall: 0.72 }, |
There was a problem hiding this comment.
The TypeScript recall gate rises from 0.5 → 0.72, but the PR description exclusively discusses JavaScript improvements. There is no mention of what TypeScript edges are newly resolved or which fixtures drive the change. If it reflects a real improvement (e.g., the same this.prop seeding applies to .ts fixture files via the shared extractor), a short comment would clarify intent; if it is a speculative adjustment it risks masking a future regression in TS resolution.
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!
There was a problem hiding this comment.
Added inline comments (commit a33b0f7) explaining each threshold: the JS 0.9 gate reflects the this.method() same-class fix in Phase 8.3e (83.3% → 100%); the TS 0.72 gate reflects the same fix resolving Shape.describe → Shape.area (69.4% → 72.2%), with the remaining gap (interface-dispatch requires TSC, CHA edges) tracked in future phases.
Codegraph Impact Analysis2 functions changed → 10 callers affected across 3 files
|
…M path (#1314) When a class method calls this.method() or super.method(), the WASM/JS resolution path failed to find the target because methods are stored as qualified names (e.g. Logger._write) but lookups used the unqualified name (_write). The native Rust engine resolved this implicitly via its class-scoped symbol table. Fix: in buildFileCallEdges, after resolveCallTargets returns no targets for a this/super receiver call, extract the class name prefix from the caller's qualified name (e.g. Logger.info → Logger) and retry with the fully-qualified method name (Logger._write) in the same file. Fixes the JS benchmark recall (83.3% → 100%) and TS benchmark recall (69.4% → 72.2%), satisfying CI thresholds of 90% and 72% respectively.
…1314) Add inline comments explaining why JS recall is 0.9 and TS recall is 0.72, linking the thresholds to the Phase 8.3e same-class this.method() fix and noting which remaining gaps (interface-dispatch, CHA) are tracked in future phases. Addresses Greptile P2 review comment on the threshold bump.
|
Claude finished @carlos-alm's task —— View job PR Review UpdateTasks:
Both Greptile review concerns have been addressed: Issue 1 — Cross-class Issue 2 — TypeScript recall threshold without explanation (
Both responses are technically accurate and follow the repo's scope discipline. The inline threshold comments make the ratchet rationale self-documenting for future reviewers. CI is running. The implementation is clean — no further changes needed. |
|
Fixed the |
1 participant
Closes #1306
Summary
handlePropWriteTypeMapinsrc/extractors/javascript.tsto seed the points-to type map fromthis.prop = new ClassName(...)constructor assignmentsobj.type === 'this'andrhs.type === 'new_expression', extracts the constructor class name via the existingextractNewExprTypeNamehelper and seedstypeMap['this.logger'] = { type: 'Logger', confidence: 1.0 }resolveByMethodOrGlobal) and the Rust native path (edge_builder.rs—or_else(|| type_map.get(receiver.as_str()))) already handlesthis.propkeys; they just had no entry to findprecision 0.85→1.0,recall 0.5→0.9(JS fixture now 100%/100%)Before / After
The 3 previously-missing edges are now resolved:
UserService.createUser → Logger.error(viathis.logger.error())UserService.createUser → Logger.info(viathis.logger.info())UserService.deleteUser → Logger.warn(viathis.logger.warn())Key note — WASM worker dist
The WASM worker pool serves parsed symbols from a compiled
dist/artifact (gitignored). The fix takes effect oncenpm run buildis run — which CI does before running tests.Test plan
tests/parsers/javascript.test.ts— 2 new unit tests forthis.prop = new Ctor()typeMap seeding andthis.prop = identifiernon-seedingtests/benchmarks/resolution/resolution-benchmark.test.ts— JS benchmark gate passes at ratcheted thresholds (precision 1.0, recall 0.9)