Merge #8375

8375: feat: show errors from `cargo metadata` and initial `cargo check` in the status bar r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/paths/src/lib.rs

@@ -1,6 +1,7 @@
 //! Thin wrappers around `std::path`, distinguishing between absolute and
 //! relative paths.
 use std::{
+    borrow::Borrow,
     convert::{TryFrom, TryInto},
     ops,
     path::{Component, Path, PathBuf},
@@ -35,6 +36,12 @@ impl AsRef<AbsPath> for AbsPathBuf {
     }
 }
 
+impl Borrow<AbsPath> for AbsPathBuf {
+    fn borrow(&self) -> &AbsPath {
+        self.as_path()
+    }
+}
+
 impl TryFrom<PathBuf> for AbsPathBuf {
     type Error = PathBuf;
     fn try_from(path_buf: PathBuf) -> Result<AbsPathBuf, PathBuf> {

crates/project_model/src/build_data.rs

@@ -13,12 +13,12 @@ use cargo_metadata::{BuildScript, Message};
 use itertools::Itertools;
 use paths::{AbsPath, AbsPathBuf};
 use rustc_hash::FxHashMap;
-use stdx::JodChild;
+use stdx::{format_to, JodChild};
 
 use crate::{cfg_flag::CfgFlag, CargoConfig};
 
 #[derive(Debug, Clone, Default, PartialEq, Eq)]
-pub(crate) struct BuildData {
+pub(crate) struct PackageBuildData {
     /// List of config flags defined by this package's build script
     pub(crate) cfgs: Vec<CfgFlag>,
     /// List of cargo-related environment variables with their value
@@ -32,6 +32,17 @@ pub(crate) struct BuildData {
     pub(crate) proc_macro_dylib_path: Option<AbsPathBuf>,
 }
 
+#[derive(Debug, Default, PartialEq, Eq, Clone)]
+pub(crate) struct WorkspaceBuildData {
+    per_package: FxHashMap<String, PackageBuildData>,
+    error: Option<String>,
+}
+
+#[derive(Debug, Default, PartialEq, Eq, Clone)]
+pub struct BuildDataResult {
+    per_workspace: FxHashMap<AbsPathBuf, WorkspaceBuildData>,
+}
+
 #[derive(Clone, Debug)]
 pub(crate) struct BuildDataConfig {
     cargo_toml: AbsPathBuf,
@@ -52,13 +63,6 @@ pub struct BuildDataCollector {
     configs: FxHashMap<AbsPathBuf, BuildDataConfig>,
 }
 
-#[derive(Debug, Default, PartialEq, Eq, Clone)]
-pub struct BuildDataResult {
-    data: FxHashMap<AbsPathBuf, BuildDataMap>,
-}
-
-pub(crate) type BuildDataMap = FxHashMap<String, BuildData>;
-
 impl BuildDataCollector {
     pub(crate) fn add_config(&mut self, workspace_root: &AbsPath, config: BuildDataConfig) {
         self.configs.insert(workspace_root.to_path_buf(), config);
@@ -67,7 +71,7 @@ impl BuildDataCollector {
     pub fn collect(&mut self, progress: &dyn Fn(String)) -> Result<BuildDataResult> {
         let mut res = BuildDataResult::default();
         for (path, config) in self.configs.iter() {
-            res.data.insert(
+            res.per_workspace.insert(
                 path.clone(),
                 collect_from_workspace(
                     &config.cargo_toml,
@@ -81,9 +85,28 @@ impl BuildDataCollector {
     }
 }
 
+impl WorkspaceBuildData {
+    pub(crate) fn get(&self, package_id: &str) -> Option<&PackageBuildData> {
+        self.per_package.get(package_id)
+    }
+}
+
 impl BuildDataResult {
-    pub(crate) fn get(&self, root: &AbsPath) -> Option<&BuildDataMap> {
+    pub(crate) fn get(&self, workspace_root: &AbsPath) -> Option<&WorkspaceBuildData> {
-        self.data.get(&root.to_path_buf())
+        self.per_workspace.get(workspace_root)
+    }
+    pub fn error(&self) -> Option<String> {
+        let mut buf = String::new();
+        for (_workspace_root, build_data) in &self.per_workspace {
+            if let Some(err) = &build_data.error {
+                format_to!(buf, "cargo check failed:\n{}", err);
+            }
+        }
+        if buf.is_empty() {
+            return None;
+        }
+
+        Some(buf)
     }
 }
 
@@ -102,7 +125,7 @@ fn collect_from_workspace(
     cargo_features: &CargoConfig,
     packages: &Vec<cargo_metadata::Package>,
     progress: &dyn Fn(String),
-) -> Result<BuildDataMap> {
+) -> Result<WorkspaceBuildData> {
     let mut cmd = Command::new(toolchain::cargo());
     cmd.args(&["check", "--workspace", "--message-format=json", "--manifest-path"])
         .arg(cargo_toml.as_ref());
@@ -130,13 +153,13 @@ fn collect_from_workspace(
         }
     }
 
-    cmd.stdout(Stdio::piped()).stderr(Stdio::null()).stdin(Stdio::null());
+    cmd.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null());
 
     let mut child = cmd.spawn().map(JodChild)?;
     let child_stdout = child.stdout.take().unwrap();
     let stdout = BufReader::new(child_stdout);
 
-    let mut res = BuildDataMap::default();
+    let mut res = WorkspaceBuildData::default();
     for message in cargo_metadata::Message::parse_stream(stdout).flatten() {
         match message {
             Message::BuildScriptExecuted(BuildScript {
@@ -154,16 +177,17 @@ fn collect_from_workspace(
                     }
                     acc
                 };
-                let res = res.entry(package_id.repr.clone()).or_default();
+                let package_build_data =
+                    res.per_package.entry(package_id.repr.clone()).or_default();
                 // cargo_metadata crate returns default (empty) path for
                 // older cargos, which is not absolute, so work around that.
                 if !out_dir.as_str().is_empty() {
                     let out_dir = AbsPathBuf::assert(PathBuf::from(out_dir.into_os_string()));
-                    res.out_dir = Some(out_dir);
+                    package_build_data.out_dir = Some(out_dir);
-                    res.cfgs = cfgs;
+                    package_build_data.cfgs = cfgs;
                 }
 
-                res.envs = env;
+                package_build_data.envs = env;
             }
             Message::CompilerArtifact(message) => {
                 progress(format!("metadata {}", message.target.name));
@@ -173,8 +197,9 @@ fn collect_from_workspace(
                     // Skip rmeta file
                     if let Some(filename) = message.filenames.iter().find(|name| is_dylib(name)) {
                         let filename = AbsPathBuf::assert(PathBuf::from(&filename));
-                        let res = res.entry(package_id.repr.clone()).or_default();
+                        let package_build_data =
-                        res.proc_macro_dylib_path = Some(filename);
+                            res.per_package.entry(package_id.repr.clone()).or_default();
+                        package_build_data.proc_macro_dylib_path = Some(filename);
                     }
                 }
             }
@@ -188,16 +213,25 @@ fn collect_from_workspace(
     }
 
     for package in packages {
-        let build_data = res.entry(package.id.repr.clone()).or_default();
+        let package_build_data = res.per_package.entry(package.id.repr.clone()).or_default();
-        inject_cargo_env(package, build_data);
+        inject_cargo_env(package, package_build_data);
-        if let Some(out_dir) = &build_data.out_dir {
+        if let Some(out_dir) = &package_build_data.out_dir {
             // NOTE: cargo and rustc seem to hide non-UTF-8 strings from env! and option_env!()
             if let Some(out_dir) = out_dir.to_str().map(|s| s.to_owned()) {
-                build_data.envs.push(("OUT_DIR".to_string(), out_dir));
+                package_build_data.envs.push(("OUT_DIR".to_string(), out_dir));
             }
         }
     }
 
+    let output = child.into_inner().wait_with_output()?;
+    if !output.status.success() {
+        let mut stderr = String::from_utf8(output.stderr).unwrap_or_default();
+        if stderr.is_empty() {
+            stderr = "cargo check failed".to_string();
+        }
+        res.error = Some(stderr)
+    }
+
     Ok(res)
 }
 
@@ -212,7 +246,7 @@ fn is_dylib(path: &Utf8Path) -> bool {
 /// Recreates the compile-time environment variables that Cargo sets.
 ///
 /// Should be synced with <https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates>
-fn inject_cargo_env(package: &cargo_metadata::Package, build_data: &mut BuildData) {
+fn inject_cargo_env(package: &cargo_metadata::Package, build_data: &mut PackageBuildData) {
     let env = &mut build_data.envs;
 
     // FIXME: Missing variables:

crates/project_model/src/workspace.rs

@@ -12,7 +12,7 @@ use proc_macro_api::ProcMacroClient;
 use rustc_hash::{FxHashMap, FxHashSet};
 
 use crate::{
-    build_data::{BuildData, BuildDataMap, BuildDataResult},
+    build_data::{BuildDataResult, PackageBuildData, WorkspaceBuildData},
     cargo_workspace,
     cfg_flag::CfgFlag,
     rustc_cfg,
@@ -354,10 +354,10 @@ fn cargo_to_crate_graph(
     proc_macro_loader: &dyn Fn(&Path) -> Vec<ProcMacro>,
     load: &mut dyn FnMut(&AbsPath) -> Option<FileId>,
     cargo: &CargoWorkspace,
-    build_data_map: Option<&BuildDataMap>,
+    build_data_map: Option<&WorkspaceBuildData>,
     sysroot: &Sysroot,
     rustc: &Option<CargoWorkspace>,
-    rustc_build_data_map: Option<&BuildDataMap>,
+    rustc_build_data_map: Option<&WorkspaceBuildData>,
 ) -> CrateGraph {
     let _p = profile::span("cargo_to_crate_graph");
     let mut crate_graph = CrateGraph::default();
@@ -464,7 +464,7 @@ fn handle_rustc_crates(
     rustc_workspace: &CargoWorkspace,
     load: &mut dyn FnMut(&AbsPath) -> Option<FileId>,
     crate_graph: &mut CrateGraph,
-    rustc_build_data_map: Option<&FxHashMap<String, BuildData>>,
+    rustc_build_data_map: Option<&WorkspaceBuildData>,
     cfg_options: &CfgOptions,
     proc_macro_loader: &dyn Fn(&Path) -> Vec<ProcMacro>,
     pkg_to_lib_crate: &mut FxHashMap<la_arena::Idx<crate::PackageData>, CrateId>,
@@ -555,7 +555,7 @@ fn handle_rustc_crates(
 fn add_target_crate_root(
     crate_graph: &mut CrateGraph,
     pkg: &cargo_workspace::PackageData,
-    build_data: Option<&BuildData>,
+    build_data: Option<&PackageBuildData>,
     cfg_options: &CfgOptions,
     proc_macro_loader: &dyn Fn(&Path) -> Vec<ProcMacro>,
     file_id: FileId,

crates/rust-analyzer/src/reload.rs

@@ -109,6 +109,11 @@ impl GlobalState {
             quiescent: self.is_quiescent(),
             message: None,
         };
+
+        if let Some(error) = self.build_data_error() {
+            status.health = lsp_ext::Health::Warning;
+            status.message = Some(error)
+        }
         if !self.config.cargo_autoreload()
             && self.is_quiescent()
             && self.fetch_workspaces_queue.op_requested()
@@ -116,9 +121,10 @@ impl GlobalState {
             status.health = lsp_ext::Health::Warning;
             status.message = Some("Workspace reload required".to_string())
         }
-        if let Some(error) = self.loading_error() {
+
+        if let Some(error) = self.fetch_workspace_error() {
             status.health = lsp_ext::Health::Error;
-            status.message = Some(format!("Workspace reload failed: {}", error))
+            status.message = Some(error)
         }
 
         if self.last_reported_status.as_ref() != Some(&status) {
@@ -217,7 +223,7 @@ impl GlobalState {
         let _p = profile::span("GlobalState::switch_workspaces");
         log::info!("will switch workspaces");
 
-        if let Some(error_message) = self.loading_error() {
+        if let Some(error_message) = self.fetch_workspace_error() {
             log::error!("failed to switch workspaces: {}", error_message);
             self.show_message(lsp_types::MessageType::Error, error_message);
             if !self.workspaces.is_empty() {
@@ -225,6 +231,11 @@ impl GlobalState {
             }
         }
 
+        if let Some(error_message) = self.build_data_error() {
+            log::error!("failed to switch build data: {}", error_message);
+            self.show_message(lsp_types::MessageType::Error, error_message);
+        }
+
         let workspaces = self
             .fetch_workspaces_queue
             .last_op_result()
@@ -343,22 +354,30 @@ impl GlobalState {
         log::info!("did switch workspaces");
     }
 
-    fn loading_error(&self) -> Option<String> {
+    fn fetch_workspace_error(&self) -> Option<String> {
-        let mut message = None;
+        let mut buf = String::new();
 
         for ws in self.fetch_workspaces_queue.last_op_result() {
             if let Err(err) = ws {
-                let message = message.get_or_insert_with(String::new);
+                stdx::format_to!(buf, "rust-analyzer failed to load workspace: {:#}\n", err);
-                stdx::format_to!(message, "rust-analyzer failed to load workspace: {:#}\n", err);
             }
         }
 
-        if let Some(Err(err)) = self.fetch_build_data_queue.last_op_result() {
+        if buf.is_empty() {
-            let message = message.get_or_insert_with(String::new);
+            return None;
-            stdx::format_to!(message, "rust-analyzer failed to fetch build data: {:#}\n", err);
         }
 
-        message
+        Some(buf)
+    }
+
+    fn build_data_error(&self) -> Option<String> {
+        match self.fetch_build_data_queue.last_op_result() {
+            Some(Err(err)) => {
+                Some(format!("rust-analyzer failed to fetch build data: {:#}\n", err))
+            }
+            Some(Ok(data)) => data.error(),
+            None => None,
+        }
     }
 
     fn reload_flycheck(&mut self) {

crates/stdx/src/lib.rs

@@ -178,6 +178,7 @@ where
     start..start + len
 }
 
+#[repr(transparent)]
 pub struct JodChild(pub process::Child);
 
 impl ops::Deref for JodChild {
@@ -200,6 +201,13 @@ impl Drop for JodChild {
     }
 }
 
+impl JodChild {
+    pub fn into_inner(self) -> process::Child {
+        // SAFETY: repr transparent
+        unsafe { std::mem::transmute::<JodChild, process::Child>(self) }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;