From a37349e11b8253cc2b5a0e48269b337a0690328d Mon Sep 17 00:00:00 2001 From: vitali87 Date: Wed, 23 Sep 2026 00:20:45 +0100 Subject: [PATCH] feat(signature): contract expectation, shared call-site helpers and constants for change_signature (#1533, 1/9) --- codebase_rag/constants/ast_python.py | 18 ++++ codebase_rag/constants/cli.py | 103 +++++++++++++++++++++++ codebase_rag/constants/core.py | 1 + codebase_rag/constants/graph.py | 4 + codebase_rag/editing/__init__.py | 4 +- codebase_rag/editing/contract.py | 22 ++++- codebase_rag/editing/rename.py | 67 ++------------- codebase_rag/editing/sites.py | 72 ++++++++++++++++ codebase_rag/tests/test_edit_contract.py | 66 +++++++++++++-- 9 files changed, 286 insertions(+), 71 deletions(-) create mode 100644 codebase_rag/editing/sites.py diff --git a/codebase_rag/constants/ast_python.py b/codebase_rag/constants/ast_python.py index 76332fd630..e7f698ffb5 100644 --- a/codebase_rag/constants/ast_python.py +++ b/codebase_rag/constants/ast_python.py @@ -51,6 +51,24 @@ TS_PY_DICTIONARY_SPLAT_PATTERN = "dictionary_splat_pattern" TS_PY_POSITIONAL_SEPARATOR = "positional_separator" TS_PY_KEYWORD_SEPARATOR = "keyword_separator" +# A definition's parameter list node, and the splats an argument list can +# carry (`f(*args)`, `f(**opts)`); the `_pattern` forms above are the +# definition-side spellings. +TS_PY_PARAMETERS = "parameters" +TS_PY_LIST_SPLAT = "list_splat" +TS_PY_DICTIONARY_SPLAT = "dictionary_splat" +# Scopes nested in a function body, where a parameter's name may be re-bound +# rather than read, and the statements that re-bind a name in place. +TS_PY_SET_COMPREHENSION = "set_comprehension" +TS_PY_DICTIONARY_COMPREHENSION = "dictionary_comprehension" +TS_PY_GENERATOR_EXPRESSION = "generator_expression" +TS_PY_NONLOCAL_STATEMENT = "nonlocal_statement" +# `typing` names a literal-versus-annotation check understands. +PY_TYPING_OPTIONAL = "Optional" +PY_TYPING_UNION = "Union" +# The first parameter of a method that names the instance or class; a +# signature change keeps it in place and never remaps it. +PY_RECEIVER_NAMES = frozenset({"self", "cls"}) TS_PY_SUBSCRIPT = "subscript" # The `subscript` node's index field (`os.environ["K"]` -> the `"K"` string). TS_PY_FIELD_SUBSCRIPT = "subscript" diff --git a/codebase_rag/constants/cli.py b/codebase_rag/constants/cli.py index ccec7224c7..94a85d9329 100644 --- a/codebase_rag/constants/cli.py +++ b/codebase_rag/constants/cli.py @@ -612,6 +612,109 @@ class DiffMarker: RENAME_CONTRACT_UNMEASURED = ( "Rename applied, but its postcondition could not be measured: {error}" ) +# Change signature (issue #1533). +SIGNATURE_PARAM_PROBE = "def _({text}): pass" +SIGNATURE_NOT_PYTHON = ( + "Cannot change the signature of {qn}: {path} is not Python, and only Python " + "definitions are supported for now (issue #{issue})" +) +SIGNATURE_DEFINITION_UNREADABLE = ( + "Cannot change the signature of {qn}: its file {path} cannot be read " + "({error}). The index names a file the tree no longer has; re-index and retry." +) +SIGNATURE_NO_HEADER = "Could not locate the definition of {qn} in {path}" +SIGNATURE_UNSUPPORTED_PARAMS = ( + "Cannot change the signature of {qn}: `{text}` is not a plain " + "positional-or-keyword parameter, and only those can be remapped" +) +SIGNATURE_UNUSUAL_RECEIVER = ( + "Cannot change the signature of {qn}: its first parameter `{name}` is not " + "self or cls, so the receiver cannot be told from the parameters; rename " + "it to self or cls first" +) +SIGNATURE_STAGE_FAILED = "Cannot stage the signature change: {error}" +SIGNATURE_BAD_PARAM = ( + "Not a parameter: `{text}` (expected `name`, `name: type`, `name = default` " + "or `name: type = default`)" +) +SIGNATURE_DUPLICATE_NEW = "Parameter {name} is listed twice" +SIGNATURE_REQUIRED_AFTER_DEFAULT = ( + "Parameter {name} has no default but comes after a defaulted parameter" +) +SIGNATURE_BODY_REBINDS = ( + "Cannot rename parameter {old} of {qn} to {new}: `{old}` is used inside a " + "nested scope, or re-bound by a global, nonlocal or import statement, in " + "its body; rename it by hand first" +) +SIGNATURE_DEFAULT_REFERENCES_RENAMED = ( + "Cannot rename parameter {old} of {qn} to {new}: the default of `{param}` " + "reads `{old}`, which is evaluated at definition time and would raise " + "NameError; rewrite that default by hand first" +) +SIGNATURE_DROPPED_STILL_READ = ( + "Cannot drop parameter {name} of {qn}: its body still reads `{name}`, " + "which would raise NameError when called; remove that use by hand first" +) +SIGNATURE_BODY_NAME_TAKEN = ( + "Cannot rename parameter {old} of {qn} to {new}: `{new}` is already used " + "in its body" +) +SIGNATURE_HIERARCHY_MISMATCH = ( + "Cannot change the signature of {qn}: its override {member} declares " + "({theirs}) where {qn} declares ({ours}); make the hierarchy agree first" +) +SIGNATURE_MAPPING_UNKNOWN_NEW = ( + "The mapping names {name}, which is not a new parameter ({names})" +) +SIGNATURE_MAPPING_UNKNOWN_OLD = ( + "The mapping feeds {name} from `{source}`, which is neither an old parameter " + "({names}), an index into them, nor a `=literal`" +) +SIGNATURE_MAPPING_BAD_INDEX = ( + "The mapping feeds {name} from index {index}, but only {count} old " + "parameter(s) exist" +) +SIGNATURE_MAPPING_EMPTY_LITERAL = "The mapping gives {name} an empty literal" +SIGNATURE_MAPPING_DUPLICATE = ( + "Old parameter {old} cannot feed both {first} and {second}" +) +SIGNATURE_LITERAL_MISMATCH = ( + "{literal} does not fit the declared type of {name} ({annotation})" +) +SIGNATURE_BAD_MAP = "Mapping entries take the form NEW=SOURCE, got `{entry}`" +SIGNATURE_SITE_NO_LOCATION = "site carries no location ({resolution})" +SIGNATURE_SITE_GUESSED = ( + "site was resolved by {resolution}; pass allow_heuristic to rewrite it" +) +SIGNATURE_SITE_NOT_PYTHON = "{path} is not Python" +SIGNATURE_SITE_UNREADABLE_FILE = "file cannot be read ({error})" +SIGNATURE_SITE_NO_CALL = "no call found at the recorded position" +SIGNATURE_SITE_UNREADABLE = "arguments cannot be read positionally: `{text}`" +SIGNATURE_SITE_UNKNOWN_KEYWORD = "passes keyword `{name}`, which is not declared" +SIGNATURE_SITE_DUPLICATE = "passes `{name}` more than once" +SIGNATURE_SITE_TOO_MANY = ( + "passes {given} positional arguments but {declared} are declared" +) +SIGNATURE_SITE_NO_VALUE = "no value for `{name}`; map it or give it a default" +SIGNATURE_SITE_OVERLAPS = "its arguments overlap another edit of this change" +SIGNATURE_PLANNED = "{count} site(s) would be rewritten, {skipped} left unmapped" +SIGNATURE_PARSE_FAILED = "Signature change rolled back: {files} would no longer parse" +SIGNATURE_CONTRACT_FAILED = ( + "Signature change rolled back, postcondition failed: {reasons}" +) +SIGNATURE_ROLLBACK_REFUSED = ( + "Signature change kept: its postcondition failed ({reasons}) but a later " + "edit was recorded on top of it, so it was not rolled back; undo the later " + "edit and rerun" +) +SIGNATURE_ROLLBACK_UNMEASURED = ( + "Signature change 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" +) +SIGNATURE_CONTRACT_UNMEASURED = ( + "Signature change applied, but its postcondition could not be measured: {error}" +) # Postcondition contract (issue #1531). CONTRACT_OP_RENAME = "rename" CONTRACT_OP_CHANGE_SIGNATURE = "change_signature" diff --git a/codebase_rag/constants/core.py b/codebase_rag/constants/core.py index a208b45eeb..4909cdf8ed 100644 --- a/codebase_rag/constants/core.py +++ b/codebase_rag/constants/core.py @@ -106,6 +106,7 @@ class EventType(StrEnum): CHAR_SPACE = " " SEPARATOR_COMMA_SPACE = ", " +SEPARATOR_SEMICOLON_SPACE = "; " PUNCTUATION_TYPES = (CHAR_PAREN_OPEN, CHAR_PAREN_CLOSE, CHAR_COMMA) REGEX_METHOD_CHAIN_SUFFIX = r"\)\.[^)]*$" diff --git a/codebase_rag/constants/graph.py b/codebase_rag/constants/graph.py index aac829b401..2353ea1ab7 100644 --- a/codebase_rag/constants/graph.py +++ b/codebase_rag/constants/graph.py @@ -60,6 +60,10 @@ # Rename report fields (issue #1532). KEY_SITES = "sites" KEY_AMBIGUOUS = "ambiguous" +# Change-signature report fields (issue #1533); `verdict` is shared +# with rename. +KEY_UNMAPPED = "unmapped" +KEY_VERDICT = "verdict" KEY_STRUCTURAL_DELTA = "structural_delta" KEY_DISPATCH_LITERAL = "dispatch_literal" diff --git a/codebase_rag/editing/__init__.py b/codebase_rag/editing/__init__.py index 3786e8b039..66bd2f8214 100644 --- a/codebase_rag/editing/__init__.py +++ b/codebase_rag/editing/__init__.py @@ -1,5 +1,7 @@ """Graph-aware editing primitives: span-preserving patchers (issue #1529), -transactions (issue #1528) and import rewriting (issue #1530).""" +transactions (issue #1528), import rewriting (issue #1530) and the graph +operations built on them: rename (issue #1532) and change_signature (issue +#1533).""" from .contract import ( Expectation, diff --git a/codebase_rag/editing/contract.py b/codebase_rag/editing/contract.py index 0e3b974499..495a099df3 100644 --- a/codebase_rag/editing/contract.py +++ b/codebase_rag/editing/contract.py @@ -86,12 +86,15 @@ def rename_expectation( ) -def change_signature_expectation(unmapped: Iterable[str]) -> Expectation: +def change_signature_expectation( + unmapped: Iterable[str], heuristic_allowed: bool = False +) -> Expectation: return Expectation( operation=cs.CONTRACT_OP_CHANGE_SIGNATURE, unmapped=tuple(sorted(set(unmapped))), # A parameter added with a default changes no caller count. caller_count_unchanged=True, + heuristic_allowed=heuristic_allowed, ) @@ -111,6 +114,15 @@ def _site_key(site: Mapping[str, object]) -> str: return f"{site.get(cs.KEY_PATH)}:{site.get(cs.KEY_LINE)}" +def _call_key(site: Mapping[str, object]) -> str: + """A call's identity for the signature checks. + + Two calls can share a line (`helper(1) + helper(2)`), and rewriting or + listing one must not cover the other, so the column is part of the key. + """ + return f"{_site_key(site)}:{site.get(cs.KEY_COL)}" + + def _carried_by_ancestor( pair: tuple[str, str], expected: Iterable[tuple[str, str]] ) -> bool: @@ -200,7 +212,7 @@ def _changed_site_fault( loops plus these three verdicts put that function one point over the threshold. The decision is per site and reads better named anyway. """ - key = _site_key(site) + key = _call_key(site) verdict = site["verdict"] if verdict == cs.DELTA_ARITY_OK or key in unmapped: return None @@ -232,7 +244,7 @@ def _check_sites_mapped( if fault := _changed_site_fault(site, unmapped, rewritten): bad.append(fault) for site in delta["arity_findings"]: - key = _site_key(site) + key = _call_key(site) if key not in unmapped: bad.append(f"{key} ({site['verdict']})") if bad: @@ -303,7 +315,9 @@ def verify( """Pass or fail the delta against the expectation, with reasons. `rewritten` lists the sites the operation rewrote as `(path:line, - resolution)`; `parse_failures` the files the transaction found not to + resolution)`, or `(path:line:col, resolution)` for a signature change, + whose per-site checks key on the column; `parse_failures` the files the + transaction found not to parse (the transaction refuses those itself, so a caller normally passes none, but the contract states the rule in one place). """ diff --git a/codebase_rag/editing/rename.py b/codebase_rag/editing/rename.py index 3257871d96..fb05b255d4 100644 --- a/codebase_rag/editing/rename.py +++ b/codebase_rag/editing/rename.py @@ -44,6 +44,7 @@ from .contract import Reingest, Verdict, measure, rename_expectation, verify from .imports import ANY_MODULE, ImportRewriter, ImportSite, SymbolMove, _imported from .patcher import Patcher, PatcherError, line_col_to_byte +from .sites import AMBIGUOUS, call_node_at, hierarchy from .transaction import ( EditTransaction, StagedTree, @@ -55,13 +56,6 @@ QueryFn = Callable[[str, PropertyDict | None], list[ResultRow]] -_AMBIGUOUS = frozenset( - { - cs.EdgeResolution.HEURISTIC.value, - cs.EdgeResolution.OVERLOAD.value, - cs.EdgeResolution.DYNAMIC.value, - } -) _IDENTIFIER_RE = r"(? list[str]: - """`qn` plus every method it overrides or is overridden by, transitively.""" - seen: list[str] = [qn] - frontier = [qn] - while frontier: - current = frontier.pop() - for row in graph_query.overrides(fetch_all, project, current): - other = row["qualified_name"] - if other not in seen: - seen.append(other) - frontier.append(other) - return seen - - def _name_token( source: bytes, language: cs.SupportedLanguage | None, @@ -193,39 +173,6 @@ def _name_token( return None -def _best_call_at( - root: Node, line: int, col: int, recorded_end: tuple[int, int] | None -) -> Node | None: - """The call node at (line, col) that the graph site refers to. - - Several calls can share a start point -- `helper(helper(1))`, - `helper(2).upper()`, and both links of `obj.helper(1).helper(2)` -- so the - right one is the call ending where the site recorded its end, or the - outermost when no end was recorded. - - Extracted from `_callee_span` to keep it under the cognitive complexity - limit (S3776). This walk is where that complexity lives: a loop with - three levels of nested branching, and nesting multiplies the cost. - Extracting the straight-line setup around it would not have helped. - """ - best: Node | None = None - stack: list[Node] = [root] - while stack: - node = stack.pop() - if node.start_point == (line - 1, col): - func = node.child_by_field_name(cs.FIELD_FUNCTION) - if func is not None: - if node.end_point == recorded_end: - return node - if best is None or ( - best.end_point != recorded_end and node.end_byte > best.end_byte - ): - best = node - if node.start_point[0] <= line - 1 <= node.end_point[0]: - stack.extend(node.children) - return best - - def _callee_span( source: bytes, language: cs.SupportedLanguage | None, @@ -253,7 +200,7 @@ def _callee_span( if end_line is not None and end_col is not None else None ) - best = _best_call_at(root, line, col, recorded_end) + best = call_node_at(root, line, col, recorded_end) if best is None: return None func = best.child_by_field_name(cs.FIELD_FUNCTION) @@ -610,11 +557,11 @@ def plan( """Collect everything a rename touches; refuse on ambiguity.""" if not re.fullmatch(r"[A-Za-z_]\w*", new_name): raise RenameRefused(cs.RENAME_BAD_NAME.format(name=new_name), [], []) - hierarchy = _hierarchy(self.fetch_all, self.project, qn) + members = hierarchy(self.fetch_all, self.project, qn) sites: list[RenameSite] = [] unlocatable: list[str] = [] old_name: str | None = None - for member in hierarchy: + for member in members: member_sites, member_unlocatable, member_name, _label = self._collect( member ) @@ -646,7 +593,7 @@ def plan( unlocatable, ) ambiguous = [ - s for s in sites if s.resolution in _AMBIGUOUS or s.resolution == _CHAIN + s for s in sites if s.resolution in AMBIGUOUS or s.resolution == _CHAIN ] if ambiguous and not allow_heuristic: raise RenameRefused( @@ -654,7 +601,7 @@ def plan( ambiguous, unlocatable, ) - for member in hierarchy: + for member in members: for site, module in self._import_sites(member, old_name): sites.append( RenameSite( @@ -681,7 +628,7 @@ def plan( ambiguous=tuple(ambiguous), unlocatable=tuple(unlocatable), doc_mentions=tuple(self._doc_mentions(old_name)), - hierarchy=tuple(hierarchy), + hierarchy=tuple(members), diff="", message=cs.RENAME_PLANNED.format(count=len(sites)), ) diff --git a/codebase_rag/editing/sites.py b/codebase_rag/editing/sites.py new file mode 100644 index 0000000000..abbea9426d --- /dev/null +++ b/codebase_rag/editing/sites.py @@ -0,0 +1,72 @@ +"""Graph-site helpers the edit operations share. + +`rename` (issue #1532) and `change_signature` (issue #1533) both walk a +method's override hierarchy and both need the call node a recorded site +refers to; keeping one copy means the two operations cannot disagree about +which call a `(line, col)` names. +""" + +from __future__ import annotations + +from tree_sitter import Node + +from .. import constants as cs +from .. import graph_query +from ..graph_query import QueryFn + +# Resolutions that bound a site by guesswork: an operation rewrites through +# them only when the caller accepts the risk with `allow_heuristic`. +AMBIGUOUS = frozenset( + { + cs.EdgeResolution.HEURISTIC.value, + cs.EdgeResolution.OVERLOAD.value, + cs.EdgeResolution.DYNAMIC.value, + } +) + + +def hierarchy(fetch_all: QueryFn, project: str, qn: str) -> list[str]: + """`qn` plus every method it overrides or is overridden by, transitively.""" + seen: list[str] = [qn] + frontier = [qn] + while frontier: + current = frontier.pop() + for row in graph_query.overrides(fetch_all, project, current): + other = row["qualified_name"] + if other not in seen: + seen.append(other) + frontier.append(other) + return seen + + +def call_node_at( + root: Node, line: int, col: int, recorded_end: tuple[int, int] | None +) -> Node | None: + """The call node at (line, col) that the graph site refers to. + + Several calls can share a start point -- `helper(helper(1))`, + `helper(2).upper()`, and both links of `obj.helper(1).helper(2)` -- so the + right one is the call ending where the site recorded its end, or the + outermost when no end was recorded. A recorded end that matches no call + names nothing: the position is stale, and rewriting a neighbour that + happens to share the start would be a guess. + """ + calls = _calls_starting_at(root, line - 1, col) + if recorded_end is not None: + return next((call for call in calls if call.end_point == recorded_end), None) + return max(calls, key=lambda call: call.end_byte, default=None) + + +def _calls_starting_at(root: Node, row: int, col: int) -> list[Node]: + calls: list[Node] = [] + stack: list[Node] = [root] + while stack: + node = stack.pop() + if ( + node.start_point == (row, col) + and node.child_by_field_name(cs.FIELD_FUNCTION) is not None + ): + calls.append(node) + if node.start_point[0] <= row <= node.end_point[0]: + stack.extend(node.children) + return calls diff --git a/codebase_rag/tests/test_edit_contract.py b/codebase_rag/tests/test_edit_contract.py index bc2946ac5f..044f391c86 100644 --- a/codebase_rag/tests/test_edit_contract.py +++ b/codebase_rag/tests/test_edit_contract.py @@ -68,12 +68,12 @@ def _dangling(path: str = "pkg/app.py", line: int = 5) -> dict[str, object]: } -def _site(path: str, line: int, verdict: str) -> dict[str, object]: +def _site(path: str, line: int, verdict: str, col: int = 11) -> dict[str, object]: return { "caller": "p.pkg.app.run", "path": path, "line": line, - "col": 11, + "col": col, "arg_count": 2, "kwarg_names": [], "declared_count": 1, @@ -383,6 +383,22 @@ def test_rename_refuses_silently_rewritten_heuristic_sites() -> None: assert verify(allowed, delta, rewritten=rewritten).ok +def test_change_signature_refuses_silently_rewritten_heuristic_sites() -> None: + # The same leave a rename carries: without it, `allow_heuristic` on a + # signature change rewrote the guessed site and the contract then undid + # the whole change for having done what it was told. + delta = _delta( + symbols={"added": [], "removed": [], "renamed": [], "changed": ["p.h"]} + ) + rewritten = [("pkg/app.py:5", "exact"), ("pkg/other.py:9", "heuristic")] + strict = verify(change_signature_expectation([]), delta, rewritten=rewritten) + assert strict.failures == ( + cs.CONTRACT_HEURISTIC_REWRITTEN.format(sites="pkg/other.py:9"), + ) + allowed = change_signature_expectation([], heuristic_allowed=True) + assert verify(allowed, delta, rewritten=rewritten).ok + + def test_change_signature_requires_every_site_mapped_or_listed() -> None: change = { "qualified_name": "p.pkg.util.helper", @@ -408,23 +424,61 @@ def test_change_signature_requires_every_site_mapped_or_listed() -> None: strict = verify(change_signature_expectation([]), delta) assert strict.failures == ( cs.CONTRACT_SITES_UNMAPPED.format( - sites="pkg/app.py:9 (possibly_missing), pkg/cli.py:3 (too_many)" + sites="pkg/app.py:9:11 (possibly_missing), pkg/cli.py:3:11 (too_many)" ), ) listed = verify( - change_signature_expectation(["pkg/app.py:9", "pkg/cli.py:3"]), delta + change_signature_expectation(["pkg/app.py:9:11", "pkg/cli.py:3:11"]), delta ) assert listed.ok # A site the operation rewrote is mapped by construction: a # `possibly_missing` there relies on a default the mapping supplied. rewritten = verify( - change_signature_expectation(["pkg/cli.py:3"]), + change_signature_expectation(["pkg/cli.py:3:11"]), delta, - rewritten=[("pkg/app.py:9", "exact")], + rewritten=[("pkg/app.py:9:11", "exact")], ) assert rewritten.ok +def test_change_signature_tells_two_calls_on_one_line_apart() -> None: + # `helper(1) + helper(2)`: rewriting or listing one call must not cover + # the other, so the site's identity carries its column. + change = { + "qualified_name": "p.pkg.util.helper", + "path": "pkg/util.py", + "before": ["a"], + "after": ["a", "b"], + "sites": [ + _site("pkg/app.py", 9, cs.DELTA_ARITY_POSSIBLY_MISSING, col=4), + _site("pkg/app.py", 9, cs.DELTA_ARITY_POSSIBLY_MISSING, col=20), + ], + } + delta = _delta( + symbols={ + "added": [], + "removed": [], + "renamed": [], + "changed": ["p.pkg.util.helper"], + }, + signature_changes=[change], + ) + one_rewritten = verify( + change_signature_expectation([]), + delta, + rewritten=[("pkg/app.py:9:4", "exact")], + ) + assert one_rewritten.failures == ( + cs.CONTRACT_SITES_UNMAPPED.format(sites="pkg/app.py:9:20 (possibly_missing)"), + ) + one_listed = verify( + change_signature_expectation(["pkg/app.py:9:4"]), + delta, + rewritten=[("pkg/app.py:9:20", "exact")], + ) + assert one_listed.ok + + def test_move_requires_no_new_cycle_and_updated_importers() -> None: expectation = move_expectation("p.pkg.util.helper", "p.pkg.core.helper") moved = [