From d80da2cdc6cf19a36ae1902347c30562b34990bb Mon Sep 17 00:00:00 2001 From: davalillo <1905197+davalillo@users.noreply.github.com> Date: Mon, 21 Sep 2026 20:46:05 +0200 Subject: [PATCH] fix(tools): expand unmatched-group $!N backreferences to the empty string (#2070) * fix(tools): expand unmatched-group $!N backreferences to the empty string A $!N backreference in a regex-mode replacement template refers to the Nth group of the search expression. When the group exists but did not participate in the match (e.g. it sits inside an optional construct that was skipped), the expansion emitted the literal template text instead of the empty string. Observed in practice when an agent replaced 15 occurrences in an MQL header with a template containing EA_INPUT$!1(...): the group was optional and never participated, so every site came out containing the literal EA_INPUT$!1(...). A reference to a group the expression does not define at all now raises a clear ValueError instead of a raw IndexError - which also crashed literal-mode replacements whose template contained $!N, since literal mode compiles an escaped pattern without any groups. * chore(ci): re-trigger test workflow The previous run failed in native (macos-latest) on test_cpp_basic.py::test_get_document_symbols[cpp_ccls] ("Expected 'main' in document symbols, got: []"), a ccls language-server startup/indexing flake unrelated to this PR's changes (text_utils.py and its tests only). * fix(tools): pass literal-mode replacement through verbatim (no $!N expansion) ContentReplacer ran the $!N backreference expansion on the replacement template in both modes, so a literal-mode replacement whose template contained a $!N sequence (e.g. documenting the convention itself in a memory) failed with a backreference error instead of writing the text. This contradicted the tools' documented contract ('the replacement string (verbatim)' in literal mode) and the behavior of the dry-run path, where MultiFileContentReplacer.find_occurrences already gated the expansion on regex mode. The expansion is now gated on the mode, mirroring the dry-run path; the ambiguity validation still applies in both modes. The literal-mode test that asserted the crash is inverted accordingly and the changelog entry is corrected. --- CHANGELOG.md | 6 ++++ src/serena/util/text_utils.py | 39 ++++++++++++++++------- test/serena/test_text_utils.py | 57 +++++++++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f661bb3..8d518a05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,12 @@ Status of the `main` branch. Changes prior to the next official version change w including its project configuration, are left untouched (#2029) * Tools: + - Fix: `$!N` backreferences in regex-mode replacements expanded to the literal template text + (e.g. `EA_INPUT$!1(...)`) when the referenced group existed but did not participate in the + match (e.g. a group inside an optional construct that was skipped); unmatched groups now expand + to the empty string, and a reference to a group that the search expression does not define + raises a clear error instead of a raw `IndexError`. In literal mode, the replacement is now + used verbatim (`$!N` sequences need no escaping) instead of failing with a backreference error - Fix: the file-editing tools saved the edited file with `open(path, "w")`, which truncates it before the new content is complete, so a crash, an OOM kill or a full disk partway through the write could leave a source file empty or half-written. Saves now go through the same atomic diff --git a/src/serena/util/text_utils.py b/src/serena/util/text_utils.py index 986eed85..a926abec 100644 --- a/src/serena/util/text_utils.py +++ b/src/serena/util/text_utils.py @@ -406,13 +406,18 @@ class ContentReplacer: self.regex_multiline = regex_multiline @staticmethod - def _create_replacement_function(regex_pattern: str, repl_template: str, regex_flags: int) -> Callable[[re.Match], str]: + def _create_replacement_function( + regex_pattern: str, repl_template: str, regex_flags: int, expand_backrefs: bool + ) -> Callable[[re.Match], str]: """ Creates a replacement function that validates for ambiguity and handles backreferences. :param regex_pattern: The regex pattern being used for matching - :param repl_template: The replacement template with $!1, $!2, etc. for backreferences + :param repl_template: The replacement template; in regex mode, it may contain $!1, $!2, etc. for + backreferences; in literal mode, it is used verbatim :param regex_flags: The flags to use when searching (e.g., re.DOTALL | re.MULTILINE) + :param expand_backrefs: Whether $!N backreferences are expanded in the template; false in literal mode, + mirroring the mode gate in MultiFileContentReplacer.find_occurrences :return: A function suitable for use with re.sub() or re.subn() """ @@ -434,11 +439,19 @@ class ContentReplacer: "e.g. by matching specific context after the match, or try using the literal mode." ) - # Handle backreferences: replace $!1, $!2, etc. with actual matched groups + # in literal mode, the template is the final replacement; $!N sequences need no escaping + if not expand_backrefs: + return repl_template + + # Handle backreferences: replace $!1, $!2, etc. with actual matched groups; groups that + # exist but did not participate in the match expand to the empty string def expand_backreference(m: re.Match) -> str: group_num = int(m.group(1)) - group_value = match.group(group_num) - return group_value if group_value is not None else m.group(0) + try: + group_value = match.group(group_num) + except IndexError as e: + raise ValueError(f"Backreference $!{group_num} refers to a group that does not exist in the search expression") from e + return group_value if group_value is not None else "" result = re.sub(r"\$!(\d+)", expand_backreference, repl_template) return result @@ -458,8 +471,8 @@ class ContentReplacer: :param content: the content in which to perform the replacement :param needle: the search expression, which is either a literal string or a regular expression, depending on the mode - :param repl: the replacement string, which, in regex mode, may contain backreferences in the form of $!1, $!2, etc. to - refer to matched groups in the search expression + :param repl: the replacement string; in regex mode, it may contain backreferences in the form of $!1, $!2, etc. + to refer to matched groups in the search expression; in literal mode, it is used verbatim :return: the updated content after performing the replacement """ if self.mode == "literal": @@ -471,8 +484,8 @@ class ContentReplacer: regex_flags = (re.MULTILINE | re.DOTALL) if self.regex_multiline else 0 - # create replacement function with validation and backreference handling - repl_fn = self._create_replacement_function(regex, repl, regex_flags=regex_flags) + # create replacement function with ambiguity validation and, in regex mode, backreference handling + repl_fn = self._create_replacement_function(regex, repl, regex_flags=regex_flags, expand_backrefs=self.mode == "regex") # perform replacement updated_content, n = re.subn(regex, repl_fn, content, flags=regex_flags) @@ -548,8 +561,12 @@ class MultiFileContentReplacer: """Expands $!1, $!2, ... in the replacement template (same syntax as :class:`ContentReplacer`).""" def expand(m: re.Match) -> str: - group_value = match.group(int(m.group(1))) - return group_value if group_value is not None else m.group(0) + group_num = int(m.group(1)) + try: + group_value = match.group(group_num) + except IndexError as e: + raise ValueError(f"Backreference $!{group_num} refers to a group that does not exist in the search expression") from e + return group_value if group_value is not None else "" return re.sub(r"\$!(\d+)", expand, repl_template) diff --git a/test/serena/test_text_utils.py b/test/serena/test_text_utils.py index 865b71b4..6fbea535 100644 --- a/test/serena/test_text_utils.py +++ b/test/serena/test_text_utils.py @@ -3,7 +3,14 @@ from collections.abc import Callable import pytest from serena.util.file_proxy import FileCollection, FileProxy -from serena.util.text_utils import GlobMatcher, LineType, MultiFileContentReplacer, search_files, search_text +from serena.util.text_utils import ( + ContentReplacer, + GlobMatcher, + LineType, + MultiFileContentReplacer, + search_files, + search_text, +) class TestSearchText: @@ -656,3 +663,51 @@ class TestMultiFileContentReplacer: occ = replacer.find_occurrences([(path, content)], "old_pkg", "new_pkg")[0] with pytest.raises(AssertionError): replacer.apply_to_content("completely different content", [occ]) + + +class TestBackreferenceExpansion: + """$!N backreferences in regex-mode replacements refer to matched groups. A group that + exists but did not participate in the match (e.g. inside an optional construct that was + skipped) must expand to the empty string; a reference to a group that the search + expression does not define at all must fail with an error naming the problem instead of + a raw IndexError. Literal mode has no backreference expansion at all: the replacement is + used verbatim (observed in practice when an agent tried to document the $!N convention + itself and the literal-mode replacement crashed instead of writing the text). + """ + + def test_unmatched_group_expands_to_empty_string(self): + replacer = ContentReplacer(mode="regex", allow_multiple_occurrences=False) + needle = r"EA_INPUT(?:\((\w*)\))?" + + # the group participated and captured an empty string (empty parentheses) + assert replacer.replace("EA_INPUT()\n", needle, r"EA_INPUT$!1(...)") == "EA_INPUT(...)\n" + # the group did not participate at all (no parentheses) + assert replacer.replace("EA_INPUT\n", needle, r"EA_INPUT$!1(...)") == "EA_INPUT(...)\n" + + def test_matched_group_expands_to_its_value(self): + replacer = ContentReplacer(mode="regex", allow_multiple_occurrences=False) + assert replacer.replace("id=alpha", r"id=(\w+)", r"[$!1]") == "[alpha]" + + def test_nonexistent_group_reference_raises_clear_error(self): + replacer = ContentReplacer(mode="regex", allow_multiple_occurrences=False) + with pytest.raises(ValueError, match="does not exist"): + replacer.replace("id=alpha", r"id=(\w+)", r"[$!2]") + + def test_literal_mode_repl_is_verbatim(self): + """Literal mode has no groups at all and no backreference expansion: a replacement + containing $!N sequences is written as-is instead of failing with a backreference error. + """ + replacer = ContentReplacer(mode="literal", allow_multiple_occurrences=False) + assert replacer.replace("literal needle", "literal needle", "$!1 stuff $!2") == "$!1 stuff $!2" + + def test_multi_file_replacer_expands_unmatched_group_to_empty_string(self): + replacer = MultiFileContentReplacer(mode="regex") + files = [("f.txt", "EA_INPUT\n")] + occurrences = replacer.find_occurrences(files, r"EA_INPUT(?:\((\w*)\))?", r"EA_INPUT$!1(...)") + assert [o.replacement for o in occurrences] == ["EA_INPUT(...)"] + + def test_multi_file_replacer_nonexistent_group_reference_raises_clear_error(self): + replacer = MultiFileContentReplacer(mode="regex") + files = [("f.txt", "id=alpha\n")] + with pytest.raises(ValueError, match="does not exist"): + replacer.find_occurrences(files, r"id=(\w+)", r"[$!2]")