mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-13 23:49:14 +00:00
fix(ts): type-only imports no longer manufacture Import Cycles (#3123)
A type-only import (import type { T } from './m') produced an import edge like a value
import, so type-only round-trips showed up as false import cycles. Stamp type_only on the
module and symbol edges from type-only statements (correctly not firing for a mixed
import { type A, B }, which keeps its value edge) and exclude stamped edges from
find_import_cycles, mirroring the existing deferred-import exclusion.
This commit is contained in:
1 parent
8906834ff0
commit
b84b4a7316
5 files changed
+179
-2
No files matched your search
@@ -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:
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
Reference in new issue
Block a user