[red-knot] Support files outside of any workspace by dhruvmanila · Pull Request #13041 · astral-sh/ruff

dhruvmanila · 2024-08-22T06:56:57Z

Summary

This PR adds basic support for files outside of any workspace in the red knot server.

This also limits the red knot server to only work in a single workspace. The server will not start if there are multiple workspaces.

Test Plan

Screen.Recording.2024-08-22.at.16.26.18.mov

dhruvmanila · 2024-08-22T09:44:28Z

crates/red_knot_server/src/server/api.rs

+        let db = match session.workspace_db_for_path(path.as_std_path()) {
+            Some(db) => db.snapshot(),
+            None => session.default_workspace_db().snapshot(),
+        };


I want this entire logic in the Session but the borrow checker always complains when I do something like:

self.workspaces .range_mut(..=path.as_ref().to_path_buf()) .next_back() .map(|(_, db)| db) .unwrap_or_else(|| self.default_workspace_db_mut())

There are two mutable references:

error[E0500]: closure requires unique access to `*self` but it is already borrowed --> crates/red_knot_server/src/session.rs:104:29 | 97 | &mut self, | - let's call the lifetime of this reference `'1` ... 100 | self.workspaces | --------------- | | | _________borrow occurs here | | 101 | | .range_mut(..=path.as_ref().to_path_buf()) 102 | | .next_back() 103 | | .map(|(_, db)| db) 104 | | .unwrap_or_else(|| self.default_workspace_db_mut()) | |_____________________________^^_----___________________________- returning this value requires that `self.workspaces` is borrowed for `'1` | | | | | second borrow occurs due to use of `*self` in closure | closure construction occurs here For more information about this error, try `rustc --explain E0500`.

This seems to work

pub(crate) fn workspace_db_for_path_or_default(&self, path: impl AsRef<Path>) -> &RootDatabase { self.workspace_db_for_path(path) .unwrap_or_else(|| self.default_workspace_db()) }

This also compiles fine for me...

pub(crate) fn workspace_db_for_path(&self, path: impl AsRef<Path>) -> &RootDatabase { self.workspaces .range(..=path.as_ref().to_path_buf()) .next_back() .map(|(_, db)| db) .unwrap_or_else(|| self.default_workspace_db()) }

Not sure if I'm doing something wrong. But I think you face the problem with workspace_db_for_path_mut?

But yeah, not sure why the mut case doesn't work. Sounds like a borrow checker limitation? Maybe @BurntSushi has an idea

I tried to hint rust as best as I can but it just won't believe that we only return one mutable borrow

let workspace = self .workspaces .range_mut(..=path.as_ref().to_path_buf()) .next_back(); if let Some((_, db)) = workspace { return db; } drop(workspace); return self.workspaces.values_mut().next().unwrap();

Not sure if I'm doing something wrong. But I think you face the problem with workspace_db_for_path_mut?

Yes, it's for the mutable version.

I tried to with as many hints as possible but it just won't believe that we only return one mutable borrow

Yeah, I tried a couple of them myself, all raised the same error

Nothing is immediately popping out at me here. This might be something that is fixed by Polonius. Specifically, this might be the problem you're hitting? https://blog.rust-lang.org/inside-rust/2023/10/06/polonius-update.html#background-on-polonius

Thanks. Yes, that might be it.

An alternative solution could be to accept a closure that applies the mutation to the db. Not as nice but hopefully that works?

Otherwise transmute your way to happiness 😂

An alternative solution could be to accept a closure that applies the mutation to the db. Not as nice but hopefully that works?

Huh, this works but it gets a little bit complicated because we need to move the path to the ChangedEvent (and also that the path could be system or virtual). I'll keep the existing logic for now with a TODO to link to this discussion.

crates/red_knot_server/src/server/api/notifications/did_close.rs

dhruvmanila · 2024-08-22T09:47:11Z

crates/red_knot_server/src/server/api/requests/diagnostic.rs

