Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
theelderbeever opened this issue May 29, 2024 · 17 comments · Fixed by astral-sh/ruff#11745
Assignees
Labels
bug Something isn't working server

Comments

@theelderbeever
Copy link

theelderbeever commented May 29, 2024

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
Copy link
Member

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
Copy link
Author

theelderbeever commented May 29, 2024

@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
Copy link
Author

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

@MichaReiser
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

MichaReiser commented May 31, 2024

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

@MichaReiser MichaReiser added bug Something isn't working server labels May 31, 2024
@theelderbeever
Copy link
Author

@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
Copy link

hcrosse commented May 31, 2024

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
Copy link
Member

@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 astral-sh/ruff#11588

@MichaReiser
Copy link
Member

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(

@snowsignal snowsignal self-assigned this Jun 4, 2024
@BabakAmini
Copy link

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 added a commit to astral-sh/ruff that referenced this issue Jun 5, 2024
…pams a visible error popup (#11745)

## Summary

Fixes astral-sh/ruff-vscode#482.

I've made adjustments to `format` and `format_range` that handle parsing
errors before they become server errors. We'll still log this as a
problem, but there will no longer be a visible popup.

## Test Plan

Instead of seeing a visible error when formatting a document with syntax
issues, you should see this warning in the LSP logs:

<img width="991" alt="Screenshot 2024-06-04 at 3 38 23 PM"
src="https://github.com/astral-sh/ruff/assets/19577865/9d68947d-6462-4ca6-ab5a-65e573c91db6">

Similarly, if you try to format a range with syntax issues, you should
see this warning in the LSP logs instead of a visible error popup:

<img width="1010" alt="Screenshot 2024-06-04 at 3 39 10 PM"
src="https://github.com/astral-sh/ruff/assets/19577865/99fff098-798d-406a-976e-81ead0da0352">

---------

Co-authored-by: Zanie Blue <contact@zanie.dev>
@snowsignal
Copy link
Contributor

This should be fixed in ruff v0.4.8 😄

@BabakAmini
Copy link

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

image

@dhruvmanila
Copy link
Member

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

@BabakAmini
Copy link

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
Copy link
Member

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
6 participants