Repository: python-lsp/python-lsp-server (python)

Issue:
Flake8 Severity too high
After https://github.com/python-lsp/python-lsp-server/pull/234, all Flake8 codes starting with "F" are reported with error severity. This is overly high: the only codes that really errors are F821, F823, F831, and _maybe_ F405 and F822.

Can we add explicit checks for error codes to classify them as errors vs. warnings?

Pull request:
Refine diagnostic severity for flake8
Previously all diagnostics that starts with "F" (generated from pyflakes) are reported as errors, which could be too high. As such, in this commit we change to report only certain pyflakes diagnostics as errors and all the others as warnings. The current list of error codes is in sync with the `pyflakes_lint` plugin.

While it is possible to generate such a list (or more precisely, a `set`) during runtime (see the snippet in the comment), that might be less ideal because:

  - Flake8 does not guarantee a stable API.
  - We run `flake8` as an executable and parse its outputs, so using the `flake8` module in Python may bring inconsistency (e.g., the user install Flake8 in a different virtual environment from pylsp).
  - Doing so couples two plugins (flake8 and pyflakes) together.

So it seems better to maintain such a list of error codes manually.

Fix #256.

Patch:
diff --git a/pylsp/plugins/flake8_lint.py b/pylsp/plugins/flake8_lint.py
index 8d8d4c5..77aa22b 100644
--- a/pylsp/plugins/flake8_lint.py
+++ b/pylsp/plugins/flake8_lint.py
@@ -9,9 +9,13 @@ import sys
 from pathlib import PurePath
 from subprocess import PIPE, Popen
 
+from flake8.plugins.pyflakes import FLAKE8_PYFLAKES_CODES
+
 from pylsp import hookimpl, lsp
+from pylsp.plugins.pyflakes_lint import PYFLAKES_ERROR_MESSAGES
 
 log = logging.getLogger(__name__)
+
 FIX_IGNORES_RE = re.compile(r"([^a-zA-Z0-9_,]*;.*(\W+||$))")
 UNNECESSITY_CODES = {
     "F401",  # `module` imported but unused
@@ -20,6 +24,14 @@ UNNECESSITY_CODES = {
     "F523",  # .format(...) unused positional arguments
     "F841",  # local variable `name` is assigned to but never used
 }
+# NOTE: If the user sets the flake8 executable with workspace configuration, the
+# error codes in this set may be inaccurate.
+ERROR_CODES = (
+    # Errors from the pyflakes plugin of flake8
+    {FLAKE8_PYFLAKES_CODES.get(m.__name__, "E999") for m in PYFLAKES_ERROR_MESSAGES}
+    # Syntax error from flake8 itself
+    | {"E999"}
+)
 
 
 @hookimpl
@@ -208,7 +220,7 @@ def parse_stdout(source, stdout):
         # show also the code in message
         msg = code + " " + msg
         severity = lsp.DiagnosticSeverity.Warning
-        if code == "E999" or code[0] == "F":
+        if code in ERROR_CODES:
             severity = lsp.DiagnosticSeverity.Error
         diagnostic = {
             "source": "flake8",
diff --git a/pylsp/plugins/jedi_completion.py b/pylsp/plugins/jedi_completion.py
index 2ecf0be..65f7523 100644
--- a/pylsp/plugins/jedi_completion.py
+++ b/pylsp/plugins/jedi_completion.py
@@ -88,6 +88,7 @@ def pylsp_completions(config, document, position):
             include_params=include_params if c.type in ["class", "function"] else False,
             resolve=resolve_eagerly,
             resolve_label_or_snippet=(i < max_to_resolve),
+            snippet_support=snippet_support,
         )
         for i, c in enumerate(completions)
     ]
@@ -102,6 +103,7 @@ def pylsp_completions(config, document, position):
                     include_params=False,
                     resolve=resolve_eagerly,
                     resolve_label_or_snippet=(i < max_to_resolve),
+                    snippet_support=snippet_support,
                 )
                 completion_dict["kind"] = lsp.CompletionItemKind.TypeParameter
                 completion_dict["label"] += " object"
@@ -116,6 +118,7 @@ def pylsp_completions(config, document, position):
                     include_params=False,
                     resolve=resolve_eagerly,
                     resolve_label_or_snippet=(i < max_to_resolve),
+                    snippet_support=snippet_support,
                 )
                 completion_dict["kind"] = lsp.CompletionItemKind.TypeParameter
                 completion_dict["label"] += " object"
