Merge pull request #1804 from oraios/ls-notify-on-reopen-changed

Fix stale file state in language servers (with files kept open)

CHANGELOG.md

@@ -43,6 +43,13 @@ Status of the `main` branch. Changes prior to the next official version change w
     now separated by `#` instead (e.g. `create_user#4`), so the reported name path round-trips and
     `find_referencing_symbols`/`replace_symbol_body`/`insert_after_symbol` work on Erlang
     functions #1797
+  - Fix: `LSPFileBuffer`: a stale content hash could be returned if files are kept open 
+    and file contents were not read before trying to retrieve the hash value  
+  - Fix: Change semantics of file opening (`open_file`) in the language server from "open file (if not already open)"
+    to "ensure that the language server has the (current) contents of the file" (by sending `textDocument/didOpen`
+    or `textDocument/didChange`), as this is always the intention of calling the method.
+    If files were kept open in the language server (which the Svelte and Vue language servers did),
+    the language server was not necessarily informed about updated contents.
 
 * JetBrains:
   - `jet_brains_find_symbol`: Disallow wildcard-only search, delegating to overview tool if request is for file

src/solidlsp/ls.py

@@ -107,6 +107,7 @@ class LSPFileBuffer:
         self.language_server = language_server
         self.uri = uri
         self._read_file_modified_date: float | None = None
+        self._read_file_modified_date_passed_to_ls: float | None = None
         self._contents: str | None = None
         self.version = version
         self.language_id = language_id
@@ -120,20 +121,40 @@ class LSPFileBuffer:
     def _open_in_ls(self) -> None:
         """
         Open the file in the language server if it is not already open.
+        If it is already open, make sure the language server has the latest contents of the file.
         """
-        if self._is_open_in_ls:
-            return
-        self._is_open_in_ls = True
-        self.language_server.server.notify.did_open_text_document(
-            {  # ty: ignore[invalid-argument-type]  # dict built from LSPConstants keys; shape matches the TypedDict
-                LSPConstants.TEXT_DOCUMENT: {
-                    LSPConstants.URI: self.uri,
-                    LSPConstants.LANGUAGE_ID: self.language_id,
-                    LSPConstants.VERSION: 0,
-                    LSPConstants.TEXT: self.contents,
+        if not self._is_open_in_ls:
+            self._is_open_in_ls = True
+            current_contents = self.contents
+            self._read_file_modified_date_passed_to_ls = self._read_file_modified_date
+            self.language_server.server.notify.did_open_text_document(
+                {  # ty: ignore[invalid-argument-type]  # dict built from LSPConstants keys; shape matches the TypedDict
+                    LSPConstants.TEXT_DOCUMENT: {
+                        LSPConstants.URI: self.uri,
+                        LSPConstants.LANGUAGE_ID: self.language_id,
+                        LSPConstants.VERSION: self.version,
+                        LSPConstants.TEXT: current_contents,
+                    }
                 }
-            }
-        )
+            )
+        else:
+            # file already open: check if contents have changed and notify if so
+            current_contents = self.contents
+            if self._read_file_modified_date != self._read_file_modified_date_passed_to_ls:
+                self._read_file_modified_date_passed_to_ls = self._read_file_modified_date
+                self.language_server.server.notify.did_change_text_document(
+                    {  # ty: ignore[invalid-argument-type]  # dict built from LSPConstants keys; shape matches the TypedDict
+                        LSPConstants.TEXT_DOCUMENT: {
+                            LSPConstants.URI: self.uri,
+                            LSPConstants.VERSION: self.version,
+                        },
+                        LSPConstants.CONTENT_CHANGES: [
+                            {
+                                LSPConstants.TEXT: current_contents,
+                            }
+                        ],
+                    }
+                )
 
     def close(self) -> None:
         if self._is_open_in_ls:
@@ -146,24 +167,34 @@ class LSPFileBuffer:
             )
 
     def ensure_open_in_ls(self) -> None:
-        """Ensure that the file is opened in the language server."""
+        """
+        Ensure that the file is opened in the language server (or, if it is already open,
+        that the language server is made aware of the file's updated contents in case it
+        has changed on disk).
+        """
         self._open_in_ls()
 
