feat: Phase 5 type resolution — chained calls, pattern matching, class-as-receiver#315
feat: Phase 5 type resolution — chained calls, pattern matching, class-as-receiver#315magyargergo merged 4 commits intomainfrom
Conversation
…s-as-receiver, code review fixes Phase 5.1: Chained method call resolution (depth-capped at 3) - resolveChainedReceiver() resolves a.getUser().save() by walking the chain and looking up intermediate return types from the SymbolTable - extractReceiverNode() + extractCallChain() shared in utils.ts - receiverCallChain on ExtractedCall for worker path parity - MAX_CHAIN_DEPTH=3 enforced in both extraction and resolution Phase 5.2: Pattern matching binding extractors - PatternBindingExtractor type added to LanguageTypeConfig - declarationTypeNodes map tracks original type AST nodes for generic unwrapping - Rust: if let Some(x)/Ok(x) unwrapping with extractGenericTypeArgs - Java: instanceof pattern variables (Java 16+) - C#: is-pattern disambiguation fixture (already working via extractDeclaration) Phase 5.5d: Python standalone type annotations (name: str) - expression_statement with type child now captured in DECLARATION_NODE_TYPES Phase 5.5e: ReceiverKey collision fix for overloaded methods - receiverKey preserves @StartIndex to prevent same-name method collisions - lookupReceiverType does prefix scan with ambiguity refusal Class-as-receiver for static method calls (#289) - UserService.find_user() now resolves via ctx.resolve() tiered lookup - Respects import scoping — no false positives from unrelated packages Code review fixes: - Extracted CALL_EXPRESSION_TYPES + extractCallChain to utils.ts (eliminated duplication) - Converted resolveChainedReceiver from recursion to loop (no exposed depth param) - Added depth cap to extractReturnTypeName (defense against nested wrapper types) - Replaced lookupFuzzy with ctx.resolve for class-as-receiver (architecturally consistent) Closes #289 Test coverage: 6 new fixtures, 12+ new unit tests, 7 new integration test suites
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Phase 5 Type Resolution — Implementation NotesWhat's newChained method calls ( Pattern matching (Rust Class-as-receiver ( Architecture decisions
Related issues
|
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 2351 tests passed 17 test(s) skipped — expand for detailsIntegration:
Code CoverageCombined (Unit + Integration)
Coverage breakdown by test suiteUnit Tests
Integration Tests
📋 View full run · Generated by CI |
|
@claude ultra-think while identifying per-language gaps for this pull request and verify those findings. Act as a senior compiler font-end engineer and review this change from that perpective and architevtural fit. |
|
Claude finished @magyargergo's task in 6m 56s —— View job Phase 5 Type Resolution — Compiler Front-End ReviewTasks
Overall AssessmentThis is a well-structured pass over a genuinely hard problem — heuristic type inference without a real type system. The architecture choices are sound (tiered resolution, scope-aware TypeEnv, depth-capped chain walking), the code review fixes were applied, and the test matrix is solid for the languages covered. Below are the findings by feature area, with language-specific gaps verified against the actual AST field names in each tree-sitter grammar. 1. Chained Call Resolution
The loop builds Verified language coverage via field names:
Ruby gap ( const funcNode = current.childForFieldName?.('function')
?? current.childForFieldName?.('name');returns
const candidates = ctx.symbols.lookupFuzzy(name);
const symDef = candidates.find(c => c.nodeId === resolved.nodeId);This is a secondary lookup by nodeId (to retrieve Missing integration tests for: Go, Python, PHP, C# chained calls. The implementation logic covers them (verified above) but they are untested. 2. Pattern Matching ExtractorsRust Rust Gap — if (wrapperName !== 'Some' && wrapperName !== 'Ok') return undefined;
Java C# Gap — C# switch expression patterns:
3. Class-as-Receiver for Static CallsImplementation (call-processor.ts:265–272): const typeResolved = ctx.resolve(receiverName, file.path);
if (typeResolved && typeResolved.candidates.some(
d => d.type === 'Class' || d.type === 'Interface' || d.type === 'Struct',
)) {
receiverTypeName = receiverName;
}Correct design — tiered resolution, no raw fuzzy lookup. Gap — Asymmetry between sequential and worker path: 4. ReceiverKey Collision FixCorrectness:
5. Python Standalone Annotations
One nuance: Python 3.10+ 6. Architectural Observations from a Compiler FE PerspectiveSingle-pass, no fixpoint:
Worker/sequential parity: 7. Per-Language Gap Summary
SummaryApprove with notes. The core features are correct and well-tested for the primary languages (TS, Java, Rust, C#). The Must-fix before merge:
Recommended follow-ups (Phase 6 candidates):
|
Review Response — Compiler FE Review FindingsVerified findings against the codebase. Three implemented, four deferred. Implemented1. Ruby 2. Rust 3. Deferred (valid, different scope)
All tests green. Build clean. |
Address three per-language gaps identified in Phase 5 code review: - Ruby: add `method`/`receiver` field fallbacks to extractCallChain (tree-sitter-ruby uses different field names than other grammars) - Rust: handle `Err(e)` pattern binding via typeArgs[1] from Result<T,E> - Enum: include Enum type in class-as-receiver filter (both paths) Integration tests added for all three fixes.
|
@claude ultra-think while identifying per-language gaps for this pull request and verify those findings. Act as a senior compiler font-end engineer and review this change from that perpective and architectural fit. Also check out the previous comments. |
|
Claude finished @magyargergo's task in 5m 43s —— View job Phase 5 Type Resolution — Deep Compiler FE Review (Follow-up)Tasks
Deep Compiler FE ReviewThis is the follow-up pass, building on the previous review and verifying all claimed fixes. All source files were read directly. Verification of the 3 Claimed Fixes1. Ruby
const funcNode = current.childForFieldName?.('function')
?? current.childForFieldName?.('name')
?? current.childForFieldName?.('method'); // Ruby `call` node
if (!innerReceiver && current.type === 'call') {
innerReceiver = current.childForFieldName?.('receiver');
}The two-pronged fix is correct. For Ruby's 2. Rust
if (wrapperName !== 'Some' && wrapperName !== 'Ok' && wrapperName !== 'Err') return undefined;
const argIndex = wrapperName === 'Err' ? 1 : 0;Extraction of 3.
New Findings Not in Previous ReviewThese are issues discovered in this review pass by tracing execution paths for edge cases not covered by the existing test matrix. Finding 1 — HIGH: Worker/serial asymmetry in chain base-type resolutionLocation: The serial path resolves the chain base receiver through typeEnv at line 284: let baseType = extracted.baseReceiverName && typeEnv
? typeEnv.lookup(extracted.baseReceiverName, callNode)
: undefined;The worker path has a comment describing the same intent ( receiverName = extracted.baseReceiverName;
// receiverTypeName NOT updated — base receiver type not passed to processCallsFromExtractedConsequence: For chained calls where the base receiver is an explicitly-typed parameter or annotation-declared variable (
The serial path works because it queries typeEnv directly. This asymmetry means the feature works on small or sequential-processed codebases but silently degrades on worker-processed ones. Minimal fix: In receiverTypeName = typeEnv.lookup(receiverName, callNode);This mirrors what the serial path does at line 284. Fix this → Finding 2 — MEDIUM: Sequential path misses class-as-receiver for chain baseLocation: The sequential path's chain branch: let baseType = extracted.baseReceiverName && typeEnv
? typeEnv.lookup(extracted.baseReceiverName, callNode)
: undefined;
if (!baseType && extracted.baseReceiverName && verifiedReceivers.size > 0) {
baseType = lookupReceiverType(verifiedReceivers, funcName, extracted.baseReceiverName);
}
// MISSING: ctx.resolve fallback for class names
receiverTypeName = resolveChainedReceiver(extracted.chain, baseType, file.path, ctx);If the chain base is a class name directly — e.g., The worker path handles this correctly because Step 1b (class-as-receiver) runs before Step 2 and sees Fix: After the if (!baseType && extracted.baseReceiverName) {
const cr = ctx.resolve(extracted.baseReceiverName, file.path);
if (cr?.candidates.some(d =>
d.type === 'Class' || d.type === 'Interface' || d.type === 'Struct' || d.type === 'Enum'
)) {
baseType = extracted.baseReceiverName;
}
}Finding 3 — LOW: Misleading comment at parse-worker.ts:1021-1024The comment Finding 4 — OBSERVATION:
|
| Language | Chain Calls | Pattern Matching | Class-as-Receiver | Notes |
|---|---|---|---|---|
| TypeScript | ✅ tested | (structural) | ✅ tested | |
| Java | ✅ tested | ✅ instanceof | ✅ + Enum ✅ | switch-expr patterns deferred |
| Rust | ✅ tested | ✅ Some/Ok/Err ✅ | ✅ | |
| C# | ✅ unit | ✅ is-pattern | ✅ | switch-expr deferred |
| Python | ✅ unit | walrus ✅; match/case deferred | ✅ | standalone annotations ✅ new |
| Go | ✅ unit | n/a | ✅ | |
| PHP | ✅ unit | n/a | ✅ repro verified | |
| Ruby | ✅ integration added | n/a | ✅ | method/receiver fields fixed ✅ |
| Kotlin | ✅ unit | n/a | ✅ | when-expression patterns deferred |
| Swift | ❌ skipped | n/a | ✅ | all 17 tests remain skipped |
| All langs | Finding 1 |
Architectural Fit Assessment
PatternBindingExtractor contract: The interface signature at types.ts:51-56 requires declarationTypeNodes as a positional parameter regardless of whether the language uses it. Java's extractJavaPatternBinding only uses node and ignores the other three parameters. A future Kotlin when-expression extractor would also ignore declarationTypeNodes. Consider making declarationTypeNodes optional in the type signature (with a default empty map at the call site). Current approach forces all future implementors to accept a parameter they won't use.
resolveChainedReceiver's lookupFuzzy secondary scan: As noted in the previous review, the lookupFuzzy(name) call at line 418 is a nodeId-scan idiom to retrieve SymbolDefinition.returnType after resolveCallTarget finds the target. resolveCallTarget returns a ResolveResult (nodeId + confidence) but not the SymbolDefinition. The clean fix is to return SymbolDefinition | null from resolveCallTarget directly — but that's a refactor touching the type signature. As deferred tech debt, it's acceptable but it means every chain step makes two O(n) scans over the symbol table.
Dual-path maintenance: Finding 1 shows this risk is already materializing — the serial and worker paths diverged on the typeEnv lookup for chain base receivers. Given that these two paths implement the same logic via different mechanisms, consider adding an integration test that specifically exercises the worker path (large enough repo to trigger the worker) for chained calls on typed parameters. The current ruby-chain-call fixture uses constructor binding (svc = UserService.new), which works in both paths.
Summary
Approve — the three claimed fixes are correctly implemented and the CI is green. Two new gaps were found in the chain resolution paths that are testable but not yet tested:
- Finding 1 (worker path, typed parameter chain base) — recommend fixing before merge. It's a 1-line addition in parse-worker.ts that mirrors the existing serial-path logic.
- Finding 2 (serial path, class-name chain base) — recommend fixing before merge. It's a 5-line addition mirroring Step 1b in the worker path.
- Finding 3 (misleading comment) — minor, fix whenever.
· Branch
#315) - Worker path: add typeEnv.lookup for chain base receiver after extraction (typed parameters like `fn process(svc: &UserService)` were silently lost) - Serial path: add ctx.resolve class-as-receiver fallback for chain base (class-name chains like `UserService.find_user().save()` failed) - Fix misleading comment in parse-worker.ts that described unimplemented logic - Integration tests: typed-parameter chain, static class-name chain
|
@claude ultra-think while identifying per-language gaps for this pull request and verify those findings. Act as a senior compiler font-end engineer and review this change from that perpective and architectural fit. Also check out the previous comments. |
|
Claude finished @magyargergo's task in 8m 43s —— View job Phase 5 Type Resolution — Third Compiler FE ReviewTasks
Verification of Prior Review FindingsThree commits have landed since the initial PR:
Round 1 findings — all ✅ verified correct:
Round 2 findings — all ✅ verified or correctly deferred:
New FindingsThe two prior reviews verified correctness of the new features and caught implementation gaps. This pass looks further: verifying the chain extraction layer for every language that uses Finding 1 — HIGH: Kotlin chain call extraction is silently brokenLocation: The function extracts intermediate method names by looking up named fields on the call node: const funcNode = current.childForFieldName?.('function')
?? current.childForFieldName?.('name')
?? current.childForFieldName?.('method');For Kotlin's Execution trace for
Why tests don't catch this: The Kotlin "chain" integration tests ( Fix: In // Kotlin/Swift: call_expression exposes callee as firstNamedChild (no 'function' field).
// navigation_expression: method name is in the lastNamedChild (navigation_suffix → simple_identifier).
if (!funcNode && current.type === 'call_expression') {
const callee = current.firstNamedChild;
if (callee?.type === 'navigation_expression') {
const suffix = callee.lastNamedChild;
if (suffix?.type === 'navigation_suffix') {
methodName = suffix.lastNamedChild?.text;
innerReceiver = callee.children.find(c => c.isNamed && c.type !== 'navigation_suffix') ?? null;
}
}
}Finding 2 — MEDIUM: "chain" integration test coverage conflation (test quality)All prior reviews described Kotlin chain call support as "✅ unit" or "✅ (unit only)". This assessment was based on unit tests that construct A comprehensive test matrix should include:
Similarly, the Finding 3 — LOW (observation):
|
| Language | Chain Calls | Pattern Matching | Class-as-Receiver | Annotations |
|---|---|---|---|---|
| TypeScript | ✅ integration | (structural) | ✅ integration | ✅ existing |
| Java | ✅ integration | ✅ instanceof integration | ✅ + Enum ✅ | ✅ |
| Rust | ✅ unit | ✅ Some/Ok/Err ✅ | ✅ | ✅ |
| C# | ✅ unit | ✅ is-pattern | ✅ | ✅ |
| Python | ✅ unit | walrus ✅ | ✅ | ✅ new |
| Go | ✅ unit | n/a | ✅ | n/a |
| PHP | ✅ unit | n/a | ✅ repro verified | n/a |
| Ruby | ✅ integration added | n/a | ✅ | n/a |
| Kotlin | n/a | ✅ | ✅ | |
| Swift | ❌ all tests skipped | n/a | ✅ | n/a |
Architectural Assessment
Chain resolution correctness: The dual-path architecture (serial AST + worker serialization) is now correctly symmetric after c8f8584. The comment fix in parse-worker.ts is particularly valuable — the previous comment was actively misleading. The ctx.resolve fallback for chain bases is consistent with the non-chain class-as-receiver path.
lookupFuzzy in resolveChainedReceiver: The secondary scan at call-processor.ts:427 was flagged in Round 2 as tech debt. Still present. Still a 2× symbol-table scan per chain step. The cleanest fix (returning SymbolDefinition from resolveCallTarget) remains deferred. Acceptable as-is.
PatternBindingExtractor signature overhead: The interface requires declarationTypeNodes as a positional parameter regardless of whether the language uses it. Java's extractJavaPatternBinding signature accepts all four params and ignores three. Low impact, but a future (node: SyntaxNode, options: PatternBindingOptions) refactor would be cleaner.
Depth-cap correctness: MAX_CHAIN_DEPTH = 3 is applied in extractCallChain. extractReturnTypeName has a depth cap of 10 for recursive unwrapping. These are independent guards with no interaction — correct.
Summary
Approve with two recommendations:
-
Finding 1 (Kotlin chain extraction) — recommend fixing before merge. The 10-line fallback is well-scoped, follows the existing pattern in
scanKotlinConstructorBinding, and would be accompanied by akotlin-chain-callfixture. Without this, Kotlin users with fluent APIs get silent non-resolution with no error signal. -
Finding 3 (
createClassNameLookup) — minor, can be follow-up. Two-character addition that makes the codebase internally consistent.
The three prior-review fixes are all correct. The dual-path parity work in c8f8584 is clean and architecturally sound. The test infrastructure (fixtures + integration suites) now covers the primary languages comprehensively.
· Branch
…315) - Kotlin: extractCallChain now handles navigation_expression → navigation_suffix AST structure (Kotlin's call_expression has no 'function' field) - createClassNameLookup: include Enum and Struct alongside Class for consistent constructor recognition in extractInitializer - Integration test: kotlin-chain-call fixture verifying svc.getUser().save()
|
@claude ultra-think while identifying per-language gaps for this pull request and verify those findings. Act as a senior compiler font-end engineer and review this change from that perpective and architectural fit. Also check out the previous comments. |
|
Claude finished @magyargergo's task in 7m 25s —— View job Phase 5 Type Resolution — Fourth Compiler FE Review
|
| Language | Chain Calls | Pattern Matching | Class-as-Receiver | Annotations |
|---|---|---|---|---|
| TypeScript | ✅ integration | (structural) | ✅ integration | ✅ |
| Java | ✅ integration | ✅ instanceof + integration | ✅ + Enum ✅ | ✅ |
| Rust | ✅ unit | ✅ Some/Ok/Err ✅ | ✅ | ✅ |
| C# | ✅ unit | ✅ is-pattern | ✅ | ✅ |
| Python | ✅ unit | walrus ✅ | ✅ | ✅ new |
| Go | ✅ unit | n/a | ✅ | n/a |
| PHP | ✅ unit (no integration) | n/a | ✅ verified | n/a |
| Ruby | ✅ integration ✅ | n/a | ✅ | n/a |
| Kotlin | ✅ integration ✅ fixed | n/a (when-expr Phase 6) | ✅ + Enum/Struct ✅ | ✅ |
| Swift | ❌ all tests skipped | n/a | ✅ | n/a |
Architectural Assessment (Final)
Four-round convergence: The three prior reviews found and fixed five HIGH/MEDIUM issues. This pass finds only LOW/OBSERVATION items. The codebase is in a clean state.
Dual-path symmetry: After c8f8584, the sequential and worker paths are fully symmetric for chain resolution (base typeEnv lookup, base class-as-receiver, chain walking). The Round 3 Kotlin fix (bd47b0c) applies equally to both paths via the shared extractCallChain function — no separate worker fix was needed since both paths call the same extractCallChain utility.
navigation_suffix/navigation_expression design: The Kotlin fix is correctly conservative (guarded by current.type === 'call_expression' AND callee?.type === 'navigation_expression' AND suffix?.type === 'navigation_suffix'). It does not fire for TS/JS/Go/Rust/Python/Ruby since those all produce a non-null funcNode from the first three field lookups. Swift shares Kotlin's call_expression type and navigation_expression structure, so the fix would also unblock Swift chained calls — but Swift tests remain skipped.
lookupFuzzy residual tech debt: resolveChainedReceiver at call-processor.ts:427 still does a secondary lookupFuzzy scan to retrieve returnType after resolveCallTarget finds the target. This 2× symbol table scan per chain step is the same tech debt noted in Round 1. Correctly deferred; clean fix requires returning SymbolDefinition from resolveCallTarget.
Summary
Approve. All three Round 3 findings are correctly implemented. No new HIGH or MEDIUM issues found. The Kotlin chain call fix is architecturally sound, correctly conservative, and does not regress any other language. The integration test provides a proper disambiguation proof.
Phase 6 recommendations (not blocking):
- Add
Enum/StructtoverifyConstructorBindingsisClasscheck for internal consistency (2-char addition) - Add a depth-2 Kotlin chain call fixture (
svc.getUser().format().save()) - PHP chain call integration test
lookupFuzzy→lookupByIdrefactor inresolveChainedReceiver
· Branch

Summary
Phase 5 extends the type resolution system with four major capabilities:
Chained method call resolution —
svc.getUser().save()now resolves by walking the chain and looking up intermediate return types from the SymbolTable. Depth-capped at 3 to prevent fluent API explosion. Both sequential and worker paths supported viareceiverCallChainonExtractedCall.Pattern matching binding extractors — Rust
if let Some(user) = optandif let Ok(user) = resultextract typed bindings viaextractGenericTypeArgs. Java 16+instanceofpattern variables (obj instanceof User user) also supported. Infrastructure:PatternBindingExtractortype onLanguageTypeConfig+declarationTypeNodesmap for generic type arg access.Class-as-receiver for static method calls —
UserService.find_user()now resolves because the pipeline recognizesUserServiceas a class name viactx.resolve()tiered lookup. Works across all languages. Fixes Python: context() and impact() return empty for classes that use static methods #289.ReceiverKey collision fix — overloaded methods with the same name (e.g.,
User.save()andRepo.save()) no longer collide in the receiver map.receiverKeypreserves@startIndex.Python standalone type annotations —
name: str(without default value) now captured by TypeEnv.Code review fixes applied
CALL_EXPRESSION_TYPES+extractCallChaintoutils.ts(eliminated duplication between call-processor and parse-worker)resolveChainedReceiverfrom recursion to loopextractReturnTypeName(defense-in-depth)lookupFuzzywithctx.resolvefor class-as-receiver (architecturally consistent)Test plan
tsc --noEmitcleanFoo::create()+$foo->greet()both resolve correctlyCloses
find_user()now resolves via class-as-receiver)Deferred to Phase 6
declarationTypeNodesinfrastructure is in placeinstanceof/isin if-branches) — downgraded per simplicity reviewa.b.save()) — requires field-type metadata in schema