From 6ce559943c876c40ffd8cf0641a9ed13cd9a01a1 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 19 Jun 2024 11:45:09 -0700 Subject: [PATCH 1/3] Remove close_document from didCloseNotebook to prevent double-call --- .../api/notifications/did_close_notebook.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs b/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs index 561f2d8e68126..c18d4b20131b3 100644 --- a/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs +++ b/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs @@ -1,8 +1,6 @@ -use crate::server::api::LSPResult; use crate::server::client::{Notifier, Requester}; use crate::server::Result; use crate::session::Session; -use lsp_server::ErrorCode; use lsp_types as types; use lsp_types::notification as notif; @@ -14,20 +12,14 @@ impl super::NotificationHandler for DidCloseNotebook { impl super::SyncNotificationHandler for DidCloseNotebook { fn run( - session: &mut Session, + _session: &mut Session, _notifier: Notifier, _requester: &mut Requester, - types::DidCloseNotebookDocumentParams { - notebook_document: types::NotebookDocumentIdentifier { uri }, - .. - }: types::DidCloseNotebookDocumentParams, + _params: types::DidCloseNotebookDocumentParams, ) -> Result<()> { - let key = session.key_from_url(uri); - - session - .close_document(&key) - .with_failure_code(ErrorCode::InternalError)?; - + // `textDocument/didClose` is called after didCloseNotebook, + // and the document is removed from the index at that point. + // For this specific notification, we don't need to do anything. Ok(()) } } From f152ed954298f557251eb1f5613018b2376b5357 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Thu, 20 Jun 2024 11:31:27 -0700 Subject: [PATCH 2/3] Close documents in notebookDocument/didClose and allow snapshotting to fail --- .../src/server/api/notifications/did_close.rs | 13 +++++++------ .../api/notifications/did_close_notebook.rs | 17 +++++++++++------ crates/ruff_server/src/session/index.rs | 8 +++++--- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/crates/ruff_server/src/server/api/notifications/did_close.rs b/crates/ruff_server/src/server/api/notifications/did_close.rs index e8837327fd47a..491fa06c3d0ae 100644 --- a/crates/ruff_server/src/server/api/notifications/did_close.rs +++ b/crates/ruff_server/src/server/api/notifications/did_close.rs @@ -21,15 +21,16 @@ impl super::SyncNotificationHandler for DidClose { text_document: types::TextDocumentIdentifier { uri }, }: types::DidCloseTextDocumentParams, ) -> Result<()> { + let key = session.key_from_url(uri); // Publish an empty diagnostic report for the document. This will de-register any existing diagnostics. - let snapshot = session - .take_snapshot(uri.clone()) - .ok_or_else(|| anyhow::anyhow!("Unable to take snapshot for document with URL {uri}")) - .with_failure_code(lsp_server::ErrorCode::InternalError)?; + let Some(snapshot) = session.take_snapshot(key.clone().into_url()) else { + tracing::debug!( + "Unable to close document with key {key} - the snapshot was unavailable" + ); + return Ok(()); + }; clear_diagnostics_for_document(snapshot.query(), ¬ifier)?; - let key = snapshot.query().make_key(); - session .close_document(&key) .with_failure_code(lsp_server::ErrorCode::InternalError) diff --git a/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs b/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs index c18d4b20131b3..913ccf5d4a234 100644 --- a/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs +++ b/crates/ruff_server/src/server/api/notifications/did_close_notebook.rs @@ -1,8 +1,9 @@ +use crate::server::api::LSPResult; use crate::server::client::{Notifier, Requester}; use crate::server::Result; use crate::session::Session; -use lsp_types as types; use lsp_types::notification as notif; +use lsp_types::{self as types, NotebookDocumentIdentifier}; pub(crate) struct DidCloseNotebook; @@ -12,14 +13,18 @@ impl super::NotificationHandler for DidCloseNotebook { impl super::SyncNotificationHandler for DidCloseNotebook { fn run( - _session: &mut Session, + session: &mut Session, _notifier: Notifier, _requester: &mut Requester, - _params: types::DidCloseNotebookDocumentParams, + types::DidCloseNotebookDocumentParams { + notebook_document: NotebookDocumentIdentifier { uri }, + .. + }: types::DidCloseNotebookDocumentParams, ) -> Result<()> { - // `textDocument/didClose` is called after didCloseNotebook, - // and the document is removed from the index at that point. - // For this specific notification, we don't need to do anything. + let key = session.key_from_url(uri); + session + .close_document(&key) + .with_failure_code(lsp_server::ErrorCode::InternalError)?; Ok(()) } } diff --git a/crates/ruff_server/src/session/index.rs b/crates/ruff_server/src/session/index.rs index 341e92cc73815..c9a5fc4a5b784 100644 --- a/crates/ruff_server/src/session/index.rs +++ b/crates/ruff_server/src/session/index.rs @@ -357,9 +357,11 @@ impl Index { }; if let Some(notebook) = controller.as_notebook() { for url in notebook.urls() { - self.notebook_cells.remove(url).ok_or_else(|| { - anyhow!("tried to de-register notebook cell with URL {url} that didn't exist") - })?; + if self.notebook_cells.remove(url).is_none() { + tracing::debug!( + "tried to de-register notebook cell with URL {url} that didn't exist" + ); + } } } Ok(()) From 858d5c6ffde5d29917337fb84f61526472f1a776 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Fri, 21 Jun 2024 10:47:06 -0700 Subject: [PATCH 3/3] Remove redundant cell removal loop in close_document --- crates/ruff_server/src/session/index.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/crates/ruff_server/src/session/index.rs b/crates/ruff_server/src/session/index.rs index c9a5fc4a5b784..9c236a660e449 100644 --- a/crates/ruff_server/src/session/index.rs +++ b/crates/ruff_server/src/session/index.rs @@ -352,18 +352,9 @@ impl Index { anyhow::bail!("Tried to close unavailable document `{key}`"); }; - let Some(controller) = self.documents.remove(&url) else { + let Some(_) = self.documents.remove(&url) else { anyhow::bail!("tried to close document that didn't exist at {}", url) }; - if let Some(notebook) = controller.as_notebook() { - for url in notebook.urls() { - if self.notebook_cells.remove(url).is_none() { - tracing::debug!( - "tried to de-register notebook cell with URL {url} that didn't exist" - ); - } - } - } Ok(()) }