Auto merge of #12431 - Veykril:request-retry, r=Veykril

fix: Fix completions disappearing when typing two keys in quick succession

With this PR we now retry requests if they get cancelled due to document changes.

This fixes the completions problem we have where completions seem to randomly disappear, see https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Completions.20not.20always.20appearing

Fixes #10187
Fixes #7560
Fixes #12153

Author: bors

crates/rust-analyzer/src/bin/main.rs

@@ -157,7 +157,7 @@ fn run_server() -> Result<()> {
     let (initialize_id, initialize_params) = connection.initialize_start()?;
     tracing::info!("InitializeParams: {}", initialize_params);
     let initialize_params =
-        from_json::<lsp_types::InitializeParams>("InitializeParams", initialize_params)?;
+        from_json::<lsp_types::InitializeParams>("InitializeParams", &initialize_params)?;
 
     let root_path = match initialize_params
         .root_uri

crates/rust-analyzer/src/dispatch.rs

@@ -1,12 +1,12 @@
 //! See [RequestDispatcher].
 use std::{fmt, panic, thread};
 
+use ide::Cancelled;
 use lsp_server::ExtractError;
 use serde::{de::DeserializeOwned, Serialize};
 
 use crate::{
     global_state::{GlobalState, GlobalStateSnapshot},
-    lsp_utils::is_cancelled,
     main_loop::Task,
     LspError, Result,
 };
@@ -37,49 +37,53 @@ impl<'a> RequestDispatcher<'a> {
     pub(crate) fn on_sync_mut<R>(
         &mut self,
         f: fn(&mut GlobalState, R::Params) -> Result<R::Result>,
-    ) -> Result<&mut Self>
+    ) -> &mut Self
     where
         R: lsp_types::request::Request,
         R::Params: DeserializeOwned + panic::UnwindSafe + fmt::Debug,
         R::Result: Serialize,
     {
-        let (id, params, panic_context) = match self.parse::<R>() {
+        let (req, params, panic_context) = match self.parse::<R>() {
             Some(it) => it,
-            None => return Ok(self),
+            None => return self,
         };
-        let _pctx = stdx::panic_context::enter(panic_context);
-
-        let result = f(self.global_state, params);
-        let response = result_to_response::<R>(id, result);
+        let result = {
+            let _pctx = stdx::panic_context::enter(panic_context);
+            f(self.global_state, params)
+        };
+        if let Ok(response) = result_to_response::<R>(req.id.clone(), result) {
+            self.global_state.respond(response);
+        }
 
-        self.global_state.respond(response);
-        Ok(self)
+        self
     }
 
     /// Dispatches the request onto the current thread.
     pub(crate) fn on_sync<R>(
         &mut self,
         f: fn(GlobalStateSnapshot, R::Params) -> Result<R::Result>,
-    ) -> Result<&mut Self>
+    ) -> &mut Self
     where
-        R: lsp_types::request::Request + 'static,
+        R: lsp_types::request::Request,
         R::Params: DeserializeOwned + panic::UnwindSafe + fmt::Debug,
         R::Result: Serialize,
     {
-        let (id, params, panic_context) = match self.parse::<R>() {
+        let (req, params, panic_context) = match self.parse::<R>() {
             Some(it) => it,
-            None => return Ok(self),
+            None => return self,
         };
         let global_state_snapshot = self.global_state.snapshot();
 
         let result = panic::catch_unwind(move || {
             let _pctx = stdx::panic_context::enter(panic_context);
             f(global_state_snapshot, params)
         });
-        let response = thread_result_to_response::<R>(id, result);
 
-        self.global_state.respond(response);
-        Ok(self)
+        if let Ok(response) = thread_result_to_response::<R>(req.id.clone(), result) {
+            self.global_state.respond(response);
+        }
+
+        self
     }
 
     /// Dispatches the request onto thread pool
@@ -92,7 +96,7 @@ impl<'a> RequestDispatcher<'a> {
         R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug,
         R::Result: Serialize,
     {
-        let (id, params, panic_context) = match self.parse::<R>() {
+        let (req, params, panic_context) = match self.parse::<R>() {
             Some(it) => it,
             None => return self,
         };
@@ -104,8 +108,10 @@ impl<'a> RequestDispatcher<'a> {
                     let _pctx = stdx::panic_context::enter(panic_context);
                     f(world, params)
                 });
-                let response = thread_result_to_response::<R>(id, result);
-                Task::Response(response)
+                match thread_result_to_response::<R>(req.id.clone(), result) {
+                    Ok(response) => Task::Response(response),
+                    Err(_) => Task::Retry(req),
+                }
             }
         });
 
@@ -124,7 +130,7 @@ impl<'a> RequestDispatcher<'a> {
         }
     }
 
