Fix: Compensate for incorrect body end coordinates in Godot LS (#1974)

Godot's GDScript parser can report a symbol's end column one column past the
line-end convention every other language server follows (closing a node's range from
the next lookahead token instead of the last consumed one, when that lookahead is a
synthesized newline); `replace_symbol_body` on the last function in a file silently
consumed the separating blank line as a result. `GodotLanguageServer` now corrects this
specific, measured overshoot when building its high-level document symbols (#1974)

---------

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

CHANGELOG.md

@@ -65,6 +65,12 @@ Status of the `main` branch. Changes prior to the next official version change w
     Serena's own tools to close the gap (#1852)
 
 * Language Servers:
+  - Fix: Godot's GDScript parser can report a symbol's end column one column past the
+    line-end convention every other language server follows (closing a node's range from
+    the next lookahead token instead of the last consumed one, when that lookahead is a
+    synthesized newline); `replace_symbol_body` on the last function in a file silently
+    consumed the separating blank line as a result. `GodotLanguageServer` now corrects this
+    specific, measured overshoot when building its high-level document symbols (#1974)
   - Fix: High-level document symbol cache was not invalidated when the LS-specific low-level result 
     version changed
   - Fix: TypeScript and VTS now disable automatic type acquisition as intended, while VTS

src/solidlsp/language_servers/godot_language_server.py

@@ -10,8 +10,9 @@ The editor must be open with its built-in language server enabled (default).
 import logging
 import os
 from collections.abc import Callable
+from typing import Any
 
-from solidlsp.ls import SolidLanguageServer
+from solidlsp.ls import DocumentSymbols, LSPFileBuffer, SolidLanguageServer
 from solidlsp.ls_config import LanguageServerConfig
 from solidlsp.ls_process import LanguageServerInterface, TCPConnectionInfo, TCPLanguageServer
 from solidlsp.lsp_protocol_handler.server import StringDict
@@ -38,6 +39,10 @@ class GodotLanguageServer(SolidLanguageServer):
         - ``request_timeout`` (float): seconds to wait for an LSP response (default: 30.0).
     """
 
+    # Bump whenever _fix_range_end/_fix_symbol_ranges below changes, so a stale cached
+    # high-level result (from before this fix existed) is not served back to callers.
+    _DOCUMENT_SYMBOLS_CACHE_VERSION = 1
+
     def __init__(self, config: LanguageServerConfig, repository_root_path: str, solidlsp_settings: SolidLSPSettings) -> None:
         self._godot_version = self._detect_godot_version(repository_root_path)
         if self._godot_version is not None:
@@ -137,3 +142,66 @@ class GodotLanguageServer(SolidLanguageServer):
         self.server.send.initialize(initialize_params)
         self.server.notify.initialized({})
         log.info("Godot LSP initialized")
+
+    def _build_document_symbols_from_raw_symbols(self, relative_file_path: str, file_buffer: LSPFileBuffer) -> DocumentSymbols:
+        """Override to correct a Godot GDScript parser off-by-one in reported end columns.
+
+        See :meth:`_fix_range_end` for the mechanism (oraios/serena#1974). Applied here, on the
+        converted high-level symbols, rather than in ``_request_raw_document_symbols`` or in the
+        generic ``TextUtils.get_index_from_line_col`` (used by every language server): the
+        overshoot is specific to Godot's own parser, not a property of LSP position math in
+        general, and post-processing at this level only needs to invalidate the high-level
+        symbol cache, not the (expensive to rebuild) raw one the language server itself answers.
+
+        TODO: gate this behind a Godot version check once the upstream parser bug is fixed
+        (tracked at https://github.com/godotengine/godot/issues, not yet filed there).
+        """
+        document_symbols = super()._build_document_symbols_from_raw_symbols(relative_file_path, file_buffer)
+        lines = file_buffer.split_lines()
+        for root_symbol in document_symbols.root_symbols:
+            self._fix_symbol_ranges(root_symbol, lines)
+        return document_symbols
+
+    def _document_symbols_cache_fingerprint(self) -> int:
+        return self._DOCUMENT_SYMBOLS_CACHE_VERSION
+
+    @staticmethod
+    def _fix_range_end(rng: Any, lines: list[str]) -> None:
+        """Correct a Godot GDScript parser off-by-one in an LSP ``Range``'s end position, in place.
+
+        Godot's ``gdscript_parser.cpp`` closes a node's range using the *next* lookahead token
+        instead of the *last consumed* one. When that lookahead is a synthesized NEWLINE token,
+        ``gdscript_tokenizer.cpp``'s ``newline()`` sets the token's own ``end_column`` to the
+        column reached *after* consuming the newline character, one past where a real content
+        token would end. Stacked on top of the usual one-past-the-end range convention, a symbol
+        whose body ends at that line reports an end column two past its last character instead of
+        one past it (oraios/serena#1974).
+
+        Only that exact, measured overshoot is corrected; a larger one is not this bug and is left
+        alone rather than guessed at.
+        """
+        end = rng.get("end")
+        if end is None:
+            return
+        end_line, end_char = end.get("line"), end.get("character")
+        if end_line is None or end_char is None or not (0 <= end_line < len(lines)):
+            return
+        # The correct one-past-the-end column for this line is len(lines[end_line]).
+        correct_end_char = len(lines[end_line])
+        if end_char == correct_end_char + 1:
+            end["character"] = correct_end_char
+
+    @staticmethod
+    def _fix_symbol_ranges(symbol: Any, lines: list[str]) -> None:
+        """Recursively apply :meth:`_fix_range_end` to a (raw or unified) symbol and its children."""
+        location = symbol.get("location")
+        if location is not None:
+            GodotLanguageServer._fix_range_end(location.get("range", {}), lines)
+        symbol_range = symbol.get("range")
+        if symbol_range is not None:
+            GodotLanguageServer._fix_range_end(symbol_range, lines)
+        selection_range = symbol.get("selectionRange")
+        if selection_range is not None:
+            GodotLanguageServer._fix_range_end(selection_range, lines)
+        for child in symbol.get("children") or []:
+            GodotLanguageServer._fix_symbol_ranges(child, lines)

test/solidlsp/util/test_godot_symbol_fix.py

@@ -0,0 +1,80 @@
+"""Unit tests for GodotLanguageServer's document-symbol range correction.
+
+See oraios/serena#1974: Godot's GDScript parser can report a symbol's end column one
+column past the line-end convention every other language server follows, which silently
+rolls over into the following line's content (or, at the last line of a body, into the
+separating blank line) when the position is later turned into a text index.
+"""
+
+from __future__ import annotations
+
+from solidlsp.language_servers.godot_language_server import GodotLanguageServer
+
+_fix_range_end = GodotLanguageServer._fix_range_end
+_fix_symbol_ranges = GodotLanguageServer._fix_symbol_ranges
+
+
+def _range(end_line: int, end_char: int, start_line: int = 0, start_char: int = 0) -> dict:
+    return {"start": {"line": start_line, "character": start_char}, "end": {"line": end_line, "character": end_char}}
+
+
+# The exact fixture from the reported issue: a 13-char line whose reported end column is 14.
+_ISSUE_1952_LINES = ["extends Node", "", "func first():", '\tprint("old")', "", "func second():", '\tprint("keep")']
+
+
+def test_fix_range_end_corrects_the_measured_off_by_one() -> None:
+    rng = _range(end_line=3, end_char=14)
+    _fix_range_end(rng, _ISSUE_1952_LINES)
+    assert rng["end"]["character"] == 13
+
+
+def test_fix_range_end_leaves_a_correct_one_past_end_column_alone() -> None:
+    rng = _range(end_line=3, end_char=13)
+    _fix_range_end(rng, _ISSUE_1952_LINES)
+    assert rng["end"]["character"] == 13
+
+
+def test_fix_range_end_does_not_guess_at_a_larger_overshoot() -> None:
+    # Two past the line's end is not the measured Godot mechanism; leave it as reported
+    # rather than assume the same +1 correction applies.
+    rng = _range(end_line=3, end_char=15)
+    _fix_range_end(rng, _ISSUE_1952_LINES)
+    assert rng["end"]["character"] == 15
+
+
+def test_fix_range_end_ignores_an_out_of_range_line() -> None:
+    rng = _range(end_line=99, end_char=5)
+    _fix_range_end(rng, _ISSUE_1952_LINES)
+    assert rng["end"]["character"] == 5
+
+
+def test_fix_range_end_tolerates_a_missing_end() -> None:
+    _fix_range_end({"start": {"line": 0, "character": 0}}, _ISSUE_1952_LINES)  # must not raise
+
+
+def test_fix_symbol_ranges_corrects_range_selection_range_and_location_recursively() -> None:
+    # line 2, "func first():", is 13 chars: end_char=14 is the off-by-one, end_char=5 is not.
+    child = {
+        "name": "inner",
+        "range": _range(end_line=3, end_char=14),
+        "selectionRange": _range(end_line=2, end_char=14, start_line=2),
+    }
+    root = {
+        "name": "first",
+        "range": _range(end_line=3, end_char=14),
+        "selectionRange": _range(end_line=2, end_char=5, start_line=2),
+        "children": [child],
+    }
+    _fix_symbol_ranges(root, _ISSUE_1952_LINES)
+
+    assert root["range"]["end"]["character"] == 13
+    assert root["selectionRange"]["end"]["character"] == 5  # unaffected, no overshoot
+    assert child["range"]["end"]["character"] == 13
+    assert child["selectionRange"]["end"]["character"] == 13  # line 2 is 13 chars, 14 was the overshoot
+
+
+def test_fix_symbol_ranges_corrects_symbol_information_style_location() -> None:
+    # SymbolInformation (the flat, non-hierarchical shape) nests its range under "location".
+    symbol = {"name": "first", "location": {"uri": "file:///script.gd", "range": _range(end_line=3, end_char=14)}}
+    _fix_symbol_ranges(symbol, _ISSUE_1952_LINES)
+    assert symbol["location"]["range"]["end"]["character"] == 13