Auto merge of #17956 - Veykril:metadata-err, r=Veykril

bors · bors · commit f4dbbac7cab6 · 2024-08-25T07:30:09.000Z
fix: Fix metadata retrying eating original errors
diff --git a/crates/project-model/src/cargo_workspace.rs b/crates/project-model/src/cargo_workspace.rs
@@ -33,8 +33,6 @@ pub struct CargoWorkspace {
     workspace_root: AbsPathBuf,
     target_directory: AbsPathBuf,
     manifest_path: ManifestPath,
-    // Whether this workspace was queried with `--no-deps`.
-    no_deps: bool,
 }
 
 impl ops::Index<Package> for CargoWorkspace {
@@ -253,14 +251,17 @@ struct PackageMetadata {
 }
 
 impl CargoWorkspace {
+    /// Fetches the metadata for the given `cargo_toml` manifest.
+    /// A successful result may contain another metadata error if the initial fetching failed but
+    /// the `--no-deps` retry succeeded.
     pub fn fetch_metadata(
         cargo_toml: &ManifestPath,
         current_dir: &AbsPath,
         config: &CargoConfig,
         sysroot: &Sysroot,
         locked: bool,
         progress: &dyn Fn(String),
-    ) -> anyhow::Result<cargo_metadata::Metadata> {
+    ) -> anyhow::Result<(cargo_metadata::Metadata, Option<anyhow::Error>)> {
         Self::fetch_metadata_(cargo_toml, current_dir, config, sysroot, locked, false, progress)
     }
 
@@ -272,7 +273,7 @@ impl CargoWorkspace {
         locked: bool,
         no_deps: bool,
         progress: &dyn Fn(String),
-    ) -> anyhow::Result<cargo_metadata::Metadata> {
+    ) -> anyhow::Result<(cargo_metadata::Metadata, Option<anyhow::Error>)> {
         let targets = find_list_of_build_targets(config, cargo_toml, sysroot);
 
         let cargo = sysroot.tool(Tool::Cargo);
@@ -337,13 +338,17 @@ impl CargoWorkspace {
         // unclear whether cargo itself supports it.
         progress("metadata".to_owned());
 
-        (|| -> Result<cargo_metadata::Metadata, cargo_metadata::Error> {
+        (|| -> anyhow::Result<(_, _)> {
             let output = meta.cargo_command().output()?;
             if !output.status.success() {
+                let error = cargo_metadata::Error::CargoMetadata {
+                    stderr: String::from_utf8(output.stderr)?,
+                }
+                .into();
                 if !no_deps {
                     // If we failed to fetch metadata with deps, try again without them.
                     // This makes r-a still work partially when offline.
-                    if let Ok(metadata) = Self::fetch_metadata_(
+                    if let Ok((metadata, _)) = Self::fetch_metadata_(
                         cargo_toml,
                         current_dir,
                         config,
@@ -352,20 +357,23 @@ impl CargoWorkspace {
                         true,
                         progress,
                     ) {
-                        return Ok(metadata);
+                        return Ok((metadata, Some(error)));
                     }
                 }
-
-                return Err(cargo_metadata::Error::CargoMetadata {
-                    stderr: String::from_utf8(output.stderr)?,
-                });
+                return Err(error);
             }
             let stdout = from_utf8(&output.stdout)?
                 .lines()
                 .find(|line| line.starts_with('{'))
                 .ok_or(cargo_metadata::Error::NoJson)?;
-            cargo_metadata::MetadataCommand::parse(stdout)
+            Ok((cargo_metadata::MetadataCommand::parse(stdout)?, None))
         })()
+        .map(|(metadata, error)| {
+            (
+                metadata,
+                error.map(|e| e.context(format!("Failed to run `{:?}`", meta.cargo_command()))),
+            )
+        })
         .with_context(|| format!("Failed to run `{:?}`", meta.cargo_command()))
     }
 
@@ -463,7 +471,6 @@ impl CargoWorkspace {
                 pkg_data.targets.push(tgt);
             }
         }
-        let no_deps = meta.resolve.is_none();
         for mut node in meta.resolve.map_or_else(Vec::new, |it| it.nodes) {
             let &source = pkg_by_id.get(&node.id).unwrap();
             node.deps.sort_by(|a, b| a.pkg.cmp(&b.pkg));
@@ -483,14 +490,7 @@ impl CargoWorkspace {
 
         let target_directory = AbsPathBuf::assert(meta.target_directory);
 
-        CargoWorkspace {
-            packages,
-            targets,
-            workspace_root,
-            target_directory,
-            manifest_path,
-            no_deps,
-        }
+        CargoWorkspace { packages, targets, workspace_root, target_directory, manifest_path }
     }
 
     pub fn packages(&self) -> impl ExactSizeIterator<Item = Package> + '_ {
@@ -572,10 +572,6 @@ impl CargoWorkspace {
     fn is_unique(&self, name: &str) -> bool {
         self.packages.iter().filter(|(_, v)| v.name == name).count() == 1
     }
-
-    pub fn no_deps(&self) -> bool {
-        self.no_deps
-    }
 }
 
 fn find_list_of_build_targets(
diff --git a/crates/project-model/src/sysroot.rs b/crates/project-model/src/sysroot.rs
@@ -325,7 +325,7 @@ impl Sysroot {
             "nightly".to_owned(),
         );
 
-        let mut res = match CargoWorkspace::fetch_metadata(
+        let (mut res, _) = match CargoWorkspace::fetch_metadata(
             &library_manifest,
             sysroot_src_dir,
             &cargo_config,
diff --git a/crates/project-model/src/tests.rs b/crates/project-model/src/tests.rs
@@ -34,6 +34,7 @@ fn load_cargo_with_overrides(
             build_scripts: WorkspaceBuildScripts::default(),
             rustc: Err(None),
             cargo_config_extra_env: Default::default(),
+            error: None,
         },
         cfg_overrides,
         sysroot: Sysroot::empty(),
@@ -58,6 +59,7 @@ fn load_cargo_with_fake_sysroot(
             build_scripts: WorkspaceBuildScripts::default(),
             rustc: Err(None),
             cargo_config_extra_env: Default::default(),
+            error: None,
         },
         sysroot: get_fake_sysroot(),
         rustc_cfg: Vec::new(),
@@ -300,6 +302,7 @@ fn smoke_test_real_sysroot_cargo() {
             build_scripts: WorkspaceBuildScripts::default(),
             rustc: Err(None),
             cargo_config_extra_env: Default::default(),
+            error: None,
         },
         sysroot,
         rustc_cfg: Vec::new(),
diff --git a/crates/project-model/src/workspace.rs b/crates/project-model/src/workspace.rs
@@ -69,6 +69,8 @@ pub enum ProjectWorkspaceKind {
     Cargo {
         /// The workspace as returned by `cargo metadata`.
         cargo: CargoWorkspace,
+        /// Additional `cargo metadata` error. (only populated if retried fetching via `--no-deps` succeeded).
+        error: Option<Arc<anyhow::Error>>,
         /// The build script results for the workspace.
         build_scripts: WorkspaceBuildScripts,
         /// The rustc workspace loaded for this workspace. An `Err(None)` means loading has been
@@ -93,7 +95,7 @@ pub enum ProjectWorkspaceKind {
         /// The file in question.
         file: ManifestPath,
         /// Is this file a cargo script file?
-        cargo: Option<(CargoWorkspace, WorkspaceBuildScripts)>,
+        cargo: Option<(CargoWorkspace, WorkspaceBuildScripts, Option<Arc<anyhow::Error>>)>,
         /// Environment variables set in the `.cargo/config` file.
         cargo_config_extra_env: FxHashMap<String, String>,
     },
@@ -106,6 +108,7 @@ impl fmt::Debug for ProjectWorkspace {
         match kind {
             ProjectWorkspaceKind::Cargo {
                 cargo,
+                error: _,
                 build_scripts: _,
                 rustc,
                 cargo_config_extra_env,
@@ -256,7 +259,7 @@ impl ProjectWorkspace {
                         false,
                         progress,
                     ) {
-                        Ok(meta) => {
+                        Ok((meta, _error)) => {
                             let workspace = CargoWorkspace::new(meta, cargo_toml.clone());
                             let buildscripts = WorkspaceBuildScripts::rustc_crates(
                                 &workspace,
@@ -301,7 +304,7 @@ impl ProjectWorkspace {
                     tracing::error!(%e, "failed fetching data layout for {cargo_toml:?} workspace");
                 }
 
-                let meta = CargoWorkspace::fetch_metadata(
+                let (meta, error) = CargoWorkspace::fetch_metadata(
                     cargo_toml,
                     cargo_toml.parent(),
                     config,
@@ -324,6 +327,7 @@ impl ProjectWorkspace {
                         build_scripts: WorkspaceBuildScripts::default(),
                         rustc,
                         cargo_config_extra_env,
+                        error: error.map(Arc::new),
                     },
                     sysroot,
                     rustc_cfg,
@@ -404,10 +408,11 @@ impl ProjectWorkspace {
         let cargo_script =
             CargoWorkspace::fetch_metadata(detached_file, dir, config, &sysroot, false, &|_| ())
                 .ok()
-                .map(|ws| {
+                .map(|(ws, error)| {
                     (
                         CargoWorkspace::new(ws, detached_file.clone()),
                         WorkspaceBuildScripts::default(),
+                        error.map(Arc::new),
                     )
                 });
 
@@ -440,10 +445,8 @@ impl ProjectWorkspace {
         progress: &dyn Fn(String),
     ) -> anyhow::Result<WorkspaceBuildScripts> {
         match &self.kind {
-            ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _)), .. }
-            | ProjectWorkspaceKind::Cargo { cargo, .. }
-                if !cargo.no_deps() =>
-            {
+            ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, None)), .. }
+            | ProjectWorkspaceKind::Cargo { cargo, error: None, .. } => {
                 WorkspaceBuildScripts::run_for_workspace(config, cargo, progress, &self.sysroot)
                     .with_context(|| {
                         format!("Failed to run build scripts for {}", cargo.workspace_root())
@@ -502,7 +505,7 @@ impl ProjectWorkspace {
     pub fn set_build_scripts(&mut self, bs: WorkspaceBuildScripts) {
         match &mut self.kind {
             ProjectWorkspaceKind::Cargo { build_scripts, .. }
-            | ProjectWorkspaceKind::DetachedFile { cargo: Some((_, build_scripts)), .. } => {
+            | ProjectWorkspaceKind::DetachedFile { cargo: Some((_, build_scripts, _)), .. } => {
                 *build_scripts = bs
             }
             _ => assert_eq!(bs, WorkspaceBuildScripts::default()),
@@ -593,6 +596,7 @@ impl ProjectWorkspace {
                 rustc,
                 build_scripts,
                 cargo_config_extra_env: _,
+                error: _,
             } => {
                 cargo
                     .packages()
@@ -648,7 +652,7 @@ impl ProjectWorkspace {
                     include: vec![file.to_path_buf()],
                     exclude: Vec::new(),
                 })
-                .chain(cargo_script.iter().flat_map(|(cargo, build_scripts)| {
+                .chain(cargo_script.iter().flat_map(|(cargo, build_scripts, _)| {
                     cargo.packages().map(|pkg| {
                         let is_local = cargo[pkg].is_local;
                         let pkg_root = cargo[pkg].manifest.parent().to_path_buf();
@@ -703,7 +707,7 @@ impl ProjectWorkspace {
             }
             ProjectWorkspaceKind::DetachedFile { cargo: cargo_script, .. } => {
                 sysroot_package_len
-                    + cargo_script.as_ref().map_or(1, |(cargo, _)| cargo.packages().len())
+                    + cargo_script.as_ref().map_or(1, |(cargo, _, _)| cargo.packages().len())
             }
         }
     }
@@ -733,6 +737,7 @@ impl ProjectWorkspace {
                 rustc,
                 build_scripts,
                 cargo_config_extra_env: _,
+                error: _,
             } => (
                 cargo_to_crate_graph(
                     load,
@@ -746,7 +751,7 @@ impl ProjectWorkspace {
                 sysroot,
             ),
             ProjectWorkspaceKind::DetachedFile { file, cargo: cargo_script, .. } => (
-                if let Some((cargo, build_scripts)) = cargo_script {
+                if let Some((cargo, build_scripts, _)) = cargo_script {
                     cargo_to_crate_graph(
                         &mut |path| load(path),
                         None,
@@ -795,12 +800,14 @@ impl ProjectWorkspace {
                     rustc,
                     cargo_config_extra_env,
                     build_scripts: _,
+                    error: _,
                 },
                 ProjectWorkspaceKind::Cargo {
                     cargo: o_cargo,
                     rustc: o_rustc,
                     cargo_config_extra_env: o_cargo_config_extra_env,
                     build_scripts: _,
+                    error: _,
                 },
             ) => {
                 cargo == o_cargo
@@ -813,12 +820,12 @@ impl ProjectWorkspace {
             (
                 ProjectWorkspaceKind::DetachedFile {
                     file,
-                    cargo: Some((cargo_script, _)),
+                    cargo: Some((cargo_script, _, _)),
                     cargo_config_extra_env,
                 },
                 ProjectWorkspaceKind::DetachedFile {
                     file: o_file,
-                    cargo: Some((o_cargo_script, _)),
+                    cargo: Some((o_cargo_script, _, _)),
                     cargo_config_extra_env: o_cargo_config_extra_env,
                 },
             ) => {
diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs
@@ -667,7 +667,7 @@ impl GlobalStateSnapshot {
         for workspace in self.workspaces.iter() {
             match &workspace.kind {
                 ProjectWorkspaceKind::Cargo { cargo, .. }
-                | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _)), .. } => {
+                | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, _)), .. } => {
                     let Some(target_idx) = cargo.target_by_root(path) else {
                         continue;
                     };
diff --git a/crates/rust-analyzer/src/handlers/notification.rs b/crates/rust-analyzer/src/handlers/notification.rs
@@ -344,7 +344,7 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
                 let package = match &ws.kind {
                     project_model::ProjectWorkspaceKind::Cargo { cargo, .. }
                     | project_model::ProjectWorkspaceKind::DetachedFile {
-                        cargo: Some((cargo, _)),
+                        cargo: Some((cargo, _, _)),
                         ..
                     } => cargo.packages().find_map(|pkg| {
                         let has_target_with_root = cargo[pkg]
diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs
@@ -799,7 +799,7 @@ pub(crate) fn handle_parent_module(
                 .iter()
                 .filter_map(|ws| match &ws.kind {
                     ProjectWorkspaceKind::Cargo { cargo, .. }
-                    | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _)), .. } => {
+                    | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, _)), .. } => {
                         cargo.parent_manifests(&manifest_path)
                     }
                     _ => None,
@@ -1839,7 +1839,7 @@ pub(crate) fn handle_open_docs(
 
     let ws_and_sysroot = snap.workspaces.iter().find_map(|ws| match &ws.kind {
         ProjectWorkspaceKind::Cargo { cargo, .. }
-        | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _)), .. } => {
+        | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, _)), .. } => {
             Some((cargo, &ws.sysroot))
         }
         ProjectWorkspaceKind::Json { .. } => None,
diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs
@@ -180,16 +180,17 @@ impl GlobalState {
                 self.proc_macro_clients.iter().map(Some).chain(iter::repeat_with(|| None));
 
             for (ws, proc_macro_client) in self.workspaces.iter().zip(proc_macro_clients) {
-                if matches!(
-                    &ws.kind,
-                    ProjectWorkspaceKind::Cargo { cargo, .. } | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _)), .. }
-                    if cargo.no_deps()
-                ) {
+                if let ProjectWorkspaceKind::Cargo { error: Some(error), .. }
+                | ProjectWorkspaceKind::DetachedFile {
+                    cargo: Some((_, _, Some(error))), ..
+                } = &ws.kind
+                {
                     status.health |= lsp_ext::Health::Warning;
                     format_to!(
                         message,
-                        "Failed to read Cargo metadata for `{}`, the `Cargo.toml` might be invalid or you have no internet connection.\n\n",
-                        ws.manifest_or_root()
+                        "Failed to read Cargo metadata with dependencies for `{}`: {:#}\n\n",
+                        ws.manifest_or_root(),
+                        error
                     );
                 }
                 if let Some(err) = ws.sysroot.error() {
@@ -822,7 +823,7 @@ impl GlobalState {
                             match &ws.kind {
                                 ProjectWorkspaceKind::Cargo { cargo, .. }
                                 | ProjectWorkspaceKind::DetachedFile {
-                                    cargo: Some((cargo, _)),
+                                    cargo: Some((cargo, _, _)),
                                     ..
                                 } => (cargo.workspace_root(), Some(cargo.manifest_path())),
                                 ProjectWorkspaceKind::Json(project) => {
diff --git a/crates/rust-analyzer/tests/crate_graph.rs b/crates/rust-analyzer/tests/crate_graph.rs
@@ -20,6 +20,7 @@ fn load_cargo_with_fake_sysroot(file: &str) -> ProjectWorkspace {
             build_scripts: WorkspaceBuildScripts::default(),
             rustc: Err(None),
             cargo_config_extra_env: Default::default(),
+            error: None,
         },
         sysroot: get_fake_sysroot(),
         rustc_cfg: Vec::new(),
