Merge #3632

3632: ra_cargo_watch: log errors r=matklad a=Veetaha

Until this moment we totally ignored all the errors from cargo process. Though this is still true, but we
now try to log ones that are critical (i.e. misconfiguration errors and ignore compile errors).

This fixes #3631, and gives us a better error message to more gracefully handle the #3265
![image](https://user-images.githubusercontent.com/36276403/76958683-d7e1f080-6920-11ea-83d8-04561c11ccc4.png)

Though I think that outputting this only to `Output` channel is not enough. We should somehow warn the user that he passed wrong arguments to `cargo-watch.args`. I didn't bother looking for how to do this now, but this PR at least gives us something.

*cc* @kiljacken @matklad 

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

Author: bors[bot]

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

crates/ra_project_model/src/cargo_workspace.rs

@@ -6,7 +6,7 @@ use std::{
 };
 
 use anyhow::{Context, Result};
-use cargo_metadata::{CargoOpt, Message, MetadataCommand, PackageId};
+use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId};
 use ra_arena::{Arena, Idx};
 use ra_cargo_watch::run_cargo;
 use ra_db::Edition;
@@ -254,7 +254,7 @@ pub fn load_out_dirs(
         "check".to_string(),
         "--message-format=json".to_string(),
         "--manifest-path".to_string(),
-        format!("{}", cargo_toml.display()),
+        cargo_toml.display().to_string(),
     ];
 
     if cargo_features.all_features {
@@ -263,19 +263,15 @@ pub fn load_out_dirs(
         // FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures`
         // https://github.com/oli-obk/cargo_metadata/issues/79
         args.push("--no-default-features".to_string());
-    } else if !cargo_features.features.is_empty() {
-        for feature in &cargo_features.features {
-            args.push(feature.clone());
-        }
+    } else {
+        args.extend(cargo_features.features.iter().cloned());
     }
 
-    let mut res = FxHashMap::default();
-    let mut child = run_cargo(&args, cargo_toml.parent(), &mut |message| {
+    let mut acc = FxHashMap::default();
+    let res = run_cargo(&args, cargo_toml.parent(), &mut |message| {
         match message {
-            Message::BuildScriptExecuted(message) => {
-                let package_id = message.package_id;
-                let out_dir = message.out_dir;
-                res.insert(package_id, out_dir);
+            Message::BuildScriptExecuted(BuildScript { package_id, out_dir, .. }) => {
+                acc.insert(package_id, out_dir);
             }
 
             Message::CompilerArtifact(_) => (),
@@ -285,6 +281,9 @@ pub fn load_out_dirs(
         true
     });
 
-    let _ = child.wait();
-    res
+    if let Err(err) = res {
+        log::error!("Failed to load outdirs: {:?}", err);
+    }
+
+    acc
 }