mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-13 08:15:59 +00:00
Fix: Language servers were not notified of external file system changes,
causing some symbolic operations (such as `find_referencing_symbols`) to report stale information. An explicit file system synchronisation mechanism is now used to detect changes prior to the affected tool executions. Note that tools like `find_symbol` and `get_symbols_overview` do not need the synchronisation, because they explicitly open all the relevant files themselves. Resolves #1718 Co-authored-by: Niccolò Franzin <niccolo@cognitivext.com>
This commit is contained in:
1 parent
5c85d961a0
commit
16e8e24da1
5 files changed
+231
-7
No files matched your search
@@ -18,6 +18,10 @@ Status of the `main` branch. Changes prior to the next official version change w
|
||||
* Language Servers:
|
||||
- Fix: Properly differentiate between raw and high-level symbol cache fingerprints, avoiding unnecessary
|
||||
invalidations of the raw cache when only the derived high-level representation changes
|
||||
- Fix: Language servers were not notified of external file system changes, causing some
|
||||
symbolic operations (such as `find_referencing_symbols`) to report stale information.
|
||||
An explicit file system polling mechanism is now used to detect changes prior to the affected
|
||||
tool executions.
|
||||
- Improve uv-based language server launch command compatibility: Use the more widely supported
|
||||
`uv tool run` instead of `uv x` #1721
|
||||
- Java (JDT-LS): add `runtimes` to `ls_specific_settings.java`, a list of extra JRE/JDK entries
|
||||
|
||||
+113
-5
@@ -2,14 +2,20 @@ import logging
|
||||
import os.path
|
||||
import threading
|
||||
from collections.abc import Iterator
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from sensai.util.logging import LogTime
|
||||
|
||||
from serena.config.serena_config import ProjectConfig, SerenaPaths
|
||||
from solidlsp import SolidLanguageServer
|
||||
from solidlsp.ls_config import Language, LanguageServerConfig
|
||||
from solidlsp.lsp_protocol_handler.lsp_types import DidChangeWatchedFilesParams, FileChangeType, FileEvent
|
||||
from solidlsp.settings import SolidLSPSettings
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from .project import Project
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
||||
|
||||
@@ -68,9 +74,7 @@ class LanguageServerManager:
|
||||
"""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
language_servers: dict[Language, SolidLanguageServer],
|
||||
language_server_factory: LanguageServerFactory | None = None,
|
||||
self, language_servers: dict[Language, SolidLanguageServer], language_server_factory: LanguageServerFactory, project: "Project"
|
||||
) -> None:
|
||||
"""
|
||||
:param language_servers: a mapping from language to language server; the servers are assumed to be already started.
|
||||
@@ -81,6 +85,7 @@ class LanguageServerManager:
|
||||
"""
|
||||
self._language_servers = language_servers
|
||||
self._language_server_factory = language_server_factory
|
||||
self._file_change_notifier = LanguageServerFileChangeNotifier(project, self)
|
||||
|
||||
@property
|
||||
def _default_language_server(self) -> SolidLanguageServer:
|
||||
@@ -89,13 +94,14 @@ class LanguageServerManager:
|
||||
return next(iter(self._language_servers.values()))
|
||||
|
||||
@staticmethod
|
||||
def from_languages(languages: list[Language], factory: LanguageServerFactory) -> "LanguageServerManager":
|
||||
def from_languages(languages: list[Language], factory: LanguageServerFactory, project: "Project") -> "LanguageServerManager":
|
||||
"""
|
||||
Creates a manager with language servers for the given languages using the given factory.
|
||||
The language servers are started in parallel threads.
|
||||
|
||||
:param languages: the languages for which to spawn language servers
|
||||
:param factory: the factory for language server creation
|
||||
:param project: the project for which the language servers are created
|
||||
:return: the instance
|
||||
"""
|
||||
|
||||
@@ -145,7 +151,7 @@ class LanguageServerManager:
|
||||
failure_messages = "\n".join([f"{lang.value}: {e}" for lang, e in exceptions.items()])
|
||||
raise LanguageServerManagerInitialisationError(f"Failed to start {len(exceptions)} language server(s):\n{failure_messages}")
|
||||
|
||||
return LanguageServerManager(language_servers, factory)
|
||||
return LanguageServerManager(language_servers, factory, project)
|
||||
|
||||
def _ensure_functional_ls(self, ls: SolidLanguageServer) -> SolidLanguageServer:
|
||||
if not ls.is_running():
|
||||
@@ -254,3 +260,105 @@ class LanguageServerManager:
|
||||
|
||||
def has_suitable_ls_for_file(self, relative_file_path: str) -> bool:
|
||||
return self._get_suitable_language_server(relative_file_path) is not None
|
||||
|
||||
def sync_file_system_changes(self) -> int:
|
||||
"""
|
||||
Polls the file system for changes to source files and notifies the language servers of any changes
|
||||
(particularly changes that happened outside of Serena's own file tools, which are not covered by
|
||||
the notifications sent by those tools/CodeEditor).
|
||||
|
||||
:return: the number of individual file change events detected (0 if nothing changed).
|
||||
"""
|
||||
log.info("Polling file system for changes to source files ...")
|
||||
num_changes = self._file_change_notifier.poll_and_notify()
|
||||
log.info(f"File system polling complete; {num_changes} change events sent to language servers.")
|
||||
return num_changes
|
||||
|
||||
|
||||
class LanguageServerFileChangeNotifier:
|
||||
"""
|
||||
Detects changes to source files on disk and notifies language servers of those changes.
|
||||
"""
|
||||
|
||||
def __init__(self, project: "Project", language_server_manager: LanguageServerManager) -> None:
|
||||
self._project = project
|
||||
self._language_server_manager = language_server_manager
|
||||
self._freshness_last_seen_mtimes: dict[str, float] | None = None
|
||||
self._freshness_lock = threading.Lock()
|
||||
|
||||
def poll_and_notify(self) -> int:
|
||||
"""
|
||||
Detects source files that were changed, created or deleted on disk since the last call
|
||||
and notifies every language server managed for this project via the LSP
|
||||
``workspace/didChangeWatchedFiles`` notification.
|
||||
|
||||
This exists because Serena's own file and symbol tools notify the language server inline
|
||||
(via didOpen/didChange/didClose) when they edit a file, but edits made through any other
|
||||
channel (another editor, a second agent, a git checkout, a build step) are otherwise
|
||||
invisible to a warm language server, causing symbolic queries to answer from a stale index.
|
||||
|
||||
The set of files considered is exactly the set Serena itself tracks (see
|
||||
:meth:`gather_source_files`), so no separate file-discovery logic has to be kept in sync.
|
||||
The dominant cost is the directory walk plus one ``os.stat`` per tracked file; this is
|
||||
intended to be called before symbolic tool invocations rather than on a timer.
|
||||
|
||||
:return: the number of change events sent (0 if nothing changed, if no language server is
|
||||
running yet, or on the first call, which only establishes the baseline).
|
||||
"""
|
||||
current: dict[str, float] = {}
|
||||
for rel_path in self._project.gather_source_files():
|
||||
try:
|
||||
current[rel_path] = os.stat(os.path.join(self._project.project_root, rel_path)).st_mtime
|
||||
except OSError:
|
||||
continue
|
||||
|
||||
# Read-diff-swap under the lock only; the filesystem walk above and the LSP notifications
|
||||
# below stay outside it so concurrent callers do not serialize on I/O.
|
||||
with self._freshness_lock:
|
||||
previous = self._freshness_last_seen_mtimes
|
||||
self._freshness_last_seen_mtimes = current
|
||||
|
||||
if previous is None:
|
||||
return 0
|
||||
|
||||
# compute the set of individual events (created, changed, deleted)
|
||||
events: list[tuple[str, FileChangeType]] = []
|
||||
for rel_path, mtime in current.items():
|
||||
prev_mtime = previous.get(rel_path)
|
||||
if prev_mtime is None:
|
||||
events.append((rel_path, FileChangeType.Created))
|
||||
elif mtime > prev_mtime:
|
||||
events.append((rel_path, FileChangeType.Changed))
|
||||
events.extend((rel_path, FileChangeType.Deleted) for rel_path in previous if rel_path not in current)
|
||||
|
||||
if not events:
|
||||
return 0
|
||||
|
||||
# create the change didChangeWatchedFiles notification
|
||||
changes: list[FileEvent] = [
|
||||
{"uri": Path(self._project.project_root, rel_path).resolve().as_uri(), "type": change_type} for rel_path, change_type in events
|
||||
]
|
||||
params: DidChangeWatchedFilesParams = {"changes": changes}
|
||||
created_paths = [rel_path for rel_path, change_type in events if change_type == FileChangeType.Created]
|
||||
|
||||
for ls in self._language_server_manager.iter_language_servers():
|
||||
# send the didChangeWatchedFiles notification to the language server
|
||||
try:
|
||||
ls.server.notify.did_change_watched_files(params)
|
||||
except Exception as e:
|
||||
log.error("Failed to notify language server of watched file changes", exc_info=e)
|
||||
|
||||
# A didChangeWatchedFiles(Created) notification alone is not enough for every backend
|
||||
# (observed with pyright) to fold a brand-new file into its cross-file reference graph;
|
||||
# an open/close cycle forces the parse+bind that Serena's own file tools trigger via
|
||||
# SolidLanguageServer.open_file().
|
||||
for rel_path in created_paths:
|
||||
if ls.is_ignored_path(rel_path, ignore_unsupported_files=True):
|
||||
continue
|
||||
try:
|
||||
with ls.open_file(rel_path):
|
||||
pass
|
||||
except Exception as e:
|
||||
log.error(f"Failed to refresh newly created file {rel_path!r} in language server", exc_info=e)
|
||||
|
||||
return len(events)
|
||||
@@ -493,7 +493,7 @@ class Project(ToStringMixin):
|
||||
ls_specific_settings=ls_specific_settings,
|
||||
trace_lsp_communication=self.serena_config.trace_lsp_communication,
|
||||
)
|
||||
self.language_server_manager = LanguageServerManager.from_languages(self.project_config.languages, factory)
|
||||
self.language_server_manager = LanguageServerManager.from_languages(self.project_config.languages, factory, self)
|
||||
return self.language_server_manager
|
||||
except Exception as e:
|
||||
self._language_server_manager_init_error = e
|
||||
@@ -557,6 +557,14 @@ class Project(ToStringMixin):
|
||||
log.info("Removing and stopping the language server for language %s ...", language.value)
|
||||
self.language_server_manager.remove_language_server(language)
|
||||
|
||||
def ls_sync_file_system_changes(self) -> int:
|
||||
"""
|
||||
Synchronizes file system changes with the project's associated language server(s), if applicable
|
||||
"""
|
||||
if self.language_server_manager:
|
||||
return self.language_server_manager.sync_file_system_changes()
|
||||
return 0
|
||||
|
||||
def shutdown(self, timeout: float = 2.0) -> None:
|
||||
if self.language_server_manager is not None:
|
||||
self.language_server_manager.stop_all(save_cache=True, timeout=timeout)
|
||||
|
||||
@@ -274,11 +274,13 @@ class FindReferencingSymbolsTool(Tool, ToolMarkerSymbolicRead):
|
||||
:param max_answer_chars: max result length; -1 for default
|
||||
:return: a list of JSON objects with the symbols referencing the requested symbol
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
include_body = False # It is probably never a good idea to include the body of the referencing symbols
|
||||
parsed_include_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in include_kinds] if include_kinds else None
|
||||
parsed_exclude_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in exclude_kinds] if exclude_kinds else None
|
||||
symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
|
||||
symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
references_in_symbols = symbol_retriever.find_referencing_symbols(
|
||||
name_path,
|
||||
relative_file_path=relative_path,
|
||||
@@ -361,6 +363,8 @@ class FindImplementationsTool(Tool, ToolMarkerSymbolicRead):
|
||||
:param max_answer_chars: max result length; -1 for default
|
||||
:return: a list of JSON objects with the symbols implementing the requested symbol
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
include_body = False
|
||||
parsed_include_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in include_kinds] if include_kinds else None
|
||||
parsed_exclude_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in exclude_kinds] if exclude_kinds else None
|
||||
@@ -414,6 +418,8 @@ class FindDeclarationTool(Tool, ToolMarkerSymbolicRead):
|
||||
:param include_body: whether to include the symbol's body in the result. Default False.
|
||||
:param include_info: whether to include additional info (hover-like). Default False.
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
relative_path = self._sanitize_input_param(relative_path)
|
||||
regex = self._sanitize_input_param(regex)
|
||||
@@ -496,6 +502,8 @@ class GetDiagnosticsForFileTool(Tool, ToolMarkerSymbolicRead):
|
||||
:param max_answer_chars: max result length; -1 for default
|
||||
:return: grouped diagnostics for the requested file.
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
diagnostics = symbol_retriever.get_file_diagnostics(
|
||||
relative_file_path=relative_path,
|
||||
@@ -547,6 +555,8 @@ class GetDiagnosticsForSymbolTool(Tool, ToolMarkerSymbolicRead, ToolMarkerOption
|
||||
:param max_answer_chars: max result length; -1 for default
|
||||
:return: grouped diagnostics for the requested symbol and, optionally, its referencing symbols.
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
diagnostics_by_symbol = symbol_retriever.get_symbol_diagnostics(
|
||||
name_path=name_path,
|
||||
@@ -693,6 +703,8 @@ class SafeDeleteSymbol(Tool, ToolMarkerSymbolicEdit):
|
||||
:param name_path_pattern: name path of the symbol to delete
|
||||
:param relative_path: the relative path to the file containing the symbol to delete
|
||||
"""
|
||||
self.project.ls_sync_file_system_changes()
|
||||
|
||||
ls_symbol_retriever = self.create_language_server_symbol_retriever()
|
||||
symbol = ls_symbol_retriever.find_unique(name_path_pattern, substring_matching=False, within_relative_path=relative_path)
|
||||
symbol_rel_path = symbol.relative_path
|
||||
|
||||
@@ -0,0 +1,92 @@
|
||||
"""
|
||||
End-to-end test for language server file synchronisation as a result of externally-made file changes.
|
||||
|
||||
Exercises all three ``FileChangeType`` branches against a real pyright backend:
|
||||
Created (new caller file), Changed (append a second caller), Deleted (remove the caller file).
|
||||
"""
|
||||
|
||||
import os
|
||||
import shutil
|
||||
|
||||
import pytest
|
||||
|
||||
from solidlsp import SolidLanguageServer
|
||||
from solidlsp.ls_config import Language
|
||||
from test.conftest import get_repo_path, project_with_ls_context
|
||||
|
||||
pytestmark = pytest.mark.python
|
||||
|
||||
# A method that already has a call site in the fixture, so its baseline reference set is non-empty.
|
||||
_TARGET_FILE = os.path.join("test_repo", "services.py")
|
||||
_TARGET_SYMBOL = "create_user"
|
||||
|
||||
_CALLER_REL_PATH = os.path.join("test_repo", "external_caller.py")
|
||||
_CALLER_ONE = "external_caller_one"
|
||||
_CALLER_TWO = "external_caller_two"
|
||||
|
||||
|
||||
def _caller_source(*function_names: str) -> str:
|
||||
body = "\n\n".join(
|
||||
f"def {name}() -> User:\n return UserService().{_TARGET_SYMBOL}('x', 'y', 'z@example.com')" for name in function_names
|
||||
)
|
||||
return "from test_repo.models import User\nfrom test_repo.services import UserService\n\n\n" + body + "\n"
|
||||
|
||||
|
||||
def _referencing_symbol_names(ls: SolidLanguageServer) -> list[str]:
|
||||
"""Names of the symbols that reference ``_TARGET_SYMBOL``, per the warm language server."""
|
||||
document_symbols = ls.request_document_symbols(_TARGET_FILE).get_all_symbols_and_roots()
|
||||
target = next((s for s in document_symbols[0] if s.get("name") == _TARGET_SYMBOL), None)
|
||||
assert target is not None and "selectionRange" in target, f"{_TARGET_SYMBOL} not found in {_TARGET_FILE}"
|
||||
start = target["selectionRange"]["start"]
|
||||
references = ls.request_referencing_symbols(_TARGET_FILE, start["line"], start["character"])
|
||||
return [ref.symbol["name"] for ref in references]
|
||||
|
||||
|
||||
def test_external_changes_become_visible_after_sync(tmp_path):
|
||||
# Work on an isolated copy so we can freely create/edit/delete files under the project root.
|
||||
repo_root = tmp_path / "repo"
|
||||
shutil.copytree(get_repo_path(Language.PYTHON), repo_root)
|
||||
caller_abs = repo_root / _CALLER_REL_PATH
|
||||
|
||||
with project_with_ls_context(Language.PYTHON, str(repo_root)) as project:
|
||||
language_server = next(iter(project.language_server_manager.iter_language_servers()))
|
||||
|
||||
def sync_fs() -> int:
|
||||
return project.language_server_manager.sync_file_system_changes()
|
||||
|
||||
# Warm the reference index, then establish the freshness baseline (first poll never notifies).
|
||||
_referencing_symbol_names(language_server)
|
||||
assert sync_fs() == 0, "first poll must only establish the baseline"
|
||||
|
||||
# --- Created -------------------------------------------------------------------------------
|
||||
caller_abs.write_text(_caller_source(_CALLER_ONE), encoding="utf-8")
|
||||
|
||||
# Adversarial: without the poll, the warm server has never seen the new file -> stale.
|
||||
assert _CALLER_ONE not in _referencing_symbol_names(language_server), "expected the new caller to be invisible before poll"
|
||||
|
||||
assert sync_fs() == 1, "one Created event expected"
|
||||
assert _CALLER_ONE in _referencing_symbol_names(language_server), "new caller must be visible after poll"
|
||||
|
||||
# --- Changed -------------------------------------------------------------------------------
|
||||
caller_abs.write_text(_caller_source(_CALLER_ONE, _CALLER_TWO), encoding="utf-8")
|
||||
assert sync_fs() == 1, "one Changed event expected"
|
||||
names_after_change = _referencing_symbol_names(language_server)
|
||||
assert _CALLER_TWO in names_after_change, "appended caller must be visible after poll"
|
||||
assert _CALLER_ONE in names_after_change
|
||||
|
||||
# --- Deleted -------------------------------------------------------------------------------
|
||||
caller_abs.unlink()
|
||||
assert sync_fs() == 1, "one Deleted event expected"
|
||||
names_after_delete = _referencing_symbol_names(language_server)
|
||||
assert _CALLER_ONE not in names_after_delete, "deleted caller must disappear after poll"
|
||||
assert _CALLER_TWO not in names_after_delete
|
||||
|
||||
|
||||
def test_first_poll_is_baseline_only(tmp_path):
|
||||
"""A newly-loaded project must not report its entire pre-existing file set as changed."""
|
||||
repo_root = tmp_path / "repo"
|
||||
shutil.copytree(get_repo_path(Language.PYTHON), repo_root)
|
||||
with project_with_ls_context(Language.PYTHON, str(repo_root)) as project:
|
||||
assert project.ls_sync_file_system_changes() == 0
|
||||
# No external edits between the two calls -> still nothing to report.
|
||||
assert project.ls_sync_file_system_changes() == 0
|
||||
Reference in new issue
Block a user