diff --git a/CHANGELOG.md b/CHANGELOG.md index e38c63f1..110e9d3d 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/solidlsp/ls_utils.py b/src/solidlsp/ls_utils.py index 11a993d7..0996c9f4 100644 --- a/src/solidlsp/ls_utils.py +++ b/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 diff --git a/test/solidlsp/test_text_utils.py b/test/solidlsp/test_text_utils.py index f6122148..abbef5b1 100644 --- a/test/solidlsp/test_text_utils.py +++ b/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)