Merge pull request #930 from krassowski/optional-virtual-documents-on-disk

Make `.virtual_documents` optional

Author: krassowski

docs/Configuring.ipynb

@@ -80,6 +80,9 @@
     "    matching the `contentsModel` against the registered file types using\n",
     "    `getFileTypeForModel()` method (if multiple MIME types are present, the\n",
     "    first one will be used).\n",
+    "- `requires_documents_on_disk` should be `false` for all new specifications, as\n",
+    "  any code paths requiring documents on disks should be fixed in the LSP servers\n",
+    "  rather than masked by using the `.virtual_documents` workaround.\n",
     "\n",
     "```python\n",
     "# ./jupyter_server_config.json                   ---------- unique! -----------\n",
@@ -211,10 +214,13 @@
     "\n",
     "> default: os.getenv(\"JP_LSP_VIRTUAL_DIR\", \".virtual_documents\")\n",
     "\n",
-    "Path to virtual documents relative to the content manager root directory.\n",
+    "Path (relative to the content manager root directory) where a transient copy of\n",
+    "the virtual documents should be written allowing LSP servers to access the file\n",
+    "using operating system's file system APIs if they need so (which is\n",
+    "discouraged).\n",
     "\n",
-    "Its default value can be set with `JP_LSP_VIRTUAL_DIR` environment variable and\n",
-    "fallback to `.virtual_documents`."
+    "Its default value can be set with `JP_LSP_VIRTUAL_DIR` environment variable. The\n",
+    "fallback value is `.virtual_documents`."
    ]
   },
   {
@@ -340,7 +346,7 @@
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
-   "version": "3.7.9"
+   "version": "3.8.6"
   },
   "widgets": {
    "application/vnd.jupyter.widget-state+json": {

packages/jupyterlab-lsp/src/connection_manager.ts

@@ -391,27 +391,38 @@ export namespace DocumentConnectionManager {
     virtual_document: VirtualDocument,
     language: string
   ): IURIs {
-    const { settings } = Private.getLanguageServerManager();
-    const wsBase = settings.wsUrl;
+    const serverManager = Private.getLanguageServerManager();
+    const wsBase = serverManager.settings.wsUrl;
     const rootUri = PageConfig.getOption('rootUri');
     const virtualDocumentsUri = PageConfig.getOption('virtualDocumentsUri');
 
-    const baseUri = virtual_document.has_lsp_supported_file
-      ? rootUri
-      : virtualDocumentsUri;
-
     // for now take the best match only
-    const matchingServers =
-      Private.getLanguageServerManager().getMatchingServers({
-        language
-      });
-    const language_server_id =
+    const serverOptions: ILanguageServerManager.IGetServerIdOptions = {
+      language
+    };
+    const matchingServers = serverManager.getMatchingServers(serverOptions);
+    const languageServerId =
       matchingServers.length === 0 ? null : matchingServers[0];
 
-    if (language_server_id === null) {
+    if (languageServerId === null) {
       throw `No language server installed for language ${language}`;
     }
 
+    const specs = serverManager.getMatchingSpecs(serverOptions);
+    const spec = specs.get(languageServerId);
+    if (!spec) {
+      console.warn(
+        `Specification not available for server ${languageServerId}`
+      );
+    }
+    const requiresOnDiskFiles = spec?.requires_documents_on_disk ?? true;
+    const supportsInMemoryFiles = !requiresOnDiskFiles;
+
+    const baseUri =
+      virtual_document.has_lsp_supported_file || supportsInMemoryFiles
+        ? rootUri
+        : virtualDocumentsUri;
+
     // workaround url-parse bug(s) (see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/595)
     let documentUri = URLExt.join(baseUri, virtual_document.uri);
     if (
@@ -435,7 +446,7 @@ export namespace DocumentConnectionManager {
         wsBase,
         ILanguageServerManager.URL_NS,
         'ws',
-        language_server_id
+        languageServerId
       )
     };
   }

python_packages/jupyter_lsp/jupyter_lsp/schema/schema.json

@@ -126,6 +126,11 @@
           "title": "Extensions",
           "type": "array"
         },
+        "requires_documents_on_disk": {
+          "default": true,
+          "description": "Whether to write un-saved documents to disk in a transient `.virtual_documents` directory. Well-behaved language servers that work against in-memory files should set this to `false`, which will become the default in the future.",
+          "type": "boolean"
+        },
         "install": {
           "$ref": "#/definitions/install-bundle",
           "description": "a list of installation approaches keyed by package manager, e.g. pip, npm, yarn, apt",

python_packages/jupyter_lsp/jupyter_lsp/serverextension.py

@@ -16,12 +16,28 @@ async def initialize(nbapp, virtual_documents_uri):  # pragma: no cover
 
     from .virtual_documents_shadow import setup_shadow_filesystem
 
-    manager = nbapp.language_server_manager
+    manager: LanguageServerManager = nbapp.language_server_manager
 
     with concurrent.futures.ThreadPoolExecutor() as pool:
         await nbapp.io_loop.run_in_executor(pool, manager.initialize)
 
-    setup_shadow_filesystem(virtual_documents_uri=virtual_documents_uri)
+    servers_requiring_disk_access = [
+        server_id
+        for server_id, server in manager.language_servers.items()
+        if server.get("requires_documents_on_disk", True)
+    ]
+
+    if any(servers_requiring_disk_access):
+        nbapp.log.debug(
+            "[lsp] Servers that requested virtual documents on disk: %s",
+            servers_requiring_disk_access,
+        )
+        setup_shadow_filesystem(virtual_documents_uri=virtual_documents_uri)
+    else:
+        nbapp.log.debug(
+            "[lsp] None of the installed servers require virtual documents"
+            " disabling shadow filesystem."
+        )
 
     nbapp.log.debug(
         "[lsp] The following Language Servers will be available: {}".format(

python_packages/jupyter_lsp/jupyter_lsp/specs/pyright.py

@@ -20,4 +20,5 @@ class PyrightLanguageServer(NodeModuleSpec):
             jlpm="jlpm add --dev {}".format(key),
         ),
         config_schema=load_config_schema(key),
+        requires_documents_on_disk=False,
     )

python_packages/jupyter_lsp/jupyter_lsp/tests/test_virtual_documents_shadow.py

@@ -1,4 +1,5 @@
 from pathlib import Path
+from types import SimpleNamespace
 from typing import List
 
 import pytest
@@ -90,6 +91,13 @@ def shadow_path(tmpdir):
     return str(tmpdir.mkdir(".virtual_documents"))
 
 
+@pytest.fixture
+def manager():
+    return SimpleNamespace(
+        language_servers={"python-lsp-server": {"requires_documents_on_disk": True}}
+    )
+
+
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
     "message_func, content, expected_content",
@@ -99,26 +107,66 @@ def shadow_path(tmpdir):
         [did_save_with_text, "content at save", "content at save"],
     ],
 )
-async def test_shadow(shadow_path, message_func, content, expected_content):
+async def test_shadow(shadow_path, message_func, content, expected_content, manager):
     shadow = setup_shadow_filesystem(Path(shadow_path).as_uri())
     ok_file_path = Path(shadow_path) / "test.py"
 
     message = message_func(ok_file_path.as_uri(), content)
-    result = await shadow("client", message, ["python"], None)
+    result = await shadow("client", message, "python-lsp-server", manager)
     assert isinstance(result, str)
 
-    with open(str(ok_file_path)) as f:  # str is a Python 3.5 relict
+    with open(ok_file_path) as f:
         assert f.read() == expected_content
 
 
 @pytest.mark.asyncio
-async def test_shadow_failures(shadow_path):
+async def test_no_shadow_for_well_behaved_server(
+    shadow_path,
+):
+    """We call server well behaved when it does not require a disk copy"""
+    shadow_path_for_well = Path(shadow_path) / "well"
+    shadow = setup_shadow_filesystem(Path(shadow_path_for_well).as_uri())
+    ok_file_path = Path(shadow_path_for_well) / "test.py"
+
+    manager = SimpleNamespace(
+        language_servers={"python-lsp-server": {"requires_documents_on_disk": False}}
+    )
+
+    message = did_open(ok_file_path.as_uri(), "content\nof\nopened\nfile")
+    result = await shadow("client", message, "python-lsp-server", manager)
+    # should short-circuit for well behaved server
+    assert result is None
+    # should not create the directory
+    assert not shadow_path_for_well.exists()
+
+
+@pytest.mark.asyncio
+async def test_shadow_created_for_ill_behaved_server(
+    shadow_path,
+):
+    shadow_path_for_ill = Path(shadow_path) / "ill"
+    shadow = setup_shadow_filesystem(shadow_path_for_ill.as_uri())
+    ok_file_path = Path(shadow_path_for_ill) / "test.py"
+
+    manager = SimpleNamespace(
+        language_servers={"python-lsp-server": {"requires_documents_on_disk": True}}
+    )
 
+    message = did_open(ok_file_path.as_uri(), "content\nof\nopened\nfile")
+    result = await shadow("client", message, "python-lsp-server", manager)
+    assert result is not None
+    # should create the directory at given path
+    assert shadow_path_for_ill.exists()
+    assert shadow_path_for_ill.is_dir()
+
+
+@pytest.mark.asyncio
+async def test_shadow_failures(shadow_path, manager):
     shadow = setup_shadow_filesystem(Path(shadow_path).as_uri())
     ok_file_uri = (Path(shadow_path) / "test.py").as_uri()
 
     def run_shadow(message):
-        return shadow("client", message, ["python"], None)
+        return shadow("client", message, "python-lsp-server", manager)
 
     # missing textDocument
     with pytest.raises(ShadowFilesystemError, match="Could not get textDocument from"):

python_packages/jupyter_lsp/jupyter_lsp/types.py

@@ -20,12 +20,14 @@
     Pattern,
     Text,
     Union,
+    cast,
 )
 
 try:
     from jupyter_server.transutils import _i18n as _
 except ImportError:  # pragma: no cover
     from jupyter_server.transutils import _
