Use window/showMessage to communicate build errors to user

Author: algesten

src/actions/mod.rs

@@ -31,7 +31,7 @@ use server::Output;
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
-use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering};
 use std::thread;
 
 
@@ -120,6 +120,7 @@ pub struct InitActionContext {
     current_project: PathBuf,
 
     previous_build_results: Arc<Mutex<BuildResults>>,
+    previous_was_build_error: Arc<AtomicBool>,
     build_queue: BuildQueue,
     // Keep a record of builds/post-build tasks currently in flight so that
     // mutating actions can block until the data is ready.
@@ -166,6 +167,7 @@ impl InitActionContext {
             config,
             current_project,
             previous_build_results: Arc::new(Mutex::new(HashMap::new())),
+            previous_was_build_error: Arc::new(AtomicBool::new(false)),
             build_queue,
             active_build_count: Arc::new(AtomicUsize::new(0)),
             client_capabilities: Arc::new(client_capabilities),
@@ -208,7 +210,10 @@ impl InitActionContext {
                 show_warnings: config.show_warnings,
                 use_black_list: config.use_crate_blacklist,
                 active_build_count: self.active_build_count.clone(),
-                notifier: Box::new(BuildDiagnosticsNotifier::new(out.clone())),
+                notifier: Box::new(BuildDiagnosticsNotifier::new(
+                    out.clone(),
+                    self.previous_was_build_error.clone(),
+                )),
                 blocked_threads: vec![],
             }
         };

src/actions/post_build.rs

@@ -79,8 +79,11 @@ impl PostBuildHandler {
                 trace!("build - Squashed");
                 self.active_build_count.fetch_sub(1, Ordering::SeqCst);
             }
-            BuildResult::Err => {
+            BuildResult::Err(msg) => {
                 trace!("build - Error");
+                self.notifier.notify_begin_diagnostics();
+                self.notifier.notify_build_error(msg);
+                self.notifier.notify_end_diagnostics();
                 self.active_build_count.fetch_sub(1, Ordering::SeqCst);
             }
         }

src/actions/progress.rs

@@ -8,10 +8,11 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
 
-use actions::notifications::{PublishDiagnostics, Progress};
-use lsp_data::{ProgressParams, PublishDiagnosticsParams};
+use actions::notifications::{PublishDiagnostics, Progress, ShowMessage};
+use lsp_data::{ProgressParams, PublishDiagnosticsParams, ShowMessageParams, MessageType};
 use server::{Output, Notification};
 
 /// Trait for communication of build progress back to the client.
@@ -34,6 +35,7 @@ pub enum ProgressUpdate {
 pub trait DiagnosticsNotifier: Send {
     fn notify_begin_diagnostics(&self);
     fn notify_publish_diagnostics(&self, PublishDiagnosticsParams);
+    fn notify_build_error(&self, msg: String);
     fn notify_end_diagnostics(&self);
 }
 
@@ -101,13 +103,19 @@ pub struct BuildDiagnosticsNotifier<O: Output> {
     // these params are used as a template and are cloned for each
     // message that is actually notified.
     progress_params: ProgressParams,
+    // shared bool in the entire context
+    previous_was_build_error: Arc<AtomicBool>,
+    // whether this current build encountered an error.
+    current_is_build_error: AtomicBool,
 }
 
 impl<O: Output> BuildDiagnosticsNotifier<O> {
-    pub fn new(out: O) -> BuildDiagnosticsNotifier<O> {
+    pub fn new(out: O, previous_was_build_error: Arc<AtomicBool>) -> BuildDiagnosticsNotifier<O> {
         BuildDiagnosticsNotifier {
             out,
             progress_params: new_progress_params("Diagnostics".into()),
+            previous_was_build_error,
+            current_is_build_error: AtomicBool::new(false),
         }
     }
 }
@@ -120,9 +128,37 @@ impl<O: Output> DiagnosticsNotifier for BuildDiagnosticsNotifier<O> {
     fn notify_publish_diagnostics(&self, params: PublishDiagnosticsParams) {
         self.out.notify(Notification::<PublishDiagnostics>::new(params));
     }
+    fn notify_build_error(&self, message: String) {
+        // This current build is definitely an error. Mark it and
+        // propagate to the shrared state in notify_end_diagnostics()
+        self.current_is_build_error.store(true, Ordering::SeqCst);
+
+        // We only want to notify a build error to the user one time,
+        // not on every keystroke. As long as a previous build error
+        // hasn't cleared, we stay silent.
+        if !self.previous_was_build_error.load(Ordering::SeqCst) {
+            let params = ShowMessageParams {
+                typ: MessageType::Error,
+                message,
+            };
+            self.out.notify(Notification::<ShowMessage>::new(params));
+        }
+    }
     fn notify_end_diagnostics(&self) {
         let mut params = self.progress_params.clone();
         params.done = Some(true);
         self.out.notify(Notification::<Progress>::new(params));
+
+        // If the diagnostics for this build were fully reported without
+        // any current_is_build_error, we know the build is clear, and
+        // that is propagated to the shared previous_was_build_error.
+        //
+        // This is not perfectly thread safe. There's a chance two threads
+        // may be in contention propagating the local current_is_build_error
+        // to the shared previous_was_build_error. Whether this happens
+        // in practice is to be seen...
+        let is_build_error = self.current_is_build_error.load(Ordering::SeqCst);
+        self.previous_was_build_error
+            .compare_and_swap(!is_build_error, is_build_error, Ordering::SeqCst);
     }
 }

src/build/cargo.rs

@@ -84,8 +84,14 @@ pub(super) fn cargo(internals: &Internals, progress_sender: Sender<ProgressUpdat
         Ok(cwd) => BuildResult::Success(cwd, vec![], vec![]),
         Err(err) => {
             let stdout = String::from_utf8(out_clone.lock().unwrap().to_owned()).unwrap();
-            info!("cargo failed\ncause: {}\nstdout: {}", err, stdout);
-            BuildResult::Err
+            let stdout_msg = if stdout.is_empty() {
+                "".to_string()
+            } else {
+                format!("({})", stdout)
+            };
+            let msg = format!("Cargo failed: {}{}", err, stdout_msg);
+            info!("{}", msg);
+            BuildResult::Err(msg)
         }
     }
 }

src/build/mod.rs

@@ -103,8 +103,9 @@ pub enum BuildResult {
     Success(PathBuf, Vec<String>, Vec<Analysis>),
     /// Build was coalesced with another build.
     Squashed,
-    /// There was an error attempting to build.
-    Err,
+    /// There was an error attempting to build. The argument is a message
+    /// to be propagated to the user.
+    Err(String),
 }
 
 /// Priority for a build request.
@@ -545,8 +546,8 @@ impl Internals {
         // In single package mode Cargo needs to be run to cache args/envs for
         // future rustc calls
         } else if needs_to_run_cargo {
-            if let BuildResult::Err = cargo::cargo(self, progress_sender) {
-                return BuildResult::Err;
+            if let BuildResult::Err(s) = cargo::cargo(self, progress_sender) {
+                return BuildResult::Err(s);
             }
         }
 

src/build/plan.rs

@@ -385,7 +385,7 @@ impl JobQueue {
                     analyses.append(&mut analysis);
                     cwd = c;
                 }
-                BuildResult::Err => return BuildResult::Err,
+                BuildResult::Err(msg) => return BuildResult::Err(msg),
                 _ => {}
             }
         }

src/test/mod.rs

@@ -1386,3 +1386,31 @@ fn test_deglob() {
         ],
     );
 }
