Skip to content

fix(extractors): stop crediting a fallback read a prior write already killed - #2554

Merged
carlos-alm merged 6 commits into
mainfrom
fix/issue-2438
Aug 17, 2026
Merged

fix(extractors): stop crediting a fallback read a prior write already killed#2554
carlos-alm merged 6 commits into
mainfrom
fix/issue-2438

Conversation

@carlos-alm

Copy link
Copy Markdown
Contributor

Summary

The #2257 value-ref liveness scan (hasLaterReferenceInEnclosingBlock / blockContainsIdentifierExcluding in src/extractors/javascript.ts, mirrored in crates/codegraph-core/src/extractors/javascript.rs) decides whether a logical-or/nullish-coalescing/ternary fallback is actually consumed by looking for a later read of the declared variable. It correctly ignores plain writes (an assignment target is a write, not a read), but did not model a write as a kill — so a read occurring after the value had already been overwritten was still credited.

function outer() {
  var fn = options.custom || fetchLatestVersion;
  fn = other;   // kills the fallback value
  fn();         // reads `other`, NOT the fallback
}

A value-ref calls edge to fetchLatestVersion was emitted in both engines, even though the fallback value is never consumed — a false positive that kept a fallback function (and its transitive callees) out of roles --role dead.

Fix

Added killsBinding/kills_binding: true when a statement — a direct child of the enclosing block, the exact granularity the per-statement scan already iterates — unconditionally overwrites the name (a top-level name = value; assignment, or a var name = value; redeclaration). hasLaterReferenceInEnclosingBlock/has_later_reference_in_enclosing_block now checks each statement for a read first (unchanged), then stops the scan entirely once a kill is found.

  • A write nested inside an if/loop/switch/try never matches — it surfaces as a single wrapping statement (if_statement, etc.), not as the assignment itself — so a conditional write correctly never kills, since the branch might not run.
  • The killing statement's own right-hand side is still scanned for a genuine read before the kill takes effect (fn = fn || other; still credits the pre-existing read), since the read-check always runs before the kill-check for the same statement.
  • Compound assignments (+=, a distinct augmented_assignment_expression grammar node) are unaffected — they already fall through to the generic identifier scan and are credited as a genuine read at that statement, independent of this change.

Tests

  • Extended tests/integration/issue-2257-logical-or-ternary-value-ref.test.ts (both WASM and native engines) with 4 new scenarios: kill via plain reassignment, kill via var redeclaration, conditional write does NOT kill, and a genuine same-statement read survives its own killing statement.
  • Added 4 matching Rust unit tests in crates/codegraph-core/src/extractors/javascript.rs's existing test module for the same scenarios.
  • Revert-verified: temporarily disabled the kill-check in both engines and confirmed the exact 2 targeted assertions fail (all others unaffected), confirming the tests exercise the fix rather than passing trivially.

Closes #2438

… killed

The #2257 value-ref liveness scan looked for ANY later reference to the
declared name anywhere in the enclosing block, but never modeled a write as
a kill — so `var fn = a || fallback; fn = other; fn();` still credited
`fallback` as reachable via `fn()`, even though `fn` no longer held it by
that point. `killsBinding`/`kills_binding` now stops the per-statement scan
once a sibling statement unconditionally overwrites the name (a top-level
assignment or `var` redeclaration), while still crediting a genuine read on
that same statement's own right-hand side first. A write nested inside a
conditional never kills, since the branch might not run.

Closes #2438

docs check acknowledged

Impact: 2 functions changed, 0 affected
@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR corrects JavaScript fallback-value liveness analysis so reads after an unconditional overwrite no longer keep the overwritten fallback live.

  • Adds ordered read-then-kill processing across statements, declarators, parenthesized expressions, and sequence expressions.
  • Mirrors the behavior in the TypeScript/WASM and Rust/native extractors.
  • Adds shared integration coverage and matching Rust unit tests for the reported overwrite-ordering cases.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/extractors/javascript.ts Adds ordered overwrite-aware liveness scanning and resolves the previously reported declarator, wrapper, and sequence-expression cases.