-    fn parse<R>(&mut self) -> Option<(lsp_server::RequestId, R::Params, String)>
+    fn parse<R>(&mut self) -> Option<(lsp_server::Request, R::Params, String)>
     where
         R: lsp_types::request::Request,
         R::Params: DeserializeOwned + fmt::Debug,
@@ -134,12 +140,12 @@ impl<'a> RequestDispatcher<'a> {
             _ => return None,
         };
 
-        let res = crate::from_json(R::METHOD, req.params);
+        let res = crate::from_json(R::METHOD, &req.params);
         match res {
             Ok(params) => {
                 let panic_context =
                     format!("\nversion: {}\nrequest: {} {:#?}", env!("REV"), R::METHOD, params);
-                Some((req.id, params, panic_context))
+                Some((req, params, panic_context))
             }
             Err(err) => {
                 let response = lsp_server::Response::new_err(
@@ -157,7 +163,7 @@ impl<'a> RequestDispatcher<'a> {
 fn thread_result_to_response<R>(
     id: lsp_server::RequestId,
     result: thread::Result<Result<R::Result>>,
-) -> lsp_server::Response
+) -> Result<lsp_server::Response, Cancelled>
 where
     R: lsp_types::request::Request,
     R::Params: DeserializeOwned,
@@ -166,53 +172,50 @@ where
     match result {
         Ok(result) => result_to_response::<R>(id, result),
         Err(panic) => {
-            let mut message = "request handler panicked".to_string();
-
             let panic_message = panic
                 .downcast_ref::<String>()
                 .map(String::as_str)
                 .or_else(|| panic.downcast_ref::<&str>().copied());
 
+            let mut message = "request handler panicked".to_string();
             if let Some(panic_message) = panic_message {
                 message.push_str(": ");
                 message.push_str(panic_message)
             };
 
-            lsp_server::Response::new_err(id, lsp_server::ErrorCode::InternalError as i32, message)
+            Ok(lsp_server::Response::new_err(
+                id,
+                lsp_server::ErrorCode::InternalError as i32,
+                message,
+            ))
         }
     }
 }
 
 fn result_to_response<R>(
     id: lsp_server::RequestId,
     result: Result<R::Result>,
-) -> lsp_server::Response
+) -> Result<lsp_server::Response, Cancelled>
 where
     R: lsp_types::request::Request,
     R::Params: DeserializeOwned,
     R::Result: Serialize,
 {
-    match result {
+    let res = match result {
         Ok(resp) => lsp_server::Response::new_ok(id, &resp),
         Err(e) => match e.downcast::<LspError>() {
             Ok(lsp_error) => lsp_server::Response::new_err(id, lsp_error.code, lsp_error.message),
-            Err(e) => {
-                if is_cancelled(&*e) {
-                    lsp_server::Response::new_err(
-                        id,
-                        lsp_server::ErrorCode::ContentModified as i32,
-                        "content modified".to_string(),
-                    )
-                } else {
-                    lsp_server::Response::new_err(
-                        id,
-                        lsp_server::ErrorCode::InternalError as i32,
-                        e.to_string(),
-                    )
-                }
-            }
+            Err(e) => match e.downcast::<Cancelled>() {
+                Ok(cancelled) => return Err(*cancelled),
+                Err(e) => lsp_server::Response::new_err(
+                    id,
+                    lsp_server::ErrorCode::InternalError as i32,
+                    e.to_string(),
+                ),
+            },
         },
-    }
+    };
+    Ok(res)
 }
 
 pub(crate) struct NotificationDispatcher<'a> {

crates/rust-analyzer/src/from_proto.rs

@@ -91,7 +91,7 @@ pub(crate) fn annotation(
 ) -> Result<Annotation> {
     let data =
         code_lens.data.ok_or_else(|| invalid_params_error("code lens without data".to_string()))?;
-    let resolve = from_json::<lsp_ext::CodeLensResolveData>("CodeLensResolveData", data)?;
+    let resolve = from_json::<lsp_ext::CodeLensResolveData>("CodeLensResolveData", &data)?;
 
     match resolve {
         lsp_ext::CodeLensResolveData::Impls(params) => {

crates/rust-analyzer/src/lib.rs

@@ -49,7 +49,7 @@ pub use crate::{caps::server_capabilities, main_loop::main_loop};
 pub type Error = Box<dyn std::error::Error + Send + Sync>;
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
-pub fn from_json<T: DeserializeOwned>(what: &'static str, json: serde_json::Value) -> Result<T> {
+pub fn from_json<T: DeserializeOwned>(what: &'static str, json: &serde_json::Value) -> Result<T> {
     let res = serde_json::from_value(json.clone())
         .map_err(|e| format!("Failed to deserialize {}: {}; {}", what, e, json))?;
     Ok(res)

crates/rust-analyzer/src/lsp_utils.rs

@@ -1,7 +1,6 @@
 //! Utilities for LSP-related boilerplate code.
-use std::{error::Error, ops::Range, sync::Arc};
+use std::{ops::Range, sync::Arc};
 
-use ide_db::base_db::Cancelled;
 use lsp_server::Notification;
 
 use crate::{
@@ -15,10 +14,6 @@ pub(crate) fn invalid_params_error(message: String) -> LspError {
     LspError { code: lsp_server::ErrorCode::InvalidParams as i32, message }
 }
 
-pub(crate) fn is_cancelled(e: &(dyn Error + 'static)) -> bool {
-    e.downcast_ref::<Cancelled>().is_some()
-}
-
 pub(crate) fn notification_is<N: lsp_types::notification::Notification>(
     notification: &Notification,
 ) -> bool {

crates/rust-analyzer/src/main_loop.rs

@@ -8,7 +8,7 @@ use std::{
 
 use always_assert::always;
 use crossbeam_channel::{select, Receiver};
-use ide_db::base_db::{SourceDatabaseExt, VfsPath};
+use ide_db::base_db::{Cancelled, SourceDatabaseExt, VfsPath};
 use lsp_server::{Connection, Notification, Request};
 use lsp_types::notification::Notification as _;
 use vfs::{ChangeKind, FileId};
@@ -19,7 +19,7 @@ use crate::{
     from_proto,
     global_state::{file_id_to_url, url_to_file_id, GlobalState},
     handlers, lsp_ext,
-    lsp_utils::{apply_document_changes, is_cancelled, notification_is, Progress},
+    lsp_utils::{apply_document_changes, notification_is, Progress},
     mem_docs::DocumentData,
     reload::{self, BuildDataProgress, ProjectWorkspaceProgress},
     Result,
@@ -60,6 +60,7 @@ enum Event {
 #[derive(Debug)]
 pub(crate) enum Task {
     Response(lsp_server::Response),
+    Retry(lsp_server::Request),
     Diagnostics(Vec<(FileId, Vec<lsp_types::Diagnostic>)>),
     PrimeCaches(PrimeCachesProgress),
     FetchWorkspace(ProjectWorkspaceProgress),
@@ -196,7 +197,7 @@ impl GlobalState {
         let was_quiescent = self.is_quiescent();
         match event {
             Event::Lsp(msg) => match msg {
-                lsp_server::Message::Request(req) => self.on_request(loop_start, req)?,
+                lsp_server::Message::Request(req) => self.on_request(loop_start, req),
                 lsp_server::Message::Notification(not) => {
                     self.on_notification(not)?;
                 }
@@ -208,6 +209,7 @@ impl GlobalState {
                 loop {
                     match task {
                         Task::Response(response) => self.respond(response),
+                        Task::Retry(req) => self.on_request(loop_start, req),
                         Task::Diagnostics(diagnostics_per_file) => {
                             for (file_id, diagnostics) in diagnostics_per_file {
                                 self.diagnostics.set_native_diagnostics(file_id, diagnostics)
@@ -553,7 +555,7 @@ impl GlobalState {
         Ok(())
     }
 
-    fn on_request(&mut self, request_received: Instant, req: Request) -> Result<()> {
+    fn on_request(&mut self, request_received: Instant, req: Request) {
         self.register_request(&req, request_received);
 
         if self.shutdown_requested {
@@ -562,8 +564,7 @@ impl GlobalState {
                 lsp_server::ErrorCode::InvalidRequest as i32,
                 "Shutdown already requested.".to_owned(),
             ));
-
-            return Ok(());
+            return;
         }
 
         // Avoid flashing a bunch of unresolved references during initial load.
@@ -573,21 +574,21 @@ impl GlobalState {
                 lsp_server::ErrorCode::ContentModified as i32,
                 "waiting for cargo metadata or cargo check".to_owned(),
             ));
-            return Ok(());
+            return;
         }
 
         RequestDispatcher { req: Some(req), global_state: self }
             .on_sync_mut::<lsp_types::request::Shutdown>(|s, ()| {
                 s.shutdown_requested = true;
                 Ok(())
-            })?
-            .on_sync_mut::<lsp_ext::ReloadWorkspace>(handlers::handle_workspace_reload)?
-            .on_sync_mut::<lsp_ext::MemoryUsage>(handlers::handle_memory_usage)?
-            .on_sync_mut::<lsp_ext::ShuffleCrateGraph>(handlers::handle_shuffle_crate_graph)?
-            .on_sync::<lsp_ext::JoinLines>(handlers::handle_join_lines)?
-            .on_sync::<lsp_ext::OnEnter>(handlers::handle_on_enter)?
-            .on_sync::<lsp_types::request::SelectionRangeRequest>(handlers::handle_selection_range)?
-            .on_sync::<lsp_ext::MatchingBrace>(handlers::handle_matching_brace)?
+            })
+            .on_sync_mut::<lsp_ext::ReloadWorkspace>(handlers::handle_workspace_reload)
+            .on_sync_mut::<lsp_ext::MemoryUsage>(handlers::handle_memory_usage)
+            .on_sync_mut::<lsp_ext::ShuffleCrateGraph>(handlers::handle_shuffle_crate_graph)
+            .on_sync::<lsp_ext::JoinLines>(handlers::handle_join_lines)
+            .on_sync::<lsp_ext::OnEnter>(handlers::handle_on_enter)
+            .on_sync::<lsp_types::request::SelectionRangeRequest>(handlers::handle_selection_range)
+            .on_sync::<lsp_ext::MatchingBrace>(handlers::handle_matching_brace)
             .on::<lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)
             .on::<lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)
             .on::<lsp_ext::ViewHir>(handlers::handle_view_hir)
@@ -644,8 +645,8 @@ impl GlobalState {
             .on::<lsp_types::request::WillRenameFiles>(handlers::handle_will_rename_files)
             .on::<lsp_ext::Ssr>(handlers::handle_ssr)
             .finish();
-        Ok(())
     }
+
     fn on_notification(&mut self, not: Notification) -> Result<()> {
         NotificationDispatcher { not: Some(not), global_state: self }
             .on::<lsp_types::notification::Cancel>(|this, params| {
@@ -792,7 +793,7 @@ impl GlobalState {
                 .filter_map(|file_id| {
                     handlers::publish_diagnostics(&snapshot, file_id)
                         .map_err(|err| {
-                            if !is_cancelled(&*err) {
+                            if err.is::<Cancelled>() {
                                 tracing::error!("failed to compute diagnostics: {:?}", err);
                             }
                         })