SolidLSP: correct the position returned by insert_text_at_position (#1842)

insert_text_at_position returns the position of the end of the inserted text.
That position was computed by _get_updated_position_from_line_and_column_and_edit,
which steps the inserted text in isolation and adds the resulting line/column
delta to the start position.

An insertion is not independent of its surroundings. Under the LSP newline
definition adopted in 9208a3ae, a "\n" inserted directly after an existing "\r"
merges with it into a single "\r\n" sequence, so the text gains no line while the
isolated arithmetic still counts one:

    insert_text_at_position("\r", 1, 0, "\n")    -> ("\r\n", 2, 0); actual (1, 0)
    insert_text_at_position("x\ry", 1, 0, "\n")  -> ("x\r\ny", 2, 0); actual (1, 0)
    insert_text_at_position("a\r", 2, 0, "X")    -> ("a\r\nX", 2, 1); actual (1, 1)

The third case reaches the same fault through the one-line-past-EOF branch, which
prepends a "\n" that then merges with the trailing "\r". The converse interaction
was wrong too: inserting between the "\r" and the "\n" of a "\r\n" splits it, and
the isolated arithmetic reported (0, 2) -- a column on a line the split had just
emptied. In every case the resulting text was correct; only the position was wrong.

9208a3ae centralised line/column computation on TextStepper and reimplemented the
TextUtils methods on it. This helper was rebased onto TextStepper in that commit
but kept its original shape: it steps one string, and the string it steps is the
insertion rather than the result. Once a newline can be formed by two adjacent
characters, no computation over the inserted text alone can be correct.

The position is now determined from the resulting text via get_line_col_from_index,
the same function that resolves positions everywhere else, which removes the last
computation in TextUtils that did not go through the centralised path. The helper
had no other callers and is deleted, along with the line/col adjustment in the
past-EOF branch that existed only to feed it.

The column is clamped to the end of the text before both the splice and the
derivation. This is not a new tolerance: the splice already clamped implicitly
(text[:i] with i > len(text) yields the whole text), so an out-of-range column has
always appended successfully. Making the clamp explicit keeps the position derived
from the same index the splice used; without it, deriving from the result would
newly raise for input that previously worked.

Columns are offsets into the Python string (code points), which the docstring now
states. LSP columns are UTF-16 code units and nothing converts between the two at
the boundary -- a real defect, but a pre-existing and separate one: the previous
arithmetic did not preserve UTF-16 units either, since it added a code-point length
to a code-unit column and so produced a value in neither unit.

    "\U0001f600" + "\U0001f600" at column 2
        old (0, 3); UTF-16 (0, 4); code points (0, 2)

Seven regression tests cover the boundary, mid-document and past-EOF merges, the
newline split, an out-of-range column, a column past a non-BMP character, and an
unaffected "\n"-terminated control. The six that pin changed behaviour were each
verified to fail before the change; the control passes either way.

CHANGELOG.md

@@ -6,6 +6,9 @@ Status of the `main` branch. Changes prior to the next official version change w
   - Fix: MCP `initialize` now reports Serena's version instead of the installed mcp SDK version (#1889)
   - Fix: Parallel agents auto-registering projects could overwrite each other's changes to the global
     project list in `serena_config.yml`
+  - Fix: `TextUtils.insert_text_at_position` returned a wrong position when the inserted text merged
+    with an adjacent character into a single newline sequence (e.g. a `\n` inserted directly after an
+    existing `\r`); the position is now determined from the resulting text
   - Fix: process-tree cleanup signaled descendant language-server processes without waiting for them,
     which could leave grandchildren as zombies; cleanup now waits for the discovered descendants (#1464)
   - Fix: `read_only` restriction in project definition was not applied to base tool set when in single-project context (#1938)

src/solidlsp/ls_utils.py

@@ -236,23 +236,6 @@ class TextUtils:
 
         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]:
-        """
-        :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)
-        """
-        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 += 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]:
         """
@@ -289,14 +272,18 @@ 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 position and returns the
+        Inserts the given text at the given position.
 
         :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 col: the 0-based column number where the text should be inserted; a column pointing
+            beyond the end of the text is clamped to its end
         :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)
+            (position after the inserted text). As everywhere in this class, columns are offsets into
+            the Python string (code points), not UTF-16 code units.
+        :raises InvalidTextLocationError: if the given line does not exist in the text (other than the
+            position one line past the last line, which is handled as an append)
         """
         try:
             change_index = TextUtils.get_index_from_line_col(text, line, col)
@@ -306,16 +293,20 @@ class TextUtils:
             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 and adjusting insertion position
-                # to the actual end coordinates of the text
+                # insert at end, adding the missing newline
                 change_index = len(text)
                 text_to_be_inserted = "\n" + text_to_be_inserted
-                line = text_stepper.line
-                col = text_stepper.col
             else:
                 raise
+        # apply the insertion, clamping a column that points beyond the end of the text
+        # (the slicing clamps implicitly; making it explicit lets the position use the same index)
+        change_index = min(change_index, len(text))
         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)
