Skip to content

collectForOfBinding misreads for (const k in of) as for-of when the iterated variable is literally named 'of' #2639

Description

@carlos-alm

What

Found while reviewing docs/plans/issue-2088.md (PR #2612, round 18) — a pre-existing bug in already-shipped code, unrelated to issue #2088's own escape-analysis design, spotted while fixing an analogous discriminator the plan proposes for a different function.

collectForOfBinding (src/extractors/javascript.ts:4280-4322) decides whether a for_in_statement is the for...of or for...in variant with:

let isForOf = false;
for (let i = 0; i < node.childCount; i++) {
  if (node.child(i)?.text === 'of') {
    isForOf = true;
    break;
  }
}

This scans every direct child's text for a literal match on 'of', rather than reading the loop's own operator field. for_in_statement carries a required, single-value operator field whose only possible tokens are in/of (verified against tree-sitter-javascript@0.25.0's own node-types.json) — but the blanket scan doesn't use it, and text-matches whatever it happens to find first.

Since of is an ordinary identifier in JavaScript, not a reserved word, a for-in loop whose iterated expression happens to be a variable named of is legal: for (const k in of) …. Here the loop's operator token is in, but the right field (the iterated expression) is an identifier whose own text is literally "of" — and the blanket scan finds that node's text matching 'of' and misclassifies the loop as for...of, seeding a forOfBindings entry ({ varName: 'k', sourceName: 'of', ... }) for a loop that's actually enumerating of's own property keys, not its values.

The Rust mirror was not checked for the identical gap; if it uses the same shape it should be checked too.

Impact

Low — requires a variable literally named of used as the object of a for...in loop, an unusual but legal construction. The consequence is a spurious points-to fact (k treated as if bound to elements of of, when it's actually bound to of's own enumerable key names) — this affects precision of the points-to solver's downstream call resolution (potentially over-crediting a call through the misclassified binding as live), not a soundness/false-"dead" direction. Not filed as OVER-escape/UNDER-escape, since that framing belongs to issue #2088's own escape-analysis exclusion ledger and this bug is unrelated to it (collectForOfBinding feeds the solver's forOfBindings, a different mechanism).

Suggested next step

Read the operator field directly (node.childForFieldName('operator')?.text === 'of') instead of scanning all children — the same fix docs/plans/issue-2088.md's round 18 applies to isTrackedReferencePosition's own, structurally identical for-of discriminator (see that plan's isTrackedReferencePosition doc comment for the fuller argument and the verified counter-example). Check the Rust mirror (crates/codegraph-core/src/extractors/javascript.rs, collect_for_of_binding or equivalent) for the same shape.

Where

  • src/extractors/javascript.ts:4280-4322 (collectForOfBinding)
  • Possibly crates/codegraph-core/src/extractors/javascript.rs (Rust mirror — not checked)
  • Related: docs/plans/issue-2088.md's isTrackedReferencePosition doc comment (round 18 nit), which fixes the identical shape in a different, not-yet-implemented function

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions