diff --git a/code_review_graph/parser.py b/code_review_graph/parser.py index 4fe3bd75..20a9b501 100644 --- a/code_review_graph/parser.py +++ b/code_review_graph/parser.py @@ -6,10 +6,12 @@ from __future__ import annotations +import ast import hashlib import json import logging import re +import threading from dataclasses import dataclass, field from pathlib import Path from typing import NamedTuple, Optional @@ -33,6 +35,10 @@ class CellInfo(NamedTuple): re.IGNORECASE, ) +_PYTHON_STAR_CACHE_MAX = 15_000 +_PYTHON_STAR_EXPORT_CACHE: dict[tuple[str, int, int], dict[str, str]] = {} +_PYTHON_STAR_EXPORT_CACHE_LOCK = threading.RLock() + # SQL keywords that can appear after FROM/JOIN but are NOT table names. _SQL_KEYWORDS: frozenset[str] = frozenset({ "SELECT", "WHERE", "GROUP", "ORDER", "HAVING", "LIMIT", "OFFSET", @@ -887,6 +893,7 @@ class CodeParser: _MODULE_CACHE_MAX = 15_000 # Evict cache to cap memory on huge monorepos def __init__(self, repo_root: Optional[Path] = None) -> None: + self._repo_root = Path(repo_root).resolve() if repo_root is not None else None self._parsers: dict[str, object] = {} self._module_file_cache: dict[str, Optional[str]] = {} self._export_symbol_cache: dict[str, Optional[str]] = {} @@ -1109,6 +1116,10 @@ def parse_bytes(self, path: Path, source: bytes) -> tuple[list[NodeInfo], list[E import_map, defined_names = self._collect_file_scope( tree.root_node, language, source, ) + if language == "python": + self._expand_python_star_imports( + tree.root_node, file_path_str, import_map, + ) # Walk the tree self._extract_from_tree( @@ -1541,6 +1552,10 @@ def _parse_notebook_cells( import_map, defined_names = self._collect_file_scope( tree.root_node, lang, concat_bytes, ) + if lang == "python": + self._expand_python_star_imports( + tree.root_node, file_path_str, import_map, + ) self._extract_from_tree( tree.root_node, concat_bytes, lang, file_path_str, all_nodes, all_edges, @@ -5816,6 +5831,202 @@ def _collect_file_scope( return import_map, defined_names + def _expand_python_star_imports( + self, + root, + file_path: str, + import_map: dict[str, str], + resolving: frozenset[str] = frozenset(), + ) -> None: + """Add public names from repository-local Python star imports.""" + for child in root.children: + if child.type != "import_from_statement": + continue + if not any(part.type == "wildcard_import" for part in child.children): + continue + module_node = child.child_by_field_name("module_name") + if module_node is None: + continue + module = module_node.text.decode("utf-8", errors="replace") + resolved = self._resolve_python_module_in_repo(module, file_path) + if resolved is None or resolved in resolving: + continue + for name, origin in self._get_python_star_exports( + resolved, resolving, + ).items(): + import_map.setdefault(name, origin) + + def _get_python_star_exports( + self, + module_file: str, + resolving: frozenset[str] = frozenset(), + ) -> dict[str, str]: + """Return exported names mapped to their repository-local origin files.""" + try: + module_path = Path(module_file).resolve() + file_stat = module_path.stat() + except (OSError, ValueError): + return {} + resolved_module = str(module_path) + if resolved_module in resolving: + return {} + + cache_key = (resolved_module, file_stat.st_mtime_ns, file_stat.st_size) + with _PYTHON_STAR_EXPORT_CACHE_LOCK: + cached = _PYTHON_STAR_EXPORT_CACHE.get(cache_key) + if cached is not None: + return dict(cached) + + exports = self._read_python_star_exports( + module_path, resolving, + ) + stale_keys = [ + key for key in _PYTHON_STAR_EXPORT_CACHE + if key[0] == resolved_module and key != cache_key + ] + for stale_key in stale_keys: + _PYTHON_STAR_EXPORT_CACHE.pop(stale_key, None) + if len(_PYTHON_STAR_EXPORT_CACHE) >= _PYTHON_STAR_CACHE_MAX: + oldest_keys = list(_PYTHON_STAR_EXPORT_CACHE)[: _PYTHON_STAR_CACHE_MAX // 2] + for oldest_key in oldest_keys: + _PYTHON_STAR_EXPORT_CACHE.pop(oldest_key, None) + _PYTHON_STAR_EXPORT_CACHE[cache_key] = dict(exports) + return exports + + def _read_python_star_exports( + self, + module_path: Path, + resolving: frozenset[str], + ) -> dict[str, str]: + """Read and parse one Python module for star-export discovery.""" + resolved_module = str(module_path) + try: + source = module_path.read_bytes() + except (OSError, PermissionError): + return {} + try: + parser = self._get_parser("python") + if not parser: + return {} + tree = parser.parse(source) # type: ignore[union-attr] + import_map, defined_names = self._collect_file_scope( + tree.root_node, "python", source, + ) + + origins: dict[str, str] = {} + next_resolving = resolving | {resolved_module} + self._expand_python_star_imports( + tree.root_node, resolved_module, origins, next_resolving, + ) + for name, module in import_map.items(): + origin = self._resolve_python_module_in_repo(module, resolved_module) + if origin is not None: + origins[name] = origin + for name in defined_names: + origins[name] = resolved_module + + explicit_exports = self._extract_python_dunder_all(tree.root_node) + if explicit_exports is not None: + return { + name: origins.get(name, resolved_module) + for name in explicit_exports + } + return { + name: origin + for name, origin in origins.items() + if not name.startswith("_") + } + except Exception as exc: + logger.debug( + "Skipping Python star exports for %s: %s", + module_path, + exc, + ) + return {} + + @staticmethod + def _extract_python_dunder_all(root) -> Optional[set[str]]: + """Return literal string names from ``__all__``, or None if absent.""" + for child in root.children: + if child.type != "assignment": + continue + left = child.child_by_field_name("left") + if left is None or left.type != "identifier" or left.text != b"__all__": + continue + right = child.child_by_field_name("right") + if right is None: + return set() + try: + value = ast.literal_eval(right.text.decode("utf-8", errors="replace")) + except (SyntaxError, ValueError): + return set() + if not isinstance(value, (list, tuple)): + return set() + return {name for name in value if isinstance(name, str)} + return None + + def _python_repo_boundary(self, file_path: str) -> Path: + """Return the filesystem boundary for safe Python import lookup.""" + caller_dir = Path(file_path).resolve().parent + if self._repo_root is not None: + return self._repo_root + for candidate in (caller_dir, *caller_dir.parents): + if (candidate / ".git").exists() or (candidate / ".svn").exists(): + return candidate + return caller_dir + + @staticmethod + def _path_is_within(path: Path, boundary: Path) -> bool: + """Return whether *path* is *boundary* or one of its descendants.""" + try: + path.relative_to(boundary) + except ValueError: + return False + return True + + def _resolve_python_module_in_repo( + self, module: str, file_path: str, + ) -> Optional[str]: + """Resolve a Python module without traversing above the repository.""" + caller_dir = Path(file_path).resolve().parent + boundary = self._python_repo_boundary(file_path) + leading_dots = len(module) - len(module.lstrip(".")) + module_name = module[leading_dots:] + relative = Path(*module_name.split(".")) if module_name else Path() + + search_roots: list[Path] = [] + if leading_dots: + base = caller_dir + for _ in range(leading_dots - 1): + if base == boundary: + return None + base = base.parent + search_roots.append(base) + else: + current = caller_dir + while self._path_is_within(current, boundary): + search_roots.append(current) + if current == boundary: + break + current = current.parent + + for root in search_roots: + base = root / relative + candidates = ( + base.with_suffix(".py") if module_name else base / "__init__.py", + base / "__init__.py", + ) + for candidate in candidates: + try: + resolved = candidate.resolve() + except (OSError, ValueError): + continue + if not self._path_is_within(resolved, boundary): + continue + if resolved.is_file(): + return str(resolved) + return None + def _collect_js_exported_local_names( self, node, defined_names: set[str], ) -> None: @@ -6138,7 +6349,18 @@ def _resolve_imported_symbol( language: str, ) -> Optional[str]: """Resolve an imported symbol to its defining qualified name when possible.""" - resolved = self._resolve_module_to_file(module, file_path, language) + module_path = Path(module) + if language == "python" and module_path.is_absolute(): + try: + candidate = module_path.resolve() + except (OSError, ValueError): + return None + boundary = self._python_repo_boundary(file_path) + if not self._path_is_within(candidate, boundary) or not candidate.is_file(): + return None + resolved = str(candidate) + else: + resolved = self._resolve_module_to_file(module, file_path, language) if not resolved: return None diff --git a/tests/test_python_star_imports.py b/tests/test_python_star_imports.py new file mode 100644 index 00000000..dea0681f --- /dev/null +++ b/tests/test_python_star_imports.py @@ -0,0 +1,293 @@ +"""Regression tests for repository-bounded Python wildcard imports.""" + +import json +from pathlib import Path + +from code_review_graph.parser import CodeParser + + +def _call_targets(repo_root: Path, source_file: Path) -> set[str]: + _, edges = CodeParser(repo_root).parse_file(source_file) + return {edge.target for edge in edges if edge.kind == "CALLS"} + + +def test_star_import_resolves_public_function_from_direct_module(tmp_path: Path) -> None: + helper = tmp_path / "helpers.py" + helper.write_text( + "def public_helper():\n" + " return 1\n\n" + "def _private_helper():\n" + " return 2\n", + encoding="utf-8", + ) + caller = tmp_path / "caller.py" + caller.write_text( + "from helpers import *\n\n" + "def run():\n" + " public_helper()\n" + " _private_helper()\n", + encoding="utf-8", + ) + + targets = _call_targets(tmp_path, caller) + + assert f"{helper}::public_helper" in targets + assert "_private_helper" in targets + + +def test_star_import_honors_explicit_dunder_all(tmp_path: Path) -> None: + helper = tmp_path / "helpers.py" + helper.write_text( + "__all__ = ['_selected_helper']\n\n" + "def public_helper():\n" + " return 1\n\n" + "def _selected_helper():\n" + " return 2\n", + encoding="utf-8", + ) + caller = tmp_path / "caller.py" + caller.write_text( + "from helpers import *\n\n" + "def run():\n" + " public_helper()\n" + " _selected_helper()\n", + encoding="utf-8", + ) + + targets = _call_targets(tmp_path, caller) + + assert "public_helper" in targets + assert f"{helper}::_selected_helper" in targets + + +def test_star_import_resolves_transitive_relative_export(tmp_path: Path) -> None: + package = tmp_path / "package" + package.mkdir() + (package / "__init__.py").write_text("", encoding="utf-8") + leaf = package / "leaf.py" + leaf.write_text( + "def transitive_helper():\n" + " return 1\n", + encoding="utf-8", + ) + (package / "bridge.py").write_text( + "from .leaf import *\n", + encoding="utf-8", + ) + caller = tmp_path / "caller.py" + caller.write_text( + "from package.bridge import *\n\n" + "def run():\n" + " transitive_helper()\n", + encoding="utf-8", + ) + + targets = _call_targets(tmp_path, caller) + + assert f"{leaf}::transitive_helper" in targets + + +def test_star_import_does_not_follow_symlink_outside_repository( + tmp_path: Path, + monkeypatch, +) -> None: + repo = tmp_path / "repo" + repo.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + outside_helper = outside / "helpers.py" + outside_helper.write_text( + "def external_helper():\n" + " return 1\n", + encoding="utf-8", + ) + outside_caller = outside / "caller.py" + outside_caller.write_text( + "from helpers import *\n\n" + "def run():\n" + " external_helper()\n", + encoding="utf-8", + ) + linked_caller = repo / "caller.py" + linked_caller.symlink_to(outside_caller) + + outside_reads = 0 + original_read_bytes = Path.read_bytes + + def count_outside_reads(path: Path) -> bytes: + nonlocal outside_reads + if path == outside_helper: + outside_reads += 1 + return original_read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", count_outside_reads) + + targets = _call_targets(repo, linked_caller) + + assert outside_reads == 0 + assert "external_helper" in targets + + +def test_parse_worker_reuses_star_export_cache( + tmp_path: Path, + monkeypatch, +) -> None: + from code_review_graph.incremental import _parse_single_file + + helper = tmp_path / "helpers.py" + helper.write_text( + "def cached_helper():\n" + " return 1\n", + encoding="utf-8", + ) + for filename in ("caller_a.py", "caller_b.py"): + (tmp_path / filename).write_text( + "from helpers import *\n\n" + "def run():\n" + " cached_helper()\n", + encoding="utf-8", + ) + + helper_reads = 0 + original_read_bytes = Path.read_bytes + + def counting_read_bytes(path: Path) -> bytes: + nonlocal helper_reads + if path == helper: + helper_reads += 1 + return original_read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", counting_read_bytes) + + _parse_single_file(("caller_a.py", str(tmp_path))) + _parse_single_file(("caller_b.py", str(tmp_path))) + + assert helper_reads == 1 + + +def test_concurrent_parsers_compute_star_exports_once( + tmp_path: Path, + monkeypatch, +) -> None: + import threading + import time + from concurrent.futures import ThreadPoolExecutor + + helper = tmp_path / "helpers.py" + helper.write_text( + "def concurrent_helper():\n" + " return 1\n", + encoding="utf-8", + ) + callers = [] + for index in range(8): + caller = tmp_path / f"caller_{index}.py" + caller.write_text( + "from helpers import *\n\n" + "def run():\n" + " concurrent_helper()\n", + encoding="utf-8", + ) + callers.append(caller) + + helper_reads = 0 + count_lock = threading.Lock() + original_read_bytes = Path.read_bytes + + def slow_counting_read_bytes(path: Path) -> bytes: + nonlocal helper_reads + if path == helper: + with count_lock: + helper_reads += 1 + time.sleep(0.02) + return original_read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", slow_counting_read_bytes) + + with ThreadPoolExecutor(max_workers=len(callers)) as executor: + targets = list( + executor.map(lambda caller: _call_targets(tmp_path, caller), callers) + ) + + assert helper_reads == 1 + assert all(f"{helper}::concurrent_helper" in result for result in targets) + + +def test_star_export_parse_error_leaves_caller_parseable( + tmp_path: Path, + monkeypatch, +) -> None: + helper = tmp_path / "helpers.py" + helper.write_text( + "def broken_helper():\n" + " return 1\n", + encoding="utf-8", + ) + caller = tmp_path / "caller.py" + caller.write_text( + "from helpers import *\n\n" + "def run():\n" + " broken_helper()\n", + encoding="utf-8", + ) + parser = CodeParser(tmp_path) + original_get_parser = parser._get_parser + get_parser_calls = 0 + + class BrokenParser: + def parse(self, _source: bytes): + raise RuntimeError("broken imported-module parser") + + def fail_on_imported_module(language: str): + nonlocal get_parser_calls + get_parser_calls += 1 + if get_parser_calls == 2: + return BrokenParser() + return original_get_parser(language) + + monkeypatch.setattr(parser, "_get_parser", fail_on_imported_module) + + nodes, edges = parser.parse_file(caller) + + assert any(node.kind == "File" for node in nodes) + assert "broken_helper" in { + edge.target for edge in edges if edge.kind == "CALLS" + } + + +def test_notebook_star_import_resolves_repository_module(tmp_path: Path) -> None: + helper = tmp_path / "helpers.py" + helper.write_text( + "def notebook_helper():\n" + " return 1\n", + encoding="utf-8", + ) + notebook = tmp_path / "analysis.ipynb" + notebook.write_text( + json.dumps({ + "cells": [{ + "cell_type": "code", + "execution_count": None, + "metadata": {}, + "outputs": [], + "source": [ + "from helpers import *\n", + "notebook_helper()\n", + ], + }], + "metadata": { + "kernelspec": { + "display_name": "Python 3", + "language": "python", + "name": "python3", + }, + }, + "nbformat": 4, + "nbformat_minor": 5, + }), + encoding="utf-8", + ) + + targets = _call_targets(tmp_path, notebook) + + assert f"{helper}::notebook_helper" in targets