diff --git a/CHANGELOG.md b/CHANGELOG.md index 105c4fff..76021f64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ Status of the `main` branch. Changes prior to the next official version change w and the request used to map diagnostics onto owning symbols just threw. `AnsibleLanguageServer` now overrides that request to return `None` directly, so diagnostics fall back to being grouped under the file-level path as already documented #1758 + - Fix: F#'s `module ` declarations reported a `selectionRange` pointing at the `module` + keyword instead of at ``, so looking up hover/references from a module symbol's position + returned the keyword's own docs instead of the module's #925 * JetBrains: - `jet_brains_find_symbol`: Disallow wildcard-only search, delegating to overview tool if request is for file diff --git a/src/solidlsp/language_servers/fsharp_language_server.py b/src/solidlsp/language_servers/fsharp_language_server.py index 0094ada3..660c4335 100644 --- a/src/solidlsp/language_servers/fsharp_language_server.py +++ b/src/solidlsp/language_servers/fsharp_language_server.py @@ -4,6 +4,7 @@ Provides F# specific instantiation of the LanguageServer class. import logging import os +import re import shutil import subprocess import threading @@ -12,8 +13,9 @@ from pathlib import Path from overrides import override from serena.util.dotnet import DotNETUtil +from solidlsp import ls_types from solidlsp.language_servers.common import RuntimeDependency, RuntimeDependencyCollection -from solidlsp.ls import SolidLanguageServer +from solidlsp.ls import DocumentSymbols, LSPFileBuffer, SolidLanguageServer from solidlsp.ls_config import LanguageServerConfig, LanguageServerId from solidlsp.ls_exceptions import SolidLSPException from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo @@ -29,6 +31,10 @@ INITIAL_FSAUTOCOMPLETE_VERSION = "0.83.0" DEFAULT_FSAUTOCOMPLETE_VERSION = "0.83.0" FSAUTOCOMPLETE_VERSION = DEFAULT_FSAUTOCOMPLETE_VERSION +# Matches an F# module declaration line ("module Foo", "module Foo =", "module rec Foo =", +# dotted names), used to recover the module name's real column. See _fix_module_selection_range. +_MODULE_DECLARATION_PATTERN = re.compile(r"^\s*module\s+(?:rec\s+)?([A-Za-z_]\w*(?:\.[A-Za-z_]\w*)*)") + class FSharpLanguageServer(SolidLanguageServer): """ @@ -68,6 +74,60 @@ class FSharpLanguageServer(SolidLanguageServer): ".ionide", ] + def _fix_module_selection_range( + self, symbol: ls_types.UnifiedSymbolInformation, file_content: str + ) -> ls_types.UnifiedSymbolInformation: + """ + FsAutoComplete's selectionRange for a `module ` declaration points at the `module` + keyword itself instead of at `` (confirmed by dsyme and MischaPanch in #925), so any + caller that hovers or searches references from selectionRange.start (e.g. + serena.symbol.LanguageServerSymbol.line/.column) gets the generic keyword's info instead of + the module's own. Other symbol kinds (functions, types, `open`) already select the + identifier correctly, so only Module symbols are touched here. + """ + if symbol.get("kind") != ls_types.SymbolKind.Module or "selectionRange" not in symbol: + return symbol + + sel_range = symbol["selectionRange"] + start_line = sel_range["start"]["line"] + lines = file_content.split("\n") + if start_line >= len(lines): + return symbol + + match = _MODULE_DECLARATION_PATTERN.match(lines[start_line]) + if not match: + return symbol + + name_start, name_end = match.span(1) + if sel_range["start"]["character"] == name_start: + return symbol # already correct (e.g. a future FsAutoComplete release) + + corrected_symbol = symbol.copy() + corrected_symbol["selectionRange"] = { + "start": {"line": start_line, "character": name_start}, + "end": {"line": start_line, "character": name_end}, + } + return corrected_symbol + + @override + def request_document_symbols(self, relative_file_path: str, file_buffer: LSPFileBuffer | None = None) -> DocumentSymbols: + # Override to fix FsAutoComplete's incorrect selectionRange for module declarations (#925): + # it points at the `module` keyword instead of the module name, so hover-by-selectionRange + # returns the keyword's docs instead of the module's own. + document_symbols = super().request_document_symbols(relative_file_path, file_buffer=file_buffer) + + with self.open_file(relative_file_path) as file_data: + file_content = file_data.contents + + def fix_symbol_and_children(symbol: ls_types.UnifiedSymbolInformation) -> ls_types.UnifiedSymbolInformation: + fixed = self._fix_module_selection_range(symbol, file_content) + if fixed.get("children"): + fixed["children"] = [fix_symbol_and_children(child) for child in fixed["children"]] + return fixed + + fixed_root_symbols = [fix_symbol_and_children(sym) for sym in document_symbols.root_symbols] + return DocumentSymbols(fixed_root_symbols) + @classmethod def _setup_runtime_dependencies(cls, config: LanguageServerConfig, solidlsp_settings: SolidLSPSettings) -> str: """ diff --git a/test/solidlsp/fsharp/test_fsharp_module_selection_range.py b/test/solidlsp/fsharp/test_fsharp_module_selection_range.py new file mode 100644 index 00000000..a3b22e85 --- /dev/null +++ b/test/solidlsp/fsharp/test_fsharp_module_selection_range.py @@ -0,0 +1,132 @@ +""" +Regression test for #925: FsAutoComplete's selectionRange for a `module ` declaration +points at the `module` keyword rather than at ``, so hovering a module reports the +generic keyword's docs instead of the module's own (confirmed by dsyme and MischaPanch in the +issue thread). This pins FSharpLanguageServer._fix_module_selection_range and the +request_document_symbols override directly, without spinning up FsAutoComplete: F# language +server tests are unconditionally disabled (test/conftest.py, category 1, "F# language server is +currently unreliable"), so a live-LS test can never run here or on CI. +""" + +from contextlib import contextmanager + +from solidlsp import ls_types +from solidlsp.language_servers.fsharp_language_server import FSharpLanguageServer +from solidlsp.ls import DocumentSymbols, SolidLanguageServer + + +def _bare_fsharp_server() -> FSharpLanguageServer: + """Instance without running __init__ (no dotnet tool install, no process); same technique + as test_typescript_timeout_policy.py's _bare_ts_server / test_rename_didopen.py. + """ + return object.__new__(FSharpLanguageServer) + + +def _module_symbol(name: str, line: int, start_char: int, end_char: int) -> ls_types.UnifiedSymbolInformation: + return { + "name": name, + "kind": ls_types.SymbolKind.Module, + "selectionRange": { + "start": {"line": line, "character": start_char}, + "end": {"line": line, "character": end_char}, + }, + "children": [], + } + + +class TestFixModuleSelectionRange: + def test_top_level_module_declaration(self) -> None: + # "module Calculator" -- FsAutoComplete reports selectionRange over "module" (0-6) + server = _bare_fsharp_server() + symbol = _module_symbol("Calculator", line=0, start_char=0, end_char=6) + file_content = "module Calculator\n\nlet add a b = a + b\n" + + fixed = server._fix_module_selection_range(symbol, file_content) + + assert fixed["selectionRange"]["start"] == {"line": 0, "character": 7} + assert fixed["selectionRange"]["end"] == {"line": 0, "character": 17} + assert file_content.splitlines()[0][7:17] == "Calculator" + + def test_nested_module_declaration_with_equals(self) -> None: + # "module PersonModule =" -- same keyword-position bug, trailing '=' + server = _bare_fsharp_server() + symbol = _module_symbol("PersonModule", line=2, start_char=0, end_char=6) + file_content = "namespace Models\n\nmodule PersonModule =\n let x = 1\n" + + fixed = server._fix_module_selection_range(symbol, file_content) + + assert fixed["selectionRange"]["start"] == {"line": 2, "character": 7} + assert fixed["selectionRange"]["end"] == {"line": 2, "character": 19} + + def test_recursive_module_declaration(self) -> None: + # "module rec Foo" -- the "rec" keyword must not be mistaken for the module name + server = _bare_fsharp_server() + symbol = _module_symbol("Foo", line=0, start_char=0, end_char=6) + file_content = "module rec Foo\n" + + fixed = server._fix_module_selection_range(symbol, file_content) + + assert fixed["selectionRange"]["start"] == {"line": 0, "character": 11} + assert fixed["selectionRange"]["end"] == {"line": 0, "character": 14} + + def test_already_correct_selection_range_is_left_alone(self) -> None: + # A future FsAutoComplete release that already points at the identifier must not be + # rewritten (guards against masking an upstream fix instead of a no-op). + server = _bare_fsharp_server() + symbol = _module_symbol("Calculator", line=0, start_char=7, end_char=17) + file_content = "module Calculator\n" + + fixed = server._fix_module_selection_range(symbol, file_content) + + assert fixed is symbol + + def test_non_module_symbol_is_untouched(self) -> None: + # Functions/types already select the identifier correctly per MischaPanch (#925); only + # Module symbols should ever be corrected. + server = _bare_fsharp_server() + symbol: ls_types.UnifiedSymbolInformation = { + "name": "add", + "kind": ls_types.SymbolKind.Function, + "selectionRange": { + "start": {"line": 2, "character": 4}, + "end": {"line": 2, "character": 7}, + }, + "children": [], + } + file_content = "module Calculator\n\nlet add a b = a + b\n" + + fixed = server._fix_module_selection_range(symbol, file_content) + + assert fixed is symbol + + +class TestRequestDocumentSymbolsWiring: + def test_fixes_every_symbol_in_the_tree_not_just_roots(self, monkeypatch) -> None: + """End-to-end wiring test: the override must fix nested symbols too, mirroring the + already-merged fortran_language_server.py recursive-fix pattern this follows. + """ + outer = _module_symbol("Outer", line=0, start_char=0, end_char=6) + inner = _module_symbol("Inner", line=1, start_char=4, end_char=10) + outer["children"] = [inner] + + file_content = "module Outer\n module Inner =\n let x = 1\n" + + def fake_super_request_document_symbols(self, relative_file_path, file_buffer=None): + return DocumentSymbols([outer]) + + monkeypatch.setattr(SolidLanguageServer, "request_document_symbols", fake_super_request_document_symbols) + + server = _bare_fsharp_server() + + @contextmanager + def fake_open_file(relative_file_path): + yield type("FakeFileData", (), {"contents": file_content})() + + monkeypatch.setattr(server, "open_file", fake_open_file) + + result = server.request_document_symbols("Test.fs") + + root = result.root_symbols[0] + assert root["selectionRange"]["start"] == {"line": 0, "character": 7} + child = root["children"][0] + assert child["selectionRange"]["start"] == {"line": 1, "character": 11}