astral-sh / ruff-vscode

A Visual Studio Code extension with support for the Ruff linter.
Other
945 stars 45 forks source link

Ruff Native Server spams `Ruff encountered a problem. Check the logs for more details.` with bad symbols. #482

Closed theelderbeever closed 4 weeks ago

theelderbeever commented 1 month ago

This might not be an actual bug however, it is somewhat annoying.... If you use the new native lsp in ruff with vscode and have a bad symbol in your file and save you will get an error pop up in the lower right corner of VSCode for the extension.

image

A simple test

# hello.py
print(some_var) # some_var hasn't been set yet.

Now save the file. Every save triggers the pop up.

While it isn't technically wrong that there is an error its a little overkill to be told to check the extension logs for the error when the typical red underline sufficiently captures the code error.

MichaReiser commented 1 month ago

Can you tell us a bit more about your setup? What version of the extension and ruff are you using? Do you use the new experimental lsp backend?

theelderbeever commented 1 month ago

@MichaReiser Yeah sorry about the lack of details. Its the new experimental backend on v0.4.5.

Pertinent settings in VSCode. Extension version v2024.22.0

{
    "ruff.nativeServer": true
    "notebook.formatOnSave.enabled": true,
    "ruff.lint.args": ["--config=./pyproject.toml"],
    "notebook.codeActionsOnSave": {
        "notebook.source.fixAll": false,
        "notebook.source.organizeImports": "explicit"
      },
    "[python]": {
        "editor.formatOnSave": true,
        "editor.defaultFormatter": "charliermarsh.ruff",
        "editor.codeActionsOnSave": {
          "source.fixAll": "never",
          "source.organizeImports": "never"
        }
    },
}

pyproject.toml


[tool.ruff]
# Exclude a variety of commonly ignored directories.
exclude = [
    ".bzr",
    ".direnv",
    ".eggs",
    ".git",
    ".git-rewrite",
    ".hg",
    ".ipynb_checkpoints",
    ".mypy_cache",
    ".nox",
    ".pants.d",
    ".pyenv",
    ".pytest_cache",
    ".pytype",
    ".ruff_cache",
    ".svn",
    ".tox",
    ".venv",
    ".vscode",
    "__pypackages__",
    "_build",
    "buck-out",
    "build",
    "dist",
    "node_modules",
    "site-packages",
    "venv",
]

# Same as Black.
line-length = 88
indent-width = 4

# Assume Python 3.11
target-version = "py311"

[tool.ruff.lint]
# Enable Pyflakes (`F`) and a subset of the pycodestyle (`E`)  codes by default.
select = ["E4", "E7", "E9", "F"]
ignore = ["E402"]

ignore-init-module-imports = true

# Allow fix for all enabled rules (when `--fix`) is provided.
fixable = ["ALL"]
unfixable = []

# Allow unused variables when underscore-prefixed.
dummy-variable-rgx = "(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$"

[tool.ruff.format]
# Like Black, use double quotes for strings.
quote-style = "double"

# Like Black, indent with spaces, rather than tabs.
indent-style = "space"

# Like Black, respect magic trailing commas.
skip-magic-trailing-comma = false

# Like Black, automatically detect the appropriate line ending.
line-ending = "auto"
theelderbeever commented 1 month ago

Turning off the native server "ruff.nativeServer": false suppresses the error modal.

MichaReiser commented 1 month ago

Thanks for sharing and sorry that I didn't ask before. Would you mind to also share the logs from the "Output" tab, then select "Ruff" in the drop down.

image

theelderbeever commented 1 month ago

@MichaReiser Okay so I just realized that the example I gave I hadn't tested and sort of assumed it would work... The following example DOES elicit the issue though and has the attached logs... So maybe it is specific to keywords.

# hello.py
def
2024-05-31 08:28:11.160 [info]  201.579174s ERROR ruff_server::server::api An error occurred with result ID 287: Expected an identifier at byte range 3..3

2024-05-31 08:28:11.161 [info] [Error - 8:28:11 AM] Request textDocument/formatting failed.
2024-05-31 08:28:11.161 [info]   Message: Expected an identifier at byte range 3..3
  Code: -32603 
