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
This commit is contained in:
Michael Panchenko
2025-05-29 13:37:10 +02:00
parent 3ba4c0c721
commit bd0923a2d3
4 changed files with 377 additions and 147 deletions
+5 -9
View File
@@ -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: {
+25 -1
View File
@@ -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:
+86 -137
View File
@@ -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: ...
+261
View File
@@ -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