3 files changed, 72 insertions, 61 deletions

diff --git a/crates/ra_cargo_watch/src/conv.rs b/crates/ra_cargo_watch/src/conv.rs
index c6f8ca329..817543deb 100644
--- a/crates/ra_cargo_watch/src/conv.rs
+++ b/crates/ra_cargo_watch/src/conv.rs
@@ -1,7 +1,8 @@
 //! This module provides the functionality needed to convert diagnostics from
 //! `cargo check` json format to the LSP diagnostic format.
 use cargo_metadata::diagnostic::{
-    Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan, DiagnosticSpanMacroExpansion,
+    Applicability, Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan,
+    DiagnosticSpanMacroExpansion,
 };
 use lsp_types::{
     CodeAction, Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag,
@@ -136,10 +137,13 @@ fn map_rust_child_diagnostic(
 
     let mut edit_map: HashMap<Url, Vec<TextEdit>> = HashMap::new();
     for &span in &spans {
-        if let Some(suggested_replacement) = &span.suggested_replacement {
-            let location = map_span_to_location(span, workspace_root);
-            let edit = TextEdit::new(location.range, suggested_replacement.clone());
-            edit_map.entry(location.uri).or_default().push(edit);
+        match (&span.suggestion_applicability, &span.suggested_replacement) {
+            (Some(Applicability::MachineApplicable), Some(suggested_replacement)) => {
+                let location = map_span_to_location(span, workspace_root);
+                let edit = TextEdit::new(location.range, suggested_replacement.clone());
+                edit_map.entry(location.uri).or_default().push(edit);
+            }
+            _ => {}
         }
     }
 
diff --git a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap
index 9e8f4eff4..a59fa84fa 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap
@@ -58,44 +58,26 @@ expression: diag
                         },
                         message: "lint level defined here",
                     },
+                    DiagnosticRelatedInformation {
+                        location: Location {
+                            uri: "file:///test/compiler/mir/tagset.rs",
+                            range: Range {
+                                start: Position {
+                                    line: 41,
+                                    character: 23,
+                                },
+                                end: Position {
+                                    line: 41,
+                                    character: 28,
+                                },
+                            },
+                        },
+                        message: "consider passing by value instead",
+                    },
                 ],
             ),
             tags: None,
         },
-        fixes: [
-            CodeAction {
-                title: "consider passing by value instead",
-                kind: Some(
-                    "quickfix",
-                ),
-                diagnostics: None,
-                edit: Some(
-                    WorkspaceEdit {
-                        changes: Some(
-                            {
-                                "file:///test/compiler/mir/tagset.rs": [
-                                    TextEdit {
-                                        range: Range {
-                                            start: Position {
-                                                line: 41,
-                                                character: 23,
-                                            },
-                                            end: Position {
-                                                line: 41,
-                                                character: 28,
-                                            },
-                                        },
-                                        new_text: "self",
-                                    },
-                                ],
-                            },
-                        ),
-                        document_changes: None,
-                    },
-                ),
-                command: None,
-                is_preferred: None,
-            },
-        ],
+        fixes: [],
     },
 ]
diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs
index bffe5eb00..7c525c430 100644
--- a/crates/ra_cargo_watch/src/lib.rs
+++ b/crates/ra_cargo_watch/src/lib.rs
@@ -8,9 +8,10 @@ use lsp_types::{
     WorkDoneProgressEnd, WorkDoneProgressReport,
 };
 use std::{
+    error, fmt,
     io::{BufRead, BufReader},
     path::{Path, PathBuf},
-    process::{Child, Command, Stdio},
+    process::{Command, Stdio},
     thread::JoinHandle,
     time::Instant,
 };
@@ -70,10 +71,10 @@ impl std::ops::Drop for CheckWatcher {
     fn drop(&mut self) {
         if let Some(handle) = self.handle.take() {
             // Take the sender out of the option
-            let recv = self.cmd_send.take();
+            let cmd_send = self.cmd_send.take();
 
             // Dropping the sender finishes the thread loop
-            drop(recv);
+            drop(cmd_send);
 
             // Join the thread, it should finish shortly. We don't really care
             // whether it panicked, so it is safe to ignore the result
@@ -246,11 +247,21 @@ enum CheckEvent {
     End,
 }
 
+#[derive(Debug)]
+pub struct CargoError(String);
+
+impl fmt::Display for CargoError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Cargo failed: {}", self.0)
+    }
+}
+impl error::Error for CargoError {}
+
 pub fn run_cargo(
     args: &[String],
     current_dir: Option<&Path>,
     on_message: &mut dyn FnMut(cargo_metadata::Message) -> bool,
-) -> Child {
+) -> Result<(), CargoError> {
     let mut command = Command::new("cargo");
     if let Some(current_dir) = current_dir {
         command.current_dir(current_dir);
@@ -273,6 +284,8 @@ pub fn run_cargo(
     // simply skip a line if it doesn't parse, which just ignores any
     // erroneus output.
     let stdout = BufReader::new(child.stdout.take().unwrap());
+    let mut read_at_least_one_message = false;
+
     for line in stdout.lines() {
         let line = match line {
             Ok(line) => line,
@@ -291,12 +304,31 @@ pub fn run_cargo(
             }
         };
 
+        read_at_least_one_message = true;
+
         if !on_message(message) {
             break;
         }
     }
 
-    child
+    // It is okay to ignore the result, as it only errors if the process is already dead
+    let _ = child.kill();
+
+    let err_msg = match child.wait() {
+        Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => {
+            // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment:
+            // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298
+            format!(
+                "the command produced no valid metadata (exit code: {:?}): cargo {}",
+                exit_code,
+                args.join(" ")
+            )
+        }
+        Err(err) => format!("io error: {:?}", err),
+        Ok(_) => return Ok(()),
+    };
+
+    Err(CargoError(err_msg))
 }
 
 impl WatchThread {
@@ -325,7 +357,7 @@ impl WatchThread {
                 // which will break out of the loop, and continue the shutdown
                 let _ = message_send.send(CheckEvent::Begin);
 
-                let mut child = run_cargo(&args, Some(&workspace_root), &mut |message| {
+                let res = run_cargo(&args, Some(&workspace_root), &mut |message| {
                     // Skip certain kinds of messages to only spend time on what's useful
                     match &message {
                         Message::CompilerArtifact(artifact) if artifact.fresh => return true,
@@ -334,26 +366,19 @@ impl WatchThread {
                         _ => {}
                     }
 
-                    match message_send.send(CheckEvent::Msg(message)) {
-                        Ok(()) => {}
-                        Err(_err) => {
-                            // The send channel was closed, so we want to shutdown
-                            return false;
-                        }
-                    };
-
-                    true
+                    // if the send channel was closed, we want to shutdown
+                    message_send.send(CheckEvent::Msg(message)).is_ok()
                 });
 
+                if let Err(err) = res {
+                    // FIXME: make the `message_send` to be `Sender<Result<CheckEvent, CargoError>>`
+                    // to display user-caused misconfiguration errors instead of just logging them here
+                    log::error!("Cargo watcher failed {:?}", err);
+                }
+
                 // We can ignore any error here, as we are already in the progress
                 // of shutting down.
                 let _ = message_send.send(CheckEvent::End);
-
-                // It is okay to ignore the result, as it only errors if the process is already dead
-                let _ = child.kill();
-
-                // Again, we don't care about the exit status so just ignore the result
-                let _ = child.wait();
             }))
         } else {
             None