fix(native): persist this/super dispatch via hybrid WASM post-pass#1337
fix(native): persist this/super dispatch via hybrid WASM post-pass#1337carlos-alm wants to merge 6 commits into
Conversation
…1326) The native orchestrator resolves typed receiver calls but does not persist raw unresolved call site receiver info (this/super) to the DB, so runPostNativeCha could not resolve this.method() or super.method() calls. Add runPostNativeThisDispatch: after the Rust pipeline completes, WASM-re- parses JS/TS/TSX files to collect call sites with this/super receivers, then resolves them through the DB class hierarchy (extends edges) using the existing resolveThisDispatch function. Only runs when extends edges exist. Removes the skipIf(engine === 'native') guards on the this-dispatch and super-dispatch integration tests — both engines now produce identical edges for ConcreteWorker.doWork → ConcreteWorker.prepare and Lion.speak → Animal.speak. The two CHA transitive skips remain (pending abstract_class_ declaration fix in a future native binary). Closes #1326
Greptile SummaryThis PR closes the native orchestrator's
Confidence Score: 5/5Safe to merge — the post-pass is additive and gated by a fast extends-edge check, so projects without inheritance are completely unaffected. The implementation is tightly scoped: it only re-parses files already involved in the DB class hierarchy, deduplicates via the seen set, and runs inside an existing transaction. Both correctness bugs flagged in the previous review round have been resolved. The integration tests now cover both engines unconditionally. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as tryNativeOrchestrator
participant C as runPostNativeCha
participant D as runPostNativeThisDispatch
participant W as parseFilesWasmForBackfill
participant DB as SQLite DB
T->>C: run CHA post-pass (interface dispatch)
C->>DB: insert cha call edges
T->>D: run this/super dispatch post-pass
D->>DB: SELECT extends edges - build parents map
D->>DB: SELECT files in extends hierarchy
D->>DB: seed seen set from existing call edges
D->>W: WASM-re-parse inheritance-related files
W-->>D: Map of relPath to ExtractorOutput with call sites
loop each call site with this or super receiver
D->>DB: findCallerByLineStmt innermost method at line
D->>D: resolveThisDispatch walk parents map
D->>DB: byName lookup for qualified method
D->>D: computeConfidence minus CHA_DISPATCH_PENALTY
end
D->>DB: batchInsertEdges transaction
D->>D: free WASM parse trees
T->>T: backfillEdgeTechniques labels all new edges
T-->>T: formatNativeTimingResult includes thisDispatchMs
Reviews (7): Last reviewed commit: "fix(native): document incremental limita..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Find the innermost containing method/function for a call at `line` in `file`. | ||
| // NULL end_line sorts last in SQLite ASC → only selected when no bounded node exists. | ||
| const findCallerByLineStmt = db.prepare(` | ||
| SELECT id, name FROM nodes | ||
| WHERE file = ? AND kind IN ('method', 'function') | ||
| AND line <= ? AND (end_line IS NULL OR end_line >= ?) | ||
| ORDER BY (end_line - line) ASC | ||
| LIMIT 1 | ||
| `); |
There was a problem hiding this comment.
end_line is absent
The comment says "NULL end_line sorts last in SQLite ASC", but SQLite actually sorts NULLs first in ASC order (NULLs are considered less than any other value). When any method/function node has end_line IS NULL, the expression (end_line - line) evaluates to NULL, which ranks before every positive span — so that unbounded node is picked by LIMIT 1 instead of the tightest-bounded enclosing method. The intent (use unbounded nodes only as a last resort) is directly contradicted by the actual SQLite behaviour, meaning this/super edges can silently be attributed to the wrong caller.
Fix: replace the ORDER BY with COALESCE(end_line - line, 999999999) ASC or (end_line - line) ASC NULLS LAST (supported since SQLite 3.30.0).
There was a problem hiding this comment.
Fixed — replaced (end_line - line) ASC with COALESCE(end_line - line, 999999999) ASC. SQLite ASC orders NULLs first so the original query would pick unbounded nodes before any bounded node, inverting the intent. The COALESCE sentinel pushes them last, which is what the comment was trying to say. Committed in fd15d3c.
| if (call.receiver !== 'this' && call.receiver !== 'self' && call.receiver !== 'super') | ||
| continue; |
There was a problem hiding this comment.
'self' as a this-like receiver for JS/TS-family files may produce false-positive dispatch edges. In JavaScript and TypeScript self is not a class-instance receiver — it refers to the global WindowOrWorkerGlobalScope object. Any call of the form self.postMessage() or similar inside a Worker would be misidentified as a this-dispatch and resolved against the class hierarchy, yielding spurious calls edges. If self support is intentional (e.g. for CoffeeScript or other transpiled sources), a comment explaining the rationale would help future readers avoid accidentally widening this further.
| if (call.receiver !== 'this' && call.receiver !== 'self' && call.receiver !== 'super') | |
| continue; | |
| if (call.receiver !== 'this' && call.receiver !== 'super') | |
| continue; |
There was a problem hiding this comment.
Fixed — removed 'self' from the receiver check in runPostNativeThisDispatch. This function only runs on JS/TS-family files where self is WindowOrWorkerGlobalScope, not a class instance. Keeping it would have produced spurious dispatch edges for any self.postMessage() or similar Worker call. Applied the suggested fix plus an explanatory comment. Committed in fd15d3c.
…f receiver SQLite ASC ordering puts NULL values first, so (end_line - line) ASC would pick unbounded nodes before any bounded node — inverting the intent. Replace with COALESCE(end_line - line, 999999999) ASC so unbounded nodes sort last. Also remove 'self' from the this/super receiver filter in runPostNativeThisDispatch. In JS/TS files 'self' refers to WindowOrWorkerGlobalScope, not a class instance — including it would produce spurious dispatch edges from Worker call sites.
Codegraph Impact Analysis4 functions changed → 6 callers affected across 6 files
|
…files only On a full native build, runPostNativeThisDispatch was WASM-re-parsing every JS/TS file in the project, adding a costly second parse pass on top of the native Rust parse (measured: +358% ms/file on codegraph itself). Narrow the file set to only files that appear in the class inheritance graph (sources and targets of 'extends' edges). Files outside the hierarchy have no class relationship, so this/super calls in them either resolve locally or are skipped by resolveThisDispatch anyway — WASM re-parsing them adds cost with zero benefit. Also replace the hardcoded 0.1 confidence penalty with the CHA_DISPATCH_PENALTY named constant (already imported), matching every other CHA confidence calculation in native-orchestrator.ts and build-edges.ts. Fixes: regression-guard failure "Build ms/file: 3.6 → 16.5 (+358%)" (#1337)
|
Addressed Greptile's remaining finding from the summary: replaced the hardcoded Also fixed the performance regression caught by the benchmark gate: the WASM re-parse on full native builds was scanning every JS/TS file in the project (+358% ms/file). Narrowed the file set to only files that participate in the class inheritance graph (sources and targets of Committed in 4a5c5b9. |
…Ms timing Add a comment to the incremental-build branch of runPostNativeThisDispatch documenting the known gap: if a parent-class method is replaced (new node ID) but the child file is unchanged, the stale super.method() edge is not refreshed until the next full rebuild. Add wall-clock timing for the this/super dispatch post-pass. The function now returns the elapsed milliseconds (Promise<number>), and the result is threaded through formatNativeTimingResult as a new thisDispatchMs phase. For large class hierarchies the WASM re-parse can be non-trivial, so surfacing it in build diagnostics makes performance regressions visible.
|
Addressed the two remaining findings from the Greptile summary: Incremental build limitation (lines 643–645): Added a comment to the Timing capture: Changed Committed in 773caf0. |
1 participant
Summary
this,super) to the DB, sothis.method()andsuper.method()dispatch was silently dropped on the native pathrunPostNativeThisDispatch: after the Rust pipeline completes, WASM-re-parses JS/TS/TSX files to collect call sites withthis/superreceivers, then resolves them through the DB class hierarchy (extendsedges) using the existingresolveThisDispatchfunctionskipIf(engine === 'native')guards on thethis-dispatchandsuper-dispatchintegration tests — both engines now produce identical edgesTest plan
this-dispatch: emits ConcreteWorker.doWork → ConcreteWorker.preparepasses for both wasm and nativesuper-dispatch: emits Lion.speak → Animal.speakpasses for both wasm and nativeabstract_class_declarationfix)Closes #1326