fix: unify Object.defineProperty accessor fallback kind-filtering between build paths#1889
Conversation
…ween build paths The full-build path's Object.defineProperty accessor fallback (resolveDefinePropertyAccessorFallback in stages/build-edges.ts, and its native-engine post-pass buildDefinePropertyPostPass) and the incremental path's (applyCallFallbacks in incremental.ts) diverged in their final same-file fallback tier: full-build returned any same-file node named call.name regardless of kind, while incremental filtered to function/method kinds only. A getter/setter registered via Object.defineProperty always dispatches to callable code, and every resolved target is unconditionally emitted as a 'calls' edge downstream — so the incremental path's kind filter is the correct behavior. Extracted the shared logic into resolveDefinePropertyAccessorTarget in call-resolver.ts (applying the kind filter) and updated all three call sites - the full-build fallback, its native-engine post-pass, and the incremental fallback - to use it, removing the divergent duplicated implementations. docs check acknowledged: internal call-resolution bug fix/refactor, no new feature/language/architecture/command surface to document in README/CLAUDE.md/ROADMAP.md. Fixes #1766 Impact: 4 functions changed, 14 affected
Greptile SummaryThis PR unifies the
Confidence Score: 5/5Safe to merge — the change is a well-contained deduplication that tightens an existing filter; no new code paths are introduced. All three call sites that handled Object.defineProperty accessor fallback previously had independent implementations; two were silently accepting class/variable nodes as call targets. The shared function correctly restricts Tier-2 lookups to function/method kinds. The Tier-1 (type-qualified) branch is unchanged in behavior. The unwrapTypeEntry helper was already in use on the WASM path before this PR, so there is no new dependency risk. Unit tests directly pin the kind-filter contract, and the integration test locks in full-build/incremental parity end-to-end. No files require special attention. The double definePropertyReceivers Map lookup in buildDefinePropertyPostPass (once before the call and once inside the shared function) is redundant but harmless. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Call site: this.X() inside getter/setter"] --> B{receiver === 'this' AND callerName known AND definePropertyReceivers set?}
B -- No --> Z["Skip fallback"]
B -- Yes --> C["resolveDefinePropertyAccessorTarget\n(call-resolver.ts — shared)"]
C --> D["Look up callerName in definePropertyReceivers → receiverVarName"]
D -- not found --> E["return []"]
D -- found --> F["unwrapTypeEntry(typeMap.get(receiverVarName)) → typeName"]
F -- typeName present --> G["lookup.byNameAndFile('TypeName.X', relPath) [Tier 1 — unfiltered]"]
G -- results found --> H["return qualified targets"]
G -- empty --> I
F -- no typeName --> I["lookup.byNameAndFile('X', relPath).filter(kind === 'function' OR 'method') [Tier 2 — kind-filtered fix #1766]"]
I --> J["return filtered targets"]
subgraph "Call sites"
CS1["WASM full-build\nresolveDefinePropertyAccessorFallback"]
CS2["Native post-pass\nbuildDefinePropertyPostPass"]
CS3["Incremental watch\napplyCallFallbacks"]
end
CS1 --> C
CS2 --> C
CS3 --> C
%%{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"}}}%%
flowchart TD
A["Call site: this.X() inside getter/setter"] --> B{receiver === 'this' AND callerName known AND definePropertyReceivers set?}
B -- No --> Z["Skip fallback"]
B -- Yes --> C["resolveDefinePropertyAccessorTarget\n(call-resolver.ts — shared)"]
C --> D["Look up callerName in definePropertyReceivers → receiverVarName"]
D -- not found --> E["return []"]
D -- found --> F["unwrapTypeEntry(typeMap.get(receiverVarName)) → typeName"]
F -- typeName present --> G["lookup.byNameAndFile('TypeName.X', relPath) [Tier 1 — unfiltered]"]
G -- results found --> H["return qualified targets"]
G -- empty --> I
F -- no typeName --> I["lookup.byNameAndFile('X', relPath).filter(kind === 'function' OR 'method') [Tier 2 — kind-filtered fix #1766]"]
I --> J["return filtered targets"]
subgraph "Call sites"
CS1["WASM full-build\nresolveDefinePropertyAccessorFallback"]
CS2["Native post-pass\nbuildDefinePropertyPostPass"]
CS3["Incremental watch\napplyCallFallbacks"]
end
CS1 --> C
CS2 --> C
CS3 --> C
Reviews (1): Last reviewed commit: "fix: unify Object.defineProperty accesso..." | Re-trigger Greptile |
Codegraph Impact Analysis4 functions changed → 14 callers affected across 2 files
|
Summary
function/methodkind filter is correct (all consumers of the resolved target unconditionally emit it as acallsedge, so an unfiltered match to a same-named class/variable would be nonsensical) and the full-build path's unfiltered version was the bug.buildDefinePropertyPostPass's own "resolve via receiver type" block inbuild-edges.ts. Unified all three into a sharedresolveDefinePropertyAccessorTargetincall-resolver.ts.definePropertyReceiversis JS/TS-extractor-only, confirmed zero occurrences incrates/codegraph-core/).Closes #1766
Test plan
npm test— full suite green (3542 passed, 0 failed)npm run lint— cleanImportant caveat, verified empirically rather than assumed: the "unrelated same-named class beats correct target" scenario is not actually observable end-to-end — a separate, already-shared primary same-file lookup runs first and shadows this fallback tier entirely, so this fix's real-world effect is narrower than the issue's original framing suggested. Rather than write a misleading integration test asserting incorrect behavior as "expected" (which CLAUDE.md explicitly forbids), the unit tests are the authoritative proof; the actual root cause of the "wrong same-named symbol wins" symptom is filed separately as #1888 (confirmed identical on both engines, all 34 languages — too broad for this fix). Also filed #1887: native's fast orchestrator path never runs
buildDefinePropertyPostPassat all (missing edges, not just wrong-kind).Stacked on #1886 (base branch
fix/issue-1765-incremental-same-class-barecall) — only the call-resolver/builder/test diff is this issue's change.