-            let line = line.parse::<u32>().unwrap_or_default();
+            let line = line.parse::<u32>().unwrap_or_default().saturating_sub(1);
            let column = column.parse::<u32>().unwrap_or_default();
            (
                Range::new(
-                    Position::new(line.saturating_sub(1), column.saturating_sub(1)),
+                    Position::new(line, column.saturating_sub(1)),


This is just to make sure the diagnostics are highlighted for at most one character. Without this, it would spill over to the next line.

github-actions · 2024-08-22T09:51:26Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/red_knot_server/src/server/api/notifications/did_close.rs

MichaReiser · 2024-08-22T11:44:52Z

crates/red_knot_server/src/server/api/notifications/did_open.rs

+            Some(db) => db,
+            None => session.default_workspace_db_mut(),
+        };
+        db.apply_changes(vec![ChangeEvent::file_created(path)], None);


I'm not sure if file_created is the correct event here. The analyzer might already have seen the file when traversing the file system. I think it is only virtual files which would be "created".

Yeah, I think that's correct. I think we might not need to emit any event here.

Or maybe it's required when publishing diagnostics for clients that doesn't support pull diagnostics. I'm thinking of adding a new event kind like Opened(SystemPathBuf).

MichaReiser · 2024-08-22T11:47:52Z

crates/red_knot_server/src/server/api.rs

-        let db = session
-            .workspace_db_for_path(path.as_std_path())
-            .map(RootDatabase::snapshot);
+        let db = match session.workspace_db_for_path(path.as_std_path()) {


Nit: We may want to consider using system path in the LSP as well to remove the back and forth between different types

Yeah, I think that should be done after adding virtual file support because then there will be two paths.

MichaReiser · 2024-08-22T11:48:55Z

crates/red_knot_server/src/server/api.rs

+        let db = match session.workspace_db_for_path(path.as_std_path()) {
+            Some(db) => db.snapshot(),
+            None => session.default_workspace_db().snapshot(),
+        };


This seems to work

pub(crate) fn workspace_db_for_path_or_default(&self, path: impl AsRef<Path>) -> &RootDatabase { self.workspace_db_for_path(path) .unwrap_or_else(|| self.default_workspace_db()) }

MichaReiser · 2024-08-22T11:51:40Z

crates/red_knot_server/src/server/api.rs

+        let db = match session.workspace_db_for_path(path.as_std_path()) {
+            Some(db) => db.snapshot(),
+            None => session.default_workspace_db().snapshot(),
+        };


This also compiles fine for me...

pub(crate) fn workspace_db_for_path(&self, path: impl AsRef<Path>) -> &RootDatabase { self.workspaces .range(..=path.as_ref().to_path_buf()) .next_back() .map(|(_, db)| db) .unwrap_or_else(|| self.default_workspace_db()) }

Not sure if I'm doing something wrong. But I think you face the problem with workspace_db_for_path_mut?

MichaReiser · 2024-08-22T12:00:37Z

crates/red_knot_server/src/server/api.rs

+        let db = match session.workspace_db_for_path(path.as_std_path()) {
+            Some(db) => db.snapshot(),
+            None => session.default_workspace_db().snapshot(),
+        };


But yeah, not sure why the mut case doesn't work. Sounds like a borrow checker limitation? Maybe @BurntSushi has an idea

I tried to hint rust as best as I can but it just won't believe that we only return one mutable borrow

let workspace = self .workspaces .range_mut(..=path.as_ref().to_path_buf()) .next_back(); if let Some((_, db)) = workspace { return db; } drop(workspace); return self.workspaces.values_mut().next().unwrap();

dhruvmanila added the ty Multi-file analysis & type inference label Aug 22, 2024

Base automatically changed from dhruv/remove-notebook-sync to main August 22, 2024 09:25

dhruvmanila force-pushed the dhruv/non-workspace-files branch 2 times, most recently from 65b6671 to f5eac02 Compare August 22, 2024 09:37

dhruvmanila commented Aug 22, 2024

View reviewed changes

crates/red_knot_server/src/server/api/notifications/did_close.rs Show resolved Hide resolved

dhruvmanila commented Aug 22, 2024

View reviewed changes

dhruvmanila force-pushed the dhruv/non-workspace-files branch from f5eac02 to bfa9e8e Compare August 22, 2024 10:49

dhruvmanila mentioned this pull request Aug 22, 2024

LSP: Support Workspace folders astral-sh/ty#89

Closed

dhruvmanila force-pushed the dhruv/non-workspace-files branch from bfa9e8e to 028cb68 Compare August 22, 2024 10:58

dhruvmanila marked this pull request as ready for review August 22, 2024 11:24

dhruvmanila requested review from AlexWaygood, MichaReiser and carljm as code owners August 22, 2024 11:24

dhruvmanila mentioned this pull request Aug 22, 2024

[red-knot] Support textDocument/didChange notification #13042

Merged

MichaReiser approved these changes Aug 22, 2024

View reviewed changes

dhruvmanila added 2 commits August 23, 2024 11:21

[red-knot] Support files outside of any workspace

c5260a6

Add ChangeEvent::Opened for didOpen notifications

0a37fad

dhruvmanila force-pushed the dhruv/non-workspace-files branch from 028cb68 to 0a37fad Compare August 23, 2024 06:47

dhruvmanila enabled auto-merge (squash) August 23, 2024 06:49

dhruvmanila merged commit c73a7bb into main Aug 23, 2024

dhruvmanila deleted the dhruv/non-workspace-files branch August 23, 2024 06:51

Conversation

dhruvmanila commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhruvmanila commented Aug 22, 2024 •

edited

Loading

MichaReiser Aug 22, 2024 •

edited

Loading

dhruvmanila Aug 22, 2024 •

edited

Loading

MichaReiser Aug 22, 2024 •

edited

Loading

dhruvmanila Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

`ruff-ecosystem` results

MichaReiser Aug 22, 2024 •

edited

Loading