Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions graphify/extractors/dart.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from pathlib import Path
from graphify.extractors.base import _file_stem, _make_id
from graphify.extractors.resolution import _resolve_dart_import_target


def extract_dart(path: Path) -> dict:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regressionextract_dart()

fans out to 7 callees (efferent coupling); 9 callers depend on it (afferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, but I don't think there's a fix here — the new callee is the change.

extract_dart previously synthesized its import targets inline (_make_id(pkg) on the raw uri string), which is precisely why they never reached a file node. Resolving them means calling a resolver, so the +1 efferent edge is the feature rather than a side effect. The alternative — inlining the resolution back into extract_dart — would keep the coupling number flat while making the function considerably larger and duplicating logic that resolution.py already owns for JS/TS, C and Lua.

For what it's worth, this is the same shape extract_objc already has: it calls _resolve_c_include_path from the same module for exactly the same reason.

Happy to restructure if you'd rather the call site sit somewhere else.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regressionextract_dart()

fans out to 7 callees (efferent coupling); 9 callers depend on it (afferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regressionextract_dart()

fans out to 7 callees (efferent coupling); 9 callers depend on it (afferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

Expand Down Expand Up @@ -500,17 +501,21 @@ def _find_matching_brace(text: str, start_pos: int) -> int:
add_edge(nid, route_nid, "navigates", context="route_object")

# 6. Imports and Exports
for m in re.finditer(r"""^\s*import\s+['"]([^'"]+)['"]""", src_clean, re.MULTILINE):
pkg = m.group(1)
tgt_nid = _make_id(pkg)
add_node(tgt_nid, pkg, source_file=None)
add_edge(file_nid, tgt_nid, "imports")

for m in re.finditer(r"""^\s*export\s+['"]([^'"]+)['"]""", src_clean, re.MULTILINE):
pkg = m.group(1)
tgt_nid = _make_id(pkg)
add_node(tgt_nid, pkg, source_file=None)
add_edge(file_nid, tgt_nid, "exports")
for kind, pattern in (("imports", r"""^\s*import\s+['"]([^'"]+)['"]"""),
("exports", r"""^\s*export\s+['"]([^'"]+)['"]""")):
for m in re.finditer(pattern, src_clean, re.MULTILINE):
pkg = m.group(1)
# Resolve the URI so the edge target is the id that file's own node
# carries. Without this every Dart import edge pointed at a bare string
# node with source_file=None, so reverse traversal ("who imports this
# file", `affected`) was blind on Dart while Python/TS resolved (#2329).
resolved = _resolve_dart_import_target(pkg, str(path))
if resolved is not None:
add_edge(file_nid, _make_id(str(resolved)), kind, context="import")
else:
Comment on lines +514 to +515

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export/import edge target uses resolved file path id, not the file node's own id scheme — agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review

Graphify suggests a fix:

Suggested change
add_edge(file_nid, _make_id(str(resolved)), kind, context="import")
else:
add_edge(file_nid, _make_id(_file_stem(resolved)), kind, context="import")
else:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Measured this rather than argue it, since the finding is flagged unverified — it doesn't reproduce, and I'd rather not change the id scheme on a hunch.

I applied the suggestion verbatim (_make_id(_file_stem(resolved))), reinstalled and re-ran the corpus from the PR description (433 Dart files): 2129/2129 resolved, same as str(resolved). Both forms work, because the id-remap post-pass re-derives the canonical repo-relative form from source_file and washes the difference out.

Keeping str(resolved) for two concrete reasons rather than a preference:

  1. It is how the file node itself is built — dart.py:35, file_nid = _make_id(str(path)). Matching the node's own construction is correct by definition; the other form relies on downstream normalization happening to agree.
  2. It's the existing precedent in this repo: objc.py:165 is exactly add_edge(file_nid, _make_id(str(resolved)), ...), and its comment says the bare-stem id doesn't survive _disambiguate_colliding_node_ids when a .h/.m pair exists (Bug: ObjC extractor — 4 bugs causing ~60% of relationships to be silently dropped #1475) — which argues specifically against the _file_stem form.

I also probed the one case where the two could genuinely diverge, since I call .resolve() and the node doesn't: a corpus reached through a symlink. Still 2129/2129.

Happy to switch if there's a scenario I haven't constructed — I just couldn't produce one where the current form is the weaker of the two.

tgt_nid = _make_id(pkg)
add_node(tgt_nid, pkg, source_file=None)
add_edge(file_nid, tgt_nid, kind)

# 7. Generic Invocations / Type Lookups (Universal Dependency Lookup)
# Matches any method call with type parameters: methodName<Type>() or object.methodName<Type>()
Expand Down
181 changes: 181 additions & 0 deletions graphify/extractors/resolution.py
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,187 @@ def _resolve_c_include_path(raw: str, str_path: str) -> "Path | None":
return candidate
return None

_DART_PUBSPEC_NAME_RE = re.compile(r"""^name:\s*['"]?([A-Za-z_]\w*)['"]?\s*(?:#.*)?$""", re.MULTILINE)

_DART_WORKSPACE_RE = re.compile(r"^workspace:\s*(?:#.*)?$", re.MULTILINE)

_DART_PACKAGE_INDEX_CACHE: "dict[str, tuple[tuple[str, str], ...]]" = {}

_DART_MAX_PARENTS = 40


def _dart_pubspec_name(pubspec: Path) -> "str | None":
"""Read the `name:` of a pubspec.yaml without a YAML dependency."""
try:
text = pubspec.read_text(encoding="utf-8", errors="replace")
except OSError:
return None
m = _DART_PUBSPEC_NAME_RE.search(text)
return m.group(1) if m else None


def _dart_workspace_members(pubspec: Path) -> "list[Path]":
"""Members listed under a pub-workspace `workspace:` block (#pub workspaces).

Only the top-level block is read; the entries are directory paths relative to
the pubspec's own directory:

workspace:
- apps/client_app
- packages/pricing_catalog
"""
try:
text = pubspec.read_text(encoding="utf-8", errors="replace")
except OSError:
return []
m = _DART_WORKSPACE_RE.search(text)
if m is None:
return []
members: list[Path] = []
for line in text[m.end():].splitlines():
if not line.strip():
continue
entry = re.match(r"^\s+-\s+(?:['\"])?([^'\"#\s]+)(?:['\"])?\s*(?:#.*)?$", line)
if entry is None:
break # block ended (a new top-level key, or a non-list line)
members.append(pubspec.parent / entry.group(1))
return members


def _dart_package_index(start_dir: str) -> "tuple[tuple[str, str], ...]":
"""Map every reachable Dart package name to its package root directory.

Walks up from `start_dir` recording each `pubspec.yaml` it passes, and at each
one also records the members of a pub `workspace:` block, so an import of a
sibling package in a monorepo resolves as well as the file's own package.

Cached per starting directory; the result is a tuple of (name, root) pairs so
it stays immutable across callers.
"""
cached = _DART_PACKAGE_INDEX_CACHE.get(start_dir)
if cached is not None:
return cached

found: dict[str, str] = {}
try:
here = Path(start_dir).resolve()
except OSError:
here = Path(start_dir)
for parent in [here, *here.parents][:_DART_MAX_PARENTS]:
pubspec = parent / "pubspec.yaml"
if not pubspec.is_file():
continue
name = _dart_pubspec_name(pubspec)
if name:
found.setdefault(name, str(parent))
for member in _dart_workspace_members(pubspec):
member_spec = member / "pubspec.yaml"
if not member_spec.is_file():
continue
member_name = _dart_pubspec_name(member_spec)
if member_name:
found.setdefault(member_name, str(member))

result = tuple(sorted(found.items()))
_DART_PACKAGE_INDEX_CACHE[start_dir] = result
return result


def _dart_lib_root(start_dir: Path) -> "Path | None":
"""The `lib/` of the package owning `start_dir`, if it is inside one.

A directory only counts when its parent holds a pubspec.yaml, so a `lib/`
belonging to something else (an asset tree, a vendored copy) is not mistaken
for a package root.
"""
try:
here = start_dir.resolve()
except OSError:
return None
for parent in [here, *here.parents][:_DART_MAX_PARENTS]:
if parent.name == "lib" and (parent.parent / "pubspec.yaml").is_file():
return parent
return None


def _resolve_dart_import_target(raw: str, str_path: str) -> "Path | None":
"""Resolve a Dart `import`/`export`/`part` URI to a real file on disk.

Dart resolution is fully deterministic — there is no extension guessing, no
index file, and no alias config, so only three forms exist:

'dart:async' SDK, always external -> None
'../cell/cell_widget.dart' relative to the importing file
'package:app/x/y.dart' `lib/x/y.dart` of package `app`

Returns the resolved path, or None when the URI names something outside the
scanned corpus (the SDK, a pub dependency, a dangling path) — mirroring
`_resolve_c_include_path`, so the caller keeps its existing external-node
behaviour untouched.
"""
if not raw:
return None
if raw.startswith("dart:"):
return None

try:
start_dir = Path(str_path).parent
except Exception:
return None

if raw.startswith("package:"):
pkg, _, sub = raw[len("package:"):].partition("/")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_resolve_dart_import_target() — high coupling complexity (Ca·Ce = 12)

Graphify suggests a fix:

Suggested change
def _resolve_dart_import_target(raw: str, str_path: str) -> "Path | None":
"""Resolve a Dart `import`/`export`/`part` URI to a real file on disk.
Dart resolution is fully deterministicthere is no extension guessing, no
index file, and no alias config, so only three forms exist:
'dart:async' SDK, always external -> None
'../cell/cell_widget.dart' relative to the importing file
'package:app/x/y.dart' `lib/x/y.dart` of package `app`
Returns the resolved path, or None when the URI names something outside the
scanned corpus (the SDK, a pub dependency, a dangling path) — mirroring
`_resolve_c_include_path`, so the caller keeps its existing external-node
behaviour untouched.
"""
if not raw:
return None
if raw.startswith("dart:"):
return None
try:
start_dir = Path(str_path).parent
except Exception:
return None
if raw.startswith("package:"):
pkg, _, sub = raw[len("package:"):].partition("/")
def _resolve_dart_uri(raw: str, start_dir: "Path") -> "Path | None":
"""Resolve a non-SDK Dart URI against `start_dir`.
Split out of `_resolve_dart_import_target` so that entry point stays a thin
dispatcher; see its docstring for the URI forms handled here.
"""
if raw.startswith("package:"):
pkg, _, sub = raw[len("package:"):].partition("/")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted in fb3ba1c, essentially as suggested. _resolve_dart_import_target is now a thin dispatcher — it guards the empty and dart: cases and derives the start directory — and _resolve_dart_uri selects between _resolve_dart_package_uri and _resolve_dart_relative_uri.

I split the relative branch out as its own function rather than leaving it inline, because it carries the package:-uri clamping rule and its explanation, which is the one non-obvious part of the file; on its own it reads as a unit.

Behaviour is unchanged: same 13 tests, and the corpus in the PR description still measures 2129/2129.

if not pkg or not sub:
return None
for name, root in _dart_package_index(str(start_dir)):
if name != pkg:
continue
try:
candidate = (Path(root) / "lib" / sub).resolve()
except OSError:
return None
return candidate if candidate.is_file() else None
return None

if raw.startswith(("http:", "https:", "asset:")):
return None

# A relative URI is resolved against the importing library's OWN uri, and for
# anything under `lib/` that uri is `package:<name>/...`, not a file path. The
# difference is not cosmetic: RFC 3986 drops `..` segments that would escape the
# base, so from `package:app/core/data/x.dart` an import of
# `../../../features/y.dart` CLAMPS at the package root and resolves to
# `package:app/features/y.dart`, while the same join over the file path escapes
# `lib/` and lands on a file that does not exist. Dart accepts the first — this
# is live, compiling code in real repos — so resolving on the filesystem alone
# reports a false miss on exactly those imports. Walk the segments against
# `lib/` and clamp there to match.
lib_root = _dart_lib_root(start_dir)
if lib_root is not None:
try:
segments = list(start_dir.resolve().relative_to(lib_root).parts)
except ValueError:
segments = []
for part in raw.split("/"):
if part in ("", "."):
continue
if part == "..":
if segments:
segments.pop()
continue # already at the package root: clamp, as `package:` does
segments.append(part)
candidate = lib_root.joinpath(*segments)
return candidate if candidate.is_file() else None

# Outside `lib/` (bin/, test/, tool/, a loose script) the base really is a file
# uri, so a plain join is the correct semantics.
try:
candidate = (start_dir / raw).resolve()
except OSError:
return None
return candidate if candidate.is_file() else None


def _resolve_lua_import_target(raw_module: str, str_path: str) -> str:
"""Resolve a Lua require() module name to a node id.

Expand Down
124 changes: 124 additions & 0 deletions tests/test_dart.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
from pathlib import Path

from graphify.extract import extract_dart, _make_id, _file_stem
from graphify.extractors.resolution import (
_DART_PACKAGE_INDEX_CACHE,
_resolve_dart_import_target,
)


class TestDart(unittest.TestCase):
Expand Down Expand Up @@ -636,5 +640,125 @@ class ChildClass extends Bloc<Pair<UserEvent, MyState>, State> {}
self.assertEqual(nav_edge["target"], "route_home_id_123_type_auth")



class TestDartImportResolution(unittest.TestCase):
"""Dart import/export URIs must land on the id of the file they name (#2329).

Before this, every Dart import edge targeted a bare string node with
source_file=None, so reverse traversal ("who imports this file", `affected`)
returned nothing on Dart while Python and TypeScript resolved.
"""

def setUp(self):
self.temp_dir = tempfile.TemporaryDirectory()
self.root = Path(self.temp_dir.name).resolve()
_DART_PACKAGE_INDEX_CACHE.clear()

def tearDown(self):
_DART_PACKAGE_INDEX_CACHE.clear()
self.temp_dir.cleanup()

def _pkg(self, name, at=".", workspace=None):
d = (self.root / at).resolve()
(d / "lib").mkdir(parents=True, exist_ok=True)
body = "name: {}\n".format(name)
if workspace:
body += "workspace:\n" + "".join(" - {}\n".format(m) for m in workspace)
(d / "pubspec.yaml").write_text(body)
return d

def _write(self, pkg_dir, rel, text=""):
f = pkg_dir / "lib" / rel
f.parent.mkdir(parents=True, exist_ok=True)
f.write_text(textwrap.dedent(text))
return f

def _import_targets(self, path):
out = extract_dart(path)
return {
e["target"]
for e in out["edges"]
if e["relation"] in ("imports", "exports")
}

def test_relative_import_targets_the_real_file_node(self):
pkg = self._pkg("app")
target = self._write(pkg, "core/data/auth_repository.dart", "class AuthRepo {}")
importer = self._write(
pkg, "features/auth/login.dart",
"import '../../core/data/auth_repository.dart';",
)
self.assertIn(_make_id(str(target)), self._import_targets(importer))

def test_package_import_of_own_package_resolves_through_pubspec_name(self):
pkg = self._pkg("app")
target = self._write(pkg, "config/theme.dart", "class Theme {}")
importer = self._write(
pkg, "features/home.dart", "import 'package:app/config/theme.dart';"
)
self.assertIn(_make_id(str(target)), self._import_targets(importer))

def test_excess_parent_segments_clamp_at_the_package_root(self):
"""A relative uri is resolved against the library's own `package:` uri, and
RFC 3986 drops `..` segments that would escape the base. So from
`package:app/core/data/x.dart` an import of `../../../features/y.dart`
resolves to `package:app/features/y.dart` — real, compiling Dart that a
plain filesystem join reports as a miss (it escapes `lib/`)."""
pkg = self._pkg("app")
target = self._write(pkg, "features/auth/auth_repository.dart", "class AuthRepo {}")
importer = self._write(
pkg, "core/data/prefs.dart",
"import '../../../features/auth/auth_repository.dart';",
)
self.assertIn(_make_id(str(target)), self._import_targets(importer))

def test_export_resolves_like_import(self):
pkg = self._pkg("app")
target = self._write(pkg, "models/user.dart", "class User {}")
importer = self._write(pkg, "models.dart", "export 'models/user.dart';")
self.assertIn(_make_id(str(target)), self._import_targets(importer))

def test_workspace_sibling_package_resolves(self):
self._pkg("root", workspace=["apps/app", "packages/shared"])
app = self._pkg("app", at="apps/app")
shared = self._pkg("shared", at="packages/shared")
target = self._write(shared, "pricing.dart", "class Pricing {}")
importer = self._write(
app, "main.dart", "import 'package:shared/pricing.dart';"
)
self.assertIn(_make_id(str(target)), self._import_targets(importer))

def test_sdk_and_third_party_imports_stay_external(self):
pkg = self._pkg("app")
importer = self._write(
pkg, "main.dart",
"""\
import 'dart:async';
import 'package:flutter/material.dart';
import 'package:app/missing.dart';
""",
)
targets = self._import_targets(importer)
for raw in ("dart:async", "package:flutter/material.dart", "package:app/missing.dart"):
self.assertIn(_make_id(raw), targets, raw)

def test_outside_lib_does_not_clamp(self):
"""In bin/ or tool/ the library's base really is a file uri, so an escaping
`..` is a genuine miss and must stay external rather than silently
clamping onto an unrelated file."""
pkg = self._pkg("app")
self._write(pkg, "helper.dart", "class Helper {}")
(pkg / "bin").mkdir(exist_ok=True)
script = pkg / "bin" / "run.dart"
script.write_text("import '../../../lib/helper.dart';")
self.assertIn(_make_id("../../../lib/helper.dart"), self._import_targets(script))

def test_resolver_returns_none_for_unresolvable_uris(self):
pkg = self._pkg("app")
f = self._write(pkg, "main.dart", "")
for raw in ("", "dart:core", "package:other/x.dart", "package:app", "./gone.dart"):
self.assertIsNone(_resolve_dart_import_target(raw, str(f)), raw)


if __name__ == "__main__":
unittest.main()