From 0b2e22bbca06ad4d05cbe22c02c599b552df6c4b Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 17 Sep 2026 02:37:04 -0600 Subject: [PATCH] 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 --- CHANGELOG.md | 6 ++ .../language_servers/godot_language_server.py | 70 +++++++++++++++- test/solidlsp/util/test_godot_symbol_fix.py | 80 +++++++++++++++++++ 3 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 test/solidlsp/util/test_godot_symbol_fix.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a202ce84..ba459db3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/solidlsp/language_servers/godot_language_server.py b/src/solidlsp/language_servers/godot_language_server.py index 52995988..6ade0c88 100644 --- a/src/solidlsp/language_servers/godot_language_server.py +++ b/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) diff --git a/test/solidlsp/util/test_godot_symbol_fix.py b/test/solidlsp/util/test_godot_symbol_fix.py new file mode 100644 index 00000000..0b3559a2 --- /dev/null +++ b/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