diff --git a/codebase_rag/cli.py b/codebase_rag/cli.py index dc1b0fa206..92342e47dd 100644 --- a/codebase_rag/cli.py +++ b/codebase_rag/cli.py @@ -2317,6 +2317,75 @@ def rename_command( raise typer.Exit(code=1) +@app.command( + name=ch.CLICommandName.MOVE, + help=ch.CMD_MOVE, + short_help=ch.CMD_MOVE, + epilog=ch.EXAMPLES_MOVE, + rich_help_panel=ch.PANEL_USE, +) +def move_command( + qualified_name: str = typer.Argument(..., help=ch.HELP_MOVE_QN), + target_module: str = typer.Argument(..., help=ch.HELP_MOVE_TARGET), + repo_path: Path = typer.Option( + Path(cs.MCP_DEFAULT_DIRECTORY), + "--repo-path", + exists=True, + file_okay=False, + help=ch.HELP_GRAPH_REPO_PATH, + ), + project: str | None = typer.Option(None, "--project", help=ch.HELP_GRAPH_PROJECT), + keep_alias: bool = typer.Option( + False, "--keep-alias", help=ch.HELP_MOVE_KEEP_ALIAS + ), + dry_run: bool = typer.Option(False, "--dry-run", help=ch.HELP_MOVE_DRY_RUN), +) -> None: + from .editing.move import MoveRefused, move + from .graph_cli import _project_and_fetch + from .graph_query import source_root_for + + name, fetch_all, ingestor = _project_and_fetch(project, repo_path) + with ingestor: + # An explicit --project may name a graph indexed from another + # checkout; its repo-relative paths must not be edited under this one. + if source_root_for(fetch_all, name, repo_path) is None: + typer.echo( + cs.MOVE_CLI_WRONG_ROOT.format(project=name, root=repo_path.resolve()), + err=True, + ) + raise typer.Exit(code=1) + parsers, queries = load_parsers() + from .graph_updater import GraphUpdater + + updater = GraphUpdater( + ingestor=ingestor, + repo_path=repo_path.resolve(), + parsers=parsers, + queries=queries, + project_name=name, + ) + try: + report = move( + repo_path.resolve(), + fetch_all, + name, + qualified_name, + target_module, + keep_alias=keep_alias, + dry_run=dry_run, + reingest=updater.reingest, + ) + except MoveRefused as refused: + typer.echo(str(refused), err=True) + raise typer.Exit(code=1) from refused + payload = dict(report._asdict()) + if report.verdict is not None: + payload[cs.KEY_VERDICT] = report.verdict._asdict() + typer.echo(json.dumps(payload, indent=cs.MCP_JSON_INDENT, sort_keys=True)) + if not report.applied and not dry_run: + raise typer.Exit(code=1) + + @app.command( name=ch.CLICommandName.CHANGE_SIGNATURE, help=ch.CMD_CHANGE_SIGNATURE, diff --git a/codebase_rag/cli_help.py b/codebase_rag/cli_help.py index 04d637a69c..53f77f5231 100644 --- a/codebase_rag/cli_help.py +++ b/codebase_rag/cli_help.py @@ -23,6 +23,7 @@ class CLICommandName(StrEnum): CHECK = "check" RENAME = "rename" CHANGE_SIGNATURE = "change-signature" + MOVE = "move" CONTEXT = "context" PORT = "port" STOP = "stop" @@ -223,6 +224,22 @@ class CLICommandName(StrEnum): "a trace as well, instead of listing them as unmapped." ) HELP_CHANGE_SIGNATURE_DRY_RUN = "Print the plan and diff without writing anything." +CMD_MOVE = ( + "Move a module-level definition to another module: importers rewritten, " + "needed imports carried, refused when it would create an import cycle." +) +EXAMPLES_MOVE = ( + "Examples:\n cgr move myproj.pkg.util.helper pkg.core --dry-run\n" + " cgr move myproj.pkg.util.helper pkg/core.py --keep-alias" +) +HELP_MOVE_QN = "Qualified name of the module-level definition to move." +HELP_MOVE_DRY_RUN = ( + "Print the plan (files, importers, copied imports) without writing anything." +) +HELP_MOVE_TARGET = "Destination module: dotted name (pkg.core) or repo-relative path." +HELP_MOVE_KEEP_ALIAS = ( + "Leave a re-export at the old location so the old import path still works." +) CMD_CONTEXT = ( "Print a graph-ranked context slice for a symbol, location or task within " "a token budget: source, caller lines, callee signatures, types, tests, docs." @@ -742,6 +759,7 @@ class CLICommandName(StrEnum): CLICommandName.CHECK: CMD_CHECK, CLICommandName.RENAME: CMD_RENAME, CLICommandName.CHANGE_SIGNATURE: CMD_CHANGE_SIGNATURE, + CLICommandName.MOVE: CMD_MOVE, CLICommandName.CONTEXT: CMD_CONTEXT, CLICommandName.PORT: CMD_PORT, CLICommandName.WORKSPACE: CMD_WORKSPACE, diff --git a/codebase_rag/constants/ast_nodes.py b/codebase_rag/constants/ast_nodes.py index fb2f45d1e1..bc5cc7ebf1 100644 --- a/codebase_rag/constants/ast_nodes.py +++ b/codebase_rag/constants/ast_nodes.py @@ -111,6 +111,7 @@ FIELD_PROPERTY = "property" FIELD_NAME = "name" FIELD_ALIAS = "alias" +FIELD_SOURCE = "source" FIELD_MODULE_NAME = "module_name" FIELD_ARGUMENTS = "arguments" FIELD_BODY = "body" diff --git a/codebase_rag/constants/cli.py b/codebase_rag/constants/cli.py index 76fd0977de..326f954ea2 100644 --- a/codebase_rag/constants/cli.py +++ b/codebase_rag/constants/cli.py @@ -1112,6 +1112,72 @@ class DiffMarker: CONTEXT_WHY_TEST = "test reaching it at depth {depth} through {through}" CONTEXT_WHY_DOC = "documentation section whose file links to it" CONTEXT_UNRESOLVED = "Nothing in the graph matches {target}" +# move (issue #1534). +MOVE_UNKNOWN = "No definition named {qn} in the graph" +MOVE_METHOD = "{qn} is a method; move its class instead" +MOVE_SAME_MODULE = "{path} already holds the definition" +MOVE_NO_GRAMMAR = "No grammar for {path}; the definition cannot be moved" +MOVE_NO_DEFINITION_TOKEN = "Could not locate the definition of {qn} in {path}" +MOVE_CYCLE = "Refusing to move: it would create the import cycle {cycle}" +MOVE_IMPORT_COLLISION = ( + "{path} already binds {name}; the import the moved definition needs would rebind it" +) +MOVE_MAYBE_UNBOUND = ( + "{names} may be unbound in {path}: it is set under a module-level if, " + "try or loop that does not bind it on every path, so the moved " + "definition cannot import it safely" +) +MOVE_SPLIT_GLOBAL = ( + "{names} is a variable of {path} rebound through a global statement; " + "the destination would import it as a separate copy, so the moved " + "definition and {path} would stop seeing each other's updates" +) +MOVE_ASSIGNS_IMPORT = ( + "{names} is a variable of {path} that the moved definition assigns; " + "imported into the destination it is a read-only binding, so the " + "assignment would throw" +) +MOVE_WILDCARD_IMPORTER = ( + "{importers} import(s) everything from {path} with *, and would no " + "longer get {name} from it; pass keep_alias to keep {path} exporting it" +) +MOVE_STAR_PROVIDED = ( + "{names} reaches {path} only through a star import from {modules}; " + "the destination would not bind it, so the moved definition would " + "raise NameError; import it by name in {path} first" +) +MOVE_FUTURE_ANNOTATIONS = ( + "{enabled} enables from __future__ import annotations and {disabled} " + "does not; the move would change how the annotations in {changed} " + "evaluate" +) +MOVE_RELATIVE_IMPORT_ESCAPES = ( + "{statement} in {qn} climbs above the top-level package of {path}, so " + "the move cannot respell it for the destination" +) +MOVE_PLANNED = "{importers} importer(s) would be rewritten, {unchanged} left unchanged" +MOVE_PARSE_FAILED = "Move rolled back: {files} would no longer parse" +MOVE_SOURCE_CHANGED = ( + "Cannot stage the move: {files} changed on disk after the move was " + "planned; retry once the tree is settled" +) +MOVE_CONTRACT_FAILED = "Move rolled back, postcondition failed: {reasons}" +MOVE_CONTRACT_UNMEASURED = ( + "Move applied, but its postcondition could not be measured: {error}" +) +MOVE_ROLLBACK_UNMEASURED = ( + "Move rolled back after its postcondition failed ({reasons}), but the " + "graph could not be re-ingested afterwards ({error}); rebuild the graph " + "before the next graph-backed operation" +) +MOVE_WRONG_ROOT = ( + "Project {project} was not indexed from this server's repository; " + "move from the MCP server rooted at its source tree" +) +MOVE_CLI_WRONG_ROOT = ( + "Project {project} was not indexed from {root}; " + "pass the --repo-path it was indexed from" +) # Postcondition contract (issue #1531). CONTRACT_OP_RENAME = "rename" CONTRACT_OP_CHANGE_SIGNATURE = "change_signature" diff --git a/codebase_rag/constants/graph.py b/codebase_rag/constants/graph.py index 696aa5efbc..7c53288d8f 100644 --- a/codebase_rag/constants/graph.py +++ b/codebase_rag/constants/graph.py @@ -123,6 +123,7 @@ class (and, uncertain, other calls in files naming the class or KEY_UNMAPPED = "unmapped" KEY_VERDICT = "verdict" KEY_STRUCTURAL_DELTA = "structural_delta" +KEY_CYCLE = "cycle" KEY_DISPATCH_LITERAL = "dispatch_literal" diff --git a/codebase_rag/constants/mcp.py b/codebase_rag/constants/mcp.py index 233007a62b..9c53e177c1 100644 --- a/codebase_rag/constants/mcp.py +++ b/codebase_rag/constants/mcp.py @@ -28,6 +28,7 @@ class MCPToolName(StrEnum): # Graph-driven edit operations (issues #1532, #1533). RENAME = "rename" CHANGE_SIGNATURE = "change_signature" + MOVE = "move" CONTEXT = "context" QUERY_CODE_GRAPH = "query_code_graph" GET_CODE_SNIPPET = "get_code_snippet" @@ -126,6 +127,8 @@ class MCPParamName(StrEnum): ALLOW_HEURISTIC = "allow_heuristic" NEW_PARAMS = "new_params" MAPPING = "mapping" + TARGET_MODULE = "target_module" + KEEP_ALIAS = "keep_alias" BUDGET_TOKENS = "budget_tokens" diff --git a/codebase_rag/cypher_queries.py b/codebase_rag/cypher_queries.py index 4560b0d1bd..7f27dbf559 100644 --- a/codebase_rag/cypher_queries.py +++ b/codebase_rag/cypher_queries.py @@ -1633,7 +1633,7 @@ def build_missing_rel_endpoints_query( RETURN labels(n)[0] AS label, n.qualified_name AS qualified_name, n.name AS name, n.path AS path, n.start_line AS start_line, n.end_line AS end_line, n.decorators AS decorators""" -CYPHER_DELTA_MODULE_IMPORTS = """MATCH (m:Module)-[:IMPORTS]->(t:Module) +CYPHER_DELTA_MODULE_IMPORTS = """MATCH (m:Module)-[r:IMPORTS]->(t:Module) WHERE m.qualified_name STARTS WITH $project_prefix AND t.qualified_name STARTS WITH $project_prefix AND ALL(longer_project IN $longer_project_prefixes @@ -1643,7 +1643,7 @@ def build_missing_rel_endpoints_query( WHERE t.qualified_name <> longer_project AND NOT t.qualified_name STARTS WITH (longer_project + '.')) RETURN DISTINCT m.qualified_name AS from_qn, m.path AS from_path, - t.qualified_name AS to_qn""" + t.qualified_name AS to_qn, r.imported_name AS imported_name""" # The import statements that bind a symbol BY NAME into or out of the # touched files (issue #2516), one row per bound name: an importer of a # touched module still naming a symbol the edit removed, and a touched @@ -1707,6 +1707,12 @@ def build_missing_rel_endpoints_query( MATCH (doc)-[:CONTAINS_SECTION]->(s:Section) RETURN doc.qualified_name AS from_qn, s.qualified_name AS qualified_name, s.name AS name, s.path AS path, s.start_line AS start_line, s.end_line AS end_line""" +# What one module imports, with each statement's site (issue #1534). +CYPHER_GRAPH_IMPORTS_OF = """MATCH (m:Module)-[r:IMPORTS]->(target) +WHERE m.qualified_name = $qn AND m.qualified_name STARTS WITH $project_prefix +RETURN target.qualified_name AS to_qn, r.line AS line, r.col AS col, + r.end_line AS end_line, r.end_col AS end_col, r.alias AS alias, + r.imported_name AS imported_name""" CYPHER_GRAPH_IMPORTERS = """MATCH (m:Module)-[r:IMPORTS]->(target) WHERE target.qualified_name = $qn AND m.qualified_name STARTS WITH $project_prefix RETURN m.qualified_name AS qualified_name, m.path AS path, r.line AS line, diff --git a/codebase_rag/editing/contract.py b/codebase_rag/editing/contract.py index dcb5f66471..9c95ef83c2 100644 --- a/codebase_rag/editing/contract.py +++ b/codebase_rag/editing/contract.py @@ -382,6 +382,7 @@ def measure( files: Iterable[str], reingest: Reingest, declared_renames: Iterable[tuple[str, str]] = (), + reshaped_renames: Iterable[tuple[str, str]] = (), ) -> StructuralDelta: """The delta of files an operation just wrote, through the re-ingest. @@ -389,6 +390,8 @@ def measure( renames from the snapshots, which cannot be done for an empty container (no fingerprint, no members, and the name is what changed), so an operation that knows says so rather than leaving it to a guess. + `reshaped_renames` are those whose body the operation also rewrote, so + the snapshots cannot pair them by shape either. """ paths = sorted(set(files)) return observe( @@ -398,4 +401,5 @@ def measure( lambda: reingest(paths), repo_root=repo_root, declared_renames=frozenset(declared_renames), + reshaped_renames=frozenset(reshaped_renames), ) diff --git a/codebase_rag/editing/move.py b/codebase_rag/editing/move.py new file mode 100644 index 0000000000..e03b51a0f2 --- /dev/null +++ b/codebase_rag/editing/move.py @@ -0,0 +1,2493 @@ +"""`move(qn, target_module)`: edit-algebra operation 3 (issue #1534). + +Moving a symbol out of a shared dumping ground into the one package that +uses it is the refactor that shrinks affected sets, and by hand it is +tedious: the definition, its own imports, every importer, re-exports. The +graph knows the definition's span, the imports its module binds, and every +importer's statement (issue #1522), so the whole move is one transaction: + +- cut the definition (decorators, docstring, adjacent comments included) + and paste it into the target with the imports it needs; +- rewrite every importer through the import rewriter; +- give the old module an import of the moved name when it still uses it, + and optionally a deprecation re-export (`keep_alias=True`); +- refuse before touching a file when the move would create an import + cycle, naming the cycle. +""" + +from __future__ import annotations + +import ast +import builtins +import re +import symtable +from collections.abc import Callable +from pathlib import Path +from typing import NamedTuple + +from loguru import logger +from tree_sitter import Node + +from .. import constants as cs +from .. import cypher_queries as cq +from .. import graph_query +from ..graph_query import QueryFn +from ..graph_updater import ReingestAborted +from ..language_spec import get_language_for_extension +from ..parser_loader import load_parsers +from ..structural_delta import _longer_project_prefixes, import_cycles, snapshot +from ..types_defs import ResultRow +from ..utils.path_utils import base_module_qn +from .contract import Reingest, Verdict, measure, move_expectation, verify +from .imports import ( + _JS_KEYWORD, + _JS_NAMED, + _JS_SPEC, + ImportRewriter, + ImportSite, + SymbolMove, + _local_name, + _match_py_from, + _relative_specifier, + _split_names, +) +from .patcher import ( + Patcher, + PatcherError, + SpanEdit, + apply_span_edits, + line_col_to_byte, +) +from .rename import _name_token +from .transaction import ( + EditTransaction, + StagedFile, + StagedTree, + TransactionConflict, + VerificationResult, + load_history, + undo_transaction, + unified_diff, +) + +_IDENTIFIER = r"(?[\w$]+)") +# A TS inline type-only entry (`{ type Foo }`) binds `Foo`; the modifier +# must be set aside before comparing, but `{ type }` alone is a real name. +_TS_INLINE_TYPE = re.compile(r"^type\s+(?=\S)") +# Module-level statements whose bodies may run once, many times or never, +# and the nodes opening a scope of their own: a name bound inside one is +# not a module binding (a definition's own name is). +_PY_FLOW = ( + ast.If, + ast.Try, + ast.TryStar, + ast.With, + ast.AsyncWith, + ast.For, + ast.AsyncFor, + ast.While, + ast.Match, +) +_PY_SCOPES = ( + ast.Lambda, + ast.ListComp, + ast.SetComp, + ast.DictComp, + ast.GeneratorExp, +) +_PY_STAR = "*" +_PY_ALL = "__all__" +_PY_PRIVATE = "_" +_PY_DUNDER = "__" +# Names every module can read without binding them: what a star import +# cannot be the source of. +_PY_BUILTINS = frozenset(dir(builtins)) +# The one future feature still optional in Python 3, so the one whose +# presence changes how a module runs: annotations stay unevaluated strings. +_FUTURE_ANNOTATIONS = "annotations" +# Builtins that compile code at runtime, inheriting the caller's future +# features: an annotation in a string they run is postponed or not with it. +_PY_DYNAMIC_CODE = frozenset({"exec", "eval", "compile"}) +# The receivers through which an attribute reaches those builtins: any other +# `.compile` (`re.compile`) is someone else's method. +_PY_BUILTINS_MODULES = frozenset({"builtins", "__builtins__"}) +_MODULE_SCOPE = "module" +_SYMTABLE_FILENAME = "" +_SYMTABLE_MODE = "exec" +# `if TYPE_CHECKING:` binds its imports for the type checker only. They are +# carried under the same guard rather than refused as maybe-unbound, since +# annotations are the one use that never needs them at runtime. +_TYPE_CHECKING = "TYPE_CHECKING" +_TYPE_CHECKING_GUARDS = frozenset({_TYPE_CHECKING, f"typing.{_TYPE_CHECKING}"}) +_TYPE_CHECKING_IMPORT = f"from typing import {_TYPE_CHECKING}" +_TYPE_CHECKING_HEAD = f"if {_TYPE_CHECKING}:\n" +_PY_INDENT = " " + + +class MoveRefused(ValueError): + """The move cannot be planned as asked; nothing was written.""" + + def __init__(self, message: str, cycle: tuple[str, ...] = ()) -> None: + super().__init__(message) + self.cycle = cycle + + +class MoveReport(NamedTuple): + qualified_name: str + new_qualified_name: str + old_path: str + new_path: str + applied: bool + transaction_id: str + files: tuple[str, ...] + importers: tuple[str, ...] + unchanged_importers: tuple[str, ...] + copied_imports: tuple[str, ...] + diff: str + message: str + verdict: Verdict | None = None + # The graph may be partial: a re-ingest failed after it began writing. + graph_incomplete: bool = False + + +class _Cut(NamedTuple): + start: int + end: int + text: str + # Where `text` begins in the source; `start` may sit before it. + origin: int + + +class _NeededImport(NamedTuple): + statement: str + target_qn: str + # The one name the statement binds at the destination. + local: str + + +def _text(node: Node | None) -> str: + if node is None or node.text is None: + return "" + return node.text.decode(cs.ENCODING_UTF8, errors="replace") + + +def _module_bound(text: str, module: str) -> bool: + """Whether `text` binds `module`'s root name via a plain `import`. + + The move rewrites call sites to `pkg.new.helper(...)`, which needs + `pkg` bound. Only a plain `import pkg.new` does that -- or a deeper + `import pkg.new.sub`, which binds `pkg` just the same (verified + against a real interpreter; it must keep answering True). + + Deciding this by searching the raw text was wrong in the direction + that breaks code. `import pkg.new as n` binds only `n`; a comment or + a string containing the words binds nothing. Each made the caller + skip adding the real import AFTER the call sites had been rewritten, + and both versions parse, so the postcondition could not catch it + either -- the program died at runtime with + `NameError: name 'pkg' is not defined` on the line the move wrote. + + Unparseable input answers False: the caller then adds an import it + may not need, which is recoverable, rather than omitting one it does. + + Only the module's own statements count. An import inside a function + or class binds that scope's name, not the module's, so `ast.walk` + let a nested `import pkg.new` suppress the top-level one the + rewritten module-level call sites need. An import under a top-level + `if`/`try` is skipped too: it may not run, and the cost of that + choice is at worst a redundant import. + """ + try: + tree = ast.parse(text) + except SyntaxError: + return False + wanted = module.split(".") + for node in tree.body: + if not isinstance(node, ast.Import): + continue + for alias in node.names: + # `import x as y` binds only `y`, never x's root. + if alias.asname is None and alias.name.split(".")[: len(wanted)] == wanted: + return True + return False + + +def _uses(text: str, name: str) -> bool: + return re.search(_IDENTIFIER % re.escape(name), text) is not None + + +def _py_bound(node: ast.AST) -> set[str]: + """Every module-scope name `node` can bind, on whichever path it runs.""" + names: set[str] = set() + stack = [node] + while stack: + current = stack.pop() + if isinstance(current, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef): + names.add(current.name) + continue + if isinstance(current, _PY_SCOPES) or ( + # `x: int` declares `x` without binding it. + isinstance(current, ast.AnnAssign) and current.value is None + ): + continue + names |= _py_own_bound(current) + stack.extend(ast.iter_child_nodes(current)) + return names + + +def _py_own_bound(node: ast.AST) -> set[str]: + """The names `node` itself binds, leaving its children to the caller.""" + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): + return {node.id} + if isinstance(node, ast.Import | ast.ImportFrom): + return { + alias.asname or alias.name.split(cs.SEPARATOR_DOT)[0] + for alias in node.names + if alias.name != _PY_STAR + } + if isinstance(node, ast.ExceptHandler | ast.MatchAs | ast.MatchStar) and node.name: + return {node.name} + if isinstance(node, ast.MatchMapping) and node.rest: + return {node.rest} + return set() + + +def _meet(left: set[str] | None, right: set[str] | None) -> set[str] | None: + # None is a path that never completes, which binds everything vacuously. + if left is None: + return right + if right is None: + return left + return left & right + + +def _py_definite(body: list[ast.stmt]) -> set[str] | None: + """Names certainly bound once `body` completes; None when it never does. + + Conservative where it cannot tell: a loop may run zero times and a + `match` may match nothing, so neither binds anything for certain. + """ + names: set[str] = set() + for statement in body: + if isinstance(statement, ast.Raise): + return None + # Nested blocks are walked here, not in the helpers, so a nesting + # level costs one frame: an `elif` chain nests a level per branch. + if isinstance(statement, ast.If): + bound = _meet(_py_definite(statement.body), _py_definite(statement.orelse)) + elif isinstance(statement, ast.Try | ast.TryStar): + bound = _try_definite( + _py_definite(statement.body + statement.orelse), + [_py_definite(handler.body) for handler in statement.handlers], + _py_definite(statement.finalbody), + ) + elif isinstance(statement, ast.With | ast.AsyncWith): + bound = _with_definite(_py_definite(statement.body), statement.items) + elif isinstance(statement, _PY_FLOW): + bound = set() + else: + bound = _py_bound(statement) + if bound is None: + return None + names |= bound + return names + + +def _try_definite( + body: set[str] | None, handlers: list[set[str] | None], final: set[str] | None +) -> set[str] | None: + """What a `try` certainly binds, from what its body (with its `else`), + each of its handlers and its `finally` certainly bind.""" + for handled in handlers: + body = _meet(body, handled) + return None if body is None or final is None else body | final + + +def _with_definite(body: set[str] | None, items: list[ast.withitem]) -> set[str] | None: + """What a `with` certainly binds, from what its body certainly binds: + that, and each `as` target once the body completes.""" + if body is None: + return None + for item in items: + if item.optional_vars is not None: + body |= _py_bound(item.optional_vars) + return body + + +def _flow_bindings(source: bytes) -> tuple[frozenset[str], frozenset[str]]: + """(carried, unsafe): module names bound under top-level control flow. + + `carried` are bound on every path that loads the module, and only + there, so the destination can import them back like any constant; + `unsafe` may be left unbound, and importing one by name would fail + on load wherever the old module skipped it, where before only a call + reaching it failed. Names a top-level statement binds are neither: + `_bindings` and `_needed_imports` already account for them. + """ + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + return frozenset(), frozenset() + simple: set[str] = set() + anywhere: set[str] = set() + for statement in tree.body: + bound = _py_bound(statement) + anywhere |= bound + if not isinstance(statement, _PY_FLOW): + simple |= bound + definite = _py_definite(tree.body) or set() + return frozenset(definite - simple), frozenset(anywhere - definite) + + +def _global_references(text: str) -> set[str] | None: + """Module-scope names the Python `text` reads; None when it won't parse. + + Scoped, unlike `_uses`: a refusal on a name the moved code only binds + locally (a loop variable sharing a module-level loop's name) would + refuse a move that is safe. + """ + try: + table = symtable.symtable(text, _SYMTABLE_FILENAME, _SYMTABLE_MODE) + except SyntaxError: + return None + names: set[str] = set() + tables = [table] + while tables: + current = tables.pop() + tables.extend(current.get_children()) + at_module = current.get_type() == _MODULE_SCOPE + names.update( + symbol.get_name() + for symbol in current.get_symbols() + if symbol.is_referenced() and (at_module or symbol.is_global()) + ) + return names + + +def _global_writes(text: str) -> set[str]: + """Module names the Python `text` rebinds from inside a function or a + class through a `global` statement.""" + try: + table = symtable.symtable(text, _SYMTABLE_FILENAME, _SYMTABLE_MODE) + except SyntaxError: + return set() + names: set[str] = set() + tables = list(table.get_children()) + while tables: + current = tables.pop() + tables.extend(current.get_children()) + names.update( + symbol.get_name() + for symbol in current.get_symbols() + if symbol.is_declared_global() and symbol.is_assigned() + ) + return names + + +def _module_scope_names(text: str) -> set[str]: + """Every module-scope name the Python `text` binds, imports or reads, + at the top level or from inside a function.""" + try: + table = symtable.symtable(text, _SYMTABLE_FILENAME, _SYMTABLE_MODE) + except SyntaxError: + return set() + # A name another scope declares `global` is listed at module scope + # with no flags of its own; only real uses count. + names = { + symbol.get_name() + for symbol in table.get_symbols() + if symbol.is_assigned() or symbol.is_imported() or symbol.is_referenced() + } + tables = list(table.get_children()) + while tables: + current = tables.pop() + tables.extend(current.get_children()) + names.update( + symbol.get_name() + for symbol in current.get_symbols() + if symbol.is_global() and (symbol.is_referenced() or symbol.is_assigned()) + ) + return names + + +def _split_globals(moved: str, remainder: str, name: str) -> list[str]: + """Module variables the move would split in two. + + The destination imports an old-module name by value, so a `global` + rebinding on one side of the move updates that side's copy only: the + moved code counting in its own module while the old one, and every + reader of it, kept the old value -- or the other way round. + """ + reads = _global_references(moved) or set() + staying = _module_scope_names(remainder) + split = {n for n in _global_writes(moved) if n in staying} + split |= {n for n in _global_writes(remainder) if n in reads} + return sorted(split - {name}) + + +def _star_exports(source: bytes, name: str) -> bool: + """Whether `from module import *` of the Python `source` binds `name`. + + Without `__all__` it binds every public name; with one, the names it + lists. An `__all__` built any other way than by one literal assignment + is read as listing everything, since the move cannot tell what it holds. + """ + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + return True + text = source.decode(cs.ENCODING_UTF8, errors="replace") + if _PY_ALL not in _module_scope_names(text): + return not name.startswith(_PY_PRIVATE) + assigned = [ + node.value + for node in tree.body + if isinstance(node, ast.Assign) + and any(isinstance(t, ast.Name) and t.id == _PY_ALL for t in node.targets) + ] + if len(assigned) != 1: + return True + try: + listed = ast.literal_eval(assigned[0]) + except ValueError: + return True + return not isinstance(listed, list | tuple) or name in listed + + +def _star_sources(tree: ast.Module) -> list[str]: + """The modules a Python module star-imports, as written.""" + return sorted( + { + cs.SEPARATOR_DOT * node.level + (node.module or "") + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) + and any(alias.name == _PY_STAR for alias in node.names) + } + ) + + +def _star_provided(source: bytes, moved: str, name: str) -> tuple[list[str], list[str]]: + """(names, modules): what the moved text reads that only a star import + of the old module can bind, and the modules it star-imports. + + The graph records `from pkg.cfg import *` under `*`, with no name to + match a use against, so `_needed_imports` copied nothing for it and + `RATE` read through it was a NameError at the destination. Every name + the module binds itself, or a builtin, is accounted for; what is left + can only come through the star. + """ + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + return [], [] + modules = _star_sources(tree) + reads = _global_references(moved) + if not modules or reads is None: + return [], [] + bound: set[str] = set() + for statement in tree.body: + bound |= _py_bound(statement) + names = sorted( + n + for n in reads - bound - _PY_BUILTINS + if n != name and not (n.startswith(_PY_DUNDER) and n.endswith(_PY_DUNDER)) + ) + return (names, modules) if names else ([], []) + + +def _builtin_aliases(tree: ast.Module) -> tuple[frozenset[str], frozenset[str]]: + """The names `tree` reaches the dynamic-code builtins through: the + receivers `builtins` is bound to (`import builtins as bi`), and the + names the builtins themselves are (`from builtins import exec as run`).""" + receivers = set(_PY_BUILTINS_MODULES) + names = set(_PY_DYNAMIC_CODE) + for node in ast.walk(tree): + if isinstance(node, ast.Import): + receivers |= { + a.asname or a.name for a in node.names if a.name in _PY_BUILTINS_MODULES + } + elif isinstance(node, ast.ImportFrom) and node.module in _PY_BUILTINS_MODULES: + names |= { + a.asname or a.name for a in node.names if a.name in _PY_DYNAMIC_CODE + } + return frozenset(receivers), frozenset(names) + + +def _has_annotations(source: bytes | str, scope: bytes | str | None = None) -> bool: + """Whether Python `source` annotates anything, or runs code that may; + True when it won't parse, since then the move cannot tell. + + `exec`, `eval` and `compile` inherit the calling module's future + features, so `exec("def g(x: Missing): pass")` relied on postponed + annotations with no annotation of its own in sight (bot review). The + imports that alias them live in `scope`, the module `source` came from + when it is only a definition cut out of one (bot review). + """ + try: + tree = ast.parse(source) + receivers, names = _builtin_aliases(tree if scope is None else ast.parse(scope)) + except (SyntaxError, ValueError): + return True + for node in ast.walk(tree): + if ( + isinstance(node, ast.AnnAssign) + or (isinstance(node, ast.Name) and node.id in names) + or ( + isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and node.value.id in receivers + and node.attr in _PY_DYNAMIC_CODE + ) + ): + return True + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) and ( + node.returns is not None + or any( + a.annotation is not None + for a in ( + *node.args.posonlyargs, + *node.args.args, + *node.args.kwonlyargs, + *filter(None, (node.args.vararg, node.args.kwarg)), + ) + ) + ): + return True + return False + + +def _identifiers(node: Node) -> set[str]: + found: set[str] = set() + stack = [node] + while stack: + current = stack.pop() + if current.type == cs.TS_IDENTIFIER: + found.add(_text(current)) + stack.extend(current.named_children) + return found + + +def _js_parameter_names(parameters: Node) -> set[str]: + """The names a JS/TS parameter list binds, leaving out its defaults.""" + names: set[str] = set() + for parameter in parameters.named_children: + bound: Node | None = parameter + if parameter.type == cs.TS_ASSIGNMENT_PATTERN: + bound = parameter.child_by_field_name(cs.FIELD_LEFT) + elif parameter.type in (cs.TS_REQUIRED_PARAMETER, cs.TS_OPTIONAL_PARAMETER): + bound = parameter.child_by_field_name(cs.TS_FIELD_PATTERN) + if bound is not None: + names |= _identifiers(bound) + return names + + +def _js_assigned(top: Node) -> set[str]: + """Names the JS/TS definition `top` assigns without declaring them. + + Scope is read coarsely: a name declared anywhere inside counts as + local throughout, so a write is reported only when no declaration of + the definition could be what it assigns. + """ + declared: set[str] = set() + written: set[str] = set() + stack = [top] + while stack: + current = stack.pop() + stack.extend(current.named_children) + target: Node | None = None + if current.type in ( + cs.TS_JS_ASSIGNMENT_EXPRESSION, + cs.TS_JS_AUGMENTED_ASSIGNMENT_EXPRESSION, + ): + target = current.child_by_field_name(cs.FIELD_LEFT) + elif current.type == cs.TS_JS_UPDATE_EXPRESSION: + target = current.child_by_field_name(cs.TS_JS_FIELD_ARGUMENT) + elif current.type == cs.TS_VARIABLE_DECLARATOR: + named = current.child_by_field_name(cs.FIELD_NAME) + if named is not None: + declared |= _identifiers(named) + elif current.type == cs.TS_JS_FORMAL_PARAMETERS: + declared |= _js_parameter_names(current) + if target is not None and target.type == cs.TS_IDENTIFIER: + written.add(_text(target)) + return written - declared + + +def _clause_exports(clause: Node) -> list[str]: + """The names a JS/TS `export { a, b as c }` clause exports: `a`, `c`.""" + return [ + _text( + spec.child_by_field_name(cs.FIELD_ALIAS) + or spec.child_by_field_name(cs.FIELD_NAME) + ) + for spec in clause.named_children + if spec.type == cs.TS_EXPORT_SPECIFIER + ] + + +def _type_checking_imports( + source: bytes, old_path: str, new_path: str +) -> dict[str, str]: + """Names an import under a top-level `if TYPE_CHECKING:` binds, each + with that import narrowed to it and spelled for `new_path`. + + Only a guard without an `else` counts: one with an `else` binds at + runtime too, and is control flow like any other `if`. + """ + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + return {} + out: dict[str, str] = {} + for node in tree.body: + for statement in _guarded_body(node): + for local, narrowed in _narrowed_imports(statement, old_path, new_path): + out.setdefault(local, narrowed) + return out + + +def _guarded_body(node: ast.stmt) -> list[ast.stmt]: + """The body of `node` when it is an `if TYPE_CHECKING:` without an + `else`; nothing for any other statement.""" + if ( + isinstance(node, ast.If) + and not node.orelse + and ast.unparse(node.test) in _TYPE_CHECKING_GUARDS + ): + return node.body + return [] + + +def _narrowed_imports( + statement: ast.stmt, old_path: str, new_path: str +) -> list[tuple[str, str]]: + """Each name the import `statement` binds, with the import narrowed to + it and spelled for `new_path`; nothing for any other statement.""" + if isinstance(statement, ast.ImportFrom): + module = _rebase_py_relative( + cs.SEPARATOR_DOT * statement.level + (statement.module or ""), + old_path, + new_path, + ) + return [ + (alias.asname or alias.name, f"from {module} import {ast.unparse(alias)}") + for alias in statement.names + if alias.name != _PY_STAR + ] + if isinstance(statement, ast.Import): + return [ + ( + alias.asname or alias.name.split(cs.SEPARATOR_DOT)[0], + f"import {ast.unparse(alias)}", + ) + for alias in statement.names + ] + return [] + + +def _type_checking_block(root: Node) -> Node | None: + """The body of a module's top-level `if TYPE_CHECKING:`, if it has one.""" + for child in root.children: + if ( + child.type == cs.TS_PY_IF_STATEMENT + and child.child_by_field_name(cs.FIELD_ALTERNATIVE) is None + and _text(child.child_by_field_name(cs.TS_FIELD_CONDITION)) + in _TYPE_CHECKING_GUARDS + ): + return child.child_by_field_name(cs.TS_FIELD_CONSEQUENCE) + return None + + +def _definition_at(root: Node, line: int, col: int) -> Node | None: + """The outermost definition whose own name token starts at (line, col).""" + stack = [root] + while stack: + node = stack.pop() + named = node.child_by_field_name(cs.FIELD_NAME) + if ( + named is not None + and named.start_point == (line - 1, col) + and (node.type not in (cs.TS_IDENTIFIER, cs.TS_PY_IDENTIFIER)) + ): + return node + if node.start_point[0] <= line - 1 <= node.end_point[0]: + stack.extend(reversed(node.children)) + return None + + +def _own_span(source: bytes, first: Node, last: Node) -> tuple[int, int] | None: + """The bytes of `first` through `last` alone, and the spaces setting + them apart on the shared line, when another statement shares their + first or last line; None when they have their lines to themselves. A + trailing comment is not a statement: it goes with the lines. + """ + before, after = first.prev_named_sibling, last.next_named_sibling + shares_last = ( + after is not None + and after.type != cs.TS_COMMENT + and after.start_point[0] == last.end_point[0] + ) + if not shares_last and ( + before is None or before.end_point[0] != first.start_point[0] + ): + return None + start, end = first.start_byte, last.end_byte + if shares_last: + while source[end : end + 1] in _INLINE_SPACE: + end += 1 + else: + while start > 0 and source[start - 1 : start] in _INLINE_SPACE: + start -= 1 + return start, end + + +def _cut_span(source: bytes, node: Node) -> _Cut: + """Whole lines of the definition plus decorators, export and comments.""" + target = node + if target.parent is not None and target.parent.type in _WRAPPERS: + target = target.parent + first = target + sibling = target.prev_named_sibling + while ( + sibling is not None + and sibling.type == cs.TS_COMMENT + and sibling.end_point[0] + 1 == first.start_point[0] + ): + first = sibling + sibling = sibling.prev_named_sibling + # Whole lines, unless other code shares the first or the last one + # (`function f() {} console.log('ready');` in JS): the whole-line cut + # deleted that statement from the old module and pasted it at the + # destination. Then only the definition's own bytes are cut. + own = _own_span(source, first, target) + if own is not None: + text = source[first.start_byte : target.end_byte].decode( + cs.ENCODING_UTF8, errors="replace" + ) + return _Cut(*own, text, first.start_byte) + start = source.rfind(b"\n", 0, first.start_byte) + 1 + end = source.find(b"\n", target.end_byte) + end = len(source) if end < 0 else end + 1 + text = source[start:end].decode(cs.ENCODING_UTF8, errors="replace") + # Swallow the blank lines that separated it from what follows. + while source[end : end + 1] == b"\n": + end += 1 + return _Cut(start, end, text, start) + + +def _line_end(source: bytes, node: Node) -> int: + end = source.find(b"\n", node.end_byte) + return len(source) if end < 0 else end + 1 + + +def _is_directive(node: Node) -> bool: + # A statement that is only a string literal: docstring or "use strict". + named = node.named_children + return ( + node.type == cs.TS_EXPRESSION_STATEMENT + and len(named) == 1 + and named[0].type == cs.TS_STRING + ) + + +def _prologue_end(source: bytes, root: Node) -> int: + """Byte offset just after the file's prologue (0 when it has none).""" + end = 0 + seen_directive = False + for child in root.children: + if child.type == cs.TS_COMMENT: + # Comments are stepped over, but only a shebang or PEP 263 + # coding line (first two lines) is pinned above the import: + # pinning every comment would split a `# note` from the + # definition it annotates. + if child.start_point[0] < _PINNED_COMMENT_LINES: + end = _line_end(source, child) + elif child.type in _PROLOGUE_TYPES: + end = _line_end(source, child) + elif not seen_directive and _is_directive(child): + # Only the FIRST statement can be the docstring or directive. + seen_directive = True + end = _line_end(source, child) + else: + break + return end + + +def _import_block_end(source: bytes, root: Node) -> int: + """Byte offset just after the last top-level import. + + With no import, just after the prologue rather than byte 0: an import + written above a module docstring turns `__doc__` into None, above a + shebang breaks the script, above `from __future__` is a SyntaxError, + and above "use strict" silently drops strict mode. All four still + parse, so the transaction's parse gate cannot catch them. + """ + end = 0 + for child in root.children: + if child.type in _IMPORT_TYPES: + end = _line_end(source, child) + return end or _prologue_end(source, root) + + +def _strip_project(qn: str, project: str) -> str: + prefix = f"{project}{cs.SEPARATOR_DOT}" + return qn[len(prefix) :] if qn.startswith(prefix) else qn + + +class Mover: + # What the import and paste generators below can write. TSX is left out + # on purpose: `_narrow_statement` spells JavaScript only for + # `_JS_LANGUAGES`, which does not name TSX, so a TSX move would copy + # its imports in Python syntax. Anything else has a grammar but no + # generator here, and every non-JS branch writes Python. + _MOVABLE = frozenset( + {cs.SupportedLanguage.PYTHON, cs.SupportedLanguage.JS, cs.SupportedLanguage.TS} + ) + _FUTURE = "__future__" + _FUTURE_IMPORT = "from __future__ import {features}\n" + # Refusals that have no home in constants/cli.py yet. + _UNSUPPORTED = "move does not support {language} ({path})" + _CROSS_LANGUAGE = ( + "{new_path} is not in the language of {old_path}; a move cannot " + "change a definition's language" + ) + _NESTED = "{qn} is nested inside another definition; move that one instead" + _COLLISION = "{path} already binds {name}; the moved definition would shadow it" + _NOT_EXPORTED = ( + "{names} is not exported from {path}, so the moved definition could " + "not import it; export it first" + ) + _ROLLBACK_REFUSED = ( + "Move failed its postcondition ({reasons}) and was not rolled back " + "({error}); the moved files may remain modified; check the working tree" + ) + + def __init__( + self, + repo_root: Path, + fetch_all: QueryFn, + project_name: str, + verify: Callable[[StagedTree], VerificationResult | bool | None] | None = None, + reingest: Reingest | None = None, + ) -> None: + self.repo_root = repo_root.resolve() + self.fetch_all = fetch_all + self.project = project_name + self.verify = verify + self.reingest = reingest + self._parsers = load_parsers()[0] + # Whether the last plan respelled imports inside the moved text, + # which changes its shape: the contract must then take the move's + # word for the pairing. + self._reshaped = False + + def _parse( + self, path: str, source: bytes + ) -> tuple[cs.SupportedLanguage | None, Node]: + language = get_language_for_extension(Path(path).suffix) + parser = self._parsers.get(language) if language is not None else None + if parser is None: + raise MoveRefused(cs.MOVE_NO_GRAMMAR.format(path=path)) + return language, parser.parse(source).root_node + + def _target_path(self, target: str, old_path: str) -> str: + suffix = Path(old_path).suffix + if target.endswith(suffix) or "/" in target: + return Path(target).as_posix() + dotted = _strip_project(target, self.project) + return Path(*dotted.split(cs.SEPARATOR_DOT)).with_suffix(suffix).as_posix() + + # --- planning --------------------------------------------------------------------- + + def plan( + self, qn: str, target: str, keep_alias: bool = False + ) -> tuple[MoveReport, Patcher, str | None]: + old_path, name, start_line, end_line = self._definition(qn) + new_path = self._target_path(target, old_path) + if new_path == old_path: + raise MoveRefused(cs.MOVE_SAME_MODULE.format(path=old_path)) + old_module = base_module_qn(Path(old_path), self.project) + new_module = base_module_qn(Path(new_path), self.project) + patcher = Patcher(self.repo_root) + source = patcher.source(old_path) + language, root = self._parse_movable(old_path, new_path, source) + node, top = self._top_definition( + qn, old_path, name, source, language, root, start_line, end_line + ) + existing = self._existing_target(patcher, new_path) + target_root: Node | None = None + if existing is not None: + _language, target_root = self._parse(new_path, existing) + cut = _cut_span(source, node) + moved_text = ( + self._rebase_local_imports(source, top, cut, qn, old_path, new_path) + if language == cs.SupportedLanguage.PYTHON + else cut.text + ) + self._reshaped = moved_text != cut.text + # A JS definition exported by its own `export { helper }` lost the + # export with the cut: the destination did not export it, and every + # rewritten importer (and the old module's import of it back) + # failed to link. The entry moves with it unless `keep_alias`. + listed = ( + self._listed_exports(source, root, name) + if language in _JS_LANGUAGES + else [] + ) + old_edits = [SpanEdit(cut.start, cut.end, b"")] + if not keep_alias: + old_edits.extend(listed) + remainder = apply_span_edits(source, old_edits).decode( + cs.ENCODING_UTF8, errors="replace" + ) + + old_spelled = _strip_project(old_module, self.project) + new_spelled = _strip_project(new_module, self.project) + needed = self._needed_imports( + old_module, old_path, new_path, source, root, cut.text, language + ) + old_bindings, guarded = self._old_bindings( + source, root, cut.text, remainder, name, old_path, new_path, language + ) + from_old = self._needed_from_old(old_path, cut.text, name, old_bindings) + self._refuse_js_imports(top, from_old, old_bindings, old_path, language) + if existing is not None: + assert target_root is not None + self._refuse_collisions( + existing, + target_root, + old_module, + new_module, + new_path, + name, + needed, + self._from_old_statement( + from_old, old_spelled, old_path, new_path, language + ), + from_old, + guarded, + language, + ) + self._refuse_future_mismatch( + source, existing, cut.text, name, old_path, new_path, language + ) + old_uses = _uses(remainder, name) + # The old module imports the moved name back: it still uses it, or + # keeps a deprecation alias of it. + old_needs_new = old_uses or keep_alias + if not keep_alias: + self._refuse_wildcard_loss( + old_module, old_path, source, root, name, language, old_uses + ) + # The cycle check runs on the graph BEFORE any file is touched. + self._refuse_cycles( + old_module, + new_module, + [n.target_qn for n in needed], + bool(from_old), + old_needs_new, + old_path, + new_path, + qn, + name, + language, + ) + + # 1. Cut from the old module, import the name back when still used. + for edit in old_edits: + patcher.replace_span(old_path, (edit.start, edit.end), edit.text) + if old_needs_new: + self._add_import( + patcher, + old_path, + source, + root, + language, + new_spelled, + new_path, + name, + use=old_uses, + # An export list `keep_alias` left in place already exports it. + export=keep_alias and not listed, + ) + # 2. Paste into the target with what it needs. + guard_lines, guard_head = self._place_guarded( + patcher, existing, target_root, guarded, new_path + ) + paste = self._paste_text( + moved_text, + needed, + from_old, + old_spelled, + old_path, + new_path, + language, + guard_lines, + guard_head, + ) + if self._exports_moved(language, top, listed, old_needs_new): + # Whatever imports it from the destination needs it exported + # there: the rewritten importers, and the old module. + paste += f"\nexport {{ {name} }};\n" + new_content = self._place_paste( + patcher, new_path, existing, target_root, source, paste, language + ) + # 3. Importers, and uses through a module import (`pkg.util.helper`). + self._retarget_attribute_uses( + patcher, qn, old_spelled, new_spelled, name, language + ) + importers, unchanged = self._retarget_importers( + patcher, old_module, old_spelled, new_spelled, new_path, name, language + ) + files = sorted(set(patcher.pending) | {new_path}) + report = MoveReport( + qualified_name=qn, + new_qualified_name=f"{new_module}{cs.SEPARATOR_DOT}{name}", + old_path=old_path, + new_path=new_path, + applied=False, + transaction_id="", + files=tuple(files), + importers=tuple(sorted(importers)), + unchanged_importers=tuple(sorted(unchanged)), + copied_imports=tuple(n.statement for n in needed) + tuple(guarded.values()), + diff=self._preview_diff(patcher, new_path, new_content), + message=cs.MOVE_PLANNED.format( + importers=len(importers), unchanged=len(unchanged) + ), + ) + return report, patcher, new_content + + def _definition(self, qn: str) -> tuple[str, str, int, int]: + """The path, name, first and last line the graph holds for `qn`; + refused when it holds no definition there, or holds a method.""" + definition = graph_query.definition( + self.fetch_all, self.project, qn, self.repo_root + ) + if not definition["found"] or not definition["path"]: + raise MoveRefused(cs.MOVE_UNKNOWN.format(qn=qn)) + if definition["label"] == cs.NodeLabel.METHOD.value: + raise MoveRefused(cs.MOVE_METHOD.format(qn=qn)) + return ( + definition["path"], + definition["name"] or qn.rsplit(cs.SEPARATOR_DOT, 1)[-1], + definition["start_line"] or 1, + definition["end_line"] or 1, + ) + + def _parse_movable( + self, old_path: str, new_path: str, source: bytes + ) -> tuple[cs.SupportedLanguage | None, Node]: + """The old module's language and tree; refused when the move cannot + write that language, or the destination is in another one.""" + language, root = self._parse(old_path, source) + if language not in self._MOVABLE: + raise MoveRefused( + self._UNSUPPORTED.format(language=language, path=old_path) + ) + # An explicit `pkg/core.ts` for a Python definition would get Python + # text under a TypeScript name, and no parser of the right language + # would ever look at it. + if get_language_for_extension(Path(new_path).suffix) != language: + raise MoveRefused( + self._CROSS_LANGUAGE.format(new_path=new_path, old_path=old_path) + ) + return language, root + + @classmethod + def _top_definition( + cls, + qn: str, + old_path: str, + name: str, + source: bytes, + language: cs.SupportedLanguage | None, + root: Node, + start_line: int, + end_line: int, + ) -> tuple[Node, Node]: + """The definition's node and the module statement holding it (the + node, or the decorators or export wrapping it); refused when the + name is not found in the span, or the definition is nested.""" + token = _name_token(source, language, start_line, end_line, name) + node = _definition_at(root, *token) if token else None + if node is None: + raise MoveRefused(cs.MOVE_NO_DEFINITION_TOKEN.format(qn=qn, path=old_path)) + # A nested function or class keeps its indentation when cut, and + # pasted at module level it no longer parses (or, one level down, + # silently changes what it closes over). + top = ( + node.parent + if node.parent is not None and node.parent.type in _WRAPPERS + else node + ) + if top.parent is None or top.parent.type != root.type: + raise MoveRefused(cls._NESTED.format(qn=qn)) + return node, top + + @classmethod + def _old_bindings( + cls, + source: bytes, + root: Node, + moved: str, + remainder: str, + name: str, + old_path: str, + new_path: str, + language: cs.SupportedLanguage | None, + ) -> tuple[dict[str, bool], dict[str, str]]: + """The old module's names the moved text can import, each with + whether it is exported, and the TYPE_CHECKING imports it carries + under a guard instead; refused when the move would split a module + variable or need a name some path leaves unbound.""" + bindings = cls._bindings(root) + if language != cs.SupportedLanguage.PYTHON: + return bindings, {} + # `_bindings` reads top-level statements only, so a constant set + # in both branches of an `if` was left behind (a NameError once + # moved); a name some path leaves unbound cannot be carried. + carried, unsafe = _flow_bindings(source) + starred, modules = _star_provided(source, moved, name) + if starred: + raise MoveRefused( + cs.MOVE_STAR_PROVIDED.format( + names=cs.SEPARATOR_COMMA_SPACE.join(starred), + modules=cs.SEPARATOR_COMMA_SPACE.join(modules), + path=old_path, + ) + ) + reads = _global_references(moved) + unbound = sorted( + n + for n in unsafe + if n != name and (n in reads if reads is not None else _uses(moved, n)) + ) + split = _split_globals(moved, remainder, name) + if split: + raise MoveRefused( + cs.MOVE_SPLIT_GLOBAL.format( + names=cs.SEPARATOR_COMMA_SPACE.join(split), path=old_path + ) + ) + typed = _type_checking_imports(source, old_path, new_path) + guarded = {n: typed[n] for n in unbound if n in typed} + unbound = [n for n in unbound if n not in guarded] + if unbound: + raise MoveRefused( + cs.MOVE_MAYBE_UNBOUND.format( + names=cs.SEPARATOR_COMMA_SPACE.join(unbound), path=old_path + ) + ) + return {**dict.fromkeys(carried, False), **bindings}, guarded + + @classmethod + def _refuse_js_imports( + cls, + top: Node, + from_old: list[str], + old_bindings: dict[str, bool], + old_path: str, + language: cs.SupportedLanguage | None, + ) -> None: + """Refuse when the JS/TS destination's import of the old module's + names could not work.""" + if language not in _JS_LANGUAGES: + return + # An imported binding is read-only: the moved `count += 1` that + # updated the old module's `let` would throw at the destination. + assigned = sorted(_js_assigned(top) & set(from_old)) + if assigned: + raise MoveRefused( + cs.MOVE_ASSIGNS_IMPORT.format( + names=cs.SEPARATOR_COMMA_SPACE.join(assigned), path=old_path + ) + ) + # `import { X }` of a name the module never exported is a load + # error in ESM and a compile error in TypeScript. + hidden = [n for n in from_old if not old_bindings.get(n, False)] + if hidden: + raise MoveRefused( + cls._NOT_EXPORTED.format(names=", ".join(hidden), path=old_path) + ) + + @staticmethod + def _exports_moved( + language: cs.SupportedLanguage | None, + top: Node, + listed: list[SpanEdit], + old_needs_new: bool, + ) -> bool: + """Whether the JS/TS destination must export the moved name, which + something imports from there and the moved text does not export.""" + return ( + language in _JS_LANGUAGES + and top.type != cs.TS_EXPORT_STATEMENT + and (bool(listed) or old_needs_new) + ) + + @classmethod + def _place_paste( + cls, + patcher: Patcher, + new_path: str, + existing: bytes | None, + target_root: Node | None, + source: bytes, + paste: str, + language: cs.SupportedLanguage | None, + ) -> str | None: + """Append `paste` to the destination, or return the whole content of + a destination the move creates.""" + # `from __future__ import annotations` is never referenced by name, + # so the use filter in `_needed_imports` drops it, and without it + # the destination evaluates the moved annotations eagerly: a forward + # reference that loaded fine before the move is a NameError after it. + futures = ( + cls._future_features(source) + if language == cs.SupportedLanguage.PYTHON + else [] + ) + if existing is None: + head = ( + cls._FUTURE_IMPORT.format(features=", ".join(futures)) + "\n" + if futures + else "" + ) + return head + paste.lstrip("\n") + assert target_root is not None + cls._append_paste(patcher, new_path, existing, target_root, paste, futures) + return None + + @classmethod + def _append_paste( + cls, + patcher: Patcher, + new_path: str, + existing: bytes, + target_root: Node, + paste: str, + futures: list[str], + ) -> None: + """Append `paste` to an existing destination, and enable the + `futures` it lacks at its head.""" + sep = ( + "" + if not existing.strip() + else ("\n" if existing.endswith(b"\n\n") else "\n\n") + ) + if existing and not existing.endswith(b"\n"): + sep = "\n" + sep + have = set(cls._future_features(existing)) + # Enabled for the whole file, `annotations` turns the destination's + # own annotations into strings (`existing.__annotations__['x'] is + # int` went False); text that annotates nothing does not need it, + # and `_refuse_future_mismatch` refused the case that does. + missing = [ + f + for f in futures + if f not in have and (f != _FUTURE_ANNOTATIONS or _has_annotations(paste)) + ] + future_line = ( + cls._FUTURE_IMPORT.format(features=", ".join(missing)) if missing else "" + ) + # A future statement must precede every other statement, so it + # goes at the head of the destination (after its docstring), + # never with the pasted imports at the end. + at = cls._head_end(existing, target_root) + if future_line and at < len(existing): + patcher.replace_span(new_path, (at, at), future_line) + future_line = "" + patcher.replace_span( + new_path, + (len(existing), len(existing)), + sep + future_line + paste.lstrip("\n"), + ) + + @classmethod + def _refuse_future_mismatch( + cls, + source: bytes, + existing: bytes, + moved: str, + name: str, + old_path: str, + new_path: str, + language: cs.SupportedLanguage | None, + ) -> None: + """Refuse when the two modules disagree on postponed annotations + and the move would change how some annotation evaluates. + + A future statement applies to its whole file. Adding the old + module's `annotations` to a destination that annotates its own code + changes those annotations' values; a destination that has it while + the old module did not changes the moved definition's. + """ + if language != cs.SupportedLanguage.PYTHON: + return + old_has = _FUTURE_ANNOTATIONS in cls._future_features(source) + new_has = _FUTURE_ANNOTATIONS in cls._future_features(existing) + if old_has == new_has or not _has_annotations(moved, source): + return + if old_has and not _has_annotations(existing): + return + enabled, disabled = (old_path, new_path) if old_has else (new_path, old_path) + raise MoveRefused( + cs.MOVE_FUTURE_ANNOTATIONS.format( + enabled=enabled, + disabled=disabled, + changed=new_path if old_has else name, + ) + ) + + @staticmethod + def _preview_diff(patcher: Patcher, new_path: str, new_content: str | None) -> str: + """The diff `apply` would write, so a dry run can be inspected. + + Built from the patched contents in memory: nothing is staged or + written, and `apply` replaces it with the committed diff. + """ + staged = [ + StagedFile(key, patcher.source(key), result.content) + for key, result in patcher.apply().items() + ] + if new_content is not None: + staged.append( + StagedFile(new_path, None, new_content.encode(cs.ENCODING_UTF8)) + ) + return "".join(unified_diff(s) for s in sorted(staged)) + + def _existing_target(self, patcher: Patcher, new_path: str) -> bytes | None: + """The destination's bytes, or None when it is a new file. + + Only an absent path inside the repository is a new file. Reading + every `PatcherError` as "absent" planned a target outside the repo + as a file to create, and the refusal came later, from the + transaction, as a crash instead of a refused move. + """ + try: + return patcher.source(new_path) + except PatcherError as error: + candidate = self.repo_root / new_path + try: + inside = candidate.resolve().is_relative_to(self.repo_root) + except OSError: + inside = False + if inside and not candidate.exists() and not candidate.is_symlink(): + return None + raise MoveRefused(str(error)) from error + + @staticmethod + def _rebase_local_imports( + source: bytes, + top: Node, + cut: _Cut, + qn: str, + old_path: str, + new_path: str, + ) -> str: + """The cut text with each relative import inside it respelled for + `new_path`, where it stays, in the scope it was written in. + + Copied verbatim, `from .deps import X` in a function moved from + pkg/sub/util.py to pkg/core.py counted its dots from `pkg` and + imported `pkg.deps` (or nothing) the first time the function ran. + """ + edits: list[SpanEdit] = [] + stack = [top] + while stack: + current = stack.pop() + stack.extend(current.named_children) + if current.type != cs.TS_PY_IMPORT_FROM_STATEMENT: + continue + module = current.child_by_field_name(cs.FIELD_MODULE_NAME) + if module is None or module.type != cs.TS_RELATIVE_IMPORT: + continue + spelled = _text(module) + level = len(spelled) - len(spelled.lstrip(cs.SEPARATOR_DOT)) + # Climbing to or past the tree's top names no package any + # destination could spell. + if level - 1 >= len(Path(old_path).parent.parts): + raise MoveRefused( + cs.MOVE_RELATIVE_IMPORT_ESCAPES.format( + statement=_text(current), qn=qn, path=old_path + ) + ) + rebased = _rebase_py_relative(spelled, old_path, new_path) + if rebased != spelled: + edits.append( + SpanEdit( + module.start_byte - cut.origin, + module.end_byte - cut.origin, + rebased.encode(cs.ENCODING_UTF8), + ) + ) + if not edits: + return cut.text + moved = apply_span_edits(cut.text.encode(cs.ENCODING_UTF8), edits) + return moved.decode(cs.ENCODING_UTF8, errors="replace") + + @staticmethod + def _bindings(root: Node) -> dict[str, bool]: + """Top-level names a module binds, each with whether it is exported. + + Definitions, and plain assignments or `const`/`let`/`var` + declarations, which the graph has no node for. Imports are not + included: they are copied by `_needed_imports`. "Exported" only + means something for JavaScript and TypeScript. + """ + bound: dict[str, bool] = {} + exported_as: set[str] = set() + for child in root.children: + exported = child.type == cs.TS_EXPORT_STATEMENT + nodes = list(child.named_children) if child.type in _WRAPPERS else [child] + for node in nodes: + if node.type == cs.TS_EXPORT_CLAUSE: + exported_as.update(_clause_exports(node)) + continue + for named in Mover._bound_names(node): + bound[named] = bound.get(named, False) or exported + bound.update(dict.fromkeys(exported_as & bound.keys(), True)) + return bound + + @staticmethod + def _listed_exports(source: bytes, root: Node, name: str) -> list[SpanEdit]: + """The edits that take `name` out of the module's `export { ... }` + lists: the entry alone, or the whole statement when it was the only + one. Only an unaliased entry is taken, since importers of `name` + are rewritten to the destination; an alias (`helper as h`) stays, + and the old module imports the name back for it. + """ + edits: list[SpanEdit] = [] + for child in root.children: + if ( + child.type != cs.TS_EXPORT_STATEMENT + or child.child_by_field_name(cs.FIELD_SOURCE) is not None + ): + continue + clause = next( + (c for c in child.named_children if c.type == cs.TS_EXPORT_CLAUSE), + None, + ) + if clause is None: + continue + specs = [ + spec + for spec in clause.named_children + if spec.type == cs.TS_EXPORT_SPECIFIER + ] + kept = [ + _text(spec) + for spec in specs + if _text(spec.child_by_field_name(cs.FIELD_NAME)) != name + or spec.child_by_field_name(cs.FIELD_ALIAS) is not None + ] + if len(kept) == len(specs): + continue + if kept: + edits.append( + SpanEdit( + clause.start_byte, + clause.end_byte, + f"{{ {', '.join(kept)} }}".encode(cs.ENCODING_UTF8), + ) + ) + continue + # Its own bytes when other code shares its line: the whole-line + # removal took `registerPlugin();` in `export { helper }; + # registerPlugin();` with it. + own = _own_span(source, child, child) + if own is not None: + edits.append(SpanEdit(*own, b"")) + continue + start = source.rfind(b"\n", 0, child.start_byte) + 1 + end = _line_end(source, child) + # Swallow the blank lines that separated it from what follows. + while source[end : end + 1] == b"\n": + end += 1 + edits.append(SpanEdit(start, end, b"")) + return edits + + @staticmethod + def _bound_names(node: Node) -> list[str]: + # An import statement has a `name` field too (the imported module). + if node.type in _IMPORT_TYPES: + return [] + named = node.child_by_field_name(cs.FIELD_NAME) + if named is not None: + return [_text(named)] + if node.type in (cs.TS_LEXICAL_DECLARATION, cs.TS_VARIABLE_DECLARATION): + declared = ( + d.child_by_field_name(cs.FIELD_NAME) + for d in node.named_children + if d.type == cs.TS_VARIABLE_DECLARATOR + ) + return [ + _text(n) + for n in declared + if n is not None and n.type == cs.TS_IDENTIFIER + ] + if node.type != cs.TS_PY_EXPRESSION_STATEMENT: + return [] + names: list[str] = [] + for assignment in node.named_children: + # `a = b = 1` nests the second target in the right-hand side. + while assignment is not None and assignment.type == cs.TS_PY_ASSIGNMENT: + left = assignment.child_by_field_name(cs.FIELD_LEFT) + if left is not None and left.type == cs.TS_PY_IDENTIFIER: + names.append(_text(left)) + elif left is not None and left.type in cs.PY_UNPACKING_TARGET_TYPES: + names.extend( + _text(n) + for n in left.named_children + if n.type == cs.TS_PY_IDENTIFIER + ) + assignment = assignment.child_by_field_name(cs.FIELD_RIGHT) + return names + + @classmethod + def _future_features(cls, source: bytes) -> list[str]: + """The `from __future__ import ...` features a Python module enables.""" + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + return [] + return [ + alias.name + for node in tree.body + if isinstance(node, ast.ImportFrom) and node.module == cls._FUTURE + for alias in node.names + ] + + @staticmethod + def _head_end(source: bytes, root: Node) -> int: + """Byte offset after a Python module's leading comments and docstring. + + Where a statement goes that must not displace them: a shebang or an + encoding line only works on the first lines, and a string that is no + longer the first statement is no longer `__doc__`. + """ + end = 0 + for child in root.children: + is_docstring = ( + child.type == cs.TS_PY_EXPRESSION_STATEMENT + and child.named_child_count == 1 + and child.named_children[0].type == cs.TS_PY_STRING + ) + if child.type != cs.TS_COMMENT and not is_docstring: + break + end = source.find(b"\n", child.end_byte) + end = len(source) if end < 0 else end + 1 + if is_docstring: + break + return end + + def _needed_imports( + self, + old_module: str, + old_path: str, + new_path: str, + source: bytes, + root: Node, + moved_text: str, + language: cs.SupportedLanguage | None, + ) -> list[_NeededImport]: + """The old module's import statements the moved text relies on. + + Only the module's own top-level imports are copied. The graph holds + an import written inside a function as well, and pasting that one at + the destination's top level makes an import the function ran only + on demand (an optional backend) run, and fail, on load. One inside + the moved definition travels with its text; one under a top-level + `if` or `try` is a module binding, left to `_flow_bindings`. + """ + top_level = [ + (child.start_byte, child.end_byte) + for child in root.children + if child.type in _IMPORT_TYPES + ] + rows = self.fetch_all( + cq.CYPHER_GRAPH_IMPORTS_OF, + { + cs.KEY_PROJECT_PREFIX: f"{self.project}{cs.SEPARATOR_DOT}", + cs.KEY_QN: old_module, + }, + ) + out: dict[str, _NeededImport] = {} + for row in rows: + site = _row_site(row, old_path) + if site is None or site.alias is None: + continue + bound = site.alias.split(cs.SEPARATOR_DOT)[0] + if not _uses(moved_text, bound) or not _starts_in(source, site, top_level): + continue + statement = _statement_text(source, site) + narrowed = _narrow_statement( + statement, site.alias, language, old_path, new_path + ) + # Future directives are placed by `plan`, at the destination's + # head; one pasted among the ordinary imports is a SyntaxError. + if ( + narrowed is None + or row.get(cs.KEY_IMPORTED_NAME) == self._FUTURE + or (row.get(cs.KEY_TO_QN) == self._FUTURE) + ): + continue + target = str(row.get(cs.KEY_TO_QN) or "") + out.setdefault(narrowed, _NeededImport(narrowed, target, bound)) + return list(out.values()) + + def _needed_from_old( + self, + old_path: str, + moved_text: str, + name: str, + bindings: dict[str, bool], + ) -> list[str]: + """Old-module names the moved text still refers to. + + The graph has definitions only, so a module constant the moved code + reads (`DEFAULT_TIMEOUT = 5`) came from the parsed `bindings` + instead; without it the move parsed, passed its contract, and died + with a NameError the first time the moved code ran. + """ + rows = self.fetch_all( + cq.CYPHER_DELTA_DEFINITIONS, + { + cs.KEY_PROJECT_PREFIX: f"{self.project}{cs.SEPARATOR_DOT}", + cs.KEY_LONGER_PROJECT_PREFIXES: list( + _longer_project_prefixes(self.fetch_all, self.project) + ), + cs.CYPHER_PARAM_PATHS: [old_path], + }, + ) + names: set[str] = set() + for row in rows: + other = row.get(cs.KEY_NAME) + qn = str(row.get(cs.KEY_QUALIFIED_NAME) or "") + if ( + not isinstance(other, str) + or other == name + or cs.SEPARATOR_DOT + in qn[len(base_module_qn(Path(old_path), self.project)) + 1 :] + ): + continue + if _uses(moved_text, other): + names.add(other) + names.update( + other for other in bindings if other != name and _uses(moved_text, other) + ) + return sorted(names) + + def _refuse_wildcard_loss( + self, + old_module: str, + old_path: str, + source: bytes, + root: Node, + name: str, + language: cs.SupportedLanguage | None, + old_uses: bool, + ) -> None: + """Refuse when a wildcard importer of the old module would lose `name`. + + The graph records `from pkg.util import *` (and JS `export *` or + `import * as ns`) under the name `*`, which no importer rewrite + matches: the move left it in place, and `from pkg.api import + helper` through the re-export raised ImportError. The old module + importing the name back keeps a Python star reaching it; in JS only + an export does, which is `keep_alias`. + """ + if language == cs.SupportedLanguage.PYTHON: + if old_uses or not _star_exports(source, name): + return + elif not self._bindings(root).get(name, False): + return + importers = sorted( + { + row["path"] + for row in graph_query.importers( + self.fetch_all, self.project, old_module + ) + if row["imported_name"] == cs.IMPORTED_NAME_WILDCARD and row["path"] + } + ) + if importers: + raise MoveRefused( + cs.MOVE_WILDCARD_IMPORTER.format( + importers=cs.SEPARATOR_COMMA_SPACE.join(importers), + name=name, + path=old_path, + ) + ) + + def _refuse_cycles( + self, + old_module: str, + new_module: str, + copied_targets: list[str], + needs_old: bool, + old_needs_new: bool, + old_path: str, + new_path: str, + qn: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> None: + # `snapshot`'s module-import query is project-wide (it filters by + # project, not by `paths`), so an importer outside the two modules + # is in `graph` and a cycle through it is still seen. + graph = { + qn: set(targets) + for qn, targets in snapshot( + self.fetch_all, self.project, [old_path, new_path] + ).imports.items() + } + before = import_cycles({qn: frozenset(t) for qn, t in graph.items()}) + graph.setdefault(new_module, set()).update(t for t in copied_targets if t) + if needs_old: + graph.setdefault(new_module, set()).add(old_module) + if old_needs_new: + graph.setdefault(old_module, set()).add(new_module) + # Only the importers the move rewires gain an edge to the new module: + # an importer of some other name from the old module is untouched, and + # modelling it as rewired refused safe moves over cycles that would + # never exist. One whose only import from the old module is the moved + # name loses that edge. + rewired, keeps_old = self._rewired_importers(old_module, qn, name, language) + for importer in rewired - {old_module, new_module}: + targets = graph.setdefault(importer, set()) + targets.add(new_module) + if importer not in keeps_old: + targets.discard(old_module) + after = import_cycles({m: frozenset(t) for m, t in graph.items()}) + # Sorted, so the cycle named is the same on every run when there are + # several: set order follows string hashing, which is per-process. + fresh = sorted( + tuple(sorted(c)) + for c in after - before + if new_module in c or old_module in c + ) + if fresh: + cycle = fresh[0] + raise MoveRefused(cs.MOVE_CYCLE.format(cycle=" -> ".join(cycle)), cycle) + + def _rewired_importers( + self, + old_module: str, + qn: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> tuple[set[str], set[str]]: + """Modules the move points at the new module, and those of them (or + others) that still import something else from the old one.""" + rows = graph_query.importers(self.fetch_all, self.project, old_module) + rewired = {r["module"] for r in rows if r["imported_name"] == name} + keeps_old = {r["module"] for r in rows if r["imported_name"] != name} + if language == cs.SupportedLanguage.PYTHON: + # `pkg.util.helper(...)` through `import pkg.util` gains an + # `import pkg.new` (`_retarget_attribute_uses`) and keeps its + # module import. + for row in graph_query.callers(self.fetch_all, self.project, qn): + if isinstance(row["path"], str): + module = base_module_qn(Path(row["path"]), self.project) + if module in keeps_old: + rewired.add(module) + return rewired, keeps_old + + def _add_import( + self, + patcher: Patcher, + path: str, + source: bytes, + root: Node, + language: cs.SupportedLanguage | None, + new_spelled: str, + new_path: str, + name: str, + use: bool, + export: bool, + ) -> None: + """Import `name` back into the old module (`use`), re-export it + from there (`export`), or both. + + Both is JavaScript's case only: a Python import of the name is + already a re-export, but a JS import is not, and writing just the + import when the old module still used the name took the old path + away from every external importer `keep_alias` was meant to keep. + """ + at = _import_block_end(source, root) + if not at and language == cs.SupportedLanguage.PYTHON: + # No imports: offset 0 would push a shebang, an encoding line or + # the module docstring down, and the docstring stops being one. + at = self._head_end(source, root) + if language in _JS_LANGUAGES: + spec = _relative_specifier(path, new_path) + if not use: + line = f"export {{ {name} }} from '{spec}';\n" + else: + line = f"import {{ {name} }} from '{spec}';\n" + if export: + line += f"export {{ {name} }};\n" + else: + line = f"from {new_spelled} import {name}\n" + if export and not use: + line = f"from {new_spelled} import {name} # noqa: F401 (moved; re-exported for compatibility)\n" + patcher.replace_span(path, (at, at), line if at else line + "\n") + + def _paste_text( + self, + moved: str, + needed: list[_NeededImport], + from_old: list[str], + old_spelled: str, + old_path: str, + new_path: str, + language: cs.SupportedLanguage | None, + guarded: list[str], + guard_head: bool, + ) -> str: + lines = [n.statement.rstrip("\n") for n in needed] + statement = self._from_old_statement( + from_old, old_spelled, old_path, new_path, language + ) + if statement is not None: + lines.append(statement) + if guard_head: + lines.append(_TYPE_CHECKING_IMPORT) + header = "\n".join(lines) + if guarded: + block = _TYPE_CHECKING_HEAD + "\n".join(_PY_INDENT + g for g in guarded) + header = (header + "\n\n" if header else "") + block + return (header + "\n\n\n" if header else "") + moved.rstrip("\n") + "\n" + + def _place_guarded( + self, + patcher: Patcher, + existing: bytes | None, + target_root: Node | None, + guarded: dict[str, str], + new_path: str, + ) -> tuple[list[str], bool]: + """Put the TYPE_CHECKING-only imports under the destination's guard. + + Merged into the destination's own `if TYPE_CHECKING:` when it has + one (and an import it already holds is not repeated); otherwise + returned for `_paste_text` to write in a new guard, with whether + `TYPE_CHECKING` itself must be imported for it. + """ + language = cs.SupportedLanguage.PYTHON + if not guarded: + return [], False + if existing is None or target_root is None: + return list(guarded.values()), True + held = self._import_texts(existing, target_root, new_path) + lines = [ + statement + for local, statement in guarded.items() + if not any( + _binding_key(other, local, language, new_path) + == _binding_key(statement, local, language, new_path) + for other in held + ) + ] + block = _type_checking_block(target_root) + if block is None: + binds_flag = _TYPE_CHECKING in self._bindings(target_root) or any( + _binding_key(other, _TYPE_CHECKING, language, new_path) is not None + for other in held + ) + return lines, bool(lines) and not binds_flag + if lines: + at = _line_end(existing, block) + indent = " " * block.named_children[0].start_point[1] + text = "".join(f"{indent}{line}\n" for line in lines) + if at == len(existing) and not existing.endswith(b"\n"): + text = "\n" + text + patcher.replace_span(new_path, (at, at), text) + return [], False + + @staticmethod + def _import_texts(source: bytes, root: Node, path: str) -> list[str]: + """A Python module's top-level imports and its TYPE_CHECKING ones.""" + top = [_text(c) for c in root.children if c.type in _IMPORT_TYPES] + return top + list(_type_checking_imports(source, path, path).values()) + + @staticmethod + def _from_old_statement( + from_old: list[str], + old_spelled: str, + old_path: str, + new_path: str, + language: cs.SupportedLanguage | None, + ) -> str | None: + """The import the destination needs of the old module's names.""" + if not from_old: + return None + if language in _JS_LANGUAGES: + spec = _relative_specifier(new_path, old_path) + return f"import {{ {', '.join(from_old)} }} from '{spec}';" + return f"from {old_spelled} import {', '.join(from_old)}" + + def _refuse_collisions( + self, + existing: bytes, + target_root: Node, + old_module: str, + new_module: str, + new_path: str, + name: str, + needed: list[_NeededImport], + from_old_statement: str | None, + from_old: list[str], + guarded: dict[str, str], + language: cs.SupportedLanguage | None, + ) -> None: + """Refuse when anything the move binds at the destination rebinds + a name the destination already binds to something else. + + Checking the moved name alone let a pasted `from pkg.util import + RATE` replace the destination's own `RATE`, and every existing + reader of it silently saw the old module's value. An import that + binds the name to what it is already bound to replaces nothing. + """ + defined = set(self._bindings(target_root)) + imports = [_text(c) for c in target_root.children if c.type in _IMPORT_TYPES] + if language == cs.SupportedLanguage.PYTHON: + carried, unsafe = _flow_bindings(existing) + # A TYPE_CHECKING import is compared like any import: binding + # the same name to the same thing under the guard is no clash. + typed = _type_checking_imports(existing, new_path, new_path) + defined |= carried | (unsafe - typed.keys()) + imports.extend(typed.values()) + # The destination's own import of the moved name from the old + # module is rewired by the move, not rebound by it. + rewired = any( + row["module"] == new_module and row["imported_name"] == name + for row in graph_query.importers(self.fetch_all, self.project, old_module) + ) + if name in defined or ( + not rewired + and any( + _binding_key(other, name, language, new_path) is not None + for other in imports + ) + ): + raise MoveRefused(self._COLLISION.format(path=new_path, name=name)) + added = [(n.local, n.statement) for n in needed] + if from_old_statement is not None: + added.extend((n, from_old_statement) for n in from_old) + added.extend(guarded.items()) + for local, statement in added: + key = _binding_key(statement, local, language, new_path) + if local in defined or any( + other_key is not None and other_key != key + for other_key in ( + _binding_key(other, local, language, new_path) for other in imports + ) + ): + raise MoveRefused( + cs.MOVE_IMPORT_COLLISION.format(path=new_path, name=local) + ) + + def _retarget_importers( + self, + patcher: Patcher, + old_module: str, + old_spelled: str, + new_spelled: str, + new_path: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> tuple[list[str], list[str]]: + rewriter = ImportRewriter(self.repo_root, patcher) + importers: set[str] = set() + unchanged: set[str] = set() + for row in graph_query.importers(self.fetch_all, self.project, old_module): + if ( + row["imported_name"] != name + or row["path"] is None + or row["line"] is None + ): + continue + site = ImportSite( + row["path"], + row["line"], + row["col"] or 0, + row["end_line"] or row["line"], + row["end_col"] or 0, + row["alias"], + row["imported_name"], + ) + spelled = old_spelled + if language in _JS_LANGUAGES: + statement = _statement_text(patcher.source(site.path), site) + spec = _JS_SPEC.search(statement) + spelled = spec.group("spec") if spec else old_spelled + move = SymbolMove(name, spelled, new_spelled, new_module_path=new_path) + if rewriter.retarget([site], move): + importers.add(site.path) + else: + unchanged.add(f"{site.path}:{site.line}") + return sorted(importers), sorted(unchanged) + + def _retarget_attribute_uses( + self, + patcher: Patcher, + qn: str, + old_spelled: str, + new_spelled: str, + name: str, + language: cs.SupportedLanguage | None, + ) -> None: + """`pkg.util.helper(...)` through `import pkg.util` follows the move.""" + if language != cs.SupportedLanguage.PYTHON: + return + old_attr = f"{old_spelled}{cs.SEPARATOR_DOT}{name}".encode(cs.ENCODING_UTF8) + new_attr = f"{new_spelled}{cs.SEPARATOR_DOT}{name}" + touched: set[str] = set() + for row in graph_query.callers(self.fetch_all, self.project, qn): + path, line, col = row["path"], row["line"], row["col"] + if not isinstance(path, str) or line is None or col is None: + continue + try: + source = patcher.source(path) + except PatcherError: + continue + start = line_col_to_byte(source, line, col) + end = line_col_to_byte( + source, row["end_line"] or line, row["end_col"] or col + ) + at = source.find(old_attr, start, end) + if at < 0: + # A member call's site starts at its name (#3166), past the + # module path it is reached through. + at = start + len(name.encode(cs.ENCODING_UTF8)) - len(old_attr) + if at < 0 or source[at : at + len(old_attr)] != old_attr: + continue + patcher.replace_span(path, (at, at + len(old_attr)), new_attr) + if path not in touched: + touched.add(path) + self._import_module(patcher, path, source, new_spelled) + + def _import_module( + self, patcher: Patcher, path: str, source: bytes, module: str + ) -> None: + """Give `path` the `import module` its rewritten call sites need, + unless it already binds the module's root name.""" + _language, root = self._parse(path, source) + text = source.decode(cs.ENCODING_UTF8, errors="replace") + if not _module_bound(text, module): + at_import = _import_block_end(source, root) + patcher.replace_span(path, (at_import, at_import), f"import {module}\n") + + # --- applying --------------------------------------------------------------------- + + def apply(self, qn: str, target: str, keep_alias: bool = False) -> MoveReport: + report, patcher, new_content = self.plan(qn, target, keep_alias) + tx = EditTransaction(self.repo_root) + results = patcher.stage_into(tx) + broken = [key for key, result in results.items() if result.parses is False] + # What each staged file held when the move was planned: the patched + # bytes the edits were built from, and nothing at a new destination. + planned: dict[str, bytes | None] = {key: patcher.source(key) for key in results} + if new_content is not None: + planned[report.new_path] = None + # `stage_into` parses every patched file; a new destination is + # not patched, so it gets the same gate here. + if self._parses(report.new_path, new_content): + tx.stage(report.new_path, new_content) + else: + broken.append(report.new_path) + # `stage` takes the disk as the baseline, so a file edited (or a + # destination created) since planning would be overwritten with + # content built from the old bytes, and commit's conflict check + # would compare against the edit itself and pass. + changed = [ + staged.path + for staged in tx.staged + if planned.get(staged.path, staged.before) != staged.before + ] + if changed: + tx.rollback() + raise MoveRefused( + cs.MOVE_SOURCE_CHANGED.format( + files=cs.SEPARATOR_COMMA_SPACE.join(changed) + ) + ) + if broken: + tx.rollback() + return report._replace( + message=cs.MOVE_PARSE_FAILED.format(files=", ".join(broken)) + ) + + def verifier(tree: StagedTree) -> VerificationResult | bool | None: + return self.verify(tree) if self.verify is not None else True + + outcome = tx.commit(verifier) + report = report._replace( + applied=outcome.applied, + transaction_id=outcome.transaction_id, + files=outcome.files, + diff=outcome.diff, + message=outcome.message, + ) + if outcome.applied and self.reingest is not None: + report = self._enforce_contract(report) + return report + + def _parses(self, path: str, content: str) -> bool: + _language, root = self._parse(path, content.encode(cs.ENCODING_UTF8)) + return not root.has_error + + def _enforce_contract(self, report: MoveReport) -> MoveReport: + assert self.reingest is not None + renamed = (report.qualified_name, report.new_qualified_name) + try: + delta = measure( + self.fetch_all, + self.project, + self.repo_root, + report.files, + self.reingest, + # An empty class has no fingerprint and no members to match + # the two sides by, so the delta cannot infer this rename; + # without it the move of one reads as a removal plus an + # addition. + declared_renames=(renamed,), + reshaped_renames=(renamed,) if self._reshaped else (), + ) + # The move has landed; a graph that cannot be measured is reported, + # never raised past the committed edit (as for change_signature). + except Exception as error: # noqa: BLE001 + logger.warning(cs.MOVE_CONTRACT_UNMEASURED.format(error=error)) + return report._replace( + verdict=None, + # `ValueError` and `ReingestAborted` are raised before the + # re-ingest writes anything, so the graph is still whole. + graph_incomplete=not isinstance(error, ValueError | ReingestAborted), + message=cs.MOVE_CONTRACT_UNMEASURED.format(error=error), + ) + verdict = verify(move_expectation(*renamed), delta) + if verdict.ok: + return report._replace(verdict=verdict) + reasons = "; ".join(verdict.failures) + report = report._replace(verdict=verdict) + try: + # This move's own transaction, not whatever is newest: the lock + # is released before the re-ingest, so another edit can land in + # between, and `undo_last` reversed THAT edit and kept the move. + outcome = undo_transaction(self.repo_root, report.transaction_id) + except TransactionConflict as conflict: + return self._not_rolled_back(report, reasons, str(conflict)) + if not outcome.applied: + return self._not_rolled_back(report, reasons, outcome.message) + try: + self.reingest(list(report.files)) + # The files are restored; the graph may have lost the subtree the + # re-ingest deleted before failing. Say so, never raise. + except Exception as error: # noqa: BLE001 + message = cs.MOVE_ROLLBACK_UNMEASURED.format(reasons=reasons, error=error) + logger.warning(message) + return report._replace( + applied=False, graph_incomplete=True, message=message + ) + return report._replace( + applied=False, message=cs.MOVE_CONTRACT_FAILED.format(reasons=reasons) + ) + + def _not_rolled_back( + self, report: MoveReport, reasons: str, error: str + ) -> MoveReport: + """The report for a failed move this op could not reverse. + + `applied` stays True while the history still records the move: a + later edit stacked on it, or a file changed since, keeps it on disk. + An entry already gone may mean another actor reversed it, which + this op cannot tell from the history, so it claims nothing then. + """ + recorded = any( + entry.get(cs.EDIT_KEY_ID) == report.transaction_id + for entry in load_history(self.repo_root) + ) + return report._replace( + applied=recorded, + message=self._ROLLBACK_REFUSED.format(reasons=reasons, error=error), + ) + + +# --- statement helpers ----------------------------------------------------------------- + + +def _statement_text(source: bytes, site: ImportSite) -> str: + start = line_col_to_byte(source, site.line, site.col) + end = line_col_to_byte(source, site.end_line, site.end_col) + return source[start:end].decode(cs.ENCODING_UTF8, errors="replace") + + +def _row_site(row: ResultRow, path: str) -> ImportSite | None: + """The import in `path` an IMPORTS row describes; None when the row + lacks its alias or its start. A missing end falls back to the start.""" + alias = row.get(cs.KEY_ALIAS) + line, col = row.get(cs.KEY_LINE), row.get(cs.KEY_COL) + end_line, end_col = row.get(cs.KEY_END_LINE), row.get(cs.KEY_END_COL) + if ( + not isinstance(alias, str) + or not isinstance(line, int) + or not isinstance(col, int) + ): + return None + imported = row.get(cs.KEY_IMPORTED_NAME) + return ImportSite( + path, + line, + col, + end_line if isinstance(end_line, int) else line, + end_col if isinstance(end_col, int) else col, + alias, + imported if isinstance(imported, str) else None, + ) + + +def _starts_in(source: bytes, site: ImportSite, spans: list[tuple[int, int]]) -> bool: + """Whether `site` starts inside one of the byte `spans` of `source`.""" + try: + at = line_col_to_byte(source, site.line, site.col) + except PatcherError: + return False + return any(start <= at < end for start, end in spans) + + +def _narrow_statement( + statement: str, + alias: str, + language: cs.SupportedLanguage | None, + old_path: str, + new_path: str, +) -> str | None: + """The statement reduced to the entry binding `alias`, respelled for + the new file where the specifier is relative. + + The result is pasted at the top of the destination, so it must bind + `alias` there and nothing else: a sibling binding copied along can + shadow a name the destination already defines. + """ + if language in _JS_LANGUAGES: + return _narrow_js(statement, alias, old_path, new_path) + if parsed := _match_py_from(statement): + # main replaced the _PY_FROM regex with a token parser returning + # (lead, module, mid, names); only those two fields are needed here. + _lead, module, _mid, raw_names = parsed + entries, _open, _close = _split_names(raw_names) + kept = [e for e in entries if _local_name(e) == alias] + if not kept: + return None + module = _rebase_py_relative(module, old_path, new_path) + return f"from {module} import {kept[0]}" + return _narrow_py_import(statement, alias) + + +def _binding_key( + statement: str, + local: str, + language: cs.SupportedLanguage | None, + path: str, +) -> tuple[str, ...] | None: + """What the import `statement` binds `local` to, in `path`; None when + it does not bind `local` at all. + + Two statements that agree on it bind the same thing however they were + spelled: `import os` and `import os.path` both bind `os` to `os`. + """ + if language in _JS_LANGUAGES: + return _js_binding_key(statement, local, path) + try: + tree = ast.parse(statement.strip()) + except SyntaxError: + return None + for node in tree.body: + if isinstance(node, ast.Import): + key = _import_key(node, local) + elif isinstance(node, ast.ImportFrom): + key = _from_key(node, local) + else: + continue + if key is not None: + return key + return None + + +def _js_binding_key(statement: str, local: str, path: str) -> tuple[str, ...] | None: + """`_binding_key` for a JS/TS import: the clause binding `local`.""" + # A side-effect import (`import './x'`) binds nothing. + if _JS_FROM.search(statement) is None: + return None + narrowed = _narrow_js(statement, local, path, path) + return None if narrowed is None else (narrowed.rstrip(";").strip(),) + + +def _import_key(node: ast.Import, local: str) -> tuple[str, ...] | None: + """`_binding_key` for a Python `import a.b [as c]`.""" + for alias in node.names: + if alias.asname == local: + return (alias.name, "", local) + if alias.asname is None and alias.name.split(cs.SEPARATOR_DOT)[0] == local: + return (local, "", local) + return None + + +def _from_key(node: ast.ImportFrom, local: str) -> tuple[str, ...] | None: + """`_binding_key` for a Python `from m import a [as b]`.""" + module = cs.SEPARATOR_DOT * node.level + (node.module or "") + for alias in node.names: + if (alias.asname or alias.name) == local: + return (module, alias.name, local) + return None + + +def _rebase_py_relative(module: str, old_path: str, new_path: str) -> str: + """`module` as `new_path` must spell it to reach what `old_path` meant. + + A relative import counts its dots from the importing file's package, + so copying `from .deps import X` from `pkg/sub/util.py` into + `pkg/core.py` would silently switch it from `pkg.sub.deps` to + `pkg.deps` -- the wrong module, or none. The target is resolved from + the old file's directory and re-expressed from the new one's; both + are directories, which is also right for `__init__.py`, whose `.` is + its own package. + """ + level = len(module) - len(module.lstrip(cs.SEPARATOR_DOT)) + if level == 0: + return module + rest = [p for p in module[level:].split(cs.SEPARATOR_DOT) if p] + old_package = list(Path(old_path).parent.parts) + ups = level - 1 + if ups > len(old_package): + # Already escapes the tree it was written in; nothing to rebase. + return module + target = old_package[: len(old_package) - ups] + rest + new_package = list(Path(new_path).parent.parts) + common = 0 + while ( + common < min(len(target), len(new_package)) + and target[common] == new_package[common] + ): + common += 1 + if common == 0: + # No shared package: a relative import cannot climb there. + return cs.SEPARATOR_DOT.join(target) + dots = cs.SEPARATOR_DOT * (len(new_package) - common + 1) + return dots + cs.SEPARATOR_DOT.join(target[common:]) + + +def _narrow_py_import(statement: str, alias: str) -> str: + """`import a, b` reduced to the entry binding `alias`. + + `alias` is the graph's local name: the `as` name, else the dotted + path itself or its root (`import pkg.dep` binds `pkg`). Unparseable + or unmatched input is returned whole: copying a spare binding is + recoverable, omitting the needed one is a NameError. + """ + text = statement.strip() + try: + tree = ast.parse(text) + except SyntaxError: + return text + if len(tree.body) != 1 or not isinstance(tree.body[0], ast.Import): + return text + for entry in tree.body[0].names: + bound = entry.asname or entry.name + if bound == alias or ( + entry.asname is None and entry.name.split(cs.SEPARATOR_DOT)[0] == alias + ): + suffix = f" as {entry.asname}" if entry.asname else "" + return f"import {entry.name}{suffix}" + return text + + +def _js_entry_local(entry: str) -> str: + return _local_name(_TS_INLINE_TYPE.sub("", entry, count=1)) + + +def _narrow_js(statement: str, alias: str, old_path: str, new_path: str) -> str | None: + """A JS/TS import reduced to the one clause that binds `alias`. + + `import D, * as ns, { a } from 'm'` binds three kinds of name; each is + kept on its own. Looking only inside the braces lost the default and + namespace bindings (the moved code then referred to an unbound name) + and dragged the default along with a named entry. + """ + spec = _JS_SPEC.search(statement) + if spec is None: + return None + text = statement + if spec.group("spec").startswith("."): + target = (Path(old_path).parent / spec.group("spec")).as_posix() + text = ( + statement[: spec.start("spec")] + + _relative_specifier(new_path, target) + + statement[spec.end("spec") :] + ) + keyword = _JS_KEYWORD.match(text) + source_at = _JS_FROM.search(text) + if keyword is None or source_at is None or source_at.start() < keyword.end(): + # A side-effect import (`import './x'`) binds nothing to narrow. + return text.strip() + clause = text[keyword.end() : source_at.start()].strip() + tail = text[source_at.start() :] + named = _JS_NAMED.search(clause) + if named is not None: + entries = [e.strip() for e in named.group("names").split(",") if e.strip()] + kept = [e for e in entries if _js_entry_local(e) == alias] + if kept: + return f"{keyword.group(0)}{{ {', '.join(kept)} }}{tail}".strip() + clause = (clause[: named.start()] + clause[named.end() :]).strip() + for part in (p.strip() for p in clause.split(",")): + namespace = _JS_NAMESPACE.fullmatch(part) + if (namespace.group("name") if namespace else part) == alias: + return f"{keyword.group(0)}{part}{tail}".strip() + return None + + +def move( + repo_root: Path, + fetch_all: QueryFn, + project_name: str, + qualified_name: str, + target_module: str, + keep_alias: bool = False, + dry_run: bool = False, + verify: Callable[[StagedTree], VerificationResult | bool | None] | None = None, + reingest: Reingest | None = None, +) -> MoveReport: + """The op: plan (refusing on a cycle) or plan and apply.""" + mover = Mover(repo_root, fetch_all, project_name, verify=verify, reingest=reingest) + if dry_run: + report, _patcher, _content = mover.plan( + qualified_name, target_module, keep_alias + ) + return report + return mover.apply(qualified_name, target_module, keep_alias) diff --git a/codebase_rag/mcp/tools.py b/codebase_rag/mcp/tools.py index ab0dd08e9a..6cf03803e7 100644 --- a/codebase_rag/mcp/tools.py +++ b/codebase_rag/mcp/tools.py @@ -159,6 +159,11 @@ def _read_file_slice( cs.MCPToolName.REINGEST, cs.MCPToolName.RENAME, cs.MCPToolName.CHANGE_SIGNATURE, + # An EDIT like RENAME and CHANGE_SIGNATURE above: it runs its own + # re-ingest behind the incomplete-run marker and holds the result to + # the postcondition contract, so refusing up front on a partial graph + # would block the operation that repairs it (issue #1534). + cs.MCPToolName.MOVE, cs.MCPToolName.SURGICAL_REPLACE_CODE, cs.MCPToolName.READ_FILE, cs.MCPToolName.WRITE_FILE, @@ -683,6 +688,7 @@ def __init__( ), cs.MCPToolName.RENAME: self._rename_tool(), cs.MCPToolName.CHANGE_SIGNATURE: self._change_signature_tool(), + cs.MCPToolName.MOVE: self._move_tool(), cs.MCPToolName.CONTEXT: self._context_tool(), cs.MCPToolName.QUERY_CODE_GRAPH: ToolMetadata( name=cs.MCPToolName.QUERY_CODE_GRAPH, @@ -3404,6 +3410,113 @@ def prop(kind: cs.MCPSchemaType, description: str) -> MCPInputSchemaProperty: returns_json=True, ) + def _move_tool(self) -> ToolMetadata: + def prop(kind: cs.MCPSchemaType, description: str) -> MCPInputSchemaProperty: + return MCPInputSchemaProperty(type=kind, description=description) + + return ToolMetadata( + name=cs.MCPToolName.MOVE, + description=td.MCP_TOOLS[cs.MCPToolName.MOVE], + input_schema=MCPInputSchema( + type=cs.MCPSchemaType.OBJECT, + properties={ + cs.MCPParamName.QUALIFIED_NAME: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_QUALIFIED_NAME + ), + cs.MCPParamName.TARGET_MODULE: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_TARGET_MODULE + ), + cs.MCPParamName.KEEP_ALIAS: prop( + cs.MCPSchemaType.BOOLEAN, td.MCP_PARAM_KEEP_ALIAS + ), + cs.MCPParamName.DRY_RUN: prop( + cs.MCPSchemaType.BOOLEAN, td.MCP_PARAM_MOVE_DRY_RUN + ), + cs.MCPParamName.PROJECT: prop( + cs.MCPSchemaType.STRING, td.MCP_PARAM_PROJECT + ), + }, + required=[ + cs.MCPParamName.QUALIFIED_NAME, + cs.MCPParamName.TARGET_MODULE, + ], + ), + handler=self.move, + returns_json=True, + ) + + async def move( + self, + qualified_name: str, + target_module: str, + keep_alias: bool = False, + dry_run: bool = False, + project: str | None = None, + ) -> object: + return await self._graph_query( + cs.MCPToolName.MOVE, + project, + lambda name: self._run_move( + name, qualified_name, target_module, keep_alias, dry_run + ), + ) + + def _run_move( + self, + project_name: str, + qualified_name: str, + target_module: str, + keep_alias: bool, + dry_run: bool, + ) -> object: + from codebase_rag.editing.move import MoveRefused, move + + # The selected project's repo-relative paths name files only under the + # root it was indexed from; resolving them beneath this server's + # checkout would cut and rewrite another tree's files (issue #1542). + root = graph_query.source_root_for( + self.ingestor.fetch_all, project_name, Path(self.project_root) + ) + if root is None: + return { + cs.DICT_KEY_ERROR: cs.MOVE_WRONG_ROOT.format(project=project_name), + cs.KEY_CYCLE: [], + } + # As for `change_signature`: a preview never re-ingests, and an + # applied move that is refused before it re-ingests releases the + # mid-update mark its hydrated updater set. + reingest, release = ( + (None, None) if dry_run else self._signature_reingest(project_name) + ) + try: + report = move( + root, + self.ingestor.fetch_all, + project_name, + qualified_name, + target_module, + keep_alias=keep_alias, + dry_run=dry_run, + reingest=reingest, + ) + except MoveRefused as refused: + return {cs.DICT_KEY_ERROR: str(refused), cs.KEY_CYCLE: list(refused.cycle)} + finally: + if release is not None: + release() + payload_marker_error: str | None = None + if report.graph_incomplete: + # The same invalidation, in memory and durably, that + # `_run_rename` applies after a failed re-ingest. + self._live_updater = None + self._invalidate_graph_for(project_name) + payload_marker_error = self._require_marker(project_name, writing=True) + payload = dict(report._asdict()) + payload[cs.KEY_VERDICT] = report.verdict._asdict() if report.verdict else None + if payload_marker_error is not None: + payload[cs.DICT_KEY_ERROR] = payload_marker_error + return payload + async def rename( self, qualified_name: str, diff --git a/codebase_rag/structural_delta.py b/codebase_rag/structural_delta.py index 19aa285b1c..1d3ced9905 100644 --- a/codebase_rag/structural_delta.py +++ b/codebase_rag/structural_delta.py @@ -115,6 +115,10 @@ class Snapshot(NamedTuple): sites: tuple[CallSite, ...] imports: dict[str, frozenset[str]] module_paths: dict[str, str] + # (importer, target, imported name) per import statement; "" for a + # whole-module import. Tells an import of a name the module still binds + # (a constant the graph has no node for) from one of a moved symbol. + imported: frozenset[tuple[str, str, str]] = frozenset() bindings: tuple[ImportBinding, ...] = () modules: frozenset[tuple[str, str]] = frozenset() module_bindings: tuple[ImportBinding, ...] = () @@ -421,12 +425,14 @@ def snapshot( callees[definition.qualified_name] = definition imports: dict[str, set[str]] = {} module_paths: dict[str, str] = {} + imported: set[tuple[str, str, str]] = set() for row in fetch_all(cq.CYPHER_DELTA_MODULE_IMPORTS, params): source = _text(row.get(cs.KEY_FROM_QN)) target = _text(row.get(cs.KEY_TO_QN)) if not source or not target: continue imports.setdefault(source, set()).add(target) + imported.add((source, target, _text(row.get(cs.KEY_IMPORTED_NAME)))) imports.setdefault(target, set()) module_paths[source] = _text(row.get(cs.KEY_FROM_PATH)) return Snapshot( @@ -436,6 +442,7 @@ def snapshot( sites=sites, imports={qn: frozenset(targets) for qn, targets in imports.items()}, module_paths=module_paths, + imported=frozenset(imported), bindings=_named_import_bindings(fetch_all, params), modules=_touched_modules(fetch_all, params), module_bindings=_module_import_bindings(fetch_all, params), @@ -590,6 +597,7 @@ def _renames( before: Snapshot, after: Snapshot, declared: frozenset[tuple[str, str]] = frozenset(), + reshaped: frozenset[tuple[str, str]] = frozenset(), ) -> list[RenameFinding]: # A rename keeps the body: the same whole-skeleton fingerprint under a # new name in the same file. Paired one-to-one in sorted order so a @@ -624,6 +632,22 @@ def _renames( still_unpaired, renames, paired_new, added, before, after, declared ) ) + # Pass 5: a pair whose body the operation itself rewrote (a move that + # respelled a relative import for its new package) changed shape by + # design, so only the operation's word can pair it. + paired_old = {r["old"] for r in renames} + for old, new in sorted(reshaped): + if ( + old in still_unpaired + and old not in paired_old + and new in added + and new not in paired_new + and before.definitions[old].label == after.definitions[new].label + ): + renames.append( + RenameFinding(old=old, new=new, path=before.definitions[old].path) + ) + paired_new.add(new) return renames @@ -657,10 +681,11 @@ def _symbols( before: Snapshot, after: Snapshot, declared: frozenset[tuple[str, str]] = frozenset(), + reshaped: frozenset[tuple[str, str]] = frozenset(), ) -> SymbolDelta: added = sorted(set(after.definitions) - set(before.definitions)) removed = sorted(set(before.definitions) - set(after.definitions)) - renamed = _renames(removed, added, before, after, declared) + renamed = _renames(removed, added, before, after, declared, reshaped) renamed_old = {r["old"] for r in renamed} renamed_new = {r["new"] for r in renamed} return SymbolDelta( @@ -2456,6 +2481,14 @@ def _cycles(graph: dict[str, frozenset[str]]) -> set[frozenset[str]]: } +def import_cycles(graph: dict[str, frozenset[str]]) -> set[frozenset[str]]: + """The cyclic strongly connected components of a module import graph. + + `move` checks a planned move against these before touching a file. + """ + return _cycles(graph) + + def _new_import_cycles(before: Snapshot, after: Snapshot) -> list[list[str]]: touched = {qn for qn, path in after.module_paths.items() if path in after.paths} touched |= {qn for qn, path in before.module_paths.items() if path in before.paths} @@ -2613,12 +2646,15 @@ def structural_delta( declared_renames: frozenset[tuple[str, str]] = frozenset(), *, longer_project_prefixes: tuple[str, ...] | None = None, + reshaped_renames: frozenset[tuple[str, str]] = frozenset(), ) -> StructuralDelta: """Diff two snapshots of the same paths, then look up what they touch. `declared_renames` are pairs the CALLER applied and therefore knows. They are needed only where the snapshots cannot show identity -- an empty container -- and are empty for a plain write, which really is inferring. + `reshaped_renames` are pairs whose body the caller rewrote on purpose, + so their shapes no longer match. """ started = time.perf_counter() longer_prefixes = ( @@ -2626,7 +2662,7 @@ def structural_delta( if longer_project_prefixes is None else longer_project_prefixes ) - symbols = _symbols(before, after, declared_renames) + symbols = _symbols(before, after, declared_renames, reshaped_renames) fresh = set(symbols["added"]) | set(symbols["changed"]) fresh |= {r["new"] for r in symbols["renamed"]} # A re-parsed file was edited: every symbol it defines may behave @@ -2695,11 +2731,16 @@ def _stale_importers(after: Snapshot, symbols: SymbolDelta) -> list[StaleImporte report every importer of its remaining siblings. """ vacated: set[str] = set() + moved_names: dict[str, set[str]] = {} for renamed in symbols["renamed"]: - old_module = _module_of(str(renamed["old"])) + old_qn = str(renamed["old"]) + old_module = _module_of(old_qn) new_module = _module_of(str(renamed["new"])) if old_module and old_module != new_module: vacated.add(old_module) + moved_names.setdefault(old_module, set()).add( + old_qn.rsplit(cs.SEPARATOR_DOT, 1)[-1] + ) if not vacated: return [] # Anything the graph still defines under a vacated module means the module @@ -2715,7 +2756,10 @@ def _stale_importers(after: Snapshot, symbols: SymbolDelta) -> list[StaleImporte # One entry per IMPORTER, not per stale target: the finding is that # this module still points at somewhere the move emptied, and naming # the same importer once per vacated target would repeat it. - if not targets & vacated: + if not any( + _imports_what_left(after, importer, target, moved_names[target]) + for target in targets & vacated + ): continue stale.append( StaleImporter( @@ -2727,6 +2771,34 @@ def _stale_importers(after: Snapshot, symbols: SymbolDelta) -> list[StaleImporte return sorted(stale, key=lambda entry: (entry["path"], entry["importer"])) +def _imports_what_left( + after: Snapshot, importer: str, target: str, moved: set[str] +) -> bool: + """Whether `importer`'s import of the vacated `target` can reach a moved name. + + The graph has no node for a module constant, so a module left holding + only `TAX_RATE = 0.2` reads as vacated; the moved function's own + `from old import TAX_RATE` then failed the move it was carried for. + Only a named import of something that did NOT move is cleared; a + whole-module import, or one the graph recorded without names, may still + reach the moved symbol and stays stale. So does a wildcard (`from old + import *`, recorded as `*`), unless the vacated module imports every + moved name back for the star to carry. A `from pkg import util` binds + the module itself under its last segment, so `util.helper()` still + reaches the moved name through it and that import stays stale too + (bot review). + """ + names = { + name for (src, dst, name) in after.imported if src == importer and dst == target + } + if cs.IMPORTED_NAME_WILDCARD in names: + kept = {name for (src, _dst, name) in after.imported if src == target} + if not moved <= kept: + return True + leaf = target.rpartition(cs.SEPARATOR_DOT)[2] + return not names or "" in names or leaf in names or bool(names & moved) + + def observe( fetch_all: QueryFn, project_name: str, @@ -2734,6 +2806,7 @@ def observe( apply: Callable[[], ReingestReport], repo_root: Path | None = None, declared_renames: frozenset[tuple[str, str]] = frozenset(), + reshaped_renames: frozenset[tuple[str, str]] = frozenset(), ) -> StructuralDelta: """Snapshot `paths`, run `apply` (the scoped re-ingest), snapshot, diff. @@ -2767,6 +2840,7 @@ def observe( repo_root, declared_renames=declared_renames, longer_project_prefixes=longer_prefixes, + reshaped_renames=reshaped_renames, ) # The re-ingest's own clock covers only its inner work; the caller sees # the wall time of the whole apply step, and `delta_ms` is everything diff --git a/codebase_rag/tests/change_signature_support.py b/codebase_rag/tests/change_signature_support.py new file mode 100644 index 0000000000..52e956e482 --- /dev/null +++ b/codebase_rag/tests/change_signature_support.py @@ -0,0 +1,94 @@ +"""Shared fixture project and helpers for the change_signature tests +(issue #1533): test_change_signature*.py import these. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +from codebase_rag import constants as cs +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.parser_loader import load_parsers +from evals.cgr_graph import _StatefulIngestor + +PROJECT = "signature_fixture" +FIXTURE: dict[str, str] = { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a: int, b: str = 'x') -> str:\n return b * a\n", + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + "def run():\n return helper(2)\n\n\n" + "def run_kw():\n return helper(2, b='y')\n\n\n" + "def run_both():\n return helper(3, 'z')\n" + ), + "tests/__init__.py": "", + "tests/test_app.py": ( + "from pkg.app import run\n\n\ndef test_run():\n assert run() == 'xx'\n" + ), +} +HELPER = f"{PROJECT}.pkg.util.helper" + + +def _write(root: Path, rel: str, text: str) -> None: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + +def _index(root: Path) -> tuple[_StatefulIngestor, GraphUpdater]: + parsers, queries = load_parsers() + store = _StatefulIngestor() + updater = GraphUpdater( + ingestor=store, + repo_path=root, + parsers=parsers, + queries=queries, + project_name=PROJECT, + ) + updater.run(force=True) + return store, updater + + +def _project(temp_repo: Path, files: dict[str, str]) -> Path: + root = temp_repo / PROJECT + root.mkdir() + for rel, text in files.items(): + _write(root, rel, text) + return root + + +def indexed_fixture(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + """The FIXTURE project written under `temp_repo` and indexed.""" + root = _project(temp_repo, FIXTURE) + store, updater = _index(root) + return root, store, updater + + +def _read(root: Path, rel: str) -> str: + return (root / rel).read_text(encoding="utf-8") + + +def _smoke(root: Path, expression: str) -> None: + """Run the rewritten fixture for real: a rewrite that parses can still + be wrong, and only executing the call sites against the new definition + shows they agree.""" + result = subprocess.run( + [sys.executable, "-c", expression], + cwd=root, + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + assert result.returncode == 0, result.stderr + + +def _set_resolution(store: _StatefulIngestor, caller: str, resolution: str) -> None: + edge = next( + e + for e in store.edge_props + if e[1] == caller and e[2] == cs.RelationshipType.CALLS.value and e[4] == HELPER + ) + store.edge_props[edge][cs.KEY_RESOLUTION] = resolution diff --git a/codebase_rag/tests/test_change_signature.py b/codebase_rag/tests/test_change_signature.py new file mode 100644 index 0000000000..9643c9f5d1 --- /dev/null +++ b/codebase_rag/tests/test_change_signature.py @@ -0,0 +1,331 @@ +# Edit algebra op 2, change_signature (issue #1533): the definition (and its +# override hierarchy) gets the new parameter list, and every graph-known call +# site is rewritten per an explicit mapping from new parameter to old value. +# Sites the mapping cannot complete, and sites the graph resolved by +# guesswork, are left as written and listed as unmapped. The graph is the +# in-memory stateful ingestor over a real index of a fixture repo. +from __future__ import annotations + +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.signature import change_signature +from codebase_rag.editing.transaction import load_history +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.tests.change_signature_support import ( + FIXTURE, + HELPER, + PROJECT, + _index, + _project, + _read, + _set_resolution, + _smoke, + _write, + indexed_fixture, +) +from evals.cgr_graph import _StatefulIngestor + + +@pytest.fixture +def repo(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + return indexed_fixture(temp_repo) + + +# --- acceptance (issue #1533) -------------------------------------------------- + + +def test_add_required_parameter_with_a_default_mapping( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.old_params == ("a", "b") + assert report.new_params == ("a", "n", "b") + # The new parameter is REQUIRED in the definition: the literal is what + # existing sites pass, not a default the definition gains. + assert "def helper(a: int, n: int, b: str = 'x') -> str:" in _read( + root, "pkg/util.py" + ) + app = _read(root, "pkg/app.py") + assert "return helper(2, 1)" in app + assert "return helper(2, 1, b='y')" in app + assert "return helper(3, 1, 'z')" in app + assert report.unmapped == () + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + assert [t["qualified_name"] for t in report.verdict.affected_tests] == [ + f"{PROJECT}.tests.test_app.test_run" + ] + assert set(report.files) == {"pkg/util.py", "pkg/app.py"} + _smoke( + root, + "from pkg.app import run, run_kw, run_both\n" + "assert (run(), run_kw(), run_both()) == ('xx', 'yy', 'zzz')\n", + ) + + +def test_reorder_parameters_rewrites_positional_callers_only( + temp_repo: Path, +) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a: int, b: str) -> str:\n return b * a\n", + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + "def run():\n return helper(2, 'x')\n\n\n" + "def run_kw():\n return helper(a=2, b='y')\n" + ), + }, + ) + store, updater = _index(root) + before_kw = "return helper(a=2, b='y')" + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["b", "a"], + reingest=updater.reingest, + ) + assert report.applied, report.message + # A bare old name carries the old annotation and default over. + assert "def helper(b: str, a: int) -> str:" in _read(root, "pkg/util.py") + app = _read(root, "pkg/app.py") + assert "return helper('x', 2)" in app + # A keyword caller binds by name and is left exactly as written. + assert before_kw in app + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + _smoke( + root, + "from pkg.app import run, run_kw\nassert (run(), run_kw()) == ('xx', 'yy')", + ) + + +def test_heuristic_site_is_listed_as_unmapped_not_rewritten( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + _set_resolution( + store, f"{PROJECT}.pkg.app.run_both", cs.EdgeResolution.HEURISTIC.value + ) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + dry_run=True, + ) + (skipped,) = report.unmapped + assert skipped.owner == f"{PROJECT}.pkg.app.run_both" + assert skipped.path == "pkg/app.py" + assert cs.EdgeResolution.HEURISTIC.value in skipped.reason + assert [s.owner for s in report.sites if s.kind == "call"] == [ + f"{PROJECT}.pkg.app.run", + f"{PROJECT}.pkg.app.run_kw", + ] + # The diff carries the two rewritten sites and not the guessed one. + assert "+ return helper(2, 1)" in report.diff + assert "helper(3, 1, 'z')" not in report.diff + assert _read(root, "pkg/app.py") == FIXTURE["pkg/app.py"] + + +def test_allow_heuristic_rewrites_the_guessed_site( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + _set_resolution( + store, f"{PROJECT}.pkg.app.run_both", cs.EdgeResolution.HEURISTIC.value + ) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + allow_heuristic=True, + dry_run=True, + ) + assert report.unmapped == () + assert "helper(3, 1, 'z')" in report.diff + + +def test_allow_heuristic_applies_through_the_contract( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + # The contract refuses a rewritten guessed site unless the operation + # was told to allow it; the leave must reach the expectation. + root, store, updater = repo + _set_resolution( + store, f"{PROJECT}.pkg.app.run_both", cs.EdgeResolution.HEURISTIC.value + ) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + allow_heuristic=True, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + assert "return helper(3, 1, 'z')" in _read(root, "pkg/app.py") + + +# --- transaction and contract --------------------------------------------------------- + + +def test_dry_run_reports_the_diff_and_writes_nothing( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + dry_run=True, + ) + assert not report.applied + assert "--- a/pkg/util.py" in report.diff + assert "+def helper(a: int, n: int, b: str = 'x') -> str:" in report.diff + for rel, text in FIXTURE.items(): + assert _read(root, rel) == text + assert load_history(root) == [] + + +def test_the_applied_change_is_recorded_for_undo( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + report = change_signature( + root, store.fetch_all, PROJECT, HELPER, ["a", "n: int", "b"], {"n": "=1"} + ) + assert report.applied, report.message + assert report.verdict is None # no re-ingest, so no contract + (entry,) = load_history(root) + assert entry[cs.EDIT_KEY_ID] == report.transaction_id + + +def test_a_contract_failure_undoes_the_change( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + # A caller the graph never saw: the operation cannot rewrite it, and once + # the re-ingest brings it in, its site passes too few arguments for the + # new signature without being listed as unmapped. The contract catches + # it after the fact and the transaction is undone. + _write( + root, + "pkg/late.py", + "from pkg.util import helper\n\n\ndef late():\n return helper(2)\n", + ) + before = {rel: _read(root, rel) for rel in FIXTURE} + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + reingest=lambda paths: updater.reingest([*paths, "pkg/late.py"]), + ) + assert not report.applied + assert report.verdict is not None + assert not report.verdict.ok + assert "pkg/late.py:5" in report.message + for rel, text in before.items(): + assert _read(root, rel) == text + assert load_history(root) == [] + + +# --- MCP tool ---------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_mcp_change_signature_tool_runs_under_the_lock_and_reports( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + from unittest.mock import MagicMock + + from codebase_rag.mcp.tools import MCPToolsRegistry + + root, store, _updater = repo + ingestor = MagicMock() + ingestor.fetch_all = store.fetch_all + ingestor.list_projects.return_value = [PROJECT] + registry = MCPToolsRegistry( + project_root=str(root), ingestor=ingestor, cypher_gen=MagicMock() + ) + schema = next( + s + for s in registry.get_tool_schemas() + if s.name == cs.MCPToolName.CHANGE_SIGNATURE + ) + assert set(schema.inputSchema["required"]) == { + cs.MCPParamName.QUALIFIED_NAME, + cs.MCPParamName.NEW_PARAMS, + } + entry = registry.get_tool_handler(cs.MCPToolName.CHANGE_SIGNATURE) + assert entry is not None + payload = await entry[0]( + qualified_name=HELPER, + new_params=["a", "n: int", "b"], + mapping={"n": "=1"}, + dry_run=True, + project=PROJECT, + ) + assert isinstance(payload, dict) + assert payload["applied"] is False + assert payload[cs.KEY_SITES] + assert payload["unmapped"] == [] + assert "helper(2, 1)" in payload["diff"] + assert _read(root, "pkg/app.py") == FIXTURE["pkg/app.py"] + + +@pytest.mark.asyncio +async def test_mcp_change_signature_refusal_is_a_payload_not_an_exception( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + from unittest.mock import MagicMock + + from codebase_rag.mcp.tools import MCPToolsRegistry + + root, store, _updater = repo + ingestor = MagicMock() + ingestor.fetch_all = store.fetch_all + ingestor.list_projects.return_value = [PROJECT] + registry = MCPToolsRegistry( + project_root=str(root), ingestor=ingestor, cypher_gen=MagicMock() + ) + payload = await registry.change_signature( + qualified_name=HELPER, + new_params=["a", "n: int", "b"], + mapping={"n": "nothing"}, + project=PROJECT, + ) + assert isinstance(payload, dict) + assert cs.DICT_KEY_ERROR in payload + assert "nothing" in payload[cs.DICT_KEY_ERROR] diff --git a/codebase_rag/tests/test_change_signature_definition.py b/codebase_rag/tests/test_change_signature_definition.py new file mode 100644 index 0000000000..aca0fc90da --- /dev/null +++ b/codebase_rag/tests/test_change_signature_definition.py @@ -0,0 +1,365 @@ +# change_signature (issue #1533): the definition's new parameter list, and +# parameters renamed where the body reads them. +from __future__ import annotations + +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.signature import change_signature +from codebase_rag.editing.signature_spec import SignatureRefused +from codebase_rag.editing.transaction import load_history +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.tests.change_signature_support import ( + FIXTURE, + HELPER, + PROJECT, + _index, + _project, + _read, + _smoke, + indexed_fixture, +) +from evals.cgr_graph import _StatefulIngestor + + +@pytest.fixture +def repo(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + return indexed_fixture(temp_repo) + + +# --- the definition ------------------------------------------------------------------ + + +def test_default_literal_incompatible_with_the_declared_type_is_refused( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + with pytest.raises(SignatureRefused, match=r"does not fit"): + change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "='no'"}, + dry_run=True, + ) + # The old annotation counts too: `b: str` is carried over by the bare name. + with pytest.raises(SignatureRefused, match=r"does not fit"): + change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "b"], + {"b": "=3"}, + dry_run=True, + ) + # An optional annotation admits None; a non-literal is not checked. + for annotation, literal in ( + ("int | None", "=None"), + ("Optional[int]", "=None"), + ("int", "=LIMIT"), + ("Sequence[int]", "=()"), + ): + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", f"n: {annotation}", "b"], + {"n": literal}, + dry_run=True, + ) + assert report.unmapped == () + + +def test_a_kept_parameter_spelled_anew_takes_the_new_spelling( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + # `b` exists already; spelling it out again re-annotates it. Only the + # bare name carries the old spelling over. + root, store, _updater = repo + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "b: int = 0"], + dry_run=True, + ) + assert "+def helper(a: int, b: int = 0) -> str:" in report.diff + # Sites keep their values: the parameters did not move. + assert "helper(2, b='y')" not in report.diff + # The new annotation is the one a literal is checked against. + with pytest.raises(SignatureRefused, match=r"does not fit"): + change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "b: int = 0"], + {"b": "='q'"}, + dry_run=True, + ) + + +def test_a_required_parameter_after_a_default_is_refused( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + with pytest.raises(SignatureRefused, match=r"after"): + change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "b", "n: int"], + {"n": "=1"}, + ) + assert _read(root, "pkg/util.py") == FIXTURE["pkg/util.py"] + assert load_history(root) == [] + + +@pytest.mark.parametrize( + "header", + [ + "def helper(a, *rest):", + "def helper(a, **extra):", + "def helper(a, /, b):", + "def helper(a, *, b):", + ], +) +def test_a_definition_with_special_parameters_is_refused( + temp_repo: Path, header: str +) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": f"{header}\n return a\n", + "pkg/app.py": "from pkg.util import helper\n\n\ndef run():\n return helper(1)\n", + }, + ) + store, _updater = _index(root) + with pytest.raises(SignatureRefused, match=r"positional-or-keyword"): + change_signature( + root, store.fetch_all, PROJECT, HELPER, ["a", "n"], dry_run=True + ) + + +def test_a_non_python_definition_is_refused(temp_repo: Path) -> None: + root = _project( + temp_repo, + { + "lib/tool.js": "export function helper(a) { return a; }\n", + "lib/app.js": "import { helper } from './tool.js';\nexport const run = () => helper(1);\n", + }, + ) + store, _updater = _index(root) + with pytest.raises(SignatureRefused, match=r"Python"): + change_signature( + root, store.fetch_all, PROJECT, f"{PROJECT}.lib.tool.helper", ["a", "n"] + ) + + +def test_an_unknown_definition_is_refused( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + with pytest.raises(SignatureRefused, match=r"No definition"): + change_signature( + root, store.fetch_all, PROJECT, f"{PROJECT}.pkg.util.nothing", ["a"] + ) + + +# --- renamed parameters in the body ---------------------------------------------- + + +def _rename_dry_run(temp_repo: Path, body: str) -> str: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": f"def helper(a, b):\n{body}", + "pkg/app.py": "from pkg.util import helper\n\n\ndef run():\n return helper(2, 'x')\n", + }, + ) + store, _updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["times", "text"], + {"times": "a", "text": "b"}, + dry_run=True, + ) + return report.diff + + +def test_a_renamed_parameter_is_renamed_where_the_body_reads_it( + temp_repo: Path, +) -> None: + diff = _rename_dry_run( + temp_repo, + # A comprehension reads the parameter without re-binding it; an + # attribute and a keyword argument merely share its spelling. + " total = [i * a for i in range(len(b))]\n" + " a = a + 1\n" + " return b.a(a=a, text=b) or total\n", + ) + assert "+def helper(times, text):" in diff + assert "+ total = [i * times for i in range(len(text))]" in diff + assert "+ times = times + 1" in diff + assert "+ return text.a(a=times, text=text) or total" in diff + + +def test_swapping_two_parameter_names_renames_both_ways(temp_repo: Path) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a, b):\n return b * a\n", + "pkg/app.py": "from pkg.util import helper\n\n\ndef run():\n return helper(2, 'x')\n", + }, + ) + store, _updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["b", "a"], + {"b": "a", "a": "b"}, + dry_run=True, + ) + assert "+def helper(b, a):" in report.diff + assert "+ return a * b" in report.diff + # The values stay where they were: only the names moved. + assert "helper(2, 'x')" not in report.diff + + +@pytest.mark.parametrize( + "body", + [ + " def inner():\n return a\n return inner() * b\n", + " return (lambda: a)() * b\n", + " return [a for a in range(len(b))]\n", + " global a\n return b\n", + " from pkg import a\n return b\n", + ], +) +def test_a_parameter_used_in_a_nested_scope_refuses_the_rename( + temp_repo: Path, body: str +) -> None: + with pytest.raises(SignatureRefused, match=r"nested scope"): + _rename_dry_run(temp_repo, body) + + +@pytest.mark.parametrize( + "body", + [ + " text = 'q'\n return b * a + text\n", + # A nested scope reading the new name would start reading the + # renamed parameter instead of whatever it read before. + " def inner():\n return text\n return inner() * b + a\n", + " return (lambda: times)() * b + a\n", + ], +) +def test_a_new_name_already_used_in_the_body_is_refused( + temp_repo: Path, body: str +) -> None: + with pytest.raises(SignatureRefused, match=r"already used"): + _rename_dry_run(temp_repo, body) + + +def test_a_recursive_call_is_rewritten_with_its_renamed_arguments( + temp_repo: Path, +) -> None: + # The site's arguments contain the body rename: one edit, not two + # overlapping ones. + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": ( + "def helper(a):\n return helper(a - 1) + 1 if a else 0\n" + ), + "pkg/app.py": "from pkg.util import helper\n\n\ndef run():\n return helper(2)\n", + }, + ) + store, updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["times", "n: int"], + {"times": "a", "n": "=1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.unmapped == () + util = _read(root, "pkg/util.py") + assert "def helper(times, n: int):" in util + assert " return helper(times - 1, 1) + 1 if times else 0" in util + assert "return helper(2, 1)" in _read(root, "pkg/app.py") + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + _smoke(root, "from pkg.app import run\nassert run() == 2") + + +def test_a_call_nested_in_another_call_is_rewritten_inside_out( + temp_repo: Path, +) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a):\n return a + 1\n", + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + "def run():\n return helper(helper(1))\n" + ), + }, + ) + store, updater = _index(root) + # The eval store keys a CALLS edge on its endpoints, so it holds one + # site per caller; the real store keys on (line, col) and holds both. + # Present both sites the way the real store would. + run_qn = f"{PROJECT}.pkg.app.run" + stored = store.fetch_all + + def both_sites(query: str, params: dict[str, object] | None = None): # type: ignore[no-untyped-def] + rows = stored(query, params) + out = [] + for row in rows: + if row.get(cs.KEY_QUALIFIED_NAME) != run_qn: + out.append(row) + continue + for col in (11, 18): + site = dict(row) + site[cs.KEY_COL] = col + site[cs.KEY_END_LINE] = None + site[cs.KEY_END_COL] = None + out.append(site) + return out + + report = change_signature( + root, + both_sites, + PROJECT, + HELPER, + ["a", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.unmapped == () + assert sum(1 for s in report.sites if s.kind == "call") == 2 + assert "return helper(helper(1, 1), 1)" in _read(root, "pkg/app.py") + # The body still adds one: helper(helper(1, 1), 1) is (1 + 1) + 1. + _smoke(root, "from pkg.app import run\nassert run() == 3") diff --git a/codebase_rag/tests/test_change_signature_hierarchy.py b/codebase_rag/tests/test_change_signature_hierarchy.py new file mode 100644 index 0000000000..979d97b11f --- /dev/null +++ b/codebase_rag/tests/test_change_signature_hierarchy.py @@ -0,0 +1,471 @@ +# change_signature (issue #1533): override hierarchies, receivers, static +# methods, and the guards on defaults and dropped parameters. +from __future__ import annotations + +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.signature import change_signature +from codebase_rag.editing.signature_spec import SignatureRefused +from codebase_rag.tests.change_signature_support import ( + PROJECT, + _index, + _project, + _read, + _smoke, +) + +# --- hierarchies --------------------------------------------------------------------- + + +SHAPES: dict[str, str] = { + "pkg/__init__.py": "", + "pkg/shapes.py": ( + "class Base:\n" + " def area(self, scale):\n" + " return 0 * scale\n\n\n" + "class Circle(Base):\n" + " def area(self, scale):\n" + " return 3 * scale\n\n\n" + "def total(shape: Base):\n" + " return shape.area(2) + Circle().area(scale=3)\n" + ), +} + + +def test_a_method_hierarchy_is_rewritten_together(temp_repo: Path) -> None: + root = _project(temp_repo, SHAPES) + store, updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.shapes.Base.area", + ["scale", "unit: str"], + {"unit": "='m'"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert set(report.hierarchy) == { + f"{PROJECT}.pkg.shapes.Base.area", + f"{PROJECT}.pkg.shapes.Circle.area", + } + shapes = _read(root, "pkg/shapes.py") + assert shapes.count("def area(self, scale, unit: str):") == 2 + assert "shape.area(2, 'm') + Circle().area(scale=3, unit='m')" in shapes + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + _smoke(root, "from pkg.shapes import total, Circle\nassert total(Circle()) == 15") + + +def test_an_override_with_different_parameters_is_refused(temp_repo: Path) -> None: + files = dict(SHAPES) + files["pkg/shapes.py"] = files["pkg/shapes.py"].replace( + " def area(self, scale):\n return 3 * scale", + " def area(self, factor):\n return 3 * factor", + ) + root = _project(temp_repo, files) + store, _updater = _index(root) + with pytest.raises(SignatureRefused, match=r"Circle\.area"): + change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.shapes.Base.area", + ["scale", "unit: str"], + {"unit": "='m'"}, + ) + assert _read(root, "pkg/shapes.py") == files["pkg/shapes.py"] + + +def test_a_method_whose_receiver_is_not_self_or_cls_is_refused( + temp_repo: Path, +) -> None: + # `this` would be remapped as a parameter and every bound call would + # lose its receiver. + files = dict(SHAPES) + files["pkg/shapes.py"] = files["pkg/shapes.py"].replace( + "def area(self, scale):", "def area(this, scale):" + ) + root = _project(temp_repo, files) + store, _updater = _index(root) + with pytest.raises(SignatureRefused, match=r"`this`.*self or cls"): + change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.shapes.Base.area", + ["scale", "unit: str"], + {"unit": "='m'"}, + ) + assert _read(root, "pkg/shapes.py") == files["pkg/shapes.py"] + + +@pytest.mark.parametrize("decorator", ["staticmethod", "builtins.staticmethod"]) +def test_a_static_method_has_no_receiver(temp_repo: Path, decorator: str) -> None: + # The indexer reads a decorator by its last name, so the operation must + # recognise the qualified spelling too. + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/shapes.py": ( + "import builtins\n\n\n" + "class K:\n" + f" @{decorator}\n" + " def helper(a):\n" + " return a\n\n\n" + "def on_class():\n" + " return K.helper(2)\n\n\n" + "def on_instance():\n" + " return K().helper(3)\n" + ), + }, + ) + store, updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.shapes.K.helper", + ["a", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.unmapped == () + shapes = _read(root, "pkg/shapes.py") + assert " def helper(a, n: int):" in shapes + assert "return K.helper(2, 1)" in shapes + assert "return K().helper(3, 1)" in shapes + _smoke( + root, + "from pkg.shapes import on_class, on_instance\n" + "assert on_class() + on_instance() == 5", + ) + + +def test_a_static_methods_self_is_an_ordinary_parameter(temp_repo: Path) -> None: + """`@staticmethod def f(self, x)` is legal and `self` is NOT a receiver. + + The receiver was popped by NAME before staticness was decided, so + `self` was treated as the receiver AND re-added from `new_params`, + writing `def helper(self, self, a, n: int)` -- a duplicate parameter, + and a SyntaxError in the file the tool had just edited (Copilot, + #1533). Deciding staticness first makes the operation refuse and roll + back instead, leaving the source untouched. + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/shapes.py": ( + "class K:\n" + " @staticmethod\n" + " def helper(self, a):\n" + " return self + a\n\n\n" + "def on_class():\n" + " return K.helper(1, 2)\n" + ), + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.shapes.K.helper", + ["self", "a", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + + # The call-site binding still counts a receiver for a static method, so + # the operation cannot map `K.helper(1, 2)` and ROLLS BACK. That is the + # correct outcome: before the reorder it wrote + # `def helper(self, self, a, n: int)` -- a duplicate parameter, and a + # SyntaxError in the file it had just edited. + assert not report.applied + assert "too_many" in report.message, report.message + shapes = _read(root, "pkg/shapes.py") + assert "def helper(self, self" not in shapes, shapes + assert " def helper(self, a):" in shapes, shapes + + +def test_a_bare_generator_argument_is_unmapped_not_rewritten( + temp_repo: Path, +) -> None: + """`helper(x for x in xs)` passes a GENERATOR, not a list of values. + + tree-sitter gives the call a `generator_expression` as its arguments + node rather than an `argument_list`, so binding it positionally would + rewrite a generator as if it were an ordinary value and change what + the call means. It must be listed unmapped instead (Copilot, #1533). + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": ( + "def helper(a):\n" + " return sum(a)\n\n\n" + "def run(xs):\n" + " return helper(x for x in xs)\n" + ), + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.helper", + ["a", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + + (skipped,) = report.unmapped + assert skipped.path == "pkg/app.py" + # The REASON matters, not just the refusal: without the argument-list + # check the call is still skipped, but by an arity miscount that counts + # the generator's own children as two positional arguments. That is the + # right answer for the wrong reason, and it would stop being right for + # a one-parameter generator call. + assert cs.SIGNATURE_SITE_UNREADABLE.split("{")[0] in skipped.reason, skipped.reason + # The generator survives verbatim: it was never bound as a value. + assert "helper(x for x in xs)" in _read(root, "pkg/app.py") + + +def test_a_default_reading_a_renamed_parameter_refuses(temp_repo: Path) -> None: + """`def f(a, b=a)` renamed a->x would write `def f(x, b=a)`. + + A default is evaluated at DEFINITION time in the enclosing scope, so + that file raises NameError the moment it is imported -- and it PARSES, + so the syntax postcondition cannot catch it. `_body_references` walks + the body only and never saw the reference (Copilot, #1533). + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": "def f(a, b=a):\n return b\n", + }, + ) + store, updater = _index(root) + + with pytest.raises(SignatureRefused, match="evaluated at definition time"): + change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.f", + ["x", "b=a"], + {"x": "a"}, + reingest=updater.reingest, + ) + + assert "def f(a, b=a):" in _read(root, "pkg/app.py") + + +def test_a_default_not_reading_the_renamed_parameter_still_renames( + temp_repo: Path, +) -> None: + """The control: a default that does NOT read the renamed name is fine, + so the refusal cannot be satisfied by refusing every default.""" + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": "def f(a, b=1):\n return a + b\n", + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.f", + ["x", "b=1"], + {"x": "a"}, + reingest=updater.reingest, + ) + + assert report.applied, report.message + assert "def f(x, b=1):" in _read(root, "pkg/app.py") + + +def test_an_explicit_spelling_re_annotates_every_override( + temp_repo: Path, +) -> None: + """Spelling a kept parameter out again re-annotates it everywhere. + + `carried` was inferred by comparing each new spec with the primary's, + and a bare name resolves TO the primary's spec -- so an explicit + `a: int` that matched the primary compared equal, counted as carried, + and left an override declaring `a: str` untouched (Copilot, #1533). + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": ( + "class Base:\n def f(self, a: int) -> int:\n return a\n\n\n" + "class Sub(Base):\n def f(self, a: str) -> int:\n" + " return len(a)\n" + ), + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.Base.f", + ["a: int", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + + assert report.applied, report.message + app = _read(root, "pkg/app.py") + assert "def f(self, a: int, n: int) -> int:" in app, app + assert "a: str" not in app, app + + +def test_a_bare_name_keeps_each_overrides_own_spelling(temp_repo: Path) -> None: + """The control: a BARE name carries each override's own annotation, so + the fix above cannot be satisfied by re-annotating unconditionally.""" + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": ( + "class Base:\n def f(self, a: int) -> int:\n return a\n\n\n" + "class Sub(Base):\n def f(self, a: str) -> int:\n" + " return len(a)\n" + ), + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.Base.f", + ["a", "n: int"], + {"n": "=1"}, + reingest=updater.reingest, + ) + + assert report.applied, report.message + app = _read(root, "pkg/app.py") + assert "def f(self, a: int, n: int) -> int:" in app, app + assert "def f(self, a: str, n: int) -> int:" in app, app + + +def test_dropping_a_parameter_the_body_reads_refuses(temp_repo: Path) -> None: + """`def f(a): return a` emptied writes `def f(): return a`. + + That PARSES, so the syntax postcondition passes, and the arity + contract only checks call sites -- the NameError surfaces when the + function is finally called (Copilot, #1533). + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": "def f(a):\n return a\n\n\ndef call():\n return f(1)\n", + }, + ) + store, updater = _index(root) + + with pytest.raises(SignatureRefused, match="still reads"): + change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.f", + [], + None, + reingest=updater.reingest, + ) + + assert "def f(a):" in _read(root, "pkg/app.py") + + +def test_dropping_a_parameter_the_body_ignores_still_applies( + temp_repo: Path, +) -> None: + """The control: a parameter the body never reads drops cleanly, so the + refusal cannot be satisfied by refusing every removal.""" + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": "def f(a):\n return 1\n\n\ndef call():\n return f(2)\n", + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.f", + [], + None, + reingest=updater.reingest, + ) + + assert report.applied, report.message + app = _read(root, "pkg/app.py") + assert "def f():" in app, app + assert "return f()" in app, app + + +@pytest.mark.parametrize( + "body", + [ + "def f(a):\n return obj.a\n", + "def f(a):\n return helper(a=1)\n", + ], + ids=["attribute-name", "keyword-label"], +) +def test_a_same_named_attribute_or_keyword_is_not_a_read( + temp_repo: Path, body: str +) -> None: + """`obj.a` and `helper(a=1)` spell `a` without reading the parameter. + + The drop guard matched any identifier, so both refused a removal that + is perfectly legal -- the guard has to use the same `_is_a_reference` + test the body rewrite does (CodeRabbit, #1533). + """ + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/app.py": body + "\n\ndef call():\n return f(1)\n", + }, + ) + store, updater = _index(root) + + report = change_signature( + root, + store.fetch_all, + PROJECT, + f"{PROJECT}.pkg.app.f", + [], + None, + reingest=updater.reingest, + ) + + assert report.applied, report.message + assert "def f():" in _read(root, "pkg/app.py") diff --git a/codebase_rag/tests/test_change_signature_mapping.py b/codebase_rag/tests/test_change_signature_mapping.py new file mode 100644 index 0000000000..f2566f2841 --- /dev/null +++ b/codebase_rag/tests/test_change_signature_mapping.py @@ -0,0 +1,358 @@ +# change_signature (issue #1533): how the mapping sources each new parameter +# at a call site, and the sites it cannot complete, which are listed. +from __future__ import annotations + +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.signature import change_signature +from codebase_rag.editing.signature_spec import SignatureRefused, parse_mapping +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.tests.change_signature_support import ( + FIXTURE, + HELPER, + PROJECT, + _index, + _project, + _read, + _smoke, + indexed_fixture, +) +from evals.cgr_graph import _StatefulIngestor + + +@pytest.fixture +def repo(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + return indexed_fixture(temp_repo) + + +# --- the mapping ------------------------------------------------------------------- + + +def test_a_parameter_renamed_by_index_renames_keyword_sites(temp_repo: Path) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a: int, b: str) -> str:\n return b * a\n", + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + "def run():\n return helper(2, 'x')\n\n\n" + "def run_kw():\n return helper(b='y', a=2)\n" + ), + }, + ) + store, updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["times: int", "text: str"], + {"times": "0", "text": "1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + util = _read(root, "pkg/util.py") + assert "def helper(times: int, text: str) -> str:" in util + # The body follows the rename, or the definition would be broken. + assert " return text * times\n" in util + app = _read(root, "pkg/app.py") + assert "return helper(2, 'x')" in app + # Keyword values keep their order and form; only the names follow. + assert "return helper(text='y', times=2)" in app + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + _smoke( + root, + "from pkg.app import run, run_kw\nassert (run(), run_kw()) == ('xx', 'yy')", + ) + + +def test_a_value_after_an_omitted_default_is_spelled_by_keyword( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "b", "n: int = 0"], + {"n": "=1"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + app = _read(root, "pkg/app.py") + # `helper(2)` relied on b's default; the value for n cannot sit in b's + # slot, so it goes by keyword. + assert "return helper(2, n=1)" in app + assert "return helper(2, b='y', n=1)" in app + assert "return helper(3, 'z', 1)" in app + _smoke( + root, + "from pkg.app import run, run_kw, run_both\n" + "assert (run(), run_kw(), run_both()) == ('xx', 'yy', 'zzz')\n", + ) + + +def test_an_unmapped_required_parameter_leaves_every_site_and_lists_it( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + reingest=updater.reingest, + ) + assert report.applied, report.message + assert "def helper(a: int, n: int, b: str = 'x') -> str:" in _read( + root, "pkg/util.py" + ) + assert _read(root, "pkg/app.py") == FIXTURE["pkg/app.py"] + assert sorted((u.path, u.line) for u in report.unmapped) == [ + ("pkg/app.py", 5), + ("pkg/app.py", 9), + ("pkg/app.py", 13), + ] + assert all("`n`" in u.reason for u in report.unmapped) + # Listed sites satisfy the contract: the operation said it left them. + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + + +def _strip_location(store: _StatefulIngestor, caller: str) -> None: + edge = next( + e + for e in store.edge_props + if e[1] == caller and e[2] == cs.RelationshipType.CALLS.value and e[4] == HELPER + ) + props = store.edge_props[edge] + props[cs.KEY_RESOLUTION] = cs.EdgeResolution.DYNAMIC.value + for key in (cs.KEY_LINE, cs.KEY_COL, cs.KEY_END_LINE, cs.KEY_END_COL): + props.pop(key, None) + + +def test_a_site_whose_recorded_end_matches_no_call_is_listed( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + # A stale end position must not fall back to another call sharing the + # start: the site is listed rather than the wrong call rewritten. + root, store, _updater = repo + edge = next( + e + for e in store.edge_props + if e[1] == f"{PROJECT}.pkg.app.run_kw" + and e[2] == cs.RelationshipType.CALLS.value + and e[4] == HELPER + ) + store.edge_props[edge][cs.KEY_END_COL] = 99 + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + dry_run=True, + ) + (skipped,) = report.unmapped + assert skipped.owner == f"{PROJECT}.pkg.app.run_kw" + assert "no call" in skipped.reason + assert "helper(2, b='y')" not in report.diff + assert "+ return helper(3, 1, 'z')" in report.diff + + +def test_chained_calls_sharing_a_start_are_both_rewritten(temp_repo: Path) -> None: + # `B().area(2).area(3)`: both calls start where `B` does and differ only + # in where they end. The graph records both; neither may be dropped. + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/shapes.py": ( + "class B:\n" + " def area(self, scale):\n" + " return self\n\n\n" + "def run():\n" + " return B().area(2).area(3)\n" + ), + }, + ) + store, updater = _index(root) + run_qn = f"{PROJECT}.pkg.shapes.run" + stored = store.fetch_all + + def both_links(query: str, params: dict[str, object] | None = None): # type: ignore[no-untyped-def] + out = [] + for row in stored(query, params): + if row.get(cs.KEY_QUALIFIED_NAME) != run_qn: + out.append(row) + continue + for end_col in (22, 30): + link = dict(row) + link[cs.KEY_LINE] = 7 + link[cs.KEY_COL] = 11 + link[cs.KEY_END_LINE] = 7 + link[cs.KEY_END_COL] = end_col + out.append(link) + return out + + report = change_signature( + root, + both_links, + PROJECT, + f"{PROJECT}.pkg.shapes.B.area", + ["scale", "unit: str"], + {"unit": "='m'"}, + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.unmapped == () + assert sum(1 for s in report.sites if s.kind == "call") == 2 + assert "return B().area(2, 'm').area(3, 'm')" in _read(root, "pkg/shapes.py") + _smoke(root, "from pkg.shapes import run, B\nassert isinstance(run(), B)") + + +def test_every_site_without_a_location_is_listed( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, _updater = repo + _strip_location(store, f"{PROJECT}.pkg.app.run") + _strip_location(store, f"{PROJECT}.pkg.app.run_kw") + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + dry_run=True, + ) + assert sorted(u.owner for u in report.unmapped) == [ + f"{PROJECT}.pkg.app.run", + f"{PROJECT}.pkg.app.run_kw", + ] + assert all("no location" in u.reason for u in report.unmapped) + assert "+ return helper(3, 1, 'z')" in report.diff + + +def test_a_site_passing_surplus_arguments_is_listed_not_truncated( + temp_repo: Path, +) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": "def helper(a, b):\n return a + b\n", + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + "def run():\n return helper(1, 2)\n\n\n" + "def broken():\n return helper(1, 2, 3)\n" + ), + }, + ) + store, updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["b", "a"], + reingest=updater.reingest, + ) + assert report.applied, report.message + app = _read(root, "pkg/app.py") + assert "return helper(2, 1)" in app + assert "return helper(1, 2, 3)" in app + (skipped,) = report.unmapped + assert (skipped.path, skipped.line) == ("pkg/app.py", 9) + assert "3" in skipped.reason + assert report.verdict is not None, report.message + assert report.verdict.ok, report.message + + +@pytest.mark.parametrize( + ("site", "fragment"), + [ + ("helper(*args)", "*args"), + ("helper(2, **opts)", "**opts"), + ("helper(2, c=1)", "c"), + ], +) +def test_a_site_the_mapping_cannot_read_is_listed( + temp_repo: Path, site: str, fragment: str +) -> None: + root = _project( + temp_repo, + { + "pkg/__init__.py": "", + "pkg/util.py": FIXTURE["pkg/util.py"], + "pkg/app.py": ( + "from pkg.util import helper\n\n\n" + f"def run(args=(), opts={{}}):\n return {site}\n" + ), + }, + ) + store, _updater = _index(root) + report = change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + {"n": "=1"}, + dry_run=True, + ) + (skipped,) = report.unmapped + assert (skipped.path, skipped.line) == ("pkg/app.py", 5) + assert fragment in skipped.reason + assert site not in report.diff + + +@pytest.mark.parametrize( + ("mapping", "reason"), + [ + ({"n": "c"}, r"neither an old parameter"), # no old parameter c + ({"n": "2"}, r"only 2 old parameter"), # only two old parameters + ({"m": "=1"}, r"not a new parameter"), # no new parameter m + # One old value cannot feed two parameters. Matched by reason: with + # the duplicate check removed this case was still refused, by the + # body check seeing `a` already in use, so a bare `raises` could + # not tell the two apart. + ({"a": "b", "b": "b"}, r"cannot feed both"), + ], +) +def test_a_mapping_that_names_nothing_is_refused( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], + mapping: dict[str, str], + reason: str, +) -> None: + root, store, _updater = repo + with pytest.raises(SignatureRefused, match=reason): + change_signature( + root, + store.fetch_all, + PROJECT, + HELPER, + ["a", "n: int", "b"], + mapping, + dry_run=True, + ) + assert _read(root, "pkg/util.py") == FIXTURE["pkg/util.py"] + + +def test_parse_mapping_text_forms() -> None: + assert parse_mapping(["n==1", "a=0", "b=old_b", "s=='x'"]) == { + "n": "=1", + "a": "0", + "b": "old_b", + "s": "='x'", + } + with pytest.raises(SignatureRefused): + parse_mapping(["n"]) diff --git a/codebase_rag/tests/test_edit_contract.py b/codebase_rag/tests/test_edit_contract.py index d95c8fae90..e0a4fdebec 100644 --- a/codebase_rag/tests/test_edit_contract.py +++ b/codebase_rag/tests/test_edit_contract.py @@ -1412,6 +1412,28 @@ def test_an_importer_of_a_vacated_module_is_reported() -> None: assert [entry["importer"] for entry in stale] == ["p.pkg.app"] +def test_a_from_import_of_the_vacated_module_itself_is_reported() -> None: + """`from pkg import util` is recorded against `pkg.util` under the name + `util`, which is not a moved name, so it was cleared like an import of a + constant left behind -- while `util.helper()` raised AttributeError + after the move (bot review).""" + from codebase_rag.structural_delta import _stale_importers + + base = _snapshot( + definitions={"p.pkg.core.helper": "pkg/core.py"}, + imports={"p.pkg.app": {"p.pkg.util"}}, + ) + module = base._replace(imported=frozenset({("p.pkg.app", "p.pkg.util", "util")})) + constant = base._replace(imported=frozenset({("p.pkg.app", "p.pkg.util", "SEP")})) + moved = _renamed("p.pkg.util.helper", "p.pkg.core.helper") + + assert [entry["importer"] for entry in _stale_importers(module, moved)] == [ + "p.pkg.app" + ] + # The control: a named import of a constant the module kept is not stale. + assert _stale_importers(constant, moved) == [] + + def test_a_module_that_still_defines_something_is_not_vacated() -> None: """Moving one helper out of a busy module must not indict its siblings. diff --git a/codebase_rag/tests/test_move_cli.py b/codebase_rag/tests/test_move_cli.py new file mode 100644 index 0000000000..6447c87bb6 --- /dev/null +++ b/codebase_rag/tests/test_move_cli.py @@ -0,0 +1,120 @@ +# `cgr move` (issue #1534): argument forwarding, the JSON report, the exit +# status, the refusal path, and the guard that keeps an explicit --project +# from editing a checkout it was not indexed from. + +from __future__ import annotations + +import json +import shutil +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +import typer +from typer.testing import CliRunner + +from codebase_rag import cli_help as ch +from codebase_rag import constants as cs +from codebase_rag.cli import app +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.tests.test_move_op import FIXTURE, PROJECT, _index, _materialise +from evals.cgr_graph import _StatefulIngestor + + +@pytest.fixture +def indexed(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + return root, store, updater + + +def _invoke( + store: _StatefulIngestor, updater: GraphUpdater, repo: Path, *args: str +) -> tuple[int, str, str]: + with ( + patch( + "codebase_rag.graph_cli._project_and_fetch", + return_value=(PROJECT, store.fetch_all, MagicMock()), + ), + patch("codebase_rag.graph_updater.GraphUpdater", return_value=updater), + ): + result = CliRunner().invoke( + app, ["move", *args, "--repo-path", str(repo), "--project", PROJECT] + ) + return result.exit_code, result.stdout, result.stderr + + +def test_cli_move_dry_run_reports_and_writes_nothing( + indexed: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = indexed + code, out, err = _invoke( + store, updater, root, f"{PROJECT}.pkg.util.helper", "pkg.core", "--dry-run" + ) + assert code == 0, err + payload = json.loads(out) + assert payload["applied"] is False + assert payload["new_qualified_name"] == f"{PROJECT}.pkg.core.helper" + assert payload["new_path"] == "pkg/core.py" + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + assert not (root / "pkg/core.py").exists() + + +def test_cli_move_applies_and_reports_the_verdict( + indexed: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = indexed + code, out, err = _invoke( + store, updater, root, f"{PROJECT}.pkg.util.helper", "pkg.core", "--keep-alias" + ) + assert code == 0, err + payload = json.loads(out) + assert payload["applied"] is True + assert payload[cs.KEY_VERDICT]["ok"] is True + assert "def helper" in (root / "pkg/core.py").read_text() + assert "from pkg.core import helper" in (root / "pkg/util.py").read_text() + + +def test_cli_move_refusal_exits_nonzero_on_stderr( + indexed: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = indexed + code, out, err = _invoke( + store, updater, root, f"{PROJECT}.pkg.util.helper", "pkg.util" + ) + assert code == 1 + assert out == "" + assert "already holds" in err + + +def test_cli_move_refuses_a_project_indexed_from_another_checkout( + indexed: tuple[Path, _StatefulIngestor, GraphUpdater], temp_repo: Path +) -> None: + root, store, updater = indexed + other = temp_repo / "other_checkout" + shutil.copytree(root, other) + before = {rel: (other / rel).read_text() for rel in FIXTURE} + + code, out, err = _invoke( + store, updater, other, f"{PROJECT}.pkg.util.helper", "pkg.core" + ) + + assert code == 1 + assert out == "" + assert cs.MOVE_CLI_WRONG_ROOT.format(project=PROJECT, root=other.resolve()) in err + for rel, text in before.items(): + assert (other / rel).read_text() == text + assert not (other / "pkg/core.py").exists() + + +def test_cli_move_help_is_move_specific() -> None: + command = typer.main.get_command(app) + # Duck-typed: typer may vendor its own click (#1409). + assert hasattr(command, "commands") + move = command.commands[ch.CLICommandName.MOVE] + helps = {param.name: getattr(param, "help", None) for param in move.params} + assert helps["qualified_name"] == ch.HELP_MOVE_QN + assert helps["dry_run"] == ch.HELP_MOVE_DRY_RUN + # Moving is not renaming, and a move plan stages no diff to print. + assert "rename" not in ch.HELP_MOVE_QN + assert "diff" not in ch.HELP_MOVE_DRY_RUN diff --git a/codebase_rag/tests/test_move_helpers_review.py b/codebase_rag/tests/test_move_helpers_review.py new file mode 100644 index 0000000000..376992a12d --- /dev/null +++ b/codebase_rag/tests/test_move_helpers_review.py @@ -0,0 +1,289 @@ +"""The module-level helpers `move` builds its imports from. + +Each case here is a way the moved code, or the file it came from, ended +up with the wrong imports while still parsing -- so the transaction's +parse gate let it through and the damage surfaced at runtime: + +- an import nested in a function counted as binding the module, so the + top-level import the rewritten call sites need was never added; +- a relative `from .x import y` was copied verbatim into a file in + another package, where the same dots name a different module; +- a default or namespace JS binding was dropped, or dragged its + siblings along, and a TS inline `type` entry was never matched; +- `import a, b` was copied whole, binding `b` in the destination; +- `.tsx` took the Python path; +- an import added to a file without imports landed above the module + docstring, shebang, `from __future__` line or "use strict" directive. +""" + +from __future__ import annotations + +import importlib +import sys +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.move import ( + _JS_LANGUAGES, + _import_block_end, + _module_bound, + _narrow_statement, +) +from codebase_rag.parser_loader import load_parsers + +PY = cs.SupportedLanguage.PYTHON +JS = cs.SupportedLanguage.JS +TS = cs.SupportedLanguage.TS +TSX = cs.SupportedLanguage.TSX + + +# --- _module_bound: only module-level imports bind the module -------------------------- + + +@pytest.mark.parametrize( + "source", + [ + pytest.param("def f():\n import pkg.new\n", id="in-function"), + pytest.param("class C:\n import pkg.new\n", id="in-class"), + pytest.param( + "def f():\n if True:\n import pkg.new\n", id="in-nested-block" + ), + ], +) +def test_a_nested_import_does_not_bind_the_module(source: str) -> None: + assert _module_bound(source, "pkg.new") is False + + +def test_a_top_level_import_next_to_a_nested_one_still_binds() -> None: + source = "import pkg.new\n\ndef f():\n import other\n" + assert _module_bound(source, "pkg.new") is True + + +# --- _narrow_statement, Python --------------------------------------------------------- + + +@pytest.mark.parametrize( + ("statement", "alias", "old_path", "new_path", "expected"), + [ + pytest.param( + "from .alpha import value", + "value", + "app/src.py", + "app/dst/mod.py", + "from ..alpha import value", + id="into-subpackage", + ), + pytest.param( + "from .deps import X", + "X", + "pkg/sub/util.py", + "pkg/core.py", + "from .sub.deps import X", + id="into-parent-package", + ), + pytest.param( + "from . import helpers", + "helpers", + "p/x.py", + "p/y.py", + "from . import helpers", + id="same-package-unchanged", + ), + pytest.param( + "from ..shared import tool", + "tool", + "pkg/a/mod.py", + "pkg/b/mod.py", + "from ..shared import tool", + id="sibling-package-same-depth", + ), + pytest.param( + "from pkg.deps import X", + "X", + "pkg/sub/util.py", + "pkg/core.py", + "from pkg.deps import X", + id="absolute-unchanged", + ), + ], +) +def test_a_relative_from_import_is_respelled_for_the_destination( + statement: str, alias: str, old_path: str, new_path: str, expected: str +) -> None: + assert _narrow_statement(statement, alias, PY, old_path, new_path) == expected + + +def test_the_respelled_relative_import_resolves_to_the_original_module( + tmp_path: Path, +) -> None: + """The interpreter, not a string comparison, is the judge of `..`.""" + (tmp_path / "pkg" / "sub").mkdir(parents=True) + (tmp_path / "pkg" / "__init__.py").write_text("") + (tmp_path / "pkg" / "sub" / "__init__.py").write_text("") + (tmp_path / "pkg" / "sub" / "deps.py").write_text("X = 'sub.deps'\n") + # A decoy the verbatim copy would have resolved to. + (tmp_path / "pkg" / "deps.py").write_text("X = 'decoy'\n") + line = _narrow_statement( + "from .deps import X", "X", PY, "pkg/sub/util.py", "pkg/core.py" + ) + assert line is not None + (tmp_path / "pkg" / "core.py").write_text(line + "\n") + sys.path.insert(0, str(tmp_path)) + try: + assert importlib.import_module("pkg.core").X == "sub.deps" + finally: + sys.path.remove(str(tmp_path)) + for name in [m for m in sys.modules if m == "pkg" or m.startswith("pkg.")]: + del sys.modules[name] + + +@pytest.mark.parametrize( + ("statement", "alias", "expected"), + [ + pytest.param("import a, b", "a", "import a", id="first-of-two"), + pytest.param("import a, b", "b", "import b", id="second-of-two"), + pytest.param( + "import pkg.dep, other", "pkg.dep", "import pkg.dep", id="dotted-by-path" + ), + pytest.param( + "import pkg.dep, other", "pkg", "import pkg.dep", id="dotted-by-root" + ), + pytest.param("import x as y, z", "y", "import x as y", id="aliased-entry"), + pytest.param("import solo", "solo", "import solo", id="single-unchanged"), + ], +) +def test_a_plain_import_is_narrowed_to_the_needed_entry( + statement: str, alias: str, expected: str +) -> None: + assert _narrow_statement(statement, alias, PY, "a/x.py", "a/y.py") == expected + + +# --- _narrow_statement, JS/TS ---------------------------------------------------------- + + +@pytest.mark.parametrize( + ("statement", "alias", "expected"), + [ + pytest.param( + 'import client, { helper } from "./api";', + "client", + 'import client from "./api";', + id="default-of-mixed", + ), + pytest.param( + 'import client, { helper } from "./api";', + "helper", + 'import { helper } from "./api";', + id="named-of-mixed-drops-default", + ), + pytest.param( + 'import D, * as ns from "./api";', + "D", + 'import D from "./api";', + id="default-of-default-and-namespace", + ), + pytest.param( + 'import D, * as ns from "./api";', + "ns", + 'import * as ns from "./api";', + id="namespace-of-default-and-namespace", + ), + pytest.param( + "import { type Foo, Bar } from './api';", + "Foo", + "import { type Foo } from './api';", + id="inline-type-entry", + ), + pytest.param( + "import { type Foo as F, Bar } from './api';", + "F", + "import { type Foo as F } from './api';", + id="inline-type-entry-aliased", + ), + pytest.param( + "import type { Foo, Bar } from './api';", + "Foo", + "import type { Foo } from './api';", + id="type-only-statement-keeps-type", + ), + pytest.param( + "import { a, b as c } from './api'", + "c", + "import { b as c } from './api'", + id="plain-named-no-semicolon", + ), + ], +) +def test_a_js_import_keeps_exactly_the_needed_binding( + statement: str, alias: str, expected: str +) -> None: + assert _narrow_statement(statement, alias, TS, "src/x.ts", "src/y.ts") == expected + + +def test_tsx_is_handled_as_javascript() -> None: + assert TSX in _JS_LANGUAGES + assert ( + _narrow_statement( + "import { a, b } from './m';", "a", TSX, "src/x.tsx", "src/ui/y.tsx" + ) + == "import { a } from '../m';" + ) + + +# --- _import_block_end: never above the prologue -------------------------------------- + + +def _end(language: cs.SupportedLanguage, source: bytes) -> int: + root = load_parsers()[0][language].parse(source).root_node + return _import_block_end(source, root) + + +@pytest.mark.parametrize( + ("language", "prologue", "body"), + [ + pytest.param(PY, b'"""Doc."""\n', b"\nX = 1\n", id="py-docstring"), + pytest.param( + PY, + b'#!/usr/bin/env python\n# -*- coding: utf-8 -*-\n"""Doc."""\n', + b"X = 1\n", + id="py-shebang-encoding-docstring", + ), + pytest.param(PY, b"#!/usr/bin/env python\n", b"X = 1\n", id="py-shebang"), + pytest.param( + PY, + b'"""Doc."""\nfrom __future__ import annotations\n', + b"X = 1\n", + id="py-future", + ), + pytest.param( + JS, b'#!/usr/bin/env node\n"use strict";\n', b"const x = 1;\n", id="js" + ), + ], +) +def test_an_import_is_never_placed_above_the_prologue( + language: cs.SupportedLanguage, prologue: bytes, body: bytes +) -> None: + assert _end(language, prologue + body) == len(prologue) + + +@pytest.mark.parametrize( + ("language", "source"), + [ + pytest.param(PY, b"X = 1\n", id="py-no-prologue"), + pytest.param(PY, b"", id="py-empty"), + pytest.param(JS, b"const x = 1;\n", id="js-no-prologue"), + # A string that is not the first statement is not a docstring. + pytest.param(PY, b'X = 1\n"""not a docstring"""\n', id="py-late-string"), + ], +) +def test_without_a_prologue_the_top_of_the_file_is_still_right( + language: cs.SupportedLanguage, source: bytes +) -> None: + assert _end(language, source) == 0 + + +def test_existing_imports_still_decide_the_insertion_point() -> None: + source = b'"""Doc."""\nimport os\nimport sys\n\nX = 1\n' + assert _end(PY, source) == source.index(b"import sys\n") + len(b"import sys\n") diff --git a/codebase_rag/tests/test_move_import_presence.py b/codebase_rag/tests/test_move_import_presence.py new file mode 100644 index 0000000000..f058ad3d5e --- /dev/null +++ b/codebase_rag/tests/test_move_import_presence.py @@ -0,0 +1,50 @@ +"""`move` must not skip an import because the text appears somewhere. + +The move rewrites call sites to `pkg.new.helper(...)` and then adds +`import pkg.new` unless it is already there. "Already there" was decided +by a regex over the raw file, so an aliased import, a comment or a string +literal all counted -- and the import was never added while the call +sites had already been rewritten. Both files parse, so a parse-based +postcondition cannot catch it; the program dies at runtime with +`NameError: name 'pkg' is not defined` on the line the move just wrote. +""" + +from __future__ import annotations + +import pytest + +from codebase_rag.editing.move import _module_bound + +BINDS = [ + pytest.param("import pkg.new\n", id="plain"), + # `import pkg.new.sub` binds `pkg`, so `pkg.new.helper()` resolves. + # Verified against a real interpreter: it must stay True. + pytest.param("import pkg.new.sub\n", id="submodule-still-binds"), +] + +DOES_NOT_BIND = [ + pytest.param("import pkg.new as n\nn.helper()\n", id="aliased"), + pytest.param("# TODO: import pkg.new later\n", id="comment"), + pytest.param('DOC = "import pkg.new"\n', id="string-literal"), + pytest.param("helper()\n", id="absent"), + pytest.param("from pkg import new\n", id="from-import-binds-new-not-pkg"), +] + + +@pytest.mark.parametrize("source", BINDS) +def test_the_module_is_bound(source: str) -> None: + assert _module_bound(source, "pkg.new") is True + + +@pytest.mark.parametrize("source", DOES_NOT_BIND) +def test_the_module_is_not_bound(source: str) -> None: + assert _module_bound(source, "pkg.new") is False + + +def test_an_alias_does_not_count_as_the_import() -> None: + """The case that breaks working code, spelled out. + + `import pkg.new as n` binds only `n`. Treating it as the import means + the move emits `pkg.new.helper()` with `pkg` unbound. + """ + assert _module_bound("import pkg.new as n\n", "pkg.new") is False diff --git a/codebase_rag/tests/test_move_mcp.py b/codebase_rag/tests/test_move_mcp.py new file mode 100644 index 0000000000..244e111caf --- /dev/null +++ b/codebase_rag/tests/test_move_mcp.py @@ -0,0 +1,122 @@ +# The MCP `move` tool (issue #1534): driven through the handler the registry +# serves, and held to the root guard `rename` and `change_signature` apply, +# so a project indexed from another checkout never has its repo-relative +# paths edited beneath this server's repository. + +from __future__ import annotations + +import re +import shutil +from pathlib import Path +from unittest.mock import MagicMock + +from codebase_rag import constants as cs +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.mcp.tools import MCPToolsRegistry +from codebase_rag.readme_sections import format_mcp_tools_table +from codebase_rag.tests.test_move_op import FIXTURE, PROJECT, _index, _materialise +from codebase_rag.tools import tool_descriptions as td +from evals.cgr_graph import _StatefulIngestor + +_DOCS = Path(__file__).resolve().parents[2] / "docs" / "guide" / "mcp-server.md" + + +def _qn(rel: str) -> str: + return f"{PROJECT}.{rel}" + + +def _registry( + root: Path, store: _StatefulIngestor, updater: GraphUpdater +) -> MCPToolsRegistry: + ingestor = MagicMock() + ingestor.fetch_all = store.fetch_all + ingestor.list_projects.return_value = [PROJECT] + registry = MCPToolsRegistry( + project_root=str(root), ingestor=ingestor, cypher_gen=MagicMock() + ) + registry._live_updater = updater + return registry + + +async def test_the_served_move_handler_refuses_and_applies_as_json( + temp_repo: Path, +) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + entry = _registry(root, store, updater).get_tool_handler(cs.MCPToolName.MOVE) + assert entry is not None + handler, returns_json = entry + assert returns_json is True + + refused = await handler( + qualified_name=_qn("pkg.util.helper"), target_module="pkg.util", project=PROJECT + ) + assert isinstance(refused, dict) + assert "already holds" in refused[cs.DICT_KEY_ERROR] + assert refused[cs.KEY_CYCLE] == [] + + payload = await handler( + qualified_name=_qn("pkg.util.helper"), target_module="pkg.core", project=PROJECT + ) + assert isinstance(payload, dict) + assert payload["applied"] is True, payload + assert payload["new_qualified_name"] == _qn("pkg.core.helper") + assert payload[cs.KEY_VERDICT]["ok"] is True + assert (root / "pkg/core.py").exists() + + +async def test_move_refuses_a_project_indexed_from_another_checkout( + temp_repo: Path, +) -> None: + # The graph project was indexed from `indexed`; this server is rooted at + # a sibling checkout holding the same repo-relative paths. Editing there + # would cut and rewrite files the graph never described. + indexed = _materialise(temp_repo, FIXTURE) + store, updater = _index(indexed) + served = temp_repo / "other_checkout" + shutil.copytree(indexed, served) + before = {rel: (served / rel).read_text() for rel in FIXTURE} + + entry = _registry(served, store, updater).get_tool_handler(cs.MCPToolName.MOVE) + assert entry is not None + payload = await entry[0]( + qualified_name=_qn("pkg.util.helper"), target_module="pkg.core", project=PROJECT + ) + + assert isinstance(payload, dict) + assert payload[cs.DICT_KEY_ERROR] == cs.MOVE_WRONG_ROOT.format(project=PROJECT) + assert payload[cs.KEY_CYCLE] == [] + for rel, text in before.items(): + assert (served / rel).read_text() == text + assert not (served / "pkg/core.py").exists() + assert not (indexed / "pkg/core.py").exists() + + +def test_move_dry_run_parameter_does_not_promise_a_diff() -> None: + # A move plan reports files, importers and copied imports; it stages no + # diff, so the schema must not advertise one. + registry = MCPToolsRegistry( + project_root=".", ingestor=MagicMock(), cypher_gen=MagicMock() + ) + schema = next( + s for s in registry.get_tool_schemas() if s.name == cs.MCPToolName.MOVE + ) + described = schema.inputSchema["properties"][cs.MCPParamName.DRY_RUN] + assert described["description"] == td.MCP_PARAM_MOVE_DRY_RUN + assert "diff" not in described["description"] + + +def test_mcp_guide_tools_table_is_the_generated_one() -> None: + # A stale copy of a row survived a regeneration once (`query_code_graph` + # listed twice with contradicting contracts); the checked-in section + # must be exactly what the generator renders. + text = _DOCS.read_text(encoding="utf-8") + match = re.search( + r"\n(.*?)\n", + text, + re.DOTALL, + ) + assert match is not None + assert match.group(1) == format_mcp_tools_table() + names = re.findall(r"^\| `(\w+)` \|", match.group(1), re.MULTILINE) + assert len(names) == len(set(names)) diff --git a/codebase_rag/tests/test_move_op.py b/codebase_rag/tests/test_move_op.py new file mode 100644 index 0000000000..46a1408b6b --- /dev/null +++ b/codebase_rag/tests/test_move_op.py @@ -0,0 +1,308 @@ +# move (issue #1534): a definition is cut from its module with the imports +# it needs and pasted into the target, every importer is rewritten, the old +# module keeps an import (or a re-export with keep_alias), and a move that +# would create an import cycle is refused before any file is touched. + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.move import MoveRefused, move +from codebase_rag.editing.transaction import load_history +from codebase_rag.graph_updater import GraphUpdater +from codebase_rag.parser_loader import load_parsers +from evals.cgr_graph import _StatefulIngestor + +PROJECT = "move_fixture" +FIXTURE: dict[str, str] = { + "pkg/__init__.py": "", + "pkg/util.py": ( + "import os\n" + "from pathlib import Path\n\n\n" + "def other():\n return Path('.')\n\n\n" + "# joins with the platform separator\n" + 'def helper(a):\n """Join."""\n return os.sep.join(a)\n\n\n' + "def unrelated():\n return 1\n" + ), + "pkg/a.py": "from pkg.util import helper\n\n\ndef run():\n return helper(['x', 'y'])\n", + "pkg/b.py": "from pkg.util import helper, other\n\n\ndef go():\n return helper([]) + str(other())\n", + "pkg/c.py": "import pkg.util\n\n\ndef via_module():\n return pkg.util.helper(['q'])\n", + "tests/__init__.py": "", + "tests/test_app.py": ( + "import os\n\nfrom pkg.a import run\n\n\ndef test_run():\n assert run() == 'x' + os.sep + 'y'\n" + ), +} + + +def _write(root: Path, rel: str, text: str) -> None: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def _index(root: Path) -> tuple[_StatefulIngestor, GraphUpdater]: + parsers, queries = load_parsers() + store = _StatefulIngestor() + updater = GraphUpdater( + ingestor=store, + repo_path=root, + parsers=parsers, + queries=queries, + project_name=PROJECT, + ) + updater.run(force=True) + return store, updater + + +def _materialise(temp_repo: Path, fixture: dict[str, str]) -> Path: + root = temp_repo / PROJECT + root.mkdir() + for rel, text in fixture.items(): + _write(root, rel, text) + return root + + +@pytest.fixture +def repo(temp_repo: Path) -> tuple[Path, _StatefulIngestor, GraphUpdater]: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + return root, store, updater + + +def _qn(rel: str) -> str: + return f"{PROJECT}.{rel}" + + +def _smoke(root: Path) -> None: + subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", "tests"], + cwd=root, + check=True, + capture_output=True, + ) + + +def test_move_updates_three_importers_without_a_cycle( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + report = move( + root, + store.fetch_all, + PROJECT, + _qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + assert report.new_qualified_name == _qn("pkg.core.helper") + core = (root / "pkg/core.py").read_text() + # The definition travelled with its comment, docstring and the one + # import it needs; the unrelated import stayed behind. + assert core == ( + "import os\n\n\n" + "# joins with the platform separator\n" + 'def helper(a):\n """Join."""\n return os.sep.join(a)\n' + ) + util = (root / "pkg/util.py").read_text() + assert "def helper" not in util + assert "def other" in util + assert "def unrelated" in util + assert "from pathlib import Path" in util + assert (root / "pkg/a.py").read_text().startswith("from pkg.core import helper\n") + assert ( + (root / "pkg/b.py") + .read_text() + .startswith("from pkg.util import other\nfrom pkg.core import helper\n") + ) + assert set(report.importers) == {"pkg/a.py", "pkg/b.py"} + # `pkg.util.helper(...)` through a module import is not an import of + # the symbol: it is left alone and named. + assert report.unchanged_importers == () + assert report.copied_imports == ("import os",) + assert report.verdict is not None + assert report.verdict.ok + assert [t["qualified_name"] for t in report.verdict.affected_tests] == [ + _qn("tests.test_app.test_run") + ] + _smoke(root) + + +def test_keep_alias_leaves_a_working_re_export( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + report = move( + root, + store.fetch_all, + PROJECT, + _qn("pkg.util.helper"), + "pkg/core.py", + keep_alias=True, + reingest=updater.reingest, + ) + assert report.applied, report.message + util = (root / "pkg/util.py").read_text() + assert "from pkg.core import helper # noqa: F401" in util + # The old import path still works, and so does the module-attribute use. + probe = subprocess.run( + [ + sys.executable, + "-c", + "from pkg.util import helper; import pkg.c; print(helper(['a', 'b']), pkg.c.via_module())", + ], + cwd=root, + check=True, + capture_output=True, + encoding=cs.ENCODING_UTF8, + ) + assert "q" in probe.stdout + _smoke(root) + + +def test_move_that_would_create_a_cycle_is_refused_before_writing( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + # helper needs `other` from util, and util's own code still calls helper: + # after the move util imports core and core imports util. + fixture["pkg/util.py"] = ( + "import os\n\n\n" + "def other():\n return 'o'\n\n\n" + "def helper(a):\n return other() + os.sep.join(a)\n\n\n" + "def run():\n return helper(['x'])\n" + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + before = {rel: (root / rel).read_text() for rel in fixture} + helper = _qn("pkg.util.helper") + with pytest.raises(MoveRefused) as excinfo: + move( + root, + store.fetch_all, + PROJECT, + helper, + "pkg.core", + reingest=updater.reingest, + ) + assert excinfo.value.cycle == (_qn("pkg.core"), _qn("pkg.util")) + assert "import cycle" in str(excinfo.value) + for rel, text in before.items(): + assert (root / rel).read_text() == text + assert not (root / "pkg/core.py").exists() + assert load_history(root) == [] + + +def test_move_into_an_existing_module_appends( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + root, store, updater = repo + _write(root, "pkg/core.py", "VERSION = 1\n") + updater.reingest(["pkg/core.py"]) + report = move( + root, + store.fetch_all, + PROJECT, + _qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert core.startswith("VERSION = 1\n\n\nimport os\n\n\n# joins") + _smoke(root) + + +def test_old_module_that_still_uses_the_name_imports_it(temp_repo: Path) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + FIXTURE["pkg/util.py"] + "\n\ndef run():\n return helper(['x'])\n" + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = move( + root, + store.fetch_all, + PROJECT, + _qn("pkg.util.helper"), + "pkg.core", + reingest=updater.reingest, + ) + assert report.applied, report.message + util = (root / "pkg/util.py").read_text() + assert util.startswith( + "import os\nfrom pathlib import Path\nfrom pkg.core import helper\n" + ) + assert "def run():\n return helper(['x'])" in util + _smoke(root) + + +def test_refusals(repo: tuple[Path, _StatefulIngestor, GraphUpdater]) -> None: + root, store, _updater = repo + nothing = _qn("pkg.util.nothing") + helper = _qn("pkg.util.helper") + with pytest.raises(MoveRefused, match="No definition"): + move( + root, + store.fetch_all, + PROJECT, + nothing, + "pkg.core", + dry_run=True, + ) + with pytest.raises(MoveRefused, match="already holds"): + move( + root, + store.fetch_all, + PROJECT, + helper, + "pkg.util", + dry_run=True, + ) + report = move( + root, store.fetch_all, PROJECT, _qn("pkg.util.helper"), "pkg.core", dry_run=True + ) + assert not report.applied + assert report.new_path == "pkg/core.py" + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + + +async def test_mcp_move_tool_reports_and_refuses( + repo: tuple[Path, _StatefulIngestor, GraphUpdater], +) -> None: + from unittest.mock import MagicMock + + from codebase_rag.mcp.tools import MCPToolsRegistry + + root, store, updater = repo + ingestor = MagicMock() + ingestor.fetch_all = store.fetch_all + ingestor.list_projects.return_value = [PROJECT] + registry = MCPToolsRegistry( + project_root=str(root), ingestor=ingestor, cypher_gen=MagicMock() + ) + registry._live_updater = updater + schema = next( + s for s in registry.get_tool_schemas() if s.name == cs.MCPToolName.MOVE + ) + assert set(schema.inputSchema["required"]) == { + cs.MCPParamName.QUALIFIED_NAME, + cs.MCPParamName.TARGET_MODULE, + } + refused = await registry.move( + qualified_name=_qn("pkg.util.helper"), target_module="pkg.util", project=PROJECT + ) + assert isinstance(refused, dict) + assert cs.DICT_KEY_ERROR in refused + payload = await registry.move( + qualified_name=_qn("pkg.util.helper"), target_module="pkg.core", project=PROJECT + ) + assert isinstance(payload, dict) + assert payload["applied"] is True + assert payload["new_qualified_name"] == _qn("pkg.core.helper") + assert payload[cs.KEY_VERDICT]["ok"] is True diff --git a/codebase_rag/tests/test_move_preservation.py b/codebase_rag/tests/test_move_preservation.py new file mode 100644 index 0000000000..d62e413fc7 --- /dev/null +++ b/codebase_rag/tests/test_move_preservation.py @@ -0,0 +1,812 @@ +# move (issue #1534) review findings from PR #2907: each test is a move that +# used to commit while changing what the program does -- a name rebound, a +# constant lost, an optional import made required, an export dropped, or a +# concurrent edit overwritten. + +from __future__ import annotations + +import importlib +import shutil +import subprocess +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.move import MoveRefused +from codebase_rag.editing.transaction import load_history +from codebase_rag.tests.test_move_op import ( + FIXTURE, + PROJECT, + _index, + _materialise, + _write, +) +from codebase_rag.tests.test_move_safety import OTHER, _move, _python + +move_mod = importlib.import_module("codebase_rag.editing.move") + + +# --- the transaction's baseline is what the move was planned from ---------------- + + +def _edit_after_planning( + monkeypatch: pytest.MonkeyPatch, path: Path, text: str +) -> None: + """Write `text` to `path` once `plan` has read the tree, before staging.""" + plan = move_mod.Mover.plan + + def plan_then_edit(self, *args, **kwargs): + planned = plan(self, *args, **kwargs) + path.write_text(text) + return planned + + monkeypatch.setattr(move_mod.Mover, "plan", plan_then_edit) + + +def test_an_edit_made_after_planning_refuses_the_move( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`stage` records the CURRENT bytes as the baseline, so a file edited + between planning and staging was overwritten with content built from + the bytes the plan read, and the edit was lost without a conflict.""" + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + edited = FIXTURE["pkg/a.py"] + "\n\ndef later():\n return 'kept'\n" + _edit_after_planning(monkeypatch, root / "pkg/a.py", edited) + with pytest.raises(MoveRefused, match="pkg/a.py changed on disk"): + _move(root, store, updater) + assert (root / "pkg/a.py").read_text() == edited + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + assert not (root / "pkg/core.py").exists() + assert load_history(root) == [] + + +def test_a_destination_created_after_planning_refuses_the_move( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The plan found no destination, so it wrote the whole file; one that + appeared in the meantime was replaced by it.""" + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + created = "VERSION = 2\n" + _edit_after_planning(monkeypatch, root / "pkg/core.py", created) + with pytest.raises(MoveRefused, match="pkg/core.py changed on disk"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == created + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + assert load_history(root) == [] + + +# --- imports stay in the scope they were written in ------------------------------- + + +def test_an_import_inside_the_moved_function_is_not_copied_to_the_top( + temp_repo: Path, +) -> None: + """The import table holds function-local imports too, and every one + whose name the moved text mentions was copied to the destination's top + level: an import the function ran only when asked became one the + module ran on load.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "def helper(enabled):\n" + " if enabled:\n" + " import optional_backend\n\n" + " return optional_backend.run()\n" + " return 'off'\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert report.copied_imports == () + core = (root / "pkg/core.py").read_text() + assert core.startswith("def helper(enabled):\n") + assert " import optional_backend\n" in core + probe = _python(root, "from pkg.core import helper; print(helper(False))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "off" + + +# --- names bound under module-level control flow ---------------------------------- + + +@pytest.mark.parametrize( + ("prelude", "call", "expected"), + [ + pytest.param( + "import sys\n\nif sys.maxsize > 0:\n SEP = '-'\nelse:\n SEP = '+'\n", + "helper(['a', 'b'])", + "a-b", + id="if-else", + ), + pytest.param( + "try:\n import optional_backend as SEP\n" + "except ImportError:\n SEP = '-'\n", + "helper(['a', 'b'])", + "a-b", + id="try-except-import", + ), + ], +) +def test_a_name_bound_in_both_branches_travels_with_the_function( + temp_repo: Path, prelude: str, call: str, expected: str +) -> None: + """Only top-level simple statements counted as module bindings, so a + constant set in both branches of an `if` (or by a `try` and its handler) + was left behind and the moved helper died with a NameError. The `try` + form also copied its guarded import unconditionally.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + prelude + "\n\ndef helper(a):\n return SEP.join(a)\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert report.copied_imports == () + assert "from pkg.util import SEP\n" in (root / "pkg/core.py").read_text() + probe = _python(root, f"from pkg.core import helper; print({call})") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == expected + + +@pytest.mark.parametrize( + "prelude", + [ + pytest.param( + "import os\n\nif os.environ.get('UNSET'):\n SEP = '-'\n", id="if" + ), + pytest.param("for SEP in ['-']:\n pass\n", id="for"), + pytest.param( + "try:\n import optional_backend as SEP\nexcept ImportError:\n pass\n", + id="try", + ), + ], +) +def test_a_name_that_may_be_unbound_refuses_the_move( + temp_repo: Path, prelude: str +) -> None: + """`from pkg.util import SEP` at the destination would fail on load + wherever the old module left SEP unbound, where before only a call + reaching it failed: the move cannot carry it, so it refuses.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + prelude + "\n\ndef helper(a):\n return SEP.join(a)\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="SEP"): + _move(root, store, updater) + assert (root / "pkg/util.py").read_text() == fixture["pkg/util.py"] + assert not (root / "pkg/core.py").exists() + + +def test_a_local_name_that_a_module_loop_also_binds_does_not_refuse( + temp_repo: Path, +) -> None: + """Only names the moved code reads from the module scope count: its own + `i` is a local, whatever a top-level loop binds.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "TOTAL = 0\nfor i in range(3):\n TOTAL += i\n\n\n" + "def helper(a):\n return [i for i in a]\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python(root, "from pkg.core import helper; print(helper(['a']))") + assert probe.returncode == 0, probe.stderr + + +# --- what the move adds must not rebind a destination name ------------------------ + +RATE_CORE = "RATE = 0.1\n\n\ndef rate():\n return RATE\n" +RATE_HELPER = "\n\ndef helper(a):\n return a * RATE\n" + OTHER + + +@pytest.mark.parametrize( + ("util", "core", "extra"), + [ + pytest.param( + "RATE = 0.5\n" + RATE_HELPER, RATE_CORE, {}, id="old-module-constant" + ), + pytest.param( + "from pkg.cfg import RATE\n" + RATE_HELPER, + RATE_CORE, + {"pkg/cfg.py": "RATE = 0.5\n"}, + id="copied-import", + ), + pytest.param( + "RATE = 0.5\n" + RATE_HELPER, + "from pkg.cfg import RATE\n\n\ndef rate():\n return RATE\n", + {"pkg/cfg.py": "RATE = 0.1\n"}, + id="destination-import", + ), + ], +) +def test_an_import_that_would_rebind_a_destination_name_refuses_the_move( + temp_repo: Path, util: str, core: str, extra: dict[str, str] +) -> None: + """Only the moved name was checked against the destination, so the + `from pkg.util import RATE` pasted for the helper silently replaced the + destination's own RATE, and its `rate()` started answering 0.5.""" + fixture = {**FIXTURE, "pkg/util.py": util, "pkg/core.py": core, **extra} + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="already binds RATE"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == core + assert (root / "pkg/util.py").read_text() == util + probe = _python(root, "from pkg.core import rate; print(rate())") + assert probe.stdout.strip() == "0.1", probe.stderr + + +def test_an_import_the_destination_already_has_is_not_a_collision( + temp_repo: Path, +) -> None: + """Binding a name to what it is already bound to replaces nothing.""" + core = "import os\n\n\ndef sep():\n return os.sep\n" + fixture = {**FIXTURE, "pkg/core.py": core} + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python( + root, "from pkg.core import helper, sep; print(helper(['a']), sep())" + ) + assert probe.returncode == 0, probe.stderr + + +# --- JavaScript: the moved name stays exported where it was ----------------------- + +NODE = shutil.which("node") +# The move writes extensionless specifiers (`./core`), as a bundler takes +# them; this hook resolves them the same way so plain node can load them. +RESOLVE_HOOK = """export async function resolve(specifier, context, next) { + try { + return await next(specifier, context); + } catch (error) { + if (!specifier.startsWith('.')) throw error; + return next(`${specifier}.js`, context); + } +} +""" +REGISTER_HOOK = ( + "import { register } from 'node:module';\n" + "register('./hooks.mjs', import.meta.url);\n" +) +needs_node = pytest.mark.skipif(NODE is None, reason="node is not installed") + + +def _node(root: Path, hooks: Path, code: str) -> subprocess.CompletedProcess[str]: + assert NODE is not None + (root / "package.json").write_text('{"type": "module"}\n') + (hooks / "hooks.mjs").write_text(RESOLVE_HOOK) + (hooks / "register.mjs").write_text(REGISTER_HOOK) + (root / "probe.mjs").write_text(code) + return subprocess.run( + [NODE, "--import", (hooks / "register.mjs").as_uri(), "probe.mjs"], + cwd=root, + check=False, + capture_output=True, + encoding=cs.ENCODING_UTF8, + ) + + +@needs_node +def test_keep_alias_still_exports_a_js_name_the_old_module_uses( + temp_repo: Path, tmp_path: Path +) -> None: + """With the old module still calling the name, `keep_alias` wrote only + the local `import { helper }`, so the old path stopped exporting it.""" + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "export function helper(a) {\n return a + 1;\n}\n\n" + "export function run() {\n return helper(1);\n}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js", keep_alias=True) + assert report.applied, report.message + probe = _node( + root, + tmp_path, + "import { helper, run } from './pkg/util.js';\nconsole.log(helper(2), run());\n", + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "3 2" + + +@needs_node +@pytest.mark.parametrize("keep_alias", [False, True], ids=["moved", "kept"]) +def test_a_js_export_list_entry_follows_the_moved_definition( + temp_repo: Path, tmp_path: Path, keep_alias: bool +) -> None: + """`export { helper }` is its own statement, and the cut took only the + declaration: the destination did not export `helper`, so the old + module's import of it back, and every rewritten importer, failed to + link.""" + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "function helper(a) {\n return a + 1;\n}\n\n" + "function other() {\n return 'o';\n}\n\n" + "export function run() {\n return helper(1);\n}\n\n" + "export { helper, other };\n", + ) + _write( + root, + "pkg/a.js", + "import { helper } from './util';\n\n" + "export function go() {\n return helper(2);\n}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js", keep_alias=keep_alias) + assert report.applied, report.message + util = (root / "pkg/util.js").read_text() + assert ("helper, other" in util) is keep_alias + probe = _node( + root, + tmp_path, + "import { go } from './pkg/a.js';\n" + "import { run, other } from './pkg/util.js';\n" + "import { helper } from './pkg/core.js';\n" + + ("import { helper as kept } from './pkg/util.js';\n" if keep_alias else "") + + "console.log(go(), run(), other(), helper(0));\n", + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "3 2 o 1" + + +# --- the cut takes the definition, not the code sharing its lines ----------------- + + +@needs_node +def test_a_statement_after_the_definition_on_its_last_line_stays( + temp_repo: Path, tmp_path: Path +) -> None: + """The cut took whole lines, so a statement sharing the definition's + last line was deleted from the old module (and pasted at the + destination, where it ran on the wrong module's load).""" + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "export function helper(x) { return x + 1; } console.log('ready');\n\n" + "export function run() {\n return helper(1);\n}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js") + assert report.applied, report.message + assert "console.log" not in (root / "pkg/core.js").read_text() + probe = _node( + root, tmp_path, "import { run } from './pkg/util.js';\nconsole.log(run());\n" + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.split() == ["ready", "2"] + + +@needs_node +def test_a_statement_before_the_definition_on_its_first_line_stays( + temp_repo: Path, tmp_path: Path +) -> None: + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "export const SEP = '-'; export function helper(a) {\n" + " return a.join(SEP);\n}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js") + assert report.applied, report.message + assert "export const SEP = '-';" in (root / "pkg/util.js").read_text() + probe = _node( + root, + tmp_path, + "import { helper } from './pkg/core.js';\n" + "import { SEP } from './pkg/util.js';\n" + "console.log(helper(['a', 'b']), SEP);\n", + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "a-b -" + + +# --- imports for type checkers only stay guarded ---------------------------------- + +# A module only a type checker may import: loading it at runtime fails. +HEAVY = ( + "raise ImportError('pkg.heavy is for type checkers only')\n\n\n" + "class Model:\n pass\n\n\nclass Other:\n pass\n" +) +GUARDED_HELPER = "\n\ndef helper(m: Model) -> str:\n return 'ok'\n" + OTHER + + +@pytest.mark.parametrize( + ("typing_import", "guard"), + [ + pytest.param("from typing import TYPE_CHECKING", "TYPE_CHECKING", id="name"), + pytest.param("import typing", "typing.TYPE_CHECKING", id="attribute"), + ], +) +def test_a_type_checking_import_moves_under_the_guard( + temp_repo: Path, typing_import: str, guard: str +) -> None: + """A name imported under `if TYPE_CHECKING:` is bound for the type + checker only. Copying the import unconditionally made the destination + load a module that must not be loaded at runtime; refusing it refused + the commonest annotation pattern there is.""" + fixture = dict(FIXTURE) + fixture["pkg/heavy.py"] = HEAVY + fixture["pkg/util.py"] = ( + f"from __future__ import annotations\n\n{typing_import}\n\n" + f"if {guard}:\n from pkg.heavy import Model\n" + GUARDED_HELPER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert "from typing import TYPE_CHECKING\n" in core + assert "if TYPE_CHECKING:\n from pkg.heavy import Model\n" in core + probe = _python(root, "from pkg.core import helper; print(helper(None))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "ok" + + +def test_a_type_checking_import_merges_into_the_destinations_guard( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/heavy.py"] = HEAVY + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\nfrom typing import TYPE_CHECKING\n\n" + "if TYPE_CHECKING:\n from pkg.heavy import Model\n" + GUARDED_HELPER + ) + fixture["pkg/core.py"] = ( + "from __future__ import annotations\n\nfrom typing import TYPE_CHECKING\n\n" + "if TYPE_CHECKING:\n from pkg.heavy import Other\n\n\n" + "def existing(o: Other) -> int:\n return 1\n" + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert core.count("TYPE_CHECKING") == 2 + assert ( + "if TYPE_CHECKING:\n" + " from pkg.heavy import Other\n" + " from pkg.heavy import Model\n" + ) in core + probe = _python( + root, + "from pkg.core import existing, helper; print(existing(None), helper(None))", + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "1 ok" + + +def test_a_type_checking_import_that_would_rebind_a_destination_name_refuses( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/heavy.py"] = HEAVY + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\nfrom typing import TYPE_CHECKING\n\n" + "if TYPE_CHECKING:\n from pkg.heavy import Model\n" + GUARDED_HELPER + ) + fixture["pkg/core.py"] = "Model = 1\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="already binds Model"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == "Model = 1\n" + + +# --- removing the export takes the export, not its line --------------------------- + + +@needs_node +@pytest.mark.parametrize( + "line", + [ + "export { helper }; registerPlugin();", + "registerPlugin(); export { helper };", + "export { helper, other }; registerPlugin();", + ], +) +def test_a_statement_sharing_the_export_line_stays( + temp_repo: Path, tmp_path: Path, line: str +) -> None: + """The sole `export { helper }` was removed with its whole line, and + `registerPlugin()` on it with it: the old module silently stopped + registering anything on load.""" + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "export const plugins = [];\n\n" + "function registerPlugin() {\n plugins.push('p');\n}\n\n" + "function helper(a) {\n return a + 1;\n}\n\n" + "function other() {\n return 'o';\n}\n\n" + f"{line}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js") + assert report.applied, report.message + probe = _node( + root, + tmp_path, + "import { plugins } from './pkg/util.js';\n" + "import { helper } from './pkg/core.js';\n" + "console.log(plugins.length, helper(1));\n", + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "1 2" + + +# --- relative imports inside the moved code keep their target --------------------- + +NESTED = { + "pkg/__init__.py": "", + "pkg/deps.py": "X = 'pkg.deps'\n", + "pkg/sub/__init__.py": "NAME = 'pkg.sub'\n", + "pkg/sub/deps.py": "X = 'pkg.sub.deps'\n", + "pkg/sub/inner/__init__.py": "", + "pkg/sub/inner/mod.py": "X = 'pkg.sub.inner.mod'\n", +} + + +@pytest.mark.parametrize( + ("body", "expected"), + [ + pytest.param("from .deps import X\n return X", "pkg.sub.deps", id="sibling"), + pytest.param("from . import deps\n return deps.X", "pkg.sub.deps", id="dot"), + pytest.param("from .. import deps\n return deps.X", "pkg.deps", id="dotdot"), + pytest.param( + "from .inner.mod import X\n return X", "pkg.sub.inner.mod", id="deep" + ), + pytest.param("from . import NAME\n return NAME", "pkg.sub", id="package"), + ], +) +def test_a_relative_import_inside_the_moved_function_keeps_its_target( + temp_repo: Path, body: str, expected: str +) -> None: + """The function-local import travelled verbatim, and its dots now + counted from the destination's package: `from .deps import X` moved + from pkg/sub/util.py to pkg/core.py silently read `pkg.deps`.""" + fixture = { + **NESTED, + "pkg/sub/util.py": f"def helper():\n {body}\n" + OTHER, + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater, qn=f"{PROJECT}.pkg.sub.util.helper") + assert report.applied, report.message + probe = _python(root, "from pkg.core import helper; print(helper())") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == expected + + +def test_a_relative_import_above_the_root_refuses_the_move(temp_repo: Path) -> None: + """It names no module the destination could spell.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "def helper():\n from .. import deps\n return deps\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="from .. import deps"): + _move(root, store, updater, target="pkg.sub.core") + assert (root / "pkg/util.py").read_text() == fixture["pkg/util.py"] + + +# --- module state the moved code rebinds stays one variable ----------------------- + + +@pytest.mark.parametrize( + ("util", "probe"), + [ + pytest.param( + "COUNT = 0\n\n\ndef helper():\n global COUNT\n COUNT += 1\n" + " return COUNT\n" + OTHER, + "import pkg.util\nfrom pkg.util import helper\n" + "helper()\nprint(pkg.util.COUNT)\n", + id="moved-writes", + ), + pytest.param( + "COUNT = 0\n\n\ndef bump():\n global COUNT\n COUNT += 1\n\n\n" + "def helper():\n return COUNT\n" + OTHER, + "from pkg.util import bump, helper\nbump()\nprint(helper())\n", + id="old-module-writes", + ), + ], +) +def test_a_global_the_move_would_split_refuses_the_move( + temp_repo: Path, util: str, probe: str +) -> None: + """The destination imported COUNT by value, so `global COUNT` then + rebound the destination's own copy: the moved helper counted in one + module while every reader of `pkg.util.COUNT` saw the other.""" + fixture = {**FIXTURE, "pkg/util.py": util} + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="COUNT"): + _move(root, store, updater) + assert (root / "pkg/util.py").read_text() == util + assert not (root / "pkg/core.py").exists() + result = _python(root, probe) + assert result.stdout.strip() == "1", result.stderr + + +def test_a_global_only_the_moved_code_uses_does_not_refuse(temp_repo: Path) -> None: + """A cache the moved function alone creates and reads moves with it.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "def helper():\n global _CACHE\n _CACHE = 'warm'\n return _CACHE\n" + + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python(root, "from pkg.core import helper; print(helper())") + assert probe.stdout.strip() == "warm", probe.stderr + + +@needs_node +@pytest.mark.parametrize("write", ["count += 1;", "count++;", "count = count + 1;"]) +def test_a_js_module_variable_the_moved_code_assigns_refuses_the_move( + temp_repo: Path, tmp_path: Path, write: str +) -> None: + """An imported binding is read-only, so the moved `count += 1` that + updated the module's `let` threw a TypeError at the destination.""" + root = temp_repo / PROJECT + util = ( + "export let count = 0;\n\n" + f"export function helper() {{\n {write}\n return count;\n}}\n\n" + "export function run() {\n return 'r';\n}\n" + ) + _write(root, "pkg/util.js", util) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="count"): + _move(root, store, updater, target="pkg/core.js") + assert (root / "pkg/util.js").read_text() == util + probe = _node( + root, + tmp_path, + "import { helper } from './pkg/util.js';\nconsole.log(helper());\n", + ) + assert probe.stdout.strip() == "1", probe.stderr + + +@needs_node +def test_a_js_local_that_shadows_a_module_variable_does_not_refuse( + temp_repo: Path, tmp_path: Path +) -> None: + root = temp_repo / PROJECT + _write( + root, + "pkg/util.js", + "export let count = 0;\n\n" + "export function helper() {\n let count = 1;\n count += 1;\n return count;\n}\n\n" + "export function run() {\n return count;\n}\n", + ) + store, updater = _index(root) + report = _move(root, store, updater, target="pkg/core.js") + assert report.applied, report.message + probe = _node( + root, + tmp_path, + "import { helper } from './pkg/core.js';\nconsole.log(helper());\n", + ) + assert probe.stdout.strip() == "2", probe.stderr + + +# --- a wildcard importer still reaches the moved name ----------------------------- + + +@pytest.mark.parametrize( + "util", + [ + pytest.param("def helper(a):\n return a + 1\n", id="last-definition"), + pytest.param("def helper(a):\n return a + 1\n" + OTHER, id="with-sibling"), + pytest.param( + "def helper(a):\n return a + 1\n" + + OTHER + + "\n\n__all__ = [n for n in dir() if not n.startswith('_')]\n", + id="computed-all", + ), + ], +) +def test_a_wildcard_reexport_of_the_moved_name_refuses_the_move( + temp_repo: Path, util: str +) -> None: + """`from pkg.util import *` is recorded with the name `*`, which neither + the move rewrote nor the contract counted as reaching `helper`: the + move passed, and `from pkg.api import helper` raised ImportError.""" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": util, + "pkg/api.py": "from pkg.util import * # noqa: F403\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="pkg/api.py"): + _move(root, store, updater) + assert (root / "pkg/util.py").read_text() == util + probe = _python(root, "from pkg.api import helper; print(helper(1))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "2" + + +@pytest.mark.parametrize( + ("util", "keep_alias"), + [ + pytest.param( + "def helper(a):\n return a + 1\n\n\nVALUE = helper(1)\n", + False, + id="old-module-imports-it-back", + ), + pytest.param("def helper(a):\n return a + 1\n", True, id="keep-alias"), + pytest.param( + "def _helper(a):\n return a + 1\n" + OTHER, False, id="private-name" + ), + ], +) +def test_a_wildcard_reexport_that_still_reaches_the_name_does_not_refuse( + temp_repo: Path, util: str, keep_alias: bool +) -> None: + """When the old module binds the name again, or the star never exported + it, the wildcard importer sees what it saw before.""" + name = "_helper" if "_helper" in util else "helper" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": util, + "pkg/api.py": "from pkg.util import * # noqa: F403\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move( + root, store, updater, qn=f"{PROJECT}.pkg.util.{name}", keep_alias=keep_alias + ) + assert report.applied, report.message + source = "pkg.util" if name == "_helper" else "pkg.api" + probe = _python(root, f"from {source} import {name}; print({name}(1))") + if name == "_helper": + # Only the old path changed; the wildcard never carried it. + assert probe.returncode != 0 + probe = _python(root, "from pkg.core import _helper; print(_helper(1))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "2" + + +def test_the_contract_counts_a_wildcard_importer_of_a_vacated_module( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The contract's own check, with the planner's refusal out of the way: + the stale-importer exemption cleared any import whose names held + neither "" nor the moved name, and `*` holds neither.""" + monkeypatch.setattr( + move_mod.Mover, "_refuse_wildcard_loss", lambda *_args, **_kwargs: None + ) + util = "def helper(a):\n return a + 1\n" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": util, + "pkg/api.py": "from pkg.util import * # noqa: F403\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert not report.applied + assert "pkg/api.py" in report.message, report.message + assert (root / "pkg/util.py").read_text() == util + probe = _python(root, "from pkg.api import helper; print(helper(1))") + assert probe.stdout.strip() == "2", probe.stderr diff --git a/codebase_rag/tests/test_move_safety.py b/codebase_rag/tests/test_move_safety.py new file mode 100644 index 0000000000..5c3ba2441b --- /dev/null +++ b/codebase_rag/tests/test_move_safety.py @@ -0,0 +1,710 @@ +# move (issue #1534) review findings from the stacked PRs #2070-#2080: each +# test names the failure it pins. The common shape is a move that used to +# report success -- or commit -- while leaving a tree that no longer runs, +# or that undid somebody else's edit on its way out. + +from __future__ import annotations + +import ast +import importlib +import subprocess +import sys +from pathlib import Path + +import pytest + +from codebase_rag import constants as cs +from codebase_rag.editing.contract import Verdict +from codebase_rag.editing.move import MoveRefused, _has_annotations, move +from codebase_rag.editing.transaction import EditTransaction, load_history +from codebase_rag.tests.test_move_op import ( + FIXTURE, + PROJECT, + _index, + _materialise, + _qn, + _write, +) + +# The module, which the monkeypatches below reach into. +move_mod = importlib.import_module("codebase_rag.editing.move") +# `pkg/b.py` imports `other` too, so every util variant keeps it defined. +OTHER = "\n\ndef other():\n return 'o'\n" + + +def _move(root: Path, fixture_store, updater, target: str = "pkg.core", **kw): + return move( + root, + fixture_store.fetch_all, + PROJECT, + kw.pop("qn", _qn("pkg.util.helper")), + target, + reingest=updater.reingest, + **kw, + ) + + +def _python(root: Path, code: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, "-c", code], + cwd=root, + check=False, + capture_output=True, + encoding=cs.ENCODING_UTF8, + ) + + +def _forced_failure(monkeypatch: pytest.MonkeyPatch) -> None: + """Make the postcondition fail whatever the delta says.""" + monkeypatch.setattr( + move_mod, "verify", lambda *_a, **_k: Verdict(False, ("forced",), (), None) + ) + + +# --- rollback --------------------------------------------------------------------- + + +def test_failed_postcondition_never_undoes_a_later_unrelated_edit( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The commit lock is released before the re-ingest, so another edit can + land between the move and its contract check. Rolling back "the newest + transaction" then reverted THAT edit and left the failed move applied.""" + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + _forced_failure(monkeypatch) + calls = [] + + def reingest(paths: list[str]) -> None: + updater.reingest(paths) + if not calls: + other = EditTransaction(root) + other.stage("notes.txt", "someone else's edit\n") + assert other.commit().applied + calls.append(paths) + + report = move( + root, + store.fetch_all, + PROJECT, + _qn("pkg.util.helper"), + "pkg.core", + reingest=reingest, + ) + # The unrelated edit survives, and the report does not claim a rollback + # that did not happen: the move is still on disk. + assert (root / "notes.txt").read_text() == "someone else's edit\n" + assert (root / "pkg/core.py").exists() + assert report.applied is True + assert "not rolled back" in report.message + assert len(load_history(root)) == 2 + + +def test_failed_postcondition_undoes_the_move_by_its_own_id( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + _forced_failure(monkeypatch) + report = _move(root, store, updater) + assert report.applied is False + assert report.message == cs.MOVE_CONTRACT_FAILED.format(reasons="forced") + for rel, text in FIXTURE.items(): + assert (root / rel).read_text() == text + assert not (root / "pkg/core.py").exists() + assert load_history(root) == [] + + +@pytest.mark.xfail( + strict=True, + reason=( + "the move now declares the pair, but structural_delta's lone-container " + "pass only pairs a declared rename within one file" + ), +) +def test_moving_an_empty_class_declares_the_rename_to_the_contract( + temp_repo: Path, +) -> None: + """An empty class has no fingerprint and no members, so the delta cannot + infer that `util.Empty` and `core.Empty` are one definition; the move + knows and must say so, or the contract sees a removal plus an addition.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = FIXTURE["pkg/util.py"] + "\n\nclass Empty:\n pass\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater, qn=_qn("pkg.util.Empty")) + assert report.applied, report.message + assert report.verdict is not None + assert report.verdict.ok + + +# --- the new destination is checked like every other file ------------------------- + + +def test_a_new_destination_that_does_not_parse_is_not_committed( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + monkeypatch.setattr( + move_mod.Mover, "_paste_text", lambda *_a, **_k: "def broken(:\n" + ) + report = _move(root, store, updater) + assert report.applied is False + assert report.message == cs.MOVE_PARSE_FAILED.format(files="pkg/core.py") + assert not (root / "pkg/core.py").exists() + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + + +def test_a_target_in_another_language_is_refused(temp_repo: Path) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="language"): + _move(root, store, updater, target="pkg/core.ts", dry_run=True) + + +def test_a_language_move_does_not_implement_is_refused(temp_repo: Path) -> None: + """A loaded grammar is not an implementation: import generation writes + Python syntax for anything that is not JavaScript or TypeScript.""" + root = temp_repo / PROJECT + _write(root, "go.mod", "module example.com/m\n\ngo 1.21\n") + _write(root, "util/util.go", "package util\n\nfunc Helper() int {\n\treturn 1\n}\n") + store, updater = _index(root) + helper = _qn("util.util.Helper") + with pytest.raises(MoveRefused, match="does not support"): + _move( + root, + store, + updater, + qn=helper, + target="core/core.go", + dry_run=True, + ) + + +def test_a_target_outside_the_repository_is_refused(temp_repo: Path) -> None: + """Only a missing file is a new destination; any other patcher refusal + used to be read as "missing" and planned as a file outside the repo.""" + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="outside the repository"): + _move(root, store, updater, target="../outside.py", dry_run=True) + + +# --- what the definition takes with it -------------------------------------------- + + +def test_a_nested_definition_is_refused(temp_repo: Path) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "def outer():\n def inner():\n return 1\n\n return inner()\n" + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + inner = _qn("pkg.util.outer.inner") + with pytest.raises(MoveRefused, match="nested"): + _move(root, store, updater, qn=inner) + assert not (root / "pkg/core.py").exists() + + +def test_a_destination_that_already_binds_the_name_is_refused( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/core.py"] = "def helper():\n return 'mine'\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="already binds helper"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == fixture["pkg/core.py"] + + +def test_a_module_constant_the_definition_reads_is_imported( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "SEP = '-'\n\n\ndef helper(a):\n return SEP.join(a)\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert "from pkg.util import SEP" in (root / "pkg/core.py").read_text() + probe = _python(root, "from pkg.core import helper; print(helper(['a', 'b']))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "a-b" + + +def test_moving_the_last_function_beside_a_module_constant_passes_its_contract( + temp_repo: Path, +) -> None: + """The graph has no node for `SEP`, so the old module read as vacated and + the moved helper's own `from pkg.util import SEP` was reported as a stale + importer: the contract rolled back a move that runs.""" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": "SEP = '-'\n\n\ndef helper(a):\n return SEP.join(a)\n", + "pkg/a.py": "from pkg.util import helper\n\n\ndef run():\n return helper(['x'])\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert report.verdict is not None + assert report.verdict.ok + assert "from pkg.util import SEP" in (root / "pkg/core.py").read_text() + assert "from pkg.core import helper" in (root / "pkg/a.py").read_text() + + +def test_an_importer_left_on_the_vacated_module_still_fails_the_contract( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The constant exemption is by imported name: an importer still naming + the moved helper on its old module is stale, and the move rolls back.""" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": "SEP = '-'\n\n\ndef helper(a):\n return SEP.join(a)\n", + "pkg/a.py": "from pkg.util import helper\n\n\ndef run():\n return helper(['x'])\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + # The importer rewrite is skipped, so a.py keeps naming the old module. + monkeypatch.setattr(move_mod.Mover, "_retarget_importers", lambda *a, **k: ([], [])) + report = _move(root, store, updater) + assert not report.applied + assert "importers still target the old module" in report.message + assert (root / "pkg/a.py").read_text() == fixture["pkg/a.py"] + + +def test_future_directives_travel_with_the_definition(temp_repo: Path) -> None: + """`annotations` is never referenced by name, so the use filter dropped + it and the destination evaluated the annotation eagerly -- `NameError` + at import time for a forward reference.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a: Later) -> Later:\n return a\n\n\n" + "class Later:\n pass\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater, qn=_qn("pkg.util.helper")) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert core.startswith("from __future__ import annotations\n") + probe = _python(root, "import pkg.core") + assert probe.returncode == 0, probe.stderr + + +def test_future_directives_are_added_at_the_head_of_an_existing_destination( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a: Later) -> Later:\n return a\n\n\n" + "class Later:\n pass\n" + OTHER + ) + fixture["pkg/core.py"] = '"""Core."""\n\nVERSION = 1\n' + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + core = (root / "pkg/core.py").read_text() + assert core.startswith('"""Core."""\nfrom __future__ import annotations\n') + assert ast.get_docstring(ast.parse(core)) == "Core." + probe = _python(root, "import pkg.core") + assert probe.returncode == 0, probe.stderr + + +def test_an_import_added_to_a_module_without_imports_keeps_its_docstring( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + '"""Utilities."""\n\n\n' + "def helper(a):\n return a\n\n\n" + "def run():\n return helper(1)\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + util = (root / "pkg/util.py").read_text() + assert ast.get_docstring(ast.parse(util)) == "Utilities." + assert "from pkg.core import helper\n" in util + + +# --- dry run ---------------------------------------------------------------------- + + +def test_dry_run_reports_the_diff_it_would_write(temp_repo: Path) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + report = _move(root, store, updater, dry_run=True) + assert not report.applied + assert "+def helper(a):" in report.diff + assert "-def helper(a):" in report.diff + assert "+from pkg.core import helper" in report.diff + assert not (root / "pkg/core.py").exists() + assert (root / "pkg/util.py").read_text() == FIXTURE["pkg/util.py"] + assert load_history(root) == [] + + +# --- cycle refusal ---------------------------------------------------------------- + + +def test_an_importer_of_an_unrelated_name_does_not_block_the_move( + temp_repo: Path, +) -> None: + """`d` imports `other` from util, and `core` imports `d`. The move only + rewires importers of `helper`; `d` gains no edge to `core`.""" + fixture = dict(FIXTURE) + fixture["pkg/d.py"] = ( + "from pkg.util import other\n\n\ndef dd():\n return other()\n" + ) + fixture["pkg/core.py"] = "from pkg.d import dd\n\nVALUE = dd\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + + +def test_a_cycle_through_a_rewired_importer_outside_both_modules_is_refused( + temp_repo: Path, +) -> None: + """`c` imports `helper` and `core` imports `c`: rewiring `c` to `core` + closes `c -> core -> c`. The simulated graph is project-wide, so a + module outside the old and new paths still takes part.""" + fixture = dict(FIXTURE) + fixture["pkg/c.py"] = ( + "from pkg.util import helper\n\n\ndef cc():\n return helper([])\n" + ) + fixture["pkg/core.py"] = "from pkg.c import cc\n\nVALUE = cc\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused) as excinfo: + _move(root, store, updater) + assert excinfo.value.cycle == (_qn("pkg.c"), _qn("pkg.core")) + + +class _Ordered(list): + """A cycle set whose iteration order the test controls.""" + + def __sub__(self, other: object) -> list[frozenset[str]]: + return [c for c in self if c not in other] # type: ignore[operator] + + +def test_the_reported_cycle_is_chosen_deterministically( + temp_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + root = _materialise(temp_repo, FIXTURE) + store, updater = _index(root) + core, b, a = _qn("pkg.core"), _qn("pkg.b"), _qn("pkg.a") + cycles = _Ordered([frozenset({b, core}), frozenset({a, core})]) + results = iter([_Ordered(), cycles]) + monkeypatch.setattr(move_mod, "import_cycles", lambda _graph: next(results)) + with pytest.raises(MoveRefused) as excinfo: + _move(root, store, updater, dry_run=True) + assert excinfo.value.cycle == (a, core) + + +# --- importer retargeting --------------------------------------------------------- + + +def test_an_importer_row_without_an_end_column_is_left_unchanged( + temp_repo: Path, +) -> None: + """A missing end column defaults to 0, which for an import not at column + 0 (here one inside a function body) makes a reversed span. The rewriter + reads an empty statement there, matches nothing and leaves the file as + it was; this pins that it keeps doing so rather than rewriting bytes.""" + fixture = dict(FIXTURE) + fixture["pkg/a.py"] = ( + "def run():\n from pkg.util import helper\n\n return helper(['x', 'y'])\n" + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + + def fetch_all(query: str, params=None): + rows = store.fetch_all(query, params) + if query == move_mod.cq.CYPHER_GRAPH_IMPORTERS: + return [{**row, cs.KEY_END_COL: None} for row in rows] + return rows + + report = move( + root, fetch_all, PROJECT, _qn("pkg.util.helper"), "pkg.core", dry_run=True + ) + assert report.importers == () + assert set(report.unchanged_importers) == {"pkg/a.py:2", "pkg/b.py:1"} + + +# --- JavaScript: what the moved code imports back must be exported --------------- + +JS_UTIL = "export function helper(a) {\n return a.join(SEP);\n}\n" + + +@pytest.mark.parametrize( + ("declaration", "exported"), + [ + pytest.param("export const SEP = '-';\n\n", True, id="exported"), + pytest.param("const SEP = '-';\n\n", False, id="module-private"), + ], +) +def test_a_js_module_constant_is_imported_only_when_exported( + temp_repo: Path, declaration: str, exported: bool +) -> None: + root = temp_repo / PROJECT + _write(root, "pkg/util.js", declaration + JS_UTIL) + store, updater = _index(root) + if not exported: + with pytest.raises(MoveRefused, match="SEP is not exported"): + _move(root, store, updater, target="pkg/core.js", dry_run=True) + return + report = _move(root, store, updater, target="pkg/core.js", dry_run=True) + assert "+import { SEP } from './util';" in report.diff + + +# --- names a star import provides, and postponed annotations ---------------------- + + +def test_a_name_only_a_star_import_provides_refuses_the_move(temp_repo: Path) -> None: + """`from pkg.cfg import *` is recorded under `*`, so nothing carried + `RATE` and the moved `helper` raised NameError once called, while the + move committed and passed its contract (bot review).""" + fixture = dict(FIXTURE) + fixture["pkg/cfg.py"] = "RATE = 3\n" + fixture["pkg/util.py"] = ( + "from pkg.cfg import * # noqa: F403\n\n\n" + "def helper(a):\n return len(a) * RATE # noqa: F405\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="RATE reaches pkg/util.py only through"): + _move(root, store, updater) + assert (root / "pkg/util.py").read_text() == fixture["pkg/util.py"] + assert not (root / "pkg/core.py").exists() + + +def test_a_star_import_the_definition_does_not_rely_on_does_not_refuse( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/cfg.py"] = "RATE = 3\n" + fixture["pkg/util.py"] = ( + "from pkg.cfg import * # noqa: F403\n\nSEP = '-'\n\n\n" + "def helper(a):\n return SEP.join(a)\n" + OTHER + ) + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python(root, "from pkg.core import helper; print(helper(['a', 'b']))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "a-b" + + +_ANNOTATED_CORE = ( + "def existing(x: int):\n return existing.__annotations__['x'] is int\n" +) + + +def test_postponed_annotations_are_not_forced_on_an_annotated_destination( + temp_repo: Path, +) -> None: + """Adding the old module's `annotations` future to the destination + turned `existing`'s annotations into strings, so it returned False + after a move that committed and passed its contract (bot review).""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a: list[str]) -> str:\n return '-'.join(a)\n" + OTHER + ) + fixture["pkg/core.py"] = _ANNOTATED_CORE + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="enables from __future__ import annotations"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == _ANNOTATED_CORE + + +def test_an_unannotated_definition_does_not_carry_postponed_annotations( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a):\n return '-'.join(a)\n" + OTHER + ) + fixture["pkg/core.py"] = _ANNOTATED_CORE + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert "__future__" not in (root / "pkg/core.py").read_text() + probe = _python( + root, "from pkg.core import existing, helper; print(existing(1), helper('ab'))" + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "True a-b" + + +def test_a_destination_postponing_annotations_refuses_an_eager_definition( + temp_repo: Path, +) -> None: + """The other direction: the moved annotations would become strings.""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "def helper(a: list[str]) -> str:\n return '-'.join(a)\n" + OTHER + ) + fixture["pkg/core.py"] = "from __future__ import annotations\n\nVERSION = 1\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="annotations in helper evaluate"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == fixture["pkg/core.py"] + + +def test_a_definition_that_execs_annotated_code_keeps_postponed_annotations( + temp_repo: Path, +) -> None: + """`exec` inherits the caller's future features, so the generated + `x: Missing` annotation stayed a string only while `annotations` was on; + the unannotated mover dropped it and the call raised NameError + (bot review).""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a):\n ns = {}\n" + " exec('def g(x: Missing):\\n return x', ns)\n" + " return ns['g'].__annotations__['x']\n" + OTHER + ) + fixture["pkg/core.py"] = "VERSION = 1\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + assert ( + (root / "pkg/core.py") + .read_text() + .startswith("from __future__ import annotations\n") + ) + probe = _python(root, "from pkg.core import helper; print(helper(1))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "Missing" + + +def test_an_exec_into_an_annotated_destination_without_the_future_refuses( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\n\n" + "def helper(a):\n ns = {}\n" + " exec('def g(x: Missing):\\n return x', ns)\n" + " return ns['g'].__annotations__['x']\n" + OTHER + ) + fixture["pkg/core.py"] = _ANNOTATED_CORE + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="enables from __future__ import annotations"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == _ANNOTATED_CORE + + +def test_a_compile_method_on_another_object_is_not_dynamic_code( + temp_repo: Path, +) -> None: + """`re.compile` compiles a pattern, not code, so an unannotated helper + calling it relies on no future feature and moves into an annotated + destination (bot review).""" + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = ( + "from __future__ import annotations\n\nimport re\n\n\n" + "def helper(a):\n return re.compile(a).pattern\n" + OTHER + ) + fixture["pkg/core.py"] = _ANNOTATED_CORE + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python( + root, "from pkg.core import existing, helper; print(helper('x'), existing(1))" + ) + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "x True" + + +@pytest.mark.parametrize("receiver", ["builtins", "__builtins__"]) +def test_a_builtins_exec_still_counts_as_dynamic_code(receiver: str) -> None: + assert _has_annotations(f"def f():\n return {receiver}.exec\n") + assert not _has_annotations("def f(p):\n return p.exec\n") + + +@pytest.mark.parametrize( + ("scope", "use"), + [ + ("import builtins as bi\n", "bi.exec"), + ("from builtins import exec as run\n", "run"), + ], +) +def test_an_aliased_builtin_counts_as_dynamic_code(scope: str, use: str) -> None: + """The alias is bound by the module, not the definition cut out of it, + so it is resolved there (bot review).""" + definition = f"def f():\n return {use}\n" + assert _has_annotations(definition, scope + definition) + assert not _has_annotations(definition, definition) + + +_ALIASED_EXEC = ( + "from __future__ import annotations\n\nimport builtins as bi\n\n\n" + "def helper(a):\n ns = {}\n" + " bi.exec('def g(x: Missing):\\n return x', ns)\n" + " return ns['g'].__annotations__['x']\n" + OTHER +) + + +def test_an_aliased_exec_keeps_postponed_annotations(temp_repo: Path) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = _ALIASED_EXEC + fixture["pkg/core.py"] = "VERSION = 1\n" + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + assert report.applied, report.message + probe = _python(root, "from pkg.core import helper; print(helper(1))") + assert probe.returncode == 0, probe.stderr + assert probe.stdout.strip() == "Missing" + + +def test_an_aliased_exec_into_an_annotated_destination_refuses( + temp_repo: Path, +) -> None: + fixture = dict(FIXTURE) + fixture["pkg/util.py"] = _ALIASED_EXEC + fixture["pkg/core.py"] = _ANNOTATED_CORE + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + with pytest.raises(MoveRefused, match="enables from __future__ import annotations"): + _move(root, store, updater) + assert (root / "pkg/core.py").read_text() == _ANNOTATED_CORE + + +def test_a_from_import_of_the_vacated_module_never_commits_a_broken_caller( + temp_repo: Path, +) -> None: + """`from pkg import util` then `util.helper()`: the contract cleared that + import as one of a constant the module kept (bot review). End to end the + caller must still run, whether the move retargets it or rolls back.""" + fixture = { + "pkg/__init__.py": "", + "pkg/util.py": "SEP = '-'\n\n\ndef helper(a):\n return SEP.join(a)\n", + "pkg/a.py": "from pkg import util\n\n\ndef run():\n return util.helper(['x', 'y'])\n", + } + root = _materialise(temp_repo, fixture) + store, updater = _index(root) + report = _move(root, store, updater) + probe = _python(root, "from pkg.a import run; print(run())") + assert probe.returncode == 0, (report.applied, report.message, probe.stderr) + assert probe.stdout.strip() == "x-y" diff --git a/codebase_rag/tests/test_variadic_header_detection.py b/codebase_rag/tests/test_variadic_header_detection.py new file mode 100644 index 0000000000..4a7d5c7135 --- /dev/null +++ b/codebase_rag/tests/test_variadic_header_detection.py @@ -0,0 +1,128 @@ +"""Surplus-positional absorption must parse the header, not scan it. + +`_is_variadic` is the SOLE suppressor of a too-many-arguments verdict, so +both error directions do damage: a false positive hides a real arity +error, and a false negative makes a CORRECT edit fail its own +postcondition and roll back. + +Scanning raw text got both wrong. A `*` inside a default (`b=2*3`) or a +string (`doc='a*b'`) reads as `*args`, and the header window was cut at +the first `)`, which can fall inside a default, a string or a comment -- +hiding a real `*rest` that comes after it. + +CPython is the ground truth here: calling the function with one more +positional than it declares is what decides whether the call raises. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +from codebase_rag.structural_delta import Definition, _is_variadic + + +def _truth(header: str) -> bool: + """Whether the header really absorbs a surplus POSITIONAL argument. + + Decided by CALLING the function with one more positional than it + declares, so the oracle is CPython's own arity rule rather than a + restatement of the implementation. + """ + fn = ast.parse(header + "\n pass\n").body[0] + assert isinstance(fn, ast.FunctionDef) + namespace: dict[str, object] = {"g": lambda: None} + exec(header + "\n pass\n", namespace) # noqa: S102 + func = namespace[fn.name] + assert callable(func) + declared = len(fn.args.posonlyargs) + len(fn.args.args) + required_kwonly = { + kw.arg: None + for kw, default in zip(fn.args.kwonlyargs, fn.args.kw_defaults, strict=True) + if default is None + } + try: + func(*range(declared + 1), **required_kwonly) + except TypeError: + return False + return True + + +def _variadic(tmp_path: Path, source: str, name: str = "f", line: int = 1) -> bool: + """`_is_variadic` for the def `name` starting on `line` of `source`.""" + (tmp_path / "m.py").write_text(source, encoding="utf-8") + definition = Definition( + label="Function", + qualified_name=f"m.{name}", + name=name, + path="m.py", + start_line=line, + end_line=len(source.splitlines()), + positional_params=(), + fingerprint="", + fingerprint_nodes=0, + branches=frozenset(), + ) + return _is_variadic(definition, tmp_path) + + +HEADERS = [ + pytest.param("def f(a):", id="plain"), + pytest.param("def f(a, *rest):", id="star-args"), + # A bare `*` takes NO extra positional; it is not a suppressor. + pytest.param("def f(a, *, k):", id="bare-star-keyword-only"), + pytest.param("def f(a, *, b=1):", id="bare-star-defaulted-keyword-only"), + pytest.param("def f(a, *rest, k=1):", id="star-args-and-keyword-only"), + pytest.param("def f(*args, **kw):", id="star-and-kwargs"), + # A regex read the `*` in the default as `*args`. + pytest.param("def f(a, b=2*3):", id="star-in-arithmetic-default"), + pytest.param("def f(a, doc='a*b'):", id="star-inside-a-string"), + # `find(")")` stopped inside the default, hiding the real `*rest`. + pytest.param("def f(a=(1, 2), *rest):", id="star-after-tuple-default"), + pytest.param("def f(a=g(), *rest):", id="star-after-call-default"), + pytest.param("def f(a, cb=lambda x: (x)):", id="lambda-default"), + pytest.param("def café(a, *rest):", id="multibyte-identifier"), +] + + +@pytest.mark.parametrize("header", HEADERS) +def test_it_agrees_with_python(tmp_path: Path, header: str) -> None: + name = header[len("def ") : header.index("(")] + source = f"def g():\n return 1\n\n\n{header}\n pass\n" + assert _variadic(tmp_path, source, name=name, line=5) is _truth(header) + + +@pytest.mark.parametrize( + "source", + [ + # The read-back used to stop at the first line containing `)`. + "def f(\n a=g(),\n *rest,\n):\n pass\n", + # A bracket inside a string or a comment is not a delimiter. + 'def f(\n a=")",\n *rest,\n):\n pass\n', + "def f(\n a=1, # )\n *rest,\n):\n pass\n", + # A return annotation runs past the `)` that closes the parameters. + "def f(a, *rest) -> tuple[\n int,\n str,\n]:\n return (a, 'x')\n", + # A one-line definition carries its suite on the header line. + "def f(a, *rest): return a\n", + ], + ids=[ + "paren-in-call-default", + "paren-in-string-default", + "paren-in-comment", + "multiline-return-annotation", + "one-line-definition", + ], +) +def test_a_star_rest_after_a_closing_bracket_is_found( + tmp_path: Path, source: str +) -> None: + """False negatives: each made a correct edit roll back.""" + assert _variadic(tmp_path, source) is True + + +def test_a_one_line_definition_without_star_rest_absorbs_nothing( + tmp_path: Path, +) -> None: + assert _variadic(tmp_path, "def f(a): return a\n") is False diff --git a/codebase_rag/tools/tool_descriptions.py b/codebase_rag/tools/tool_descriptions.py index 01cc0be59b..9f13c3ca18 100644 --- a/codebase_rag/tools/tool_descriptions.py +++ b/codebase_rag/tools/tool_descriptions.py @@ -379,6 +379,27 @@ class AgenticToolName(StrEnum): "diff without touching the tree. Applied edits are recorded for `cgr edits " "undo`." ) +MCP_MOVE = ( + "Move a module-level function or class to another module. The definition " + "(decorators, docstring, adjacent comments) is cut and pasted into " + "`target_module` (a dotted module name or a repo-relative path; created " + "when missing) with the imports it needs, every importer is rewritten, " + "uses through a module import (`pkg.util.helper`) follow, and the old " + "module keeps an import of the name when it still uses it. `keep_alias` " + "leaves a re-export at the old location so the old import path keeps " + "working. Refused before any file is touched when the move would create " + "an import cycle, naming the cycle. One transaction, held to the " + "postcondition contract; set `dry_run` to see the plan." +) +MCP_PARAM_TARGET_MODULE = ( + "Destination module: a dotted module name (`pkg.core`) or a repo-relative path." +) +MCP_PARAM_MOVE_DRY_RUN = ( + "Plan only: report the files, importers and copied imports, write nothing." +) +MCP_PARAM_KEEP_ALIAS = ( + "Leave a re-export at the old location so the old import path still works." +) MCP_PARAM_NEW_PARAMS = ( "The new parameter list in order, one entry per parameter (`name`, " "`name: type`, `name = default`, `name: type = default`)." @@ -628,6 +649,7 @@ class AgenticToolName(StrEnum): MCPToolName.GLOSSES: MCP_GLOSSES, MCPToolName.RENAME: MCP_RENAME, MCPToolName.CHANGE_SIGNATURE: MCP_CHANGE_SIGNATURE, + MCPToolName.MOVE: MCP_MOVE, MCPToolName.CONTEXT: MCP_CONTEXT, MCPToolName.QUERY_CODE_GRAPH: MCP_QUERY_CODE_GRAPH, MCPToolName.GET_CODE_SNIPPET: MCP_GET_CODE_SNIPPET, diff --git a/docs/architecture/move.md b/docs/architecture/move.md new file mode 100644 index 0000000000..1a2467a96b --- /dev/null +++ b/docs/architecture/move.md @@ -0,0 +1,66 @@ +--- +description: "How cgr moves a definition to another module: cut with its imports, paste, rewrite every importer, keep the old path working on request, and refuse an import cycle before touching a file." +--- + +# Move + +`move` is edit-algebra operation 3 (issue #1534). Moving a symbol out of a +shared dumping ground into the one package that uses it shrinks affected +sets, and by hand it is tedious: the definition, its own imports, every +importer, re-exports. The graph knows the definition's span, the imports its +module binds and every importer's statement +([edge-site properties](graph-schema.md#edge-site-properties)), so the +whole move is one transaction. + +```bash +cgr move myproj.pkg.util.helper pkg.core --dry-run +cgr move myproj.pkg.util.helper pkg/core.py --keep-alias +``` + +The MCP tool `move` takes `qualified_name`, `target_module` (a dotted +module name or a repo-relative path; created when missing), and the +optional `keep_alias`, `dry_run` and `project`. + +## What happens + +1. **Cut.** The definition's whole lines, plus its decorators (or `export` + wrapper), and the comment lines immediately above it, leave the old + module together with the blank lines that separated it from what + follows. Methods are refused: move the class. +2. **Paste with what it needs.** Of the old module's import statements + (the graph's `IMPORTS` sites of that module), those binding a name the + moved text uses are copied to the target, narrowed to that name and + respelled when the specifier is relative (JS/TS). Names the moved text + takes from the old module itself are imported from it. +3. **Importers.** Every importer of the symbol is retargeted through the + [import rewriter](patchers.md#import-rewriting-for-rename-and-move): + `from pkg.util import helper, other` becomes two statements, one per + module. Python uses through a module import (`pkg.util.helper(...)`) + follow too, gaining `import pkg.core`. Importer statements the rewriter + cannot retarget are listed as `unchanged_importers`. +4. **The old module.** When it still uses the name it imports it back from + the target. With `keep_alias` it always does, marked as a re-export + (`# noqa: F401`; `export { helper } from './core'` in JS/TS), so the old + import path keeps working. + +## Cycle refusal + +Before any file is touched, the module import graph from the graph store +is simulated with the move applied: the target gains the copied imports +and, when the moved text needs the old module, an edge back to it; the old +module gains an edge to the target when it still uses the name or keeps an +alias; every importer of the old module gains the target. A strongly +connected component containing the old or new module that did not exist +before is a new cycle, and the move is refused naming it. + +## Atomicity and contract + +Edits are [patcher](patchers.md) span edits staged in one +[edit transaction](edit-transactions.md), together with the target file when +it is new; a file that no longer parses rolls everything back. Applied moves +are held to the [postcondition contract](postcondition-contract.md) for a +move: the symbol reported moved to its new home (the structural delta pairs +a removed and an added definition across files by name and fingerprint), +importers updated with no caller left dangling, call-site count unchanged, +no new import cycle, no new duplicate. A failing contract undoes the +transaction and re-ingests the restored files. diff --git a/docs/guide/cli-reference.md b/docs/guide/cli-reference.md index 404032f413..df1cacd119 100644 --- a/docs/guide/cli-reference.md +++ b/docs/guide/cli-reference.md @@ -48,6 +48,7 @@ Every top-level command, from the CLI's own help registry: | `cgr check` | Report the structural delta of the working tree against a git ref: dangling callers and importers, arity findings, new duplicates, new import cycles, tests reaching the edited symbols. | | `cgr rename` | Rename a definition everywhere the graph references it (definition, call and reference sites, imports, overrides, __all__); refuses on guessed sites and on occurrences the graph has no site for. | | `cgr change-signature` | Change a Python definition's parameter list and rewrite every call site the graph knows; sites the mapping cannot complete are listed, not touched. | +| `cgr move` | Move a module-level definition to another module: importers rewritten, needed imports carried, refused when it would create an import cycle. | | `cgr context` | Print a graph-ranked context slice for a symbol, location or task within a token budget: source, caller lines, callee signatures, types, tests, docs. | | `cgr port` | Judge a port between languages: pair the functions of two implementations and compare their calls and arity, as JSON, no LLM. | | `cgr workspace` | Manage named groups of repositories | diff --git a/docs/guide/mcp-server.md b/docs/guide/mcp-server.md index 7dca34b8a4..998d2a234b 100644 --- a/docs/guide/mcp-server.md +++ b/docs/guide/mcp-server.md @@ -97,6 +97,7 @@ cgr daemon up | `glosses` | Notes (Glosses) about one definition: `annotating` are filed on it, `mentioning` are filed on other definitions and refer to it. `target` is resolved as `annotate` resolves it. Each note's `anchor_state` is graded after each sync of its project: EXACT when its definition's content hash still equals the one recorded at writing, STALE when it differs (EXACT again on revert), MOVED when the name is gone and exactly one definition in the project carries the recorded hash (the note follows it; `moved_from` keeps the old name), AMBIGUOUS when several do (`candidate_qns` lists them). When no definition carries the hash (a rename changes it), the note's text-quote anchor is tried: a digest of the definition's own text with its name masked, so exactly one definition in the project whose body still matches is followed as MOVED (then graded STALE, since the signature changed), several with the same body are told apart by the lines around them or listed as AMBIGUOUS, and none is LOST. The quote is recorded only when the note was written against this server's own checkout. A note on a module, or one written before hashes were recorded, is not graded while attached (there is no hash to compare) and is LOST if its name disappears (a module has no span to quote); a class, interface, enum, type or union carries a hash like a function and is graded and repaired the same way. A target that no longer resolves returns the error plus `orphaned`: the unattached notes written against that name. Deterministic: fixed graph queries, no LLM, same graph gives the same JSON. Use this instead of query_code_graph whenever you know the exact name or location. | | `rename` | Rename a function, method, class or other definition everywhere the graph knows it is referenced: the definition, every call, reference and instantiation site, import statements (aliases kept), overrides in both directions, and Python `__all__` entries. Refuses when any site was resolved heuristically, by overload fan-out, or only by a trace, unless `allow_heuristic` is true. It also refuses when the source names the symbol where the graph has no site (a call the index missed), listing those occurrences as `unplanned`; `allow_heuristic` rewrites them as guessed sites. A class referenced by an inheritance or type annotation edge that carries no rewrite location refuses regardless of `allow_heuristic`. Every rewritten file must still parse; otherwise nothing is written. Set `dry_run` to see the plan and diff without touching the tree. Applied edits are recorded for `cgr edits undo`, and Markdown mentions of the old name are listed for a human to review. | | `change_signature` | Change a Python function's or method's parameter list and rewrite every call site the graph knows, on the definition and on every override in both directions. `new_params` lists the new parameters in order (`n: int`, `limit: int = 10`, or a bare old name to carry it over unchanged). `mapping` says where each new parameter's value comes from at a call site: an old parameter's name, its 0-based index, or `=literal` to insert that text at every site; a new parameter absent from the mapping takes the old parameter of the same name. A site the mapping cannot complete (a missing required value, a splat, a surplus argument, an undeclared keyword) or that the graph resolved heuristically is left as written and listed as `unmapped`; pass `allow_heuristic` to rewrite guessed sites too. Every rewritten file must still parse, and with an indexed project the change is undone when its postcondition fails. Set `dry_run` to see the plan and diff without touching the tree. Applied edits are recorded for `cgr edits undo`. | +| `move` | Move a module-level function or class to another module. The definition (decorators, docstring, adjacent comments) is cut and pasted into `target_module` (a dotted module name or a repo-relative path; created when missing) with the imports it needs, every importer is rewritten, uses through a module import (`pkg.util.helper`) follow, and the old module keeps an import of the name when it still uses it. `keep_alias` leaves a re-export at the old location so the old import path keeps working. Refused before any file is touched when the move would create an import cycle, naming the cycle. One transaction, held to the postcondition contract; set `dry_run` to see the plan. | | `context` | A graph-ranked minimal context slice for a task, within a token budget. `target` is a qualified name, a bare name, `path:line`, or free text (matched by embedding similarity when the semantic extra is installed). Returns the target's source, its direct callers' call lines, its direct callees' signatures, the types it accepts and returns, the tests that reach it, and the documentation sections whose file links to it, ranked by graph distance (trace hotness and similarity break ties) and trimmed to `budget_tokens`. Every piece says why it is included. Use this before reading whole files. | | `query_code_graph` | Prefer the deterministic tools (resolve, definition, callers, callees, implementors, overrides, importers, tests_reaching) when you know the exact name or location: they run fixed queries with no LLM. Use this for open-ended questions. Query the codebase knowledge graph using natural language. Ask questions like 'What functions call UserService.create_user?' or 'Show me all classes that implement the Repository interface'. Pass `project` to restrict results to one indexed project; use list_projects for the available names. Omit it to search them all. The scope is enforced on the results before they are capped, so it holds regardless of the query generated and the result limit is spent on rows from the requested project. | | `get_code_snippet` | Retrieve source code for a function, class, or method by its qualified name. Returns the source code, file path, line numbers, and docstring. | diff --git a/evals/cgr_graph.py b/evals/cgr_graph.py index 7b94f4c334..75cfa99630 100644 --- a/evals/cgr_graph.py +++ b/evals/cgr_graph.py @@ -651,7 +651,8 @@ def _delta_module_imports( or _shadowed_by_longer_owner(source_qn, longer_project_prefixes) ): continue - for _fl, fv, rel, tl, tv, _site in self._out.get(node_id, ()): + for edge in self._out.get(node_id, ()): + _fl, fv, rel, tl, tv, _site = edge if ( rel == cs.RelationshipType.IMPORTS.value and tl == module @@ -663,6 +664,9 @@ def _delta_module_imports( cs.KEY_FROM_QN: _result(fv), cs.KEY_FROM_PATH: _result(props.get(cs.KEY_PATH)), cs.KEY_TO_QN: _result(tv), + cs.KEY_IMPORTED_NAME: _result( + self.props_for(edge).get(cs.KEY_IMPORTED_NAME) + ), } ) return rows @@ -1153,6 +1157,16 @@ def _graph_rows(self, query: str, params: PropertyDict) -> list[ResultRow]: rows.append( self._graph_edge_row(edge, self._GRAPH_IMPORT_KEYS, source) ) + elif query == cq.CYPHER_GRAPH_IMPORTS_OF: + for source in targets: + for edge in self._out.get(source, ()): + if edge[2] != cs.RelationshipType.IMPORTS.value: + continue + row = self._graph_edge_row( + edge, self._GRAPH_IMPORT_KEYS, (edge[3], edge[4]) + ) + row[cs.KEY_TO_QN] = _result(edge[4]) + rows.append(row) return rows # --- context slice reads (issue #1536) ----------------------------------- @@ -1394,6 +1408,7 @@ def fetch_all( | cq.CYPHER_GRAPH_IMPLEMENTORS | cq.CYPHER_GRAPH_OVERRIDES | cq.CYPHER_GRAPH_IMPORTERS + | cq.CYPHER_GRAPH_IMPORTS_OF | cq.CYPHER_GRAPH_RESOLVE_NAME | cq.CYPHER_GRAPH_RESOLVE_LOCATION | cq.CYPHER_GRAPH_CALLEES diff --git a/mkdocs.yml b/mkdocs.yml index ec82430146..7abaadc269 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -113,6 +113,7 @@ nav: - Rename Operation: architecture/rename.md - Change Signature Operation: architecture/change-signature.md - Postcondition Contract: architecture/postcondition-contract.md + - Move: architecture/move.md - Context Slice: architecture/context-slice.md - Advanced: - Adding Languages: advanced/adding-languages.md