MichaReiser commented 1 month ago

Oh that's annoying. So Ruff gives an error everytime you try to format a file with a syntax error. Arguably, showing an error here is correct because Ruff didn't format the file but it should at least be specific about what failed rather than showing a generic error message. CC: @snowsignal

theelderbeever commented 1 month ago

@MichaReiser Yep totally agree on the correctness of displaying an error however, in its current form it comes across as obnoxious due to being a generic error. I think it would really valuable if the specific error could be displayed in the modal pop up instead of sending the user to the logs. Less clicking around and all that...

hcrosse commented 1 month ago

This also triggers on every character press if you have "ruff.lint.run": "onType", so displaying a more verbose pop up would be an improvement but it would still be very spammy.

MichaReiser commented 1 month ago

@hcrosse I'm unable to reproduce this behavior with onType, except if the document is a new, unsaved (untitled) file. The handling of unsaved files should be fixed in https://github.com/astral-sh/ruff/pull/11588

MichaReiser commented 1 month ago

Okay, this is a bit more complicated to fix than i thought and we may have to wait for @snowsignal to get back.

What I tried is to change the format handlers to not return an InternalError but instead a RequestFailed error code when formatting fails because of a parse error. However, we still end up showing the dialog because the dialog handling is in the api module where the Error is just a DeserializeOwned + Serialize + Send + Sync + 'static and nothing we can match on.

I think ideally we would only show the popup for a selected set of errors or at least take the error message from the Err object.

Index: crates/ruff_server/src/server/api/requests/format_range.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_server/src/server/api/requests/format_range.rs b/crates/ruff_server/src/server/api/requests/format_range.rs
--- a/crates/ruff_server/src/server/api/requests/format_range.rs    (revision 8a25531a7144fd4a6b62c54efde1ef28e2dc18c4)
+++ b/crates/ruff_server/src/server/api/requests/format_range.rs    (date 1717185102879)
@@ -3,7 +3,7 @@
 use ruff_workspace::resolver::match_any_exclusion;

 use crate::edit::{RangeExt, ToRangeExt};
-use crate::server::api::LSPResult;
+use crate::server::api::request::format::format_result_to_server_result;
 use crate::server::{client::Notifier, Result};
 use crate::session::{DocumentQuery, DocumentSnapshot};
 use crate::{PositionEncoding, TextDocument};
@@ -65,13 +65,12 @@
     let text = text_document.contents();
     let index = text_document.index();
     let range = range.to_text_range(text, index, encoding);
