diff --git a/CHANGELOG.md b/CHANGELOG.md index 7372a80e..4b1aca56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,15 @@ Full release notes with details on each version: [GitHub Releases](https://github.com/safishamsi/graphify/releases) -## 0.9.38 (unreleased) +## 0.9.39 (unreleased) + +- Fix: `affected` now traverses a dynamic `import('…')` made inside a function or at module scope (#2584, thanks @phudayyy). The 0.9.38 dedupe keyed only on the target, so an in-function dynamic import (whose symbol-level edge is anchored on the enclosing function) suppressed the file-level edge `affected` follows; the dedupe now keys on the importing file, emitting one file-level `dynamic_import` edge per file/target while keeping the call-site edge. +- Fix: a Python member call on an untyped receiver (`x.get(...)`) no longer binds by name alone to a same-named module-level function, fabricating a false high-confidence `calls` edge and a god node (#2417, #2586, thanks @EZZEASY). Such a call is now resolved only with receiver-type, import, or `self`/`cls`/`super` evidence, matching the TypeScript fix from 0.9.37; `super().method()` still resolves. +- Fix: fuzzy dedup no longer over-merges two distinct entities in the same file whose long labels differ by a content word (#2576, thanks @wilyan09007). A one-token difference is judged on the differing tokens rather than the prefix-weighted whole-label similarity, so `asset contribution flow` and `asset consumption flow` stay separate while genuine typo and whitespace/case variants still collapse. +- Fix: `graphify watch` now rebuilds on a documentation-only deletion batch instead of only flagging it (#2580, thanks @angmeng), so a deleted doc's nodes are evicted immediately rather than waiting for the next code-file event. (The general deleted-file leak was already fixed in 0.9.10; this closes the live-watcher residual.) +- Fix: Objective-C member-call resolution (#2589, #2590, #2591, thanks @xiongjianxu). A `@protocol` declaration is no longer treated as a receiver type (it collided with a same-named class); a category or class-extension interface (`@interface Foo (Bar)`) now folds into the base class instead of minting a duplicate node; and a message send to a `@property` or ivar receiver (`[self.svc run]`, `[_svc run]`) now resolves through the property/ivar's declared type. + +## 0.9.38 (2026-08-09) - Fix: the 0.9.37 callback-body fix (#2552) no longer lets a local declared in one callback suppress a call in a sibling callback (#2568, thanks @imagineers-tyler). Each callback body's local names are now scoped to that body instead of unioned under the shared declaration, so a real `indirect_call` in one sibling closure is no longer dropped because another sibling declared a same-named local. This can only restore dropped edges, never fabricate. - Fix: Kotlin calls in a property initializer are now collected (#2565, thanks @kskchaitanya1993). A class property (`val repo = createRepo()`), a `by lazy { ... }` delegate, a companion-object property, and a top-level property initializer now produce `calls` edges attributed to the enclosing class (or file), including fully-qualified calls. A plain literal initializer produces no edge. diff --git a/graphify/extract.py b/graphify/extract.py index 3ec8711a..7b23f1f9 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -1252,7 +1252,10 @@ def _rescue_js_dynamic_imports(path: Path, result: dict) -> None: as an ``imports_from`` edge marked ``deferred`` (``_dynamic_import_js``). Re-emitting it here as a second ``dynamic_import`` edge would state the same fact twice, so a match whose resolved target already has a deferred - edge is skipped. + edge FROM THIS FILE'S NODE is skipped. The source check matters: the AST + pass anchors the edge on the enclosing function when the ``import()`` is + written inside one, and that is a different fact from "this file depends on + that module" — the only one file-level traversal can use (#2584). Regex false positives in comments/strings are the precedented trade of the Svelte/Vue rescues; a ``//``-prefix guard covers the common case. @@ -1268,8 +1271,28 @@ def _rescue_js_dynamic_imports(path: Path, result: dict) -> None: base_url = _load_tsconfig_base_url(path.parent) deferred_ids: set[str] = set() deferred_files: set[str] = set() + rescued_targets: set[str] = set() for e in result.get("edges", []): - if e.get("deferred") and e.get("relation") == "imports_from": + # Only a FILE-level deferred edge makes the rescue redundant (#2584). + # + # `_dynamic_import_js` emits `caller_nid -> target`, and `caller_nid` is this + # file's node only when the `import()` sits at module scope. Written inside a + # function it is that function's node — a different fact, at a granularity + # `affected` does not walk. Matching on target alone treated the two as one and + # skipped the rescue, so a dynamic import inside a function ended up with no + # file-level edge at all. The reverse walk then reached the enclosing function + # and stopped: the only edge pointing at it is `contains`, deliberately kept out + # of DEFAULT_AFFECTED_RELATIONS. + # + # Measured on a ~700-file TS repo: `affected --depth 3` returned 39 of 49 truly + # affected files (recall 0.80, precision 1.00) and deeper traversal did not help, + # which is a dead end rather than a depth limit. It stayed hidden because the + # usual case still resolves — when the next importer imports that exact symbol + # by name there IS an edge into the function. Switch that importer to + # `import * as ns` or a side-effect `import './dyn'` and the same graph goes + # silent. + if (e.get("deferred") and e.get("relation") == "imports_from" + and e.get("source") == file_node_id): deferred_ids.add(e.get("target")) tf = e.get("target_file") if tf: @@ -1305,6 +1328,16 @@ def _rescue_js_dynamic_imports(path: Path, result: dict) -> None: continue except OSError: pass + # One file depending on one module is one file-level fact, however many + # call sites defer it. Pre-existing (two module-scope `import('./x')` in one + # file already emitted two identical edges on v8), but #2584 routes every + # in-function dynamic import through here too, which would turn an edge case + # into the common one — a hub module deferred from eight functions of the same + # file would carry eight identical arrows. + emit_key = str(resolved_file.resolve()) if resolved_file is not None else raw + if emit_key in rescued_targets: + continue + rescued_targets.add(emit_key) _emit_rescued_import( result, existing_ids, file_node_id, path, raw, "dynamic_import", aliases, base_url, @@ -3296,9 +3329,18 @@ def _resolve_objc_member_calls( * ``self`` / ``super`` — the caller's own enclosing class -> EXTRACTED. * Capitalized receiver (``[Foo new]``) — the type named explicitly -> EXTRACTED. * ``[f doThing]`` — ``f`` typed via the file's ``Foo *f`` local table -> INFERRED. + * ``[self.bar doIt]`` / ``[_ivarBar doIt]`` — the field typed via the class's + ``@property``/ivar table (locals shadow fields for the bare-identifier + form) -> INFERRED. Only the exact ``self.`` receiver shape is + captured; a dotted receiver like ``Foo.shared`` is never passed through, + because ``_key`` would strip the dot and collide with a real ``FooShared``. An uninferable receiver is SKIPPED (no guess), so an ambiguous selector across classes never fans out. ``_merge_decl_def_classes`` folds each @interface/@impl pair into one node, so a paired class clears the single-definition guard. + ``@protocol`` declarations are excluded from the receiver-type index: a protocol + is a contract, not a message receiver, and ObjC keeps protocol and class names in + separate namespaces, so a same-named pair used to both mis-bind a message to the + protocol's declaration and, when a real class existed, trip the god-node guard. Must run after id-disambiguation so node ids and caller_nids are final. """ @@ -3308,16 +3350,49 @@ def _resolve_objc_member_calls( if tt and tt.get("path"): type_table_by_file[tt["path"]] = tt.get("table", {}) + # #1556: cross-file `field -> ClassName` tables merged per class nid (the + # .h/.m pair share one id, preserved by _merge_decl_def_classes, so the header's + # @property entries and the impl's ivar entries land in one table). A cross-file + # conflict on the same (class, field) drops the entry — no guess. + field_types_by_class: dict[str, dict[str, str]] = {} + field_conflicts: set[tuple[str, str]] = set() + for result in per_file: + ft = result.get("objc_field_types") + if not ft: + continue + for cls_nid, tbl in (ft.get("tables") or {}).items(): + merged = field_types_by_class.setdefault(cls_nid, {}) + for field, tname in tbl.items(): + if (cls_nid, field) in field_conflicts: + continue + prev = merged.get(field) + if prev is None: + merged[field] = tname + elif prev != tname: + del merged[field] + field_conflicts.add((cls_nid, field)) + def _key(label: str) -> str: return re.sub(r"[^a-zA-Z0-9]+", "", str(label)).lower() contained = {e.get("target") for e in all_edges if e.get("relation") == "contains"} + def _is_protocol_declaration(n: dict) -> bool: + """A ``@protocol`` declaration, which the ObjC extractor labels ````. + + A protocol is a contract, never a message receiver, so it must not be a + receiver-typing candidate. It stays a valid target for `implements`; only + this pass's type index excludes it. + """ + label = str(n.get("label", "")).strip() + return label.startswith("<") and label.endswith(">") + type_def_nids: dict[str, list[str]] = {} node_by_id: dict[str, dict] = {} for n in all_nodes: node_by_id[n.get("id")] = n - if n.get("source_file") and n.get("id") in contained and _is_type_like_definition(n): + if (n.get("source_file") and n.get("id") in contained + and _is_type_like_definition(n) and not _is_protocol_declaration(n)): type_def_nids.setdefault(_key(n.get("label", "")), []).append(n["id"]) method_index: dict[tuple[str, str], str] = {} @@ -3349,7 +3424,20 @@ def _resolve_objc_member_calls( src_file = rc.get("source_file", "") if rc.get("lang") != "objc": continue - if receiver in ("self", "super"): + if rc.get("receiver_kind") == "self_field": + # `[self.bar doIt]`: the extractor stamped the BARE field name; type it + # via the caller's own class's @property/ivar table. Checked before the + # capitalized arm so a capitalized field never reads as a class name. + cls = enclosing_type.get(caller) + type_name = field_types_by_class.get(cls, {}).get(receiver) if cls else None + if not type_name: + continue + type_defs = type_def_nids.get(_key(type_name), []) + if len(type_defs) != 1: # ambiguous or absent -> bail (god-node guard) + continue + type_nid = type_defs[0] + type_qualified = False + elif receiver in ("self", "super"): type_nid = enclosing_type.get(caller) if not type_nid: continue @@ -3361,7 +3449,12 @@ def _resolve_objc_member_calls( type_nid = type_defs[0] type_qualified = True else: + # Locals shadow fields: the file's `Foo *f` local table first, then the + # enclosing class's @property/ivar table (covers `[_ivarBar doIt]`). type_name = type_table_by_file.get(src_file, {}).get(receiver) + if not type_name: + cls = enclosing_type.get(caller) + type_name = field_types_by_class.get(cls, {}).get(receiver) if cls else None if not type_name: continue type_defs = type_def_nids.get(_key(type_name), []) diff --git a/graphify/extractors/engine.py b/graphify/extractors/engine.py index a044cede..c831fc27 100644 --- a/graphify/extractors/engine.py +++ b/graphify/extractors/engine.py @@ -4778,6 +4778,21 @@ def _extract_generic( obj = func_node.child_by_field_name(config.call_accessor_object_field) if obj is not None and obj.type == "identifier": member_receiver = _read_text(obj, source) + elif ( + config.ts_module == "tree_sitter_python" + and obj is not None + and obj.type == "call" + ): + # ``super().method()`` has a call node as its + # receiver. Preserve it as a known intra-class + # receiver instead of treating it as unresolved. + receiver_func = obj.child_by_field_name("function") + if ( + receiver_func is not None + and receiver_func.type == "identifier" + and _read_text(receiver_func, source) == "super" + ): + member_receiver = "super" elif (obj is not None and obj.type in config.call_accessor_node_types and config.call_accessor_object_field): @@ -4792,12 +4807,17 @@ def _extract_generic( callee_name = _read_text(func_node, source) if callee_name and callee_name not in _LANGUAGE_BUILTIN_GLOBALS: - # A capitalized-receiver member call (`ClassName.method()`) must defer - # to receiver-based cross-file resolution: the bare method name can - # collide with an in-file node — even the calling method itself, when a - # viewset action delegates to a same-named service action — which would - # match `tgt_nid == caller_nid` and silently drop the call (#1446). The - # captured receiver is resolved later in _resolve_python_member_calls. + # Python member calls defer to receiver-based resolution unless the + # receiver is known to stay in the current class. Falling back to a + # bare method name for an unresolved/lowercase receiver (`d.get()` or + # `self.store.get()`) can bind to an unrelated module function and + # inflate it into a god node (#2417). Qualified class/module calls are + # recovered later by _resolve_python_member_calls when the receiver + # supplies enough evidence (#1446/#1883). Known recall trade (#2586): + # a same-file `x = Thing(); x.method()` no longer gets an edge — it + # came from the same evidence-free bare-name map and could bind wrong + # under label collision; local-instantiation receiver typing is a + # separate follow-up. # C#: ANY member call with a captured receiver defers to the # receiver-typed resolver — a bare method-name match ignores the # receiver's declared type and mis-binds to an unrelated same-named @@ -4807,10 +4827,15 @@ def _extract_generic( config.ts_module == "tree_sitter_c_sharp" and is_member_call and member_receiver ) + _python_defer = ( + config.ts_module == "tree_sitter_python" + and is_member_call + and member_receiver not in {"self", "cls", "super"} + ) _java_defer = ( config.ts_module == "tree_sitter_java" and is_member_call ) - if _java_defer or ( + if _python_defer or _java_defer or ( is_member_call and member_receiver and ( diff --git a/pyproject.toml b/pyproject.toml index bfb64622..0693926d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "graphifyy" -version = "0.9.38" +version = "0.9.39" description = "AI coding assistant skill (Claude Code, CodeBuddy, Codex, OpenCode, Kilo Code, Cursor, Gemini CLI, Aider, OpenClaw, Factory Droid, Trae, Hermes, Kiro, Pi, Devin CLI, Google Antigravity) - turn any folder of code, docs, papers, images, or videos into a queryable knowledge graph" readme = "README.md" license = "Apache-2.0" diff --git a/tests/test_extract.py b/tests/test_extract.py index 29348576..15f8273b 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -1136,13 +1136,15 @@ def test_python_relative_import_out_of_root_target_id_is_portable(tmp_path): def test_python_module_qualified_call_resolves_extracted(tmp_path): """`module.func()` where `module` is imported resolves to the callable that - module contains, with an EXTRACTED `calls` edge (#1883). A lowercase module - receiver was previously dropped alongside instance calls.""" + module contains, with an EXTRACTED `calls` edge (#1883), even when the caller + file contains a same-named function that bare-name lookup could select.""" mathlib = tmp_path / "mathlib.py" caller = tmp_path / "caller.py" mathlib.write_text("def compute(x):\n return x * 2\n") caller.write_text( "import mathlib\n\n" + "def compute(x):\n" + " return x + 1\n\n" "def use_qualified(n):\n" " return mathlib.compute(n)\n" ) @@ -1153,9 +1155,9 @@ def test_python_module_qualified_call_resolves_extracted(tmp_path): if e["relation"] == "calls" and "use_qualified" in nodes[e["source"]]["label"] and "compute" in nodes[e["target"]]["label"] - and "mathlib.py" in (nodes[e["target"]].get("source_file") or "") ] assert len(edges) == 1, f"expected one use_qualified->compute edge, got {edges}" + assert "mathlib.py" in (nodes[edges[0]["target"]].get("source_file") or "") assert edges[0]["confidence"] == "EXTRACTED" @@ -1464,6 +1466,127 @@ def test_python_instance_member_call_not_overconnected(tmp_path): assert bad == [], f"instance member call must not connect cross-file: {bad}" +def test_python_unresolved_member_calls_do_not_bind_to_bare_function(tmp_path): + """#2417: unresolved attribute calls must not bind by bare method name. + + ``d.get()`` and ``self.store.get()`` do not identify the module-level + ``get()`` definition, so only the direct ``get()`` call is a real edge. + The result must also survive a warm cache extraction. + """ + fixture = tmp_path / "fixture.py" + fixture.write_text( + "def get(k):\n" + " return k\n\n" + "class Store:\n" + " def __init__(self):\n" + " self.store = {}\n\n" + " def read(self, k):\n" + " return self.store.get(k)\n\n" + "def other(d):\n" + " return d.get('x')\n\n" + "def real():\n" + " return get('real')\n", + encoding="utf-8", + ) + + def _get_callers(result): + nodes = {node["id"]: node for node in result["nodes"]} + target_ids = { + node["id"] for node in result["nodes"] + if node.get("label") == "get()" and node.get("source_file") + } + return sorted( + nodes[edge["source"]]["label"] + for edge in result["edges"] + if edge.get("relation") == "calls" and edge.get("target") in target_ids + ) + + cold = extract([fixture], cache_root=tmp_path, root=tmp_path) + assert _get_callers(cold) == ["real()"] + warm = extract([fixture], cache_root=tmp_path, root=tmp_path) + assert _get_callers(warm) == ["real()"] + + +def test_python_known_member_receivers_keep_local_call_edges(tmp_path): + """Preserve self/cls/super calls while deferring other call receivers.""" + fixture = tmp_path / "known_receivers.py" + fixture.write_text( + "class Base:\n" + " def inherited(self):\n" + " return 1\n\n" + "class Worker(Base):\n" + " def local(self):\n" + " return 2\n\n" + " @classmethod\n" + " def class_local(cls):\n" + " return 3\n\n" + " def via_self(self):\n" + " return self.local()\n\n" + " @classmethod\n" + " def via_cls(cls):\n" + " return cls.class_local()\n\n" + " def via_super(self):\n" + " return super().inherited()\n\n" + "def via_factory(factory):\n" + " return factory().local()\n", + encoding="utf-8", + ) + result = extract([fixture], cache_root=tmp_path, root=tmp_path) + nodes = {node["id"]: node for node in result["nodes"]} + call_pairs = { + (nodes[edge["source"]]["label"], nodes[edge["target"]]["label"]) + for edge in result["edges"] + if edge.get("relation") == "calls" + } + + for caller, callee in ( + ("via_self", "local"), + ("via_cls", "class_local"), + ("via_super", "inherited"), + ): + assert any( + caller in source_label and callee in target_label + for source_label, target_label in call_pairs + ), f"missing {caller} -> {callee} call edge: {call_pairs}" + assert not any( + "via_factory" in source_label and "local" in target_label + for source_label, target_label in call_pairs + ), f"unresolved factory() receiver must not bind by bare name: {call_pairs}" + + +def test_python_unresolved_receiver_never_crosses_modules(tmp_path): + """#2417 cross-file guard: `client.fetch('x')` must not bind to `util.fetch` + just because the caller's file imports that name — the receiver `client` + supplies no evidence it is the `util` module. A plain `fetch('y')` call to + the imported name still resolves.""" + util = tmp_path / "util.py" + caller = tmp_path / "app.py" + util.write_text("def fetch(url):\n return url\n") + caller.write_text( + "from util import fetch\n\n" + "def via_receiver(client):\n" + " return client.fetch('x')\n\n" + "def via_name():\n" + " return fetch('y')\n" + ) + result = extract([caller, util], cache_root=tmp_path) + nodes = {n["id"]: n for n in result["nodes"]} + fetch_edges = [ + (nodes[e["source"]]["label"], e) + for e in result["edges"] + if e["relation"] == "calls" + and "fetch" in nodes[e["target"]]["label"] + and "util.py" in (nodes[e["target"]].get("source_file") or "") + ] + callers = sorted(label for label, _ in fetch_edges) + assert not any("via_receiver" in c for c in callers), ( + f"unresolved receiver bound cross-module by bare name: {callers}" + ) + assert any("via_name" in c for c in callers), ( + f"imported-name call lost its edge: {callers}" + ) + + def test_python_qualified_call_ambiguous_class_bails(tmp_path): """When the class name is defined in 2+ files, the qualified call must not resolve — single-definition god-node guard (#1446).""" diff --git a/tests/test_js_dynamic_import_affected.py b/tests/test_js_dynamic_import_affected.py new file mode 100644 index 00000000..3768d82e --- /dev/null +++ b/tests/test_js_dynamic_import_affected.py @@ -0,0 +1,154 @@ +"""`affected` must traverse a dynamic `import('…')` written inside a function — #2584. + +The edge was already emitted (#2575), so an edge-existence assertion passes while the +answer users actually read is short. The reason is a granularity mismatch: a static import +emits ``file -> target``, but ``_dynamic_import_js`` emitted ``caller_nid -> target``, and +``caller_nid`` is the file node ONLY at module level. Written inside a function it is that +function's node, so the graph held ``load() --imports_from--> target`` with no file-level +edge, and the reverse walk stopped at ``load()`` — the one edge pointing at it is +``contains``, which is deliberately not in DEFAULT_AFFECTED_RELATIONS. + +Measured on a ~700-file TS repo: recall 0.80 at depth 3, precision 1.00, and raising the +depth did not help. It stayed hidden because the common case works — if the next importer +imports that exact symbol by name, there IS an edge into ``load()``. So the tests below +pin the cases where nothing points at the enclosing symbol: a namespace import and a +side-effect import. Those are the ones that were silent. +""" +from __future__ import annotations + +from pathlib import Path + +import networkx as nx + +from graphify.affected import affected_nodes +from graphify.extract import _file_node_id, extract + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +TARGET = "export const value = 1\n" + +# The dynamic import lives INSIDE a function — the case that regressed. +DYN_IN_FUNCTION = ( + "export async function load() {\n" + " const m = await import('./target')\n" + " return m.value\n" + "}\n" + "export const other = 2\n" +) + +TOP = "import { run } from './mid'\nexport const go = () => run()\n" + + +def _build(tmp_path: Path, mid_src: str, dyn_src: str = DYN_IN_FUNCTION): + files = [ + _write(tmp_path / "src/target.ts", TARGET), + _write(tmp_path / "src/dyn.ts", dyn_src), + _write(tmp_path / "src/mid.ts", mid_src), + _write(tmp_path / "src/top.ts", TOP), + ] + result = extract(files, cache_root=tmp_path, root=tmp_path) + graph = nx.DiGraph() + for n in result["nodes"]: + graph.add_node(n["id"], **n) + for e in result["edges"]: + graph.add_edge(e["source"], e["target"], **e) + return result, graph + + +def _fid(rel: str) -> str: + return _file_node_id(Path(rel)) + + +def _reaches(graph: nx.DiGraph, seed: str, wanted: str, depth: int = 3) -> bool: + hits = affected_nodes(graph, _fid(seed), depth=depth) + return _fid(wanted) in {h.node_id for h in hits} + + +def _file_edges(result: dict, source: str, target: str) -> list[dict]: + return [ + e for e in result["edges"] + if e["source"] == _fid(source) and e["target"] == _fid(target) + ] + + +def test_dynamic_import_in_function_emits_a_file_level_edge(tmp_path: Path): + """The file owning the `import()` depends on the target, whoever wrote the call.""" + result, _ = _build(tmp_path, "import './dyn'\nexport const run = () => 1\n") + + edges = _file_edges(result, "src/dyn.ts", "src/target.ts") + assert edges, "no file-level edge for a dynamic import written inside a function" + # `dynamic_import`, not `imports_from`: it keeps the deferred nature legible, it is + # already in DEFAULT_AFFECTED_RELATIONS, and find_import_cycles reads only + # `imports_from`/`re_exports` — so the phantom file cycle of #1241 cannot come back + # through this edge the way a second `imports_from` might. + assert all(e["relation"] == "dynamic_import" for e in edges) + + +def test_affected_reaches_through_a_side_effect_importer(tmp_path: Path): + """`import './dyn'` binds no symbol, so nothing points at the enclosing function.""" + _, graph = _build(tmp_path, "import './dyn'\nexport const run = () => 1\n") + + assert _reaches(graph, "src/target.ts", "src/dyn.ts", depth=1) + assert _reaches(graph, "src/target.ts", "src/top.ts", depth=3) + + +def test_affected_reaches_through_a_namespace_importer(tmp_path: Path): + """`import * as ns` binds the module, not the function that defers the load.""" + _, graph = _build(tmp_path, "import * as ns from './dyn'\nexport const run = () => ns.load()\n") + + assert _reaches(graph, "src/target.ts", "src/top.ts", depth=3) + + +def test_affected_reaches_when_importer_names_a_different_symbol(tmp_path: Path): + """`other` is a sibling export; the edge into `load()` that used to rescue this is absent.""" + _, graph = _build(tmp_path, "import { other } from './dyn'\nexport const run = () => other\n") + + assert _reaches(graph, "src/target.ts", "src/top.ts", depth=3) + + +def test_call_site_precision_is_preserved(tmp_path: Path): + """The symbol-level edge must survive: `explain` still has to name the deferring function.""" + result, _ = _build(tmp_path, "import { load } from './dyn'\nexport const run = () => load()\n") + + tgt = _fid("src/target.ts") + symbol_edges = [ + e for e in result["edges"] + if e["target"] == tgt and e["source"] != _fid("src/dyn.ts") + and e.get("deferred") is True + ] + assert symbol_edges, "the symbol-level dynamic-import edge was replaced instead of added" + assert symbol_edges[0]["relation"] == "imports_from" + + +def test_module_level_dynamic_import_emits_no_duplicate(tmp_path: Path): + """At module level `caller_nid` IS the file node — emitting again would double-count.""" + result, _ = _build( + tmp_path, + "import './dyn'\nexport const run = () => 1\n", + dyn_src="export const p = import('./target')\n", + ) + + assert len(_file_edges(result, "src/dyn.ts", "src/target.ts")) == 1 + + +def test_one_file_deferring_the_same_module_twice_emits_one_file_edge(tmp_path: Path): + """Two functions, one dependency. The file-level edge dedupes on its own key.""" + result, _ = _build( + tmp_path, + "import './dyn'\nexport const run = () => 1\n", + dyn_src=( + "export async function a() {\n" + " return (await import('./target')).value\n" + "}\n" + "export async function b() {\n" + " return (await import('./target')).value\n" + "}\n" + ), + ) + + assert len(_file_edges(result, "src/dyn.ts", "src/target.ts")) == 1 diff --git a/tests/test_js_dynamic_imports.py b/tests/test_js_dynamic_imports.py index 1db30efe..5ee3a11c 100644 --- a/tests/test_js_dynamic_imports.py +++ b/tests/test_js_dynamic_imports.py @@ -93,10 +93,26 @@ def test_module_scope_dynamic_import_edges(tmp_path: Path): assert any(e["relation"] == "dynamic_import" for e in edges) -def test_ast_captured_dynamic_import_is_one_fact_not_two(tmp_path: Path): - """Dedupe: a dynamic import the AST pass already emitted (as a deferred - imports_from edge) must not be restated by the rescue as a second - dynamic_import edge to the same target.""" +def test_ast_captured_dynamic_import_still_gets_a_file_level_edge(tmp_path: Path): + """One import() inside a function, two granularities — #2584. + + This test used to assert `len(edges) == 1`, on the reasoning that the rescue would be + restating what the AST pass had already said. The two edges are not the same statement: + the AST pass anchors on `caller_nid`, which is the enclosing FUNCTION when the import() + is written inside one, while the rescue anchors on the FILE. Only the second is a fact + `affected` can walk, since it traverses file to file. + + Suppressing it cost real recall: on a ~700-file TS repo `affected --depth 3` returned 39 + of 49 truly affected files, precision 1.00, and more depth did not help — a dead end, not + a depth limit. The reverse walk reached the enclosing function and stopped there, because + the only edge pointing at it is `contains`, deliberately not in + DEFAULT_AFFECTED_RELATIONS. + + The dedupe is still right, just keyed wrong: it now matches on (source file, target) + rather than target alone, so the genuinely redundant case — a module-scope import(), + where `caller_nid` IS the file node — still collapses to one edge. That case is pinned + by `test_module_scope_dynamic_import_is_still_one_fact` below. + """ _write(tmp_path / "src/dep.ts", "export const dep = 1\n") importer = _write( tmp_path / "src/page.ts", @@ -109,9 +125,35 @@ def test_ast_captured_dynamic_import_is_one_fact_not_two(tmp_path: Path): result = extract([tmp_path / "src/dep.ts", importer], root=tmp_path) edges = _edges_to(result, "src/dep.ts") - assert len(edges) == 1, f"one import(), one edge — got {edges}" - assert edges[0]["relation"] == "imports_from" - assert edges[0].get("deferred") is True + page = _file_node_id(Path("src/page.ts")) + + # The precise fact: which function defers the load. `explain` reads this one. + symbol_level = [e for e in edges if e["source"] != page] + assert symbol_level, f"lost the call-site edge — got {edges}" + assert symbol_level[0]["relation"] == "imports_from" + assert symbol_level[0].get("deferred") is True + + # The traversable fact: this file depends on that module. `affected` reads this one. + file_level = [e for e in edges if e["source"] == page] + assert file_level, f"no file-level edge — `affected` cannot traverse this: {edges}" + assert file_level[0]["relation"] == "dynamic_import" + + +def test_module_scope_dynamic_import_is_still_one_fact(tmp_path: Path): + """At module scope `caller_nid` IS the file node, so the rescue is genuinely redundant. + + This is the half of the old dedupe that was always correct, kept as its own test so a + future change cannot quietly restore double-counting here while fixing #2584. + """ + _write(tmp_path / "src/dep.ts", "export const dep = 1\n") + importer = _write( + tmp_path / "src/boot2.ts", + "const { dep } = await import('./dep')\nexport const booted = dep\n", + ) + + result = extract([tmp_path / "src/dep.ts", importer], root=tmp_path) + + assert len(_edges_to(result, "src/dep.ts")) == 1 def test_static_import_alongside_dynamic_is_untouched(tmp_path: Path): @@ -130,7 +172,15 @@ def test_static_import_alongside_dynamic_is_untouched(tmp_path: Path): a_edges = _edges_to(result, "src/a.ts", "imports_from") assert a_edges and not any(e.get("deferred") for e in a_edges) - assert len(_edges_to(result, "src/b.ts")) == 1 + + # The point of this test is the STATIC edge above: the rescue must leave it alone, and + # in particular must not mark it deferred. The dynamic side is asserted only for the + # property that concerns this test — every edge to b.ts is flagged as deferred one way + # or another, so no arrow into b.ts can be mistaken for a static dependency. Its COUNT + # is #2584's subject, pinned in test_ast_captured_dynamic_import_still_gets_a_file_level_edge. + b_edges = _edges_to(result, "src/b.ts") + assert b_edges + assert all(e.get("deferred") or e["relation"] == "dynamic_import" for e in b_edges) def test_tsconfig_aliased_dynamic_import_edges(tmp_path: Path):