From 93363f3b99f6bdbca9df34aaff1d734b126534e8 Mon Sep 17 00:00:00 2001 From: Yyunozor Date: Tue, 28 Jul 2026 00:44:38 +0200 Subject: [PATCH] fix(extract): shadow untracked JS/TS closure params from indirect_call args (#2241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit walk_calls flattens an inline/untracked arrow or function-expression argument (one not separately tracked in function_bodies) onto the enclosing named function's caller_nid, so its calls resolve as if made directly by that function (#1630). But the closure's own parameters and locals were never folded into the shadow set used to guard argument-based indirect_call resolution, so a call argument inside the closure that happened to share a name with an unrelated callable elsewhere in the corpus produced a fabricated indirect_call edge, confidence 0.8 — even though the identifier was, in fact, a local binding one lexical scope down: rows.map((r) => c.get(r)) // `r` is the arrow's own param, not a // reference to some other same-named function Single-letter names make this common, since they collide with same-named symbols anywhere else in the repo (loop vars, test helpers). Fix: thread an extra_locals set through walk_calls's recursion. Entering an untracked closure folds that closure's own bindings (computed the same way as a tracked function's, via _js_local_bound_names) into extra_locals for its subtree only; deeper untracked closures compound the same way on their own recursion. All six call sites that build the caller's shadow set now union in extra_locals, so the fix applies uniformly to the argument, collection, and assignment/return capture paths already sharing that guard, not just the argument one that surfaced it. Tracked closures (const-assigned arrows, methods) are unaffected — they already get their own caller_nid and their own correctly-scoped shadow set. Scope: this fixes the shadow-set gap for closures. A `for (const x of xs)` loop variable not wrapped in a variable_declarator is a separate, pre-existing gap in the same shadow computation, already addressed by #1985 — not duplicated here. --- graphify/extractors/engine.py | 29 ++- ...est_indirect_call_nested_closure_shadow.py | 182 ++++++++++++++++++ 2 files changed, 202 insertions(+), 9 deletions(-) create mode 100644 tests/test_indirect_call_nested_closure_shadow.py diff --git a/graphify/extractors/engine.py b/graphify/extractors/engine.py index 136085e23..7a3163c55 100644 --- a/graphify/extractors/engine.py +++ b/graphify/extractors/engine.py @@ -3852,6 +3852,7 @@ def walk_calls( node, caller_nid: str, java_types: dict[str, str] | None = None, + extra_locals: frozenset[str] = frozenset(), ) -> None: if node.type in config.function_boundary_types: # JS/TS: an inline/returned closure not separately tracked in @@ -3864,8 +3865,18 @@ def walk_calls( and node.type in _JS_CLOSURE_TYPES): body = node.child_by_field_name("body") if body is not None and id(body) not in _tracked_body_ids: + # This closure's own params/locals (`(r) => c.get(r)`) are + # scoped to it, not to the enclosing caller_nid — but its + # calls ARE attributed to caller_nid right here, so a bare + # reference to one of them (e.g. passed on as a call + # argument) must still be recognized as local, not resolved + # against an unrelated same-named definition elsewhere in + # the corpus (#2241). Fold this closure's own bindings into + # extra_locals for its subtree only; deeper untracked + # closures compound the same way on their own recursion. + closure_locals = extra_locals | _js_local_bound_names(node, source) for child in node.children: - walk_calls(child, caller_nid, java_types) + walk_calls(child, caller_nid, java_types, closure_locals) return if node.type in config.call_types: @@ -3875,7 +3886,7 @@ def walk_calls( edges, seen_dyn_import_pairs): # Still recurse into children (import().then(...) may have calls) for child in node.children: - walk_calls(child, caller_nid, java_types) + walk_calls(child, caller_nid, java_types, extra_locals) return callee_name: str | None = None @@ -4217,7 +4228,7 @@ def walk_calls( if config.ts_module == "tree_sitter_python": args_node = node.child_by_field_name("arguments") if args_node is not None: - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals for arg in args_node.children: if arg.type == "identifier": _emit_indirect_ref(arg, caller_nid, enclosing_locals, "argument") @@ -4242,7 +4253,7 @@ def walk_calls( # handled by the collection pass). args_node = node.child_by_field_name("arguments") if args_node is not None: - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals for arg in args_node.children: if arg.type == "identifier": _emit_indirect_ref(arg, caller_nid, enclosing_locals, "argument") @@ -4378,12 +4389,12 @@ def walk_calls( if config.ts_module == "tree_sitter_python" and node.type in ( "dictionary", "list", "set", "tuple" ): - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals for ident in _python_dispatch_value_idents(node): _emit_indirect_ref(ident, caller_nid, enclosing_locals, "collection") elif config.ts_module in ("tree_sitter_javascript", "tree_sitter_typescript") \ and node.type in ("object", "array"): - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals for ident in _js_dispatch_value_idents(node): _emit_indirect_ref(ident, caller_nid, enclosing_locals, "collection") @@ -4393,17 +4404,17 @@ def walk_calls( # TARGET is a new local binding, not a reference -- so the shared shadow guard # still holds (a param/local named on the RHS is the local, not the module fn). if config.ts_module == "tree_sitter_python" and node.type == "assignment": - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals for ident in _python_ref_value_idents(node.child_by_field_name("right")): _emit_indirect_ref(ident, caller_nid, enclosing_locals, "assignment") elif config.ts_module == "tree_sitter_python" and node.type == "return_statement": - enclosing_locals = local_bound_names.get(caller_nid, frozenset()) + enclosing_locals = local_bound_names.get(caller_nid, frozenset()) | extra_locals value = next((c for c in node.children if c.is_named), None) for ident in _python_ref_value_idents(value): _emit_indirect_ref(ident, caller_nid, enclosing_locals, "return") for child in node.children: - walk_calls(child, caller_nid, java_types) + walk_calls(child, caller_nid, java_types, extra_locals) if config.ts_module == "tree_sitter_ruby": for caller_nid, body_node in function_bodies: diff --git a/tests/test_indirect_call_nested_closure_shadow.py b/tests/test_indirect_call_nested_closure_shadow.py new file mode 100644 index 000000000..02cd2528c --- /dev/null +++ b/tests/test_indirect_call_nested_closure_shadow.py @@ -0,0 +1,182 @@ +"""Indirect-call argument shadowing across untracked JS/TS closures (#2241). + +An inline arrow / function-expression argument that is not separately tracked +(`_tracked_body_ids`) has its calls attributed to the ENCLOSING named +function's `caller_nid` (#1630). But the closure's own parameters and locals +were never folded into that enclosing function's shadow set, so a call +argument inside the closure that happened to share a name with an unrelated +callable elsewhere in the corpus produced a fabricated `indirect_call` edge — +even though the identifier was, in fact, a local binding just one lexical +scope down (e.g. `rows.map((r) => c.get(r))`, where `r` is the arrow's own +parameter). + +These tests pin the fix: the closure's own bindings now widen the shadow set +for calls made inside it, and only for that subtree. They also pin what must +NOT regress: a genuine by-name reference to a real, unshadowed callable still +emits `indirect_call` from inside the same kind of nested closure. + +Out of scope here: a `for (const x of xs)` loop variable not wrapped in a +`variable_declarator` is a separate, pre-existing gap in the same shadow-set +computation, already addressed by #1985 — not retested here to avoid +overlapping that diff. +""" +import os +from pathlib import Path + +import networkx as nx + +from graphify.affected import affected_nodes +from graphify.extract import extract + + +def _extract_js_dir(tmp_path, files: dict[str, str]): + base = tmp_path / "src" + base.mkdir() + for name, body in files.items(): + (base / name).write_text(body) + old = os.getcwd() + try: + os.chdir(tmp_path) + r = extract( + [Path("src") / name for name in files], + cache_root=Path(".cache"), parallel=False, + ) + finally: + os.chdir(old) + nid = {n["label"].rstrip("()"): n["id"] for n in r["nodes"]} + return r, nid + + +def _rels(r, relation): + return {(e["source"], e["target"]) for e in r["edges"] if e["relation"] == relation} + + +def test_untracked_arrow_param_shadow_emits_no_indirect_call(tmp_path): + """Reported shape (#2241): a one-letter test helper `r` must not become a + fabricated indirect_call target when an unrelated function's inline `.map` + callback has its own parameter named `r`, later passed on as a plain call + argument deeper in the arrow body.""" + r, nid = _extract_js_dir(tmp_path, { + "round.test.ts": "const r = (v) => Math.round(v);\n", + "report.ts": ( + "export function buildSheet(rows, cols) {\n" + " return rows.map((r) => {\n" + " const o = {};\n" + " for (const c of cols) o[c.label] = c.get(r);\n" + " return o;\n" + " });\n" + "}\n" + ), + }) + indirect = _rels(r, "indirect_call") + assert all(t != nid["r"] for _s, t in indirect) + + +def test_untracked_arrow_genuine_reference_still_emits_indirect_call(tmp_path): + """The same nested-arrow shape must still capture a REAL by-name reference + that is not shadowed by any enclosing local — widening the shadow set for + untracked closures must not blanket-suppress indirect_call inside them.""" + r, nid = _extract_js_dir(tmp_path, {"a.js": ( + "function handler(x){ return x; }\n" + "function via(rows, pool){\n" + " return rows.map((row) => {\n" + " pool.submit(handler);\n" + " return row;\n" + " });\n" + "}\n" + )}) + indirect = _rels(r, "indirect_call") + assert (nid["via"], nid["handler"]) in indirect + + +def test_double_nested_untracked_closures_shadow_compounds(tmp_path): + """Shadowing must compound through two levels of untracked inline + closures: an outer arrow's own param and an independently-named inner + arrow's own param must BOTH be recognized as local, however deep the + call that references them sits.""" + r, nid = _extract_js_dir(tmp_path, {"a.js": ( + "function outer(){}\n" + "function inner(){}\n" + "function via(rows){\n" + " return rows.map((outer) => {\n" + " return [1].map((inner) => {\n" + " pool.submit(outer);\n" + " pool.submit(inner);\n" + " return 0;\n" + " });\n" + " });\n" + "}\n" + )}) + indirect = _rels(r, "indirect_call") + assert all(t != nid["outer"] for _s, t in indirect) + assert all(t != nid["inner"] for _s, t in indirect) + + +def test_tracked_const_arrow_param_shadow_still_emits_no_indirect_call(tmp_path): + """A const-assigned arrow IS separately tracked (its own caller_nid, own + local_bound_names) — this pre-existing path must be unaffected by the new + extra_locals threading: a same-named param inside it still shadows.""" + r, nid = _extract_js_dir(tmp_path, {"a.js": ( + "function handler(){}\n" + "const via = (pool, handler) => { pool.submit(handler); };\n" + )}) + indirect = _rels(r, "indirect_call") + assert all(t != nid["handler"] for _s, t in indirect) + + +def test_affected_excludes_shadowed_untracked_closure_caller(tmp_path): + """Blast-radius traversal must not include a caller that only reached the + target through the fabricated edge this fix removes.""" + r, nid = _extract_js_dir(tmp_path, { + "round.test.ts": "const r = (v) => Math.round(v);\n", + "report.ts": ( + "export function buildSheet(rows, cols) {\n" + " return rows.map((r) => {\n" + " const o = {};\n" + " for (const c of cols) o[c.label] = c.get(r);\n" + " return o;\n" + " });\n" + "}\n" + ), + }) + g = nx.DiGraph() + for n in r["nodes"]: + g.add_node(n["id"], **n) + for e in r["edges"]: + g.add_edge(e["source"], e["target"], **e) + affected = {h.node_id for h in affected_nodes(g, nid["r"])} + assert nid["buildSheet"] not in affected + + +def test_untracked_arrow_param_shadow_stable_on_warm_cache(tmp_path): + """The fix must hold on a warm-cache re-extraction, not just a cold run — + the shadow set is recomputed from the AST each time, never itself cached, + but the extracted edge set it feeds into is.""" + files = { + "round.test.ts": "const r = (v) => Math.round(v);\n", + "report.ts": ( + "export function buildSheet(rows, cols) {\n" + " return rows.map((r) => {\n" + " const o = {};\n" + " for (const c of cols) o[c.label] = c.get(r);\n" + " return o;\n" + " });\n" + "}\n" + ), + } + r_cold, nid_cold = _extract_js_dir(tmp_path, files) + assert all(t != nid_cold["r"] for _s, t in _rels(r_cold, "indirect_call")) + + # Re-extract against the same tmp_path / cache_root: the second run reads + # the warm AST cache for both unchanged files. + old = os.getcwd() + try: + os.chdir(tmp_path) + r_warm = extract( + [Path("src") / name for name in files], + cache_root=Path(".cache"), parallel=False, + ) + finally: + os.chdir(old) + nid_warm = {n["label"].rstrip("()"): n["id"] for n in r_warm["nodes"]} + assert all(t != nid_warm["r"] for _s, t in _rels(r_warm, "indirect_call"))