Auto merge of #11960 - Veykril:config-valid, r=Veykril

internal: Show more project building errors to the user

Should help out with https://github.com/rust-analyzer/rust-analyzer/issues/9720
Fixes https://github.com/rust-analyzer/rust-analyzer/issues/11223

crates/flycheck/src/lib.rs

@@ -329,7 +329,7 @@ impl CargoActor {
             Ok(output) if output.status.success() => Ok(()),
             Ok(output)  => {
                 Err(io::Error::new(io::ErrorKind::Other, format!(
-                    "Cargo watcher failed, the command produced no valid metadata (exit code: {:?})\nCargo's stderr output:\n{}",
+                    "Cargo watcher failed, the command produced no valid metadata (exit code: {:?}):\n{}",
                     output.status, error
                 )))
             }

crates/project_model/src/build_scripts.rs

@@ -7,11 +7,11 @@
 //! here, but it covers procedural macros as well.
 
 use std::{
+    io,
     path::PathBuf,
     process::{Command, Stdio},
 };
 
-use anyhow::Result;
 use cargo_metadata::{camino::Utf8Path, Message};
 use la_arena::ArenaMap;
 use paths::AbsPathBuf;
@@ -80,7 +80,7 @@ impl WorkspaceBuildScripts {
         config: &CargoConfig,
         workspace: &CargoWorkspace,
         progress: &dyn Fn(String),
-    ) -> Result<WorkspaceBuildScripts> {
+    ) -> io::Result<WorkspaceBuildScripts> {
         let mut cmd = Self::build_command(config);
 
         if config.wrap_rustc_in_build_scripts {
@@ -107,12 +107,12 @@ impl WorkspaceBuildScripts {
             by_id.insert(workspace[package].id.clone(), package);
         }
 
-        let mut callback_err = None;
+        let mut cfg_err = None;
         let mut stderr = String::new();
         let output = stdx::process::streaming_output(
             cmd,
             &mut |line| {
-                if callback_err.is_some() {
+                if cfg_err.is_some() {
                     return;
                 }
 
@@ -126,7 +126,7 @@ impl WorkspaceBuildScripts {
                 match message {
                     Message::BuildScriptExecuted(message) => {
                         let package = match by_id.get(&message.package_id.repr) {
-                            Some(it) => *it,
+                            Some(&it) => it,
                             None => return,
                         };
                         let cfgs = {
@@ -135,7 +135,7 @@ impl WorkspaceBuildScripts {
                                 match cfg.parse::<CfgFlag>() {
                                     Ok(it) => acc.push(it),
                                     Err(err) => {
-                                        callback_err = Some(anyhow::format_err!(
+                                        cfg_err = Some(format!(
                                             "invalid cfg from cargo-metadata: {}",
                                             err
                                         ));
@@ -191,6 +191,11 @@ impl WorkspaceBuildScripts {
 
         for package in workspace.packages() {
             let package_build_data = &mut res.outputs[package];
+            tracing::info!(
+                "{} BuildScriptOutput: {:?}",
+                workspace[package].manifest.parent().display(),
+                package_build_data,
+            );
             // inject_cargo_env(package, package_build_data);
             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!()
@@ -200,6 +205,11 @@ impl WorkspaceBuildScripts {
             }
         }
 
+        if let Some(cfg_err) = cfg_err {
+            stderr.push_str(&cfg_err);
+            stderr.push('\n');
+        }
+
         if !output.status.success() {
             if stderr.is_empty() {
                 stderr = "cargo check failed".to_string();

crates/project_model/src/workspace.rs

@@ -256,7 +256,9 @@ impl ProjectWorkspace {
     ) -> Result<WorkspaceBuildScripts> {
         match self {
             ProjectWorkspace::Cargo { cargo, .. } => {
-                WorkspaceBuildScripts::run(config, cargo, progress)
+                WorkspaceBuildScripts::run(config, cargo, progress).with_context(|| {
+                    format!("Failed to run build scripts for {}", &cargo.workspace_root().display())
+                })
             }
             ProjectWorkspace::Json { .. } | ProjectWorkspace::DetachedFiles { .. } => {
                 Ok(WorkspaceBuildScripts::default())

crates/rust-analyzer/src/bin/main.rs

@@ -119,7 +119,9 @@ fn setup_logging(log_file: Option<&Path>) -> Result<()> {
         None => None,
     };
     let filter = env::var("RA_LOG").ok();
-    logger::Logger::new(log_file, filter.as_deref()).install()?;
+    // deliberately enable all `error` logs if the user has not set RA_LOG, as there is usually useful
+    // information in there for debugging
+    logger::Logger::new(log_file, filter.as_deref().or(Some("error"))).install()?;
 
     profile::init();
 

crates/rust-analyzer/src/global_state.rs

@@ -190,6 +190,7 @@ impl GlobalState {
 
             for file in changed_files {
                 if !file.is_created_or_deleted() {
+                    // FIXME: https://github.com/rust-analyzer/rust-analyzer/issues/11357
                     let crates = self.analysis_host.raw_database().relevant_crates(file.file_id);
                     let crate_graph = self.analysis_host.raw_database().crate_graph();
 
@@ -255,6 +256,7 @@ impl GlobalState {
         let request = self.req_queue.outgoing.register(R::METHOD.to_string(), params, handler);
         self.send(request.into());
     }
+
     pub(crate) fn complete_request(&mut self, response: lsp_server::Response) {
         let handler = self
             .req_queue
@@ -281,6 +283,7 @@ impl GlobalState {
             .incoming
             .register(request.id.clone(), (request.method.clone(), request_received));
     }
+
     pub(crate) fn respond(&mut self, response: lsp_server::Response) {
         if let Some((method, start)) = self.req_queue.incoming.complete(response.id.clone()) {
             if let Some(err) = &response.error {
@@ -294,6 +297,7 @@ impl GlobalState {
             self.send(response.into());
         }
     }
+
     pub(crate) fn cancel(&mut self, request_id: lsp_server::RequestId) {
         if let Some(response) = self.req_queue.incoming.cancel(request_id) {
             self.send(response.into());
@@ -307,7 +311,7 @@ impl GlobalState {
 
 impl Drop for GlobalState {
     fn drop(&mut self) {
-        self.analysis_host.request_cancellation()
+        self.analysis_host.request_cancellation();
     }
 }
 

crates/rust-analyzer/src/lsp_utils.rs

@@ -47,6 +47,26 @@ impl GlobalState {
         )
     }
 
+    /// Sends a notification to the client containing the error `message`.
+    /// If `additional_info` is [`Some`], appends a note to the notification telling to check the logs.
+    /// This will always log `message` + `additional_info` to the server's error log.
+    pub(crate) fn show_and_log_error(&mut self, message: String, additional_info: Option<String>) {
+        let mut message = message;
+        match additional_info {
+            Some(additional_info) => {
+                tracing::error!("{}\n\n{}", &message, &additional_info);
+                if tracing::enabled!(tracing::Level::ERROR) {
+                    message.push_str("\n\nCheck the server logs for additional info.");
+                }
+            }
+            None => tracing::error!("{}", &message),
+        }
+
+        self.send_notification::<lsp_types::notification::ShowMessage>(
+            lsp_types::ShowMessageParams { typ: lsp_types::MessageType::ERROR, message },
+        )
+    }
+
     /// rust-analyzer is resilient -- if it fails, this doesn't usually affect
     /// the user experience. Part of that is that we deliberately hide panics
     /// from the user.

crates/rust-analyzer/src/main_loop.rs

@@ -111,10 +111,7 @@ impl GlobalState {
             && self.config.detached_files().is_empty()
             && self.config.notifications().cargo_toml_not_found
         {
-            self.show_message(
+            self.show_and_log_error("rust-analyzer failed to discover workspace".to_string(), None);
-                lsp_types::MessageType::ERROR,
-                "rust-analyzer failed to discover workspace".to_string(),
-            );
         };
 
         if self.config.did_save_text_document_dynamic_registration() {
@@ -406,9 +403,9 @@ impl GlobalState {
                                 flycheck::Progress::DidCancel => (Progress::End, None),
                                 flycheck::Progress::DidFinish(result) => {
                                     if let Err(err) = result {
-                                        self.show_message(
+                                        self.show_and_log_error(
-                                            lsp_types::MessageType::ERROR,
+                                            "cargo check failed".to_string(),
-                                            format!("cargo check failed: {}", err),
+                                            Some(err.to_string()),
                                         );
                                     }
                                     (Progress::End, None)
@@ -564,7 +561,6 @@ impl GlobalState {
         if self.workspaces.is_empty() && !self.is_quiescent() {
             self.respond(lsp_server::Response::new_err(
                 req.id,
-                // FIXME: i32 should impl From<ErrorCode> (from() guarantees lossless conversion)
                 lsp_server::ErrorCode::ContentModified as i32,
                 "waiting for cargo metadata or cargo check".to_owned(),
             ));

crates/rust-analyzer/src/reload.rs

@@ -72,9 +72,10 @@ impl GlobalState {
             status.message =
                 Some("Reload required due to source changes of a procedural macro.".into())
         }
-        if let Some(error) = self.fetch_build_data_error() {
+        if let Err(_) = self.fetch_build_data_error() {
             status.health = lsp_ext::Health::Warning;
-            status.message = Some(error)
+            status.message =
+                Some("Failed to run build scripts of some packages, check the logs.".to_string());
         }
         if !self.config.cargo_autoreload()
             && self.is_quiescent()
@@ -84,7 +85,7 @@ impl GlobalState {
             status.message = Some("Workspace reload required".to_string())
         }
 
-        if let Some(error) = self.fetch_workspace_error() {
+        if let Err(error) = self.fetch_workspace_error() {
             status.health = lsp_ext::Health::Error;
             status.message = Some(error)
         }
@@ -167,8 +168,8 @@ impl GlobalState {
         let _p = profile::span("GlobalState::switch_workspaces");
         tracing::info!("will switch workspaces");
 
-        if let Some(error_message) = self.fetch_workspace_error() {
+        if let Err(error_message) = self.fetch_workspace_error() {
-            tracing::error!("failed to switch workspaces: {}", error_message);
+            self.show_and_log_error(error_message, None);
             if !self.workspaces.is_empty() {
                 // It only makes sense to switch to a partially broken workspace
                 // if we don't have any workspace at all yet.
@@ -176,8 +177,11 @@ impl GlobalState {
             }
         }
 
-        if let Some(error_message) = self.fetch_build_data_error() {
+        if let Err(error) = self.fetch_build_data_error() {
-            tracing::error!("failed to switch build data: {}", error_message);
+            self.show_and_log_error(
+                "rust-analyzer failed to run build scripts".to_string(),
+                Some(error),
+            );
         }
 
         let workspaces = self
@@ -277,20 +281,18 @@ impl GlobalState {
         let project_folders = ProjectFolders::new(&self.workspaces, &files_config.exclude);
 
         if self.proc_macro_client.is_none() {
-            self.proc_macro_client = match self.config.proc_macro_srv() {
+            if let Some((path, args)) = self.config.proc_macro_srv() {
-                None => None,
+                match ProcMacroServer::spawn(path.clone(), args) {
-                Some((path, args)) => match ProcMacroServer::spawn(path.clone(), args) {
+                    Ok(it) => self.proc_macro_client = Some(it),
-                    Ok(it) => Some(it),
                     Err(err) => {
                         tracing::error!(
                             "Failed to run proc_macro_srv from path {}, error: {:?}",
                             path.display(),
                             err
                         );
-                        None
                     }
-                },
+                }
-            };
+            }
         }
 
         let watch = match files_config.watcher {
@@ -348,7 +350,7 @@ impl GlobalState {
         tracing::info!("did switch workspaces");
     }
 
-    fn fetch_workspace_error(&self) -> Option<String> {
+    fn fetch_workspace_error(&self) -> Result<(), String> {
         let mut buf = String::new();
 
         for ws in self.fetch_workspaces_queue.last_op_result() {
@@ -358,35 +360,30 @@ impl GlobalState {
         }
 
         if buf.is_empty() {
-            return None;
+            return Ok(());
         }
 
-        Some(buf)
+        Err(buf)
     }
 
-    fn fetch_build_data_error(&self) -> Option<String> {
+    fn fetch_build_data_error(&self) -> Result<(), String> {
-        let mut buf = "rust-analyzer failed to run build scripts:\n".to_string();
+        let mut buf = String::new();
-        let mut has_errors = false;
 
         for ws in &self.fetch_build_data_queue.last_op_result().1 {
             match ws {
-                Ok(data) => {
+                Ok(data) => match data.error() {
-                    if let Some(err) = data.error() {
+                    Some(stderr) => stdx::format_to!(buf, "{:#}\n", stderr),
-                        has_errors = true;
+                    _ => (),
-                        stdx::format_to!(buf, "{:#}\n", err);
+                },
-                    }
+                // io errors
-                }
+                Err(err) => stdx::format_to!(buf, "{:#}\n", err),
-                Err(err) => {
-                    has_errors = true;
-                    stdx::format_to!(buf, "{:#}\n", err);
-                }
             }
         }
 
-        if has_errors {
+        if buf.is_empty() {
-            Some(buf)
+            Ok(())
         } else {
-            None
+            Err(buf)
         }
     }