Merge pull request #1530 from tphakala/fix/gopls-type-var-const-keyword-duplication

Fix gopls replace_symbol_body corrupting type/var/const declarations

CHANGELOG.md

@@ -22,6 +22,11 @@ Status of the `main` branch. Changes prior to the next official version change w
     and other JDK types reported as "cannot be resolved") for projects whose source/target level
     exceeds the bundled JDK 21 JRE JDT-LS registers by default; configured runtimes extend rather
     than replace that bundled default. #1478
+  - `gopls`: Fix `replace_symbol_body` corrupting single `type`/`var`/`const` declarations by
+    duplicating the leading keyword (e.g. `type Foo` becoming `type type Foo`). gopls reports the
+    symbol range of such declarations starting at the identifier rather than the keyword (unlike
+    `func` declarations); the range is now extended to include the keyword so the body and the
+    replacement range stay consistent.
 
 * Tools:
   - Fix: `search_for_pattern` marked one line too many as matched whenever a match ended with a line

src/solidlsp/language_servers/gopls.py

@@ -2,13 +2,15 @@ import hashlib
 import json
 import logging
 import os
+import re
 import subprocess
 from collections.abc import Hashable
 from typing import Any
 
 from overrides import override
 
-from solidlsp.ls import RawDocumentSymbol, SolidLanguageServer
+from solidlsp import ls_types
+from solidlsp.ls import DocumentSymbols, LSPFileBuffer, RawDocumentSymbol, SolidLanguageServer, SymbolBodyFactory
 from solidlsp.ls_config import LanguageServerConfig
 from solidlsp.lsp_protocol_handler.server import ProcessLaunchInfo
 from solidlsp.settings import SolidLSPSettings
@@ -21,6 +23,12 @@ class Gopls(SolidLanguageServer):
     Provides Go specific instantiation of the LanguageServer class using gopls.
     """
 
+    # matches a line prefix that consists solely of a leading `type`/`var`/`const` declaration
+    # keyword (with optional indentation and the whitespace before the declared identifier).
+    # gopls reports the symbol range of such single declarations starting at the identifier,
+    # i.e. after the keyword, whereas `func` declarations include the `func` keyword.
+    _LEADING_DECL_KEYWORD_RE = re.compile(r"(?P<indent>\s*)(?:type|var|const)\s+")
+
     @classmethod
     def supports_implementation_request(cls) -> bool:
         return True
@@ -162,11 +170,11 @@ class Gopls(SolidLanguageServer):
     @override
     def _document_symbols_cache_fingerprint(self) -> Hashable:
         """
