`ruff server`: Add support for documents not exist on disk #11588

T-256 · 2024-05-28T21:43:56Z

Summary

before this PR ruff server at first layer checked that url received from lsp-client most matched file:// and existed on disk, there are some scenarios that urls are not always file://-scheme. After this PR ruff server would also support urls other than file://
For example:

url: untitled:Untitled-1.ipynb?jupyter-notebook

url.scheme: untitled
url.path: Untitled-1.ipynb
url.quey: jupyter-notebook

Closes #11510

Test Plan

Open VSCode
Enable ruff extension's native server option
Ctrl+Win+Alt+N -> select Python File
Try write some codes and verify ruff works

github-actions · 2024-05-28T22:27:52Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ruff_server/src/server/api/notifications/did_change_workspace.rs

MichaReiser

Thanks for working on this. Overall this looks good to me and is exactly what I wanted to do as well.

I think the only part that I planned to add and isn't part of this PR is that we can't use SourceType::from_path for a new file and instead should respect the language setting coming from VS Code. In theory, this also applies for existing files because a user could have mapped all .py files to notebooks in their configuration.

That's why I think we should infer the SourceType in the didOpen request from the item.languageId. Unfortunately, the language id doesn't distinguish between python and python stub files. That's why query.source_type() still needs to look at the path if the source type communicated by VS code is Python.

crates/ruff_server/src/session/index.rs

crates/ruff_server/src/fix.rs

crates/ruff_server/src/lint.rs

crates/ruff_server/src/server.rs

crates/ruff_server/src/session.rs

charliermarsh · 2024-05-29T19:42:40Z

@T-256 - Do you want to address Micha's feedback, or want me to do so?

T-256 · 2024-05-29T21:12:05Z

Do you want to address Micha's feedback, or want me to do so?

I opened this PR as POC that what could be happened. I wanted to get time to test it yesterday, but unfortunately it didn't happen.
Good news! I tested now and worked as expected with new nativeServer enablement on VSCode and opening untitled file.
I'll update PR description accordingly.

@MichaReiser Thanks for review, few comments still unsolved, I'd be happy to Charlie help me solve them.

MichaReiser · 2024-05-30T09:24:10Z

Thank you, @T-256 for all the work. I'll take a look at addressing the remaining review comments.

…s maps actual paths * Use `Url.to_file_path` as we used to before switching to PathBuf in the notebook PR * Rename some `*path` variables to `*url` * Change `take_snapshot` to take an owned `Url` because it always clones the URL internally * More smaller fixes

MichaReiser · 2024-05-30T14:39:09Z

crates/ruff_server/src/session/index.rs

+            // If there's only a single workspace, use that configuration for an untitled document.
+            if self.settings.len() == 1 {
+                tracing::debug!(
+                    "Falling back to configuration of the only active workspace for the new document '{url}'."
+                );
+                self.settings.values().next()
+            } else {
+                None
+            }


I'm not entirely sure about this. It's nice because the file is most likely to be part of the workspace and it would be unfortunate if we show lint errors for rules that are disabled in this workspace.

However, it has the downside that the shown lint errors change when the user has an unsaved document and then opens a new workspace. A next pull diagnostic now uses the default configuration which is different from the last run. I think that's fine, considering that adding a workspace AND having an unsaved file is rare.

MichaReiser · 2024-05-30T14:39:46Z

crates/ruff_server/src/session/index.rs

+    /// Get the path for the document selected by this query.
+    ///
+    /// Returns `None` if this is an unsaved (untitled) document.
+    ///
+    /// The path isn't guaranteed to point to a real path on the filesystem. This is the case
+    /// for unsaved (untitled) documents.
+    pub(crate) fn file_path(&self) -> Option<PathBuf> {
+        self.file_url().to_file_path().ok()
+    }
+
+    /// Get the path for the document selected by this query, ignoring whether the file exists on disk.
+    ///
+    /// Returns the URL's path if this is an unsaved (untitled) document.
+    pub(crate) fn virtual_file_path(&self) -> &Path {
+        Path::new(self.file_url().path())
+    }
+


I think it's worth to distinguish between the two. E.g., I think we shouldn't test for exclusion for unsaved files because we don't have a real path.

I am wondering if we should make Path optional for the Ruff APIs like lint_fix...?

