Skip to content

fix: unify Object.defineProperty accessor fallback kind-filtering between build paths#1889

Open
carlos-alm wants to merge 1 commit into
fix/issue-1765-incremental-same-class-barecallfrom
fix/issue-1766-defineproperty-kind-filter-parity
Open

fix: unify Object.defineProperty accessor fallback kind-filtering between build paths#1889
carlos-alm wants to merge 1 commit into
fix/issue-1765-incremental-same-class-barecallfrom
fix/issue-1766-defineproperty-kind-filter-parity

Conversation

@carlos-alm

Copy link
Copy Markdown
Contributor

Summary

  • Confirmed the incremental path's function/method kind filter is correct (all consumers of the resolved target unconditionally emit it as a calls edge, so an unfiltered match to a same-named class/variable would be nonsensical) and the full-build path's unfiltered version was the bug.
  • Found a third, previously-undiscovered copy of the same unfiltered bug beyond the two named in the issue: buildDefinePropertyPostPass's own "resolve via receiver type" block in build-edges.ts. Unified all three into a shared resolveDefinePropertyAccessorTarget in call-resolver.ts.
  • No native Rust equivalent exists for this specific mechanism (definePropertyReceivers is JS/TS-extractor-only, confirmed zero occurrences in crates/codegraph-core/).

Closes #1766

Test plan

  • npm test — full suite green (3542 passed, 0 failed)
  • npm run lint — clean
  • 5 unit tests proving an unrelated same-file class/variable is excluded and only the function/method wins
  • Integration test proving full-build and incremental produce identical edges

Important 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 buildDefinePropertyPostPass at 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.

…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-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR unifies the Object.defineProperty accessor fallback resolution across three divergent call sites — resolveDefinePropertyAccessorFallback (WASM full-build), buildDefinePropertyPostPass (native-engine post-pass), and applyCallFallbacks (incremental watch path) — by extracting a single shared resolveDefinePropertyAccessorTarget in call-resolver.ts. The fix closes a kind-filter gap where the full-build and native-engine paths accepted any same-file node (including unrelated classes and variables) as a fallback target, while the incremental path had already been correctly restricting to function/method kinds.

  • Bug fix: buildDefinePropertyPostPass and resolveDefinePropertyAccessorFallback in build-edges.ts now apply the same function/method kind filter on the Tier-2 (same-file bare-name) fallback as the incremental path, via the shared function.
  • Deduplication: the previously-independent resolveDefinePropertyTarget in incremental.ts is deleted; all three call sites now delegate to resolveDefinePropertyAccessorTarget in call-resolver.ts.
  • Tests: five new unit tests pin the standalone kind-filter contract, and one integration test asserts full-build vs. incremental edge parity for the Object.defineProperty accessor pattern.

Confidence Score: 5/5

Safe 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

Filename Overview
src/domain/graph/builder/call-resolver.ts Adds resolveDefinePropertyAccessorTarget — the canonical, kind-filtered implementation shared by all three call sites. Imports unwrapTypeEntry from strategy.ts (already used by the WASM-path predecessor). Logic is correct: Tier 1 (qualified type lookup) is intentionally unfiltered; Tier 2 (bare same-file lookup) is filtered to function/method only.
src/domain/graph/builder/stages/build-edges.ts Both the WASM-path fallback (resolveDefinePropertyAccessorFallback) and the native-engine post-pass (buildDefinePropertyPostPass) now delegate to the shared function. The previously unfiltered lookup.byNameAndFile(call.name, relPath) fallback in each is replaced by the kind-filtered shared implementation.
src/domain/graph/builder/incremental.ts Removes the now-redundant private resolveDefinePropertyTarget and routes applyCallFallbacks Strategy 3 to the shared resolveDefinePropertyAccessorTarget. Behavior is unchanged (incremental path was already filtering correctly); the change only eliminates the duplicate.
tests/unit/call-resolver.test.ts Five targeted unit tests directly exercise the kind-filter contract of the new shared function: class-beats-function (expect function wins), variable-beats-method (expect method wins), no-function-or-method (expect empty), unregistered caller (expect empty), type-qualified preferred over bare-name.
tests/integration/issue-1766-defineproperty-kind-filter-parity.test.ts New integration test confirms full-build and incremental-rebuild edge sets are identical for the Object.defineProperty accessor pattern (getter -> baz). Runs for the wasm engine unconditionally and skips the native engine when unavailable.

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
Loading
%%{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
Loading

Reviews (1): Last reviewed commit: "fix: unify Object.defineProperty accesso..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

4 functions changed14 callers affected across 2 files

  • resolveDefinePropertyAccessorTarget in src/domain/graph/builder/call-resolver.ts:86 (10 transitive callers)
  • applyCallFallbacks in src/domain/graph/builder/incremental.ts:637 (5 transitive callers)
  • buildDefinePropertyPostPass in src/domain/graph/builder/stages/build-edges.ts:670 (3 transitive callers)
  • resolveDefinePropertyAccessorFallback in src/domain/graph/builder/stages/build-edges.ts:1271 (3 transitive callers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant