Merge pull request #1703 from oraios/delete-symbol-clean-empty-lines

Add heuristic to remove superfluous empty lines after deleting a symbol

CHANGELOG.md

@@ -15,6 +15,7 @@ Status of the `main` branch. Changes prior to the next official version change w
     break, because the match's exclusive end index was mapped to a line number directly and therefore
     resolved to the start of the following line. The line the match ends on is now determined correctly,
     which also keeps `context_lines_after` aligned.
+  - `safe_delete`: Add heuristic to delete superfluous empty lines after a deletion
 
 * JetBrains:
   - Allow external files from dependencies (specified via references like "<ext:FileUtil.class|472e0a13>") to be

src/serena/code_editor.py

@@ -10,7 +10,7 @@ from serena.jetbrains.jetbrains_plugin_client import JetBrainsPluginClient
 from serena.symbol import JetBrainsSymbol, LanguageServerSymbol, LanguageServerSymbolRetriever, PositionInFile, Symbol
 from solidlsp import SolidLanguageServer, ls_types
 from solidlsp.ls import LSPFileBuffer
-from solidlsp.ls_utils import PathUtils, TextUtils
+from solidlsp.ls_utils import PathUtils, TextStepper, TextUtils
 
 from .project import Project
 from .util.file_proxy import FileProxy
@@ -242,9 +242,31 @@ class CodeEditor(Generic[TSymbol], ABC):
         symbol = self._find_unique_symbol(name_path, relative_file_path)
         start_pos = symbol.get_body_start_position_or_raise()
         end_pos = symbol.get_body_end_position_or_raise()
+
         with self.edited_file_context(relative_file_path) as edited_file:
+            # do the actual deletion
             edited_file.delete_text_between_positions(start_pos, end_pos)
 
+            # empty line removal heuristic:
+            # After the deletion, go to the line where the deleted symbol started,
+            # check whether it and any subsequent lines are empty, and if so, delete them.
+            # The empty lines preceding the deleted symbol's body should normally suffice as a separator
+            # between the symbols before and after the deleted symbol.
+            start_pos.col = 0
+            stepper = TextStepper(edited_file.get_contents())
+            stepper.step_to(start_pos.line, start_pos.col)
+            apply_empty_line_deletion = False
+            while stepper.step_line():
+                line = stepper.get_last_line(with_end=False)
+                if line.strip() == "":
+                    end_pos.line = stepper.line
+                    end_pos.col = stepper.col
+                    apply_empty_line_deletion = True
+                else:
+                    break
+            if apply_empty_line_deletion:
+                edited_file.delete_text_between_positions(start_pos, end_pos)
+
     @abstractmethod
     def rename_symbol(self, name_path: str, relative_path: str, new_name: str) -> str:
         pass

src/solidlsp/ls_utils.py

@@ -122,6 +122,21 @@ class TextStepper:
             self.is_newline = False
         return True
 