+    def _invalidate_cached_data(self, mtime: float | None = None) -> float | None:
+        """
+        Invalidates cached data (file contents, hash) if the file was modified since it was read
+
+        :param: the current modification time if it was already read
+        """
+        if self._read_file_modified_date is not None:
+            if mtime is None:
+                mtime = self.abs_path.stat().st_mtime
+            if mtime > self._read_file_modified_date:
+                self._contents = None
+                self._content_hash = None
+
     @property
     def contents(self) -> str:
         file_modified_date = self.abs_path.stat().st_mtime
-
-        # if contents are cached, check if they are stale (file modification since last read) and invalidate if so
-        if self._contents is not None:
-            assert self._read_file_modified_date is not None
-            if file_modified_date > self._read_file_modified_date:
-                self._contents = None
-
+        self._invalidate_cached_data(file_modified_date)
         if self._contents is None:
             self._read_file_modified_date = file_modified_date
             self._contents = FileUtils.read_file(str(self.abs_path), self.encoding)
             self._content_hash = None
-
         return self._contents
 
     @contents.setter
@@ -179,6 +210,7 @@ class LSPFileBuffer:
 
     @property
     def content_hash(self) -> str:
+        self._invalidate_cached_data()
         if self._content_hash is None:
             self._content_hash = hashlib.md5(self.contents.encode(self.encoding)).hexdigest()
         return self._content_hash
@@ -1247,10 +1279,15 @@ class SolidLanguageServer(ABC):
     @contextmanager
     def open_file(self, relative_file_path: str, open_in_ls: bool = True) -> Iterator[LSPFileBuffer]:
         """
-        Open a file in the Language Server. This is required before making any requests to the Language Server.
+        Opens a file.
+
+        Note: Opening a file in the language server is typically a precondition for further requests
+        pertaining to the respective file.
 
         :param relative_file_path: The relative path of the file to open.
         :param open_in_ls: whether to open the file in the language server, sending the didOpen notification.
+            If the file is already open but file contents has changed since the original notification,
+            an update notification is sent instead.
             Set this to False to read the local file buffer without notifying the LS; the file can
             be opened in the LS later by calling the `ensure_open_in_ls` method on the returned LSPFileBuffer.
         """
@@ -1910,16 +1947,13 @@ class SolidLanguageServer(ABC):
             file_hash_and_result = self._document_symbols_cache.get(cache_key)
             if file_hash_and_result is None:
                 log.debug("No cache hit for document symbols in %s", relative_file_path)
-                log.debug("perf: document_symbols_cache MISS path=%s", relative_file_path)
             else:
                 file_hash, document_symbols = file_hash_and_result
                 if file_hash == file_data.content_hash:
-                    log.debug("Returning cached document symbols for %s", relative_file_path)
-                    log.debug("perf: document_symbols_cache HIT path=%s", relative_file_path)
+                    log.debug("Returning cached document symbols for %s (hash=%s)", relative_file_path, file_hash)
                     return document_symbols
 
-                log.debug("Cached document symbol content for %s has changed", relative_file_path)
-                log.debug("perf: document_symbols_cache STALE path=%s", relative_file_path)
+                log.debug("Cached document symbol content for %s has changed (old hash=%s)", relative_file_path, file_hash)
 
             # no cached result: request the root symbols from the language server
             root_symbols = self._request_document_symbols(relative_file_path, file_data)
@@ -2014,8 +2048,9 @@ class SolidLanguageServer(ABC):
             document_symbols = DocumentSymbols(unified_root_symbols)
 
             # update cache
-            log.debug("Updating cached document symbols for %s", relative_file_path)
-            self._document_symbols_cache[cache_key] = (file_data.content_hash, document_symbols)
+            content_hash = file_data.content_hash
+            log.debug("Updating cached document symbols for %s (hash=%s)", relative_file_path, content_hash)
+            self._document_symbols_cache[cache_key] = (content_hash, document_symbols)
             self._document_symbols_cache_is_modified = True
 
             return document_symbols

test/conftest.py

@@ -1,4 +1,3 @@
-import logging
 import os
 import platform
 import re
@@ -11,7 +10,7 @@ from typing import Any
 
 import pytest
 from _pytest.mark import Mark, MarkDecorator
-from sensai.util.logging import configure
+from sensai.util import logging
 
 from serena.agent import SerenaAgent
 from serena.config.serena_config import SerenaConfig, SerenaPaths
@@ -26,7 +25,9 @@ from .solidlsp.clojure import is_clojure_cli_available
 from .solidlsp.elixir import EXPERT_UNAVAILABLE
 from .solidlsp.erlang import ERLANG_LS_UNAVAILABLE
 
