From c73a7bb9294940f40dc7fe9d12b8e2fcb6535f9f Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 23 Aug 2024 12:21:48 +0530 Subject: [PATCH] [red-knot] Support files outside of any workspace (#13041) ## 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 https://github.com/user-attachments/assets/de601387-0ad5-433c-9d2c-7b6ae5137654 --- crates/red_knot_server/src/server.rs | 5 +++++ crates/red_knot_server/src/server/api.rs | 8 +++---- .../src/server/api/notifications/did_close.rs | 8 +------ .../api/notifications/did_close_notebook.rs | 8 +------ .../src/server/api/notifications/did_open.rs | 12 +++++------ .../api/notifications/did_open_notebook.rs | 12 +++++------ .../src/server/api/requests/diagnostic.rs | 10 ++++----- .../red_knot_server/src/server/api/traits.rs | 2 +- crates/red_knot_server/src/session.rs | 21 +++++++++++++++++++ crates/red_knot_workspace/src/db/changes.rs | 3 ++- crates/red_knot_workspace/src/watch.rs | 6 +++++- 11 files changed, 56 insertions(+), 39 deletions(-) diff --git a/crates/red_knot_server/src/server.rs b/crates/red_knot_server/src/server.rs index 34c8af3f5d..ef19915a5b 100644 --- a/crates/red_knot_server/src/server.rs +++ b/crates/red_knot_server/src/server.rs @@ -99,6 +99,11 @@ impl Server { anyhow::anyhow!("Failed to get the current working directory while creating a default workspace.") })?; + if workspaces.len() > 1 { + // TODO(dhruvmanila): Support multi-root workspaces + anyhow::bail!("Multi-root workspaces are not supported yet"); + } + Ok(Self { connection, worker_threads, diff --git a/crates/red_knot_server/src/server/api.rs b/crates/red_knot_server/src/server/api.rs index e6a65823c6..f1a838cba4 100644 --- a/crates/red_knot_server/src/server/api.rs +++ b/crates/red_knot_server/src/server/api.rs @@ -7,7 +7,6 @@ mod requests; mod traits; use notifications as notification; -use red_knot_workspace::db::RootDatabase; use requests as request; use self::traits::{NotificationHandler, RequestHandler}; @@ -85,9 +84,10 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>( let Ok(path) = url_to_system_path(&url) else { return Box::new(|_, _| {}); }; - 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()) { + Some(db) => db.snapshot(), + None => session.default_workspace_db().snapshot(), + }; let Some(snapshot) = session.take_snapshot(url) else { return Box::new(|_, _| {}); diff --git a/crates/red_knot_server/src/server/api/notifications/did_close.rs b/crates/red_knot_server/src/server/api/notifications/did_close.rs index 3979957b0c..cccc4f8337 100644 --- a/crates/red_knot_server/src/server/api/notifications/did_close.rs +++ b/crates/red_knot_server/src/server/api/notifications/did_close.rs @@ -2,8 +2,6 @@ use lsp_server::ErrorCode; use lsp_types::notification::DidCloseTextDocument; use lsp_types::DidCloseTextDocumentParams; -use ruff_db::files::File; - use crate::server::api::diagnostics::clear_diagnostics; use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler}; use crate::server::api::LSPResult; @@ -25,7 +23,7 @@ impl SyncNotificationHandler for DidCloseTextDocumentHandler { _requester: &mut Requester, params: DidCloseTextDocumentParams, ) -> Result<()> { - let Ok(path) = url_to_system_path(¶ms.text_document.uri) else { + let Ok(_path) = url_to_system_path(¶ms.text_document.uri) else { return Ok(()); }; @@ -34,10 +32,6 @@ impl SyncNotificationHandler for DidCloseTextDocumentHandler { .close_document(&key) .with_failure_code(ErrorCode::InternalError)?; - if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) { - File::sync_path(db, &path); - } - clear_diagnostics(key.url(), ¬ifier)?; Ok(()) diff --git a/crates/red_knot_server/src/server/api/notifications/did_close_notebook.rs b/crates/red_knot_server/src/server/api/notifications/did_close_notebook.rs index e0cfeb16f3..e136258ea4 100644 --- a/crates/red_knot_server/src/server/api/notifications/did_close_notebook.rs +++ b/crates/red_knot_server/src/server/api/notifications/did_close_notebook.rs @@ -1,8 +1,6 @@ use lsp_types::notification::DidCloseNotebookDocument; use lsp_types::DidCloseNotebookDocumentParams; -use ruff_db::files::File; - use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler}; use crate::server::api::LSPResult; use crate::server::client::{Notifier, Requester}; @@ -23,7 +21,7 @@ impl SyncNotificationHandler for DidCloseNotebookHandler { _requester: &mut Requester, params: DidCloseNotebookDocumentParams, ) -> Result<()> { - let Ok(path) = url_to_system_path(¶ms.notebook_document.uri) else { + let Ok(_path) = url_to_system_path(¶ms.notebook_document.uri) else { return Ok(()); }; @@ -32,10 +30,6 @@ impl SyncNotificationHandler for DidCloseNotebookHandler { .close_document(&key) .with_failure_code(lsp_server::ErrorCode::InternalError)?; - if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) { - File::sync_path(db, &path); - } - Ok(()) } } diff --git a/crates/red_knot_server/src/server/api/notifications/did_open.rs b/crates/red_knot_server/src/server/api/notifications/did_open.rs index 31312cf1ce..660c5fb21b 100644 --- a/crates/red_knot_server/src/server/api/notifications/did_open.rs +++ b/crates/red_knot_server/src/server/api/notifications/did_open.rs @@ -1,7 +1,7 @@ use lsp_types::notification::DidOpenTextDocument; use lsp_types::DidOpenTextDocumentParams; -use ruff_db::files::system_path_to_file; +use red_knot_workspace::watch::ChangeEvent; use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler}; use crate::server::client::{Notifier, Requester}; @@ -30,11 +30,11 @@ impl SyncNotificationHandler for DidOpenTextDocumentHandler { let document = TextDocument::new(params.text_document.text, params.text_document.version); session.open_text_document(params.text_document.uri, document); - if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) { - // TODO(dhruvmanila): Store the `file` in `DocumentController` - let file = system_path_to_file(db, &path).unwrap(); - file.sync(db); - } + let db = match session.workspace_db_for_path_mut(path.as_std_path()) { + Some(db) => db, + None => session.default_workspace_db_mut(), + }; + db.apply_changes(vec![ChangeEvent::Opened(path)], None); // TODO(dhruvmanila): Publish diagnostics if the client doesn't support pull diagnostics diff --git a/crates/red_knot_server/src/server/api/notifications/did_open_notebook.rs b/crates/red_knot_server/src/server/api/notifications/did_open_notebook.rs index b347bb0da2..fa917e17df 100644 --- a/crates/red_knot_server/src/server/api/notifications/did_open_notebook.rs +++ b/crates/red_knot_server/src/server/api/notifications/did_open_notebook.rs @@ -2,7 +2,7 @@ use lsp_server::ErrorCode; use lsp_types::notification::DidOpenNotebookDocument; use lsp_types::DidOpenNotebookDocumentParams; -use ruff_db::files::system_path_to_file; +use red_knot_workspace::watch::ChangeEvent; use crate::edit::NotebookDocument; use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler}; @@ -38,11 +38,11 @@ impl SyncNotificationHandler for DidOpenNotebookHandler { .with_failure_code(ErrorCode::InternalError)?; session.open_notebook_document(params.notebook_document.uri.clone(), notebook); - if let Some(db) = session.workspace_db_for_path_mut(path.as_std_path()) { - // TODO(dhruvmanila): Store the `file` in `DocumentController` - let file = system_path_to_file(db, &path).unwrap(); - file.sync(db); - } + let db = match session.workspace_db_for_path_mut(path.as_std_path()) { + Some(db) => db, + None => session.default_workspace_db_mut(), + }; + db.apply_changes(vec![ChangeEvent::Opened(path)], None); // TODO(dhruvmanila): Publish diagnostics if the client doesn't support pull diagnostics diff --git a/crates/red_knot_server/src/server/api/requests/diagnostic.rs b/crates/red_knot_server/src/server/api/requests/diagnostic.rs index bae3ec50c6..44998b435e 100644 --- a/crates/red_knot_server/src/server/api/requests/diagnostic.rs +++ b/crates/red_knot_server/src/server/api/requests/diagnostic.rs @@ -26,13 +26,11 @@ impl BackgroundDocumentRequestHandler for DocumentDiagnosticRequestHandler { fn run_with_snapshot( snapshot: DocumentSnapshot, - db: Option, + db: RootDatabase, _notifier: Notifier, _params: DocumentDiagnosticParams, ) -> Result { - let diagnostics = db - .map(|db| compute_diagnostics(&snapshot, &db)) - .unwrap_or_default(); + let diagnostics = compute_diagnostics(&snapshot, &db); Ok(DocumentDiagnosticReportResult::Report( DocumentDiagnosticReport::Full(RelatedFullDocumentDiagnosticReport { @@ -66,11 +64,11 @@ fn to_lsp_diagnostic(message: &str) -> Diagnostic { let (range, message) = match words.as_slice() { [_filename, line, column, message] => { - let line = line.parse::().unwrap_or_default(); + let line = line.parse::().unwrap_or_default().saturating_sub(1); let column = column.parse::().unwrap_or_default(); ( Range::new( - Position::new(line.saturating_sub(1), column.saturating_sub(1)), + Position::new(line, column.saturating_sub(1)), Position::new(line, column), ), message.trim(), diff --git a/crates/red_knot_server/src/server/api/traits.rs b/crates/red_knot_server/src/server/api/traits.rs index be539e6554..03006d0abb 100644 --- a/crates/red_knot_server/src/server/api/traits.rs +++ b/crates/red_knot_server/src/server/api/traits.rs @@ -34,7 +34,7 @@ pub(super) trait BackgroundDocumentRequestHandler: RequestHandler { fn run_with_snapshot( snapshot: DocumentSnapshot, - db: Option, + db: RootDatabase, notifier: Notifier, params: <::RequestType as Request>::Params, ) -> super::Result<<::RequestType as Request>::Result>; diff --git a/crates/red_knot_server/src/session.rs b/crates/red_knot_server/src/session.rs index 89f813588a..bc4245c132 100644 --- a/crates/red_knot_server/src/session.rs +++ b/crates/red_knot_server/src/session.rs @@ -82,6 +82,12 @@ impl Session { }) } + // TODO(dhruvmanila): Ideally, we should have a single method for `workspace_db_for_path_mut` + // and `default_workspace_db_mut` but the borrow checker doesn't allow that. + // https://github.com/astral-sh/ruff/pull/13041#discussion_r1726725437 + + /// Returns a reference to the workspace [`RootDatabase`] corresponding to the given path, if + /// any. pub(crate) fn workspace_db_for_path(&self, path: impl AsRef) -> Option<&RootDatabase> { self.workspaces .range(..=path.as_ref().to_path_buf()) @@ -89,6 +95,8 @@ impl Session { .map(|(_, db)| db) } + /// Returns a mutable reference to the workspace [`RootDatabase`] corresponding to the given + /// path, if any. pub(crate) fn workspace_db_for_path_mut( &mut self, path: impl AsRef, @@ -99,6 +107,19 @@ impl Session { .map(|(_, db)| db) } + /// Returns a reference to the default workspace [`RootDatabase`]. The default workspace is the + /// minimum root path in the workspace map. + pub(crate) fn default_workspace_db(&self) -> &RootDatabase { + // SAFETY: Currently, red knot only support a single workspace. + self.workspaces.values().next().unwrap() + } + + /// Returns a mutable reference to the default workspace [`RootDatabase`]. + pub(crate) fn default_workspace_db_mut(&mut self) -> &mut RootDatabase { + // SAFETY: Currently, red knot only support a single workspace. + self.workspaces.values_mut().next().unwrap() + } + pub fn key_from_url(&self, url: Url) -> DocumentKey { self.index().key_from_url(url) } diff --git a/crates/red_knot_workspace/src/db/changes.rs b/crates/red_knot_workspace/src/db/changes.rs index 965b7d9f0a..6b1ddf35ac 100644 --- a/crates/red_knot_workspace/src/db/changes.rs +++ b/crates/red_knot_workspace/src/db/changes.rs @@ -72,7 +72,8 @@ impl RootDatabase { } match change { - watch::ChangeEvent::Changed { path, kind: _ } => sync_path(self, &path), + watch::ChangeEvent::Changed { path, kind: _ } + | watch::ChangeEvent::Opened(path) => sync_path(self, &path), watch::ChangeEvent::Created { kind, path } => { match kind { diff --git a/crates/red_knot_workspace/src/watch.rs b/crates/red_knot_workspace/src/watch.rs index f59c0a81be..b0e6c805e9 100644 --- a/crates/red_knot_workspace/src/watch.rs +++ b/crates/red_knot_workspace/src/watch.rs @@ -20,6 +20,9 @@ mod workspace_watcher; /// event instead of emitting an event for each file or subdirectory in that path. #[derive(Debug, PartialEq, Eq)] pub enum ChangeEvent { + /// The file corresponding to the given path was opened in an editor. + Opened(SystemPathBuf), + /// A new path was created Created { path: SystemPathBuf, @@ -52,7 +55,8 @@ impl ChangeEvent { pub fn path(&self) -> Option<&SystemPath> { match self { - ChangeEvent::Created { path, .. } + ChangeEvent::Opened(path) + | ChangeEvent::Created { path, .. } | ChangeEvent::Changed { path, .. } | ChangeEvent::Deleted { path, .. } => Some(path), ChangeEvent::Rescan => None,