diff --git a/CHANGELOG.md b/CHANGELOG.md index 94e06e64..bc1ac42d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/solidlsp/ls.py b/src/solidlsp/ls.py index a52ee0b1..990b1422 100644 --- a/src/solidlsp/ls.py +++ b/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 diff --git a/test/conftest.py b/test/conftest.py index 9110b68e..c75e12db 100644 --- a/test/conftest.py +++ b/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: diff --git a/test/serena/test_ls_file_sync.py b/test/serena/test_ls_file_sync.py index 4902ffa7..618559b3 100644 --- a/test/serena/test_ls_file_sync.py +++ b/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)