Update rope to 1.11.0 for multi-threading capabilities (#498)

Author: tkrabel

pylsp/plugins/rope_autoimport.py

@@ -295,7 +295,7 @@ def pylsp_code_actions(
         word = get_name_or_module(document, diagnostic)
         log.debug(f"autoimport: searching for word: {word}")
         rope_config = config.settings(document_path=document.path).get("rope", {})
-        autoimport = workspace._rope_autoimport(rope_config, feature="code_actions")
+        autoimport = workspace._rope_autoimport(rope_config)
         suggestions = list(autoimport.search_full(word))
         log.debug("autoimport: suggestions: %s", suggestions)
         results = list(

pylsp/workspace.py

@@ -8,7 +8,7 @@
 import re
 import uuid
 import functools
-from typing import Literal, Optional, Generator, Callable, List
+from typing import Optional, Generator, Callable, List
 from threading import RLock
 
 import jedi
@@ -58,30 +58,20 @@ def __init__(self, root_uri, endpoint, config=None):
         # Whilst incubating, keep rope private
         self.__rope = None
         self.__rope_config = None
-
-        # We have a sperate AutoImport object for each feature to avoid sqlite errors
-        # from accessing the same database from multiple threads.
-        # An upstream fix discussion is here: https://github.com/python-rope/rope/issues/713
-        self.__rope_autoimport = (
-            {}
-        )  # Type: Dict[Literal["completions", "code_actions"], rope.contrib.autoimport.sqlite.AutoImport]
+        self.__rope_autoimport = None
 
     def _rope_autoimport(
         self,
         rope_config: Optional,
         memory: bool = False,
-        feature: Literal["completions", "code_actions"] = "completions",
     ):
         # pylint: disable=import-outside-toplevel
         from rope.contrib.autoimport.sqlite import AutoImport
 
-        if feature not in ["completions", "code_actions"]:
-            raise ValueError(f"Unknown feature {feature}")
-
-        if self.__rope_autoimport.get(feature, None) is None:
+        if self.__rope_autoimport is None:
             project = self._rope_project_builder(rope_config)
-            self.__rope_autoimport[feature] = AutoImport(project, memory=memory)
-        return self.__rope_autoimport[feature]
+            self.__rope_autoimport = AutoImport(project, memory=memory)
+        return self.__rope_autoimport
 
     def _rope_project_builder(self, rope_config):
         # pylint: disable=import-outside-toplevel
@@ -388,8 +378,8 @@ def _create_cell_document(
         )
 
     def close(self):
-        for _, autoimport in self.__rope_autoimport.items():
-            autoimport.close()
+        if self.__rope_autoimport:
+            self.__rope_autoimport.close()
 
 
 class Document:

pyproject.toml

@@ -34,7 +34,7 @@ all = [
     "pydocstyle>=6.3.0,<6.4.0",
     "pyflakes>=3.1.0,<3.2.0",
     "pylint>=2.5.0,<3.1",
-    "rope>1.2.0",
+    "rope>=1.11.0",
     "yapf>=0.33.0",
     "whatthepatch>=1.0.2,<2.0.0"
 ]
@@ -45,7 +45,7 @@ pycodestyle = ["pycodestyle>=2.11.0,<2.12.0"]
 pydocstyle = ["pydocstyle>=6.3.0,<6.4.0"]
 pyflakes = ["pyflakes>=3.1.0,<3.2.0"]
 pylint = ["pylint>=2.5.0,<3.1"]
-rope = ["rope>1.2.0"]
+rope = ["rope>=1.11.0"]
 yapf = ["yapf>=0.33.0", "whatthepatch>=1.0.2,<2.0.0"]
 websockets = ["websockets>=10.3"]
 test = [

test/plugins/test_autoimport.py

@@ -1,13 +1,16 @@
 # Copyright 2022- Python Language Server Contributors.
 
 from typing import Any, Dict, List
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+
+from test.test_notebook_document import wait_for_condition
+from test.test_utils import send_initialize_request, send_notebook_did_open
 
 import jedi
 import parso
 import pytest
 
-from pylsp import lsp, uris
+from pylsp import IS_WIN, lsp, uris
 from pylsp.config.config import Config
 from pylsp.plugins.rope_autoimport import (
     _get_score,
@@ -25,13 +28,18 @@
 DOC_URI = uris.from_fs_path(__file__)
 
 
-def contains_autoimport(suggestion: Dict[str, Any], module: str) -> bool:
-    """Checks if `suggestion` contains an autoimport for `module`."""
+def contains_autoimport_completion(suggestion: Dict[str, Any], module: str) -> bool:
+    """Checks if `suggestion` contains an autoimport completion for `module`."""
     return suggestion.get("label", "") == module and "import" in suggestion.get(
         "detail", ""
     )
 
 
+def contains_autoimport_quickfix(suggestion: Dict[str, Any], module: str) -> bool:
+    """Checks if `suggestion` contains an autoimport quick fix for `module`."""
+    return suggestion.get("title", "") == f"import {module}"
+
+
 @pytest.fixture(scope="session")
 def autoimport_workspace(tmp_path_factory) -> Workspace:
     "Special autoimport workspace. Persists across sessions to make in-memory sqlite3 database fast."
@@ -248,82 +256,89 @@ def test_autoimport_code_actions_get_correct_module_name(autoimport_workspace, m
     assert module_name == "os"
 
 
-# rope autoimport launches a sqlite database which checks from which thread it is called.
-# This makes the test below fail because we access the db from a different thread.
-# See https://stackoverflow.com/questions/48218065/objects-created-in-a-thread-can-only-be-used-in-that-same-thread
-# @pytest.mark.skipif(IS_WIN, reason="Flaky on Windows")
-# def test_autoimport_completions_for_notebook_document(
-#     client_server_pair,
-# ):
-#     client, server = client_server_pair
-#     send_initialize_request(client)
-
-#     with patch.object(server._endpoint, "notify") as mock_notify:
-#         # Expectations:
-#         # 1. We receive an autoimport suggestion for "os" in the first cell because
-#         #    os is imported after that.
-#         # 2. We don't receive an autoimport suggestion for "os" in the second cell because it's
-#         #    already imported in the second cell.
-#         # 3. We don't receive an autoimport suggestion for "os" in the third cell because it's
-#         #    already imported in the second cell.
-#         # 4. We receive an autoimport suggestion for "sys" because it's not already imported
-#         send_notebook_did_open(client, ["os", "import os\nos", "os", "sys"])
-#         wait_for_condition(lambda: mock_notify.call_count >= 3)
-
-#     server.m_workspace__did_change_configuration(
-#         settings={
-#             "pylsp": {
-#                 "plugins": {
-#                     "rope_autoimport": {
-#                         "memory": True,
-#                         "completions": {"enabled": True},
-#                     },
-#                 }
-#             }
-#         }
-#     )
-#     rope_autoimport_settings = server.workspace._config.plugin_settings(
-#         "rope_autoimport"
-#     )
-#     assert rope_autoimport_settings.get("completions", {}).get("enabled", False) is True
-#     assert rope_autoimport_settings.get("memory", False) is True
-
-#     # 1.
-#     suggestions = server.completions("cell_1_uri", {"line": 0, "character": 2}).get(
-#         "items"
-#     )
-#     assert any(
-#         suggestion
-#         for suggestion in suggestions
-#         if contains_autoimport(suggestion, "os")
-#     )
-
-#     # 2.
-#     suggestions = server.completions("cell_2_uri", {"line": 1, "character": 2}).get(
-#         "items"
-#     )
-#     assert not any(
-#         suggestion
-#         for suggestion in suggestions
-#         if contains_autoimport(suggestion, "os")
-#     )
-
-#     # 3.
-#     suggestions = server.completions("cell_3_uri", {"line": 0, "character": 2}).get(
-#         "items"
-#     )
-#     assert not any(
-#         suggestion
-#         for suggestion in suggestions
-#         if contains_autoimport(suggestion, "os")
-#     )
-
-#     # 4.
-#     suggestions = server.completions("cell_4_uri", {"line": 0, "character": 3}).get(
-#         "items"
-#     )
-#     assert any(
-#         suggestion
-#         for suggestion in suggestions
-#         if contains_autoimport(suggestion, "sys")
-#     )
+def make_context(module_name, line, character_start, character_end):
+    return {
+        "diagnostics": [
+            {
+                "message": f"undefined name '{module_name}'",
+                "range": {
+                    "start": {"line": line, "character": character_start},
+                    "end": {"line": line, "character": character_end},
+                },
+            }
+        ]
+    }
+
+
+def position(line, character):
+    return {"line": line, "character": character}
+
+
+@pytest.mark.skipif(IS_WIN, reason="Flaky on Windows")
+def test_autoimport_code_actions_and_completions_for_notebook_document(
+    client_server_pair,
+):
+    client, server = client_server_pair
+    send_initialize_request(
+        client,
+        {
+            "pylsp": {
+                "plugins": {
+                    "rope_autoimport": {
+                        "memory": True,
+                        "enabled": True,
+                        "completions": {"enabled": True},
+                    },
+                }
+            }
+        },
+    )
+
+    with patch.object(server._endpoint, "notify") as mock_notify:
+        # Expectations:
+        # 1. We receive an autoimport suggestion for "os" in the first cell because
+        #    os is imported after that.
+        # 2. We don't receive an autoimport suggestion for "os" in the second cell because it's
+        #    already imported in the second cell.
+        # 3. We don't receive an autoimport suggestion for "os" in the third cell because it's
+        #    already imported in the second cell.
+        # 4. We receive an autoimport suggestion for "sys" because it's not already imported.
+        # 5. If diagnostics doesn't contain "undefined name ...", we send empty quick fix suggestions.
+        send_notebook_did_open(client, ["os", "import os\nos", "os", "sys"])
+        wait_for_condition(lambda: mock_notify.call_count >= 3)
+
+    rope_autoimport_settings = server.workspace._config.plugin_settings(
+        "rope_autoimport"
+    )
+    assert rope_autoimport_settings.get("completions", {}).get("enabled", False) is True
+    assert rope_autoimport_settings.get("memory", False) is True
+
+    # 1.
+    quick_fixes = server.code_actions("cell_1_uri", {}, make_context("os", 0, 0, 2))
+    assert any(s for s in quick_fixes if contains_autoimport_quickfix(s, "os"))
+
+    completions = server.completions("cell_1_uri", position(0, 2)).get("items")
+    assert any(s for s in completions if contains_autoimport_completion(s, "os"))
+
+    # 2.
+    # We don't test code actions here as in this case, there would be no code actions sent bc
+    # there wouldn't be a diagnostics message.
+    completions = server.completions("cell_2_uri", position(1, 2)).get("items")
+    assert not any(s for s in completions if contains_autoimport_completion(s, "os"))
+
+    # 3.
+    # Same as in 2.
+    completions = server.completions("cell_3_uri", position(0, 2)).get("items")
+    assert not any(s for s in completions if contains_autoimport_completion(s, "os"))
+
+    # 4.
+    quick_fixes = server.code_actions("cell_4_uri", {}, make_context("sys", 0, 0, 3))
+    assert any(s for s in quick_fixes if contains_autoimport_quickfix(s, "sys"))
+
+    completions = server.completions("cell_4_uri", position(0, 3)).get("items")
+    assert any(s for s in completions if contains_autoimport_completion(s, "sys"))
+
+    # 5.
+    context = {"diagnostics": [{"message": "A random message"}]}
+    quick_fixes = server.code_actions("cell_4_uri", {}, context)
+    assert len(quick_fixes) == 0