diff --git a/CHANGELOG.md b/CHANGELOG.md index d5e7e093b7..f3e0cca6be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,12 @@ Status of the `main` branch. Changes prior to the next official version change w - Language servers and their dependency providers now go through the `subprocess_run` helper instead of calling `subprocess.run` directly (e.g. for installation processes), so all such subprocesses get `stdin=DEVNULL` and can no longer interfere with the stdio MCP connection #1748 + - `scala`: Fix: in a repository whose builds live below its root, Metals was given only the repository + root as a workspace folder, and its own one-level search takes just the first build it finds — so in + a monorepo all but one build were served with no build target, silently returning no cross-file + references. The build roots are now detected and passed as workspace folders, one Metals service per + build; `ls_specific_settings.scala.project_roots` and `project_root_scan_depth` override the + detection #1766 * Hooks: - Add `serena-hooks --client=grok`, including Grok-native PreToolUse allow/deny output. diff --git a/docs/03-special-guides/scala_setup_guide_for_serena.md b/docs/03-special-guides/scala_setup_guide_for_serena.md index 1e4bebca55..9bb21fd294 100644 --- a/docs/03-special-guides/scala_setup_guide_for_serena.md +++ b/docs/03-special-guides/scala_setup_guide_for_serena.md @@ -63,6 +63,21 @@ These instructions cover the setup for projects that use sbt as the build tool, Notes: - Ensure you completed the manual or auto‑import steps so that the build is compiled and indexed; otherwise, code navigation and references may be incomplete until the first successful compile. +--- +## Monorepos: builds below the repository root + +Metals serves one build per workspace folder, so what it needs is the build roots, not the repository root. Serena detects them: if the repository root is not itself a build root (no `build.sbt`, `build.mill`, `pom.xml`, `.bsp/`, …), it searches up to three levels below for directories that are, and passes those to Metals — one Metals service per build. + +Override the detection where it guesses wrong: + +```yaml +# ~/.serena/serena_config.yml or .serena/project.yml +ls_specific_settings: + scala: + project_roots: ["backend", "tooling/plugin"] # relative to the repository root + project_root_scan_depth: 3 # only applies when project_roots is unset +``` + --- ## Running Multiple Metals Instances diff --git a/src/solidlsp/language_servers/scala_language_server.py b/src/solidlsp/language_servers/scala_language_server.py index 2a7d633054..3e0f89e23c 100644 --- a/src/solidlsp/language_servers/scala_language_server.py +++ b/src/solidlsp/language_servers/scala_language_server.py @@ -10,6 +10,7 @@ from overrides import override +from solidlsp.initialize_params import DefaultInitializeParamsBuilder, InitializeParamsBuilder from solidlsp.ls import SolidLanguageServer from solidlsp.ls_config import LanguageServerConfig from solidlsp.ls_utils import PlatformUtils @@ -28,6 +29,40 @@ DEFAULT_CLIENT_NAME = "Serena" DEFAULT_ON_STALE_LOCK = "auto-clean" DEFAULT_LOG_MULTI_INSTANCE_NOTICE = True +DEFAULT_PROJECT_ROOT_SCAN_DEPTH = 3 + +# Files whose presence marks a directory as the root of a build Metals can import, following the +# per-build-tool probes in Metals' `BuildTools` (scala/meta/internal/builds/BuildTools.scala) and +# `BazelBuildTool.workspaceSupportsBsp`. Deliberately partial: the probes that need to read a file's +# contents (scala-cli's BSP scope) are left out, since missing a build root only returns us to the +# previous behaviour, whereas a false positive would hide the real builds beneath it. +BUILD_ROOT_MARKER_FILES = ( + "MODULE.bazel", + "WORKSPACE", + "build.gradle", + "build.gradle.kts", + "build.mill", + "build.mill.scala", + "build.mill.yaml", + "build.sbt", + "build.sc", + "deder.pkl", + "mill", + "mill.bat", + "pom.xml", + "project.scala", + "settings.gradle", + "settings.gradle.kts", +) + +# Directories which, when they hold a JSON file, mark an already-configured Metals project +# (`BuildTools.hasJsonFile`). An empty one is a leftover, not a build. +BUILD_ROOT_MARKER_JSON_DIRS = (".bloop", ".bsp") + +# Directories not worth descending into when scanning for build roots: build output, dependencies, +# and the directories belonging to a build we would have recognised at their parent. They are still +# probed themselves — only the descent below them is skipped. +BUILD_ROOT_SCAN_SKIP_DIRS = frozenset({"node_modules", "out", "project", "src", "target", "venv"}) class StaleLockMode(Enum): @@ -43,6 +78,128 @@ class StaleLockMode(Enum): """Raise an error and refuse to start.""" +def _contains_json_file(path: str) -> bool: + try: + return any(entry.name.endswith(".json") for entry in os.scandir(path)) + except OSError: + return False + + +def _is_build_root(path: str) -> bool: + """ + Whether `path` is the root of a build that Metals can import. + """ + if any(os.path.isfile(os.path.join(path, name)) for name in BUILD_ROOT_MARKER_FILES): + return True + if any(_contains_json_file(os.path.join(path, name)) for name in BUILD_ROOT_MARKER_JSON_DIRS): + return True + # sbt allows the build to be defined entirely under project/, with no build.sbt + build_properties = os.path.join(path, "project", "build.properties") + if os.path.isfile(build_properties): + try: + with open(build_properties, encoding="utf-8", errors="replace") as f: + return any(line.lstrip().startswith("sbt.version") for line in f) + except OSError: + return False + return False + + +def find_build_roots(repository_root_path: str, max_depth: int = DEFAULT_PROJECT_ROOT_SCAN_DEPTH) -> list[str]: + """ + Find the roots of the builds contained in the given repository. + + Metals serves one build per workspace folder, so a repository holding several builds + (or a single build below its root) must name those directories rather than the repository root. + + :param repository_root_path: the repository root + :param max_depth: how many directory levels below the repository root to search; + the search does not descend into a directory that is itself a build root + :return: the absolute paths of the build roots found, or `[repository_root_path]` if there are none + (which leaves Metals' own behaviour unchanged) + """ + if _is_build_root(repository_root_path): + return [repository_root_path] + + roots: list[str] = [] + visited: set[str] = set() + + def scan(directory: str, depth: int) -> None: + # symlinks are followed, as Metals' own search does, so guard against cycles + real_path = os.path.realpath(directory) + if depth > max_depth or real_path in visited: + return + visited.add(real_path) + try: + entries = sorted(os.scandir(directory), key=lambda e: e.name) + except OSError: + return + for entry in entries: + if entry.name.startswith(".") or not entry.is_dir(): + continue + if _is_build_root(entry.path): + roots.append(entry.path) + elif entry.name not in BUILD_ROOT_SCAN_SKIP_DIRS: + scan(entry.path, depth + 1) + + scan(repository_root_path, 1) + return roots or [repository_root_path] + + +class ScalaInitializeParamsBuilder(DefaultInitializeParamsBuilder): + """ + Sends the repository's build roots as the workspace folders, so that Metals creates one + service per build (see `MetalsLanguageServer.initialize`), in place of `ls_workspace_folders`, + which is about what SolidLSP indexes and is shared across a project's language servers. + + `ls_additional_workspace_folders` is still honoured: those folders can lie outside the + repository and so could never be detected, which is the whole point of the setting. + """ + + def __init__(self, ls: SolidLanguageServer, build_roots: list[str]): + super().__init__(ls, set_workspace_folders=False) + self._build_roots = build_roots + + @override + def _apply_updates(self) -> None: + super()._apply_updates() + folders = list(self._build_roots) + for path in self._ls.config.get_absolute_additional_workspace_folders(self._ls.repository_root_path): + if path not in folders: + folders.append(path) + log.info("Workspace folders sent to Metals: %s", folders) + self._set("workspaceFolders", [self._create_workspace_folder_entry(path) for path in folders]) + + +def _parse_project_roots(value: object) -> list[str] | None: + """ + Validate the `project_roots` setting, returning None (i.e. detect them) if it is unusable. + """ + if value is None: + return None + if isinstance(value, str) or not isinstance(value, list) or not all(isinstance(item, str) for item in value): + log.warning(f"Invalid project_roots value {value!r}, expected a list of paths; detecting the build roots instead") + return None + roots: list[str] = [item for item in value if isinstance(item, str)] + if not roots: + log.warning("Empty project_roots; detecting the build roots instead") + return None + return roots + + +def _parse_project_root_scan_depth(value: object) -> int: + """ + Validate the `project_root_scan_depth` setting, falling back to the default if it is unusable. + """ + if value is None: + return DEFAULT_PROJECT_ROOT_SCAN_DEPTH + if isinstance(value, bool) or not isinstance(value, int) or value < 1: + log.warning( + f"Invalid project_root_scan_depth value {value!r}, expected a positive integer; using {DEFAULT_PROJECT_ROOT_SCAN_DEPTH}" + ) + return DEFAULT_PROJECT_ROOT_SCAN_DEPTH + return value + + def _get_scala_settings(solidlsp_settings: SolidLSPSettings) -> dict[str, object]: """ Extract Scala-specific settings with defaults applied. @@ -52,6 +209,8 @@ def _get_scala_settings(solidlsp_settings: SolidLSPSettings) -> dict[str, object - client_name: str - on_stale_lock: StaleLockMode - log_multi_instance_notice: bool + - project_roots: list[str] | None + - project_root_scan_depth: int """ from solidlsp.ls_config import LanguageServerId @@ -60,6 +219,8 @@ def _get_scala_settings(solidlsp_settings: SolidLSPSettings) -> dict[str, object "client_name": DEFAULT_CLIENT_NAME, "on_stale_lock": StaleLockMode.AUTO_CLEAN, "log_multi_instance_notice": DEFAULT_LOG_MULTI_INSTANCE_NOTICE, + "project_roots": None, + "project_root_scan_depth": DEFAULT_PROJECT_ROOT_SCAN_DEPTH, } if not solidlsp_settings.ls_specific_settings: @@ -80,6 +241,8 @@ def _get_scala_settings(solidlsp_settings: SolidLSPSettings) -> dict[str, object "client_name": scala_settings.get("client_name", DEFAULT_CLIENT_NAME), "on_stale_lock": on_stale_lock, "log_multi_instance_notice": scala_settings.get("log_multi_instance_notice", DEFAULT_LOG_MULTI_INSTANCE_NOTICE), + "project_roots": _parse_project_roots(scala_settings.get("project_roots")), + "project_root_scan_depth": _parse_project_root_scan_depth(scala_settings.get("project_root_scan_depth")), } @@ -100,6 +263,16 @@ class ScalaLanguageServer(SolidLanguageServer): metals_version: '1.6.4' # Client identifier sent to Metals (default: DEFAULT_CLIENT_NAME) client_name: 'Serena' + # Build roots to serve, relative to the repository root; when unset, they are + # auto-detected (see find_build_roots) + project_roots: ['backend', 'tooling/plugin'] + # How many levels below the repository root auto-detection searches + project_root_scan_depth: 3 + + Monorepo support: + Metals serves one build per workspace folder, so the build roots — not the repository + root — are what it must be given. They are detected automatically; `project_roots` + overrides the detection where it guesses wrong. Multi-instance support: Metals uses H2 AUTO_SERVER mode (enabled by default) to support multiple @@ -113,8 +286,12 @@ def __init__(self, config: LanguageServerConfig, repository_root_path: str, soli Creates a ScalaLanguageServer instance. This class is not meant to be instantiated directly. Use LanguageServer.create() instead. """ + self._build_roots = self._resolve_build_roots(repository_root_path, solidlsp_settings) + log.info(f"Metals will be given these build roots as workspace folders: {self._build_roots}") + # Check for stale locks before setting up dependencies (fail-fast) - self._check_metals_db_status(repository_root_path, solidlsp_settings) + for build_root in self._build_roots: + self._check_metals_db_status(build_root, solidlsp_settings) scala_lsp_executable_path = self._setup_runtime_dependencies(config, solidlsp_settings) super().__init__( @@ -125,9 +302,37 @@ def __init__(self, config: LanguageServerConfig, repository_root_path: str, soli solidlsp_settings, ) - def _check_metals_db_status(self, repository_root_path: str, solidlsp_settings: SolidLSPSettings) -> None: + @staticmethod + def _resolve_build_roots(repository_root_path: str, solidlsp_settings: SolidLSPSettings) -> list[str]: + """ + Determine the build roots to serve, from the `project_roots` setting if given and by + detection otherwise. + """ + settings = _get_scala_settings(solidlsp_settings) + configured_roots: list[str] | None = settings["project_roots"] # type: ignore[assignment] + if configured_roots is None: + scan_depth: int = settings["project_root_scan_depth"] # type: ignore[assignment] + return find_build_roots(repository_root_path, scan_depth) + + roots = [] + for root in configured_roots: + abs_root = os.path.abspath(os.path.join(repository_root_path, root)) + if os.path.isdir(abs_root): + roots.append(abs_root) + else: + log.warning(f"Configured Scala project root does not exist, skipping: {abs_root}") + if not roots: + log.error("No configured Scala project root exists; detecting the build roots instead") + return find_build_roots(repository_root_path, settings["project_root_scan_depth"]) # type: ignore[arg-type] + return roots + + @override + def _create_initialize_params_builder(self) -> InitializeParamsBuilder: + return ScalaInitializeParamsBuilder(self, self._build_roots) + + def _check_metals_db_status(self, build_root_path: str, solidlsp_settings: SolidLSPSettings) -> None: """ - Check the Metals H2 database status and handle stale locks. + Check the Metals H2 database status of one build root and handle stale locks. This method is called before setting up runtime dependencies to fail-fast if there's a stale lock that the user has configured to fail on. @@ -141,7 +346,7 @@ def _check_metals_db_status(self, repository_root_path: str, solidlsp_settings: cleanup_stale_lock, ) - project_path = Path(repository_root_path) + project_path = Path(build_root_path) status, lock_info = check_metals_db_status(project_path) # Get settings using the shared helper function diff --git a/test/solidlsp/scala/test_scala_build_roots.py b/test/solidlsp/scala/test_scala_build_roots.py new file mode 100644 index 0000000000..2977bdb3a1 --- /dev/null +++ b/test/solidlsp/scala/test_scala_build_roots.py @@ -0,0 +1,182 @@ +""" +Unit tests for the detection of Scala build roots, which Metals is given as workspace folders. +""" + +from pathlib import Path +from types import SimpleNamespace +from typing import cast +from urllib.parse import urlparse +from urllib.request import url2pathname + +import pytest + +from solidlsp import SolidLanguageServer +from solidlsp.language_servers.scala_language_server import ( + DEFAULT_PROJECT_ROOT_SCAN_DEPTH, + ScalaInitializeParamsBuilder, + ScalaLanguageServer, + find_build_roots, +) +from solidlsp.ls_config import LanguageServerConfig, LanguageServerId +from solidlsp.settings import SolidLSPSettings + + +def make_sbt_build(path: Path) -> Path: + path.mkdir(parents=True, exist_ok=True) + (path / "build.sbt").write_text('ThisBuild / scalaVersion := "3.3.6"\n') + (path / "src" / "main" / "scala").mkdir(parents=True) + return path + + +@pytest.mark.scala +class TestFindBuildRoots: + def test_repository_root_is_the_build_root(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path) + assert find_build_roots(str(tmp_path)) == [str(tmp_path)] + + def test_single_build_below_the_root(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "backend") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend")] + + def test_several_builds_below_the_root(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "backend") + make_sbt_build(tmp_path / "tooling") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend"), str(tmp_path / "tooling")] + + def test_nested_build(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "scala" / "backend") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "scala" / "backend")] + + def test_scan_depth_is_bounded(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "a" / "b" / "c") + assert find_build_roots(str(tmp_path), max_depth=2) == [str(tmp_path)] + assert find_build_roots(str(tmp_path), max_depth=3) == [str(tmp_path / "a" / "b" / "c")] + + def test_does_not_descend_into_a_build_root(self, tmp_path: Path) -> None: + """A subproject of an sbt build is not a build root of its own.""" + make_sbt_build(tmp_path / "backend") + make_sbt_build(tmp_path / "backend" / "module") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend")] + + def test_falls_back_to_the_repository_root(self, tmp_path: Path) -> None: + """With nothing to find, Metals' own behaviour is left unchanged.""" + (tmp_path / "docs").mkdir() + assert find_build_roots(str(tmp_path)) == [str(tmp_path)] + + def test_hidden_and_uninteresting_directories_are_skipped(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / ".git" / "backend") + make_sbt_build(tmp_path / "node_modules" / "backend") + assert find_build_roots(str(tmp_path)) == [str(tmp_path)] + + def test_bsp_connection_file_marks_a_build_root(self, tmp_path: Path) -> None: + (tmp_path / "backend" / ".bsp").mkdir(parents=True) + (tmp_path / "backend" / ".bsp" / "sbt.json").write_text("{}") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend")] + + def test_sbt_build_defined_only_under_project(self, tmp_path: Path) -> None: + (tmp_path / "backend" / "project").mkdir(parents=True) + (tmp_path / "backend" / "project" / "build.properties").write_text("sbt.version=1.11.7\n") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend")] + + def test_build_properties_without_a_version_is_not_a_build_root(self, tmp_path: Path) -> None: + (tmp_path / "backend" / "project").mkdir(parents=True) + (tmp_path / "backend" / "project" / "build.properties").write_text("# nothing to see here\n") + assert find_build_roots(str(tmp_path)) == [str(tmp_path)] + + def test_an_empty_bsp_directory_is_not_a_build_root(self, tmp_path: Path) -> None: + """Metals requires a connection file in there, not merely the directory.""" + (tmp_path / "backend" / ".bsp").mkdir(parents=True) + make_sbt_build(tmp_path / "backend" / "module") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend" / "module")] + + def test_a_skipped_directory_is_still_probed(self, tmp_path: Path) -> None: + """`src` and friends are not descended into, but may themselves be a build root.""" + make_sbt_build(tmp_path / "src") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "src")] + + def test_a_skipped_directory_is_not_descended_into(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "node_modules" / "backend") + assert find_build_roots(str(tmp_path)) == [str(tmp_path)] + + def test_bazel_workspace_marker(self, tmp_path: Path) -> None: + (tmp_path / "backend").mkdir() + (tmp_path / "backend" / "MODULE.bazel").write_text("") + assert find_build_roots(str(tmp_path)) == [str(tmp_path / "backend")] + + def test_symlinked_build_is_found_without_looping(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "elsewhere" / "backend") + (tmp_path / "repo").mkdir() + (tmp_path / "repo" / "link").symlink_to(tmp_path / "elsewhere", target_is_directory=True) + (tmp_path / "repo" / "loop").symlink_to(tmp_path / "repo", target_is_directory=True) + assert find_build_roots(str(tmp_path / "repo")) == [str(tmp_path / "repo" / "link" / "backend")] + + +@pytest.mark.scala +class TestResolveBuildRoots: + """The `project_roots` / `project_root_scan_depth` settings, which override the detection.""" + + @staticmethod + def resolve(root: Path, **scala_settings) -> list[str]: + settings = SolidLSPSettings(ls_specific_settings={LanguageServerId.SCALA: scala_settings}) + return ScalaLanguageServer._resolve_build_roots(str(root), settings) + + def test_detection_is_used_when_unset(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "backend") + assert self.resolve(tmp_path) == [str(tmp_path / "backend")] + + def test_configured_roots_are_resolved_against_the_repository_root(self, tmp_path: Path) -> None: + (tmp_path / "a" / "b").mkdir(parents=True) + assert self.resolve(tmp_path, project_roots=["a/b"]) == [str(tmp_path / "a" / "b")] + + def test_configured_roots_need_not_look_like_builds(self, tmp_path: Path) -> None: + """The setting exists precisely for where the detection is wrong.""" + (tmp_path / "odd").mkdir() + assert self.resolve(tmp_path, project_roots=["odd"]) == [str(tmp_path / "odd")] + + def test_a_missing_configured_root_is_skipped(self, tmp_path: Path) -> None: + (tmp_path / "here").mkdir() + assert self.resolve(tmp_path, project_roots=["here", "gone"]) == [str(tmp_path / "here")] + + def test_detection_takes_over_when_no_configured_root_exists(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "backend") + assert self.resolve(tmp_path, project_roots=["gone"]) == [str(tmp_path / "backend")] + + @pytest.mark.parametrize("bad", ["backend", 42, [1, 2], []]) + def test_a_malformed_project_roots_falls_back_to_detection(self, tmp_path: Path, bad: object) -> None: + make_sbt_build(tmp_path / "backend") + assert self.resolve(tmp_path, project_roots=bad) == [str(tmp_path / "backend")] + + @pytest.mark.parametrize("bad", [None, "3", 0, -1, True]) + def test_a_malformed_scan_depth_falls_back_to_the_default(self, tmp_path: Path, bad: object) -> None: + make_sbt_build(tmp_path / "a" / "b" / "c") + assert self.resolve(tmp_path, project_root_scan_depth=bad) == [str(tmp_path / "a" / "b" / "c")] + assert DEFAULT_PROJECT_ROOT_SCAN_DEPTH == 3 + + def test_scan_depth_is_honoured(self, tmp_path: Path) -> None: + make_sbt_build(tmp_path / "a" / "b" / "c") + assert self.resolve(tmp_path, project_root_scan_depth=2) == [str(tmp_path)] + + +@pytest.mark.scala +class TestScalaInitializeParamsBuilder: + @staticmethod + def workspace_folder_paths(tmp_path: Path, build_roots: list[str], additional: list[str]) -> list[str]: + ls = SimpleNamespace( + repository_root_path=str(tmp_path), + custom_settings={}, + config=LanguageServerConfig(ls_id=LanguageServerId.SCALA, additional_workspace_folders=additional), + ) + params = ScalaInitializeParamsBuilder(cast(SolidLanguageServer, ls), build_roots).build() + folders = params["workspaceFolders"] or [] + return [url2pathname(urlparse(folder["uri"]).path) for folder in folders] + + def test_the_build_roots_become_the_workspace_folders(self, tmp_path: Path) -> None: + roots = [str(tmp_path / "alpha"), str(tmp_path / "beta")] + assert self.workspace_folder_paths(tmp_path, roots, additional=[]) == roots + + def test_additional_workspace_folders_are_kept(self, tmp_path: Path) -> None: + """They can lie outside the repository, so detection could never recover them.""" + outside = tmp_path.parent / "shared-lib" + outside.mkdir(exist_ok=True) + roots = [str(tmp_path / "alpha")] + assert self.workspace_folder_paths(tmp_path, roots, additional=[str(outside)]) == [*roots, str(outside)]