-        Compute a deterministic fingerprint of the Go build context.
-
-        The fingerprint includes gopls_settings and selected env vars that affect symbol discovery.
+        Compute a deterministic fingerprint of method/implementation versions and Gopls settings/the Go build context.
         """
         normalize_symbol_name_version = 1
+        request_document_symbols_impl_version = 2
+
         gopls_settings_raw = self._custom_settings.settings.get("gopls_settings")
 
         gopls_settings: dict | None
@@ -185,23 +193,108 @@ class Gopls(SolidLanguageServer):
 
         # Version processed symbols even when the build context itself is empty.
         if gopls_settings is None and not env_subset:
-            return normalize_symbol_name_version
+            additional_settings_fingerprint = ""
+        else:
+            fingerprint_data: dict[str, object] = {
+                "env": env_subset,
+            }
+            if gopls_settings is not None:
+                fingerprint_data["gopls_settings"] = gopls_settings
+            canonical_json = self._canonical_json_or_raise(json, fingerprint_data)
+            additional_settings_fingerprint = hashlib.sha256(canonical_json.encode("utf-8")).hexdigest()[:16]
 
-        fingerprint_data: dict[str, object] = {
-            "env": env_subset,
-            "normalize_symbol_name_version": normalize_symbol_name_version,
-        }
-        if gopls_settings is not None:
-            fingerprint_data["gopls_settings"] = gopls_settings
-
-        canonical_json = self._canonical_json_or_raise(json, fingerprint_data)
-
-        return hashlib.sha256(canonical_json.encode("utf-8")).hexdigest()[:16]
+        return normalize_symbol_name_version, request_document_symbols_impl_version, additional_settings_fingerprint
 
     @override
     def _normalize_symbol_name(self, symbol: RawDocumentSymbol, relative_file_path: str) -> str:
         return symbol["name"].rsplit(".", 1)[-1]
 
+    @override
+    def request_document_symbols(self, relative_file_path: str, file_buffer: LSPFileBuffer | None = None) -> DocumentSymbols:
+        # Override to extend single `type`/`var`/`const` declaration ranges to include the leading
+        # keyword. gopls excludes the keyword from such ranges (unlike `func` declarations), which
+        # causes replace_symbol_body to drop the keyword from the symbol body and replacement range;
+        # a natural keyword-inclusive round-trip edit would then corrupt the file (e.g. `type Foo`
+        # becomes `type type Foo`). See _extend_go_symbol_range_to_include_leading_keyword.
+        document_symbols = super().request_document_symbols(relative_file_path, file_buffer=file_buffer)
+        if not document_symbols.root_symbols:
+            return document_symbols
+
+        # obtain the file lines and a body factory to recompute the bodies of extended symbols
+        with self._open_file_context(relative_file_path, file_buffer, open_in_ls=False) as file_data:
+            file_lines = file_data.split_lines()
+            body_factory = SymbolBodyFactory(file_data)
+
+            # extend ranges recursively, operating on copies so the cached symbols are not mutated.
+            # Children keep their `parent` back-pointer aimed at the original (un-extended) node;
+            # this is intentional and safe, because ancestor traversal only reads name/kind/overload
+            # index (see Symbol.iter_ancestors), none of which the extension changes. Rebinding the
+            # back-pointers would force a deep copy of every extended subtree for no observable gain.
+            def extend_symbol_and_children(symbol: ls_types.UnifiedSymbolInformation) -> ls_types.UnifiedSymbolInformation:
+                extended = self._extend_go_symbol_range_to_include_leading_keyword(symbol, file_lines, body_factory)
+                children = symbol.get("children")
+                if children:
+                    if extended is symbol:
+                        extended = symbol.copy()
+                    extended["children"] = [extend_symbol_and_children(child) for child in children]
+                return extended
+
+            extended_root_symbols = [extend_symbol_and_children(sym) for sym in document_symbols.root_symbols]
+
+        return DocumentSymbols(extended_root_symbols)
+
+    def _extend_go_symbol_range_to_include_leading_keyword(
+        self,
+        symbol: ls_types.UnifiedSymbolInformation,
+        file_lines: list[str],
+        body_factory: SymbolBodyFactory,
+    ) -> ls_types.UnifiedSymbolInformation:
+        """
+        Extend a Go symbol's body range to include a leading `type`/`var`/`const` keyword.
+
+        gopls reports the range of a single `type`/`var`/`const` declaration starting at the
+        declared identifier (after the keyword), whereas the range of a `func` declaration includes
+        the `func` keyword. This asymmetry makes :meth:`replace_symbol_body` omit the keyword from
+        both the displayed body and the replacement range, so re-supplying the keyword in an edit
+        corrupts the file (e.g. ``type Foo`` becomes ``type type Foo``).
+
+        :param symbol: the symbol whose range may be extended.
+        :param file_lines: the lines of the file in which the symbol is defined.
+        :param body_factory: the factory used to recompute the symbol body from the extended range.
+        :return: a copy of the symbol with an extended range, or the original symbol if no leading
+            keyword precedes the identifier on the start line (e.g. for funcs or for grouped
+            declarations such as ``var ( ... )`` whose keyword sits on a separate line).
+        """
+        # determine whether only a declaration keyword precedes the identifier on the start line
+        range_info = symbol["range"]
+        start_line = range_info["start"]["line"]
+        start_char = range_info["start"]["character"]
+        if start_line >= len(file_lines):
+            return symbol
+        prefix = file_lines[start_line][:start_char]
+        match = self._LEADING_DECL_KEYWORD_RE.fullmatch(prefix)
+        if match is None:
+            return symbol
+
+        # extend the range start back to the keyword (excluding indentation), updating both the
+        # symbol range and its location range so the replacement range covers the keyword
+        new_start = ls_types.Position(line=start_line, character=len(match.group("indent")))
+        extended = symbol.copy()
+        extended["range"] = ls_types.Range(start=new_start, end=range_info["end"])
+        location = extended.get("location")
+        if location:
+            location = location.copy()
+            if "range" in location:
+                location["range"] = ls_types.Range(start=new_start, end=location["range"]["end"])
+            extended["location"] = location
+
+        # recompute the body from the now-extended location range so the displayed body stays
+        # consistent with the replacement range; the stale body must be removed first, since the
+        # factory returns an existing SymbolBody as-is and otherwise reads the updated location range
+        extended.pop("body", None)
+        extended["body"] = body_factory.create_symbol_body(extended)
+        return extended
+
     def _start_server(self) -> None:
         """Start gopls server process"""
 

test/resources/repos/go/test_repo/symbol_body.go

@@ -0,0 +1,26 @@
+package main
+
+// BodyStruct is a single type declaration used to verify that replace_symbol_body
+// includes the leading `type` keyword in the symbol body and replacement range.
+type BodyStruct struct {
+	Value int
+}
+
+// NamedInt is a single defined-type declaration (non-struct).
+type NamedInt int
+
+// AliasInt is a type alias declaration.
+type AliasInt = int
+
+// GlobalCounter is a single package-level var declaration.
+var GlobalCounter int = 0
+
+// MaxItems is a single package-level const declaration.
+const MaxItems = 100
+
+// GroupedA and GroupedB are declared in a grouped var block, where the `var`
+// keyword is on a separate line and must NOT be folded into either symbol body.
+var (
+	GroupedA int    = 1
+	GroupedB string = "two"
+)

test/serena/__snapshots__/test_symbol_editing.ambr

@@ -302,6 +302,37 @@
   
   '''
 # ---
+# name: test_go_symbol_replacement_no_double_keyword
+  '''
+  package main
+  
+  // BodyStruct is a single type declaration used to verify that replace_symbol_body
+  // includes the leading `type` keyword in the symbol body and replacement range.
+  type BodyStruct struct {
+  	Value int
+  }
+  
+  // NamedInt is a single defined-type declaration (non-struct).
+  type NamedInt int64
+  
+  // AliasInt is a type alias declaration.
+  type AliasInt = int
+  
+  // GlobalCounter is a single package-level var declaration.
+  var GlobalCounter int = 0
+  
+  // MaxItems is a single package-level const declaration.
+  const MaxItems = 100
+  
+  // GroupedA and GroupedB are declared in a grouped var block, where the `var`
+  // keyword is on a separate line and must NOT be folded into either symbol body.
+  var (
+  	GroupedA int    = 1
+  	GroupedB string = "two"
+  )
+  
+  '''
+# ---
 # name: test_insert_in_rel_to_symbol[test_case0-after]
   '''
   """

