Merge pull request #1691 from oraios/line-counting

Apply consistent, LSP-compliant line counting

CHANGELOG.md

@@ -35,6 +35,9 @@ Status of the `main` branch. Changes prior to the next official version change w
   - During project creation, language composition percentages are now computed relative to the total number 
     of recognised source files instead of all files, i.e. unrecognised files are ignored in the percentage 
     computation.
+  - Fix: Use LSP-compliant line splitting ("\n", "\r\n" and "\r" can define line breaks); previously only "\n" considered
+  - Fix: Apply consistent line splitting across tools, uniformly applying the LSP splitting semantics; 
+    affects `search_text` used by `search_for_pattern` tool (reported in #1684)
 
 * CLI:
   - Fix `--project-from-cwd` hijacking git worktrees nested under a Serena project. `find_project_root`

src/serena/util/text_utils.py

@@ -100,7 +100,7 @@ class MatchedConsecutiveLines:
     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")
+        line_contents = TextUtils.split_lines(file_contents)
         start_lineno = max(0, line - context_lines_before)
         end_lineno = min(len(line_contents) - 1, line + context_lines_after)
         text_lines: list[TextLine] = []
@@ -169,7 +169,7 @@ def search_text(
         raise ValueError("Pass either content or source_file_path")
 
     matches = []
-    lines = content.splitlines()
+    lines = TextUtils.split_lines(content)
     total_lines = len(lines)
 
     # Convert pattern to a compiled regex if it's a string
@@ -185,8 +185,8 @@ def search_text(
         end_pos = match.end()
 
         # Find the line numbers for the start and end positions
-        start_line_num = content[:start_pos].count("\n")
-        end_line_num = content[:end_pos].count("\n")
+        start_line_num = TextUtils.get_line_from_index(content, start_pos)
+        end_line_num = TextUtils.get_line_from_index(content, end_pos)
 
         # Calculate the range of lines to include in the context
         context_start = max(0, start_line_num - context_lines_before)

src/solidlsp/ls_utils.py

@@ -31,6 +31,124 @@ class InvalidTextLocationError(Exception):
     pass
 
 
+class TextStepper:
+    def __init__(self, chars: str):
+        self._chars = chars
+        self._len = len(chars)
+        self.line = 0
+        """
+        the current 0-based line index
+        """
+        self.col = 0
+        """
+        the current 0-based column index
+        """
+        self.idx = 0
+        """
+        the current 0-based index in the full text.
+        The index specifies the next character to be processed, i.e. if this is
+        the length of the text, then the end of the text has been reached.
+        It specifies a location in the same way as a cursor insertion position:
+        cursor at the very beginning (idx=0) means insert before the first character.
+        """
+        self.is_newline = False
+        """
+        whether the last step processed a full line ending in a line break
+        """
+        self.prev_line_start_idx = 0
+        """
+        start index of the last fully processed line (inclusive)    
+        """
+        self.prev_line_end_idx = 0
+        """
+        end of the last fully processed line (exclusive), excluding newline characters
+        """
+        self.line_start_idx = 0
+        """
+        start of the current, not yet completed line (inclusive)
+        """
+
+    def _get_char(self, idx: int) -> str | None:
+        if idx < 0 or idx >= self._len:
+            return None
+        return self._chars[idx]
+
+    def step_line(self) -> bool:
+        """
+        Processes the next line in the text, advancing past the next newline sequence or,
+        if no further newline is present, to the end of the text.
+
+        :return: True if processing was possible, False if the end of the text had already been reached
+            (idx not advanced)
+        """
+        if self.idx >= self._len:
+            return False
+
+        # find the next newline sequence
+        # Note: LSP defines that a newline is given by either "\n", "\r\n", or "\r" on its own
+        # Reference: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#textDocuments
+        # The search for "\r" is bounded by the position of the next "\n".
+        lf_idx = self._chars.find("\n", self.idx)
+        cr_idx = self._chars.find("\r", self.idx, lf_idx if lf_idx != -1 else self._len)
+        if cr_idx != -1:
+            newline_start_idx = cr_idx
+            newline_end_idx = cr_idx + 2 if self._get_char(cr_idx + 1) == "\n" else cr_idx + 1
+        elif lf_idx != -1:
+            newline_start_idx = lf_idx
+            newline_end_idx = lf_idx + 1
+        else:
+            newline_start_idx = None
+            newline_end_idx = None
+
+        # advance past the newline sequence or, in its absence, consume the trailing line
+        if newline_start_idx is not None and newline_end_idx is not None:
+            self.idx = newline_end_idx
+            self.line += 1
+            self.col = 0
+            self.is_newline = True
+            self.prev_line_start_idx = self.line_start_idx
+            self.prev_line_end_idx = newline_start_idx
+            self.line_start_idx = newline_end_idx
+        else:
+            self.idx = self._len
+            self.col = self._len - self.line_start_idx
+            self.is_newline = False
+        return True
+
+    def process_all(self):
+        """
+        Processes all characters in the text, updating the line and column numbers accordingly.
+        """
+        while self.step_line():
+            pass
+
+    def _get_last_line(self, with_end: bool) -> str:
+        """
+        Returns the last line processed, optionally including the newline character(s) at the end
+        """
+        start_idx = self.prev_line_start_idx
+        end_idx = self.prev_line_end_idx if not with_end else self.line_start_idx
+        return self._chars[start_idx:end_idx]
+
+    def process_all_gather_lines(self, with_ends: bool) -> list[str]:
+        """
+        Processes all characters in the text and returns a list of lines
+
+        :param with_ends: whether to include the newline character(s) at the end of each line
+        :return: the list of lines
+        """
+        lines = []
+        while self.step_line():
+            if self.is_newline:
+                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 :]
+        lines.append(last_line)
+
+        return lines
+
+
 class TextUtils:
     """
     Utilities for text operations.
@@ -39,54 +157,84 @@ class TextUtils:
     @staticmethod
     def get_line_col_from_index(text: str, index: int) -> tuple[int, int]:
         """
-        Returns the zero-indexed line and column number of the given index in the given text
+        :param text: the text in which the index is to be located
+        :param index: the 0-based index in the text
+        :return: a tuple (0-based line number, 0-based column number) corresponding to the index in the text
         """
-        l = 0
-        c = 0
-        idx = 0
-        while idx < index:
-            if text[idx] == "\n":
-                l += 1
-                c = 0
-            else:
-                c += 1
-            idx += 1
+        # step over full lines; once the line containing the index has been processed, compute
+        # the position from the difference to the respective line's start index
+        text_stepper = TextStepper(text)
+        while text_stepper.step_line():
+            if text_stepper.idx > index:
+                if text_stepper.is_newline:
+                    # position was stepped over as part of the previous line
+                    if index > text_stepper.prev_line_end_idx:
+                        # edge case: the index points into a multi-character newline sequence ("\r\n");
+                        # map this to the beginning of the following line
+                        return text_stepper.line, 0
+                    return text_stepper.line - 1, index - text_stepper.prev_line_start_idx
+                else:
+                    # position was stepped over as part of the current line (which was not followed by a newline)
+                    return text_stepper.line, index - text_stepper.line_start_idx
 
-        return l, c
+        # handle the case where the end of the text was reached without stepping past the index
+        if index > text_stepper.idx:
+            raise InvalidTextLocationError(f"{index=}")
+        return text_stepper.line, text_stepper.col
+
+    @classmethod
+    def get_line_from_index(cls, text: str, index: int) -> int:
+        """
+        :param text: the text in which the index is to be located
+        :param index: the 0-based index in the text
+        :return: the 0-based line number corresponding to the index in the text
+        """
+        return cls.get_line_col_from_index(text, index)[0]
 
     @staticmethod
     def get_index_from_line_col(text: str, line: int, col: int) -> int:
         """
-        Returns the index of the given zero-indexed line and column number in the given text
+        :param text: the text in which the coordinates are to be located
+        :param line: the 0-based line number
+        :param col: the 0-based column number
+        :return: the corresponding 0-based index in the text
         """
-        idx = 0
-        while line > 0:
-            if idx >= len(text):
-                raise InvalidTextLocationError
-            if text[idx] == "\n":
-                line -= 1
-            idx += 1
-        idx += col
-        return idx
+        # step over full lines until the requested line is reached
+        text_stepper = TextStepper(text)
+        while text_stepper.line < line:
+            if not text_stepper.step_line():
+                raise InvalidTextLocationError("{line=}, {col=}")
+
+        return text_stepper.line_start_idx + col
 
     @staticmethod
     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.
+        :param l: the 0-based line number before the edit
+        :param c: the 0-based column number before the edit
+        :param text_to_be_inserted: the text that was inserted at the given position
+        :return: the updated 0-based line and column numbers after the edit (end of insertion)
         """
-        num_newlines_in_gen_text = text_to_be_inserted.count("\n")
-        if num_newlines_in_gen_text > 0:
-            l += num_newlines_in_gen_text
-            c = len(text_to_be_inserted.split("\n")[-1])
+        text_stepper = TextStepper(text_to_be_inserted)
+        text_stepper.process_all()
+        if text_stepper.line > 0:
+            l += text_stepper.line
+            c = text_stepper.col
         else:
-            c += len(text_to_be_inserted)
-        return (l, c)
+            c += text_stepper.col
+        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.
+
+        :param text: the original text
+        :param start_line: the 0-based line number of the start position
+        :param start_col: the 0-based column number of the start position
+        :param end_line: the 0-based line number of the end position
+        :param end_col: the 0-based column number of the end position
+        :return: a tuple containing 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)
@@ -98,18 +246,29 @@ class TextUtils:
     @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.
+        Inserts the given text at the given position and returns the
+
+        :param text: the original text
+        :param line: the 0-based line number where the text should be inserted
+        :param col: the 0-based column number where the text should be inserted
+        :param text_to_be_inserted: the text to be inserted
+        :return: a tuple containing the modified text, the updated line number, and the updated column number
+            (position after the inserted text)
         """
         try:
             change_index = TextUtils.get_index_from_line_col(text, line, col)
         except InvalidTextLocationError:
-            num_lines_in_text = text.count("\n") + 1
+            text_stepper = TextStepper(text)
+            text_stepper.process_all()
+            num_lines_in_text = text_stepper.line + 1
             max_line = num_lines_in_text - 1
             if line == max_line + 1 and col == 0:  # trying to insert at new line after full text
-                # insert at end, adding missing newline
+                # insert at end, adding missing newline and adjusting insertion position
+                # to the actual end coordinates of the text
                 change_index = len(text)
                 text_to_be_inserted = "\n" + text_to_be_inserted
+                line = text_stepper.line
+                col = text_stepper.col
             else:
                 raise
         new_text = text[:change_index] + text_to_be_inserted + text[change_index:]
@@ -125,14 +284,22 @@ class TextUtils:
         end_idx = TextUtils.get_index_from_line_col(text, end_line, end_col)
         return text[start_idx:end_idx]
 
-    @staticmethod
-    def get_text_in_lines_range(text: str, start_line: int, end_line: int) -> str:
+    @classmethod
+    def get_text_in_lines_range(cls, text: str, start_line: int, end_line: int) -> str:
         """
         Returns the text encompassed by the given start and end lines (inclusive).
         """
-        lines = text.splitlines(keepends=True)
+        lines = cls.split_lines(text, with_ends=True)
         return "".join(lines[start_line : end_line + 1])
 
+    @staticmethod
+    def split_lines(text: str, with_ends: bool = False) -> list[str]:
+        """
+        Splits the given text into lines, optionally including the newline character(s) at the end of each line.
+        """
+        text_stepper = TextStepper(text)
+        return text_stepper.process_all_gather_lines(with_ends=with_ends)
+
 
 class PathUtils:
     """

test/solidlsp/test_text_utils.py

@@ -0,0 +1,50 @@
+from solidlsp.ls_utils import TextUtils
+
+
+class TestTextUtils:
+    LINE = "012"
+    TEXT = LINE + "\n" + LINE + "\r\n" + LINE + "\r" + LINE
+
+    def test_split_lines(self):
+        lines = TextUtils.split_lines(self.TEXT, with_ends=False)
+        assert len(lines) == 4
+        for line in lines:
+            assert line == self.LINE
+
+    def test_split_lines_with_ends(self):
+        lines = TextUtils.split_lines(self.TEXT, with_ends=True)
+        assert len(lines) == 4
+        for i, line in enumerate(lines):
+            assert line[: len(self.LINE)] == self.LINE
+        for i, ending in enumerate(["\n", "\r\n", "\r", ""]):
+            assert lines[i][len(self.LINE) :] == ending
+
+    def test_line_col_from_idx(self):
+        assert TextUtils.get_line_col_from_index(self.LINE, 0) == (0, 0)
+        assert TextUtils.get_line_col_from_index(self.LINE, 1) == (0, 1)
+        assert TextUtils.get_line_col_from_index(self.TEXT, 0) == (0, 0)
+        assert TextUtils.get_line_col_from_index(self.TEXT, 1) == (0, 1)
+        assert TextUtils.get_line_col_from_index(self.TEXT, 3 + 1 + 1) == (1, 1)
+        assert TextUtils.get_line_col_from_index(self.TEXT, 3 + 1 + 3 + 2 + 1) == (2, 1)
+
+    def test_idx_from_line_col(self):
+        assert TextUtils.get_index_from_line_col(self.TEXT, 0, 0) == 0
+        assert TextUtils.get_index_from_line_col(self.TEXT, 0, 1) == 1
+        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_insert_text_at_index(self):
+        insertion = "XXX"
+        new_text, l, c = TextUtils.insert_text_at_position(self.TEXT, 0, 1, insertion)
+        assert (l, c) == (0, 1 + len(insertion))
+        assert new_text.startswith("0XXX12")
+
+    def test_insert_text_in_next_line_beyond_content(self):
+        """
+        Test inserting text at a line index 1 beyond the actual number of lines.
+        This case is specifically handled as an edge case in the implementation.
+        """
+        insertion = "XXX"
+        new_text, l, c = TextUtils.insert_text_at_position(self.TEXT, 4, 0, insertion)
+        assert (l, c) == (4, len(insertion))
+        assert new_text == self.TEXT + "\n" + insertion