Use window/showMessage to communicate build errors to user

Author: algesten

src/actions/mod.rs

@@ -22,16 +22,17 @@ use span;
 use Span;
 
 use actions::post_build::{BuildResults, PostBuildHandler, DiagnosticsNotifier};
-use actions::notifications::{PublishDiagnostics, Progress};
+use actions::notifications::{PublishDiagnostics, Progress, ShowMessage};
 use build::*;
 use lsp_data;
 use lsp_data::*;
+use ls_types::MessageType;
 use server::{Output, Notification};
 
 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;
 
 
@@ -119,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.
@@ -165,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),
@@ -219,6 +222,10 @@ impl InitActionContext {
             out: O,
             active_build_count: Arc<AtomicUsize>,
             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,
         }
 
         // base progress parameters for notification of the analysis.
@@ -235,11 +242,39 @@ impl InitActionContext {
             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) {
                 self.active_build_count.fetch_sub(1, Ordering::SeqCst);
                 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);
             }
         }
 
@@ -287,6 +322,8 @@ impl InitActionContext {
                     out: out.clone(),
                     active_build_count: self.active_build_count.clone(),
                     progress_params: diganostics_progress_params,
+                    previous_was_build_error: self.previous_was_build_error.clone(),
+                    current_is_build_error: AtomicBool::new(false),
                 }),
                 blocked_threads: vec![],
             }

src/actions/post_build.rs

@@ -48,6 +48,7 @@ pub struct PostBuildHandler {
 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);
 }
 
@@ -82,8 +83,11 @@ impl PostBuildHandler {
             BuildResult::Squashed => {
                 trace!("build - Squashed");
             }
-            BuildResult::Err => {
+            BuildResult::Err(msg) => {
                 trace!("build - Error");
+                self.notifier.notify_begin_diagnostics();
+                self.notifier.notify_build_error(msg);
+                self.notifier.notify_end_diagnostics();
             }
         }
     }

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

@@ -384,7 +384,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!");
+}