Like, could we remove this entirely? Why is the path needed in such cases? (If it's just for the extension, we could implement something special in source_type.)

It's for diagnostics and maybe file based lint rules?

We might get away with it but I'm not very eager to change it. Like it would probably be an improvement but I rather finish res knot where this will be gone anyway 😅

T-256

LGTM, Thanks for done this.
Just for last question, shouldn't we explicitly specify what URL schemes we support? AFAIK and according to docs of LSP specification it should be file or untitled. but URL itself could contain other schemes, IMO we should disallow other schemes. what do you think?

MichaReiser · 2024-05-30T15:09:47Z

crates/ruff_server/src/session/index.rs

+                    // For a new unsaved and untitled document, use the ruff settings from the top of the workspace
+                    // but only IF:
+                    // * It is the only workspace
+                    // * The ruff setting is at the top of the workspace (in the root folder)
+                    // Otherwise, use the fallback settings.
+                    if self.settings.len() == 1 {
+                        let workspace_path = self.settings.keys().next().unwrap();
+                        settings.ruff_settings.get(workspace_path)
+                    } else {
+                        settings.ruff_settings.fallback()
+                    }


I think users generally want this (take the settings from the root configuration in the only active workspace). But it does have the downside that adding a second workspace later on means that we suddenly take the fallback which changes the output. But I think that's fine.

I think there are such cases in vscode internals too. IIRC they solved by choosing first workspace was added. can we consider same at here?

Possibly. We would need to keep a second vector that stores the workspace URLs in insertion order. I would prefer to leave this out of this PR for now.

MichaReiser · 2024-05-30T16:06:03Z

I tested:

Creating an untitled file
- In a single folder workspace: It used the configuration at the root of the folder if present, and otherwise used the fallback configuration
- In a multi-folder workspace: It used the fallback configuration
- The file doesn't get excluded
I tested that, for an existing file
- It uses the closest configuration
- It doesn't get formatted when it is excluded
It formats and lints an untitled notebook document

I didn't change the code to use the language from the didOpen notification. I think we can wait with using that until we support more languages than Python

MichaReiser · 2024-05-30T16:07:38Z

crates/ruff_server/src/fix.rs

-        );
-        return Ok(Fixes::default());
-    }
+    let package = if let Some(document_path) = document_path.as_ref() {


The way I implemented it here is that an untitled file can never be excluded. However, that means that an exclude = ["*"] won't apply where users might expect that it should.

But other than that, I think not handling it in exclusion makes sense because we don't know in which folder the document will be stored.

Makes sense.

T-256 force-pushed the textDocument/didOpen branch 3 times, most recently from 955f9d5 to cbd5a84 Compare May 28, 2024 22:12

Add support for documents not exist on disk

abd95b4

T-256 force-pushed the textDocument/didOpen branch from cbd5a84 to abd95b4 Compare May 28, 2024 22:14

MichaReiser reviewed May 29, 2024

View reviewed changes

crates/ruff_server/src/server/api/notifications/did_change_workspace.rs Outdated Show resolved Hide resolved

MichaReiser reviewed May 29, 2024

View reviewed changes

T-256 added 2 commits May 30, 2024 00:14

Thanks to Micha for review

81858bb

resolve conflicts

0ce22f5

T-256 marked this pull request as ready for review May 29, 2024 21:12

MichaReiser added the server Related to the LSP server label May 30, 2024

MichaReiser reviewed May 30, 2024

View reviewed changes

T-256 commented May 30, 2024

View reviewed changes

MichaReiser reviewed May 30, 2024

View reviewed changes

Adopt configuration fallback logic

edeaae6

MichaReiser force-pushed the textDocument/didOpen branch from c5b4ea4 to edeaae6 Compare May 30, 2024 15:58

MichaReiser reviewed May 30, 2024

View reviewed changes

MichaReiser requested review from charliermarsh and dhruvmanila and removed request for dhruvmanila May 30, 2024 16:08

Clippy

aef8ce7

charliermarsh approved these changes May 30, 2024

View reviewed changes

MichaReiser merged commit 5b500fc into astral-sh:main May 31, 2024
19 checks passed

MichaReiser mentioned this pull request May 31, 2024

Ruff Native Server spams Ruff encountered a problem. Check the logs for more details. with bad symbols. astral-sh/ruff-vscode#482

Closed

BrewTestBot mentioned this pull request May 31, 2024

ruff 0.4.7 Homebrew/homebrew-core#173374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ruff server`: Add support for documents not exist on disk #11588

`ruff server`: Add support for documents not exist on disk #11588

T-256 commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

MichaReiser left a comment

charliermarsh commented May 29, 2024

T-256 commented May 29, 2024

MichaReiser commented May 30, 2024

MichaReiser May 30, 2024

MichaReiser May 30, 2024

charliermarsh May 30, 2024

charliermarsh May 30, 2024

MichaReiser May 30, 2024

T-256 left a comment

MichaReiser May 30, 2024

T-256 May 30, 2024

MichaReiser May 30, 2024

MichaReiser commented May 30, 2024

MichaReiser May 30, 2024

charliermarsh May 30, 2024

ruff server: Add support for documents not exist on disk #11588

ruff server: Add support for documents not exist on disk #11588

Conversation

T-256 commented May 28, 2024 • edited Loading

Summary

Test Plan

github-actions bot commented May 28, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

MichaReiser left a comment

Choose a reason for hiding this comment

charliermarsh commented May 29, 2024

T-256 commented May 29, 2024

MichaReiser commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T-256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`ruff server`: Add support for documents not exist on disk #11588

`ruff server`: Add support for documents not exist on disk #11588

T-256 commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

`ruff-ecosystem` results