Skip to content

follow-up: call/property-read attribution doesn't respect this-binding scope boundaries #2085

Description

@carlos-alm

Deferred from PR #2031 review (fix(extractors): attribute same-file ES6 getter/setter property reads as call edges).

Original finding: surfaced by an independent code-review pass during the #2031 sweep (no inline PR comment — found via direct empirical testing, not GitHub review UI).

The bug

Call/property-read attribution to a this-qualified receiver's enclosing class does not check whether an intervening function scope between the access site and the nearest enclosing class breaks JS/TS's this-binding rules. A plain (non-arrow) function — whether a named nested declaration or an anonymous callback (e.g. passed to setTimeout, an event handler, etc.) — does not inherit this lexically, so this inside it does not refer to the enclosing class instance unless explicitly bound (.bind(this), .call(this, ...), an arrow-function wrapper, etc). Both engines currently ignore this and attribute the access to the enclosing class anyway, producing a false-positive calls edge.

Confirmed empirically (both engines, via codegraph build+codegraph query) with:

export class Session {
  isReady(): boolean { return this._ready; }
  private _ready = true;

  checkExplicit(): void {
    setTimeout(function () {
      return this.isReady(); // NOT the Session instance at runtime
    }, 100);
  }
}

codegraph query Session.isReady shows a Session.checkExplicit -> Session.isReady call edge, which is wrong — the callback's this is not bound to the Session instance.

Why this is pre-existing, not introduced by #2031

This reproduces identically for an ordinary explicit this.method() call, which predates #2031 entirely — the root cause is in the shared caller-attribution mechanism (findParentClass/findParentNode in src/extractors/helpers.ts and its Rust mirror, plus the line-range-based "which Definition owns this call site" attribution), not in anything #2031 added. #2031's new same-file accessor-read feature (collectAccessorPropertyRead/handle_accessor_property_read) reuses the exact same pre-existing mechanism by design (its own commit message: "they flow through the existing (unchanged) call-resolution cascade in both engines") and therefore inherits the same false-positive, extended to bare property reads:

export class Session {
  get accessorReady(): boolean { return this._ready; }
  private _ready = true;

  checkAccessor(): void {
    setTimeout(function () {
      return this.accessorReady; // same hazard, new call site kind
    }, 100);
  }
}

Why it's out of scope for #2031

Fixing this properly means making the shared "what class does this refer to here" / caller-attribution logic aware of function-scope this-binding rules (stop the walk at any non-arrow function/function_expression/generator_function boundary, matching the existing FUNCTION_SCOPE_TYPES set already used elsewhere in the extractor for a related purpose). That is a cross-cutting change to call attribution used by every call in the codebase, not something scoped to getter/setter property reads — well beyond #2031's stated, narrowly-scoped fix.

Related, narrower observation (same root cause class)

The new same-file accessor registry (collectLocalAccessors/collect_local_accessors in #2031) keys accessors by class name text (ClassName.propName), consistent with the pre-existing convention handle_method_def/handle_method_def already uses for naming method definitions. Because it's a text key rather than a node-identity key, two distinct classes that happen to share the same name in one file (e.g., one at top level and another shadowed inside a function body) would have their accessor declarations conflated in the registry. This is the same "name-keyed rather than scope-aware" limitation as the main finding above, extended to the accessor registry specifically — narrow (same-named classes in one file are rare) and, like the main finding, a consequence of this codebase's broader name-based (not lexical-scope-based) symbol model rather than something #2031 specifically got wrong.

Suggested fix direction

  • Add a scope-boundary-respecting variant of findParentClass/findParentNode (stop at function_declaration/function_expression/generator_function/generator_function_declaration, but not arrow_function) for receiver-class resolution specifically, and audit call sites that determine "what does this refer to" (as opposed to sites that name a declaration by its lexical parent class, which is correct as-is) to use it.
  • Consider whether the same fix should also cover .bind(this) / arrow-wrapped callbacks correctly continuing to resolve (they should still work — only the unbound plain function case should stop the walk).

Metadata

Metadata

Assignees

No one assigned

    Labels

    follow-upDeferred work from PR reviews that needs tracking

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions