From d8cad28246b4b192375871fdfd2d590245590cd3 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Wed, 28 May 2025 19:45:55 +0200 Subject: [PATCH 01/17] Typing --- src/serena/mcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/serena/mcp.py b/src/serena/mcp.py index 9508a33..a8c2191 100644 --- a/src/serena/mcp.py +++ b/src/serena/mcp.py @@ -163,7 +163,7 @@ def create_mcp_server_and_agent( class ProjectType(click.ParamType): name = "[PROJECT_NAME|PROJECT_PATH]" - def convert(self, value, param, ctx): + def convert(self, value: str, param: click.Parameter | None, ctx: click.Context | None) -> str: path = Path(value).resolve() if path.exists() and path.is_dir(): return str(path) # Valid path From b9764fce673357e00985bc35f96ed2abe248d71b Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Wed, 28 May 2025 19:46:50 +0200 Subject: [PATCH 02/17] Symbol: added find, insert, replace and delete by name. Also added delete by location. Renamed name to name_path --- src/serena/agent.py | 8 +-- src/serena/symbol.py | 117 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index 52b948c..8201286 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -1202,7 +1202,7 @@ class FindReferencingSymbolsTool(Tool): """ parsed_include_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in include_kinds] if include_kinds else None parsed_exclude_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in exclude_kinds] if exclude_kinds else None - symbols = self.symbol_manager.find_referencing_symbols( + symbols = self.symbol_manager.find_referencing_symbols_by_location( SymbolLocation(relative_path, line, column), include_body=include_body, include_kinds=parsed_include_kinds, @@ -1279,7 +1279,7 @@ class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): :param body: the new symbol body. Important: Provide the correct level of indentation (as the original body). Note that the first line must not be indented (i.e. no leading spaces). """ - self.symbol_manager.replace_body( + self.symbol_manager.replace_body_at_location( SymbolLocation(relative_path, line, column), body=body, ) @@ -1308,7 +1308,7 @@ class InsertAfterSymbolTool(Tool, ToolMarkerCanEdit): :param body: the body/content to be inserted """ location = SymbolLocation(relative_path, line, column) - self.symbol_manager.insert_after( + self.symbol_manager.insert_after_symbol_at_location( location, body=body, ) @@ -1337,7 +1337,7 @@ class InsertBeforeSymbolTool(Tool, ToolMarkerCanEdit): :param column: the column :param body: the body/content to be inserted """ - self.symbol_manager.insert_before( + self.symbol_manager.insert_before_symbol_at_location( SymbolLocation(relative_path, line, column), body=body, ) diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 3178d55..0616f2a 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -1,3 +1,4 @@ +import json import logging import os from collections.abc import Iterator, Sequence @@ -337,7 +338,7 @@ class SymbolManager: def find_by_name( self, - name: str, + name_path: str, include_body: bool = False, include_kinds: Sequence[SymbolKind] | None = None, exclude_kinds: Sequence[SymbolKind] | None = None, @@ -353,7 +354,9 @@ class SymbolManager: symbol_roots = self.lang_server.request_full_symbol_tree(within_relative_path=within_relative_path, include_body=include_body) for root in symbol_roots: symbols.extend( - Symbol(root).find(name, include_kinds=include_kinds, exclude_kinds=exclude_kinds, substring_matching=substring_matching) + Symbol(root).find( + name_path, include_kinds=include_kinds, exclude_kinds=exclude_kinds, substring_matching=substring_matching + ) ) return symbols @@ -373,6 +376,35 @@ class SymbolManager: return None def find_referencing_symbols( + self, + name_path: str, + relative_file_path: str, + include_body: bool = False, + include_kinds: Sequence[SymbolKind] | None = None, + exclude_kinds: Sequence[SymbolKind] | None = None, + ) -> tuple[Symbol, list[Symbol]] | None: + """ + Find all symbols that reference the symbol with the given name. + If multiple symbols fit the name (e.g. for variables that are overwritten), will use the first one. + """ + symbol_candidates = self.find_by_name(name_path, substring_matching=False, within_relative_path=relative_file_path) + if len(symbol_candidates) == 0: + log.warning(f"No symbol with name {name_path} found in file {relative_file_path}") + return None + if len(symbol_candidates) > 1: + log.error( + f"Found {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}." + f"May be an overwritten variable, in which case you can ignore this error. Proceeding with the first one. " + f"Found symbols for {name_path=} in {relative_file_path=}: \n" + f"" + ) + symbol = symbol_candidates[0] + referencing_symbols = self.find_referencing_symbols_by_location( + symbol.location, include_body=include_body, include_kinds=include_kinds, exclude_kinds=exclude_kinds + ) + return symbol, referencing_symbols + + def find_referencing_symbols_by_location( self, symbol_location: SymbolLocation, include_body: bool = False, @@ -434,7 +466,24 @@ class SymbolManager: with self._edited_file(location.relative_path): yield symbol - def replace_body(self, location: SymbolLocation, body: str) -> None: + def replace_body(self, name_path: str, relative_file_path: str, body: str) -> None: + """ + Replace the body of the symbol with the given name in the given file. + """ + symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) + if len(symbol_candidates) == 0: + raise ValueError(f"No symbol with name {name_path} found in file {relative_file_path}") + if len(symbol_candidates) > 1: + raise ValueError( + f"Found multiple {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}. " + "Will not replace the body of any of them, but you can use `replace_body_at_location`, the replace lines tool or other editing " + "tools to perform your edits. Their locations are: \n " + + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) + ) + symbol = symbol_candidates[0] + self.replace_body_at_location(symbol.location, body) + + def replace_body_at_location(self, location: SymbolLocation, body: str) -> None: """ Replace the body of the symbol at the given location with the given body @@ -454,7 +503,23 @@ class SymbolManager: self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - def insert_after(self, location: SymbolLocation, body: str) -> None: + def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str) -> None: + """ + Inserts content after the symbol with the given name in the given file. + """ + symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) + if len(symbol_candidates) == 0: + raise ValueError(f"No symbol with name {name_path} found in file {relative_file_path}") + if len(symbol_candidates) > 1: + raise ValueError( + f"Found multiple {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}. " + f"May be an overwritten variable, in which case you can ignore this error. Proceeding with the last one. " + f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) + ) + symbol = symbol_candidates[-1] + self.insert_after_symbol_at_location(symbol.location, body) + + def insert_after_symbol_at_location(self, location: SymbolLocation, body: str) -> None: """ Appends content after the given symbol @@ -472,7 +537,23 @@ class SymbolManager: assert location.relative_path is not None self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) - def insert_before(self, location: SymbolLocation, body: str) -> None: + def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str) -> None: + """ + Inserts content before the symbol with the given name in the given file. + """ + symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) + if len(symbol_candidates) == 0: + raise ValueError(f"No symbol with name {name_path} found in file {relative_file_path}") + if len(symbol_candidates) > 1: + raise ValueError( + f"Found multiple {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}. " + f"May be an overwritten variable, in which case you can ignore this error. Proceeding with the first one. " + f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) + ) + symbol = symbol_candidates[0] + self.insert_before_symbol_at_location(symbol.location, body) + + def insert_before_symbol_at_location(self, location: SymbolLocation, body: str) -> None: """ Inserts content before the given symbol @@ -512,3 +593,29 @@ class SymbolManager: start_pos = Position(line=start_line, character=0) end_pos = Position(line=end_line + 1, character=0) self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) + + def delete_symbol_at_location(self, location: SymbolLocation) -> None: + """ + Deletes the symbol at the given location. + """ + with self._edited_symbol_location(location) as symbol: + assert location.relative_path is not None + assert symbol.body_start_position is not None + assert symbol.body_end_position is not None + self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) + + def delete_symbol(self, name_path: str, relative_file_path: str) -> None: + """ + Deletes the symbol with the given name in the given file. + """ + symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) + if len(symbol_candidates) == 0: + raise ValueError(f"No symbol with name {name_path} found in file {relative_file_path}") + if len(symbol_candidates) > 1: + raise ValueError( + f"Found multiple {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}. " + "Will not delete any of them, but you can use `delete_symbol_at_location` or a corresponding tool to perform your edits. " + "Their locations are: \n " + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) + ) + symbol = symbol_candidates[0] + self.delete_symbol_at_location(symbol.location) From 3ba4c0c721fdd8a273be543e3b7ad4a0c2e76aa0 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Wed, 28 May 2025 23:52:50 +0200 Subject: [PATCH 03/17] Added dry-run mode for editing operations --- src/serena/symbol.py | 414 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 373 insertions(+), 41 deletions(-) diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 0616f2a..19cb46c 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -4,8 +4,9 @@ import os from collections.abc import Iterator, Sequence from contextlib import contextmanager from copy import copy -from dataclasses import asdict, dataclass -from typing import TYPE_CHECKING, Any, Self +from dataclasses import asdict, dataclass, field +from difflib import SequenceMatcher +from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, overload from sensai.util.string import ToStringMixin @@ -18,6 +19,151 @@ if TYPE_CHECKING: log = logging.getLogger(__name__) +class LineChange(NamedTuple): + """Represents a change to a specific line or range of lines.""" + + operation: Literal["insert", "delete", "replace"] + original_start: int + original_end: int + modified_start: int + modified_end: int + original_lines: list[str] + modified_lines: list[str] + + +@dataclass +class CodeDiff: + """ + Represents the difference between original and modified code. + Provides object-oriented access to diff information including line numbers. + """ + + relative_path: str + original_content: str + modified_content: str + _line_changes: list[LineChange] = field(init=False) + + def __post_init__(self) -> None: + """Compute the diff using difflib's SequenceMatcher.""" + original_lines = self.original_content.splitlines(keepends=True) + modified_lines = self.modified_content.splitlines(keepends=True) + + matcher = SequenceMatcher(None, original_lines, modified_lines) + self._line_changes = [] + + for tag, orig_start, orig_end, mod_start, mod_end in matcher.get_opcodes(): + if tag == "equal": + continue + if tag == "insert": + self._line_changes.append( + LineChange( + operation="insert", + original_start=orig_start, + original_end=orig_start, + modified_start=mod_start, + modified_end=mod_end, + original_lines=[], + modified_lines=modified_lines[mod_start:mod_end], + ) + ) + elif tag == "delete": + self._line_changes.append( + LineChange( + operation="delete", + original_start=orig_start, + original_end=orig_end, + modified_start=mod_start, + modified_end=mod_start, + original_lines=original_lines[orig_start:orig_end], + modified_lines=[], + ) + ) + elif tag == "replace": + self._line_changes.append( + LineChange( + operation="replace", + original_start=orig_start, + original_end=orig_end, + modified_start=mod_start, + modified_end=mod_end, + original_lines=original_lines[orig_start:orig_end], + modified_lines=modified_lines[mod_start:mod_end], + ) + ) + + @property + def line_changes(self) -> list[LineChange]: + """Get all line changes in the diff.""" + return self._line_changes + + @property + def has_changes(self) -> bool: + """Check if there are any changes.""" + return len(self._line_changes) > 0 + + @property + def added_lines(self) -> list[tuple[int, str]]: + """Get all added lines with their line numbers (0-based) in the modified file.""" + result = [] + for change in self._line_changes: + if change.operation in ("insert", "replace"): + for i, line in enumerate(change.modified_lines): + result.append((change.modified_start + i, line)) + return result + + @property + def deleted_lines(self) -> list[tuple[int, str]]: + """Get all deleted lines with their line numbers (0-based) in the original file.""" + result = [] + for change in self._line_changes: + if change.operation in ("delete", "replace"): + for i, line in enumerate(change.original_lines): + result.append((change.original_start + i, line)) + return result + + @property + def modified_line_numbers(self) -> list[int]: + """Get all line numbers (0-based) that were modified in the modified file.""" + line_nums: set[int] = set() + for change in self._line_changes: + if change.operation in ("insert", "replace"): + line_nums.update(range(change.modified_start, change.modified_end)) + return sorted(line_nums) + + @property + def affected_original_line_numbers(self) -> list[int]: + """Get all line numbers (0-based) that were affected in the original file.""" + line_nums: set[int] = set() + for change in self._line_changes: + if change.operation in ("delete", "replace"): + line_nums.update(range(change.original_start, change.original_end)) + return sorted(line_nums) + + def get_unified_diff(self, context_lines: int = 3) -> str: + """Get the unified diff as a string.""" + import difflib + + original_lines = self.original_content.splitlines(keepends=True) + modified_lines = self.modified_content.splitlines(keepends=True) + + diff = difflib.unified_diff( + original_lines, modified_lines, fromfile=f"a/{self.relative_path}", tofile=f"b/{self.relative_path}", n=context_lines + ) + return "".join(diff) + + def get_context_diff(self, context_lines: int = 3) -> str: + """Get the context diff as a string.""" + import difflib + + original_lines = self.original_content.splitlines(keepends=True) + modified_lines = self.modified_content.splitlines(keepends=True) + + diff = difflib.context_diff( + original_lines, modified_lines, fromfile=f"a/{self.relative_path}", tofile=f"b/{self.relative_path}", n=context_lines + ) + return "".join(diff) + + @dataclass class SymbolLocation: """ @@ -466,7 +612,41 @@ class SymbolManager: with self._edited_file(location.relative_path): yield symbol - def replace_body(self, name_path: str, relative_file_path: str, body: str) -> None: + def _get_code_file_content(self, relative_path: str) -> str: + """Get the content of a file using the language server.""" + return self.lang_server.language_server.retrieve_full_file_content(relative_path) + + @staticmethod + def _apply_text_edit(content: str, start_line: int, start_char: int, end_line: int, end_char: int, replacement: str) -> str: + """Apply a text edit to content and return the modified content.""" + lines = content.splitlines(keepends=True) + + # Handle edge cases + if not lines: + return replacement + + # Ensure we have enough lines + while len(lines) <= max(start_line, end_line): + lines.append("") + + # Extract the portion to keep before the edit + before_lines = lines[:start_line] + before_on_line = lines[start_line][:start_char] if start_line < len(lines) else "" + + # Extract the portion to keep after the edit + after_on_line = lines[end_line][end_char:] if end_line < len(lines) else "" + after_lines = lines[end_line + 1 :] if end_line + 1 < len(lines) else [] + + # Combine everything + result = before_lines + [before_on_line + replacement + after_on_line] + after_lines + + return "".join(result) + + @overload + def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Replace the body of the symbol with the given name in the given file. """ @@ -481,29 +661,60 @@ class SymbolManager: + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - self.replace_body_at_location(symbol.location, body) + return self.replace_body_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] - def replace_body_at_location(self, location: SymbolLocation, body: str) -> None: + @overload + def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Replace the body of the symbol at the given location with the given body :param location: the location of the symbol to replace. :param body: the new body + :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - with self._edited_symbol_location(location) as symbol: + + if dry_run: assert location.relative_path is not None + original_content = self._get_code_file_content(location.relative_path) + + # Find the symbol to get its body positions + symbol = self.find_by_location(location) + if symbol is None: + raise ValueError("Symbol not found/has no defined location within a file") + start_pos = symbol.body_start_position end_pos = symbol.body_end_position if start_pos is None or end_pos is None: raise ValueError(f"Symbol at {location} does not have a defined body range.") - # At this point, start_pos and end_pos are guaranteed to be Position objects - self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) - self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str) -> None: + # Apply the edit + modified_content = self._apply_text_edit( + original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"], body + ) + + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_symbol_location(location) as symbol: + assert location.relative_path is not None + start_pos = symbol.body_start_position + end_pos = symbol.body_end_position + if start_pos is None or end_pos is None: + raise ValueError(f"Symbol at {location} does not have a defined body range.") + self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) + self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) + return None + + @overload + def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Inserts content after the symbol with the given name in the given file. """ @@ -517,27 +728,55 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[-1] - self.insert_after_symbol_at_location(symbol.location, body) + return self.insert_after_symbol_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] - def insert_after_symbol_at_location(self, location: SymbolLocation, body: str) -> None: + @overload + def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Appends content after the given symbol :param location: the location of the symbol after which to add new lines :param body: the body of the entity to append + :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - with self._edited_symbol_location(location) as symbol: + + if dry_run: + assert location.relative_path is not None + original_content = self._get_code_file_content(location.relative_path) + + # Find the symbol to get its end position + symbol = self.find_by_location(location) + if symbol is None: + raise ValueError("Symbol not found/has no defined location within a file") + pos = symbol.body_end_position if pos is None: raise ValueError(f"Symbol at {location} does not have a defined end position.") - # At this point, pos is guaranteed to be a Position object - assert location.relative_path is not None - self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) - def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str) -> None: + # Apply the edit - insert at end position + modified_content = self._apply_text_edit(original_content, pos["line"], pos["character"], pos["line"], pos["character"], body) + + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_symbol_location(location) as symbol: + pos = symbol.body_end_position + if pos is None: + raise ValueError(f"Symbol at {location} does not have a defined end position.") + assert location.relative_path is not None + self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) + return None + + @overload + def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Inserts content before the symbol with the given name in the given file. """ @@ -551,62 +790,155 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - self.insert_before_symbol_at_location(symbol.location, body) + return self.insert_before_symbol_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] - def insert_before_symbol_at_location(self, location: SymbolLocation, body: str) -> None: + @overload + def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: """ Inserts content before the given symbol :param location: the location of the symbol before which to add new lines :param body: the body of the entity to insert + :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - with self._edited_symbol_location(location) as symbol: + + if dry_run: + assert location.relative_path is not None + original_content = self._get_code_file_content(location.relative_path) + + # Find the symbol to get its start position + symbol = self.find_by_location(location) + if symbol is None: + raise ValueError("Symbol not found/has no defined location within a file") + original_start_pos = symbol.body_start_position if original_start_pos is None: raise ValueError(f"Symbol at {location} does not have a defined start position.") - # At this point, original_start_pos is guaranteed to be a Position object, so copying is safe. - pos = copy(original_start_pos) - assert location.relative_path is not None - self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) - def insert_at_line(self, relative_path: str, line: int, content: str) -> None: + # Apply the edit - insert at start position + modified_content = self._apply_text_edit( + original_content, + original_start_pos["line"], + original_start_pos["character"], + original_start_pos["line"], + original_start_pos["character"], + body, + ) + + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_symbol_location(location) as symbol: + original_start_pos = symbol.body_start_position + if original_start_pos is None: + raise ValueError(f"Symbol at {location} does not have a defined start position.") + pos = copy(original_start_pos) + assert location.relative_path is not None + self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) + return None + + @overload + def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: bool = False) -> CodeDiff | None: """ Inserts content at the given line in the given file. :param line: the 0-based index of the line to insert content at :param content: the content to insert + :param dry_run: if True, return a CodeDiff instead of modifying the file """ - with self._edited_file(relative_path): - self.lang_server.insert_text_at_position(relative_path, line, 0, content) + if dry_run: + original_content = self._get_code_file_content(relative_path) - def delete_lines(self, relative_path: str, start_line: int, end_line: int) -> None: + # Apply the edit - insert at beginning of line + modified_content = self._apply_text_edit(original_content, line, 0, line, 0, content) + + return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_file(relative_path): + self.lang_server.insert_text_at_position(relative_path, line, 0, content) + return None + + @overload + def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[False] = False) -> None: ... + @overload + def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[True]) -> CodeDiff: ... + def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: bool = False) -> CodeDiff | None: """ Deletes lines in the given file. :param start_line: the 0-based index of the first line to delete (inclusive) :param end_line: the 0-based index of the last line to delete (inclusive) + :param dry_run: if True, return a CodeDiff instead of modifying the file """ - with self._edited_file(relative_path): - start_pos = Position(line=start_line, character=0) - end_pos = Position(line=end_line + 1, character=0) - self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) + if dry_run: + original_content = self._get_code_file_content(relative_path) - def delete_symbol_at_location(self, location: SymbolLocation) -> None: + # Apply the edit - delete from start of start_line to start of end_line+1 + modified_content = self._apply_text_edit(original_content, start_line, 0, end_line + 1, 0, "") + + return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_file(relative_path): + start_pos = Position(line=start_line, character=0) + end_pos = Position(line=end_line + 1, character=0) + self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) + return None + + @overload + def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: Literal[False] = False) -> None: ... + @overload + def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: Literal[True]) -> CodeDiff: ... + def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: bool = False) -> CodeDiff | None: """ Deletes the symbol at the given location. - """ - with self._edited_symbol_location(location) as symbol: - assert location.relative_path is not None - assert symbol.body_start_position is not None - assert symbol.body_end_position is not None - self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) - def delete_symbol(self, name_path: str, relative_file_path: str) -> None: + :param dry_run: if True, return a CodeDiff instead of modifying the file + """ + if dry_run: + assert location.relative_path is not None + original_content = self._get_code_file_content(location.relative_path) + + # Find the symbol to get its body positions + symbol = self.find_by_location(location) + if symbol is None: + raise ValueError("Symbol not found/has no defined location within a file") + + start_pos = symbol.body_start_position + end_pos = symbol.body_end_position + if start_pos is None or end_pos is None: + raise ValueError(f"Symbol at {location} does not have a defined body range.") + + # Apply the edit - delete the entire symbol + modified_content = self._apply_text_edit( + original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"], "" + ) + + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: + with self._edited_symbol_location(location) as symbol: + assert location.relative_path is not None + assert symbol.body_start_position is not None + assert symbol.body_end_position is not None + self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) + return None + + @overload + def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: Literal[False] = False) -> None: ... + @overload + def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: Literal[True]) -> CodeDiff: ... + def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: bool = False) -> CodeDiff | None: """ Deletes the symbol with the given name in the given file. + + :param dry_run: if True, return a CodeDiff instead of modifying the file """ symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) if len(symbol_candidates) == 0: @@ -618,4 +950,4 @@ class SymbolManager: "Their locations are: \n " + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - self.delete_symbol_at_location(symbol.location) + return self.delete_symbol_at_location(symbol.location, dry_run=dry_run) # type: ignore[call-overload] From bd0923a2d3226cd7b108e0bd36e441896da2e110 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Thu, 29 May 2025 13:37:10 +0200 Subject: [PATCH 04/17] Refactoring: move out editing methods from LS to TextUtils Now these methods can be used in dry-run mode, assuring there is no logic duplication --- src/multilspy/language_server.py | 14 +- src/multilspy/multilspy_utils.py | 26 ++- src/serena/symbol.py | 223 ++++++++++-------------- test/serena/test_symbol_editing.py | 261 +++++++++++++++++++++++++++++ 4 files changed, 377 insertions(+), 147 deletions(-) create mode 100644 test/serena/test_symbol_editing.py diff --git a/src/multilspy/language_server.py b/src/multilspy/language_server.py index fab22e5..9c02bd3 100644 --- a/src/multilspy/language_server.py +++ b/src/multilspy/language_server.py @@ -433,10 +433,9 @@ class LanguageServer: file_buffer = self.open_file_buffers[uri] file_buffer.version += 1 - change_index = TextUtils.get_index_from_line_col(file_buffer.contents, line, column) - file_buffer.contents = ( - file_buffer.contents[:change_index] + text_to_be_inserted + file_buffer.contents[change_index:] - ) + + new_contents, new_l, new_c = TextUtils.insert_text_at_position(file_buffer.contents, line, column, text_to_be_inserted) + file_buffer.contents = new_contents self.server.notify.did_change_text_document( { LSPConstants.TEXT_DOCUMENT: { @@ -454,7 +453,6 @@ class LanguageServer: ], } ) - new_l, new_c = TextUtils.get_updated_position_from_line_and_column_and_edit(line, column, text_to_be_inserted) return multilspy_types.Position(line=new_l, character=new_c) def delete_text_between_positions( @@ -481,10 +479,8 @@ class LanguageServer: file_buffer = self.open_file_buffers[uri] file_buffer.version += 1 - del_start_idx = TextUtils.get_index_from_line_col(file_buffer.contents, start["line"], start["character"]) - del_end_idx = TextUtils.get_index_from_line_col(file_buffer.contents, end["line"], end["character"]) - deleted_text = file_buffer.contents[del_start_idx:del_end_idx] - file_buffer.contents = file_buffer.contents[:del_start_idx] + file_buffer.contents[del_end_idx:] + new_contents, deleted_text = TextUtils.delete_text_between_positions(file_buffer.contents, start_line=start["line"], start_col=start["character"], end_line=end["line"], end_col=end["character"]) + file_buffer.contents = new_contents self.server.notify.did_change_text_document( { LSPConstants.TEXT_DOCUMENT: { diff --git a/src/multilspy/multilspy_utils.py b/src/multilspy/multilspy_utils.py index 9dcfd8a..c649ba9 100644 --- a/src/multilspy/multilspy_utils.py +++ b/src/multilspy/multilspy_utils.py @@ -57,7 +57,7 @@ class TextUtils: return idx @staticmethod - def get_updated_position_from_line_and_column_and_edit(l: int, c: int, text_to_be_inserted: str) -> Tuple[int, int]: + def _get_updated_position_from_line_and_column_and_edit(l: int, c: int, text_to_be_inserted: str) -> Tuple[int, int]: """ Utility function to get the position of the cursor after inserting text at a given line and column. """ @@ -68,6 +68,30 @@ class TextUtils: else: c += len(text_to_be_inserted) return (l, c) + + @staticmethod + def delete_text_between_positions(text: str, start_line: int, start_col: int, end_line: int, end_col: int) -> Tuple[str, str]: + """ + Deletes the text between the given start and end positions. + Returns the modified text and the deleted text. + """ + del_start_idx = TextUtils.get_index_from_line_col(text, start_line, start_col) + del_end_idx = TextUtils.get_index_from_line_col(text, end_line, end_col) + + deleted_text = text[del_start_idx:del_end_idx] + new_text = text[:del_start_idx] + text[del_end_idx:] + return new_text, deleted_text + + @staticmethod + def insert_text_at_position(text: str, line: int, col: int, text_to_be_inserted: str) -> Tuple[str, int, int]: + """ + Inserts the given text at the given line and column. + Returns the modified text and the new line and column. + """ + change_index = TextUtils.get_index_from_line_col(text, line, col) + new_text = text[:change_index] + text_to_be_inserted + text[change_index:] + new_l, new_c = TextUtils._get_updated_position_from_line_and_column_and_edit(line, col, text_to_be_inserted) + return new_text, new_l, new_c class PathUtils: diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 19cb46c..578c5dc 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -2,16 +2,17 @@ import json import logging import os from collections.abc import Iterator, Sequence -from contextlib import contextmanager +from contextlib import contextmanager, nullcontext from copy import copy from dataclasses import asdict, dataclass, field from difflib import SequenceMatcher -from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, overload +from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union, overload from sensai.util.string import ToStringMixin from multilspy import SyncLanguageServer from multilspy.multilspy_types import Position, SymbolKind, UnifiedSymbolInformation +from multilspy.multilspy_utils import TextUtils if TYPE_CHECKING: from .agent import SerenaAgent @@ -475,7 +476,12 @@ class Symbol(ToStringMixin): class SymbolManager: - def __init__(self, lang_server: SyncLanguageServer, agent: "SerenaAgent") -> None: + def __init__(self, lang_server: SyncLanguageServer, agent: Union["SerenaAgent", None] = None) -> None: + """ + :param lang_server: the language server to use for symbol retrieval as well as editing operations. + :param agent: the agent to use (only needed for marking files as modified). You can pass None if you don't + need an agent to be avare of file modifications performed by the symbol manager. + """ self.lang_server = lang_server self.agent = agent @@ -601,47 +607,29 @@ class SymbolManager: abs_path = os.path.join(root_path, relative_path) with open(abs_path, "w", encoding="utf-8") as f: f.write(file_buffer.contents) - self.agent.mark_file_modified(relative_path) + if self.agent is not None: + self.agent.mark_file_modified(relative_path) @contextmanager - def _edited_symbol_location(self, location: SymbolLocation) -> Iterator[Symbol]: + def _edited_symbol_location(self, location: SymbolLocation, dry_run: bool = False) -> Iterator[Symbol]: + """ + Context manager for locating and editing a symbol in a file. + If dry_run is True, the file is not actually modified, but the symbol is still located. + The dry_run flag is primarily implemented to allow the same code to be used for both editing and diffing + (the latter mostly for tests). + """ symbol = self.find_by_location(location) if symbol is None: raise ValueError("Symbol not found/has no defined location within a file") assert location.relative_path is not None - with self._edited_file(location.relative_path): + edit_context = self._edited_file(location.relative_path) if not dry_run else nullcontext() + with edit_context: yield symbol def _get_code_file_content(self, relative_path: str) -> str: """Get the content of a file using the language server.""" return self.lang_server.language_server.retrieve_full_file_content(relative_path) - @staticmethod - def _apply_text_edit(content: str, start_line: int, start_char: int, end_line: int, end_char: int, replacement: str) -> str: - """Apply a text edit to content and return the modified content.""" - lines = content.splitlines(keepends=True) - - # Handle edge cases - if not lines: - return replacement - - # Ensure we have enough lines - while len(lines) <= max(start_line, end_line): - lines.append("") - - # Extract the portion to keep before the edit - before_lines = lines[:start_line] - before_on_line = lines[start_line][:start_char] if start_line < len(lines) else "" - - # Extract the portion to keep after the edit - after_on_line = lines[end_line][end_char:] if end_line < len(lines) else "" - after_lines = lines[end_line + 1 :] if end_line + 1 < len(lines) else [] - - # Combine everything - result = before_lines + [before_on_line + replacement + after_on_line] + after_lines - - return "".join(result) - @overload def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... @overload @@ -679,36 +667,27 @@ class SymbolManager: if not body.endswith("\n"): body += "\n" - if dry_run: + with self._edited_symbol_location(location, dry_run=dry_run) as symbol: assert location.relative_path is not None - original_content = self._get_code_file_content(location.relative_path) - - # Find the symbol to get its body positions - symbol = self.find_by_location(location) - if symbol is None: - raise ValueError("Symbol not found/has no defined location within a file") - start_pos = symbol.body_start_position end_pos = symbol.body_end_position if start_pos is None or end_pos is None: raise ValueError(f"Symbol at {location} does not have a defined body range.") - - # Apply the edit - modified_content = self._apply_text_edit( - original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"], body - ) - - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_symbol_location(location) as symbol: - assert location.relative_path is not None - start_pos = symbol.body_start_position - end_pos = symbol.body_end_position - if start_pos is None or end_pos is None: - raise ValueError(f"Symbol at {location} does not have a defined body range.") + if dry_run: + original_content = self._get_code_file_content(location.relative_path) + modified_content, _ = TextUtils.delete_text_between_positions( + original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"] + ) + modified_content, _, _ = TextUtils.insert_text_at_position( + modified_content, start_pos["line"], start_pos["character"], body + ) + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: + # TODO: add method (in LS and TextUtils) replace_text_between_positions, calling two methods in LS adds extra overhead + # Use it here and above self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - return None + return None @overload def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... @@ -745,30 +724,28 @@ class SymbolManager: # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" + if not body.startswith("\n"): + body = "\n" + body + + assert location.relative_path is not None + + # Find the symbol to get its end position + symbol = self.find_by_location(location) + if symbol is None: + raise ValueError("Symbol not found/has no defined location within a file") + + pos = symbol.body_end_position + if pos is None: + raise ValueError(f"Symbol at {location} does not have a defined end position.") if dry_run: - assert location.relative_path is not None original_content = self._get_code_file_content(location.relative_path) - - # Find the symbol to get its end position - symbol = self.find_by_location(location) - if symbol is None: - raise ValueError("Symbol not found/has no defined location within a file") - - pos = symbol.body_end_position - if pos is None: - raise ValueError(f"Symbol at {location} does not have a defined end position.") - - # Apply the edit - insert at end position - modified_content = self._apply_text_edit(original_content, pos["line"], pos["character"], pos["line"], pos["character"], body) - + modified_content, _, _ = TextUtils.insert_text_at_position(original_content, pos["line"], pos["character"], body) return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) else: - with self._edited_symbol_location(location) as symbol: - pos = symbol.body_end_position - if pos is None: - raise ValueError(f"Symbol at {location} does not have a defined end position.") - assert location.relative_path is not None + # The _edited_symbol_location context manager handles LSP notifications like didOpen. + # We use the pre-calculated 'pos' for the insertion. + with self._edited_symbol_location(location): self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) return None @@ -807,40 +784,23 @@ class SymbolManager: # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" + if not body.startswith("\n"): + body = "\n" + body - if dry_run: - assert location.relative_path is not None - original_content = self._get_code_file_content(location.relative_path) - - # Find the symbol to get its start position - symbol = self.find_by_location(location) - if symbol is None: - raise ValueError("Symbol not found/has no defined location within a file") - + with self._edited_symbol_location(location) as symbol: original_start_pos = symbol.body_start_position if original_start_pos is None: raise ValueError(f"Symbol at {location} does not have a defined start position.") + pos = copy(original_start_pos) + assert location.relative_path is not None - # Apply the edit - insert at start position - modified_content = self._apply_text_edit( - original_content, - original_start_pos["line"], - original_start_pos["character"], - original_start_pos["line"], - original_start_pos["character"], - body, - ) - - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_symbol_location(location) as symbol: - original_start_pos = symbol.body_start_position - if original_start_pos is None: - raise ValueError(f"Symbol at {location} does not have a defined start position.") - pos = copy(original_start_pos) - assert location.relative_path is not None + if dry_run: + original_content = self._get_code_file_content(location.relative_path) + modified_content, _, _ = TextUtils.insert_text_at_position(original_content, pos["line"], pos["character"], body) + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) - return None + return None @overload def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[False] = False) -> None: ... @@ -856,10 +816,7 @@ class SymbolManager: """ if dry_run: original_content = self._get_code_file_content(relative_path) - - # Apply the edit - insert at beginning of line - modified_content = self._apply_text_edit(original_content, line, 0, line, 0, content) - + modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line, 0, content) return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) else: with self._edited_file(relative_path): @@ -878,17 +835,19 @@ class SymbolManager: :param end_line: the 0-based index of the last line to delete (inclusive) :param dry_run: if True, return a CodeDiff instead of modifying the file """ + start_col = 0 + end_line_for_delete = end_line + 1 + end_col = 0 if dry_run: original_content = self._get_code_file_content(relative_path) - - # Apply the edit - delete from start of start_line to start of end_line+1 - modified_content = self._apply_text_edit(original_content, start_line, 0, end_line + 1, 0, "") - + modified_content, _ = TextUtils.delete_text_between_positions( + original_content, start_line, start_col, end_line_for_delete, end_col + ) return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) else: with self._edited_file(relative_path): - start_pos = Position(line=start_line, character=0) - end_pos = Position(line=end_line + 1, character=0) + start_pos = Position(line=start_line, character=start_col) + end_pos = Position(line=end_line_for_delete, character=end_col) self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) return None @@ -902,33 +861,23 @@ class SymbolManager: :param dry_run: if True, return a CodeDiff instead of modifying the file """ - if dry_run: + with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None - original_content = self._get_code_file_content(location.relative_path) - - # Find the symbol to get its body positions - symbol = self.find_by_location(location) - if symbol is None: - raise ValueError("Symbol not found/has no defined location within a file") - - start_pos = symbol.body_start_position - end_pos = symbol.body_end_position - if start_pos is None or end_pos is None: - raise ValueError(f"Symbol at {location} does not have a defined body range.") - - # Apply the edit - delete the entire symbol - modified_content = self._apply_text_edit( - original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"], "" - ) - - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_symbol_location(location) as symbol: - assert location.relative_path is not None - assert symbol.body_start_position is not None - assert symbol.body_end_position is not None + assert symbol.body_start_position is not None + assert symbol.body_end_position is not None + if dry_run: + original_content = self._get_code_file_content(location.relative_path) + modified_content, _ = TextUtils.delete_text_between_positions( + original_content, + symbol.body_start_position["line"], + symbol.body_start_position["character"], + symbol.body_end_position["line"], + symbol.body_end_position["character"], + ) + return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) + else: self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) - return None + return None @overload def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: Literal[False] = False) -> None: ... diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py new file mode 100644 index 0000000..9f2b5cf --- /dev/null +++ b/test/serena/test_symbol_editing.py @@ -0,0 +1,261 @@ +import os + +import pytest + +from multilspy import SyncLanguageServer +from multilspy.multilspy_config import Language +from src.serena.symbol import SymbolManager + +# Python test file path +PYTHON_TEST_REL_FILE_PATH = os.path.join("test_repo", "variables.py") + +# TypeScript test file path +TYPESCRIPT_TEST_FILE = "index.ts" + +# Expected deleted lines for Python VariableContainer +EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON = '''class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result +''' + +# Expected deleted lines for TypeScript DemoClass +EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT = """export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + printValue() { + console.log(this.value); + } +}""" + + +@pytest.mark.parametrize( + "language_server, relative_file_path, symbol_name, expected_deleted_lines", + [ + ( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "VariableContainer", + EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON.strip().splitlines(), + ), + ( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass", + EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT.strip().splitlines(), + ), + ], + indirect=["language_server"], +) +def test_delete_symbol_dry_run( + language_server: SyncLanguageServer, + relative_file_path: str, + symbol_name: str, + expected_deleted_lines: list[str], +): + symbol_manager = SymbolManager(lang_server=language_server) + code_diff = symbol_manager.delete_symbol(symbol_name, relative_file_path, dry_run=True) + + assert code_diff is not None + assert code_diff.relative_path == relative_file_path + assert len(code_diff.added_lines) == 0 + + actual_deleted_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] + + # Normalize expected lines by stripping whitespace and removing empty lines + normalized_expected_deleted_lines = [line.strip() for line in expected_deleted_lines if line.strip()] + assert actual_deleted_lines == normalized_expected_deleted_lines + assert code_diff.original_content != code_diff.modified_content + + +NEW_PYTHON_FUNCTION = """def new_inserted_function(): + print("This is a new function inserted before another.")""" + +NEW_TYPESCRIPT_FUNCTION = """function newInsertedFunction(): void { + console.log("This is a new function inserted before another."); +}""" + + +@pytest.mark.parametrize( + "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", + [ + ( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "use_module_variables", + NEW_PYTHON_FUNCTION, + NEW_PYTHON_FUNCTION.strip().splitlines(), + ), + ( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "helperFunction", + NEW_TYPESCRIPT_FUNCTION, + NEW_TYPESCRIPT_FUNCTION.strip().splitlines(), + ), + ], + indirect=["language_server"], +) +def test_insert_before_symbol_dry_run( + language_server: SyncLanguageServer, + relative_file_path: str, + symbol_name: str, + new_content: str, + expected_added_lines_content: list[str], +): + symbol_manager = SymbolManager(lang_server=language_server) + code_diff = symbol_manager.insert_before_symbol(symbol_name, relative_file_path, new_content, dry_run=True) + + assert code_diff is not None + assert code_diff.relative_path == relative_file_path + assert len(code_diff.deleted_lines) == 0 + actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] + normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] + + assert actual_added_lines_content == normalized_expected_added_lines + assert code_diff.original_content != code_diff.modified_content + + +NEW_PYTHON_VARIABLE = 'new_module_var = "Inserted after typed_module_var"' + +NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { + console.log("This function is after DemoClass."); +}""" + + +@pytest.mark.parametrize( + "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", + [ + ( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "typed_module_var", + NEW_PYTHON_VARIABLE, + [NEW_PYTHON_VARIABLE], + ), + ( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass", + NEW_TYPESCRIPT_FUNCTION_AFTER, + NEW_TYPESCRIPT_FUNCTION_AFTER.strip().splitlines(), + ), + ], + indirect=["language_server"], +) +def test_insert_after_symbol_dry_run( + language_server: SyncLanguageServer, + relative_file_path: str, + symbol_name: str, + new_content: str, + expected_added_lines_content: list[str], +): + symbol_manager = SymbolManager(lang_server=language_server) + code_diff = symbol_manager.insert_after_symbol(symbol_name, relative_file_path, new_content, dry_run=True) + + assert code_diff is not None + assert code_diff.relative_path == relative_file_path + assert len(code_diff.deleted_lines) == 0 + actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] + normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] + assert actual_added_lines_content == normalized_expected_added_lines + assert code_diff.original_content != code_diff.modified_content + + +PYTHON_REPLACED_BODY = """ # This body has been replaced + self.instance_var = "Replaced!" + self.reassignable_instance_var = 999 +""" + +TYPESCRIPT_REPLACED_BODY = """ // This body has been replaced + console.warn("New value: " + this.value); +""" + +EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON = """ def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned""" + +# For single line original content, direct list is fine +EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT = [" printValue() {", " console.log(this.value);", " }"] + + +@pytest.mark.parametrize( + "language_server, relative_file_path, symbol_name, new_body, expected_original_lines_content, expected_modified_lines_content", + [ + ( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "VariableContainer/modify_instance_var", + PYTHON_REPLACED_BODY, + EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON.strip().splitlines(), + PYTHON_REPLACED_BODY.strip().splitlines(), + ), + ( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass/printValue", + TYPESCRIPT_REPLACED_BODY, + EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT, # Already a list of lines + TYPESCRIPT_REPLACED_BODY.strip().splitlines(), + ), + ], + indirect=["language_server"], +) +def test_replace_body_dry_run( + language_server: SyncLanguageServer, + relative_file_path: str, + symbol_name: str, + new_body: str, + expected_original_lines_content: list[str], + expected_modified_lines_content: list[str], +): + symbol_manager = SymbolManager(lang_server=language_server) + code_diff = symbol_manager.replace_body(symbol_name, relative_file_path, new_body, dry_run=True) + + assert code_diff is not None + assert code_diff.relative_path == relative_file_path + + actual_original_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] + normalized_expected_original_lines = [line.strip() for line in expected_original_lines_content if line.strip()] + assert actual_original_lines == normalized_expected_original_lines + + actual_modified_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] + normalized_expected_modified_lines = [line.strip() for line in expected_modified_lines_content if line.strip()] + assert actual_modified_lines == normalized_expected_modified_lines + + assert code_diff.original_content != code_diff.modified_content From 0f18385f4c10399d6c2a032a1faba698dc7b9ff7 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Thu, 29 May 2025 14:10:12 +0200 Subject: [PATCH 05/17] Better handling of lines and columns at insertions --- scripts/demo_run_tools.py | 33 ++++++-- src/serena/symbol.py | 167 ++++++++++++++++++++++++++++++-------- 2 files changed, 160 insertions(+), 40 deletions(-) diff --git a/scripts/demo_run_tools.py b/scripts/demo_run_tools.py index 4d639b4..eafae78 100644 --- a/scripts/demo_run_tools.py +++ b/scripts/demo_run_tools.py @@ -10,17 +10,34 @@ from pprint import pprint from serena.agent import * -project_root = Path(__file__).parent.parent + +@dataclass +class InMemorySerenaConfig(SerenaConfigBase): + """ + In-memory implementation of Serena configuration with the GUI disabled. + """ + + gui_log_window_enabled: bool = False + web_dashboard: bool = False + if __name__ == "__main__": - agent = SerenaAgent(project_config=str(project_root / ".serena" / "project.yml")) - overview_tool = agent.get_tool(GetSymbolsOverviewTool) + project_path = str(Path("test") / "resources" / "repos" / "python" / "test_repo") + agent = SerenaAgent(project=project_path, serena_config=InMemorySerenaConfig()) find_symbol_tool = agent.get_tool(FindSymbolTool) - print("Getting an overview of the util package\n") - pprint(json.loads(overview_tool.apply("src/serena/util"))) + print("Finding the symbol 'VariableContainer'\n") + pprint(json.loads(find_symbol_tool.apply("VariableContainer", within_relative_path=str(Path("test_repo") / "variables.py")))) - print("\n\n") + # modifying with dry-run + symbol_manager = agent.symbol_manager - print("Finding the symbol 'SerenaAgent'\n") - pprint(json.loads(find_symbol_tool.apply("SerenaAgent"))) + code_diff = symbol_manager.insert_after_symbol( + "VariableContainer/modify_instance_var", + relative_file_path=str(Path("test_repo") / "variables.py"), + body="test test\nsecond line", + dry_run=True, + ) + print(f"Deleted lines: {code_diff.deleted_lines}") + print(f"Added lines: {code_diff.added_lines}") + print(f"Edited module: {code_diff.modified_content}") diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 578c5dc..201cca7 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -3,7 +3,6 @@ import logging import os from collections.abc import Iterator, Sequence from contextlib import contextmanager, nullcontext -from copy import copy from dataclasses import asdict, dataclass, field from difflib import SequenceMatcher from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union, overload @@ -690,10 +689,37 @@ class SymbolManager: return None @overload - def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... + def insert_after_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + use_same_indentation: bool = True, + at_new_line: bool = True, + dry_run: Literal[False] = False, + ) -> None: ... @overload - def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_after_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_after_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + use_same_indentation: bool = True, + at_new_line: bool = True, + dry_run: Literal[True], + ) -> CodeDiff: ... + def insert_after_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + use_same_indentation: bool = True, + at_new_line: bool = True, + dry_run: bool = False, + ) -> CodeDiff | None: """ Inserts content after the symbol with the given name in the given file. """ @@ -707,13 +733,25 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[-1] - return self.insert_after_symbol_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] + return self.insert_after_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] @overload - def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... + def insert_after_symbol_at_location( + self, + location: SymbolLocation, + body: str, + *, + at_new_line: bool = True, + use_same_indentation: bool = True, + dry_run: Literal[False] = False, + ) -> None: ... @overload - def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_after_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_after_symbol_at_location( + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] + ) -> CodeDiff: ... + def insert_after_symbol_at_location( + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False + ) -> CodeDiff | None: """ Appends content after the given symbol @@ -724,8 +762,6 @@ class SymbolManager: # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - if not body.startswith("\n"): - body = "\n" + body assert location.relative_path is not None @@ -738,22 +774,74 @@ class SymbolManager: if pos is None: raise ValueError(f"Symbol at {location} does not have a defined end position.") + line, col = pos["line"], pos["character"] + if use_same_indentation: + symbol_start_pos = symbol.body_start_position + assert symbol_start_pos is not None, f"Symbol at {location=} does not have a defined start position." + symbol_identifier_col = symbol_start_pos["character"] + indent = " " * (symbol_identifier_col) + body = "\n".join(indent + line for line in body.splitlines()) + if at_new_line: + line += 1 + # IMPORTANT: without this, the insertion does the wrong thing. See implementation of insert_text_at_position in TextUtils, + # it is somewhat counterintuitive (never inserts whitespace) + # I am not 100% sure whether col=0 is always the best choice here. + # + # Without col=0, inserting after dataclass_instance in variables.py: + # > dataclass_instance = VariableDataclass(id=1, name="Test") + # > test test + # > dataclass_instancetest test + # > second line + # > .status = "active" # Reassign dataclass field + # + # With col=0: + # > dataclass_instance = VariableDataclass(id=1, name="Test") + # > test test + # > second line + # > dataclass_instance.status = "active" # Reassign dataclass field + col = 0 + if dry_run: original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, pos["line"], pos["character"], body) + modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) else: - # The _edited_symbol_location context manager handles LSP notifications like didOpen. - # We use the pre-calculated 'pos' for the insertion. with self._edited_symbol_location(location): - self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) return None @overload - def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... + def insert_before_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + at_new_line: bool = True, + use_same_indentation: bool = True, + dry_run: Literal[False] = False, + ) -> None: ... @overload - def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_before_symbol(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_before_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + at_new_line: bool = True, + use_same_indentation: bool = True, + dry_run: Literal[True], + ) -> CodeDiff: ... + def insert_before_symbol( + self, + name_path: str, + relative_file_path: str, + body: str, + *, + at_new_line: bool = True, + use_same_indentation: bool = True, + dry_run: bool = False, + ) -> CodeDiff | None: """ Inserts content before the symbol with the given name in the given file. """ @@ -767,13 +855,25 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.insert_before_symbol_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] + return self.insert_before_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] @overload - def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... + def insert_before_symbol_at_location( + self, + location: SymbolLocation, + body: str, + *, + at_new_line: bool = True, + use_same_indentation: bool = True, + dry_run: Literal[False] = False, + ) -> None: ... @overload - def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_before_symbol_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_before_symbol_at_location( + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] + ) -> CodeDiff: ... + def insert_before_symbol_at_location( + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False + ) -> CodeDiff | None: """ Inserts content before the given symbol @@ -781,25 +881,28 @@ class SymbolManager: :param body: the body of the entity to insert :param dry_run: if True, return a CodeDiff instead of modifying the file """ - # make sure body always ends with at least one newline - if not body.endswith("\n"): - body += "\n" - if not body.startswith("\n"): - body = "\n" + body - with self._edited_symbol_location(location) as symbol: - original_start_pos = symbol.body_start_position - if original_start_pos is None: + symbol_start_pos = symbol.body_start_position + if symbol_start_pos is None: raise ValueError(f"Symbol at {location} does not have a defined start position.") - pos = copy(original_start_pos) + line = symbol_start_pos["line"] + col = symbol_start_pos["character"] + if use_same_indentation: + indent = " " * (col) + body = "\n".join(indent + line for line in body.splitlines()) + + # similar problems as in insert_after_symbol_at_location, see comment there + if at_new_line: + col = 0 + line -= 1 assert location.relative_path is not None if dry_run: original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, pos["line"], pos["character"], body) + modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) else: - self.lang_server.insert_text_at_position(location.relative_path, pos["line"], pos["character"], body) + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) return None @overload From bed72f417a20aba3261bb43aaa2371b5a1128c44 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 29 May 2025 18:15:07 +0200 Subject: [PATCH 06/17] Add test case abstraction for editing tests Add implementation for "delete symbol", removing unnecessary dry-run flag and overloads from the implementation --- src/serena/symbol.py | 32 ++------- test/conftest.py | 8 ++- test/serena/test_symbol_editing.py | 105 ++++++++++++++++++++++------- 3 files changed, 90 insertions(+), 55 deletions(-) diff --git a/src/serena/symbol.py b/src/serena/symbol.py index b2c764a..4821bc2 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -949,43 +949,19 @@ class SymbolManager: self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) return None - @overload - def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: Literal[False] = False) -> None: ... - @overload - def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: Literal[True]) -> CodeDiff: ... - def delete_symbol_at_location(self, location: SymbolLocation, *, dry_run: bool = False) -> CodeDiff | None: + def delete_symbol_at_location(self, location: SymbolLocation) -> None: """ Deletes the symbol at the given location. - - :param dry_run: if True, return a CodeDiff instead of modifying the file """ with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None assert symbol.body_start_position is not None assert symbol.body_end_position is not None - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _ = TextUtils.delete_text_between_positions( - original_content, - symbol.body_start_position["line"], - symbol.body_start_position["character"], - symbol.body_end_position["line"], - symbol.body_end_position["character"], - ) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) - return None + self.lang_server.delete_text_between_positions(location.relative_path, symbol.body_start_position, symbol.body_end_position) - @overload - def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def delete_symbol(self, name_path: str, relative_file_path: str, *, dry_run: bool = False) -> CodeDiff | None: + def delete_symbol(self, name_path: str, relative_file_path: str) -> None: """ Deletes the symbol with the given name in the given file. - - :param dry_run: if True, return a CodeDiff instead of modifying the file """ symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) if len(symbol_candidates) == 0: @@ -997,4 +973,4 @@ class SymbolManager: "Their locations are: \n " + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.delete_symbol_at_location(symbol.location, dry_run=dry_run) # type: ignore[call-overload] + self.delete_symbol_at_location(symbol.location) diff --git a/test/conftest.py b/test/conftest.py index 4e403df..a5ca841 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -22,13 +22,17 @@ def get_repo_path(language: Language) -> Path: return Path(__file__).parent / "resources" / "repos" / language / "test_repo" -def create_default_ls(language: Language) -> SyncLanguageServer: +def create_ls(language: Language, repo_path: str): config = MultilspyConfig(code_language=language) - repo_path = str(get_repo_path(language)) logger = MultilspyLogger() return SyncLanguageServer.create(config, logger, repo_path) +def create_default_ls(language: Language) -> SyncLanguageServer: + repo_path = str(get_repo_path(language)) + return create_ls(language, repo_path) + + @pytest.fixture(scope="session") def repo_path(request: LanguageParamRequest) -> Path: """Get the repository path for a specific language. diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index 9f2b5cf..0ea19bc 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -1,10 +1,66 @@ import os +import shutil +import tempfile +from abc import abstractmethod +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path import pytest from multilspy import SyncLanguageServer from multilspy.multilspy_config import Language +from serena.symbol import CodeDiff from src.serena.symbol import SymbolManager +from test.conftest import create_ls, get_repo_path + + +class EditingTest: + def __init__(self, language: Language, rel_path: str): + """ + :param language: the language + :param rel_path: the relative path of the edited file + """ + self.rel_path = rel_path + self.language = language + self.original_repo_path = get_repo_path(language) + self.repo_path: Path | None = None + + @contextmanager + def _setup(self) -> Iterator[SymbolManager]: + """Context manager for setup/teardown with a temporary directory, providing the symbol manager.""" + temp_dir = Path(tempfile.mkdtemp()) + self.repo_path = temp_dir / self.original_repo_path.name + try: + shutil.copytree(self.original_repo_path, self.repo_path) + language_server = create_ls(self.language, str(self.repo_path)) + language_server.start() + yield SymbolManager(lang_server=language_server) + finally: + shutil.rmtree(temp_dir) + + def _read_file(self, rel_path: str) -> str: + """Read the content of a file in the test repository.""" + file_path = self.repo_path / rel_path + with open(file_path, encoding="utf-8") as f: + return f.read() + + def run_test(self) -> None: + with self._setup() as symbol_manager: + content_before = self._read_file(self.rel_path) + self._apply_edit(symbol_manager) + content_after = self._read_file(self.rel_path) + code_diff = CodeDiff(self.rel_path, original_content=content_before, modified_content=content_after) + self._test_diff(code_diff) + + @abstractmethod + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + pass + + @abstractmethod + def _test_diff(self, code_diff: CodeDiff) -> None: + pass + # Python test file path PYTHON_TEST_REL_FILE_PATH = os.path.join("test_repo", "variables.py") @@ -63,43 +119,42 @@ EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT = """export class DemoClass { }""" +class DeleteSymbolTest(EditingTest): + def __init__(self, language: Language, rel_path: str, deleted_symbol: str, expected_deleted_lines: str): + super().__init__(language, rel_path) + self.expected_deleted_lines = expected_deleted_lines + self.deleted_symbol = deleted_symbol + self.rel_path = rel_path + + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + symbol_manager.delete_symbol(self.deleted_symbol, self.rel_path) + + def _test_diff(self, code_diff: CodeDiff) -> None: + assert code_diff.original_content != code_diff.modified_content + actual_deleted_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] + normalized_expected_deleted_lines = [line.strip() for line in self.expected_deleted_lines.splitlines() if line.strip()] + assert actual_deleted_lines == normalized_expected_deleted_lines + + @pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, expected_deleted_lines", + "test_case", [ - ( + DeleteSymbolTest( Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "VariableContainer", - EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON.strip().splitlines(), + EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON, ), - ( + DeleteSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "DemoClass", - EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT.strip().splitlines(), + EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT, ), ], - indirect=["language_server"], ) -def test_delete_symbol_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - expected_deleted_lines: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.delete_symbol(symbol_name, relative_file_path, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - assert len(code_diff.added_lines) == 0 - - actual_deleted_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] - - # Normalize expected lines by stripping whitespace and removing empty lines - normalized_expected_deleted_lines = [line.strip() for line in expected_deleted_lines if line.strip()] - assert actual_deleted_lines == normalized_expected_deleted_lines - assert code_diff.original_content != code_diff.modified_content +def test_delete_symbol(test_case): + test_case.run_test() NEW_PYTHON_FUNCTION = """def new_inserted_function(): From e9d10fde19dd20df724afa412f002fb3691b9d27 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Thu, 29 May 2025 19:07:32 +0200 Subject: [PATCH 07/17] Remove dry_run and overloads from remaining editing methods Add corresponding test classes and test cases (not yet working) --- scripts/demo_run_tools.py | 16 +-- src/serena/symbol.py | 199 +++++------------------------ test/serena/test_symbol_editing.py | 158 ++++++++--------------- 3 files changed, 86 insertions(+), 287 deletions(-) diff --git a/scripts/demo_run_tools.py b/scripts/demo_run_tools.py index eafae78..5d8e450 100644 --- a/scripts/demo_run_tools.py +++ b/scripts/demo_run_tools.py @@ -24,20 +24,8 @@ class InMemorySerenaConfig(SerenaConfigBase): if __name__ == "__main__": project_path = str(Path("test") / "resources" / "repos" / "python" / "test_repo") agent = SerenaAgent(project=project_path, serena_config=InMemorySerenaConfig()) - find_symbol_tool = agent.get_tool(FindSymbolTool) + # apply a tool + find_symbol_tool = agent.get_tool(FindSymbolTool) print("Finding the symbol 'VariableContainer'\n") pprint(json.loads(find_symbol_tool.apply("VariableContainer", within_relative_path=str(Path("test_repo") / "variables.py")))) - - # modifying with dry-run - symbol_manager = agent.symbol_manager - - code_diff = symbol_manager.insert_after_symbol( - "VariableContainer/modify_instance_var", - relative_file_path=str(Path("test_repo") / "variables.py"), - body="test test\nsecond line", - dry_run=True, - ) - print(f"Deleted lines: {code_diff.deleted_lines}") - print(f"Added lines: {code_diff.added_lines}") - print(f"Edited module: {code_diff.modified_content}") diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 4821bc2..a284a14 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -2,16 +2,15 @@ import json import logging import os from collections.abc import Iterator, Sequence -from contextlib import contextmanager, nullcontext +from contextlib import contextmanager from dataclasses import asdict, dataclass, field from difflib import SequenceMatcher -from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union, overload +from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union from sensai.util.string import ToStringMixin from multilspy import SyncLanguageServer from multilspy.multilspy_types import Position, SymbolKind, UnifiedSymbolInformation -from multilspy.multilspy_utils import TextUtils if TYPE_CHECKING: from .agent import SerenaAgent @@ -605,30 +604,22 @@ class SymbolManager: self.agent.mark_file_modified(relative_path) @contextmanager - def _edited_symbol_location(self, location: SymbolLocation, dry_run: bool = False) -> Iterator[Symbol]: + def _edited_symbol_location(self, location: SymbolLocation) -> Iterator[Symbol]: """ Context manager for locating and editing a symbol in a file. - If dry_run is True, the file is not actually modified, but the symbol is still located. - The dry_run flag is primarily implemented to allow the same code to be used for both editing and diffing - (the latter mostly for tests). """ symbol = self.find_by_location(location) if symbol is None: raise ValueError("Symbol not found/has no defined location within a file") assert location.relative_path is not None - edit_context = self._edited_file(location.relative_path) if not dry_run else nullcontext() - with edit_context: + with self._edited_file(location.relative_path): yield symbol def _get_code_file_content(self, relative_path: str) -> str: """Get the content of a file using the language server.""" return self.lang_server.language_server.retrieve_full_file_content(relative_path) - @overload - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def replace_body(self, name_path: str, relative_file_path: str, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def replace_body(self, name_path: str, relative_file_path: str, body: str) -> None: """ Replace the body of the symbol with the given name in the given file. """ @@ -643,47 +634,28 @@ class SymbolManager: + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.replace_body_at_location(symbol.location, body, dry_run=dry_run) # type: ignore[call-overload] + return self.replace_body_at_location(symbol.location, body) - @overload - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def replace_body_at_location(self, location: SymbolLocation, body: str, *, dry_run: bool = False) -> CodeDiff | None: + def replace_body_at_location(self, location: SymbolLocation, body: str) -> None: """ Replace the body of the symbol at the given location with the given body :param location: the location of the symbol to replace. :param body: the new body - :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): body += "\n" - with self._edited_symbol_location(location, dry_run=dry_run) as symbol: + with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None start_pos = symbol.body_start_position end_pos = symbol.body_end_position if start_pos is None or end_pos is None: raise ValueError(f"Symbol at {location} does not have a defined body range.") - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _ = TextUtils.delete_text_between_positions( - original_content, start_pos["line"], start_pos["character"], end_pos["line"], end_pos["character"] - ) - modified_content, _, _ = TextUtils.insert_text_at_position( - modified_content, start_pos["line"], start_pos["character"], body - ) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - # TODO: add method (in LS and TextUtils) replace_text_between_positions, calling two methods in LS adds extra overhead - # Use it here and above - self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) - self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - return None + self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) + self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) - @overload def insert_after_symbol( self, name_path: str, @@ -692,29 +664,7 @@ class SymbolManager: *, use_same_indentation: bool = True, at_new_line: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_after_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - use_same_indentation: bool = True, - at_new_line: bool = True, - dry_run: Literal[True], - ) -> CodeDiff: ... - def insert_after_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - use_same_indentation: bool = True, - at_new_line: bool = True, - dry_run: bool = False, - ) -> CodeDiff | None: + ) -> None: """ Inserts content after the symbol with the given name in the given file. """ @@ -728,31 +678,18 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[-1] - return self.insert_after_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] + return self.insert_after_symbol_at_location( + symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation + ) - @overload def insert_after_symbol_at_location( - self, - location: SymbolLocation, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_after_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] - ) -> CodeDiff: ... - def insert_after_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False - ) -> CodeDiff | None: + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True + ) -> None: """ Appends content after the given symbol :param location: the location of the symbol after which to add new lines :param body: the body of the entity to append - :param dry_run: if True, return a CodeDiff instead of modifying the file """ # make sure body always ends with at least one newline if not body.endswith("\n"): @@ -796,16 +733,9 @@ class SymbolManager: # > dataclass_instance.status = "active" # Reassign dataclass field col = 0 - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_symbol_location(location): - self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - return None + with self._edited_symbol_location(location): + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - @overload def insert_before_symbol( self, name_path: str, @@ -814,29 +744,7 @@ class SymbolManager: *, at_new_line: bool = True, use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_before_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[True], - ) -> CodeDiff: ... - def insert_before_symbol( - self, - name_path: str, - relative_file_path: str, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: bool = False, - ) -> CodeDiff | None: + ) -> None: """ Inserts content before the symbol with the given name in the given file. """ @@ -850,31 +758,16 @@ class SymbolManager: f"Found symbols at locations: \n" + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.insert_before_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation, dry_run=dry_run) # type: ignore[call-overload] + self.insert_before_symbol_at_location(symbol.location, body, at_new_line=at_new_line, use_same_indentation=use_same_indentation) - @overload def insert_before_symbol_at_location( - self, - location: SymbolLocation, - body: str, - *, - at_new_line: bool = True, - use_same_indentation: bool = True, - dry_run: Literal[False] = False, - ) -> None: ... - @overload - def insert_before_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: Literal[True] - ) -> CodeDiff: ... - def insert_before_symbol_at_location( - self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True, dry_run: bool = False - ) -> CodeDiff | None: + self, location: SymbolLocation, body: str, *, at_new_line: bool = True, use_same_indentation: bool = True + ) -> None: """ Inserts content before the given symbol :param location: the location of the symbol before which to add new lines :param body: the body of the entity to insert - :param dry_run: if True, return a CodeDiff instead of modifying the file """ with self._edited_symbol_location(location) as symbol: symbol_start_pos = symbol.body_start_position @@ -892,62 +785,32 @@ class SymbolManager: line -= 1 assert location.relative_path is not None - if dry_run: - original_content = self._get_code_file_content(location.relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line=line, col=col, text_to_be_inserted=body) - return CodeDiff(relative_path=location.relative_path, original_content=original_content, modified_content=modified_content) - else: - self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - return None + self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) - @overload - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[False] = False) -> None: ... - @overload - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: Literal[True]) -> CodeDiff: ... - def insert_at_line(self, relative_path: str, line: int, content: str, *, dry_run: bool = False) -> CodeDiff | None: + def insert_at_line(self, relative_path: str, line: int, content: str) -> None: """ Inserts content at the given line in the given file. :param line: the 0-based index of the line to insert content at :param content: the content to insert - :param dry_run: if True, return a CodeDiff instead of modifying the file """ - if dry_run: - original_content = self._get_code_file_content(relative_path) - modified_content, _, _ = TextUtils.insert_text_at_position(original_content, line, 0, content) - return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_file(relative_path): - self.lang_server.insert_text_at_position(relative_path, line, 0, content) - return None + with self._edited_file(relative_path): + self.lang_server.insert_text_at_position(relative_path, line, 0, content) - @overload - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[False] = False) -> None: ... - @overload - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: Literal[True]) -> CodeDiff: ... - def delete_lines(self, relative_path: str, start_line: int, end_line: int, *, dry_run: bool = False) -> CodeDiff | None: + def delete_lines(self, relative_path: str, start_line: int, end_line: int) -> None: """ Deletes lines in the given file. :param start_line: the 0-based index of the first line to delete (inclusive) :param end_line: the 0-based index of the last line to delete (inclusive) - :param dry_run: if True, return a CodeDiff instead of modifying the file """ start_col = 0 end_line_for_delete = end_line + 1 end_col = 0 - if dry_run: - original_content = self._get_code_file_content(relative_path) - modified_content, _ = TextUtils.delete_text_between_positions( - original_content, start_line, start_col, end_line_for_delete, end_col - ) - return CodeDiff(relative_path=relative_path, original_content=original_content, modified_content=modified_content) - else: - with self._edited_file(relative_path): - start_pos = Position(line=start_line, character=start_col) - end_pos = Position(line=end_line_for_delete, character=end_col) - self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) - return None + with self._edited_file(relative_path): + start_pos = Position(line=start_line, character=start_col) + end_pos = Position(line=end_line_for_delete, character=end_col) + self.lang_server.delete_text_between_positions(relative_path, start_pos, end_pos) def delete_symbol_at_location(self, location: SymbolLocation) -> None: """ diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index 0ea19bc..dcbfe70 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -8,7 +8,6 @@ from pathlib import Path import pytest -from multilspy import SyncLanguageServer from multilspy.multilspy_config import Language from serena.symbol import CodeDiff from src.serena.symbol import SymbolManager @@ -165,46 +164,6 @@ NEW_TYPESCRIPT_FUNCTION = """function newInsertedFunction(): void { }""" -@pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", - [ - ( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "use_module_variables", - NEW_PYTHON_FUNCTION, - NEW_PYTHON_FUNCTION.strip().splitlines(), - ), - ( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "helperFunction", - NEW_TYPESCRIPT_FUNCTION, - NEW_TYPESCRIPT_FUNCTION.strip().splitlines(), - ), - ], - indirect=["language_server"], -) -def test_insert_before_symbol_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_content: str, - expected_added_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.insert_before_symbol(symbol_name, relative_file_path, new_content, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - assert len(code_diff.deleted_lines) == 0 - actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] - - assert actual_added_lines_content == normalized_expected_added_lines - assert code_diff.original_content != code_diff.modified_content - - NEW_PYTHON_VARIABLE = 'new_module_var = "Inserted after typed_module_var"' NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { @@ -212,43 +171,53 @@ NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { }""" +class InsertBeforeSymbolTest(EditingTest): + def __init__(self, language: Language, rel_path: str, symbol_name: str, new_content: str): + super().__init__(language, rel_path) + self.symbol_name = symbol_name + self.new_content = new_content + + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + symbol_manager.insert_before_symbol(self.symbol_name, self.rel_path, self.new_content) + + def _test_diff(self, code_diff: CodeDiff) -> None: + assert code_diff.original_content != code_diff.modified_content + actual_added_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] + normalized_expected_added_lines = [line.strip() for line in self.new_content.splitlines() if line.strip()] + assert actual_added_lines == normalized_expected_added_lines + + @pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_content, expected_added_lines_content", + "test_case", [ - ( + InsertBeforeSymbolTest( Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "typed_module_var", NEW_PYTHON_VARIABLE, - [NEW_PYTHON_VARIABLE], ), - ( + InsertBeforeSymbolTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "use_module_variables", + NEW_PYTHON_FUNCTION, + ), + InsertBeforeSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "DemoClass", NEW_TYPESCRIPT_FUNCTION_AFTER, - NEW_TYPESCRIPT_FUNCTION_AFTER.strip().splitlines(), + ), + InsertBeforeSymbolTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "helperFunction", + NEW_TYPESCRIPT_FUNCTION, ), ], - indirect=["language_server"], ) -def test_insert_after_symbol_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_content: str, - expected_added_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.insert_after_symbol(symbol_name, relative_file_path, new_content, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - assert len(code_diff.deleted_lines) == 0 - actual_added_lines_content = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_added_lines = [line.strip() for line in expected_added_lines_content if line.strip()] - assert actual_added_lines_content == normalized_expected_added_lines - assert code_diff.original_content != code_diff.modified_content +def test_insert_before_symbol(test_case): + test_case.run_test() PYTHON_REPLACED_BODY = """ # This body has been replaced @@ -269,48 +238,27 @@ EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON = """ def modify_instance_var(se EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT = [" printValue() {", " console.log(this.value);", " }"] +class ReplaceBodyTest(EditingTest): + def __init__( + self, language: Language, rel_path: str, symbol_name: str, new_body: str, expected_removed_lines: str, expected_added_lines: str + ): + super().__init__(language, rel_path) + self.symbol_name = symbol_name + self.new_body = new_body + + def _apply_edit(self, symbol_manager: SymbolManager) -> None: + symbol_manager.replace_body(self.symbol_name, self.rel_path, self.new_body) + + def _test_diff(self, code_diff: CodeDiff) -> None: + assert code_diff.original_content != code_diff.modified_content + # TODO test properly + + @pytest.mark.parametrize( - "language_server, relative_file_path, symbol_name, new_body, expected_original_lines_content, expected_modified_lines_content", + "test_case", [ - ( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "VariableContainer/modify_instance_var", - PYTHON_REPLACED_BODY, - EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON.strip().splitlines(), - PYTHON_REPLACED_BODY.strip().splitlines(), - ), - ( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "DemoClass/printValue", - TYPESCRIPT_REPLACED_BODY, - EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT, # Already a list of lines - TYPESCRIPT_REPLACED_BODY.strip().splitlines(), - ), + # TODO add tests ], - indirect=["language_server"], ) -def test_replace_body_dry_run( - language_server: SyncLanguageServer, - relative_file_path: str, - symbol_name: str, - new_body: str, - expected_original_lines_content: list[str], - expected_modified_lines_content: list[str], -): - symbol_manager = SymbolManager(lang_server=language_server) - code_diff = symbol_manager.replace_body(symbol_name, relative_file_path, new_body, dry_run=True) - - assert code_diff is not None - assert code_diff.relative_path == relative_file_path - - actual_original_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] - normalized_expected_original_lines = [line.strip() for line in expected_original_lines_content if line.strip()] - assert actual_original_lines == normalized_expected_original_lines - - actual_modified_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_modified_lines = [line.strip() for line in expected_modified_lines_content if line.strip()] - assert actual_modified_lines == normalized_expected_modified_lines - - assert code_diff.original_content != code_diff.modified_content +def test_replace_body(test_case: ReplaceBodyTest): + test_case.run_test() From e038cfa18814d09a472df3d4260b630aa847f2d2 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Fri, 30 May 2025 00:58:02 +0200 Subject: [PATCH 08/17] Snapshot tests for editing, WIP Need to look over the snapshot results to see whether they're what we expect. Also, for some reason tests are failing... --- pyproject.toml | 1 + .../__snapshots__/test_symbol_editing.ambr | 700 ++++++++++++++++++ test/serena/test_symbol_editing.py | 146 ++-- uv.lock | 14 + 4 files changed, 761 insertions(+), 100 deletions(-) create mode 100644 test/serena/__snapshots__/test_symbol_editing.ambr diff --git a/pyproject.toml b/pyproject.toml index 5ef68fe..f66c7e5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,6 +56,7 @@ dev = [ "ruff>=0.0.285", "toml-sort>=0.24.2", "types-pyyaml>=6.0.12.20241230", + "syrupy>=4.9.1", ] agno = [ "agno>=1.2.6", diff --git a/test/serena/__snapshots__/test_symbol_editing.ambr b/test/serena/__snapshots__/test_symbol_editing.ambr new file mode 100644 index 0000000..c2f3716 --- /dev/null +++ b/test/serena/__snapshots__/test_symbol_editing.ambr @@ -0,0 +1,700 @@ +# serializer version: 1 +# name: test_delete_symbol[test_case0] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + # Module-level variable with type annotation + typed_module_var: int = 42 + + + # Regular class with class and instance variables + + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + # Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_delete_symbol[test_case1] + ''' + + + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + + helperFunction(); + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case0-after] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + # Module-level variable with type annotation + typed_module_var: int = 42 + new_module_var = "Inserted after typed_module_var" + + # Regular class with class and instance variables + class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + # Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case0-before] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + new_module_var = "Inserted after typed_module_var"# Module-level variable with type annotation + typed_module_var: int = 42 + + + # Regular class with class and instance variables + class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + # Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case1-after] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + # Module-level variable with type annotation + typed_module_var: int = 42 + + + # Regular class with class and instance variables + class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + # Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + def new_inserted_function(): + print("This is a new function inserted before another.") + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case1-before] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + # Module-level variable with type annotation + typed_module_var: int = 42 + + + # Regular class with class and instance variables + class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + def modify_instance_var(self): + # Reassign instance variable + self.instance_var = "Modified instance value" + self.reassignable_instance_var = 200 # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + def new_inserted_function(): + print("This is a new function inserted before another.")# Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case2-after] + ''' + export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + printValue() { + console.log(this.value); + } + } + function newFunctionAfterClass(): void { + console.log("This function is after DemoClass."); + # } + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + + helperFunction(); + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case2-before] + ''' + function newFunctionAfterClass(): void { + console.log("This function is after DemoClass."); + # }export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + printValue() { + console.log(this.value); + } + } + + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + + helperFunction(); + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case3-after] + ''' + export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + printValue() { + console.log(this.value); + } + } + + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + function newInsertedFunction(): void { + console.log("This is a new function inserted before another."); + } + helperFunction(); + + ''' +# --- +# name: test_insert_in_rel_to_symbol[test_case3-before] + ''' + export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + printValue() { + console.log(this.value); + } + } + function newInsertedFunction(): void { + console.log("This is a new function inserted before another."); + } + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + + helperFunction(); + + ''' +# --- +# name: test_replace_body[test_case0] + ''' + """ + Test module for variable declarations and usage. + + This module tests various types of variable declarations and usages including: + - Module-level variables + - Class-level variables + - Instance variables + - Variable reassignments + """ + + from dataclasses import dataclass, field + + # Module-level variables + module_var = "Initial module value" + + reassignable_module_var = 10 + reassignable_module_var = 20 # Reassigned + + # Module-level variable with type annotation + typed_module_var: int = 42 + + + # Regular class with class and instance variables + class VariableContainer: + """Class that contains various variables.""" + + # Class-level variables + class_var = "Initial class value" + + reassignable_class_var = True + reassignable_class_var = False # Reassigned #noqa: PIE794 + + # Class-level variable with type annotation + typed_class_var: str = "typed value" + + def __init__(self): + # Instance variables + self.instance_var = "Initial instance value" + self.reassignable_instance_var = 100 + + # Instance variable with type annotation + self.typed_instance_var: list[str] = ["item1", "item2"] + + # This body has been replaced + self.instance_var = "Replaced!" + self.reassignable_instance_var = 999 + # Reassigned + + def use_module_var(self): + # Use module-level variables + result = module_var + " used in method" + other_result = reassignable_module_var + 5 + return result, other_result + + def use_class_var(self): + # Use class-level variables + result = VariableContainer.class_var + " used in method" + other_result = VariableContainer.reassignable_class_var + return result, other_result + + + # Dataclass with variables + @dataclass + class VariableDataclass: + """Dataclass that contains various fields.""" + + # Field variables with type annotations + id: int + name: str + items: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) + optional_value: float | None = None + + # This will be reassigned in various places + status: str = "pending" + + + # Function that uses the module variables + def use_module_variables(): + """Function that uses module-level variables.""" + result = module_var + " used in function" + other_result = reassignable_module_var * 2 + return result, other_result + + + # Create instances and use variables + dataclass_instance = VariableDataclass(id=1, name="Test") + dataclass_instance.status = "active" # Reassign dataclass field + + # Use variables at module level + module_result = module_var + " used at module level" + other_module_result = reassignable_module_var + 30 + + # Create a second dataclass instance with different status + second_dataclass = VariableDataclass(id=2, name="Another Test") + second_dataclass.status = "completed" # Another reassignment of status + + ''' +# --- +# name: test_replace_body[test_case1] + ''' + export class DemoClass { + value: number; + constructor(value: number) { + this.value = value; + } + // This body has been replaced + console.warn("New value: " + this.value); + + } + + export function helperFunction() { + const demo = new DemoClass(42); + demo.printValue(); + } + + helperFunction(); + + ''' +# --- diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index dcbfe70..e80a898 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -5,6 +5,7 @@ from abc import abstractmethod from collections.abc import Iterator from contextlib import contextmanager from pathlib import Path +from typing import Literal import pytest @@ -30,35 +31,38 @@ class EditingTest: """Context manager for setup/teardown with a temporary directory, providing the symbol manager.""" temp_dir = Path(tempfile.mkdtemp()) self.repo_path = temp_dir / self.original_repo_path.name + language_server = None # Initialize language_server try: shutil.copytree(self.original_repo_path, self.repo_path) language_server = create_ls(self.language, str(self.repo_path)) language_server.start() yield SymbolManager(lang_server=language_server) finally: + if language_server is not None and language_server.is_running(): + language_server.stop() shutil.rmtree(temp_dir) def _read_file(self, rel_path: str) -> str: """Read the content of a file in the test repository.""" + assert self.repo_path is not None file_path = self.repo_path / rel_path with open(file_path, encoding="utf-8") as f: return f.read() - def run_test(self) -> None: + def run_test(self, content_after_ground_truth: str) -> None: with self._setup() as symbol_manager: content_before = self._read_file(self.rel_path) self._apply_edit(symbol_manager) content_after = self._read_file(self.rel_path) code_diff = CodeDiff(self.rel_path, original_content=content_before, modified_content=content_after) - self._test_diff(code_diff) + self._test_diff(code_diff, content_after_ground_truth) @abstractmethod def _apply_edit(self, symbol_manager: SymbolManager) -> None: pass - @abstractmethod - def _test_diff(self, code_diff: CodeDiff) -> None: - pass + def _test_diff(self, code_diff: CodeDiff, snapshot) -> None: + assert code_diff.modified_content == snapshot # Python test file path @@ -67,73 +71,16 @@ PYTHON_TEST_REL_FILE_PATH = os.path.join("test_repo", "variables.py") # TypeScript test file path TYPESCRIPT_TEST_FILE = "index.ts" -# Expected deleted lines for Python VariableContainer -EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON = '''class VariableContainer: - """Class that contains various variables.""" - - # Class-level variables - class_var = "Initial class value" - - reassignable_class_var = True - reassignable_class_var = False # Reassigned #noqa: PIE794 - - # Class-level variable with type annotation - typed_class_var: str = "typed value" - - def __init__(self): - # Instance variables - self.instance_var = "Initial instance value" - self.reassignable_instance_var = 100 - - # Instance variable with type annotation - self.typed_instance_var: list[str] = ["item1", "item2"] - - def modify_instance_var(self): - # Reassign instance variable - self.instance_var = "Modified instance value" - self.reassignable_instance_var = 200 # Reassigned - - def use_module_var(self): - # Use module-level variables - result = module_var + " used in method" - other_result = reassignable_module_var + 5 - return result, other_result - - def use_class_var(self): - # Use class-level variables - result = VariableContainer.class_var + " used in method" - other_result = VariableContainer.reassignable_class_var - return result, other_result -''' - -# Expected deleted lines for TypeScript DemoClass -EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT = """export class DemoClass { - value: number; - constructor(value: number) { - this.value = value; - } - printValue() { - console.log(this.value); - } -}""" - class DeleteSymbolTest(EditingTest): - def __init__(self, language: Language, rel_path: str, deleted_symbol: str, expected_deleted_lines: str): + def __init__(self, language: Language, rel_path: str, deleted_symbol: str): super().__init__(language, rel_path) - self.expected_deleted_lines = expected_deleted_lines self.deleted_symbol = deleted_symbol self.rel_path = rel_path def _apply_edit(self, symbol_manager: SymbolManager) -> None: symbol_manager.delete_symbol(self.deleted_symbol, self.rel_path) - def _test_diff(self, code_diff: CodeDiff) -> None: - assert code_diff.original_content != code_diff.modified_content - actual_deleted_lines = [line.strip() for _, line in code_diff.deleted_lines if line.strip()] - normalized_expected_deleted_lines = [line.strip() for line in self.expected_deleted_lines.splitlines() if line.strip()] - assert actual_deleted_lines == normalized_expected_deleted_lines - @pytest.mark.parametrize( "test_case", @@ -142,18 +89,16 @@ class DeleteSymbolTest(EditingTest): Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "VariableContainer", - EXPECTED_DELETED_VARIABLE_CONTAINER_PYTHON, ), DeleteSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "DemoClass", - EXPECTED_DELETED_DEMOCLASS_TYPESCRIPT, ), ], ) -def test_delete_symbol(test_case): - test_case.run_test() +def test_delete_symbol(test_case, snapshot): + test_case.run_test(content_after_ground_truth=snapshot) NEW_PYTHON_FUNCTION = """def new_inserted_function(): @@ -168,47 +113,50 @@ NEW_PYTHON_VARIABLE = 'new_module_var = "Inserted after typed_module_var"' NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { console.log("This function is after DemoClass."); -}""" +# }""" -class InsertBeforeSymbolTest(EditingTest): +class InsertInRelToSymbolTest(EditingTest): def __init__(self, language: Language, rel_path: str, symbol_name: str, new_content: str): super().__init__(language, rel_path) self.symbol_name = symbol_name self.new_content = new_content + self.mode: Literal["before", "after"] | None = None + + def set_mode(self, mode: Literal["before", "after"]): + self.mode = mode def _apply_edit(self, symbol_manager: SymbolManager) -> None: - symbol_manager.insert_before_symbol(self.symbol_name, self.rel_path, self.new_content) - - def _test_diff(self, code_diff: CodeDiff) -> None: - assert code_diff.original_content != code_diff.modified_content - actual_added_lines = [line.strip() for _, line in code_diff.added_lines if line.strip()] - normalized_expected_added_lines = [line.strip() for line in self.new_content.splitlines() if line.strip()] - assert actual_added_lines == normalized_expected_added_lines + assert self.mode is not None + if self.mode == "before": + symbol_manager.insert_before_symbol(self.symbol_name, self.rel_path, self.new_content) + elif self.mode == "after": + symbol_manager.insert_after_symbol(self.symbol_name, self.rel_path, self.new_content) +@pytest.mark.parametrize("mode", ["before", "after"]) @pytest.mark.parametrize( "test_case", [ - InsertBeforeSymbolTest( + InsertInRelToSymbolTest( Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "typed_module_var", NEW_PYTHON_VARIABLE, ), - InsertBeforeSymbolTest( + InsertInRelToSymbolTest( Language.PYTHON, PYTHON_TEST_REL_FILE_PATH, "use_module_variables", NEW_PYTHON_FUNCTION, ), - InsertBeforeSymbolTest( + InsertInRelToSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "DemoClass", NEW_TYPESCRIPT_FUNCTION_AFTER, ), - InsertBeforeSymbolTest( + InsertInRelToSymbolTest( Language.TYPESCRIPT, TYPESCRIPT_TEST_FILE, "helperFunction", @@ -216,8 +164,9 @@ class InsertBeforeSymbolTest(EditingTest): ), ], ) -def test_insert_before_symbol(test_case): - test_case.run_test() +def test_insert_in_rel_to_symbol(test_case, mode, snapshot): + test_case.set_mode(mode) + test_case.run_test(content_after_ground_truth=snapshot) PYTHON_REPLACED_BODY = """ # This body has been replaced @@ -229,19 +178,9 @@ TYPESCRIPT_REPLACED_BODY = """ // This body has been replaced console.warn("New value: " + this.value); """ -EXPECTED_ORIGINAL_MODIFY_INSTANCE_VAR_PYTHON = """ def modify_instance_var(self): - # Reassign instance variable - self.instance_var = "Modified instance value" - self.reassignable_instance_var = 200 # Reassigned""" - -# For single line original content, direct list is fine -EXPECTED_ORIGINAL_PRINTVALUE_TYPESCRIPT = [" printValue() {", " console.log(this.value);", " }"] - class ReplaceBodyTest(EditingTest): - def __init__( - self, language: Language, rel_path: str, symbol_name: str, new_body: str, expected_removed_lines: str, expected_added_lines: str - ): + def __init__(self, language: Language, rel_path: str, symbol_name: str, new_body: str): super().__init__(language, rel_path) self.symbol_name = symbol_name self.new_body = new_body @@ -249,16 +188,23 @@ class ReplaceBodyTest(EditingTest): def _apply_edit(self, symbol_manager: SymbolManager) -> None: symbol_manager.replace_body(self.symbol_name, self.rel_path, self.new_body) - def _test_diff(self, code_diff: CodeDiff) -> None: - assert code_diff.original_content != code_diff.modified_content - # TODO test properly - @pytest.mark.parametrize( "test_case", [ - # TODO add tests + ReplaceBodyTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "VariableContainer/modify_instance_var", + PYTHON_REPLACED_BODY, + ), + ReplaceBodyTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass/printValue", + TYPESCRIPT_REPLACED_BODY, + ), ], ) -def test_replace_body(test_case: ReplaceBodyTest): - test_case.run_test() +def test_replace_body(test_case: ReplaceBodyTest, snapshot): + test_case.run_test(content_after_ground_truth=snapshot) diff --git a/uv.lock b/uv.lock index b9d538e..c098ce2 100644 --- a/uv.lock +++ b/uv.lock @@ -1006,6 +1006,7 @@ dev = [ { name = "poethepoet" }, { name = "pytest" }, { name = "ruff" }, + { name = "syrupy" }, { name = "toml-sort" }, { name = "types-pyyaml" }, ] @@ -1043,6 +1044,7 @@ requires-dist = [ { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.0.285" }, { name = "sensai-utils", specifier = ">=1.4.0" }, { name = "sqlalchemy", marker = "extra == 'agno'", specifier = ">=2.0.40" }, + { name = "syrupy", marker = "extra == 'dev'", specifier = ">=4.9.1" }, { name = "toml-sort", marker = "extra == 'dev'", specifier = ">=0.24.2" }, { name = "types-pyyaml", specifier = ">=6.0.12.20241230" }, { name = "types-pyyaml", marker = "extra == 'dev'", specifier = ">=6.0.12.20241230" }, @@ -1136,6 +1138,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/a0/4b/528ccf7a982216885a1ff4908e886b8fb5f19862d1962f56a3fce2435a70/starlette-0.46.1-py3-none-any.whl", hash = "sha256:77c74ed9d2720138b25875133f3a2dae6d854af2ec37dceb56aef370c1d8a227", size = 71995, upload-time = "2025-03-08T10:55:32.662Z" }, ] +[[package]] +name = "syrupy" +version = "4.9.1" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "pytest" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/8c/f8/022d8704a3314f3e96dbd6bbd16ebe119ce30e35f41aabfa92345652fceb/syrupy-4.9.1.tar.gz", hash = "sha256:b7d0fcadad80a7d2f6c4c71917918e8ebe2483e8c703dfc8d49cdbb01081f9a4", size = 52492, upload-time = "2025-03-24T01:36:37.225Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/ec/9d/aef9ec5fd5a4ee2f6a96032c4eda5888c5c7cec65cef6b28c4fc37671d88/syrupy-4.9.1-py3-none-any.whl", hash = "sha256:b94cc12ed0e5e75b448255430af642516842a2374a46936dd2650cfb6dd20eda", size = 52214, upload-time = "2025-03-24T01:36:35.278Z" }, +] + [[package]] name = "tokenize-rt" version = "6.1.0" From 04fa309170b79b669478e9148f958dc68811792c Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Fri, 30 May 2025 01:36:17 +0200 Subject: [PATCH 09/17] Add type hints --- test/serena/test_symbol_editing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index e80a898..4de831d 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -164,7 +164,7 @@ class InsertInRelToSymbolTest(EditingTest): ), ], ) -def test_insert_in_rel_to_symbol(test_case, mode, snapshot): +def test_insert_in_rel_to_symbol(test_case: InsertInRelToSymbolTest, mode: Literal["before", "after"], snapshot): test_case.set_mode(mode) test_case.run_test(content_after_ground_truth=snapshot) From 9bb8c798e819e05d29573a90c8fdf4583874ce30 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Fri, 30 May 2025 16:55:55 +0200 Subject: [PATCH 10/17] Added pytest markers to editing tests --- test/serena/test_symbol_editing.py | 100 ++++++++++++++++++----------- 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index e80a898..955b112 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -85,15 +85,21 @@ class DeleteSymbolTest(EditingTest): @pytest.mark.parametrize( "test_case", [ - DeleteSymbolTest( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "VariableContainer", + pytest.param( + DeleteSymbolTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "VariableContainer", + ), + marks=pytest.mark.python, ), - DeleteSymbolTest( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "DemoClass", + pytest.param( + DeleteSymbolTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass", + ), + marks=pytest.mark.typescript, ), ], ) @@ -138,29 +144,41 @@ class InsertInRelToSymbolTest(EditingTest): @pytest.mark.parametrize( "test_case", [ - InsertInRelToSymbolTest( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "typed_module_var", - NEW_PYTHON_VARIABLE, + pytest.param( + InsertInRelToSymbolTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "typed_module_var", + NEW_PYTHON_VARIABLE, + ), + marks=pytest.mark.python, ), - InsertInRelToSymbolTest( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "use_module_variables", - NEW_PYTHON_FUNCTION, + pytest.param( + InsertInRelToSymbolTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "use_module_variables", + NEW_PYTHON_FUNCTION, + ), + marks=pytest.mark.python, ), - InsertInRelToSymbolTest( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "DemoClass", - NEW_TYPESCRIPT_FUNCTION_AFTER, + pytest.param( + InsertInRelToSymbolTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass", + NEW_TYPESCRIPT_FUNCTION_AFTER, + ), + marks=pytest.mark.typescript, ), - InsertInRelToSymbolTest( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "helperFunction", - NEW_TYPESCRIPT_FUNCTION, + pytest.param( + InsertInRelToSymbolTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "helperFunction", + NEW_TYPESCRIPT_FUNCTION, + ), + marks=pytest.mark.typescript, ), ], ) @@ -192,17 +210,23 @@ class ReplaceBodyTest(EditingTest): @pytest.mark.parametrize( "test_case", [ - ReplaceBodyTest( - Language.PYTHON, - PYTHON_TEST_REL_FILE_PATH, - "VariableContainer/modify_instance_var", - PYTHON_REPLACED_BODY, + pytest.param( + ReplaceBodyTest( + Language.PYTHON, + PYTHON_TEST_REL_FILE_PATH, + "VariableContainer/modify_instance_var", + PYTHON_REPLACED_BODY, + ), + marks=pytest.mark.python, ), - ReplaceBodyTest( - Language.TYPESCRIPT, - TYPESCRIPT_TEST_FILE, - "DemoClass/printValue", - TYPESCRIPT_REPLACED_BODY, + pytest.param( + ReplaceBodyTest( + Language.TYPESCRIPT, + TYPESCRIPT_TEST_FILE, + "DemoClass/printValue", + TYPESCRIPT_REPLACED_BODY, + ), + marks=pytest.mark.typescript, ), ], ) From 6cd4356a97c6d5e2b3e1e1e85237b2d8e185ac8a Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Sat, 31 May 2025 14:02:15 +0200 Subject: [PATCH 11/17] LS: finding symbol references now also includes the references' locations --- src/multilspy/language_server.py | 39 +++++++++++++------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/multilspy/language_server.py b/src/multilspy/language_server.py index f63f34b..2f4b8c7 100644 --- a/src/multilspy/language_server.py +++ b/src/multilspy/language_server.py @@ -23,7 +23,6 @@ from typing import AsyncIterator, Dict, Iterator, List, Optional, Tuple, Union, import pathspec -from serena.text_utils import LineType, MatchedConsecutiveLines, TextLine, search_files from . import multilspy_types from .lsp_protocol_handler import lsp_types as LSPTypes from .lsp_protocol_handler.lsp_constants import LSPConstants @@ -47,10 +46,19 @@ from .lsp_protocol_handler import lsp_types # since it caches (in-memory) file contents, so we can avoid reading from disk. # Moreover, the way we want to use the language server (for retrieving actual content), # it makes sense to have more content-related utils directly in it. +from serena.text_utils import LineType, MatchedConsecutiveLines, TextLine, search_files + GenericDocumentSymbol = Union[LSPTypes.DocumentSymbol, LSPTypes.SymbolInformation, multilspy_types.UnifiedSymbolInformation] +@dataclasses.dataclass(kw_only=True) +class ReferenceInSymbol: + """A symbol retrieved when requesting reference to a symbol, together with the location of the reference""" + symbol: multilspy_types.UnifiedSymbolInformation + line: int + character: int + @dataclasses.dataclass class LSPFileBuffer: """ @@ -683,22 +691,7 @@ class LanguageServer: """ with self.open_file(relative_file_path) as file_data: file_contents = file_data.contents - - line_contents = file_contents.split("\n") - start_lineno = max(0, line - context_lines_before) - end_lineno = min(len(line_contents) - 1, line + context_lines_after) - # instantiate TextLines with the write LineType - text_lines: list[TextLine] = [] - # before the line - for lineno in range(start_lineno, line): - text_lines.append(TextLine(line_number=lineno, line_content=line_contents[lineno], match_type=LineType.BEFORE_MATCH)) - # the line - text_lines.append(TextLine(line_number=line, line_content=line_contents[line], match_type=LineType.MATCH)) - # after the line - for lineno in range(line + 1, end_lineno + 1): - text_lines.append(TextLine(line_number=lineno, line_content=line_contents[lineno], match_type=LineType.AFTER_MATCH)) - - return MatchedConsecutiveLines(lines=text_lines, source_file_path=relative_file_path) + return MatchedConsecutiveLines.from_file_contents(file_contents, line=line, context_lines_before=context_lines_before, context_lines_after=context_lines_after, source_file_path=relative_file_path) async def request_completions( @@ -1234,7 +1227,7 @@ class LanguageServer: include_self: bool = False, include_body: bool = False, include_file_symbols: bool = False, - ) -> List[multilspy_types.UnifiedSymbolInformation]: + ) -> List[ReferenceInSymbol]: """ Finds all symbols that reference the symbol at the given location. This is similar to request_references but filters to only include symbols @@ -1251,7 +1244,7 @@ class LanguageServer: :param include_body: whether to include the body of the symbols in the result. :param include_file_symbols: whether to include references that are file symbols. This is often a fallback mechanism for when the reference cannot be resolved to a symbol. - :return: List of symbols that reference the target symbol. + :return: List of objects containing the symbol and the location of the reference. """ if not self.server_started: self.logger.log( @@ -1346,7 +1339,7 @@ class LanguageServer: ): incoming_symbol = containing_symbol if include_self: - result.append(containing_symbol) + result.append(ReferenceInSymbol(symbol=containing_symbol, line=ref_line, character=ref_col)) continue else: self.logger.log(f"Found self-reference for {incoming_symbol['name']}, skipping it since {include_self=}", logging.DEBUG) @@ -1368,7 +1361,7 @@ class LanguageServer: ) continue - result.append(containing_symbol) + result.append(ReferenceInSymbol(symbol=containing_symbol, line=ref_line, character=ref_col)) return result @@ -1917,7 +1910,7 @@ class SyncLanguageServer: include_imports: bool = True, include_self: bool = False, include_body: bool = False, include_file_symbols: bool = False, - ) -> List[multilspy_types.UnifiedSymbolInformation]: + ) -> List[ReferenceInSymbol]: """ Finds all symbols that reference the symbol at the given location. This is similar to request_references but filters to only include symbols @@ -1934,7 +1927,7 @@ class SyncLanguageServer: :param include_body: whether to include the body of the symbols in the result. :param include_file_symbols: whether to include references that are file symbols. This is often a fallback mechanism for when the reference cannot be resolved to a symbol. - :return: List of symbols that reference the target symbol. + :return: List of objects containing the symbol and the location of the reference. """ assert self.loop result = asyncio.run_coroutine_threadsafe( From fe6a7d04f420fb8d6c3814fc1aaeea3c544c3404 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Sat, 31 May 2025 14:03:45 +0200 Subject: [PATCH 12/17] Major: replace location-based symbol referencing by name_path based Also: 1. find_references now cannot return bodies, instead content around the reference is included 2. find_reference_snippets tool removed since it's no longer necessary --- scripts/demo_run_tools.py | 12 +- src/serena/agent.py | 143 ++++++------------ src/serena/symbol.py | 50 ++++-- src/serena/text_utils.py | 40 ++++- .../multilspy/python/test_symbol_retrieval.py | 32 ++-- 5 files changed, 142 insertions(+), 135 deletions(-) diff --git a/scripts/demo_run_tools.py b/scripts/demo_run_tools.py index 0ae262b..ebdafef 100644 --- a/scripts/demo_run_tools.py +++ b/scripts/demo_run_tools.py @@ -3,10 +3,10 @@ This script demonstrates how to use Serena's tools locally, useful for testing or development. Here the tools will be operation the serena repo itself. """ -import json from pprint import pprint from serena.agent import * +from serena.constants import REPO_ROOT @dataclass @@ -20,10 +20,10 @@ class InMemorySerenaConfig(SerenaConfigBase): if __name__ == "__main__": - project_path = str(Path("test") / "resources" / "repos" / "python" / "test_repo") - agent = SerenaAgent(project=project_path, serena_config=InMemorySerenaConfig()) + # project_path = str(Path("test") / "resources" / "repos" / "python" / "test_repo") + agent = SerenaAgent(project=REPO_ROOT) # apply a tool - find_symbol_tool = agent.get_tool(FindSymbolTool) - print("Finding the symbol 'VariableContainer'\n") - pprint(json.loads(find_symbol_tool.apply("VariableContainer", within_relative_path=str(Path("test_repo") / "variables.py")))) + find_refs_tool = agent.get_tool(FindReferencingSymbolsTool) + print("Finding the symbol 'SyncLanguageServer'\n") + pprint(json.loads(find_refs_tool.apply(name_path="SyncLanguageServer", relative_file_path="src/multilspy/language_server.py"))) diff --git a/src/serena/agent.py b/src/serena/agent.py index f9771be..da5bf5f 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -37,7 +37,7 @@ from serena.config import SerenaAgentContext, SerenaAgentMode from serena.constants import PROJECT_TEMPLATE_FILE, SERENA_MANAGED_DIR_NAME from serena.dashboard import MemoryLogHandler, SerenaDashboardAPI from serena.prompt_factory import PromptFactory, SerenaPromptFactory -from serena.symbol import SymbolLocation, SymbolManager +from serena.symbol import SymbolManager from serena.text_utils import search_files from serena.util.file_system import scan_directory from serena.util.general import load_yaml, save_yaml @@ -1196,98 +1196,50 @@ class FindReferencingSymbolsTool(Tool): def apply( self, - relative_path: str, - line: int, - column: int, - include_body: bool = False, + name_path: str, + relative_file_path: str, include_kinds: list[int] | None = None, exclude_kinds: list[int] | None = None, max_answer_chars: int = _DEFAULT_MAX_ANSWER_LENGTH, ) -> str: """ - Finds symbols that reference the symbol at the given location. + Finds symbols that reference the symbol at the given `name_path`. The result will contain metadata about the referencing symbols + as well as a short code snippet around the reference (unless `include_body` is True, then the short snippet will be omitted). Note that among other kinds of references, this function can be used to find (direct) subclasses of a class, as subclasses are referencing symbols that have the kind class. - :param relative_path: the relative path to the file containing the symbol - :param line: the line number - :param column: the column - :param include_body: whether to include the body of the symbols in the result. - Note that this might lead to a very long output, so you should only use this if you actually need the body - of the referencing symbols for the task at hand. Usually it is a better idea to find - the referencing symbols without the body and then use the find_symbol tool to get the body of - specific symbols if needed. - :param include_kinds: an optional list of integers representing the LSP symbol kinds to include. - If provided, only symbols of the given kinds will be included in the result. - Valid kinds: - 1=file, 2=module, 3=namespace, 4=package, 5=class, 6=method, 7=property, 8=field, 9=constructor, 10=enum, - 11=interface, 12=function, 13=variable, 14=constant, 15=string, 16=number, 17=boolean, 18=array, 19=object, - 20=key, 21=null, 22=enum member, 23=struct, 24=event, 25=operator, 26=type parameter - :param exclude_kinds: If provided, symbols of the given kinds will be excluded from the result. - Takes precedence over include_kinds. - :param max_answer_chars: if the output is longer than this number of characters, - no content will be returned. Don't adjust unless there is really no other way to get the content - required for the task. Instead, if the output is too long, you should - make a stricter query. + :param name_path: for finding the symbol to find references for, same logic as in the `find_symbol` tool. + :param relative_file_path: the relative path to the file containing the symbol for which to find references. + :param include_kinds: same as in the `find_symbol` tool. + :param exclude_kinds: same as in the `find_symbol` tool. + :param max_answer_chars: same as in the `find_symbol` tool. :return: a list of JSON objects with the symbols referencing the requested symbol """ + include_body = False # It is probably never a good idea to include the body of the referencing symbols parsed_include_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in include_kinds] if include_kinds else None parsed_exclude_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in exclude_kinds] if exclude_kinds else None - symbols = self.symbol_manager.find_referencing_symbols_by_location( - SymbolLocation(relative_path, line, column), + references_in_symbols = self.symbol_manager.find_referencing_symbols( + name_path, + relative_file_path=relative_file_path, include_body=include_body, include_kinds=parsed_include_kinds, exclude_kinds=parsed_exclude_kinds, ) - symbol_dicts = [s.to_dict(kind=True, location=True, depth=0, include_body=include_body) for s in symbols] - result = json.dumps(symbol_dicts) + reference_dicts = [] + for ref in references_in_symbols: + ref_dict = ref.symbol.to_dict(kind=True, location=True, depth=0, include_body=include_body) + if not include_body: + ref_relative_path = ref.symbol.location.relative_path + assert ref_relative_path is not None, f"Referencing symbol {ref.symbol.name} has no relative path, this is likely a bug." + content_around_ref = self.language_server.retrieve_content_around_line( + relative_file_path=ref_relative_path, line=ref.line, context_lines_before=1, context_lines_after=1 + ) + ref_dict["content_around_reference"] = content_around_ref.to_display_string() + reference_dicts.append(ref_dict) + result = json.dumps(reference_dicts) return self._limit_length(result, max_answer_chars) -class FindReferencingCodeSnippetsTool(Tool): - """ - Finds code snippets in which the symbol at the given location is referenced. - """ - - def apply( - self, - relative_path: str, - line: int, - column: int, - context_lines_before: int = 0, - context_lines_after: int = 0, - max_answer_chars: int = _DEFAULT_MAX_ANSWER_LENGTH, - ) -> str: - """ - Returns short code snippets where the symbol at the given location is referenced. - - Contrary to the `find_referencing_symbols` tool, this tool returns references that are not symbols but instead - code snippets that may or may not be contained in a symbol (for example, file-level calls). - It may make sense to use this tool to get a quick overview of the code that references - the symbol. Usually, just looking at code snippets is not enough to understand the full context, - unless the case you are investigating is very simple, - or you already have read the relevant symbols using the find_referencing_symbols tool and - now want to get an overview of how the referenced symbol (at the given location) is used in them. - The size of the snippets is controlled by the context_lines_before and context_lines_after parameters. - - :param relative_path: the relative path to the file containing the symbol - :param line: the line number of the symbol to find references for - :param column: the column of the symbol to find references for - :param context_lines_before: the number of lines to include before the line containing the reference - :param context_lines_after: the number of lines to include after the line containing the reference - :param max_answer_chars: if the output is longer than this number of characters, - no content will be returned. Don't adjust unless there is really no other way to get the content - required for the task. Instead, if the output is too long, you should - make a stricter query. - """ - matches = self.language_server.request_references_with_content( - relative_path, line, column, context_lines_before, context_lines_after - ) - result = [match.to_display_string() for match in matches] - result_json_str = json.dumps(result) - return self._limit_length(result_json_str, max_answer_chars) - - class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): """ Replaces the full definition of a symbol. @@ -1295,23 +1247,21 @@ class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): def apply( self, + name_path: str, relative_path: str, - line: int, - column: int, body: str, ) -> str: """ - Replaces the body of the symbol at the given location. - Important: Do not try to guess symbol locations but instead use the find_symbol tool to get the correct location. + Replaces the body of the symbol with the given `name_path`. + :param name_path: for finding the symbol to replace, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol - :param line: the line number - :param column: the column :param body: the new symbol body. Important: Provide the correct level of indentation (as the original body). Note that the first line must not be indented (i.e. no leading spaces). """ - self.symbol_manager.replace_body_at_location( - SymbolLocation(relative_path, line, column), + self.symbol_manager.replace_body( + name_path, + relative_file_path=relative_path, body=body, ) return SUCCESS_RESULT @@ -1324,23 +1274,22 @@ class InsertAfterSymbolTool(Tool, ToolMarkerCanEdit): def apply( self, + name_path: str, relative_path: str, - line: int, - column: int, body: str, ) -> str: """ Inserts the given body/content after the end of the definition of the given symbol (via the symbol's location). A typical use case is to insert a new class, function, method, field or variable assignment. + :param name_path: for finding the symbol to insert after, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol - :param line: the line number - :param column: the column - :param body: the body/content to be inserted + :param body: the body/content to be inserted. Important: the insterted code will automatically have the + same indentation as the symbol's body, so you do not need to provide any indentation. """ - location = SymbolLocation(relative_path, line, column) - self.symbol_manager.insert_after_symbol_at_location( - location, + self.symbol_manager.insert_after_symbol( + name_path, + relative_file_path=relative_path, body=body, ) return SUCCESS_RESULT @@ -1353,9 +1302,8 @@ class InsertBeforeSymbolTool(Tool, ToolMarkerCanEdit): def apply( self, + name_path: str, relative_path: str, - line: int, - column: int, body: str, ) -> str: """ @@ -1363,13 +1311,14 @@ class InsertBeforeSymbolTool(Tool, ToolMarkerCanEdit): A typical use case is to insert a new class, function, method, field or variable assignment. It also can be used to insert a new import statement before the first symbol in the file. + :param name_path: for finding the symbol to insert before, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol - :param line: the line number - :param column: the column - :param body: the body/content to be inserted + :param body: the body/content to be inserted. Important: the insterted code will automatically have the + same indentation as the symbol's body, so you do not need to provide any indentation. """ - self.symbol_manager.insert_before_symbol_at_location( - SymbolLocation(relative_path, line, column), + self.symbol_manager.insert_before_symbol( + name_path, + relative_file_path=relative_path, body=body, ) return SUCCESS_RESULT diff --git a/src/serena/symbol.py b/src/serena/symbol.py index a284a14..9f96df3 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING, Any, Literal, NamedTuple, Self, Union from sensai.util.string import ToStringMixin from multilspy import SyncLanguageServer +from multilspy.language_server import ReferenceInSymbol as LSPReferenceInSymbol from multilspy.multilspy_types import Position, SymbolKind, UnifiedSymbolInformation if TYPE_CHECKING: @@ -468,6 +469,24 @@ class Symbol(ToStringMixin): return result +@dataclass +class ReferenceInSymbol(ToStringMixin): + """Same as the class of the same name in the language server, but using Serena's Symbol class. + Be careful to not confuse it with counterpart! + """ + + symbol: Symbol + line: int + character: int + + def get_relative_path(self) -> str | None: + return self.symbol.location.relative_path + + @classmethod + def from_lsp_reference(cls, reference: LSPReferenceInSymbol) -> Self: + return cls(symbol=Symbol(reference.symbol), line=reference.line, character=reference.character) + + class SymbolManager: def __init__(self, lang_server: SyncLanguageServer, agent: Union["SerenaAgent", None] = None) -> None: """ @@ -478,9 +497,6 @@ class SymbolManager: self.lang_server = lang_server self.agent = agent - def _to_symbols(self, items: list[UnifiedSymbolInformation]) -> list[Symbol]: - return [Symbol(s) for s in items] - def find_by_name( self, name_path: str, @@ -527,27 +543,33 @@ class SymbolManager: include_body: bool = False, include_kinds: Sequence[SymbolKind] | None = None, exclude_kinds: Sequence[SymbolKind] | None = None, - ) -> tuple[Symbol, list[Symbol]] | None: + ) -> list[ReferenceInSymbol]: """ Find all symbols that reference the symbol with the given name. If multiple symbols fit the name (e.g. for variables that are overwritten), will use the first one. + + :param name_path: the name path of the symbol to find + :param relative_file_path: the relative path of the file in which the referenced symbol is defined. + :param include_body: whether to include the body of all symbols in the result. + Not recommended, as the referencing symbols will often be files, and thus the bodies will be very long. + :param include_kinds: which kinds of symbols to include in the result. + :param exclude_kinds: which kinds of symbols to exclude from the result. """ symbol_candidates = self.find_by_name(name_path, substring_matching=False, within_relative_path=relative_file_path) if len(symbol_candidates) == 0: log.warning(f"No symbol with name {name_path} found in file {relative_file_path}") - return None + return [] if len(symbol_candidates) > 1: log.error( f"Found {len(symbol_candidates)} symbols with name {name_path} in file {relative_file_path}." f"May be an overwritten variable, in which case you can ignore this error. Proceeding with the first one. " f"Found symbols for {name_path=} in {relative_file_path=}: \n" - f"" + f"{json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2)}" ) symbol = symbol_candidates[0] - referencing_symbols = self.find_referencing_symbols_by_location( + return self.find_referencing_symbols_by_location( symbol.location, include_body=include_body, include_kinds=include_kinds, exclude_kinds=exclude_kinds ) - return symbol, referencing_symbols def find_referencing_symbols_by_location( self, @@ -555,13 +577,14 @@ class SymbolManager: include_body: bool = False, include_kinds: Sequence[SymbolKind] | None = None, exclude_kinds: Sequence[SymbolKind] | None = None, - ) -> list[Symbol]: + ) -> list[ReferenceInSymbol]: """ Find all symbols that reference the symbol at the given location. :param symbol_location: the location of the symbol for which to find references. Does not need to include an end_line, as it is unused in the search. :param include_body: whether to include the body of all symbols in the result. + Not recommended, as the referencing symbols will often be files, and thus the bodies will be very long. Note: you can filter out the bodies of the children if you set include_children_body=False in the to_dict method. :param include_kinds: an optional sequence of ints representing the LSP symbol kind. @@ -575,22 +598,23 @@ class SymbolManager: assert symbol_location.relative_path is not None assert symbol_location.line is not None assert symbol_location.column is not None - symbol_dicts = self.lang_server.request_referencing_symbols( + references = self.lang_server.request_referencing_symbols( relative_file_path=symbol_location.relative_path, line=symbol_location.line, column=symbol_location.column, include_imports=False, include_self=False, include_body=include_body, + include_file_symbols=True, ) if include_kinds is not None: - symbol_dicts = [s for s in symbol_dicts if s["kind"] in include_kinds] + references = [s for s in references if s.symbol["kind"] in include_kinds] if exclude_kinds is not None: - symbol_dicts = [s for s in symbol_dicts if s["kind"] not in exclude_kinds] + references = [s for s in references if s.symbol["kind"] not in exclude_kinds] - return self._to_symbols(symbol_dicts) + return [ReferenceInSymbol.from_lsp_reference(r) for r in references] @contextmanager def _edited_file(self, relative_path: str) -> Iterator[None]: diff --git a/src/serena/text_utils.py b/src/serena/text_utils.py index 6cab02a..338b295 100644 --- a/src/serena/text_utils.py +++ b/src/serena/text_utils.py @@ -3,7 +3,7 @@ import re from collections.abc import Callable from dataclasses import dataclass, field from enum import StrEnum -from typing import Any +from typing import Any, Self from joblib import Parallel, delayed from pathspec import PathSpec @@ -38,11 +38,16 @@ class TextLine: return " >" return "..." - def format_line(self) -> str: - """Format the line for display with line number and content.""" + def format_line(self, include_line_numbers: bool = True) -> str: + """Format the line for display (e.g.,for logging or passing to an LLM). + + :param include_line_numbers: Whether to include the line number in the result. + """ prefix = self.get_display_prefix() - line_num = str(self.line_number).rjust(4) - return f"{prefix}{line_num}: {self.line_content}" + if include_line_numbers: + line_num = str(self.line_number).rjust(4) + prefix = f"{prefix}{line_num}" + return f"{prefix}:{self.line_content}" @dataclass(kw_only=True) @@ -52,7 +57,7 @@ class MatchedConsecutiveLines: """ lines: list[TextLine] - """All lines in the context of the match. At least one of them should be of match_type MATCH.""" + """All lines in the context of the match. At least one of them is of `match_type` `MATCH`.""" source_file_path: str | None = None """Path to the file where the match was found (Metadata).""" @@ -84,8 +89,27 @@ class MatchedConsecutiveLines: def num_matched_lines(self) -> int: return len(self.matched_lines) - def to_display_string(self) -> str: - return "\n".join([line.format_line() for line in self.lines]) + def to_display_string(self, include_line_numbers: bool = True) -> str: + return "\n".join([line.format_line(include_line_numbers) for line in self.lines]) + + @classmethod + def from_file_contents( + cls, file_contents: str, line: int, context_lines_before: int = 0, context_lines_after: int = 0, source_file_path: str | None = None + ) -> Self: + line_contents = file_contents.split("\n") + start_lineno = max(0, line - context_lines_before) + end_lineno = min(len(line_contents) - 1, line + context_lines_after) + text_lines: list[TextLine] = [] + # before the line + for lineno in range(start_lineno, line): + text_lines.append(TextLine(line_number=lineno, line_content=line_contents[lineno], match_type=LineType.BEFORE_MATCH)) + # the line + text_lines.append(TextLine(line_number=line, line_content=line_contents[line], match_type=LineType.MATCH)) + # after the line + for lineno in range(line + 1, end_lineno + 1): + text_lines.append(TextLine(line_number=lineno, line_content=line_contents[lineno], match_type=LineType.AFTER_MATCH)) + + return cls(lines=text_lines, source_file_path=source_file_path) def search_text( diff --git a/test/multilspy/python/test_symbol_retrieval.py b/test/multilspy/python/test_symbol_retrieval.py index 61a83ce..ba5bd61 100644 --- a/test/multilspy/python/test_symbol_retrieval.py +++ b/test/multilspy/python/test_symbol_retrieval.py @@ -40,7 +40,7 @@ class TestLanguageServerSymbols: """Test request_referencing_symbols for a variable.""" file_path = os.path.join("test_repo", "variables.py") # Line 75 contains the field status that is later modified - ref_symbols = language_server.request_referencing_symbols(file_path, 74, 4) + ref_symbols = [ref.symbol for ref in language_server.request_referencing_symbols(file_path, 74, 4)] assert len(ref_symbols) > 0 ref_lines = [ref["location"]["range"]["start"]["line"] for ref in ref_symbols if "location" in ref and "range" in ref["location"]] @@ -111,7 +111,9 @@ class TestLanguageServerSymbols: if not create_user_symbol or "selectionRange" not in create_user_symbol: raise AssertionError("create_user symbol or its selectionRange not found") sel_start = create_user_symbol["selectionRange"]["start"] - ref_symbols = language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ref_symbols = [ + ref.symbol for ref in language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ] assert len(ref_symbols) > 0, "No referencing symbols found for create_user (selectionRange)" # Verify the structure of referencing symbols @@ -133,7 +135,9 @@ class TestLanguageServerSymbols: if not user_symbol or "selectionRange" not in user_symbol: raise AssertionError("User symbol or its selectionRange not found") sel_start = user_symbol["selectionRange"]["start"] - ref_symbols = language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ref_symbols = [ + ref.symbol for ref in language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ] services_references = [ symbol for symbol in ref_symbols @@ -152,7 +156,9 @@ class TestLanguageServerSymbols: if not get_user_symbol or "selectionRange" not in get_user_symbol: raise AssertionError("get_user symbol or its selectionRange not found") sel_start = get_user_symbol["selectionRange"]["start"] - ref_symbols = language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ref_symbols = [ + ref.symbol for ref in language_server.request_referencing_symbols(file_path, sel_start["line"], sel_start["character"]) + ] method_refs = [ symbol for symbol in ref_symbols @@ -169,7 +175,7 @@ class TestLanguageServerSymbols: file_path = os.path.join("test_repo", "services.py") # Line 3 is a blank line or comment try: - ref_symbols = language_server.request_referencing_symbols(file_path, 3, 0) + ref_symbols = [ref.symbol for ref in language_server.request_referencing_symbols(file_path, 3, 0)] # If we get here, make sure we got an empty result assert ref_symbols == [] or ref_symbols is None except Exception: @@ -448,15 +454,19 @@ class TestLanguageServerSymbols: # Get the containing symbol of a variable in a file file_path = os.path.join("test_repo", "services.py") # import of typing - references_to_typing = language_server.request_referencing_symbols( - file_path, 4, 6, include_imports=False, include_file_symbols=True - ) + references_to_typing = [ + ref.symbol + for ref in language_server.request_referencing_symbols(file_path, 4, 6, include_imports=False, include_file_symbols=True) + ] assert {ref["kind"] for ref in references_to_typing} == {SymbolKind.File} assert {ref["body"] for ref in references_to_typing} == {""} # now include bodies - references_to_typing = language_server.request_referencing_symbols( - file_path, 4, 6, include_imports=False, include_file_symbols=True, include_body=True - ) + references_to_typing = [ + ref.symbol + for ref in language_server.request_referencing_symbols( + file_path, 4, 6, include_imports=False, include_file_symbols=True, include_body=True + ) + ] assert {ref["kind"] for ref in references_to_typing} == {SymbolKind.File} assert references_to_typing[0]["body"] From e8d5cefe9d59ea34f8c6c40a11995ec7b7902dcf Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Sat, 31 May 2025 14:12:38 +0200 Subject: [PATCH 13/17] Adjusted test --- test/serena/test_serena_agent.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/serena/test_serena_agent.py b/test/serena/test_serena_agent.py index 691c75d..a005a5d 100644 --- a/test/serena/test_serena_agent.py +++ b/test/serena/test_serena_agent.py @@ -92,7 +92,7 @@ class TestSerenaAgent: # sel_start = def_symbol["location"]["selectionRange"]["start"] # Now find references find_refs_tool = agent.get_tool(FindReferencingSymbolsTool) - result = find_refs_tool.apply(def_file, loc["line"], loc["column"]) + result = find_refs_tool.apply(name_path=def_symbol["name_path"], relative_file_path=loc["relative_path"]) refs = json.loads(result) assert any( ref["location"]["relative_path"] == ref_file for ref in refs From 950caaf91cd29690e877aee12ad3ce56e40c559f Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Sun, 1 Jun 2025 23:26:11 +0200 Subject: [PATCH 14/17] Add and use use_same_indentation flag to body-replacement --- src/serena/symbol.py | 39 +++++++++++++------ .../__snapshots__/test_symbol_editing.ambr | 25 +++++++++--- test/serena/test_symbol_editing.py | 17 +++++--- 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/serena/symbol.py b/src/serena/symbol.py index 9f96df3..e566f7f 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -643,9 +643,15 @@ class SymbolManager: """Get the content of a file using the language server.""" return self.lang_server.language_server.retrieve_full_file_content(relative_path) - def replace_body(self, name_path: str, relative_file_path: str, body: str) -> None: + def replace_body(self, name_path: str, relative_file_path: str, body: str, *, use_same_indentation: bool = True) -> None: """ - Replace the body of the symbol with the given name in the given file. + Replace the body of the symbol with the given name_path in the given file. + + :param name_path: the name path of the symbol to replace. + :param relative_file_path: the relative path of the file in which the symbol is defined. + :param body: the new body + :param use_same_indentation: whether to use the same indentation as the original body. This means that + the user doesn't have to provide the correct indentation, but can just write the body. """ symbol_candidates = self.find_by_name(name_path, within_relative_path=relative_file_path) if len(symbol_candidates) == 0: @@ -658,27 +664,33 @@ class SymbolManager: + json.dumps([s.location.to_dict() for s in symbol_candidates], indent=2) ) symbol = symbol_candidates[0] - return self.replace_body_at_location(symbol.location, body) + return self.replace_body_at_location(symbol.location, body, use_same_indentation=use_same_indentation) - def replace_body_at_location(self, location: SymbolLocation, body: str) -> None: + def replace_body_at_location(self, location: SymbolLocation, body: str, *, use_same_indentation: bool = True) -> None: """ Replace the body of the symbol at the given location with the given body :param location: the location of the symbol to replace. :param body: the new body + :param use_same_indentation: whether to use the same indentation as the original body. This means that + the user doesn't have to provide the correct indentation, but can just write the body. """ - # make sure body always ends with at least one newline - if not body.endswith("\n"): - body += "\n" - with self._edited_symbol_location(location) as symbol: assert location.relative_path is not None start_pos = symbol.body_start_position end_pos = symbol.body_end_position if start_pos is None or end_pos is None: raise ValueError(f"Symbol at {location} does not have a defined body range.") + start_line, start_col = start_pos["line"], start_pos["character"] + if use_same_indentation: + indent = " " * start_col + body = "\n".join(indent + line for line in body.splitlines()) + + # make sure body always ends with at least one newline + if not body.endswith("\n"): + body += "\n" self.lang_server.delete_text_between_positions(location.relative_path, start_pos, end_pos) - self.lang_server.insert_text_at_position(location.relative_path, start_pos["line"], start_pos["character"], body) + self.lang_server.insert_text_at_position(location.relative_path, start_line, start_col, body) def insert_after_symbol( self, @@ -731,14 +743,17 @@ class SymbolManager: raise ValueError(f"Symbol at {location} does not have a defined end position.") line, col = pos["line"], pos["character"] + if at_new_line: + line += 1 + col = 0 + if not body.startswith("\n"): + body = "\n" + body if use_same_indentation: symbol_start_pos = symbol.body_start_position assert symbol_start_pos is not None, f"Symbol at {location=} does not have a defined start position." symbol_identifier_col = symbol_start_pos["character"] indent = " " * (symbol_identifier_col) body = "\n".join(indent + line for line in body.splitlines()) - if at_new_line: - line += 1 # IMPORTANT: without this, the insertion does the wrong thing. See implementation of insert_text_at_position in TextUtils, # it is somewhat counterintuitive (never inserts whitespace) # I am not 100% sure whether col=0 is always the best choice here. @@ -807,6 +822,8 @@ class SymbolManager: if at_new_line: col = 0 line -= 1 + if not body.endswith("\n"): + body += "\n" assert location.relative_path is not None self.lang_server.insert_text_at_position(location.relative_path, line=line, column=col, text_to_be_inserted=body) diff --git a/test/serena/__snapshots__/test_symbol_editing.ambr b/test/serena/__snapshots__/test_symbol_editing.ambr index c2f3716..4fa7309 100644 --- a/test/serena/__snapshots__/test_symbol_editing.ambr +++ b/test/serena/__snapshots__/test_symbol_editing.ambr @@ -100,6 +100,7 @@ # Module-level variable with type annotation typed_module_var: int = 42 + new_module_var = "Inserted after typed_module_var" # Regular class with class and instance variables @@ -199,7 +200,8 @@ reassignable_module_var = 10 reassignable_module_var = 20 # Reassigned - new_module_var = "Inserted after typed_module_var"# Module-level variable with type annotation + new_module_var = "Inserted after typed_module_var" + # Module-level variable with type annotation typed_module_var: int = 42 @@ -365,6 +367,7 @@ result = module_var + " used in function" other_result = reassignable_module_var * 2 return result, other_result + def new_inserted_function(): print("This is a new function inserted before another.") @@ -462,7 +465,8 @@ def new_inserted_function(): - print("This is a new function inserted before another.")# Function that uses the module variables + print("This is a new function inserted before another.") + # Function that uses the module variables def use_module_variables(): """Function that uses module-level variables.""" result = module_var + " used in function" @@ -495,9 +499,10 @@ console.log(this.value); } } + function newFunctionAfterClass(): void { console.log("This function is after DemoClass."); - # } + } export function helperFunction() { const demo = new DemoClass(42); demo.printValue(); @@ -511,7 +516,8 @@ ''' function newFunctionAfterClass(): void { console.log("This function is after DemoClass."); - # }export class DemoClass { + } + export class DemoClass { value: number; constructor(value: number) { this.value = value; @@ -546,6 +552,7 @@ const demo = new DemoClass(42); demo.printValue(); } + function newInsertedFunction(): void { console.log("This is a new function inserted before another."); } @@ -567,6 +574,7 @@ function newInsertedFunction(): void { console.log("This is a new function inserted before another."); } + export function helperFunction() { const demo = new DemoClass(42); demo.printValue(); @@ -621,7 +629,9 @@ # Instance variable with type annotation self.typed_instance_var: list[str] = ["item1", "item2"] - # This body has been replaced + + def modify_instance_var(self): + # This body has been replaced self.instance_var = "Replaced!" self.reassignable_instance_var = 999 # Reassigned @@ -684,8 +694,11 @@ constructor(value: number) { this.value = value; } - // This body has been replaced + + function printValue() { + // This body has been replaced console.warn("New value: " + this.value); + } } diff --git a/test/serena/test_symbol_editing.py b/test/serena/test_symbol_editing.py index 6c9c35b..6b86fe8 100644 --- a/test/serena/test_symbol_editing.py +++ b/test/serena/test_symbol_editing.py @@ -119,7 +119,7 @@ NEW_PYTHON_VARIABLE = 'new_module_var = "Inserted after typed_module_var"' NEW_TYPESCRIPT_FUNCTION_AFTER = """function newFunctionAfterClass(): void { console.log("This function is after DemoClass."); -# }""" +}""" class InsertInRelToSymbolTest(EditingTest): @@ -187,13 +187,18 @@ def test_insert_in_rel_to_symbol(test_case: InsertInRelToSymbolTest, mode: Liter test_case.run_test(content_after_ground_truth=snapshot) -PYTHON_REPLACED_BODY = """ # This body has been replaced - self.instance_var = "Replaced!" - self.reassignable_instance_var = 999 +PYTHON_REPLACED_BODY = """ +def modify_instance_var(self): + # This body has been replaced + self.instance_var = "Replaced!" + self.reassignable_instance_var = 999 """ -TYPESCRIPT_REPLACED_BODY = """ // This body has been replaced - console.warn("New value: " + this.value); +TYPESCRIPT_REPLACED_BODY = """ +function printValue() { + // This body has been replaced + console.warn("New value: " + this.value); +} """ From 5392d376308cdc6649143632265a28efaf158bb2 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Mon, 2 Jun 2025 00:19:27 +0200 Subject: [PATCH 15/17] Prompting, tool descriptions. Overview tool no longer returns location info --- config/modes/editing.yml | 32 ++++++++++++++++++++++++++++++-- src/serena/agent.py | 32 +++++++++++++++++++++----------- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/config/modes/editing.yml b/config/modes/editing.yml index adb0277..431fe55 100644 --- a/config/modes/editing.yml +++ b/config/modes/editing.yml @@ -5,8 +5,36 @@ prompt: | Use symbolic editing tools whenever possible for precise code modifications. If no editing task has yet been provided, wait for the user to provide one. - Your primary tool for editing code is a regex-based replacement. You use other tools to find the relevant content and - then use your knowledge of the codebase to write the regex. + You have two main approaches for editing code - editing by regex and editing by symbol. + The symbol-based approach is appropriate if you need to adjust an entire symbol, e.g. a method, a class, a function, etc. + But it is not appropriate if you need to adjust just a few lines of code within a symbol, for that you should + use the regex-based approach that is described below. + + Let us first discuss the symbol-based approach. + Symbols are identified by their name path and relative file path, see the description of the `find_symbols` tool for more details + on how the `name_path` matches symbols. + You can get information about available symbols by using the `get_symbols_overview` tool for finding top-level symbols in a file + or directory, or by using `find_symbol` if you already know the symbol's name path. You generally try to read as little code as possible + while still solving your task, meaning you only read the bodies when you need to, and after you have found the symbol you want to edit. + For example, if you are working with python code and alread know that you need to read the body of the constructor of the class Foo, you can directly + use `find_symbol` with the name path `Foo/__init__` and `include_body=True`. If you don't know yet which methods in `Foo` you need to read or edit, + you can use `find_symbol` with the name path `Foo`, `include_body=False` and `depth=1` to get all (top-level) methods of `Foo` before proceeding + to read the desired methods with `include_body=True`. + Note that you never need to add additional indentation, as all symbol editing tools will automatically add the indentation of the symbol that + you are replacing or inserting above or below. In particular, keep in mind the description of the `replace_symbol_body` tool. If you want to add some new code at the end of the file, you should + use the `insert_after_symbol` tool with the last top-level symbol in the file. If you want to add an import, often a good strategy is to use + `insert_before_symbol` with the first top-level symbol in the file. + You can unterstand relationships between symbols by using the `find_referencing_symbols` tool. If not explicitly requested otherwise by a user, + you make sure that when you edit a symbol, it is either done in a backward-compatible way, or you find and adjust the references as needed. + The `find_referencing_symbols` tool will give you code snippets around the references, as well as symbolic information. + You will generally be able to use the info from the snippets and the regex-based approach to adjust the references as well. + You can assume that all symbol editing tools are reliable, so you don't need to verify the results if the tool returns without error. + + Now let us discuss the regex-based approach. + The regex-based approach is your primary tool for editing code whenever replacing or deleting a whole symbol would be a more expensive operation. + This is the case if you need to adjust just a few lines of code within a method, or a chunk that is much smaller than a whole symbol. + You use other tools to find the relevant content and + then use your knowledge of the codebase to write the regex, if you haven't collected enough information of this content yet. You are extremely good at regex, so you never need to check whether the replacement produced the correct result. In particular, you know what to escape and what not to escape, and you know how to use wildcards. Moreover, the replacement tool will fail if it can't perform the desired replacement, and this is all the feedback you need. diff --git a/src/serena/agent.py b/src/serena/agent.py index 3a27991..8985b65 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -1109,7 +1109,7 @@ class GetSymbolsOverviewTool(Tool): def apply(self, relative_path: str, max_answer_chars: int = _DEFAULT_MAX_ANSWER_LENGTH) -> str: """ Gets an overview of the given file or directory. - For each analyzed file, we list the top-level symbols in the file (name, kind, line). + For each analyzed file, we list the top-level symbols in the file (name_path, kind). Use this tool to get a high-level understanding of the code symbols. Calling this is often a good idea before more targeted reading, searching or editing operations on the code symbols. @@ -1118,12 +1118,14 @@ class GetSymbolsOverviewTool(Tool): no content will be returned. Don't adjust unless there is really no other way to get the content required for the task. If the overview is too long, you should use a smaller directory instead, (e.g. a subdirectory). - :return: a JSON object mapping relative paths of all contained files to info about top-level symbols in the file (name, kind, line, column). + :return: a JSON object mapping relative paths of all contained files to info about top-level symbols in the file (name_path, kind). """ path_to_symbol_infos = self.language_server.request_overview(relative_path) result = {} for file_path, symbols in path_to_symbol_infos.items(): - result[file_path] = [_tuple_to_info(*symbol_info) for symbol_info in symbols] + # TODO: maybe include not just top-level symbols? We could filter by kind to exclude variables + # The language server methods would need to be adjusted for this. + result[file_path] = [{"name_path": symbol[0], "kind": int(symbol[1])} for symbol in symbols] result_json_str = json.dumps(result) return self._limit_length(result_json_str, max_answer_chars) @@ -1268,16 +1270,26 @@ class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): ) -> str: """ Replaces the body of the symbol with the given `name_path`. + IMPORTANT: + You don't need to provide an adjusted indentation, + as the tool will automatically add the indentation of the original symbol to each line. For example, + for replacing a method in python, you can just write (using the standard python indentation): + body="def my_method_replacement(self, ...):\n first_line\n second_line...". So each line after the first line only has + an indentation of 4 (the indentation relative to the first characted), + since the additional indentation will be added by the tool. Same for more deeply nested + cases. You always only need to write the relative indentation to the first character of the first line, and that + in turn should not have any indentation. + ALWAYS REMEMBER TO USE THE CORRECT INDENTATION IN THE BODY! :param name_path: for finding the symbol to replace, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol - :param body: the new symbol body. Important: Provide the correct level of indentation - (as the original body). Note that the first line must not be indented (i.e. no leading spaces). + :param body: the new symbol body. """ self.symbol_manager.replace_body( name_path, relative_file_path=relative_path, body=body, + use_same_indentation=True, ) return SUCCESS_RESULT @@ -1300,12 +1312,13 @@ class InsertAfterSymbolTool(Tool, ToolMarkerCanEdit): :param name_path: for finding the symbol to insert after, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol :param body: the body/content to be inserted. Important: the insterted code will automatically have the - same indentation as the symbol's body, so you do not need to provide any indentation. + same indentation as the symbol's body, so you do not need to provide any additional indentation. """ self.symbol_manager.insert_after_symbol( name_path, relative_file_path=relative_path, body=body, + use_same_indentation=True, ) return SUCCESS_RESULT @@ -1329,12 +1342,13 @@ class InsertBeforeSymbolTool(Tool, ToolMarkerCanEdit): :param name_path: for finding the symbol to insert before, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol :param body: the body/content to be inserted. Important: the insterted code will automatically have the - same indentation as the symbol's body, so you do not need to provide any indentation. + same indentation as the symbol's body, so you do not need to provide any additional indentation. """ self.symbol_manager.insert_before_symbol( name_path, relative_file_path=relative_path, body=body, + use_same_indentation=True, ) return SUCCESS_RESULT @@ -1955,7 +1969,3 @@ class ToolRegistry: for tool_name in sorted(tool_dict.keys()): tool_class = tool_dict[tool_name] print(f" * `{tool_name}`: {tool_class.get_tool_description().strip()}") - - -def _tuple_to_info(name: str, symbol_type: SymbolKind, line: int, column: int) -> dict[str, int | str]: - return {"name": name, "symbol_kind": symbol_type, "line": line, "column": column} From 37ee90ae41ef9732311da5f800c462a622f35a0a Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Mon, 2 Jun 2025 00:43:11 +0200 Subject: [PATCH 16/17] Fix indentation in replace_body --- src/serena/agent.py | 10 ++++++---- src/serena/symbol.py | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index 8985b65..59b98f1 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -1268,14 +1268,15 @@ class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): relative_path: str, body: str, ) -> str: - """ + r""" Replaces the body of the symbol with the given `name_path`. - IMPORTANT: + + Important: You don't need to provide an adjusted indentation, as the tool will automatically add the indentation of the original symbol to each line. For example, for replacing a method in python, you can just write (using the standard python indentation): body="def my_method_replacement(self, ...):\n first_line\n second_line...". So each line after the first line only has - an indentation of 4 (the indentation relative to the first characted), + an indentation of 4 (the indentation relative to the first characted), since the additional indentation will be added by the tool. Same for more deeply nested cases. You always only need to write the relative indentation to the first character of the first line, and that in turn should not have any indentation. @@ -1283,7 +1284,8 @@ class ReplaceSymbolBodyTool(Tool, ToolMarkerCanEdit): :param name_path: for finding the symbol to replace, same logic as in the `find_symbol` tool. :param relative_path: the relative path to the file containing the symbol - :param body: the new symbol body. + :param body: the new symbol body. + """ self.symbol_manager.replace_body( name_path, diff --git a/src/serena/symbol.py b/src/serena/symbol.py index e566f7f..7f75098 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -684,7 +684,8 @@ class SymbolManager: start_line, start_col = start_pos["line"], start_pos["character"] if use_same_indentation: indent = " " * start_col - body = "\n".join(indent + line for line in body.splitlines()) + body_lines = body.splitlines() + body = body_lines[0] + "\n" + "\n".join(indent + line for line in body_lines[1:]) # make sure body always ends with at least one newline if not body.endswith("\n"): From fad38603340ea155b8b0942eb31d72e4c072cf6d Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Mon, 2 Jun 2025 00:57:12 +0200 Subject: [PATCH 17/17] Changelog [ci skip] --- CHANGELOG.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 573bd6d..42d5bc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,10 +3,17 @@ Status of the main branch. Changes prior to the next official version change wil ## Highlights +### This version is a major change and improvement of Serena + +* **Overhaul and major improvement of editing tools!** + This represents a very important change in Serena. Symbols can now be addressed by their name_path (including nested ones) + and we introduced a regex-based replaced tools. We tuned the prompts and tested the new editing mechanism. + It is much more reliable, flexible, and at the same time uses fewer tokens. + The line-replacement tools are disabled by default and deprecated, we will likely remove them soon. * **Better multi-project support and zero-config setup**: We significantly simplified the config setup, you no longer need to manually create `project.yaml` for each project. Project activation is now always available. Any project can now be activated by just asking the LLM to do so and passing the path to a repo. -* Dashboard as web app and possibility to shut down Serena during +* Dashboard as web app and possibility to shut down Serena from it (or the old log GUI). * Initial prompt for project supported (has to be added manually for the moment) * Massive performance improvement of pattern search tool