+
+        # determine the end position from the resulting text, because the insertion can merge with
+        # an adjacent character into a single newline sequence (a "\n" inserted directly after an
+        # existing "\r" forms one "\r\n"), which stepping the inserted text alone cannot observe
+        new_l, new_c = TextUtils.get_line_col_from_index(new_text, change_index + len(text_to_be_inserted))
         return new_text, new_l, new_c
 
     @staticmethod

test/solidlsp/test_text_utils.py

@@ -83,3 +83,64 @@ class TestTextUtils:
         with pytest.raises(InvalidTextLocationError):
             # end_line = 5 is well past the one-line-past-EOF position (3) for a 3-line file.
             TextUtils.delete_text_between_positions("a\nb\nc", 0, 0, 5, 0)
+
+    def test_insert_position_accounts_for_newline_merge(self) -> None:
+        r"""A leading "\n" inserted directly after an existing "\r" merges with it into a single
+        "\r\n" newline sequence. The returned position must therefore be derived from the
+        resulting text; computing it from the inserted text alone reports a line too far.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("\r", 1, 0, "\n")
+        assert new_text == "\r\n"
+        assert (line, col) == (1, 0)
+
+    def test_insert_position_after_full_text_accounts_for_newline_merge(self) -> None:
+        r"""Inserting one line past the last line prepends a "\n" to the inserted text; when the
+        text ends in "\r" the two merge into a single "\r\n". The returned position must describe
+        where the inserted text actually ended up.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("a\r", 2, 0, "X")
+        assert new_text == "a\r\nX"
+        assert (line, col) == (1, 1)
+
+    def test_insert_position_after_full_text_without_merge(self) -> None:
+        r"""The same one-line-past-EOF insertion into "\n"-terminated text is unaffected."""
+        new_text, line, col = TextUtils.insert_text_at_position("a\n", 2, 0, "X")
+        assert new_text == "a\n\nX"
+        assert (line, col) == (2, 1)
+
+    def test_insert_position_accounts_for_newline_merge_mid_document(self) -> None:
+        r"""The merge is not specific to the end of the buffer: inserting "\n" at the start of a
+        line that follows a bare "\r" merges with that "\r" in the middle of the document too.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("x\ry", 1, 0, "\n")
+        assert new_text == "x\r\ny"
+        assert (line, col) == (1, 0)
+
+    def test_insert_position_inside_newline_sequence_splits_it(self) -> None:
+        r"""Inserting between the "\r" and the "\n" of a "\r\n" splits it into two newline
+        sequences, adding a line. The position must again come from the resulting text.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("\r\n", 0, 1, "Z")
+        assert new_text == "\rZ\n"
+        assert (line, col) == (1, 1)
+
+    def test_insert_at_column_beyond_text_appends(self) -> None:
+        """A column past the end of the text appends and reports the true end position.
+
+        Callers may pass a column that exceeds the text length -- notably an LSP column, which
+        counts UTF-16 code units, for a line containing non-BMP characters. The insertion clamps
+        to the end of the text, so the reported position must too, rather than addressing an
+        index that does not exist in the result.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("abc", 0, 10, "X")
+        assert new_text == "abcX"
+        assert (line, col) == (0, 4)
+
+    def test_insert_at_column_beyond_text_containing_non_bmp_character(self) -> None:
+        """A non-BMP character is one code point but two UTF-16 code units, which is one way a
+        caller's column comes to exceed the text length. The column is clamped, and the returned
+        position is expressed in code points like every other position in TextUtils.
+        """
+        new_text, line, col = TextUtils.insert_text_at_position("\U0001f600", 0, 2, "X")
+        assert new_text == "\U0001f600X"
+        assert (line, col) == (0, 2)