@@ -226,6 +229,7 @@ def _format_completion(
     include_params=True,
     resolve=False,
     resolve_label_or_snippet=False,
+    snippet_support=False,
 ):
     completion = {
         "label": _label(d, resolve_label_or_snippet),
@@ -240,16 +244,20 @@ def _format_completion(
     # Adjustments for file completions
     if d.type == "path":
         path = os.path.normpath(d.name)
-        path = path.replace("\\", "\\\\")
-        path = path.replace("/", "\\/")
 
-        # If the completion ends with os.sep, it means it's a directory. So we add an escaped os.sep
-        # at the end to ease additional file completions.
+        # If the completion ends with os.sep, it means it's a directory. So we add os.sep at the end
+        # to ease additional file completions.
         if d.name.endswith(os.sep):
             if os.name == "nt":
-                path = path + "\\\\"
+                path = path + "\\"
             else:
-                path = path + "\\/"
+                path = path + "/"
+
+        # Escape to prevent conflicts with the code snippets grammer
+        # See also https://github.com/python-lsp/python-lsp-server/issues/373
+        if snippet_support:
+            path = path.replace("\\", "\\\\")
+            path = path.replace("/", "\\/")
 
         completion["insertText"] = path
 
diff --git a/pylsp/plugins/rope_autoimport.py b/pylsp/plugins/rope_autoimport.py
index ca3db1c..ebcc707 100644
--- a/pylsp/plugins/rope_autoimport.py
+++ b/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(
diff --git a/pylsp/python_lsp.py b/pylsp/python_lsp.py
index 52a22a3..a31e761 100644
--- a/pylsp/python_lsp.py
+++ b/pylsp/python_lsp.py
@@ -687,7 +687,36 @@ class PythonLSPServer(MethodDispatcher):
     def m_text_document__code_lens(self, textDocument=None, **_kwargs):
         return self.code_lens(textDocument["uri"])
 
+    def _cell_document__completion(self, cellDocument, position=None, **_kwargs):
+        workspace = self._match_uri_to_workspace(cellDocument.notebook_uri)
+        notebookDocument = workspace.get_maybe_document(cellDocument.notebook_uri)
+        if notebookDocument is None:
+            raise ValueError("Invalid notebook document")
+
+        cell_data = notebookDocument.cell_data()
+
+        # Concatenate all cells to be a single temporary document
+        total_source = "\n".join(data["source"] for data in cell_data.values())
+        with workspace.temp_document(total_source) as temp_uri:
+            # update position to be the position in the temp document
+            if position is not None:
+                position["line"] += cell_data[cellDocument.uri]["line_start"]
+
+            completions = self.completions(temp_uri, position)
+
+            # Translate temp_uri locations to cell document locations
+            for item in completions.get("items", []):
+                if item.get("data", {}).get("doc_uri") == temp_uri:
+                    item["data"]["doc_uri"] = cellDocument.uri
+
+            return completions
+
     def m_text_document__completion(self, textDocument=None, position=None, **_kwargs):
+        # textDocument here is just a dict with a uri
+        workspace = self._match_uri_to_workspace(textDocument["uri"])
+        document = workspace.get_document(textDocument["uri"])
+        if isinstance(document, Cell):
+            return self._cell_document__completion(document, position, **_kwargs)
         return self.completions(textDocument["uri"], position)
 
     def _cell_document__definition(self, cellDocument, position=None, **_kwargs):
diff --git a/pylsp/workspace.py b/pylsp/workspace.py
index 5c6880c..5e8e548 100644
--- a/pylsp/workspace.py
+++ b/pylsp/workspace.py
@@ -8,7 +8,7 @@ import os
 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 @@ class Workspace:
         # 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 @@ class Workspace:
         )
 
     def close(self):
-        for _, autoimport in self.__rope_autoimport.items():
-            autoimport.close()
+        if self.__rope_autoimport:
+            self.__rope_autoimport.close()
 
 
 class Document:
diff --git a/pyproject.toml b/pyproject.toml
index 1756952..be764e4 100644
--- a/pyproject.toml
+++ b/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 patch:
diff --git a/test/plugins/test_autoimport.py b/test/plugins/test_autoimport.py
index ec5c0a3..9f02965 100644
--- a/test/plugins/test_autoimport.py
+++ b/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 @@ from pylsp.workspace import Workspace
 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
diff --git a/test/plugins/test_completion.py b/test/plugins/test_completion.py
index 00a54eb..f64e81e 100644
--- a/test/plugins/test_completion.py
+++ b/test/plugins/test_completion.py
@@ -583,9 +583,18 @@ def test_file_completions(workspace, tmpdir):
     # Check completions
     assert len(completions) == 2
     assert [c["kind"] == lsp.CompletionItemKind.File for c in completions]
-    assert (
-        completions[0]["insertText"] == ("bar" + "\\\\")
-        if os.name == "nt"
-        else ("bar" + "\\/")
+    assert completions[0]["insertText"] == (
+        ("bar" + "\\") if os.name == "nt" else ("bar" + "/")
+    )
+    assert completions[1]["insertText"] == 'foo.txt"'
+
+    # When snippets are supported, ensure that path separators are escaped.
+    support_snippet = {
+        "textDocument": {"completion": {"completionItem": {"snippetSupport": True}}}
+    }
+    doc._config.capabilities.update(support_snippet)
+    completions = pylsp_jedi_completions(doc._config, doc, com_position)
+    assert completions[0]["insertText"] == (
+        ("bar" + "\\\\") if os.name == "nt" else ("bar" + "\\/")
     )
     assert completions[1]["insertText"] == 'foo.txt"'
diff --git a/test/plugins/test_flake8_lint.py b/test/plugins/test_flake8_lint.py
index 882bc99..c2d711e 100644
--- a/test/plugins/test_flake8_lint.py
+++ b/test/plugins/test_flake8_lint.py
@@ -40,7 +40,7 @@ def test_flake8_unsaved(workspace):
     assert unused_var["code"] == "F841"
     assert unused_var["range"]["start"] == {"line": 5, "character": 1}
     assert unused_var["range"]["end"] == {"line": 5, "character": 11}
-    assert unused_var["severity"] == lsp.DiagnosticSeverity.Error
+    assert unused_var["severity"] == lsp.DiagnosticSeverity.Warning
     assert unused_var["tags"] == [lsp.DiagnosticTag.Unnecessary]
 
 
@@ -55,7 +55,7 @@ def test_flake8_lint(workspace):
         assert unused_var["code"] == "F841"
         assert unused_var["range"]["start"] == {"line": 5, "character": 1}
         assert unused_var["range"]["end"] == {"line": 5, "character": 11}
-        assert unused_var["severity"] == lsp.DiagnosticSeverity.Error
+        assert unused_var["severity"] == lsp.DiagnosticSeverity.Warning
     finally:
         os.remove(name)
 
@@ -101,7 +101,7 @@ def test_flake8_respecting_configuration(workspace):
                 "end": {"line": 5, "character": 11},
             },
             "message": "F841 local variable 'a' is assigned to but never used",
-            "severity": 1,
+            "severity": 2,
             "tags": [1],
         },
     ]
