fix(parity): resolve C# static receiver calls in WASM engine (#1372)#1395
Conversation
The WASM resolver had no fallback for calls where the receiver is a class
name used directly (e.g. `Validators.IsValidEmail(...)`). Static class names
never appear in variable declarations, so typeMap has no entry for them.
The old code fell through and returned an empty target set.
Add a fallback in resolveByMethodOrGlobal: when typeName is null (no typeMap
entry for the receiver), try looking up `${receiver}.${method}` directly in
the symbol DB. This catches static dispatch where the receiver IS the class
name, matching what the native engine already produces. The kind='method'
filter keeps it scoped to genuine method calls.
Fixes build-parity test failures for the csharp Repository fixture:
- Program.Main → Validators.IsValidEmail
- Program.RunWithValidation → Validators.ValidateUser
- UserService.AddUser → Validators.ValidateUser
|
Claude finished @carlos-alm's task —— View job PR Review — fix(parity): resolve C# static receiver calls in WASM engine (#1372)
Phase 1: Root Cause Analysis ✓The root cause is correctly identified and the fix is genuine — not a symptom masker. The WASM resolver's The fix adds a This is fixing the correct engine (WASM) to match native output. No workarounds. Phase 2: Configuration Change Detection ✓No linting, build, or TypeScript config changes. No biome rules relaxed, no timeouts increased, no coverage thresholds modified. Clean. Phase 3: Test Integrity —
|
Greptile SummaryThis PR adds a regression test for issue #1372 (C# static receiver call edges missing in the WASM engine) and pins an existing prototype-method-resolution integration test to the WASM engine explicitly.
Confidence Score: 5/5Safe to merge — both changes are test-only and introduce no production code modifications. All changes are in integration tests. The new regression test is well-structured with proper initialisation, timeout, and cleanup. The engine pin in the prototype-method-resolution test formalises already-implicit WASM behaviour and is partially backstopped by build-parity.test.ts which exercises both engines on the benchmark fixtures. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildGraph - engine: wasm] --> B[WASM parser\nproduces call sites]
B --> C[resolveByMethodOrGlobal]
C --> D{typeMap has entry\nfor receiver?}
D -- Yes --> E[typed method lookup\ntypeName.callName]
D -- No --> F{inline new\nexpression?}
F -- Yes --> E
F -- No --> G[static fallback\neffectiveReceiver.callName\nkind = method or function]
G --> H[emit call edge\ne.g. Program.Main → Validators.IsValidEmail]
E --> H
Reviews (8): Last reviewed commit: "Merge branch 'main' into fix/csharp-stat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!typeName) { | ||
| const staticCandidates = lookup | ||
| .byName(`${effectiveReceiver}.${call.name}`) | ||
| .filter((n) => n.kind === 'method'); | ||
| if (staticCandidates.length > 0) return staticCandidates; | ||
| } |
There was a problem hiding this comment.
typeName is null for two distinct reasons: the receiver is a static class name (the intended case), or the receiver is an instance variable that simply wasn't seeded into typeMap. In the second case the fallback performs a DB lookup of effectiveReceiver.callName — if a class in the codebase happens to share the same identifier as the unresolved variable, a spurious call edge is created. Languages where variable names collide with class names (e.g. lowercase-named Python/Go classes, or a TypeScript variable named cache when a cache module exports a get method) are affected. The typed branch at line 94 is similarly unguarded but only runs when typeName was positively resolved from typeMap, so that gap is narrower.
There was a problem hiding this comment.
Acknowledged. The concern is valid — typeName being null covers both intended (static class name) and unintended (unresolved instance variable) cases, and the DB lookup without a confidence filter could produce false edges when two unrelated classes share a method name.
Adding confidence scoring to the static fallback (like all other byName() paths use) is the right long-term fix, but it requires care: static dispatch is typically cross-file by design, so a 0.5 confidence floor could suppress legitimate edges for callers in different directories. That analysis goes beyond this PR's scope.
Filed #1398 to track this follow-up. The current kind='method' guard plus the fully-qualified Receiver.Method key makes false positives unlikely in practice.
| let tmpDir: string; | ||
| let edges: Array<{ src: string; tgt: string; kind: string }>; |
There was a problem hiding this comment.
edges to [] ensures the it blocks produce a clear assertion failure ("expected [] to contain...") rather than a cryptic "Cannot read properties of undefined" when beforeAll rejects.
| let tmpDir: string; | |
| let edges: Array<{ src: string; tgt: string; kind: string }>; | |
| let tmpDir: string; | |
| let edges: Array<{ src: string; tgt: string; kind: string }> = []; |
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 — edges is now initialised to [] in commit f367fbd.
Codegraph Impact Analysis1 functions changed → 14 callers affected across 5 files
|
… Array.from (JS) Fixes more1 jelly-micro benchmark from 0% → 100% recall (10/10 named edges). The PTS resolution for for-of, Set+for-of, Array.from, and spread patterns was already fully implemented. The 0% recall was caused by a naming mismatch in the fixture: _iterPlain/_iterSet (underscore-prefixed) vs the expected-edges.json which referenced iterPlain/iterSet (without prefix). Renaming the fixture functions to match the expected edges exposes all four PTS patterns resolving correctly. Closes #1378
…Set, and Array.from (JS)" This reverts commit 70236a5.
|
Responding to the Claude review concerns: Concern 1 — Missing confidence filter on static fallback: Concern 2 — Regression test fixture diverges from PR description: |
The test was using auto engine (native-preferred), causing it to pick the published npm native binary which predates the prototype-method fixes. WASM correctly extracts Dog.prototype.bark and resolves all call edges. Fixes #1381
|
Merged with main and removed a duplicate static-receiver fallback block that had been superseded by the broader version landed in #1382. The PR's original |
1 participant
Summary
Edges fixed
These edges are from the benchmark fixture in `tests/benchmarks/resolution/fixtures/csharp/`:
Note: the regression test uses a separate minimal fixture (different method names) that directly exercises the same code path.
Test plan
Closes #1372