diff --git a/graphify/analyze.py b/graphify/analyze.py index 0707e2be7..844a7c0d2 100644 --- a/graphify/analyze.py +++ b/graphify/analyze.py @@ -679,6 +679,11 @@ def find_import_cycles( # hard file-level cycle, so they are excluded from cycle detection (#1241). if data.get("deferred"): continue + # Type-only imports/re-exports (`import type` / `export type ... from`) + # are erased at compile time - a cycle that closes through one cannot + # exist at runtime (#3123). The edge itself stays in the graph. + if data.get("type_only"): + continue src_file_attr = data.get("source_file", "") if not isinstance(src_file_attr, str) or not src_file_attr: diff --git a/graphify/extract.py b/graphify/extract.py index 3477fa788..94a0d4f60 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -411,6 +411,19 @@ def _import_js(node, source: bytes, file_nid: str, stem: str, edges: list, str_p if not has_from: return + # `import type {...} from` / `export type {...} from` are erased by the + # TypeScript compiler: no runtime emit, no module-graph edge. The + # dependency is still real for "what references this type", so the edge is + # stamped `type_only` rather than dropped, and Import Cycles excludes it + # the way it already excludes deferred `import(...)` (#1241) - on the + # reporter's corpus all 3 reported cycles closed only through these + # (#3123). The grammar keeps `import type from './x'` (a default binding + # NAMED type) distinct: there `type` sits inside the import_clause, not as + # a bare keyword child of the statement. A mixed `import { type B, C }` + # stays a runtime edge - C is a runtime import. + is_type_only = any( + child.type == "type" and not child.is_named for child in node.children + ) resolved_path: "Path | None" = None module_string = None for child in node.children: @@ -453,6 +466,8 @@ def _import_js(node, source: bytes, file_nid: str, stem: str, edges: list, str_p # back onto the importer's own variant, a phantom self-loop (#1814). if resolved_path is not None: edge["target_file"] = str(resolved_path) + if is_type_only: + edge["type_only"] = True edges.append(edge) # Emit symbol-level edges for named imports/re-exports from local/aliased files. @@ -478,6 +493,7 @@ def _import_js(node, source: bytes, file_nid: str, stem: str, edges: list, str_p if sym == "default": continue # skip default re-exports for ID matching edges.append({ + **({"type_only": True} if is_type_only else {}), "source": file_nid, "target": _make_id(target_stem, sym), "relation": "re_exports", diff --git a/graphify/extractors/models.py b/graphify/extractors/models.py index bd8677aec..014907c0d 100644 --- a/graphify/extractors/models.py +++ b/graphify/extractors/models.py @@ -85,12 +85,18 @@ class _SymbolExportFact: local_name: str | None = None target_path: Path | None = None target_name: str | None = None + # `export type { X } from ...`: erased at compile time, so the re-export + # edge it produces is stamped type_only and excluded from Import Cycles + # (#3123). The fact itself still participates in symbol resolution - a + # type import resolved through a type-only barrel is itself type-only. + type_only: bool = False @dataclass(frozen=True) class _StarExportFact: file_path: Path target_path: Path line: int + type_only: bool = False @dataclass(frozen=True) class _NamespaceExportFact: @@ -98,6 +104,7 @@ class _NamespaceExportFact: exported_name: str target_path: Path line: int + type_only: bool = False @dataclass(frozen=True) class _SymbolUseFact: diff --git a/graphify/extractors/resolution.py b/graphify/extractors/resolution.py index 5369637ae..4982c7d05 100644 --- a/graphify/extractors/resolution.py +++ b/graphify/extractors/resolution.py @@ -876,7 +876,7 @@ def _apply_symbol_resolution_facts( for edge in edges } - def add_edge(source: str, target: str, relation: str, context: str, line: int, source_path: Path, target_file: str | None = None, local_alias: str | None = None) -> None: + def add_edge(source: str, target: str, relation: str, context: str, line: int, source_path: Path, target_file: str | None = None, local_alias: str | None = None, type_only: bool = False) -> None: key = (source, target, relation, context or "") if key in existing_edges: return @@ -901,6 +901,9 @@ def _apply_symbol_resolution_facts( # cross-file member-call resolver match `alias.func()` (#2082). if local_alias is not None: edge["local_alias"] = local_alias + # Erased at compile time (#3123): Import Cycles skips these edges. + if type_only: + edge["type_only"] = True edges.append(edge) for declaration in facts.declarations: @@ -948,6 +951,7 @@ def _apply_symbol_resolution_facts( star_fact.line, star_fact.file_path, target_file=str(path_by_resolved.get(target_path, target_path)), + type_only=star_fact.type_only, ) for namespace_fact in facts.namespace_exports: @@ -979,6 +983,7 @@ def _apply_symbol_resolution_facts( namespace_fact.line, namespace_fact.file_path, target_file=str(path_by_resolved.get(target_path, target_path)), + type_only=namespace_fact.type_only, ) for export_fact in facts.exports: @@ -1004,6 +1009,7 @@ def _apply_symbol_resolution_facts( export_fact.line, export_fact.file_path, target_file=str(path_by_resolved.get(origin[0], origin[0])), + type_only=export_fact.type_only, ) def resolve_exported_origin(target_path: Path, imported_name: str, seen: set[tuple[Path, str]] | None = None) -> tuple[Path, str]: @@ -1542,6 +1548,13 @@ def _collect_js_symbol_resolution_facts(paths: list[Path], facts: _SymbolResolut raw_module = _js_module_specifier(node, source) export_clause = _js_export_clause(node) + # `export type { X } from ...` / `export type * from ...`: the + # statement-level `type` keyword is a bare anonymous child; the + # default binding NAMED type sits inside the clause instead (#3123). + stmt_type_only = any( + child.type == "type" and not child.is_named + for child in node.children + ) if raw_module is not None: target_path = _resolve_js_module_path(raw_module, path.parent) if target_path is None: @@ -1555,11 +1568,13 @@ def _collect_js_symbol_resolution_facts(paths: list[Path], facts: _SymbolResolut namespace_name, target_path, node.start_point[0] + 1, + type_only=stmt_type_only, ) ) elif _js_export_statement_is_star(node): facts.star_exports.append( - _StarExportFact(path, target_path, node.start_point[0] + 1) + _StarExportFact(path, target_path, node.start_point[0] + 1, + type_only=stmt_type_only) ) if export_clause is not None: for original_name, exported_name in _js_named_specifiers( @@ -1572,6 +1587,7 @@ def _collect_js_symbol_resolution_facts(paths: list[Path], facts: _SymbolResolut node.start_point[0] + 1, target_path=target_path, target_name=original_name, + type_only=stmt_type_only, ) ) continue diff --git a/tests/test_type_only_import_cycles.py b/tests/test_type_only_import_cycles.py new file mode 100644 index 000000000..b1a3ea9f5 --- /dev/null +++ b/tests/test_type_only_import_cycles.py @@ -0,0 +1,133 @@ +"""Type-only imports must not manufacture Import Cycles (#3123). + +`import type` / `export type ... from` are erased by the TypeScript compiler +— no runtime emit, no module-graph edge — yet the extractor emitted an +ordinary `imports_from` edge for them, and every one of the reporter's +reported cycles (3 of 3 on a ~7,900-node repo) closed only through such +edges. The report even flagged code whose comment said the `export type` +form was chosen for "zero runtime emit"; the mitigation was reported as the +defect. + +The edge is kept — the type dependency is real for "what references this +type" — stamped `type_only`, and `find_import_cycles` excludes it the way +it already excludes deferred `import(...)` (#1241). +""" +from __future__ import annotations + +import tempfile +from pathlib import Path + +from graphify.analyze import find_import_cycles +from graphify.build import build_from_json +from graphify.extract import extract + + +def _extract(tmp_path, files: dict[str, str]): + for name, body in files.items(): + p = tmp_path / name + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(body, encoding="utf-8") + return extract([tmp_path / n for n in files], cache_root=Path(tempfile.mkdtemp()), + root=tmp_path, parallel=False) + + +def _module_edges(result, rel="imports_from"): + return [e for e in result["edges"] if e["relation"] == rel] + + +# --------------------------------------------------------------------------- +# The stamp +# --------------------------------------------------------------------------- + +def test_import_type_statement_is_stamped(tmp_path): + r = _extract(tmp_path, { + "types.ts": "export type Variables = { requestId: string };\n", + "v1.ts": 'import type { Variables } from "./types.js";\n' + "export function makeRouter() { return {} as unknown; }\n", + }) + edges = [e for e in _module_edges(r) if e["source_file"].endswith("v1.ts")] + assert edges and all(e.get("type_only") for e in edges) + + +def test_export_type_reexport_is_stamped(tmp_path): + r = _extract(tmp_path, { + "v1.ts": "export type RouterType = { v: number };\n", + "types.ts": 'export type { RouterType } from "./v1.js";\n', + }) + stamped = [e for e in r["edges"] + if e["source_file"].endswith("types.ts") + and e["relation"] in ("imports_from", "re_exports")] + assert stamped and all(e.get("type_only") for e in stamped) + + +def test_a_default_binding_named_type_is_a_runtime_import(tmp_path): + """`import type from './x.js'` imports a value whose name is `type` — + erased by nothing.""" + r = _extract(tmp_path, { + "x.ts": "const type = 1;\nexport default type;\n", + "user.ts": 'import type from "./x.js";\nexport const y = type;\n', + }) + edges = [e for e in _module_edges(r) if e["source_file"].endswith("user.ts")] + assert edges and not any(e.get("type_only") for e in edges) + + +def test_a_mixed_specifier_list_stays_a_runtime_edge(tmp_path): + """`import { type B, C }` still imports C at runtime.""" + r = _extract(tmp_path, { + "b.ts": "export type B = number;\nexport const C = 1;\n", + "user.ts": 'import { type B, C } from "./b.js";\nexport const y = C;\n', + }) + edges = [e for e in _module_edges(r) if e["source_file"].endswith("user.ts")] + assert edges and not any(e.get("type_only") for e in edges) + + +def test_plain_imports_are_untouched(tmp_path): + r = _extract(tmp_path, { + "b.ts": "export const C = 1;\n", + "user.ts": 'import { C } from "./b.js";\nexport const y = C;\n', + }) + edges = [e for e in _module_edges(r) if e["source_file"].endswith("user.ts")] + assert edges and not any("type_only" in e for e in edges) + + +# --------------------------------------------------------------------------- +# The cycles — the reporter's minimal repro +# --------------------------------------------------------------------------- + +def _cycles(tmp_path, files): + r = _extract(tmp_path, files) + # directed=True mirrors a --directed build; in the default undirected + # graph two module edges between one file pair collapse to a single + # stored edge, so a synthetic 2-cycle needs distinct endpoints anyway. + G = build_from_json({"nodes": r["nodes"], "edges": r["edges"], "hyperedges": []}, + root=str(tmp_path), directed=True) + return find_import_cycles(G) + + +def test_the_reporters_two_file_cycle_is_gone(tmp_path): + cycles = _cycles(tmp_path, { + "types.ts": ("export type Variables = { requestId: string };\n" + 'export type { RouterType } from "./v1.js";\n'), + "v1.ts": ('import type { Variables } from "./types.js";\n' + "export type RouterType = { v: Variables };\n" + "export function makeRouter() { return {} as RouterType; }\n"), + }) + assert cycles == [], cycles + + +def test_a_real_runtime_cycle_is_still_reported(tmp_path): + cycles = _cycles(tmp_path, { + "a.ts": 'import { b } from "./b.js";\nexport const a = 1;\n', + "b.ts": 'import { a } from "./a.js";\nexport const b = 2;\n', + }) + assert len(cycles) == 1 + assert sorted(Path(f).name for f in cycles[0]["cycle"]) == ["a.ts", "b.ts"] + + +def test_a_cycle_with_one_type_only_leg_is_not_a_cycle(tmp_path): + """One runtime leg + one type-only leg: no runtime cycle exists.""" + cycles = _cycles(tmp_path, { + "a.ts": 'import { b } from "./b.js";\nexport type AT = number;\nexport const a = 1;\n', + "b.ts": 'import type { AT } from "./a.js";\nexport const b = 2;\n', + }) + assert cycles == [], cycles