+
+#[test]
+fn test_cargo_fail() {
+    let mut env = Environment::new("cargo_fail");
+
+    let root_path = env.cache.abs_path(Path::new("."));
+    let messages = vec![
+        initialize(0, root_path.as_os_str().to_str().map(|x| x.to_owned())).to_string(),
+    ];
+
+    let (mut server, results) = env.mock_server(messages);
+    // Initialize and build.
+    assert_eq!(
+        ls_server::LsService::handle_message(&mut server),
+        ls_server::ServerStateChange::Continue
+    );
+    expect_messages(
+        results.clone(),
+        &[
+            ExpectedMessage::new(Some(0)).expect_contains("capabilities"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains("Build"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#""done":true"#),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains("Diagnostics"),
+            ExpectedMessage::new(None).expect_contains("showMessage").expect_contains("Cargo failed: no matching package named `some_non_existant_crate`"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#""done":true"#),
+        ],
+    );
+}

test_data/cargo_fail/Cargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "cargo_fail"
+version = "0.1.0"
+authors = ["Martin Algesten <[email protected]>"]
+
+[dependencies]
+some_non_existant_crate = "*"

test_data/cargo_fail/src/main.rs

@@ -0,0 +1,7 @@
+
+extern crate some_none_existant_crate;
+
+fn main() {
+// deliberate bad formatting
+println!("Hello, world!");
+}