mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 00:36:39 +00:00
fix(js): an import from outside the corpus shadows indirect_call resolution (#2757)
An identifier bound by an import whose target resolves OUTSIDE the scanned corpus (e.g. a lucide-react icon) is now shadowed within the file, so using it as a value no longer fabricates an INFERRED indirect_call onto an unrelated same-named callable elsewhere in the corpus. The internal-vs-external decision is delegated to the existing import resolver, so a relative/in-corpus import still resolves to its real target. Same shadow family as #2241/#2568/#2685. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
safishamsi
co-authored by
Claude Opus 4.8
parent
774b12fe9e
commit
ceeafb05ce
@@ -1287,6 +1287,81 @@ def _js_module_bound_names(root, source: bytes) -> set[str]:
|
||||
walk(root)
|
||||
return bound
|
||||
|
||||
def _js_import_binds_external(raw: str, str_path: str) -> bool:
|
||||
"""True when a JS/TS import specifier names a module outside the scanned corpus.
|
||||
|
||||
Reuses `_resolve_js_import_target`, so this is graphify's own verdict rather
|
||||
than a second opinion: a specifier it cannot resolve is an external package
|
||||
(the `ref`-namespaced branch). The extra `node_modules` test covers the case
|
||||
where resolution *succeeds* but lands in a dependency tree — a `tsconfig`
|
||||
`paths` entry mapping a package to its own installed copy
|
||||
(`"lucide-react": ["./node_modules/lucide-react"]`) is common, and
|
||||
`node_modules` is pruned from every scan, so the target is never a node.
|
||||
"""
|
||||
resolved = _resolve_js_import_target(raw, str_path)
|
||||
if resolved is None:
|
||||
return False # empty specifier — binds nothing
|
||||
_target_nid, resolved_path = resolved
|
||||
if resolved_path is None:
|
||||
return True # unresolved after relative / alias / workspace lookup
|
||||
return "node_modules" in resolved_path.parts
|
||||
|
||||
|
||||
def _js_external_import_names(root, source: bytes, str_path: str) -> set[str]:
|
||||
"""Names an `import` binds to a module OUTSIDE the corpus.
|
||||
|
||||
An imported name is a module-scoped binding: within this file it denotes the
|
||||
imported symbol and nothing else. Neither shadow set collects it —
|
||||
`_js_local_bound_names` reads parameters and `variable_declarator`s and
|
||||
`_js_module_bound_names` only the latter — so the name reaches
|
||||
`_emit_indirect_ref` as an unresolved by-name reference, gets resolved against
|
||||
the corpus-wide label index, and fabricates an `indirect_call` (INFERRED, 0.8)
|
||||
to any unique same-named callable elsewhere in the corpus. That is the symptom
|
||||
already fixed for `catch` bindings, single-parameter arrows and untracked
|
||||
closures; an import binding is the same class of shadow, and a UI kit makes it
|
||||
land constantly because icon names (`Palette`, `Search`, `Filter`) collide with
|
||||
ordinary component names.
|
||||
|
||||
Only imports the corpus cannot contain are collected. A relative specifier
|
||||
resolves to a real file and that edge is the graph's whole point, so those
|
||||
names stay resolvable.
|
||||
"""
|
||||
bound: set[str] = set()
|
||||
|
||||
def _clause_names(clause) -> None:
|
||||
for c in clause.children:
|
||||
if c.type == "identifier": # import Default from "pkg"
|
||||
bound.add(_read_text(c, source))
|
||||
elif c.type == "namespace_import": # import * as NS from "pkg"
|
||||
for ident in c.children:
|
||||
if ident.type == "identifier":
|
||||
bound.add(_read_text(ident, source))
|
||||
elif c.type == "named_imports": # import { A, B as C } from "pkg"
|
||||
for spec in c.children:
|
||||
if spec.type != "import_specifier":
|
||||
continue
|
||||
idents = [g for g in spec.children if g.type == "identifier"]
|
||||
# `B as C` exposes both names; only the LAST one is bound here.
|
||||
if idents:
|
||||
bound.add(_read_text(idents[-1], source))
|
||||
|
||||
def walk(n) -> None:
|
||||
for c in n.children:
|
||||
if c.type == "import_statement":
|
||||
src_node = c.child_by_field_name("source")
|
||||
if src_node is not None:
|
||||
raw = _read_text(src_node, source).strip("\"'`")
|
||||
if _js_import_binds_external(raw, str_path):
|
||||
for child in c.children:
|
||||
if child.type == "import_clause":
|
||||
_clause_names(child)
|
||||
continue
|
||||
walk(c)
|
||||
|
||||
walk(root)
|
||||
return bound
|
||||
|
||||
|
||||
def _js_dispatch_value_idents(coll_node):
|
||||
"""Yield identifier value-nodes of a JS/TS object/array literal that are
|
||||
function-reference candidates: object property VALUES and shorthand properties
|
||||
@@ -2688,6 +2763,14 @@ def _extract_generic(
|
||||
|
||||
stem = _file_stem(path)
|
||||
str_path = str(path)
|
||||
# Names bound by an import of a module outside the corpus. Module-scoped, so it
|
||||
# is computed once per file and consulted from every scope — see
|
||||
# `_js_external_import_names`.
|
||||
js_external_imports: set[str] = (
|
||||
_js_external_import_names(root, source, str_path)
|
||||
if config.ts_module in ("tree_sitter_javascript", "tree_sitter_typescript")
|
||||
else set()
|
||||
)
|
||||
nodes: list[dict] = []
|
||||
edges: list[dict] = []
|
||||
seen_ids: set[str] = set()
|
||||
@@ -4540,6 +4623,11 @@ def _extract_generic(
|
||||
# shadowing: a param / local binding names a local value, not the module fn
|
||||
if ident_name in enclosing_locals or ident_name in ("self", "cls"):
|
||||
return
|
||||
# An import from outside the corpus binds the name for the whole module, so
|
||||
# it shadows in every scope — no unique same-named definition elsewhere in
|
||||
# the corpus is what this identifier refers to.
|
||||
if ident_name in js_external_imports:
|
||||
return
|
||||
_emit_indirect_by_name(ident_name, ident, scope_nid, context)
|
||||
|
||||
def _python_dispatch_value_idents(coll_node):
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
"""An import from outside the corpus must shadow indirect_call resolution.
|
||||
|
||||
`_js_local_bound_names` collects a function's locals from parameters and
|
||||
`variable_declarator` nodes, and `_js_module_bound_names` only the latter. A name
|
||||
introduced by `import { X } from "pkg"` is neither, so it was absent from both
|
||||
shadow sets: listing it in a dispatch table (`{ icon: X }`) or passing it on as a
|
||||
call argument read as an unresolved by-name reference, resolved against the
|
||||
corpus-wide label index, and fabricated an `indirect_call` edge (INFERRED, 0.8) to
|
||||
an unrelated same-named callable elsewhere in the corpus.
|
||||
|
||||
Same class as the `catch`-binding, single-parameter-arrow and untracked-closure
|
||||
shadows already fixed — an import is simply a module-scoped binding. A UI icon kit
|
||||
makes it land constantly: `Palette`, `Search`, `Filter` and `Menu` are icon exports
|
||||
*and* ordinary component names, so any repo with both grows cross-package edges
|
||||
between files that never referenced one another.
|
||||
|
||||
The guard asks `_resolve_js_import_target` rather than second-guessing it, so a
|
||||
relative import — which resolves to a real file, and whose edge is the whole point
|
||||
of the graph — keeps resolving. `node_modules` is tested separately because a
|
||||
`tsconfig` `paths` entry pointing a package at its own installed copy
|
||||
(`"lucide-react": ["./node_modules/lucide-react"]`) *does* resolve, to a tree every
|
||||
scan prunes.
|
||||
"""
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
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():
|
||||
target = base / name
|
||||
target.parent.mkdir(parents=True, exist_ok=True)
|
||||
target.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_external_named_import_emits_no_indirect_call(tmp_path):
|
||||
"""Reported shape: an icon imported from a UI kit must not become a fabricated
|
||||
indirect_call target because an unrelated component shares its name."""
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"Palette.tsx": "export function Palette() { return null; }\n",
|
||||
"Sidebar.tsx": (
|
||||
"import { Palette } from 'lucide-react';\n"
|
||||
"export function Sidebar() {\n"
|
||||
" return [{ label: 'personalise', icon: Palette }];\n"
|
||||
"}\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert all(t != nid["Palette"] for _s, t in indirect)
|
||||
|
||||
|
||||
def test_external_default_import_emits_no_indirect_call(tmp_path):
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"Chart.tsx": "export function Chart() { return null; }\n",
|
||||
"Panel.tsx": (
|
||||
"import Chart from 'some-chart-lib';\n"
|
||||
"export function Panel(sink) { sink.register(Chart); }\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert all(t != nid["Chart"] for _s, t in indirect)
|
||||
|
||||
|
||||
def test_external_namespace_import_emits_no_indirect_call(tmp_path):
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"Utils.ts": "export function Utils() { return 1; }\n",
|
||||
"run.ts": (
|
||||
"import * as Utils from 'vendor-utils';\n"
|
||||
"export function run(sink) { sink.push(Utils); }\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert all(t != nid["Utils"] for _s, t in indirect)
|
||||
|
||||
|
||||
def test_aliased_import_shadows_the_local_name_only(tmp_path):
|
||||
"""`import { Search as Find }` binds `Find` in this file, not `Search`. The
|
||||
shadow must follow the binding: a same-named local `Find` is not referenced
|
||||
here, while an unrelated `Search` definition stays reachable by its own name."""
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"Find.ts": "export function Find() { return 1; }\n",
|
||||
"app.ts": (
|
||||
"import { Search as Find } from 'icon-pack';\n"
|
||||
"export function app(sink) { sink.push(Find); }\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert all(t != nid["Find"] for _s, t in indirect)
|
||||
|
||||
|
||||
def test_tsconfig_alias_into_node_modules_still_counts_as_external(tmp_path):
|
||||
"""A `paths` entry pointing a package at its own installed copy resolves to a
|
||||
real path — inside `node_modules`, which every scan prunes, so no node is ever
|
||||
created for it. Resolution succeeding must not read as 'internal'."""
|
||||
(tmp_path / "tsconfig.json").write_text(
|
||||
'{"compilerOptions": {"paths": {"icon-kit": ["./node_modules/icon-kit"]}}}\n'
|
||||
)
|
||||
nm = tmp_path / "node_modules" / "icon-kit"
|
||||
nm.mkdir(parents=True)
|
||||
(nm / "index.js").write_text("export function Palette(){}\n")
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"Palette.tsx": "export function Palette() { return null; }\n",
|
||||
"Bar.tsx": (
|
||||
"import { Palette } from 'icon-kit';\n"
|
||||
"export function Bar() { return [{ icon: Palette }]; }\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert all(t != nid["Palette"] for _s, t in indirect)
|
||||
|
||||
|
||||
def test_relative_import_still_resolves(tmp_path):
|
||||
"""The counter-test that bounds the fix: an import of a file INSIDE the corpus
|
||||
is exactly the relationship the graph exists to record, so its name must stay
|
||||
resolvable. Shadowing every import would delete real edges."""
|
||||
r, nid = _extract_js_dir(tmp_path, {
|
||||
"widgets.ts": "export function Widget() { return null; }\n",
|
||||
"host.ts": (
|
||||
"import { Widget } from './widgets';\n"
|
||||
"export function host(sink) { sink.push(Widget); }\n"
|
||||
),
|
||||
})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert (nid["host"], nid["Widget"]) in indirect
|
||||
|
||||
|
||||
def test_unimported_same_file_callable_still_emits(tmp_path):
|
||||
"""Widening the shadow set must not blanket-suppress a file that also happens to
|
||||
import something external: an unshadowed by-name reference still emits."""
|
||||
r, nid = _extract_js_dir(tmp_path, {"a.ts": (
|
||||
"import { Icon } from 'icon-pack';\n"
|
||||
"function handler(x) { return x; }\n"
|
||||
"export function run(pool) { pool.submit(handler); }\n"
|
||||
)})
|
||||
indirect = _rels(r, "indirect_call")
|
||||
assert (nid["run"], nid["handler"]) in indirect
|
||||
Reference in New Issue
Block a user