Merge #5046

5046: Fix progress reporting for flycheck r=matklad a=matklad

bors r+

Co-authored-by: veetaha <[email protected]>
Co-authored-by: Veetaha <[email protected]>
Co-authored-by: Aleksey Kladov <[email protected]>

5 files changed, 134 insertions, 117 deletions

diff --git a/crates/ra_flycheck/src/lib.rs b/crates/ra_flycheck/src/lib.rs
index 6c4170529..0e2ee8698 100644
--- a/crates/ra_flycheck/src/lib.rs
+++ b/crates/ra_flycheck/src/lib.rs
@@ -3,6 +3,7 @@
 //! LSP diagnostics based on the output of the command.
 
 use std::{
+    fmt,
     io::{self, BufReader},
     path::PathBuf,
     process::{Command, Stdio},
@@ -31,6 +32,17 @@ pub enum FlycheckConfig {
     },
 }
 
+impl fmt::Display for FlycheckConfig {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            FlycheckConfig::CargoCommand { command, .. } => write!(f, "cargo {}", command),
+            FlycheckConfig::CustomCommand { command, args } => {
+                write!(f, "{} {}", command, args.join(" "))
+            }
+        }
+    }
+}
+
 /// Flycheck wraps the shared state and communication machinery used for
 /// running `cargo check` (or other compatible command) and providing
 /// diagnostics based on the output.
diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs
index 87f3fe4db..64d4e2787 100644
--- a/crates/rust-analyzer/src/global_state.rs
+++ b/crates/rust-analyzer/src/global_state.rs
@@ -29,7 +29,7 @@ use rustc_hash::{FxHashMap, FxHashSet};
 
 fn create_flycheck(workspaces: &[ProjectWorkspace], config: &FlycheckConfig) -> Option<Flycheck> {
     // FIXME: Figure out the multi-workspace situation
-    workspaces.iter().find_map(|w| match w {
+    workspaces.iter().find_map(move |w| match w {
         ProjectWorkspace::Cargo { cargo, .. } => {
             let cargo_project_root = cargo.workspace_root().to_path_buf();
             Some(Flycheck::new(config.clone(), cargo_project_root.into()))
diff --git a/crates/rust-analyzer/src/lib.rs b/crates/rust-analyzer/src/lib.rs
index d6cd04303..794286672 100644
--- a/crates/rust-analyzer/src/lib.rs
+++ b/crates/rust-analyzer/src/lib.rs
@@ -29,16 +29,14 @@ mod markdown;
 mod diagnostics;
 mod line_endings;
 mod request_metrics;
+mod lsp_utils;
 pub mod lsp_ext;
 pub mod config;
 
 use serde::de::DeserializeOwned;
 
 pub type Result<T, E = Box<dyn std::error::Error + Send + Sync>> = std::result::Result<T, E>;
-pub use crate::{
-    caps::server_capabilities,
-    main_loop::{main_loop, show_message},
-};
+pub use crate::{caps::server_capabilities, lsp_utils::show_message, main_loop::main_loop};
 use std::fmt;
 
 pub fn from_json<T: DeserializeOwned>(what: &'static str, json: serde_json::Value) -> Result<T> {
diff --git a/crates/rust-analyzer/src/lsp_utils.rs b/crates/rust-analyzer/src/lsp_utils.rs
new file mode 100644
index 000000000..078f8778e
--- /dev/null
+++ b/crates/rust-analyzer/src/lsp_utils.rs
@@ -0,0 +1,44 @@
+//! Utilities for LSP-related boilerplate code.
+use std::error::Error;
+
+use crossbeam_channel::Sender;
+use lsp_server::{Message, Notification};
+use ra_db::Canceled;
+use serde::{de::DeserializeOwned, Serialize};
+
+pub fn show_message(
+    typ: lsp_types::MessageType,
+    message: impl Into<String>,
+    sender: &Sender<Message>,
+) {
+    let message = message.into();
+    let params = lsp_types::ShowMessageParams { typ, message };
+    let not = notification_new::<lsp_types::notification::ShowMessage>(params);
+    sender.send(not.into()).unwrap();
+}
+
+pub(crate) fn is_canceled(e: &(dyn Error + 'static)) -> bool {
+    e.downcast_ref::<Canceled>().is_some()
+}
+
+pub(crate) fn notification_is<N: lsp_types::notification::Notification>(
+    notification: &Notification,
+) -> bool {
+    notification.method == N::METHOD
+}
+
+pub(crate) fn notification_cast<N>(notification: Notification) -> Result<N::Params, Notification>
+where
+    N: lsp_types::notification::Notification,
+    N::Params: DeserializeOwned,
+{
+    notification.extract(N::METHOD)
+}
+
+pub(crate) fn notification_new<N>(params: N::Params) -> Notification
+where
+    N: lsp_types::notification::Notification,
+    N::Params: Serialize,
+{
+    Notification::new(N::METHOD.to_string(), params)
+}
diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs
index eb9e7f913..03569086a 100644
--- a/crates/rust-analyzer/src/main_loop.rs
+++ b/crates/rust-analyzer/src/main_loop.rs
@@ -25,6 +25,7 @@ use crate::{
     from_proto,
     global_state::{file_id_to_url, GlobalState, GlobalStateSnapshot, Status},
     handlers, lsp_ext,
+    lsp_utils::{is_canceled, notification_cast, notification_is, notification_new, show_message},
     request_metrics::RequestMetrics,
     LspError, Result,
 };
@@ -138,7 +139,7 @@ pub fn main_loop(config: Config, connection: Connection) -> Result<()> {
                 recv(global_state.flycheck.as_ref().map_or(&never(), |it| &it.task_recv)) -> task => match task {
                     Ok(task) => Event::CheckWatcher(task),
                     Err(RecvError) => return Err("check watcher died".into()),
-                }
+                },
             };
             if let Event::Msg(Message::Request(req)) = &event {
                 if connection.handle_shutdown(&req)? {
@@ -168,7 +169,6 @@ pub fn main_loop(config: Config, connection: Connection) -> Result<()> {
 #[derive(Debug)]
 enum Task {
     Respond(Response),
-    Notify(Notification),
     Diagnostic(DiagnosticTask),
 }
 
@@ -193,11 +193,6 @@ impl fmt::Debug for Event {
                     return debug_verbose_not(not, f);
                 }
             }
-            Event::Task(Task::Notify(not)) => {
-                if notification_is::<lsp_types::notification::PublishDiagnostics>(not) {
-                    return debug_verbose_not(not, f);
-                }
-            }
             Event::Task(Task::Respond(resp)) => {
                 return f
                     .debug_struct("Response")
@@ -254,14 +249,29 @@ fn loop_turn(
                 }
             }
             vfs::loader::Message::Progress { n_total, n_done } => {
-                if n_done == n_total {
+                let state = if n_done == 0 {
+                    ProgressState::Start
+                } else if n_done < n_total {
+                    ProgressState::Report
+                } else {
+                    assert_eq!(n_done, n_total);
                     global_state.status = Status::Ready;
                     became_ready = true;
-                }
-                report_progress(global_state, &connection.sender, n_done, n_total, "roots scanned")
+                    ProgressState::End
+                };
+                report_progress(
+                    global_state,
+                    &connection.sender,
+                    "roots scanned",
+                    state,
+                    Some(format!("{}/{}", n_done, n_total)),
+                    Some(percentage(n_done, n_total)),
+                )
             }
         },
-        Event::CheckWatcher(task) => on_check_task(task, global_state, task_sender)?,
+        Event::CheckWatcher(task) => {
+            on_check_task(task, global_state, task_sender, &connection.sender)?
+        }
         Event::Msg(msg) => match msg {
             Message::Request(req) => {
                 on_request(global_state, pool, task_sender, &connection.sender, loop_start, req)?
@@ -335,9 +345,6 @@ fn on_task(task: Task, msg_sender: &Sender<Message>, global_state: &mut GlobalSt
                 msg_sender.send(response.into()).unwrap();
             }
         }
-        Task::Notify(n) => {
-            msg_sender.send(n.into()).unwrap();
-        }
         Task::Diagnostic(task) => on_diagnostic_task(task, msg_sender, global_state),
     }
 }
@@ -589,6 +596,7 @@ fn on_check_task(
     task: CheckTask,
     global_state: &mut GlobalState,
     task_sender: &Sender<Task>,
+    msg_sender: &Sender<Message>,
 ) -> Result<()> {
     match task {
         CheckTask::ClearDiagnostics => {
@@ -620,39 +628,13 @@ fn on_check_task(
         }
 
         CheckTask::Status(status) => {
-            if global_state.config.client_caps.work_done_progress {
-                let progress = match status {
-                    ra_flycheck::Status::Being => {
-                        lsp_types::WorkDoneProgress::Begin(lsp_types::WorkDoneProgressBegin {
-                            title: "Running `cargo check`".to_string(),
-                            cancellable: Some(false),
-                            message: None,
-                            percentage: None,
-                        })
-                    }
-                    ra_flycheck::Status::Progress(target) => {
-                        lsp_types::WorkDoneProgress::Report(lsp_types::WorkDoneProgressReport {
-                            cancellable: Some(false),
-                            message: Some(target),
-                            percentage: None,
-                        })
-                    }
-                    ra_flycheck::Status::End => {
-                        lsp_types::WorkDoneProgress::End(lsp_types::WorkDoneProgressEnd {
-                            message: None,
-                        })
-                    }
-                };
+            let (state, message) = match status {
+                ra_flycheck::Status::Being => (ProgressState::Start, None),
+                ra_flycheck::Status::Progress(target) => (ProgressState::Report, Some(target)),
+                ra_flycheck::Status::End => (ProgressState::End, None),
+            };
 
-                let params = lsp_types::ProgressParams {
-                    token: lsp_types::ProgressToken::String(
-                        "rustAnalyzer/cargoWatcher".to_string(),
-                    ),
-                    value: lsp_types::ProgressParamsValue::WorkDone(progress),
-                };
-                let not = notification_new::<lsp_types::notification::Progress>(params);
-                task_sender.send(Task::Notify(not)).unwrap();
-            }
+            report_progress(global_state, msg_sender, "cargo check", state, message, None);
         }
     };
 
@@ -671,39 +653,55 @@ fn on_diagnostic_task(task: DiagnosticTask, msg_sender: &Sender<Message>, state:
     }
 }
 
+#[derive(Eq, PartialEq)]
+enum ProgressState {
+    Start,
+    Report,
+    End,
+}
+
+fn percentage(done: usize, total: usize) -> f64 {
+    (done as f64 / total.max(1) as f64) * 100.0
+}
+
 fn report_progress(
     global_state: &mut GlobalState,
     sender: &Sender<Message>,
-    done: usize,
-    total: usize,
-    message: &str,
+    title: &str,
+    state: ProgressState,
+    message: Option<String>,
+    percentage: Option<f64>,
 ) {
-    let token = lsp_types::ProgressToken::String(format!("rustAnalyzer/{}", message));
-    let message = Some(format!("{}/{} {}", done, total, message));
-    let percentage = Some(100.0 * done as f64 / total.max(1) as f64);
-    let work_done_progress = if done == 0 {
-        let work_done_progress_create = global_state.req_queue.outgoing.register(
-            lsp_types::request::WorkDoneProgressCreate::METHOD.to_string(),
-            lsp_types::WorkDoneProgressCreateParams { token: token.clone() },
-            DO_NOTHING,
-        );
-        sender.send(work_done_progress_create.into()).unwrap();
-
-        lsp_types::WorkDoneProgress::Begin(lsp_types::WorkDoneProgressBegin {
-            title: "rust-analyzer".into(),
-            cancellable: None,
-            message,
-            percentage,
-        })
-    } else if done < total {
-        lsp_types::WorkDoneProgress::Report(lsp_types::WorkDoneProgressReport {
-            cancellable: None,
-            message,
-            percentage,
-        })
-    } else {
-        assert!(done == total);
-        lsp_types::WorkDoneProgress::End(lsp_types::WorkDoneProgressEnd { message })
+    if !global_state.config.client_caps.work_done_progress {
+        return;
+    }
+    let token = lsp_types::ProgressToken::String(format!("rustAnalyzer/{}", title));
+    let work_done_progress = match state {
+        ProgressState::Start => {
+            let work_done_progress_create = global_state.req_queue.outgoing.register(
+                lsp_types::request::WorkDoneProgressCreate::METHOD.to_string(),
+                lsp_types::WorkDoneProgressCreateParams { token: token.clone() },
+                DO_NOTHING,
+            );
+            sender.send(work_done_progress_create.into()).unwrap();
+
+            lsp_types::WorkDoneProgress::Begin(lsp_types::WorkDoneProgressBegin {
+                title: title.into(),
+                cancellable: None,
+                message,
+                percentage,
+            })
+        }
+        ProgressState::Report => {
+            lsp_types::WorkDoneProgress::Report(lsp_types::WorkDoneProgressReport {
+                cancellable: None,
+                message,
+                percentage,
+            })
+        }
+        ProgressState::End => {
+            lsp_types::WorkDoneProgress::End(lsp_types::WorkDoneProgressEnd { message })
+        }
     };
     let notification =
         notification_new::<lsp_types::notification::Progress>(lsp_types::ProgressParams {
@@ -826,7 +824,7 @@ where
         Err(e) => match e.downcast::<LspError>() {
             Ok(lsp_error) => Response::new_err(id, lsp_error.code, lsp_error.message),
             Err(e) => {
-                if is_canceled(&e) {
+                if is_canceled(&*e) {
                     Response::new_err(
                         id,
                         ErrorCode::ContentModified as i32,
@@ -853,7 +851,7 @@ fn update_file_notifications_on_threadpool(
             for file_id in subscriptions {
                 match handlers::publish_diagnostics(&world, file_id) {
                     Err(e) => {
-                        if !is_canceled(&e) {
+                        if !is_canceled(&*e) {
                             log::error!("failed to compute diagnostics: {:?}", e);
                         }
                     }
@@ -866,41 +864,6 @@ fn update_file_notifications_on_threadpool(
     }
 }
 
-pub fn show_message(
-    typ: lsp_types::MessageType,
-    message: impl Into<String>,
-    sender: &Sender<Message>,
-) {
-    let message = message.into();
-    let params = lsp_types::ShowMessageParams { typ, message };
-    let not = notification_new::<lsp_types::notification::ShowMessage>(params);
-    sender.send(not.into()).unwrap();
-}
-
-fn is_canceled(e: &Box<dyn std::error::Error + Send + Sync>) -> bool {
-    e.downcast_ref::<Canceled>().is_some()
-}
-
-fn notification_is<N: lsp_types::notification::Notification>(notification: &Notification) -> bool {
-    notification.method == N::METHOD
-}
-
-fn notification_cast<N>(notification: Notification) -> std::result::Result<N::Params, Notification>
-where
-    N: lsp_types::notification::Notification,
-    N::Params: DeserializeOwned,
-{
-    notification.extract(N::METHOD)
-}
-
-fn notification_new<N>(params: N::Params) -> Notification
-where
-    N: lsp_types::notification::Notification,
-    N::Params: Serialize,
-{
-    Notification::new(N::METHOD.to_string(), params)
-}
-
 #[cfg(test)]
 mod tests {
     use lsp_types::{Position, Range, TextDocumentContentChangeEvent};