crates/codegraph-core/src/extractors/javascript.rs Mirrors the TypeScript liveness semantics in the native extractor and adds focused unit coverage.
tests/integration/issue-2257-logical-or-ternary-value-ref.test.ts Exercises the overwrite-ordering fixes through the shared WASM and native integration suite.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A["Fallback declarator"] --> B["Scan later executable item"]
  B --> C{"Reads binding before overwrite?"}
  C -->|Yes| D["Credit fallback liveness"]
  C -->|No| E{"Unconditionally overwrites binding?"}
  E -->|Yes| F["Stop scan without credit"]
  E -->|No| B
Loading

Reviews (5): Last reviewed commit: "fix(extractors): stop the last same-decl..." | Re-trigger Greptile

Comment thread src/extractors/javascript.ts
Comment thread src/extractors/javascript.ts Outdated
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

4 functions changed13 callers affected across 1 files

  • blockContainsIdentifierExcluding in src/extractors/javascript.ts:4987 (5 transitive callers)
  • declaratorKillsName in src/extractors/javascript.ts:5220 (6 transitive callers)
  • killsBinding in src/extractors/javascript.ts:5251 (6 transitive callers)
  • hasLaterReferenceInEnclosingBlock in src/extractors/javascript.ts:5334 (14 transitive callers)

…eclarator order

Greptile review on PR #2554 found two more shapes the new kill detection
missed:

- A kill wrapped in a parenthesized or sequence expression
  (`(fn = replacement);`) wasn't recognized, since `killsBinding` only
  peeled one layer of `expression_statement` — now unwraps recursively
  through nested `parenthesized_expression`s and treats a
  `sequence_expression` as a kill when any of its parts kills the name.
- A LATER statement with multiple declarators (`var fn = replacement,
  result = fn();`) still credited the second declarator's read, since the
  read-check scanned all declarators of a non-excluded `variable_declaration`
  unconditionally. `blockContainsIdentifierExcluding` now walks such
  declarators in order and stops once one of them kills the name, mirroring
  the same read-then-kill ordering already used at the block level.

docs check acknowledged

Impact: 3 functions changed, 7 affected
@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai please re-review — addressed both findings: kills_binding now recurses through parenthesized/sequence expressions, and block_contains_identifier_excluding now stops crediting reads from later declarators in the same statement once an earlier one kills the name.

Comment thread src/extractors/javascript.ts
Comment thread src/extractors/javascript.ts
Greptile review on PR #2554: (fn = replacement, fn()) still credited the
fn() read, since blockContainsIdentifierExcluding recursed into a
sequence_expression's parts generically with no concept of the
left-to-right order they actually execute in. Added the same ordered
read-then-kill scan already used for top-level block statements and
multi-declarator statements: each part of a sequence is checked for a read
first, then whether it kills the name, stopping the scan once a kill is
found.

docs check acknowledged

Impact: 1 functions changed, 5 affected
@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai please re-review — addressed the sequence-expression ordering finding: block_contains_identifier_excluding now walks a sequence_expression's parts in order, checking each for a read before checking whether it kills the name, mirroring the same pattern used for top-level block statements and multi-declarator statements.

Comment thread src/extractors/javascript.ts
Greptile review on PR #2554: the hasExcludedDeclarator branch (the
declaration statement containing the ORIGINAL fallback declarator) scanned
its later sibling declarators for reads without checking whether one of
them itself unconditionally redeclares the name first — `var fn = a ||
fallback, fn = other, result = fn();` still credited result's read to
fallback. Now checks declaratorKillsName after each sibling's read-check,
same pattern already used everywhere else in this function.

docs check acknowledged

Impact: 1 functions changed, 0 affected
@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai please re-review — addressed the same-declaration finding: the hasExcludedDeclarator branch now checks declaratorKillsName after each later sibling declarator's read-check, matching the pattern used everywhere else in this function.

@carlos-alm
carlos-alm merged commit e15fa2b into main Aug 17, 2026
30 checks passed
@carlos-alm
carlos-alm deleted the fix/issue-2438 branch August 17, 2026 09:21
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

follow-up: fallback liveness scan credits a read that a prior write already killed

1 participant