fix(resolver/native/incremental): callerName parity, func-prop cross-file edges, O(n) Phase 8.3f, qualified callerName dispatch - #1383
Conversation
Sorry, something went wrong.
…f scoped typeMap lookup resolveCallTargets in incremental.ts was called without the callerName argument, causing all callerName-dependent resolution paths to be silently skipped during watch-mode (rebuildFile) incremental rebuilds: - class-scoped this.prop typeMap lookup (ClassName.prop seeded by #1323/#1382) - same-class this.method() fallback via callerClass.methodName - Object.defineProperty accessor this-dispatch (callerName:this key) caller.callerName is already computed by findCaller() on the line immediately above the resolveCallTargets call, so this is a one-line fix. The full-build path (build-edges.ts) has always passed callerName correctly; the incremental path was left behind when the scoped-key logic was added. Adds a regression test that drives rebuildFile directly (the JS watch cascade) and asserts that UserService.run → ServiceA.doA is resolved after a watch rebuild — which requires the class-scoped typeMap key UserService.service to be reachable via callerName. Closes #1370
|
Claude finished @carlos-alm's task —— View job PR Review — fix(incremental): pass callerName to resolveCallTargets (#1370)
Root Cause AnalysisProblem Being Solved: During watch-mode incremental rebuilds (rebuildFile), resolveCallTargets was called in incremental.ts:buildCallEdges without the callerName argument. This caused all callerName-gated resolution paths inside resolveByMethodOrGlobal to silently fall through, producing an incomplete call graph after a file change. Full rebuilds were unaffected because build-edges.ts:buildFileCallEdges has always passed caller.callerName correctly. Why This Approach: The fix is correct and minimal — caller is already computed on the line above the resolveCallTargets call, so passing caller.callerName requires no new computation. This mirrors the equivalent call site in build-edges.ts:1102. Risk Assessment: Very low. The change is a one-argument addition to a function call. The parameter is optional (callerName?: string | null), so the previous behavior (omitting it) simply defaulted to undefined, meaning no regression was possible from the fix — it strictly adds resolution coverage. Phase 2: Configuration ChangesNo configuration, lint rules, TypeScript flags, or build settings were modified. ✓ Phase 3: Test IntegrityThe test in tests/integration/issue-1370-incremental-caller-name.test.ts is well-structured:
No assertions are weakened, no tests are skipped, no mocks short-circuit the real path. The makeStmts helper correctly wires getNodeId from src/db/index.js rather than reimplementing it. One minor observation: the getNodeId wrapper in makeStmts (line 79) returns id != null ? { id } : undefined, which correctly adapts the DB function's number | undefined return type to the { id: number } | undefined shape that IncrementalStmts expects. ✓ Phase 4: Residual Divergence Between Incremental and Full-Build PathsThis is not a blocker for this PR but should be tracked: incremental.ts:buildCallEdges (lines 509–553) is still missing two fallback blocks that exist in build-edges.ts:buildFileCallEdges:
This PR's test passes because the class-scoped typeMap lookup (Phase 8.3f, callerName-prefixed key UserService.service) is handled inside resolveCallTargets/resolveByMethodOrGlobal, so the one-line fix is sufficient for the reported issue. However, cases relying on the two fallback blocks above would still diverge after an incremental rebuild. I'd recommend opening a follow-up issue to bring buildCallEdges in incremental.ts to full parity with buildFileCallEdges in build-edges.ts. Backlog Compliance
Final Assessment## Root Cause Analysis **Problem Being Solved**: callerName was not forwarded to resolveCallTargets in the incremental (watch-mode) path, silently skipping class-scoped typeMap lookups and this-method fallbacks during rebuildFile. **Why This Approach**: Minimal, correct fix — caller is already in scope. Mirrors the existing build-edges.ts call site. **Risk Assessment**: Very low. The argument is optional; the fix only enables previously skipped logic. ## Backlog Compliance - Zero-dep: ✓ - Foundation-aligned: ✓ - Problem-fit: 5 — incorrect graph = wrong agent context - Breaking: No - Tier: 1 ## Critical Concerns None for this PR. A follow-up issue should track the remaining incremental/full-build divergence (same-class this.method() fallback and Object.defineProperty accessor fallback in buildCallEdges). ## Final Recommendation - Rating: ⭐⭐⭐⭐ (4/5) - Action: APPROVE - Reasoning: Root cause is correctly identified and precisely fixed. Test coverage is strong — both a targeted regression test and a full-parity check. No config relaxations, no weakened assertions, no new dependencies. One point withheld because the PR description mentions two other callerName- dependent paths (same-class this.method() fallback, Object.defineProperty accessor fallback) that are still absent from the incremental path — the fix is necessary but the incremental path is not yet at full parity with the full-build path. That gap should be tracked in a follow-up issue. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Tracked the remaining incremental/full-build divergence (same-class this.method() fallback + Object.defineProperty accessor fallback) in #1384 as recommended. |
Sorry, something went wrong.
Greptile SummaryThis PR delivers five targeted bug fixes to the native resolver path: missing callerName in the incremental rebuild path, dropped cross-file edges for caller-only files in the func-prop post-pass, an O(n³)→O(n) rewrite of the Phase 8.3f object-rest dispatch, a namespace-qualified callerClass extraction fix, and a funcStack correction for assignment_expression nodes in for-of extraction.
Confidence Score: 5/5All five fixes are narrowly scoped, semantically correct, and guarded by new unit and integration tests that exercise both the happy path and cross-class false-positive prevention. Each change addresses a well-understood silent data-loss bug: the callerName omission in incremental.ts is a one-line additive fix; the second WASM pass correctly excludes already-parsed protoFiles via a Set guard; the O(n) rewrite in points-to.ts is semantically identical to the old triple-nested loop; and the double-lastIndexOf extraction in call-resolver.ts is backward-compatible for simple Class.method names. Test coverage is thorough, including parity checks between full and incremental builds. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant O as runPostNativePrototypeMethods
participant DB as SQLite DB
participant FS as File System
participant W as WASM Parser
O->>FS: filter jsFiles to protoFiles
O->>W: parseFilesWasmForBackfill(protoFiles)
O->>DB: batchInsertNodes(newNodeRows)
Note over O: Build newMethodSuffixes, compile escaped regexes
O->>FS: scan remaining jsFiles for suffix matches
FS-->>O: callerCandidateAbs[]
O->>W: parseFilesWasmForBackfill(callerCandidateAbs)
Note over O: mergedWasmResults = wasmResults + callerWasmResults
loop for each file in mergedWasmResults
O->>O: resolveByMethodOrGlobal(call, caller.callerName)
alt targets empty AND receiver not this/super/self
O->>O: fallback qualified-name lookup filtered by newNodeIds
end
O->>DB: insert edge if newNodeIds.has(target.id)
end
Reviews (13): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
Sorry, something went wrong.
Codegraph Impact Analysis24 functions changed → 39 callers affected across 11 files
|
Sorry, something went wrong.
The `runPostNativePrototypeMethods` post-pass only WASM-parsed files that
matched the func-prop definition regex (protoFiles), so any file that only
called `f.method()` without defining `fn.x = function(){}` was absent from
`wasmResults` and its call edges to newly-inserted nodes were silently dropped.
Two changes fix this:
1. Second WASM pass after node insertion: text-searches remaining JS/TS files
for calls matching the newly-inserted method name suffixes, then
WASM-parses those caller-only files.
2. Direct receiver.method lookup fallback in the resolution loop: when
`resolveByMethodOrGlobal` returns empty for a non-this receiver (common in
plain JS where the caller file has no typeMap entry for the receiver),
tries `lookup.byName('f.method')` scoped to newly-inserted nodes to resolve
the edge without typeMap data.
Both paths are bounded to newly-inserted func-prop node IDs so no duplicate
edges are emitted for calls the Rust engine already resolved.
Closes #1371
…hase 8.3f
The Phase 8.3f block in buildPointsToMap had three nested for-of loops —
O(|objectRestParamBindings| × |paramBindings| × |objectPropBindings|) — with
equality guards discarding most pairs only after iterating all combinations.
Fix: pre-index both input arrays before the outer loop:
- paramBindings → Map<"callee::argIndex", argName[]> (O(|paramBindings|) build)
- objectPropBindings → Map<objectName, {propName, valueName}[]> (O(|objectPropBindings|) build)
The outer loop now does two O(1) map lookups per rest-binding, reducing the
overall complexity to O(n) where n ≈ sum of matched argName × matched prop counts.
Both indices are built inside the existing guard so they're only allocated when
all three binding arrays are present.
Adds 7 unit tests for Phase 8.3f covering: basic seeding, multiple props, value
filter (not in definitionNames/importedNames), multiple callers, imported values,
absent bindings, and a 50-binding correctness regression guard.
Closes #1374
| const matchesAny = [...newMethodSuffixes].some((m) => | ||
| new RegExp(`\\.${m}\\s*\\(`).test(content), | ||
| ); |
There was a problem hiding this comment.
The method suffix is interpolated directly into new RegExp() without escaping. $ is a valid JavaScript identifier character (e.g. f.$init = function(){}, f.fn$), and in a regex pattern it is the end-of-string anchor. A suffix like $init produces the pattern \.$init\s*\(, which never matches the literal text .$init( anywhere in a file, silently dropping all caller candidates for that method. Since the regex is rebuilt on every relPath iteration and the same bad pattern fires each time, the catch block suppresses the failure for each file individually but the end result is that the entire second pass produces zero candidates whenever any newly-inserted method name starts with or contains $.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in commit 0eb7a5a. The method suffix is now escaped before being interpolated into new RegExp(), so method names containing $ or other regex metacharacters are handled correctly: const escaped = m.replace(/[.*+?^${}()|[\]\\]/g, '\\$&').
Sorry, something went wrong.
…od suffix search (#1383)
Sorry, something went wrong.
…ame-class dispatch (#1385) When callerName is 'Namespace.ClassName.method', lastIndexOf('.') yielded 'Namespace.ClassName' as the class prefix — but symbols are stored under bare 'ClassName', so the lookup silently failed. Use the segment immediately before the method name instead (second lastIndexOf walk).
#1383) Remove redundant call.receiver guard after the receiver-null early continue, and pre-compile method suffix RegExp objects once before the file scan loop to avoid re-compiling up to newMethodSuffixes.size patterns per file.
|
Addressed the two items from the Greptile review summary (commit 69bd0bc):
|
Sorry, something went wrong.
Sorry, something went wrong.
…SpreadForOfWalk (#1373) Add an `assignment_expression` branch in `extractSpreadForOfWalk` that detects `obj.method = function() { ... }` assignments and pushes `obj.method` onto `funcStack`. This ensures for-of loops inside the body emit the correct `enclosingFunc` so `buildPointsToMap` seeds the right PTS key and `build-edges.ts` can match callback call edges.
…-javascript more1 was hand-authored to test for-of/Set/Array.from/spread pts patterns, not imported from Jelly's micro-test corpus. Keeping it in jelly-micro/ was misleading and inflated the Jelly fixture count. Moving it to fixtures/javascript/ caused 1.0 precision failures because the pts resolver pools function literals across sections of the same file. Splitting each pattern into its own file (for-of.js, set-iter.js, array-from.js, spread.js) eliminates intra-file cross-pollination — all 13 expected edges resolve at 100% precision and 100% recall. Adds pts-for-of, pts-set, pts-array-from, pts-spread to TECHNIQUE_MAP and a pts-javascript threshold (precision 1.0, recall 0.9) to the benchmark. Closes #1388
The more1 hand-authored fixture has been moved to tests/benchmarks/resolution/fixtures/pts-javascript/ (previous commit).
The more1 hand-authored fixture has been moved to tests/benchmarks/resolution/fixtures/pts-javascript/ (see previous commit).
|
Pushed 3 additional commits that address issue #1388: the more1 hand-authored fixture (array iteration patterns) has been moved from jelly-micro/ to a new pts-javascript/ fixture directory. The fixture is now split into 4 focused single-pattern files (for-of.js, set-iter.js, array-from.js, spread.js) to eliminate intra-file cross-pollination. All 13 expected edges pass at 100% precision/100% recall. The Windows CI failure on the previous CI run was a transient HTTP 504 from GitHub's CDN for tree-sitter-windows-x64.gz (not caused by this PR). New CI run triggered by this push. Also filed #1418 for a pre-existing C# build-parity test failure (native vs WASM role/edge divergence on main) that is unrelated to this PR. |
Sorry, something went wrong.
Sorry, something went wrong.
Summary
Five fixes in the native/resolver path, all identified in Greptile reviews:
Fix 1 (#1370): pass callerName in incremental path
Fix 2 (#1371): second WASM pass for caller-only files in func-prop post-pass
Fix 3 (#1374): O(n³) → O(n) for Phase 8.3f in buildPointsToMap
Fix 4 (#1385): extract bare class name from qualified callerName in same-class dispatch
Test plan
Closes #1370
Closes #1371
Closes #1374
Closes #1385