[red-knot] Support untitled files in the server #13044

dhruvmanila · 2024-08-22T06:58:48Z

Summary

This PR adds support for untitled files in the red knot server.

Test Plan

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

dhruvmanila · 2024-08-22T11:43:35Z

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

    let (range, message) = match words.as_slice() {
-        [_filename, line, column, message] => {
+        [_, _, line, column, message] | [_, line, column, message] => {
            let line = line.parse::<u32>().unwrap_or_default().saturating_sub(1);


I think this should be temporary. This is because the virtual path would contain the entire URL (untitled:Untitled-1) which means the message would have an extra :

dhruvmanila · 2024-08-22T11:44:54Z

crates/red_knot_server/src/session.rs

    pub(crate) fn file(&self, db: &RootDatabase) -> Option<File> {
-        let path = url_to_system_path(self.document_ref.file_url()).ok()?;
-        system_path_to_file(db, path).ok()
+        match url_to_any_system_path(self.document_ref.file_url()).ok()? {
+            AnySystemPath::System(path) => system_path_to_file(db, path).ok(),
+            AnySystemPath::SystemVirtual(virtual_path) => db
+                .files()
+                .try_virtual_file(&virtual_path)
+                .map(|virtual_file| virtual_file.file()),
+        }
    }


I thought about adding File to the DocumentController but my concern is about notebook cells which also uses TextDocument to represent them. I'm not exactly sure how to represent them. I do want to follow-up on this when adding support for notebooks.

dhruvmanila · 2024-08-22T11:45:40Z

crates/red_knot_workspace/src/db/changes.rs

+                watch::ChangeEvent::DeletedVirtual(path) => {
+                    if let Some(virtual_file) = self.files().try_virtual_file(&path) {
+                        virtual_file.close(self);
+                    }
+                }


Or, should we expose a close_virtual_file on Files instead?

I think this is fine for now

github-actions · 2024-08-22T11:55:12Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2024-08-22T12:10:47Z

crates/red_knot_server/src/system.rs

+        Ok(AnySystemPath::System(
+            SystemPathBuf::from_path_buf(url.to_file_path()?).map_err(|_| ())?,
+        ))


What are the cases where url.to_file_path() can fail if the schema is file?

It would be nice if this method wouldn't have to return a Result.

From the docs (https://docs.rs/url/2.5.2/url/struct.Url.html#method.to_file_path):

Returns Err if the host is neither empty nor "localhost" (except on Windows, where file: URLs may have a non-local host), or if Path::new_opt() returns None. (That is, if the percent-decoded path contains a NUL byte or, for a Windows path, is not UTF-8.)

Not sure what Path::new_opt is but the code doesn't seem to be doing that.

Going to merge the PR, will look at this in a follow-up.

MichaReiser · 2024-08-22T12:11:38Z

crates/red_knot_workspace/src/watch.rs

@@ -75,7 +84,7 @@ impl ChangeEvent {
            ChangeEvent::Created { path, .. }
            | ChangeEvent::Changed { path, .. }
            | ChangeEvent::Deleted { path, .. } => Some(path),
-            ChangeEvent::Rescan => None,
+            _ => None,


Nit: Rename the method to system_path

crates/red_knot_server/src/server/api.rs

MichaReiser · 2024-08-22T12:17:40Z

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

+                let db = session.default_workspace_db_mut();
+                db.apply_changes(vec![ChangeEvent::ChangedVirtual(virtual_path)], None);


This is more a note to myself. It's unfortunate that we need to go through apply_changes here which requires going from file -> path but this seems necessary when they e.g. open a pyproject.toml (and we support linting it in the future)

MichaReiser · 2024-08-22T12:19:08Z

crates/red_knot_server/src/session.rs

    pub(crate) fn file(&self, db: &RootDatabase) -> Option<File> {
-        let path = url_to_system_path(self.document_ref.file_url()).ok()?;
-        system_path_to_file(db, path).ok()
+        match url_to_any_system_path(self.document_ref.file_url()).ok()? {
+            AnySystemPath::System(path) => system_path_to_file(db, path).ok(),
+            AnySystemPath::SystemVirtual(virtual_path) => db
+                .files()
+                .try_virtual_file(&virtual_path)
+                .map(|virtual_file| virtual_file.file()),
+        }
    }


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

crates/red_knot_workspace/src/db/changes.rs

+                watch::ChangeEvent::DeletedVirtual(path) => {
+                    if let Some(virtual_file) = self.files().try_virtual_file(&path) {
+                        virtual_file.close(self);
+                    }
+                }


I think this is fine for now

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

dhruvmanila force-pushed the dhruv/virtual-file branch 3 times, most recently from 7f18134 to b7c8fe2 Compare August 22, 2024 10:59

dhruvmanila force-pushed the dhruv/lsp-virtual-file branch from f2920a9 to a2a7562 Compare August 22, 2024 11:41

dhruvmanila commented Aug 22, 2024

View reviewed changes

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

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

dhruvmanila mentioned this pull request Aug 22, 2024

[red-knot] Support files outside of any workspace #13041

Merged

MichaReiser approved these changes Aug 22, 2024

View reviewed changes

dhruvmanila force-pushed the dhruv/virtual-file branch from b7c8fe2 to 482af9a Compare August 23, 2024 06:59

Base automatically changed from dhruv/virtual-file to main August 23, 2024 07:04

dhruvmanila added 2 commits August 23, 2024 12:35

[red-knot] Support untitled files in the server

55af65d

Rename path -> system_path on ChangeEvent

4697f8b

dhruvmanila force-pushed the dhruv/lsp-virtual-file branch from a2a7562 to 4697f8b Compare August 23, 2024 07:05

dhruvmanila merged commit cfe25ab into main Aug 23, 2024
20 checks passed

dhruvmanila deleted the dhruv/lsp-virtual-file branch August 23, 2024 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Support untitled files in the server #13044

[red-knot] Support untitled files in the server #13044

dhruvmanila commented Aug 22, 2024 •

edited

Loading

dhruvmanila Aug 22, 2024

dhruvmanila Aug 22, 2024

MichaReiser Aug 22, 2024

dhruvmanila Aug 22, 2024

MichaReiser Aug 22, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

MichaReiser Aug 22, 2024

dhruvmanila Aug 23, 2024

dhruvmanila Aug 23, 2024

dhruvmanila Aug 23, 2024

MichaReiser Aug 22, 2024

MichaReiser Aug 22, 2024

MichaReiser Aug 22, 2024

MichaReiser Aug 22, 2024

		let db = session.default_workspace_db_mut();
		db.apply_changes(vec![ChangeEvent::ChangedVirtual(virtual_path)], None);

[red-knot] Support untitled files in the server #13044

[red-knot] Support untitled files in the server #13044

Conversation

dhruvmanila commented Aug 22, 2024 • edited Loading

Summary

Test Plan

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

github-actions bot commented Aug 22, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvmanila commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

`ruff-ecosystem` results