-    let formatted_range = crate::format::format_range(
+    let formatted_range = format_result_to_server_result(crate::format::format_range(
         text_document,
         query.source_type(),
         formatter_settings,
         range,
-    )
-    .with_failure_code(lsp_server::ErrorCode::InternalError)?;
+    ))?;

     Ok(Some(vec![types::TextEdit {
         range: formatted_range
Index: crates/ruff_server/src/server/client.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_server/src/server/client.rs b/crates/ruff_server/src/server/client.rs
--- a/crates/ruff_server/src/server/client.rs   (revision 8a25531a7144fd4a6b62c54efde1ef28e2dc18c4)
+++ b/crates/ruff_server/src/server/client.rs   (date 1717185111886)
@@ -48,7 +48,6 @@
     }
 }

-#[allow(dead_code)] // we'll need to use `Notifier` in the future
 impl Notifier {
     pub(crate) fn notify<N>(&self, params: N::Params) -> crate::Result<()>
     where
@@ -61,6 +60,7 @@
         self.0.send(message)
     }

+    #[allow(unused)]
     pub(crate) fn notify_method(&self, method: String) -> crate::Result<()> {
         self.0
             .send(lsp_server::Message::Notification(Notification::new(
Index: crates/ruff_server/src/server/api/requests/format.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_server/src/server/api/requests/format.rs b/crates/ruff_server/src/server/api/requests/format.rs
--- a/crates/ruff_server/src/server/api/requests/format.rs  (revision 8a25531a7144fd4a6b62c54efde1ef28e2dc18c4)
+++ b/crates/ruff_server/src/server/api/requests/format.rs  (date 1717185149949)
@@ -1,6 +1,7 @@
 use lsp_types::{self as types, request as req};
 use types::TextEdit;

+use ruff_python_formatter::FormatModuleError;
 use ruff_source_file::LineIndex;
 use ruff_workspace::resolver::match_any_exclusion;

@@ -98,9 +99,11 @@
     }

     let source = text_document.contents();
-    let mut formatted =
-        crate::format::format(text_document, query.source_type(), formatter_settings)
-            .with_failure_code(lsp_server::ErrorCode::InternalError)?;
+    let mut formatted = format_result_to_server_result(crate::format::format(
+        text_document,
+        query.source_type(),
+        formatter_settings,
+    ))?;
     // fast path - if the code is the same, return early
     if formatted == source {
         return Ok(None);
@@ -140,3 +143,16 @@
         new_text: formatted[formatted_range].to_owned(),
     }]))
 }
+
+pub(super) fn format_result_to_server_result<T>(
+    result: std::result::Result<T, FormatModuleError>,
+) -> Result<T> {
+    match result {
+        Ok(value) => Ok(value),
+        Err(FormatModuleError::ParseError(error)) => Err(anyhow::anyhow!(
+            "Failed to format document due to a syntax error: {error}"
+        ))
+        .with_failure_code(lsp_server::ErrorCode::RequestFailed),
+        Err(err) => Err(err).with_failure_code(lsp_server::ErrorCode::InternalError),
+    }
+}
Index: crates/ruff_server/src/format.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_server/src/format.rs b/crates/ruff_server/src/format.rs
--- a/crates/ruff_server/src/format.rs  (revision 8a25531a7144fd4a6b62c54efde1ef28e2dc18c4)
+++ b/crates/ruff_server/src/format.rs  (date 1717184800042)
@@ -1,6 +1,6 @@
 use ruff_formatter::PrintedRange;
 use ruff_python_ast::PySourceType;
-use ruff_python_formatter::format_module_source;
+use ruff_python_formatter::{format_module_source, FormatModuleError};
 use ruff_text_size::TextRange;
 use ruff_workspace::FormatterSettings;

@@ -10,7 +10,7 @@
     document: &TextDocument,
     source_type: PySourceType,
     formatter_settings: &FormatterSettings,
-) -> crate::Result<String> {
+) -> Result<String, FormatModuleError> {
     let format_options = formatter_settings.to_format_options(source_type, document.contents());
     let formatted = format_module_source(document.contents(), format_options)?;
     Ok(formatted.into_code())
@@ -21,7 +21,7 @@
     source_type: PySourceType,
     formatter_settings: &FormatterSettings,
     range: TextRange,
-) -> crate::Result<PrintedRange> {
+) -> Result<PrintedRange, FormatModuleError> {
     let format_options = formatter_settings.to_format_options(source_type, document.contents());

     Ok(ruff_python_formatter::format_range(
BabakAmini commented 4 weeks ago

I had the same issue since a week ago. It can be resolved by setting ruff.nativeServer to false, which is obviously not what the extension's creator intended. To make it function, I had to downgrade to 2024.20.0.

snowsignal commented 4 weeks ago

This should be fixed in ruff v0.4.8 😄

BabakAmini commented 4 weeks ago

I updated to 2024.26.0, which stated that the issue had been fixed, however the problem persists:

image

dhruvmanila commented 4 weeks ago

@BabakAmini can you open a new issue with relevant details?

BabakAmini commented 4 weeks ago

I had the same issue since a week ago. It can be resolved by setting ruff.nativeServer to false, which is obviously not what the extension's creator intended. To make it function, I had to downgrade to 2024.20.0.

@dhruvmanila As I mentioned before, my issue is related to nativeServer. If it's necessary, I will open a new issue.

MichaReiser commented 4 weeks ago

@BabakAmini your issue looks somewhat different. Ruff is actually crashing. It's nit just requests failing. That's why it's best to open a new issue where you include the ruff version, extension version, and the log output

BabakAmini commented 4 weeks ago

I discovered that the Ruff version I have installed on my system is outdated. It appears that updating it fixes my problem.