test/serena/test_symbol_editing.py

@@ -6,6 +6,7 @@ Recreate the snapshots with `pytest --snapshot-update`.
 import logging
 import os
 import shutil
+import sys
 import tempfile
 import time
 from abc import ABC, abstractmethod
@@ -474,6 +475,62 @@ def test_nix_symbol_replacement_no_double_semicolon(snapshot: SnapshotAssertion)
     test_case.run_test(content_after_ground_truth=snapshot)
 
 
+# A single `type` declaration body that re-supplies the leading `type` keyword, as a user would
+# after copying the body returned by find_symbol. Replacing the body with this must not duplicate
+# the keyword.
+GO_DECL_REPLACEMENT = """type NamedInt int64"""
+
+
+class GoDeclReplacementTest(EditingTest):
+    """Test for replacing a single Go ``type``/``var``/``const`` declaration that should NOT result
+    in a duplicated leading keyword (e.g. ``type type NamedInt``).
+    """
+
+    def __init__(self, language: Language, rel_path: str, symbol_name: str, new_body: str):
+        super().__init__(language, rel_path)
+        self.symbol_name = symbol_name
+        self.new_body = new_body
+
+    def _apply_edit(self, code_editor: CodeEditor) -> None:
+        code_editor.replace_body(self.symbol_name, self.rel_path, self.new_body)
+
+    @overrides
+    def _test_diff(self, code_diff: CodeDiff, snapshot: SnapshotAssertion | None) -> None:
+        # the leading declaration keyword must appear exactly once (no `type type` corruption)
+        for keyword in ("type", "var", "const"):
+            assert f"{keyword} {keyword} " not in code_diff.modified_content, (
+                f"Replacement duplicated the '{keyword}' keyword: {code_diff.modified_content!r}"
+            )
+        return super()._test_diff(code_diff, snapshot)
+
+
+@pytest.mark.go
+@pytest.mark.skipif(sys.platform == "win32", reason="gopls is not provisioned on the Windows CI runner")
+def test_go_symbol_replacement_no_double_keyword(snapshot: SnapshotAssertion):
+    """
+    Test that replacing a Go ``type``/``var``/``const`` declaration does not duplicate the leading
+    keyword.
+
+    This exercises the bug where:
+    - Original: type NamedInt int
+    - Replacement body (as displayed by find_symbol): type NamedInt int64
+    - Bug result would be: type type NamedInt int64 (the original `type ` prefix is kept because the
+      gopls symbol range starts at the identifier, and the supplied body adds another `type`)
+    - Correct result should be: type NamedInt int64
+
+    gopls reports the symbol range of such declarations starting at the identifier (after the
+    keyword), unlike ``func`` declarations whose range includes the ``func`` keyword. The gopls
+    range extension prevents the duplicated keyword.
+    """
+    test_case = GoDeclReplacementTest(
+        Language.GO,
+        "symbol_body.go",
+        "NamedInt",
+        GO_DECL_REPLACEMENT,
+    )
+    test_case.run_test(content_after_ground_truth=snapshot)
+
+
 class RenameSymbolTest(EditingTest):
     def __init__(self, language: Language, rel_path: str, symbol_name: str, new_name: str):
         super().__init__(language, rel_path)

test/solidlsp/go/test_go_basic.py

@@ -48,6 +48,46 @@ class TestGoLanguageServer:
         refs = language_server.request_references(file_path, sel_start["line"], sel_start["character"])
         assert any("main.go" in ref.get("uri", "") for ref in refs), "Expected at least one reference result to point at main.go"
 
+    @pytest.mark.parametrize("language_server", [Language.GO], indirect=True)
+    def test_type_var_const_body_includes_leading_keyword(self, language_server: SolidLanguageServer) -> None:
+        """
+        Single ``type``/``var``/``const`` declarations must expose a body and replacement range that
+        include the leading keyword, just like ``func`` declarations do.
+
+        Regression test for gopls reporting the symbol range of such declarations starting at the
+        declared identifier (after the keyword) rather than at the keyword. That asymmetry made
+        replace_symbol_body drop the keyword from the body and replacement range, so a natural
+        keyword-inclusive round-trip edit corrupted the file (e.g. ``type Foo`` -> ``type type Foo``).
+        """
+        all_symbols, _ = language_server.request_document_symbols("symbol_body.go").get_all_symbols_and_roots()
+        symbols_by_name = {sym.get("name"): sym for sym in all_symbols}
+
+        # single declarations: body starts with the keyword, the range start moves to the keyword
+        # (column 0 here), and the selection range still points at the identifier after the keyword
+        expected_keyword_by_name = {
+            "BodyStruct": "type ",
+            "NamedInt": "type ",
+            "AliasInt": "type ",
+            "GlobalCounter": "var ",
+            "MaxItems": "const ",
+        }
+        for name, keyword in expected_keyword_by_name.items():
+            sym = symbols_by_name.get(name)
+            assert sym is not None, f"{name} not found in symbol_body.go"
+            body = sym["body"].get_text()
+            assert body.startswith(keyword), f"Expected body of {name} to start with {keyword!r}, got {body[:24]!r}"
+            assert sym["location"]["range"]["start"]["character"] == 0, f"Expected {name} body range to start at the keyword (col 0)"
+            assert sym["selectionRange"]["start"]["character"] > 0, f"Expected {name} selectionRange to point at the identifier"
+
+        # grouped declarations keep the keyword on a separate line (e.g. ``var ( ... )``), so their
+        # bodies must NOT include it and their ranges must be left untouched
+        for name in ("GroupedA", "GroupedB"):
+            sym = symbols_by_name.get(name)
+            assert sym is not None, f"{name} not found in symbol_body.go"
+            body = sym["body"].get_text()
+            assert body.startswith(name), f"Expected grouped var {name} body to start with the identifier, got {body[:24]!r}"
+            assert not body.startswith("var"), f"Grouped var {name} body must not include the 'var' keyword"
+
     if language_has_verified_implementation_support(Language.GO):
 
         @pytest.mark.parametrize("language_server", [Language.GO], indirect=True)