fix(types): narrow parser return types and cachedStmt in buildTestFileIds#569
fix(types): narrow parser return types and cachedStmt in buildTestFileIds#569carlos-alm merged 9 commits intomainfrom
Conversation
…Output - parseFileAuto, parseFilesAuto, parseFileIncremental now return Promise<ExtractorOutput | null> and Promise<Map<string, ExtractorOutput>> - patchNativeResult returns ExtractorOutput (normalizes typeMap array→Map) - WasmExtractResult.symbols narrowed from any to ExtractorOutput - WasmExtractResult.langId narrowed from string to LanguageId - backfillTypeMap returns typed Map<string, TypeMapEntry> - Add _typeMapBackfilled field to ExtractorOutput interface - Remove dead instanceof Map branches now that types are consistent Impact: 8 functions changed, 16 affected
Apply StmtCache<T> pattern to the two static SQL queries in buildTestFileIds (module-map.ts), consistent with the rest of the query layer. Impact: 1 functions changed, 6 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Type Safety ImprovementsReviewing this pull request with a focus on type safety improvements and proper TypeScript patterns. Todo List
Root Cause AnalysisProblem Being Solved: Addresses legitimate type safety regression where parser functions returned Why This Approach: The approach is sound - it narrows return types to their actual contracts ( Risk Assessment: Low risk. This is purely additive type safety with no behavioral changes. Backlog Compliance
Code Quality AssessmentStrengths:
Technical Details:
Verification:
Minor Observations
Final Recommendation
This PR exemplifies good TypeScript hygiene - narrowing overly broad types to their actual contracts while maintaining backward compatibility. |
Greptile SummaryThis PR follows up on review feedback from #558 by narrowing the return types of Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["parseFileAuto / parseFileIncremental\n(Promise<ExtractorOutput | null>)"] --> B{native engine?}
B -- yes --> C["native.parseFile()"]
C --> D["patchNativeResult(r): ExtractorOutput"]
D --> E{typeMap present?}
E -- "r.typeMap is falsy" --> F["r.typeMap = new Map()"]
E -- "array shape" --> G["new Map(array → TypeMapEntry objects)\n{ type, confidence: 0.9 }"]
E -- "already a Map" --> H["no-op"]
F & G & H --> I{TS/TSX ext AND\ntypeMap.size === 0?}
I -- yes --> J["backfillTypeMap()\n(WASM typeMap extraction)"]
J --> K["patched._typeMapBackfilled = true"]
I -- no --> L["return patched"]
K --> L
B -- no --> M["wasmExtractSymbols()\n→ extracted.symbols: ExtractorOutput"]
M --> N["return symbols (typeMap already a Map)"]
P["parseFilesAuto\n(Promise<Map<string, ExtractorOutput>>)"] --> Q{native engine?}
Q -- yes --> R["native.parseFiles() batch"]
R --> S["patchNativeResult per file"]
S --> T{typeMap.size === 0 AND TS/TSX?}
T -- yes --> U["backfill via WASM inline"]
T -- no --> V["result.set(relPath, patched)"]
U --> V
Q -- no --> W["WASM path: wasmExtractSymbols per file"]
W --> X["result.set(relPath, extracted.symbols)"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/domain/parser.ts
Outdated
| // typeMap: native returns an array of {name, typeName}; normalize to Map | ||
| if (r.typeMap && !(r.typeMap instanceof Map)) { | ||
| r.typeMap = new Map( | ||
| r.typeMap.map((e: { name: string; typeName: string }) => [e.name, e.typeName]), | ||
| ); | ||
| } |
There was a problem hiding this comment.
TypeMapEntry objects
The PR's stated goal is to "normalize native typeMap so the return genuinely conforms to ExtractorOutput". However, the conversion maps each entry to e.typeName — a plain string — while ExtractorOutput.typeMap is typed as Map<string, TypeMapEntry> where TypeMapEntry = { type: string; confidence: number }.
This means patchNativeResult returns a Map<string, string> disguised as Map<string, TypeMapEntry>, which TypeScript doesn't catch because r is typed as any. The actual type still doesn't conform at runtime, even though tsc --noEmit passes.
Runtime breakage is avoided only because downstream consumers (incremental.js:352, build-edges.js:109) already have defensive code guarding against string values (typeof entry === 'string' ? entry : entry.type). But the type-safety goal of this PR is not actually met for the native path.
The fix is to construct proper TypeMapEntry objects:
| // typeMap: native returns an array of {name, typeName}; normalize to Map | |
| if (r.typeMap && !(r.typeMap instanceof Map)) { | |
| r.typeMap = new Map( | |
| r.typeMap.map((e: { name: string; typeName: string }) => [e.name, e.typeName]), | |
| ); | |
| } | |
| // typeMap: native returns an array of {name, typeName}; normalize to Map | |
| if (r.typeMap && !(r.typeMap instanceof Map)) { | |
| r.typeMap = new Map( | |
| r.typeMap.map((e: { name: string; typeName: string }) => [ | |
| e.name, | |
| { type: e.typeName, confidence: 0.9 }, | |
| ]), | |
| ); | |
| } |
There was a problem hiding this comment.
Fixed — typeMap normalization now constructs proper TypeMapEntry objects with { type: e.typeName, confidence: 0.9 } instead of mapping to bare strings.
src/domain/parser.ts
Outdated
| // biome-ignore lint/suspicious/noExplicitAny: native result has dynamic shape | ||
| function patchNativeResult(r: any): any { | ||
| // biome-ignore lint/suspicious/noExplicitAny: native addon result has no type declarations |
There was a problem hiding this comment.
biome-ignore suppression for the same line
Two consecutive biome-ignore lint/suspicious/noExplicitAny comments apply to the same function declaration — a single comment suffices for one suppression target.
| // biome-ignore lint/suspicious/noExplicitAny: native result has dynamic shape | |
| function patchNativeResult(r: any): any { | |
| // biome-ignore lint/suspicious/noExplicitAny: native addon result has no type declarations | |
| // biome-ignore lint/suspicious/noExplicitAny: native addon result has no type declarations |
There was a problem hiding this comment.
Fixed — removed the duplicate biome-ignore suppression. Single comment now suffices.
…emove duplicate biome-ignore (#569)
src/domain/parser.ts
Outdated
| // typeMap: native returns an array of {name, typeName}; normalize to Map | ||
| if (r.typeMap && !(r.typeMap instanceof Map)) { | ||
| r.typeMap = new Map( | ||
| r.typeMap.map((e: { name: string; typeName: string }) => [ | ||
| e.name, | ||
| { type: e.typeName, confidence: 0.9 } as TypeMapEntry, | ||
| ]), | ||
| ); | ||
| } |
There was a problem hiding this comment.
typeMap can remain undefined for non-TS native results
patchNativeResult now claims to return ExtractorOutput, which requires typeMap: Map<string, TypeMapEntry> (non-optional). However, the normalization block only runs when r.typeMap is truthy — if the native engine omits typeMap entirely (common for Python, Go, Rust, Java, etc.), r.typeMap stays undefined and the returned object silently violates the interface contract.
Downstream callers that now rely on the ExtractorOutput typing (e.g. looping patched.typeMap.entries() or checking .size) will throw Cannot read properties of undefined at runtime for any non-TS file parsed via the native path.
The backfill in parseFileAuto/parseFileIncremental only runs for TS_BACKFILL_EXTS, so other languages have no safety net.
Suggest restructuring the block to always guarantee a Map:
if (!r.typeMap) {
r.typeMap = new Map();
} else if (!(r.typeMap instanceof Map)) {
r.typeMap = new Map(
r.typeMap.map((e: { name: string; typeName: string }) => [
e.name,
{ type: e.typeName, confidence: 0.9 } as TypeMapEntry,
]),
);
}There was a problem hiding this comment.
Fixed — patchNativeResult now defaults typeMap to an empty Map when the native engine omits it entirely (common for non-TS languages). This ensures the ExtractorOutput contract is satisfied and callers can safely access .entries()/.size. Also fixed commitlint by shortening the commit header to under 100 chars.
carlos-alm
force-pushed
the
fix/narrow-parser-return-types
branch
from
0c2fef3 to
87aebfb
Compare
src/domain/parser.ts
Outdated
| // and the native engine already handles `new Expr()` patterns. | ||
| if ( | ||
| (!patched.typeMap || patched.typeMap.length === 0) && | ||
| (!patched.typeMap || patched.typeMap.size === 0) && |
There was a problem hiding this comment.
!patched.typeMap guard is now dead code
patchNativeResult unconditionally assigns r.typeMap to a Map (either new empty Map or the converted array), so patched.typeMap is always truthy after the call. The !patched.typeMap branch of the OR condition can never be entered.
The same redundant guard appears in two other places patched by this PR:
src/domain/parser.ts:553(parseFilesAuto)src/domain/parser.ts:668(parseFileIncremental)
Suggest simplifying all three to just the .size === 0 check:
| (!patched.typeMap || patched.typeMap.size === 0) && | |
| (patched.typeMap.size === 0) && |
There was a problem hiding this comment.
Fixed — removed the dead !patched.typeMap check from all three sites (lines 513, 553, 668). Since patchNativeResult now always sets typeMap to a Map, only the .size === 0 check is needed.
patchNativeResult now always sets typeMap to a Map, so the falsy check is dead code. Simplified all three sites to just check .size === 0.
src/domain/parser.ts
Outdated
| if (!extracted?.symbols?.typeMap) { | ||
| return { typeMap: [], backfilled: false }; | ||
| return { typeMap: new Map(), backfilled: false }; | ||
| } | ||
| const tm = extracted.symbols.typeMap; | ||
| return { | ||
| typeMap: | ||
| tm instanceof Map | ||
| ? tm | ||
| : new Map(tm.map((e: { name: string; typeName: string }) => [e.name, e.typeName])), | ||
| backfilled: true, | ||
| }; | ||
| return { typeMap: extracted.symbols.typeMap, backfilled: true }; |
There was a problem hiding this comment.
extracted, not empty typeMap
Since WasmExtractResult.symbols is now typed as ExtractorOutput, typeMap is a required (non-optional) Map<string, TypeMapEntry>. A Map is always truthy — even when empty — so the ?.typeMap portion of the guard can never be falsy once extracted is non-null.
This means when the WASM extractor successfully parses a TS file but finds no type-annotated variables (legitimate for small or purely structural files), this function returns { typeMap: new Map(), backfilled: true } rather than { typeMap: new Map(), backfilled: false }. The _typeMapBackfilled flag then gets set on the caller side even though nothing was actually backfilled.
The flag is currently informational-only, so this doesn't cause incorrect behavior, but the check should reflect the new invariant:
| if (!extracted?.symbols?.typeMap) { | |
| return { typeMap: [], backfilled: false }; | |
| return { typeMap: new Map(), backfilled: false }; | |
| } | |
| const tm = extracted.symbols.typeMap; | |
| return { | |
| typeMap: | |
| tm instanceof Map | |
| ? tm | |
| : new Map(tm.map((e: { name: string; typeName: string }) => [e.name, e.typeName])), | |
| backfilled: true, | |
| }; | |
| return { typeMap: extracted.symbols.typeMap, backfilled: true }; | |
| if (!extracted || extracted.symbols.typeMap.size === 0) { | |
| return { typeMap: new Map(), backfilled: false }; | |
| } | |
| return { typeMap: extracted.symbols.typeMap, backfilled: true }; |
Note: since this whole backfill path is already marked TODO: Remove once all published native binaries include typeMap extraction (>= 3.2.0), this is low-priority.
There was a problem hiding this comment.
Fixed — the backfillTypeMap guard now checks !extracted || extracted.symbols.typeMap.size === 0 instead of !extracted?.symbols?.typeMap. Since typeMap is always a Map (always truthy), only the .size check is meaningful for detecting empty results. The _typeMapBackfilled flag will now correctly remain false when WASM finds zero type entries.
src/domain/parser.ts
Outdated
| if (extracted?.symbols?.typeMap) { | ||
| const symbols = result.get(relPath); | ||
| if (!symbols) continue; | ||
| symbols.typeMap = | ||
| extracted.symbols.typeMap instanceof Map | ||
| ? extracted.symbols.typeMap | ||
| : new Map( | ||
| // biome-ignore lint/suspicious/noExplicitAny: defensive fallback for legacy array-shaped typeMap | ||
| (extracted.symbols.typeMap as any).map( | ||
| (e: { name: string; typeName: string }) => [e.name, e.typeName], | ||
| ), | ||
| ); | ||
| symbols.typeMap = extracted.symbols.typeMap; | ||
| symbols._typeMapBackfilled = true; | ||
| } |
There was a problem hiding this comment.
?.typeMap truthiness guard sets _typeMapBackfilled for empty Maps
WasmExtractResult.symbols is typed as ExtractorOutput, where typeMap is a required Map<string, TypeMapEntry>. A Map is always truthy — even when empty — so extracted?.symbols?.typeMap is effectively just extracted?.symbols. This means when WASM successfully parses a TS file but finds zero type entries, the block executes, symbols.typeMap is overwritten with the empty Map, and _typeMapBackfilled is set to true even though nothing was actually backfilled.
This is the same invariant addressed for backfillTypeMap in the previous review round (line 452 thread), but the inline backfill path inside parseFilesAuto was not updated consistently.
| if (extracted?.symbols?.typeMap) { | |
| const symbols = result.get(relPath); | |
| if (!symbols) continue; | |
| symbols.typeMap = | |
| extracted.symbols.typeMap instanceof Map | |
| ? extracted.symbols.typeMap | |
| : new Map( | |
| // biome-ignore lint/suspicious/noExplicitAny: defensive fallback for legacy array-shaped typeMap | |
| (extracted.symbols.typeMap as any).map( | |
| (e: { name: string; typeName: string }) => [e.name, e.typeName], | |
| ), | |
| ); | |
| symbols.typeMap = extracted.symbols.typeMap; | |
| symbols._typeMapBackfilled = true; | |
| } | |
| if (extracted?.symbols && extracted.symbols.typeMap.size > 0) { | |
| const symbols = result.get(relPath); | |
| if (!symbols) continue; | |
| symbols.typeMap = extracted.symbols.typeMap; | |
| symbols._typeMapBackfilled = true; | |
| } |
There was a problem hiding this comment.
Fixed — changed the guard from extracted?.symbols?.typeMap (always truthy for a Map) to extracted?.symbols && extracted.symbols.typeMap.size > 0, consistent with the same invariant fixed elsewhere in this PR.
…l guard typeMap is always a Map (truthy even when empty), so the ?.typeMap guard was incorrectly setting _typeMapBackfilled for empty Maps. Check .size > 0 to match the same invariant fixed elsewhere.
1 participant
Summary
parseFileAuto,parseFilesAuto,parseFileIncrementalreturn types fromPromise<any>toPromise<ExtractorOutput | null>/Promise<Map<string, ExtractorOutput>>, restoring compile-time type safety for downstream callerstypeMap(array) toMapinsidepatchNativeResultso the return genuinely conforms toExtractorOutputStmtCache<T>pattern to the two static SQL queries inbuildTestFileIds(module-map.ts)Addresses Greptile review feedback from #558.
Test plan
tsc --noEmitpasses with zero errors