+
 from traitlets import Instance
 from traitlets import List as List_
 from traitlets import Unicode, default
@@ -44,7 +46,7 @@ def __call__(
             scope: Text,
             message: LanguageServerMessage,
             language_server: Text,
-            manager: "HasListeners",
+            manager: "LanguageServerManagerAPI",
         ) -> Awaitable[None]:
             ...
 
@@ -89,7 +91,7 @@ async def __call__(
         scope: Text,
         message: LanguageServerMessage,
         language_server: Text,
-        manager: "HasListeners",
+        manager: "LanguageServerManagerAPI",
     ) -> None:
         """actually dispatch the message to the listener and capture any errors"""
         try:
@@ -182,7 +184,7 @@ async def wait_for_listeners(
                     scope_val,
                     message=message,
                     language_server=language_server,
-                    manager=self,
+                    manager=cast("LanguageServerManagerAPI", self),
                 )
                 for listener in listeners
                 if listener.wants(message, language_server)
@@ -195,6 +197,8 @@ async def wait_for_listeners(
 class LanguageServerManagerAPI(LoggingConfigurable, HasListeners):
     """Public API that can be used for python-based spec finders and listeners"""
 
+    language_servers: KeyedLanguageServerSpecs
+
     nodejs = Unicode(help=_("path to nodejs executable")).tag(config=True)
 
     node_roots = List_([], help=_("absolute paths in which to seek node_modules")).tag(

python_packages/jupyter_lsp/jupyter_lsp/virtual_documents_shadow.py

@@ -8,6 +8,7 @@
 
 from .manager import lsp_message_listener
 from .paths import file_uri_to_path
+from .types import LanguageServerManagerAPI
 
 # TODO: make configurable
 MAX_WORKERS = 4
@@ -101,21 +102,16 @@ class ShadowFilesystemError(ValueError):
     """Error in the shadow file system."""
 
 
-def setup_shadow_filesystem(virtual_documents_uri):
+def setup_shadow_filesystem(virtual_documents_uri: str):
 
     if not virtual_documents_uri.startswith("file:/"):
         raise ShadowFilesystemError(  # pragma: no cover
             'Virtual documents URI has to start with "file:/", got '
             + virtual_documents_uri
         )
 
+    initialized = False
     shadow_filesystem = Path(file_uri_to_path(virtual_documents_uri))
-    # create if does no exist (so that removal does not raise)
-    shadow_filesystem.mkdir(parents=True, exist_ok=True)
-    # remove with contents
-    rmtree(str(shadow_filesystem))
-    # create again
-    shadow_filesystem.mkdir(parents=True, exist_ok=True)
 
     @lsp_message_listener("client")
     async def shadow_virtual_documents(scope, message, language_server, manager):
@@ -124,6 +120,12 @@ async def shadow_virtual_documents(scope, message, language_server, manager):
         Only create the shadow file if the URI matches the virtual documents URI.
         Returns the path on filesystem where the content was stored.
         """
+        nonlocal initialized
+
+        # short-circut if language server does not require documents on disk
+        server_spec = manager.language_servers[language_server]
+        if not server_spec.get("requires_documents_on_disk", True):
+            return
 
         if not message.get("method") in WRITE_ONE:
             return
@@ -141,6 +143,16 @@ async def shadow_virtual_documents(scope, message, language_server, manager):
         if not uri.startswith(virtual_documents_uri):
             return
 
+        # initialization (/any file system operations) delayed until needed
+        if not initialized:
+            # create if does no exist (so that removal does not raise)
+            shadow_filesystem.mkdir(parents=True, exist_ok=True)
+            # remove with contents
+            rmtree(str(shadow_filesystem))
+            # create again
+            shadow_filesystem.mkdir(parents=True, exist_ok=True)
+            initialized = True
+
         path = file_uri_to_path(uri)
         editable_file = EditableFile(path)