+    def step_to(self, line: int, col: int):
+        """
+        Steps through the text until the given line and column are reached, or until the end of the text is reached.
+
+        :param line: the 0-based line number to step to
+        :param col: the 0-based column number to step to
+        """
+        while self.line < line:
+            if not self.step_line():
+                break
+        if self.line != line:
+            raise InvalidTextLocationError
+        self.idx += col
+        self.col = col
+
     def process_all(self):
         """
         Processes all characters in the text, updating the line and column numbers accordingly.
@@ -129,7 +144,7 @@ class TextStepper:
         while self.step_line():
             pass
 
-    def _get_last_line(self, with_end: bool) -> str:
+    def get_last_line(self, with_end: bool) -> str:
         """
         Returns the last line processed, optionally including the newline character(s) at the end
         """
@@ -147,7 +162,7 @@ class TextStepper:
         lines = []
         while self.step_line():
             if self.is_newline:
-                lines.append(self._get_last_line(with_end=with_ends))
+                lines.append(self.get_last_line(with_end=with_ends))
 
         # add the last line (which was not followed by a newline), even if empty
         last_line = self._chars[self.line_start_idx :]

test/resources/repos/python/test_repo/test_repo/variables.py

@@ -20,7 +20,6 @@ reassignable_module_var = 20  # Reassigned
 typed_module_var: int = 42
 
 
-# Regular class with class and instance variables
 class VariableContainer:
     """Class that contains various variables."""
 
@@ -59,7 +58,6 @@ class VariableContainer:
         return result, other_result
 
 
-# Dataclass with variables
 @dataclass
 class VariableDataclass:
     """Dataclass that contains various fields."""
@@ -75,7 +73,6 @@ class VariableDataclass:
     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"

test/serena/__snapshots__/test_symbol_editing.ambr

@@ -23,11 +23,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
-  
-  
-  
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -43,7 +38,6 @@
       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"
@@ -69,8 +63,6 @@
   '''
   import { ConsoleGreeter, Greeter } from "./formatters";
   
-  
-  
   export function helperFunction() {
       const demo = new DemoClass(42);
       demo.printValue();
@@ -334,7 +326,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -373,7 +364,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -389,7 +379,6 @@
       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"
@@ -438,7 +427,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -477,7 +465,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -493,7 +480,6 @@
       status: str = "pending"
   
   
-  # Function that uses the module variables
   def new_inserted_function():
       print("This is a new function inserted before another.")
   
@@ -1138,7 +1124,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -1177,7 +1162,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -1197,7 +1181,6 @@
       pass
   
   
-  # Function that uses the module variables
   def use_module_variables():
       """Function that uses module-level variables."""
       result = module_var + " used in function"
@@ -1243,7 +1226,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -1282,7 +1264,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   class NewInsertedClass:
       pass
   
@@ -1302,7 +1283,6 @@
       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"
@@ -1531,7 +1511,6 @@
   renamed_typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -1570,7 +1549,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -1586,7 +1564,6 @@
       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"
@@ -1632,7 +1609,6 @@
   typed_module_var: int = 42
   
   
-  # Regular class with class and instance variables
   class VariableContainer:
       """Class that contains various variables."""
   
@@ -1671,7 +1647,6 @@
           return result, other_result
   
   
-  # Dataclass with variables
   @dataclass
   class VariableDataclass:
       """Dataclass that contains various fields."""
@@ -1687,7 +1662,6 @@
       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"

test/serena/test_symbol_editing.py

@@ -216,7 +216,7 @@ class EditingTest(ABC):
         with open(file_path, encoding="utf-8") as f:
             return f.read()
 
-    def run_test(self, content_after_ground_truth: SnapshotAssertion) -> None:
+    def run_test(self, content_after_ground_truth: SnapshotAssertion | None) -> None:
         with self._setup() as symbol_retriever:
             content_before = self._read_file(self.rel_path)
             code_editor = LanguageServerCodeEditor(symbol_retriever)
@@ -229,9 +229,10 @@ class EditingTest(ABC):
     def _apply_edit(self, code_editor: CodeEditor) -> None:
         pass
 
-    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion) -> None:
+    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion | None) -> None:
         assert code_diff.has_changes, f"Sanity check failed: No changes detected in {code_diff.relative_path}"
-        assert code_diff.modified_content == snapshot
+        if snapshot is not None:
+            assert code_diff.modified_content == snapshot
 
 
 # Python test file path
@@ -483,7 +484,7 @@ class RenameSymbolTest(EditingTest):
         code_editor.rename_symbol(self.symbol_name, self.rel_path, self.new_name)
 
     @overrides
-    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion) -> None:
+    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion | None) -> None:
         # sanity check (e.g., for newly generated snapshots) that the new name is actually in the modified content
         assert self.new_name in code_diff.modified_content, f"New name '{self.new_name}' not found in modified content."
         return super()._test_diff(code_diff, snapshot)
@@ -500,6 +501,49 @@ def test_rename_symbol(snapshot: SnapshotAssertion):
     test_case.run_test(content_after_ground_truth=snapshot)
 
 
+class InsertAndDeleteSymbolTest(EditingTest):
+    """
+    Inserts a symbol after another symbol and then deletes the inserted symbol, which should result
+    in no change to the file content.
+    """
+
+    def __init__(self, language: Language, rel_path: str, rel_symbol: str, deleted_symbol: str, insertion: str):
+        """
+        :param language: specifies the language server to use
+        :param rel_path: relative path of the file
+        :param rel_symbol: symbol after which the insertion is made
+        :param deleted_symbol: the symbol to be deleted after the insertion
+        :param insertion: text which inserts the symbol `deleted_symbol`
+        """
+        super().__init__(language, rel_path)
+        self.rel_symbol = rel_symbol
+        self.deleted_symbol = deleted_symbol
+        self.rel_path = rel_path
+        self.insertion = insertion
+
+    def _apply_edit(self, code_editor: CodeEditor) -> None:
+        code_editor.insert_after_symbol(self.rel_symbol, self.rel_path, self.insertion)
+        code_editor.delete_symbol(self.deleted_symbol, self.rel_path)
+
+    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion | None) -> None:
+        assert not code_diff.has_changes
+
+
+def test_insert_and_delete_no_change():
+    """
+    Tests that inserting and immediately deleting a symbol results in no change to the file content.
+    """
+    insertion = "    def inserted_function():\n        pass"
+    test_case = InsertAndDeleteSymbolTest(
+        Language.PYTHON,
+        PYTHON_TEST_REL_FILE_PATH,
+        "VariableContainer/modify_instance_var",
+        "VariableContainer/inserted_function",
+        insertion,
+    )
+    test_case.run_test(None)
+
+
 # ===== VUE WRITE OPERATIONS TESTS =====
 
 VUE_TEST_FILE = os.path.join("src", "components", "CalculatorButton.vue")

test/solidlsp/python/test_symbol_retrieval.py

@@ -11,6 +11,7 @@ import os
 import pytest
 
 from serena.symbol import LanguageServerSymbol
+from serena.util.text_utils import find_text_coordinates
 from solidlsp import SolidLanguageServer
 from solidlsp.ls_types import SymbolKind
 from test.solidlsp.conftest import PYTHON_BACKEND_LANGUAGES
@@ -40,14 +41,15 @@ class TestLanguageServerSymbols:
     def test_references_to_variables(self, language_server: SolidLanguageServer) -> None:
         """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 = [ref.symbol for ref in language_server.request_referencing_symbols(file_path, 74, 4)]
