fix(native): add Object.defineProperty accessor dispatch post-pass to native orchestrator#2024
Conversation
… native orchestrator The native orchestrator's fast path (tryNativeOrchestrator) skips runPipelineStages entirely on success, so buildDefinePropertyPostPass never ran for native full builds -- this.method() calls inside getter/setter functions registered via Object.defineProperty produced no edge at all when the accessor target was a typed class instance (the object-literal variant already worked via Rust's own composite-pts mechanism). Adds runPostNativeDefinePropertyDispatch, a hybrid WASM re-parse post-pass mirroring the existing runPostNativeThisDispatch for this/super dispatch: selects JS/TS candidate files (full build: all known files; incremental: changedFiles), narrows them with a cheap text pre-filter for the literal "defineProperty" substring to avoid a full-project WASM re-parse, then resolves targets via the same resolveDefinePropertyAccessorTarget already shared by the WASM engine and the incremental watch path. Wired into runPostNativePasses alongside this/super dispatch and CHA expansion. Extracts the caller-by-line lookup and the DB-backed CallNodeLookup construction (previously inlined only in runPostNativeThisDispatch) into shared helpers now used by both hybrid post-passes. Impact: 12 functions changed, 11 affected
Greptile SummaryThis PR fixes a silent regression in the
Confidence Score: 4/5Safe to merge — the orchestrator change correctly mirrors the established Phase 8.5 pattern, shares the same resolution function as all other call sites, and the integration test validates end-to-end parity between engines. The implementation is structurally sound and the logic path is well-tested. One test-robustness gap: the first describe.each(ENGINES) block runs the native iteration without an isNativeAvailable() guard, so on environments where the native binary is absent the native arm may pass silently via WASM fallback rather than skipping — the same guard the second block in the same file uses correctly. tests/integration/issue-1887-native-defineproperty-postpass.test.ts — the native guard inconsistency in the first describe block. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant B as buildGraph (engine=native)
participant TO as tryNativeOrchestrator
participant PP as runPostNativePasses
participant TD as Phase 8.5: runPostNativeThisDispatch
participant DP as Phase 8.5b: runPostNativeDefinePropertyDispatch (NEW)
participant CH as Phase 8.6: runPostNativeCha
B->>TO: "full build, incremental=false"
TO->>PP: native build succeeded
PP->>TD: WASM re-parse (this/super dispatch)
TD-->>PP: targetIds, affectedFiles
PP->>DP: WASM re-parse (Object.defineProperty accessor dispatch)
Note over DP: selectDefinePropertyDispatchFiles<br/>text pre-filter defineProperty<br/>parseFilesWasmForBackfill<br/>resolveDefinePropertyAccessorTarget
DP-->>PP: targetIds, affectedFiles
PP->>CH: CHA expansion (both prior passes edges eligible)
CH-->>PP: chaEdgeCount, affectedFiles
PP->>PP: role re-classify (scoped to all affectedFiles union)
PP-->>TO: PostPassTimings (includes definePropertyDispatchMs)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant B as buildGraph (engine=native)
participant TO as tryNativeOrchestrator
participant PP as runPostNativePasses
participant TD as Phase 8.5: runPostNativeThisDispatch
participant DP as Phase 8.5b: runPostNativeDefinePropertyDispatch (NEW)
participant CH as Phase 8.6: runPostNativeCha
B->>TO: "full build, incremental=false"
TO->>PP: native build succeeded
PP->>TD: WASM re-parse (this/super dispatch)
TD-->>PP: targetIds, affectedFiles
PP->>DP: WASM re-parse (Object.defineProperty accessor dispatch)
Note over DP: selectDefinePropertyDispatchFiles<br/>text pre-filter defineProperty<br/>parseFilesWasmForBackfill<br/>resolveDefinePropertyAccessorTarget
DP-->>PP: targetIds, affectedFiles
PP->>CH: CHA expansion (both prior passes edges eligible)
CH-->>PP: chaEdgeCount, affectedFiles
PP->>PP: role re-classify (scoped to all affectedFiles union)
PP-->>TO: PostPassTimings (includes definePropertyDispatchMs)
Reviews (1): Last reviewed commit: "fix(native): add Object.defineProperty a..." | Re-trigger Greptile |
| } | ||
|
|
||
| const ENGINES: EngineMode[] = ['wasm', 'native']; | ||
|
|
There was a problem hiding this comment.
Native engine not guarded by
isNativeAvailable()
The first describe.each(ENGINES) block runs the native iteration unconditionally. On a machine where the native binary is absent, buildGraph(..., { engine: 'native' }) likely falls back to WASM, causing the test to pass — but without actually exercising the new runPostNativeDefinePropertyDispatch post-pass that the PR fixes. The second describe.skipIf(!isNativeAvailable()) block in the same file already gets this right. The first block should use the same guard to avoid either a false-pass (via silent WASM fallback) or a confusing failure (if native is a hard requirement).
Codegraph Impact Analysis14 functions changed → 11 callers affected across 4 files
|
Summary
For
engine: 'native'full builds,buildGraphtakes thetryNativeOrchestratorfast path, which never callsrunPipelineStages— and therefore never callsbuildDefinePropertyPostPass(stages/build-edges.ts). That post-pass exists specifically to resolvethis.method()calls inside getter/setter functions registered viaObject.defineProperty(obj, "x", { get: getter }), since the native Rust engine has no concept ofdefinePropertyReceiversat all. Being architecturally unreachable on the orchestrator-success path meant it silently produced no edge at all (not just the wrong kind — see #1766) for the typed-instance-receiver case:Before this fix:
buildGraph(dir, { engine: 'native', incremental: false })produced nogetter -> ...edge. WASM correctly resolvedgetter -> Registry.bar.(The object-literal variant, e.g.
const obj = { bar() {} }, already worked under native via Rust's own independent composite-pts-key mechanism — unaffected by this bug.)Fix
Adds
runPostNativeDefinePropertyDispatch, a new hybrid WASM re-parse post-pass wired intorunPostNativePasses, structurally mirroring the existingrunPostNativeThisDispatch(Phase 8.5) that already solves the identical "Rust doesn't persist X" problem for this/super dispatch:nodes, already populated by the native build), orchangedFileson an incremental build."defineProperty"substring before paying for a full WASM parse — avoiding the same +358% ms/file full-project-reparse regressionselectThisDispatchFilesalready guards against. The extractor only recognises the literalObject.defineProperty(...)call form, so this pre-filter can never produce a false negative.definePropertyReceivers+ call sites + typeMap, then resolves targets viaresolveDefinePropertyAccessorTarget(call-resolver.ts) — the exact same function already shared by the WASM full-build path, the native fast-path insiderunPipelineStages, and the incremental watch path, so all four call sites stay in lockstep.callsedges taggedtechnique = 'ts-native', matching the other three call sites (direct name/type-lookup resolution, not CHA virtual dispatch), and wires its target/affected-file sets into the existing role-reclassification and node/edge recount scoping.Also extracts two small pieces of
runPostNativeThisDispatchthat are now shared by both hybrid post-passes: the caller-by-line prepared statement (prepareCallerByLineStmt) and the DB-backedCallNodeLookupconstruction (makePostNativeCallLookup). RenamedcleanupThisDispatchWasmTreestocleanupWasmParseTreessince it's generic WASM-tree cleanup already reused by a third, unrelated call site (dropped-language backfill) even before this change.No Rust changes:
definePropertyReceiversis JS/TS-extractor-only with no native equivalent (confirmed zero occurrences incrates/codegraph-core/), so the fix is entirely in the TS-side post-pass orchestration, matching the issue's own root-cause analysis.Test plan
tests/integration/issue-1887-native-defineproperty-postpass.test.ts: engine-parity test (native full build now matches WASM), a dedicated native-vs-WASM edge-equality assertion, and an incremental-rebuild test that adds the accessor pattern to a previously-clean file.tests/integration/issue-1766-defineproperty-kind-filter-parity.test.tsthat referenced this issue as still-unfixed.npm test— full suite green except one pre-existing, already-tracked flaky test (Flaky cycle-node ordering in checkNoNewCycles full-suite run (tests/integration/check.test.ts) #1990, cycle-node ordering, unrelated) and confirmed unrelated to this change.npm run lint— clean.codegraph diff-impact --staged -T— impact contained entirely withinnative-orchestrator.ts's build pipeline, as expected.Out-of-scope findings filed separately
While validating, found two pre-existing bugs unrelated to this fix (both confirmed to reproduce on the base branch with no
src/changes):resolveExactGlobalMatch(bare-call global fallback) lacks a cross-language guard, causing an incidental two-file test repro to pick up an unrelated same-named symbol in a different language. Same class of bug as the already-filed Same-file bare-name lookup (resolveCallTargets) is unfiltered by kind and runs before type-aware dispatch, causing false calls-edges to unrelated classes/variables #1888/bug: resolveByReceiver typeMap lookup lacks cross-language guard, unlike resolveByGlobal #1981.check.test.ts) is pre-existing and unrelated.Closes #1887
Stacked on #1889 (base branch
fix/issue-1884-computed-string-literal-keys-not-unwrapped-in) — only the native-orchestrator/types/test diff is this issue's change.