@@ -116,7 +116,7 @@ def test_flake8_respecting_configuration(workspace):
                 "end": {"line": 0, "character": 9},
             },
             "message": "F401 'os' imported but unused",
-            "severity": 1,
+            "severity": 2,
             "tags": [1],
         }
     ]
diff --git a/test/test_notebook_document.py b/test/test_notebook_document.py
index c63d279..f97088b 100644
--- a/test/test_notebook_document.py
+++ b/test/test_notebook_document.py
@@ -488,3 +488,46 @@ def test_notebook_definition(client_server_pair):
             },
         }
     ]
+
+
+@pytest.mark.skipif(IS_WIN, reason="Flaky on Windows")
+def test_notebook_completion(client_server_pair):
+    """
+    Tests that completions work across cell boundaries for notebook document support
+    """
+    client, server = client_server_pair
+    send_initialize_request(client)
+
+    # Open notebook
+    with patch.object(server._endpoint, "notify") as mock_notify:
+        send_notebook_did_open(
+            client, ["answer_to_life_universe_everything = 42", "answer_"]
+        )
+        # wait for expected diagnostics messages
+        wait_for_condition(lambda: mock_notify.call_count >= 2)
+        assert len(server.workspace.documents) == 3
+        for uri in ["cell_1_uri", "cell_2_uri", "notebook_uri"]:
+            assert uri in server.workspace.documents
+
+    future = client._endpoint.request(
+        "textDocument/completion",
+        {
+            "textDocument": {
+                "uri": "cell_2_uri",
+            },
+            "position": {"line": 0, "character": 7},
+        },
+    )
+    result = future.result(CALL_TIMEOUT_IN_SECONDS)
+    assert result == {
+        "isIncomplete": False,
+        "items": [
+            {
+                "data": {"doc_uri": "cell_2_uri"},
+                "insertText": "answer_to_life_universe_everything",
+                "kind": 6,
+                "label": "answer_to_life_universe_everything",
+                "sortText": "aanswer_to_life_universe_everything",
+            },
+        ],
+    }