+
+        # find references to field `status`
+        with language_server.open_file(file_path, open_in_ls=False) as f:
+            file_content = f.contents
+        coords = find_text_coordinates(file_content, r"(status): str")
+        ref_symbols = [ref.symbol for ref in language_server.request_referencing_symbols(file_path, coords.line, coords.col)]
 
         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"]]
         ref_names = [ref["name"] for ref in ref_symbols]
-        assert 87 in ref_lines
-        assert 95 in ref_lines
         assert "dataclass_instance" in ref_names
         assert "second_dataclass" in ref_names
 

test/solidlsp/test_text_utils.py

@@ -1,6 +1,6 @@
 import pytest
 
-from solidlsp.ls_utils import InvalidTextLocationError, TextUtils
+from solidlsp.ls_utils import InvalidTextLocationError, TextStepper, TextUtils
 
 
 class TestTextUtils:
@@ -35,6 +35,13 @@ class TestTextUtils:
         assert TextUtils.get_index_from_line_col(self.TEXT, 1, 1) == 3 + 1 + 1
         assert TextUtils.get_index_from_line_col(self.TEXT, 2, 1) == 3 + 1 + 3 + 2 + 1
 
+    def test_step_to(self):
+        stepper = TextStepper(self.TEXT)
+        stepper.step_to(2, 1)
+        assert stepper.line == 2
+        assert stepper.col == 1
+        assert stepper.idx == 3 + 1 + 3 + 2 + 1
+
     def test_insert_text_at_index(self):
         insertion = "XXX"
         new_text, l, c = TextUtils.insert_text_at_position(self.TEXT, 0, 1, insertion)