-configure(level=logging.INFO)
+PYTEST_LOG_LEVEL = logging.DEBUG
+
+logging.configure(level=PYTEST_LOG_LEVEL)
 
 log = logging.getLogger(__name__)
 
@@ -131,7 +132,7 @@ def start_default_ls_context(ls_id: LanguageServerId) -> Iterator[SolidLanguageS
 
 
 def create_default_serena_config():
-    return SerenaConfig().with_headless_mode_overrides()
+    return SerenaConfig(log_level=PYTEST_LOG_LEVEL).with_headless_mode_overrides()
 
 
 def _create_default_project(ls_id: LanguageServerId, repo_root_override: str | None = None) -> Project:

test/serena/test_ls_file_sync.py

@@ -13,7 +13,7 @@ import pytest
 
 from serena.agent import SerenaAgent
 from serena.project import Project
-from serena.tools import FindReferencingSymbolsTool
+from serena.tools import FindReferencingSymbolsTool, FindSymbolTool
 from solidlsp.ls_config import LanguageServerId
 from test.conftest import agent_for_project_context, get_repo_path
 
@@ -132,3 +132,60 @@ def test_ls_low_level_find_references_with_explicit_sync(tmp_path):
     Tests that requesting references directly from the LS works if synchronisation is explicitly requested after external file changes.
     """
     FileSystemSyncTestCase(use_serena_tool=False).run(tmp_path)
+
+
+class SymbolPositionStaleAfterExternalEditTestCase:
+    """
+    Tests that a Changed (not Created/Deleted) external edit to a file that already has an open
+    buffer in the language server session is reflected in symbol positions returned by
+    FindSymbolTool. This exercises SolidLanguageServer.resync_open_buffer, which is needed in
+    addition to workspace/didChangeWatchedFiles: once a document is open, a server may treat the
+    client (not the filesystem) as authoritative for its content and silently ignore watched-file
+    notifications for it (observed with pyright).
+    """
+
+    _TARGET_FILE = os.path.join("test_repo", "services.py")
+    _TARGET_SYMBOL = "create_user"
+    _PAD_LINES = 5
+
+    def _find(self, tool: FindSymbolTool) -> dict:
+        with tool.symbol_dict_grouper.disabled_context():
+            response = tool.apply(name_path_pattern=self._TARGET_SYMBOL, relative_path=self._TARGET_FILE)
+        symbols = json.loads(response)
+        assert len(symbols) == 1, f"expected exactly one match for {self._TARGET_SYMBOL}, got {symbols}"
+        return symbols[0]
+
+    def run(self, tmp_path):
+        repo_root = tmp_path / "repo"
+        shutil.copytree(get_repo_path(LanguageServerId.PYTHON), repo_root)
+        target_abs = repo_root / self._TARGET_FILE
+
+        with agent_for_project_context(LanguageServerId.PYTHON, str(repo_root)) as agent:
+            project = agent.get_active_project_or_raise()
+            ls = next(iter(project.language_server_manager.iter_language_servers()))
+            tool = agent.get_tool(FindSymbolTool)
+
+            # Hold the file's buffer open across the external edit, mirroring the state left
+            # behind by a multi-step editing sequence that keeps a buffer open between calls.
+            with ls.open_file(self._TARGET_FILE):
+                baseline_start_line = self._find(tool)["body_location"]["start_line"]
+
+                original = target_abs.read_text(encoding="utf-8")
+                target_abs.write_text(("# pad\n" * self._PAD_LINES) + original, encoding="utf-8")
+
+                after_start_line = self._find(tool)["body_location"]["start_line"]
+
+        assert after_start_line == baseline_start_line + self._PAD_LINES, (
+            f"expected {self._TARGET_SYMBOL} to have shifted by {self._PAD_LINES} lines after the "
+            f"external edit ({baseline_start_line} -> {baseline_start_line + self._PAD_LINES}), "
+            f"but FindSymbolTool reported {after_start_line}"
+        )
+
+
+def test_find_symbol_tool_reflects_external_change_to_open_buffer(tmp_path):
+    """
+    Regression test for oraios/serena#1593: find_symbol returned a stale position for a symbol
+    in a file that was already open in the language server session and was then edited outside
+    of Serena's own edit tools.
+    """
+    SymbolPositionStaleAfterExternalEditTestCase().run(tmp_path)