From 258cc437e9df11d4689be9098924d4972768bf18 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Wed, 23 Sep 2026 00:23:10 +0100 Subject: [PATCH 1/8] feat(signature): read the header, bind call arguments, render the rewrite (#1533, 3/9) --- codebase_rag/editing/signature_bind.py | 491 +++++++++++++++++++++++++ 1 file changed, 491 insertions(+) create mode 100644 codebase_rag/editing/signature_bind.py diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py new file mode 100644 index 0000000000..8310d12d0e --- /dev/null +++ b/codebase_rag/editing/signature_bind.py @@ -0,0 +1,491 @@ +"""Reading a Python header with tree-sitter, binding call arguments to the +new parameter list and rendering the rewritten text, for `change_signature` +(issue #1533). Split from `signature.py`. +""" + +from __future__ import annotations + +import re +from collections.abc import Iterable, Mapping, Sequence + +from tree_sitter import Node + +from .. import constants as cs +from ..parsers.utils import _is_static_decorator +from .signature_spec import ( + _LITERAL_PREFIX, + ParamSpec, + SignatureRefused, + _Binding, + _Candidate, + _Edit, + _Header, + _Source, + _Unmapped, +) + + +def _text(node: Node, source: bytes) -> str: + return source[node.start_byte : node.end_byte].decode(cs.ENCODING_UTF8) + + +def _is_static(function: Node, source: bytes) -> bool: + """Decorated `@staticmethod`: every parameter is one the caller passes. + + Read the way the indexer reads decorators, by the last name, so the + qualified `@builtins.staticmethod` counts too. + """ + parent = function.parent + if parent is None or parent.type != cs.TS_PY_DECORATED_DEFINITION: + return False + return _is_static_decorator( + [ + _text(child, source) + for child in parent.children + if child.type == cs.TS_PY_DECORATOR + ] + ) + + +def _find_definition(root: Node, name: str, start: int, end: int) -> Node | None: + """The `function_definition` named `name` starting within lines start..end.""" + wanted = name.encode(cs.ENCODING_UTF8) + stack: list[Node] = [root] + while stack: + node = stack.pop() + if ( + node.type == cs.TS_PY_FUNCTION_DEFINITION + and start <= node.start_point[0] + 1 <= end + ): + ident = node.child_by_field_name(cs.FIELD_NAME) + if ident is not None and ident.text == wanted: + return node + if node.start_point[0] < end and node.end_point[0] >= start - 1: + stack.extend(reversed(node.children)) + return None + + +def _param_spec(node: Node, source: bytes) -> ParamSpec | None: + """A plain positional-or-keyword parameter, or None for anything else.""" + text = _text(node, source) + if node.type == cs.TS_PY_IDENTIFIER: + return ParamSpec(text, text, None, False) + if node.type == cs.TS_PY_TYPED_PARAMETER: + # `*args: int` is a typed_parameter over a splat pattern. + first = node.named_children[0] if node.named_children else None + annotation = node.child_by_field_name(cs.FIELD_TYPE) + if first is None or first.type != cs.TS_PY_IDENTIFIER or annotation is None: + return None + return ParamSpec(_text(first, source), text, _text(annotation, source), False) + if node.type in (cs.TS_PY_DEFAULT_PARAMETER, cs.TS_PY_TYPED_DEFAULT_PARAMETER): + name = node.child_by_field_name(cs.FIELD_NAME) + if name is None or name.type != cs.TS_PY_IDENTIFIER: + return None + annotation = node.child_by_field_name(cs.FIELD_TYPE) + return ParamSpec( + _text(name, source), + text, + _text(annotation, source) if annotation is not None else None, + True, + ) + return None + + +# --- renamed parameters in the body ---------------------------------------------- + +# Scopes of their own inside a function body. A comprehension binds only its +# `for` targets, so one that merely reads the parameter follows the rename; +# a nested function, lambda or class can bind the name in ways the body walk +# cannot see (an assignment there is a fresh local), so any use of the name +# inside one refuses. +_COMPREHENSIONS = frozenset( + { + cs.TS_PY_LIST_COMPREHENSION, + cs.TS_PY_SET_COMPREHENSION, + cs.TS_PY_DICTIONARY_COMPREHENSION, + cs.TS_PY_GENERATOR_EXPRESSION, + } +) +_OWN_SCOPES = frozenset( + {cs.TS_PY_FUNCTION_DEFINITION, cs.TS_PY_LAMBDA, cs.TS_PY_CLASS_DEFINITION} +) +_REBINDING_STATEMENTS = frozenset( + { + cs.TS_PY_GLOBAL_STATEMENT, + cs.TS_PY_NONLOCAL_STATEMENT, + cs.TS_PY_IMPORT_STATEMENT, + cs.TS_PY_IMPORT_FROM_STATEMENT, + } +) +_OPAQUE_TO_THE_WALK = _OWN_SCOPES | _REBINDING_STATEMENTS + + +def _is_a_reference(node: Node) -> bool: + """A bare identifier, not the `.attr` of an attribute or a keyword's name.""" + parent = node.parent + if parent is None: + return True + if parent.type == cs.TS_PY_ATTRIBUTE: + return parent.child_by_field_name(cs.TS_PY_FIELD_ATTRIBUTE) != node + if parent.type == cs.TS_PY_KEYWORD_ARGUMENT: + return parent.child_by_field_name(cs.FIELD_NAME) != node + return True + + +def _mentions(node: Node, name: bytes) -> bool: + stack = [node] + while stack: + current = stack.pop() + if current.type == cs.TS_PY_IDENTIFIER and current.text == name: + return True + stack.extend(current.children) + return False + + +def _comprehension_binds(node: Node, name: bytes) -> bool: + return any( + child.type == cs.TS_PY_FOR_IN_CLAUSE + and (left := child.child_by_field_name(cs.FIELD_LEFT)) is not None + and _mentions(left, name) + for child in node.children + ) + + +def _body_reads(header: _Header, name: str) -> bool: + """Whether the body READS `name`. + + Through `_is_a_reference`, so `obj.a` and `helper(a=1)` do not count: + an attribute's name and a keyword argument's label are identifiers + spelled the same as the parameter but are not uses of it, and + refusing on those blocked legitimate removals (CodeRabbit, PR #1533). + """ + body = header.function.child_by_field_name(cs.FIELD_BODY) + if body is None: + return False + wanted = name.encode(cs.ENCODING_UTF8) + stack = [body] + while stack: + node = stack.pop() + if ( + node.type == cs.TS_PY_IDENTIFIER + and node.text == wanted + and _is_a_reference(node) + ): + return True + stack.extend(node.children) + return False + + +def _body_references( + header: _Header, old: str, new: str, renamed_away: Iterable[str] +) -> list[tuple[int, int]]: + """Byte spans of the parameter `old` in the body, to become `new`. + + Refuses when the name is used inside a nested scope or re-bound by a + statement, since the walk cannot tell those uses from the parameter's, + and when `new` is already read in the body, since the parameter would + then shadow it. A name that is itself being renamed away does not count + as taken: swapping two parameters is a rename in each direction. + """ + body = header.function.child_by_field_name(cs.FIELD_BODY) + assert body is not None + wanted = old.encode(cs.ENCODING_UTF8) + taken = None if new in renamed_away else new.encode(cs.ENCODING_UTF8) + rebinds = cs.SIGNATURE_BODY_REBINDS.format(old=old, qn=header.qn, new=new) + shadows = cs.SIGNATURE_BODY_NAME_TAKEN.format(old=old, qn=header.qn, new=new) + spans: list[tuple[int, int]] = [] + stack: list[Node] = [body] + while stack: + node = stack.pop() + if node.type in _OPAQUE_TO_THE_WALK: + _refuse_mentions(node, wanted, taken, rebinds, shadows) + continue + if node.type in _COMPREHENSIONS and _comprehension_binds(node, wanted): + raise SignatureRefused(rebinds) + if node.type == cs.TS_PY_IDENTIFIER and _is_a_reference(node): + if node.text == wanted: + spans.append((node.start_byte, node.end_byte)) + elif node.text == taken: + raise SignatureRefused(shadows) + stack.extend(node.children) + return spans + + +def _refuse_mentions( + node: Node, wanted: bytes, taken: bytes | None, rebinds: str, shadows: str +) -> None: + """A nested scope or re-binding statement may mention neither name. + + The walk cannot tell a use of the old name there from the parameter's; + and a nested scope reading the new name would start reading the renamed + parameter instead of whatever it read before. + """ + if _mentions(node, wanted): + raise SignatureRefused(rebinds) + if taken is not None and _mentions(node, taken): + raise SignatureRefused(shadows) + + +# --- call sites ----------------------------------------------------------------- + + +def _bind_arguments( + args: Node, source: bytes, old: Sequence[ParamSpec] +) -> dict[int, _Binding]: + """Old parameter index -> the value this site passes for it.""" + names = [p.name for p in old] + children = args.named_children + positional_kinds = { + cs.TS_PY_KEYWORD_ARGUMENT, + cs.TS_PY_LIST_SPLAT, + cs.TS_PY_DICTIONARY_SPLAT, + cs.TS_COMMENT, + } + given = sum(1 for child in children if child.type not in positional_kinds) + if given > len(old): + raise _Unmapped( + cs.SIGNATURE_SITE_TOO_MANY.format(given=given, declared=len(old)) + ) + bound: dict[int, _Binding] = {} + positional = 0 + for position, child in enumerate(children): + text = _text(child, source) + if child.type in ( + cs.TS_PY_LIST_SPLAT, + cs.TS_PY_DICTIONARY_SPLAT, + cs.TS_COMMENT, + ): + raise _Unmapped(cs.SIGNATURE_SITE_UNREADABLE.format(text=text)) + if child.type == cs.TS_PY_KEYWORD_ARGUMENT: + key = child.child_by_field_name(cs.FIELD_NAME) + value = child.child_by_field_name(cs.FIELD_VALUE) + assert key is not None and value is not None + name = _text(key, source) + if name not in names: + raise _Unmapped(cs.SIGNATURE_SITE_UNKNOWN_KEYWORD.format(name=name)) + index = names.index(name) + if index in bound: + raise _Unmapped(cs.SIGNATURE_SITE_DUPLICATE.format(name=name)) + bound[index] = _Binding( + _text(value, source), True, position, (value.start_byte, value.end_byte) + ) + continue + bound[positional] = _Binding( + text, False, position, (child.start_byte, child.end_byte) + ) + positional += 1 + return bound + + +def _fold( + candidate: _Candidate, edits: Sequence[_Edit] +) -> tuple[dict[int, _Binding], list[_Edit]]: + """The site's bindings with the planned edits inside them applied. + + A recursive call carries the body rename in its arguments; a call nested + in another call's arguments carries the inner site's rewrite. Folding + those into the value text lets the enclosing site be one edit, and the + folded edits are returned so the caller can drop them from the plan. + Any other edit overlapping the argument list cannot be folded, and the + site is left unmapped. + """ + path = candidate.site.path + start, end = candidate.span + inside = [ + edit + for edit in edits + if edit.path == path and edit.span[0] < end and start < edit.span[1] + ] + folded: dict[int, _Binding] = {} + consumed: list[_Edit] = [] + for index, binding in candidate.bound.items(): + lo, hi = binding.span + nested = [e for e in inside if lo <= e.span[0] and e.span[1] <= hi] + if not nested: + folded[index] = binding + continue + raw = bytearray(binding.text.encode(cs.ENCODING_UTF8)) + for edit in sorted(nested, key=lambda e: e.span[0], reverse=True): + raw[edit.span[0] - lo : edit.span[1] - lo] = edit.text.encode( + cs.ENCODING_UTF8 + ) + folded[index] = binding._replace(text=raw.decode(cs.ENCODING_UTF8)) + consumed.extend(nested) + if len(consumed) != len(inside): + raise _Unmapped(cs.SIGNATURE_SITE_OVERLAPS) + return folded, consumed + + +def _render_arguments( + new: Sequence[ParamSpec], + sources: Sequence[_Source | None], + bound: Mapping[int, _Binding], +) -> str: + """The site's new argument list, parenthesised. + + Positional values keep positional form in the new order until a value + goes by keyword or a defaulted parameter is left out; from there every + later value is spelled by keyword, since it can no longer sit in its + slot. Values that were keywords keep their original relative order + under their new names; values forced to keyword form follow in the new + parameter order. + """ + positional: list[str] = [] + keywords: list[tuple[tuple[int, int], str]] = [] + forced = False + for order, (spec, source) in enumerate(zip(new, sources, strict=True)): + found = _value_at_site(spec, source, bound) + if found is None: + forced = True + continue + value, binding = found + if binding is not None and binding.keyword: + forced = True + keywords.append(((0, binding.position), f"{spec.name}={value}")) + elif forced: + keywords.append(((1, order), f"{spec.name}={value}")) + else: + positional.append(value) + keywords.sort(key=lambda item: item[0]) + parts = positional + [text for _rank, text in keywords] + return f"({cs.SEPARATOR_COMMA_SPACE.join(parts)})" + + +def _value_at_site( + spec: ParamSpec, source: _Source | None, bound: Mapping[int, _Binding] +) -> tuple[str, _Binding | None] | None: + """The value this site passes for `spec`, with the binding it came from. + + None when the parameter's default stands in; unmapped when there is + neither a value nor a default. + """ + if source is not None and source.literal is not None: + return source.literal, None + if source is not None and source.index in bound: + binding = bound[source.index] + return binding.text, binding + if spec.has_default: + return None + raise _Unmapped(cs.SIGNATURE_SITE_NO_VALUE.format(name=spec.name)) + + +def _render_site( + candidate: _Candidate, + new: Sequence[ParamSpec], + sources: Sequence[_Source | None], + edits: list[_Edit], +) -> _Edit | None: + """The site's edit, with any edit planned inside its arguments folded in. + + The folded edits leave the plan only once the site renders, so a site + the mapping cannot complete keeps the inner rewrites as they were. + """ + bound, consumed = _fold(candidate, edits) + text = _render_arguments(new, sources, bound) + for edit in consumed: + edits.remove(edit) + if not consumed and text == candidate.text: + return None + return _Edit(candidate.site.path, candidate.span, text) + + +def _check_hierarchy( + qn: str, headers: Sequence[_Header], old_names: Sequence[str] +) -> None: + """Every override declares the same parameter names as the definition.""" + for header in headers[1:]: + theirs = [p.name for p in header.params] + if theirs != list(old_names): + raise SignatureRefused( + cs.SIGNATURE_HIERARCHY_MISMATCH.format( + qn=qn, + member=header.qn, + theirs=cs.SEPARATOR_COMMA_SPACE.join(theirs), + ours=cs.SEPARATOR_COMMA_SPACE.join(old_names), + ) + ) + + +def _definition_edits( + header: _Header, + new: Sequence[ParamSpec], + carried: set[str], + renamed: Sequence[tuple[str, str]], +) -> list[_Edit]: + """The header's new parameter list, and every body reference renamed.""" + own = {p.name: p for p in header.params} + texts = [own[spec.name].text if spec.name in carried else spec.text for spec in new] + if header.receiver is not None: + texts.insert(0, header.receiver) + edits = [ + _Edit(header.path, header.span, f"({cs.SEPARATOR_COMMA_SPACE.join(texts)})") + ] + renamed_away = {old for old, _new in renamed} + for old_name, new_name in renamed: + # A default is evaluated at DEFINITION time, in the enclosing scope, + # so `def f(a, b=a)` renamed a->x writes `def f(x, b=a)` and raises + # NameError the moment the module loads. `_body_references` walks the + # body only, so it cannot see the reference (Copilot, PR #1533). + _refuse_default_reading(header, texts, old_name, new_name) + edits.extend( + _Edit(header.path, span, new_name) + for span in _body_references(header, old_name, new_name, renamed_away) + ) + return edits + + +def _refuse_default_reading( + header: _Header, texts: Sequence[str], old: str, new: str +) -> None: + """Refuse when a parameter's default reads the name being renamed.""" + pattern = re.compile(rf"\b{re.escape(old)}\b") + for text in texts: + name, sep, default = text.partition(_LITERAL_PREFIX) + if not sep or not pattern.search(default): + continue + raise SignatureRefused( + cs.SIGNATURE_DEFAULT_REFERENCES_RENAMED.format( + old=old, qn=header.qn, new=new, param=name.strip() + ) + ) + + +def _params_of(params: Node, source: bytes, qn: str) -> list[ParamSpec]: + """Every parameter as a plain spec, or a refusal naming the odd one.""" + specs: list[ParamSpec] = [] + for child in params.named_children: + spec = _param_spec(child, source) + if spec is None: + raise SignatureRefused( + cs.SIGNATURE_UNSUPPORTED_PARAMS.format(qn=qn, text=_text(child, source)) + ) + specs.append(spec) + return specs + + +def _take_receiver( + qn: str, label: object, specs: list[ParamSpec], node: Node, source: bytes +) -> str | None: + """Pop a method's `self`/`cls` off `specs`; a static method has none. + + A method whose first parameter is named anything else refuses: `this` + would be remapped as a parameter and every bound call would lose its + receiver. + + Staticness is decided BEFORE the name: `@staticmethod def f(self, x)` + is legal Python and its `self` is an ordinary parameter, so popping it + by name dropped a real one from the header and every call site's + binding shifted by one (Copilot, PR #1912). + """ + if label != cs.NodeLabel.METHOD or not specs: + return None + if _is_static(node, source): + return None + if specs[0].name in cs.PY_RECEIVER_NAMES: + return specs.pop(0).text + raise SignatureRefused( + cs.SIGNATURE_UNUSUAL_RECEIVER.format(qn=qn, name=specs[0].name) + ) + + +# --- the operation --------------------------------------------------------------- From 756e8bf335f39f601a3aa2851eaee5dcf2cff969 Mon Sep 17 00:00:00 2001 From: Vitali Avagyan Date: Tue, 29 Sep 2026 12:06:25 +0000 Subject: [PATCH 2/8] fix(signature): look a site's bound argument up with get for ty 0.0.84 --- codebase_rag/editing/signature_bind.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py index 8310d12d0e..1d08d3bfc1 100644 --- a/codebase_rag/editing/signature_bind.py +++ b/codebase_rag/editing/signature_bind.py @@ -361,8 +361,9 @@ def _value_at_site( """ if source is not None and source.literal is not None: return source.literal, None - if source is not None and source.index in bound: - binding = bound[source.index] + index = source.index if source is not None else None + binding = bound.get(index) if index is not None else None + if binding is not None: return binding.text, binding if spec.has_default: return None From 0a50e6b950f6563957102b1e8fe3812ae3841ce2 Mon Sep 17 00:00:00 2001 From: Vitali Avagyan Date: Tue, 29 Sep 2026 15:36:23 +0000 Subject: [PATCH 3/8] test(signature): cover header reading, argument binding and rendering Unit tests for signature_bind on real tree-sitter parses: plain parameter specs and refusals, receiver handling (static, classmethod, unusual name), body renames and the scopes that refuse them, binding a call's arguments to old indexes, rendering the new argument list, folding nested edits, hierarchy agreement and the definition's own edits. --- codebase_rag/tests/test_signature_bind.py | 378 ++++++++++++++++++++++ 1 file changed, 378 insertions(+) create mode 100644 codebase_rag/tests/test_signature_bind.py diff --git a/codebase_rag/tests/test_signature_bind.py b/codebase_rag/tests/test_signature_bind.py new file mode 100644 index 0000000000..44504e205a --- /dev/null +++ b/codebase_rag/tests/test_signature_bind.py @@ -0,0 +1,378 @@ +# Unit tests for the tree-sitter half of `change_signature` (issue #1533): +# reading a Python header, renaming a parameter in the body, binding a call +# site's arguments to the old parameters and rendering the new argument +# list. Each helper runs on a real tree-sitter parse of a small snippet. + +from __future__ import annotations + +import re +from collections.abc import Sequence + +import pytest +import tree_sitter_python as tspython +from tree_sitter import Language, Node, Parser + +from codebase_rag import constants as cs +from codebase_rag.editing.signature_bind import ( + _bind_arguments, + _body_reads, + _body_references, + _check_hierarchy, + _definition_edits, + _find_definition, + _params_of, + _render_arguments, + _render_site, + _take_receiver, +) +from codebase_rag.editing.signature_spec import ( + ParamSpec, + SignatureRefused, + SignatureSite, + _Candidate, + _Edit, + _Header, + _Source, + _Unmapped, +) + +_PARSER = Parser(Language(tspython.language())) +PATH = "m.py" + + +def _root(code: str) -> tuple[Node, bytes]: + source = code.encode() + return _PARSER.parse(source).root_node, source + + +def _function(root: Node, name: str) -> Node: + node = _find_definition(root, name, 1, root.end_point[0] + 1) + assert node is not None + return node + + +def _header( + code: str, name: str = "f", label: cs.NodeLabel = cs.NodeLabel.FUNCTION +) -> _Header: + root, source = _root(code) + function = _function(root, name) + params = function.child_by_field_name("parameters") + assert params is not None + specs = _params_of(params, source, name) + receiver = _take_receiver(name, label, specs, function, source) + return _Header( + qn=f"m.{name}", + path=PATH, + span=(params.start_byte, params.end_byte), + line=function.start_point[0] + 1, + col=function.start_point[1], + receiver=receiver, + params=specs, + function=function, + source=source, + ) + + +def _apply(source: bytes, edits: list[_Edit]) -> str: + out = bytearray(source) + for edit in sorted(edits, key=lambda e: e.span[0], reverse=True): + out[edit.span[0] : edit.span[1]] = edit.text.encode() + return out.decode() + + +def _calls(root: Node) -> list[Node]: + found: list[Node] = [] + stack = [root] + while stack: + node = stack.pop() + if node.type == "call": + found.append(node) + stack.extend(reversed(node.children)) + return found + + +def _candidate(code: str, old: list[ParamSpec], nth: int = 0) -> _Candidate: + root, source = _root(code) + call = _calls(root)[nth] + args = call.child_by_field_name("arguments") + assert args is not None + site = SignatureSite("call", PATH, call.start_point[0] + 1, 0, "m.g", "exact") + return _Candidate( + site, + (args.start_byte, args.end_byte), + source[args.start_byte : args.end_byte].decode(), + _bind_arguments(args, source, old), + ) + + +def _p(name: str, default: bool = False) -> ParamSpec: + return ParamSpec(name, f"{name}=0" if default else name, None, default) + + +# --- reading the header -------------------------------------------------------------- + + +def test_find_definition_matches_name_within_line_window() -> None: + root, _ = _root("def f():\n pass\n\nclass C:\n def f(self):\n pass\n") + top = _find_definition(root, "f", 1, 1) + method = _find_definition(root, "f", 5, 5) + assert top is not None and top.start_point[0] == 0 + assert method is not None and method.start_point[0] == 4 + assert _find_definition(root, "f", 2, 3) is None + assert _find_definition(root, "g", 1, 6) is None + + +def test_params_are_read_as_plain_specs() -> None: + header = _header("def f(a, b: int, c=1, d: str = 'x'):\n pass\n") + assert header.receiver is None + assert header.params == [ + ParamSpec("a", "a", None, False), + ParamSpec("b", "b: int", "int", False), + ParamSpec("c", "c=1", None, True), + ParamSpec("d", "d: str = 'x'", "str", True), + ] + + +@pytest.mark.parametrize( + ("params", "odd"), + [ + ("a, *args", "*args"), + ("a, **kw", "**kw"), + ("a, *, k", "*"), + ("a, /, b", "/"), + ("*args: int", "*args: int"), + ], +) +def test_non_plain_parameter_is_refused(params: str, odd: str) -> None: + with pytest.raises(SignatureRefused, match=f"`{re.escape(odd)}` is not a plain"): + _header(f"def f({params}):\n pass\n") + + +def test_method_receiver_is_taken_off_the_params() -> None: + code = "class C:\n def f(self, a):\n pass\n" + header = _header(code, label=cs.NodeLabel.METHOD) + assert header.receiver == "self" + assert [p.name for p in header.params] == ["a"] + + +def test_function_keeps_a_parameter_named_self() -> None: + header = _header("def f(self, a):\n pass\n") + assert header.receiver is None + assert [p.name for p in header.params] == ["self", "a"] + + +@pytest.mark.parametrize("decorator", ["staticmethod", "builtins.staticmethod"]) +def test_static_method_has_no_receiver(decorator: str) -> None: + code = f"class C:\n @{decorator}\n def f(self, a):\n pass\n" + header = _header(code, label=cs.NodeLabel.METHOD) + assert header.receiver is None + assert [p.name for p in header.params] == ["self", "a"] + + +def test_classmethod_receiver_is_taken() -> None: + code = "class C:\n @classmethod\n def f(cls, a):\n pass\n" + assert _header(code, label=cs.NodeLabel.METHOD).receiver == "cls" + + +def test_method_with_no_params_has_no_receiver() -> None: + code = "class C:\n def f():\n pass\n" + assert _header(code, label=cs.NodeLabel.METHOD).receiver is None + + +def test_unusual_receiver_name_is_refused() -> None: + code = "class C:\n def f(this, a):\n pass\n" + with pytest.raises(SignatureRefused, match="`this` is not"): + _header(code, label=cs.NodeLabel.METHOD) + + +# --- the body -------------------------------------------------------------------- + + +def test_body_reads_ignores_attribute_and_keyword_labels() -> None: + header = _header("def f(a, b):\n obj.a\n g(a=1)\n return b\n") + assert _body_reads(header, "b") + assert not _body_reads(header, "a") + + +def test_body_references_are_the_parameter_uses() -> None: + code = "def f(a):\n x = a + obj.a\n g(a=a)\n return [v for v in a]\n" + header = _header(code) + spans = _body_references(header, "a", "z", ()) + renamed = _apply(header.source, [_Edit(PATH, s, "z") for s in spans]) + assert renamed == ( + "def f(a):\n x = z + obj.a\n g(a=z)\n return [v for v in z]\n" + ) + + +@pytest.mark.parametrize( + "body", + [ + " def inner():\n return a\n", + " h = lambda: a\n", + " class K:\n y = a\n", + " global a\n", + " import a\n", + " return [1 for a in range(3)]\n", + ], +) +def test_use_the_walk_cannot_follow_is_refused(body: str) -> None: + header = _header(f"def f(a):\n{body}") + with pytest.raises(SignatureRefused, match="nested scope"): + _body_references(header, "a", "z", ()) + + +@pytest.mark.parametrize( + "body", + [" return z\n", " def inner():\n return z\n"], +) +def test_new_name_already_read_is_refused(body: str) -> None: + header = _header(f"def f(a):\n a\n{body}") + with pytest.raises(SignatureRefused, match="already used"): + _body_references(header, "a", "z", ()) + + +def test_nested_scope_not_mentioning_either_name_is_left_alone() -> None: + header = _header("def f(a):\n def inner(q):\n return q\n return a\n") + at = header.source.index(b"return a") + len("return ") + assert _body_references(header, "a", "z", ()) == [(at, at + 1)] + + +def test_swapping_two_names_is_not_a_clash() -> None: + header = _header("def f(a, b):\n return a - b\n") + at = header.source.index(b"a - b") + assert _body_references(header, "a", "b", ["a", "b"]) == [(at, at + 1)] + + +# --- binding a call site --------------------------------------------------------- + + +OLD = [_p("a"), _p("b"), _p("c", default=True)] + + +def test_positional_and_keyword_arguments_bind_to_old_indexes() -> None: + candidate = _candidate("g(1, c=3, b=2)\n", OLD) + assert {i: (b.text, b.keyword, b.position) for i, b in candidate.bound.items()} == { + 0: ("1", False, 0), + 2: ("3", True, 1), + 1: ("2", True, 2), + } + + +@pytest.mark.parametrize( + ("call", "reason"), + [ + ("g(1, 2, 3, 4)\n", "4 positional arguments but 3"), + ("g(*xs)\n", "cannot be read positionally"), + ("g(**kw)\n", "cannot be read positionally"), + ("g(1, # note\n 2)\n", "cannot be read positionally"), + ("g(1, d=2)\n", "keyword `d`"), + ("g(1, a=2)\n", "`a` more than once"), + ], +) +def test_unreadable_site_is_unmapped(call: str, reason: str) -> None: + with pytest.raises(_Unmapped, match=reason): + _candidate(call, OLD) + + +# --- rendering a call site ------------------------------------------------------- + + +def _render(call: str, new: list[ParamSpec], sources: Sequence[_Source | None]) -> str: + return _render_arguments(new, sources, _candidate(call, OLD).bound) + + +def test_reordered_positional_values_stay_positional() -> None: + new = [_p("b"), _p("a")] + assert _render("g(1, 2)\n", new, [_Source(index=1), _Source(index=0)]) == "(2, 1)" + + +def test_values_after_an_omitted_default_go_by_keyword() -> None: + new = [_p("a"), _p("c", default=True), _p("b")] + sources = [_Source(index=0), _Source(index=2), _Source(index=1)] + assert _render("g(1, 2)\n", new, sources) == "(1, b=2)" + + +def test_keywords_keep_their_order_and_literals_are_inserted() -> None: + new = [_p("a"), _p("n"), _p("x"), _p("y")] + sources = [ + _Source(index=0), + _Source(literal="'lit'"), + _Source(index=2), + _Source(index=1), + ] + assert _render("g(1, c=3, b=2)\n", new, sources) == "(1, 'lit', x=3, y=2)" + + +def test_missing_value_without_default_is_unmapped() -> None: + with pytest.raises(_Unmapped, match="no value for `n`"): + _render("g(1, 2)\n", [_p("a"), _p("n")], [_Source(index=0), None]) + + +def test_unchanged_site_renders_no_edit() -> None: + candidate = _candidate("g(1, 2)\n", OLD) + sources = [_Source(index=0), _Source(index=1), _Source(index=2)] + assert _render_site(candidate, OLD, sources, []) is None + + +def test_edit_nested_in_an_argument_is_folded_into_the_site() -> None: + code = "g(a, g(1, 2))\n" + outer = _candidate(code, OLD, nth=0) + inner = _candidate(code, OLD, nth=1) + new = [_p("b"), _p("a")] + sources: list[_Source | None] = [_Source(index=1), _Source(index=0)] + body_rename = _Edit(PATH, (2, 3), "z") + inner_edit = _render_site(inner, new, sources, []) + assert inner_edit is not None and inner_edit.text == "(2, 1)" + edits = [body_rename, inner_edit] + outer_edit = _render_site(outer, new, sources, edits) + assert outer_edit is not None + assert outer_edit.text == "(g(2, 1), z)" + assert edits == [] # both folded edits left the plan + + +def test_edit_straddling_an_argument_leaves_the_site_unmapped() -> None: + candidate = _candidate("g(1, 2)\n", OLD) + straddling = _Edit(PATH, (2, 5), "x") + edits = [straddling] + with pytest.raises(_Unmapped, match="overlap"): + _render_site(candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], edits) + assert edits == [straddling] + + +def test_edit_in_another_file_is_not_folded() -> None: + candidate = _candidate("g(1, 2)\n", OLD) + elsewhere = _Edit("other.py", (2, 3), "x") + edit = _render_site(candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], []) + assert edit == _Edit(PATH, candidate.span, "(2, 1)") + assert _render_site( + candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], [elsewhere] + ) == _Edit(PATH, candidate.span, "(2, 1)") + + +# --- the definition and its hierarchy -------------------------------------------- + + +def test_hierarchy_must_declare_the_same_names() -> None: + base = _header("def f(a, b):\n pass\n") + same = _header("def f(a, b=1):\n pass\n") + other = _header("def f(a, c):\n pass\n") + _check_hierarchy("m.f", [base, same], ["a", "b"]) + with pytest.raises(SignatureRefused, match=r"declares \(a, c\)"): + _check_hierarchy("m.f", [base, same, other], ["a", "b"]) + + +def test_definition_edits_rewrite_header_and_body() -> None: + code = "class C:\n def f(self, a: int, b=2):\n return a + b\n" + header = _header(code, label=cs.NodeLabel.METHOD) + new = [ParamSpec("z", "z", None, False), header.params[1], _p("q", True)] + edits = _definition_edits(header, new, {"b"}, [("a", "z")]) + assert _apply(header.source, edits) == ( + "class C:\n def f(self, z, b=2, q=0):\n return z + b\n" + ) + + +def test_definition_edits_refuse_a_default_reading_the_renamed_name() -> None: + header = _header("def f(a, b=a):\n return b\n") + new = [ParamSpec("x", "x", None, False), header.params[1]] + with pytest.raises(SignatureRefused, match="default of `b` reads `a`"): + _definition_edits(header, new, {"b"}, [("a", "x")]) From f53049a367af4edf6baebe9a4751b51014a8c457 Mon Sep 17 00:00:00 2001 From: vitali87 Date: Mon, 5 Oct 2026 09:48:32 +0000 Subject: [PATCH 4/8] test(signature): split composite assertions Sonar flags Each assertion now checks one thing, and the pytest.raises blocks hold only the call expected to raise. --- codebase_rag/tests/test_signature_bind.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/codebase_rag/tests/test_signature_bind.py b/codebase_rag/tests/test_signature_bind.py index 44504e205a..26c4b48afd 100644 --- a/codebase_rag/tests/test_signature_bind.py +++ b/codebase_rag/tests/test_signature_bind.py @@ -116,8 +116,10 @@ def test_find_definition_matches_name_within_line_window() -> None: root, _ = _root("def f():\n pass\n\nclass C:\n def f(self):\n pass\n") top = _find_definition(root, "f", 1, 1) method = _find_definition(root, "f", 5, 5) - assert top is not None and top.start_point[0] == 0 - assert method is not None and method.start_point[0] == 4 + assert top is not None + assert top.start_point[0] == 0 + assert method is not None + assert method.start_point[0] == 4 assert _find_definition(root, "f", 2, 3) is None assert _find_definition(root, "g", 1, 6) is None @@ -304,8 +306,10 @@ def test_keywords_keep_their_order_and_literals_are_inserted() -> None: def test_missing_value_without_default_is_unmapped() -> None: + new = [_p("a"), _p("n")] + sources = [_Source(index=0), None] with pytest.raises(_Unmapped, match="no value for `n`"): - _render("g(1, 2)\n", [_p("a"), _p("n")], [_Source(index=0), None]) + _render("g(1, 2)\n", new, sources) def test_unchanged_site_renders_no_edit() -> None: @@ -322,7 +326,8 @@ def test_edit_nested_in_an_argument_is_folded_into_the_site() -> None: sources: list[_Source | None] = [_Source(index=1), _Source(index=0)] body_rename = _Edit(PATH, (2, 3), "z") inner_edit = _render_site(inner, new, sources, []) - assert inner_edit is not None and inner_edit.text == "(2, 1)" + assert inner_edit is not None + assert inner_edit.text == "(2, 1)" edits = [body_rename, inner_edit] outer_edit = _render_site(outer, new, sources, edits) assert outer_edit is not None @@ -334,8 +339,10 @@ def test_edit_straddling_an_argument_leaves_the_site_unmapped() -> None: candidate = _candidate("g(1, 2)\n", OLD) straddling = _Edit(PATH, (2, 5), "x") edits = [straddling] + new = [_p("b"), _p("a")] + sources = [_Source(1), _Source(0)] with pytest.raises(_Unmapped, match="overlap"): - _render_site(candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], edits) + _render_site(candidate, new, sources, edits) assert edits == [straddling] From 73ad607eeb7169eae14ac3cc1672bf8742444d7e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 22:23:53 +0000 Subject: [PATCH 5/8] fix(signature): keep a site's arguments in the order it wrote them Reordering parameters used to reorder positional values, so `(a, b)` to `(b, a)` turned `f(first(), second())` into `f(second(), first())` and ran `second()` first. Values now stay positional only while they keep the order the site wrote them; the rest go by keyword in that order. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah --- codebase_rag/editing/signature_bind.py | 73 ++++++++++++++++++----- codebase_rag/editing/signature_spec.py | 2 +- codebase_rag/tests/test_signature_bind.py | 41 +++++++++++-- 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py index 1d08d3bfc1..b30b27a892 100644 --- a/codebase_rag/editing/signature_bind.py +++ b/codebase_rag/editing/signature_bind.py @@ -323,34 +323,77 @@ def _render_arguments( ) -> str: """The site's new argument list, parenthesised. - Positional values keep positional form in the new order until a value - goes by keyword or a defaulted parameter is left out; from there every - later value is spelled by keyword, since it can no longer sit in its - slot. Values that were keywords keep their original relative order - under their new names; values forced to keyword form follow in the new - parameter order. + Python evaluates a call's arguments left to right as written, so the + site's own values must come out in the order the site wrote them: + otherwise reordering `(a, b)` to `(b, a)` would run `second()` before + `first()` in `f(first(), second())`. A leading run of values stays + positional (see `_positional_run`); every other value is spelled by + keyword, in the order the site wrote it. A mapped literal is the same + text at every site: it keeps its slot inside the run and otherwise + follows the site's values, in the new parameter order. """ + values = [ + _value_at_site(spec, source, bound) + for spec, source in zip(new, sources, strict=True) + ] + run = _positional_run(values) positional: list[str] = [] keywords: list[tuple[tuple[int, int], str]] = [] - forced = False - for order, (spec, source) in enumerate(zip(new, sources, strict=True)): - found = _value_at_site(spec, source, bound) + for order, (spec, found) in enumerate(zip(new, values, strict=True)): if found is None: - forced = True continue value, binding = found - if binding is not None and binding.keyword: - forced = True + if order < run: + positional.append(value) + elif binding is not None: keywords.append(((0, binding.position), f"{spec.name}={value}")) - elif forced: - keywords.append(((1, order), f"{spec.name}={value}")) else: - positional.append(value) + keywords.append(((1, order), f"{spec.name}={value}")) keywords.sort(key=lambda item: item[0]) parts = positional + [text for _rank, text in keywords] return f"({cs.SEPARATOR_COMMA_SPACE.join(parts)})" +def _written_at(found: tuple[str, _Binding | None] | None) -> int | None: + """Where the site wrote this value; None for a default or a literal.""" + if found is None: + return None + _value, binding = found + return binding.position if binding is not None else None + + +def _positional_run(values: Sequence[tuple[str, _Binding | None] | None]) -> int: + """How many leading values can stay positional without reordering the site. + + The run ends at an omitted default, at a value that was a keyword, and + before a value the site wrote ahead of one already in the run. Keywords + follow the run, so it then ends before its first value written after + one of the values left for keywords. + """ + run = 0 + last = -1 + for found in values: + if found is None: + break + _value, binding = found + if binding is not None: + if binding.keyword or binding.position <= last: + break + last = binding.position + run += 1 + left = [ + at for at in (_written_at(found) for found in values[run:]) if at is not None + ] + if not left: + return run + earliest = min(left) + for index, found in enumerate(values[:run]): + at = _written_at(found) + if at is not None and at > earliest: + return index + return run + + def _value_at_site( spec: ParamSpec, source: _Source | None, bound: Mapping[int, _Binding] ) -> tuple[str, _Binding | None] | None: diff --git a/codebase_rag/editing/signature_spec.py b/codebase_rag/editing/signature_spec.py index 49cb2d1ea4..87ff0a71d0 100644 --- a/codebase_rag/editing/signature_spec.py +++ b/codebase_rag/editing/signature_spec.py @@ -107,7 +107,7 @@ class _Header(NamedTuple): class _Binding(NamedTuple): text: str keyword: bool - position: int # index among the site's arguments, for keyword order + position: int # index among the site's arguments: the order they evaluate in span: tuple[int, int] # byte span of the value, for edits nested in it diff --git a/codebase_rag/tests/test_signature_bind.py b/codebase_rag/tests/test_signature_bind.py index 26c4b48afd..7a4cfdd062 100644 --- a/codebase_rag/tests/test_signature_bind.py +++ b/codebase_rag/tests/test_signature_bind.py @@ -283,9 +283,38 @@ def _render(call: str, new: list[ParamSpec], sources: Sequence[_Source | None]) return _render_arguments(new, sources, _candidate(call, OLD).bound) -def test_reordered_positional_values_stay_positional() -> None: +def test_reordered_values_go_by_keyword_in_the_order_written() -> None: new = [_p("b"), _p("a")] - assert _render("g(1, 2)\n", new, [_Source(index=1), _Source(index=0)]) == "(2, 1)" + sources = [_Source(index=1), _Source(index=0)] + assert _render("g(1, 2)\n", new, sources) == "(a=1, b=2)" + + +def test_reordering_keeps_effectful_arguments_in_evaluation_order() -> None: + # Positional `(second(), first())` would run `second()` first. + new = [_p("b"), _p("a")] + sources = [_Source(index=1), _Source(index=0)] + rendered = _render("g(first(), second())\n", new, sources) + assert rendered == "(a=first(), b=second())" + + +def test_positional_run_ends_before_a_value_written_after_a_keyword() -> None: + # `c` keeps its slot in new order but was written after `b`, which + # cannot: leaving `c` positional would run it before `b`. + new = [_p("a"), _p("c"), _p("b")] + sources = [_Source(index=0), _Source(index=2), _Source(index=1)] + assert _render("g(1, 2, 3)\n", new, sources) == "(1, b=2, c=3)" + + +def test_values_in_written_order_stay_positional_around_a_literal() -> None: + new = [_p("a"), _p("n"), _p("b")] + sources = [_Source(index=0), _Source(literal="0"), _Source(index=1)] + assert _render("g(x(), y())\n", new, sources) == "(x(), 0, y())" + + +def test_a_literal_left_for_keywords_follows_the_site_values() -> None: + new = [_p("n"), _p("b"), _p("a")] + sources = [_Source(literal="0"), _Source(index=1), _Source(index=0)] + assert _render("g(x(), y())\n", new, sources) == "(0, a=x(), b=y())" def test_values_after_an_omitted_default_go_by_keyword() -> None: @@ -327,11 +356,11 @@ def test_edit_nested_in_an_argument_is_folded_into_the_site() -> None: body_rename = _Edit(PATH, (2, 3), "z") inner_edit = _render_site(inner, new, sources, []) assert inner_edit is not None - assert inner_edit.text == "(2, 1)" + assert inner_edit.text == "(a=1, b=2)" edits = [body_rename, inner_edit] outer_edit = _render_site(outer, new, sources, edits) assert outer_edit is not None - assert outer_edit.text == "(g(2, 1), z)" + assert outer_edit.text == "(a=z, b=g(a=1, b=2))" assert edits == [] # both folded edits left the plan @@ -350,10 +379,10 @@ def test_edit_in_another_file_is_not_folded() -> None: candidate = _candidate("g(1, 2)\n", OLD) elsewhere = _Edit("other.py", (2, 3), "x") edit = _render_site(candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], []) - assert edit == _Edit(PATH, candidate.span, "(2, 1)") + assert edit == _Edit(PATH, candidate.span, "(a=1, b=2)") assert _render_site( candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], [elsewhere] - ) == _Edit(PATH, candidate.span, "(2, 1)") + ) == _Edit(PATH, candidate.span, "(a=1, b=2)") # --- the definition and its hierarchy -------------------------------------------- From 01b5627789691ff17da2269c51b93c6392db23d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 22:43:39 +0000 Subject: [PATCH 6/8] fix(signature): reorder a site positionally when none of its values runs code Names and plain literals evaluate the same in any order while nothing runs between them, so a site passing only those keeps the positional reorder #1533 asks for: `helper(2, 'x')` becomes `helper('x', 2)`. A site with a call, attribute or f-string keeps its written order by keyword. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah --- codebase_rag/constants/ast_python.py | 7 ++ codebase_rag/editing/signature_bind.py | 88 ++++++++++++++++++----- codebase_rag/editing/signature_spec.py | 1 + codebase_rag/tests/test_signature_bind.py | 50 +++++++++---- 4 files changed, 113 insertions(+), 33 deletions(-) diff --git a/codebase_rag/constants/ast_python.py b/codebase_rag/constants/ast_python.py index b95fc64b2b..59669ca184 100644 --- a/codebase_rag/constants/ast_python.py +++ b/codebase_rag/constants/ast_python.py @@ -160,6 +160,13 @@ TS_FIELD_ARGUMENT = "argument" TS_PY_UNARY_OPERATOR = "unary_operator" TS_PY_CONCATENATED_STRING = "concatenated_string" +# Argument values that run no code when evaluated, so `change_signature` may +# reorder a call site made only of them (issue #1533). +TS_PY_INTEGER = "integer" +TS_PY_FLOAT = "float" +TS_PY_TRUE = "true" +TS_PY_FALSE = "false" +TS_PY_NONE = "none" # Read positions the binding scan of `python_name_binding_sites` recognises. TS_PY_SLICE = "slice" TS_PY_ASSERT_STATEMENT = "assert_statement" diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py index b30b27a892..1d872f5454 100644 --- a/codebase_rag/editing/signature_bind.py +++ b/codebase_rag/editing/signature_bind.py @@ -228,6 +228,34 @@ def _refuse_mentions( # --- call sites ----------------------------------------------------------------- +_INERT_VALUES = frozenset( + { + cs.TS_PY_IDENTIFIER, + cs.TS_PY_INTEGER, + cs.TS_PY_FLOAT, + cs.TS_PY_TRUE, + cs.TS_PY_FALSE, + cs.TS_PY_NONE, + } +) +_NUMBERS = frozenset({cs.TS_PY_INTEGER, cs.TS_PY_FLOAT}) + + +def _is_inert(node: Node) -> bool: + """A name or a plain literal: evaluating it runs no code. + + An f-string runs its interpolations, so only a string without one + counts; `-1` is a unary operator over a number. + """ + if node.type in _INERT_VALUES: + return True + if node.type == cs.TS_PY_STRING: + return all(child.type != cs.TS_PY_INTERPOLATION for child in node.children) + if node.type == cs.TS_PY_UNARY_OPERATOR: + operand = node.child_by_field_name(cs.TS_FIELD_ARGUMENT) + return operand is not None and operand.type in _NUMBERS + return False + def _bind_arguments( args: Node, source: bytes, old: Sequence[ParamSpec] @@ -267,11 +295,15 @@ def _bind_arguments( if index in bound: raise _Unmapped(cs.SIGNATURE_SITE_DUPLICATE.format(name=name)) bound[index] = _Binding( - _text(value, source), True, position, (value.start_byte, value.end_byte) + _text(value, source), + True, + position, + (value.start_byte, value.end_byte), + _is_inert(value), ) continue bound[positional] = _Binding( - text, False, position, (child.start_byte, child.end_byte) + text, False, position, (child.start_byte, child.end_byte), _is_inert(child) ) positional += 1 return bound @@ -323,20 +355,17 @@ def _render_arguments( ) -> str: """The site's new argument list, parenthesised. - Python evaluates a call's arguments left to right as written, so the - site's own values must come out in the order the site wrote them: - otherwise reordering `(a, b)` to `(b, a)` would run `second()` before - `first()` in `f(first(), second())`. A leading run of values stays - positional (see `_positional_run`); every other value is spelled by - keyword, in the order the site wrote it. A mapped literal is the same - text at every site: it keeps its slot inside the run and otherwise - follows the site's values, in the new parameter order. + A leading run of values stays positional (see `_positional_run`); + every other value is spelled by keyword, in the order the site wrote + it. A mapped literal is the same text at every site: it keeps its slot + inside the run and otherwise follows the site's values, in the new + parameter order. """ values = [ _value_at_site(spec, source, bound) for spec, source in zip(new, sources, strict=True) ] - run = _positional_run(values) + run = _positional_run(values, _order_matters(values)) positional: list[str] = [] keywords: list[tuple[tuple[int, int], str]] = [] for order, (spec, found) in enumerate(zip(new, values, strict=True)): @@ -362,13 +391,34 @@ def _written_at(found: tuple[str, _Binding | None] | None) -> int | None: return binding.position if binding is not None else None -def _positional_run(values: Sequence[tuple[str, _Binding | None] | None]) -> int: - """How many leading values can stay positional without reordering the site. +def _order_matters(values: Sequence[tuple[str, _Binding | None] | None]) -> bool: + """Whether evaluating the site's values in a new order could be observed. - The run ends at an omitted default, at a value that was a keyword, and - before a value the site wrote ahead of one already in the run. Keywords - follow the run, so it then ends before its first value written after - one of the values left for keywords. + Python evaluates a call's arguments left to right as written, so once + one of them runs code, reordering `(a, b)` to `(b, a)` positionally + would run `second()` before `first()` in `f(first(), second())`. Names + and plain literals read the same in any order while nothing runs + between them. + """ + for found in values: + if found is None: + continue + _value, binding = found + if binding is not None and not binding.inert: + return True + return False + + +def _positional_run( + values: Sequence[tuple[str, _Binding | None] | None], ordered: bool +) -> int: + """How many leading values can stay positional. + + The run ends at an omitted default and at a value that was a keyword. + When the site's order matters, it also ends before a value the site + wrote ahead of one already in the run, and, since keywords follow the + run, before its first value written after one of those left for + keywords. """ run = 0 last = -1 @@ -377,10 +427,12 @@ def _positional_run(values: Sequence[tuple[str, _Binding | None] | None]) -> int break _value, binding = found if binding is not None: - if binding.keyword or binding.position <= last: + if binding.keyword or (ordered and binding.position <= last): break last = binding.position run += 1 + if not ordered: + return run left = [ at for at in (_written_at(found) for found in values[run:]) if at is not None ] diff --git a/codebase_rag/editing/signature_spec.py b/codebase_rag/editing/signature_spec.py index 87ff0a71d0..e4efbb4a4b 100644 --- a/codebase_rag/editing/signature_spec.py +++ b/codebase_rag/editing/signature_spec.py @@ -109,6 +109,7 @@ class _Binding(NamedTuple): keyword: bool position: int # index among the site's arguments: the order they evaluate in span: tuple[int, int] # byte span of the value, for edits nested in it + inert: bool = False # a name or plain literal: evaluating it runs no code class _Edit(NamedTuple): diff --git a/codebase_rag/tests/test_signature_bind.py b/codebase_rag/tests/test_signature_bind.py index 7a4cfdd062..4ae8d4f9e8 100644 --- a/codebase_rag/tests/test_signature_bind.py +++ b/codebase_rag/tests/test_signature_bind.py @@ -283,18 +283,37 @@ def _render(call: str, new: list[ParamSpec], sources: Sequence[_Source | None]) return _render_arguments(new, sources, _candidate(call, OLD).bound) -def test_reordered_values_go_by_keyword_in_the_order_written() -> None: - new = [_p("b"), _p("a")] - sources = [_Source(index=1), _Source(index=0)] - assert _render("g(1, 2)\n", new, sources) == "(a=1, b=2)" +SWAP = [_p("b"), _p("a")] +SWAP_SOURCES = [_Source(index=1), _Source(index=0)] -def test_reordering_keeps_effectful_arguments_in_evaluation_order() -> None: - # Positional `(second(), first())` would run `second()` first. - new = [_p("b"), _p("a")] - sources = [_Source(index=1), _Source(index=0)] - rendered = _render("g(first(), second())\n", new, sources) - assert rendered == "(a=first(), b=second())" +@pytest.mark.parametrize( + ("call", "rendered"), + [ + ("g(1, 2)\n", "(2, 1)"), + ("g(x, 'y')\n", "('y', x)"), + ("g(-1, None)\n", "(None, -1)"), + ], +) +def test_reordered_names_and_literals_stay_positional(call: str, rendered: str) -> None: + assert _render(call, SWAP, SWAP_SOURCES) == rendered + + +@pytest.mark.parametrize( + ("call", "rendered"), + [ + # Positional `(second(), first())` would run `second()` first. + ("g(first(), second())\n", "(a=first(), b=second())"), + # `h()` could rebind `x`, so the name keeps its place too. + ("g(x, h())\n", "(a=x, b=h())"), + ("g(f'{x()}', 2)\n", "(a=f'{x()}', b=2)"), + ("g(o.attr, 2)\n", "(a=o.attr, b=2)"), + ], +) +def test_reordering_code_that_runs_keeps_the_written_order( + call: str, rendered: str +) -> None: + assert _render(call, SWAP, SWAP_SOURCES) == rendered def test_positional_run_ends_before_a_value_written_after_a_keyword() -> None: @@ -302,7 +321,7 @@ def test_positional_run_ends_before_a_value_written_after_a_keyword() -> None: # cannot: leaving `c` positional would run it before `b`. new = [_p("a"), _p("c"), _p("b")] sources = [_Source(index=0), _Source(index=2), _Source(index=1)] - assert _render("g(1, 2, 3)\n", new, sources) == "(1, b=2, c=3)" + assert _render("g(x(), y(), z())\n", new, sources) == "(x(), b=y(), c=z())" def test_values_in_written_order_stay_positional_around_a_literal() -> None: @@ -356,11 +375,12 @@ def test_edit_nested_in_an_argument_is_folded_into_the_site() -> None: body_rename = _Edit(PATH, (2, 3), "z") inner_edit = _render_site(inner, new, sources, []) assert inner_edit is not None - assert inner_edit.text == "(a=1, b=2)" + assert inner_edit.text == "(2, 1)" edits = [body_rename, inner_edit] outer_edit = _render_site(outer, new, sources, edits) assert outer_edit is not None - assert outer_edit.text == "(a=z, b=g(a=1, b=2))" + # The outer site passes a call, so its values keep their written order. + assert outer_edit.text == "(a=z, b=g(2, 1))" assert edits == [] # both folded edits left the plan @@ -379,10 +399,10 @@ def test_edit_in_another_file_is_not_folded() -> None: candidate = _candidate("g(1, 2)\n", OLD) elsewhere = _Edit("other.py", (2, 3), "x") edit = _render_site(candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], []) - assert edit == _Edit(PATH, candidate.span, "(a=1, b=2)") + assert edit == _Edit(PATH, candidate.span, "(2, 1)") assert _render_site( candidate, [_p("b"), _p("a")], [_Source(1), _Source(0)], [elsewhere] - ) == _Edit(PATH, candidate.span, "(a=1, b=2)") + ) == _Edit(PATH, candidate.span, "(2, 1)") # --- the definition and its hierarchy -------------------------------------------- From 43c457f8b0bee3caa3a4046e3ba7f5b3f85a6f51 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 23:25:25 +0000 Subject: [PATCH 7/8] refactor(signature): split the positional-run rule into its two passes Sonar flags `_positional_run` over the cognitive-complexity limit; the leading run and the cut before keyword values are now separate helpers, with the same behaviour. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah --- codebase_rag/editing/signature_bind.py | 31 +++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py index 1d872f5454..2fb4380bb7 100644 --- a/codebase_rag/editing/signature_bind.py +++ b/codebase_rag/editing/signature_bind.py @@ -420,19 +420,30 @@ def _positional_run( run, before its first value written after one of those left for keywords. """ - run = 0 + run = _leading_run(values, ordered) + return _before_keywords(values, run) if ordered else run + + +def _leading_run( + values: Sequence[tuple[str, _Binding | None] | None], ordered: bool +) -> int: last = -1 - for found in values: + for run, found in enumerate(values): if found is None: - break + return run _value, binding = found - if binding is not None: - if binding.keyword or (ordered and binding.position <= last): - break - last = binding.position - run += 1 - if not ordered: - return run + if binding is None: + continue + if binding.keyword or (ordered and binding.position <= last): + return run + last = binding.position + return len(values) + + +def _before_keywords( + values: Sequence[tuple[str, _Binding | None] | None], run: int +) -> int: + """`run`, cut before its first value written after one left for keywords.""" left = [ at for at in (_written_at(found) for found in values[run:]) if at is not None ] From b008970f74935fb9de82bfb017603f56f20cee4a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 16 Oct 2026 23:37:36 +0000 Subject: [PATCH 8/8] fix(signature): leave match-pattern labels alone when renaming a parameter A class pattern's keyword (`case Point(a=a)`) names an attribute of the subject, and the trailing parts of a dotted value pattern (`case Color.a:`) are attribute names too. The body rename treated both as uses of the parameter, so renaming `a` to `z` produced `case Point(z=z)`, which no longer matches objects with an `a` attribute. Only the capture and the head of a dotted name are references now; the same check stops those labels counting as reads of the new name. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y9fLCP6Y2cycV2iRvFM8Ah --- codebase_rag/editing/signature_bind.py | 12 +++++++- codebase_rag/tests/test_signature_bind.py | 34 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/codebase_rag/editing/signature_bind.py b/codebase_rag/editing/signature_bind.py index 2fb4380bb7..0d21d4662d 100644 --- a/codebase_rag/editing/signature_bind.py +++ b/codebase_rag/editing/signature_bind.py @@ -121,7 +121,13 @@ def _param_spec(node: Node, source: bytes) -> ParamSpec | None: def _is_a_reference(node: Node) -> bool: - """A bare identifier, not the `.attr` of an attribute or a keyword's name.""" + """A bare identifier, not a label that merely shares the spelling. + + The labels: the `.attr` of an attribute or of a dotted value pattern + (`case Color.a:`), a keyword argument's name, and a class pattern's + keyword (`case Point(a=a)`, whose first `a` names the subject's + attribute and whose second is a capture). + """ parent = node.parent if parent is None: return True @@ -129,6 +135,10 @@ def _is_a_reference(node: Node) -> bool: return parent.child_by_field_name(cs.TS_PY_FIELD_ATTRIBUTE) != node if parent.type == cs.TS_PY_KEYWORD_ARGUMENT: return parent.child_by_field_name(cs.FIELD_NAME) != node + if parent.type == cs.TS_PY_KEYWORD_PATTERN: + return parent.named_children[0] != node + if parent.type == cs.TS_PY_DOTTED_NAME: + return parent.named_children[0] == node return True diff --git a/codebase_rag/tests/test_signature_bind.py b/codebase_rag/tests/test_signature_bind.py index 4ae8d4f9e8..daeb3b989a 100644 --- a/codebase_rag/tests/test_signature_bind.py +++ b/codebase_rag/tests/test_signature_bind.py @@ -206,6 +206,40 @@ def test_body_references_are_the_parameter_uses() -> None: ) +def test_body_references_skip_pattern_labels() -> None: + code = ( + "def f(a):\n" + " match g():\n" + " case Point(a=a):\n" + " return a\n" + " case Color.a:\n" + " return None\n" + " case [a, *_]:\n" + " return a\n" + ) + header = _header(code) + spans = _body_references(header, "a", "z", ()) + renamed = _apply(header.source, [_Edit(PATH, s, "z") for s in spans]) + assert renamed == ( + "def f(a):\n" + " match g():\n" + " case Point(a=z):\n" + " return z\n" + " case Color.a:\n" + " return None\n" + " case [z, *_]:\n" + " return z\n" + ) + + +def test_body_reads_ignores_pattern_labels() -> None: + header = _header( + "def f(a):\n match a:\n case P(z=1) | C.z:\n pass\n" + ) + assert not _body_reads(header, "z") + assert _body_reads(header, "C") + + @pytest.mark.parametrize( "body", [