Merge #9638

9638: internal: replace some unwraps with types r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

Author: bors[bot]

crates/project_model/src/cargo_workspace.rs

@@ -1,8 +1,9 @@
 //! See [`CargoWorkspace`].
 
+use std::convert::TryInto;
 use std::iter;
 use std::path::PathBuf;
-use std::{convert::TryInto, ops, process::Command};
+use std::{ops, process::Command};
 
 use anyhow::{Context, Result};
 use base_db::Edition;
@@ -13,8 +14,8 @@ use rustc_hash::FxHashMap;
 use serde::Deserialize;
 use serde_json::from_value;
 
-use crate::utf8_stdout;
 use crate::CfgOverrides;
+use crate::{utf8_stdout, ManifestPath};
 
 /// [`CargoWorkspace`] represents the logical structure of, well, a Cargo
 /// workspace. It pretty closely mirrors `cargo metadata` output.
@@ -108,7 +109,7 @@ pub struct PackageData {
     /// Name as given in the `Cargo.toml`
     pub name: String,
     /// Path containing the `Cargo.toml`
-    pub manifest: AbsPathBuf,
+    pub manifest: ManifestPath,
     /// Targets provided by the crate (lib, bin, example, test, ...)
     pub targets: Vec<Target>,
     /// Is this package a member of the current workspace
@@ -215,12 +216,6 @@ impl TargetKind {
     }
 }
 
-impl PackageData {
-    pub fn root(&self) -> &AbsPath {
-        self.manifest.parent().unwrap()
-    }
-}
-
 #[derive(Deserialize, Default)]
 // Deserialise helper for the cargo metadata
 struct PackageMetadata {
@@ -230,10 +225,16 @@ struct PackageMetadata {
 
 impl CargoWorkspace {
     pub fn fetch_metadata(
-        cargo_toml: &AbsPath,
+        cargo_toml: &ManifestPath,
         config: &CargoConfig,
         progress: &dyn Fn(String),
     ) -> Result<cargo_metadata::Metadata> {
+        let target = config
+            .target
+            .clone()
+            .or_else(|| cargo_config_build_target(cargo_toml))
+            .or_else(|| rustc_discover_host_triple(cargo_toml));
+
         let mut meta = MetadataCommand::new();
         meta.cargo_path(toolchain::cargo());
         meta.manifest_path(cargo_toml.to_path_buf());
@@ -249,16 +250,8 @@ impl CargoWorkspace {
                 meta.features(CargoOpt::SomeFeatures(config.features.clone()));
             }
         }
-        if let Some(parent) = cargo_toml.parent() {
-            meta.current_dir(parent.to_path_buf());
-        }
-        let target = if let Some(target) = &config.target {
-            Some(target.clone())
-        } else if let stdout @ Some(_) = cargo_config_build_target(cargo_toml) {
-            stdout
-        } else {
-            rustc_discover_host_triple(cargo_toml)
-        };
+        meta.current_dir(cargo_toml.parent().as_os_str());
+
         if let Some(target) = target {
             meta.other_options(vec![String::from("--filter-platform"), target]);
         }
@@ -269,21 +262,7 @@ impl CargoWorkspace {
         progress("metadata".to_string());
 
         let meta = meta.exec().with_context(|| {
-            let cwd: Option<AbsPathBuf> =
-                std::env::current_dir().ok().and_then(|p| p.try_into().ok());
-
-            let workdir = cargo_toml
-                .parent()
-                .map(|p| p.to_path_buf())
-                .or(cwd)
-                .map(|dir| format!(" in `{}`", dir.display()))
-                .unwrap_or_default();
-
-            format!(
-                "Failed to run `cargo metadata --manifest-path {}`{}",
-                cargo_toml.display(),
-                workdir
-            )
+            format!("Failed to run `cargo metadata --manifest-path {}`", cargo_toml.display(),)
         })?;
 
         Ok(meta)
@@ -312,7 +291,7 @@ impl CargoWorkspace {
                 id: id.repr.clone(),
                 name: name.clone(),
                 version: version.clone(),
-                manifest: AbsPathBuf::assert(PathBuf::from(&manifest_path)),
+                manifest: AbsPathBuf::assert(PathBuf::from(&manifest_path)).try_into().unwrap(),
                 targets: Vec::new(),
                 is_member,
                 edition,
@@ -376,7 +355,7 @@ impl CargoWorkspace {
     }
 
     pub fn from_cargo_metadata3(
-        cargo_toml: &AbsPath,
+        cargo_toml: &ManifestPath,
         config: &CargoConfig,
         progress: &dyn Fn(String),
     ) -> Result<CargoWorkspace> {
@@ -412,9 +391,9 @@ impl CargoWorkspace {
     }
 }
 
-fn rustc_discover_host_triple(cargo_toml: &AbsPath) -> Option<String> {
+fn rustc_discover_host_triple(cargo_toml: &ManifestPath) -> Option<String> {
     let mut rustc = Command::new(toolchain::rustc());
-    rustc.current_dir(cargo_toml.parent().unwrap()).arg("-vV");
+    rustc.current_dir(cargo_toml.parent()).arg("-vV");
     log::debug!("Discovering host platform by {:?}", rustc);
     match utf8_stdout(rustc) {
         Ok(stdout) => {
@@ -435,10 +414,10 @@ fn rustc_discover_host_triple(cargo_toml: &AbsPath) -> Option<String> {
     }
 }
 
-fn cargo_config_build_target(cargo_toml: &AbsPath) -> Option<String> {
+fn cargo_config_build_target(cargo_toml: &ManifestPath) -> Option<String> {
     let mut cargo_config = Command::new(toolchain::cargo());
     cargo_config
-        .current_dir(cargo_toml.parent().unwrap())
+        .current_dir(cargo_toml.parent())
         .args(&["-Z", "unstable-options", "config", "get", "build.target"])
         .env("RUSTC_BOOTSTRAP", "1");
     // if successful we receive `build.target = "target-triple"`

crates/project_model/src/lib.rs

@@ -15,6 +15,7 @@
 //!   procedural macros).
 //! * Lowering of concrete model to a [`base_db::CrateGraph`]
 
+mod manifest_path;
 mod cargo_workspace;
 mod cfg_flag;
 mod project_json;
@@ -24,12 +25,13 @@ mod rustc_cfg;
 mod build_scripts;
 
 use std::{
+    convert::{TryFrom, TryInto},
     fs::{self, read_dir, ReadDir},
     io,
     process::Command,
 };
 
-use anyhow::{bail, Context, Result};
+use anyhow::{bail, format_err, Context, Result};
 use paths::{AbsPath, AbsPathBuf};
 use rustc_hash::FxHashSet;
 
@@ -39,6 +41,7 @@ pub use crate::{
         CargoConfig, CargoWorkspace, Package, PackageData, PackageDependency, RustcSource, Target,
         TargetData, TargetKind,
     },
+    manifest_path::ManifestPath,
     project_json::{ProjectJson, ProjectJsonData},
     sysroot::Sysroot,
     workspace::{CfgOverrides, PackageRoot, ProjectWorkspace},
@@ -48,12 +51,14 @@ pub use proc_macro_api::ProcMacroClient;
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
 pub enum ProjectManifest {
-    ProjectJson(AbsPathBuf),
-    CargoToml(AbsPathBuf),
+    ProjectJson(ManifestPath),
+    CargoToml(ManifestPath),
 }
 
 impl ProjectManifest {
     pub fn from_manifest_file(path: AbsPathBuf) -> Result<ProjectManifest> {
+        let path = ManifestPath::try_from(path)
+            .map_err(|path| format_err!("bad manifest path: {}", path.display()))?;
         if path.file_name().unwrap_or_default() == "rust-project.json" {
             return Ok(ProjectManifest::ProjectJson(path));
         }
@@ -83,38 +88,43 @@ impl ProjectManifest {
         return find_cargo_toml(path)
             .map(|paths| paths.into_iter().map(ProjectManifest::CargoToml).collect());
 
-        fn find_cargo_toml(path: &AbsPath) -> io::Result<Vec<AbsPathBuf>> {
+        fn find_cargo_toml(path: &AbsPath) -> io::Result<Vec<ManifestPath>> {
             match find_in_parent_dirs(path, "Cargo.toml") {
                 Some(it) => Ok(vec![it]),
                 None => Ok(find_cargo_toml_in_child_dir(read_dir(path)?)),
             }
         }
 
-        fn find_in_parent_dirs(path: &AbsPath, target_file_name: &str) -> Option<AbsPathBuf> {
+        fn find_in_parent_dirs(path: &AbsPath, target_file_name: &str) -> Option<ManifestPath> {
             if path.file_name().unwrap_or_default() == target_file_name {
-                return Some(path.to_path_buf());
+                if let Ok(manifest) = ManifestPath::try_from(path.to_path_buf()) {
+                    return Some(manifest);
+                }
             }
 
             let mut curr = Some(path);
 
             while let Some(path) = curr {
                 let candidate = path.join(target_file_name);
                 if fs::metadata(&candidate).is_ok() {
-                    return Some(candidate);
+                    if let Ok(manifest) = ManifestPath::try_from(candidate) {
+                        return Some(manifest);
+                    }
                 }
                 curr = path.parent();
             }
 
             None
         }
 
-        fn find_cargo_toml_in_child_dir(entities: ReadDir) -> Vec<AbsPathBuf> {
+        fn find_cargo_toml_in_child_dir(entities: ReadDir) -> Vec<ManifestPath> {
             // Only one level down to avoid cycles the easy way and stop a runaway scan with large projects
             entities
                 .filter_map(Result::ok)
                 .map(|it| it.path().join("Cargo.toml"))
                 .filter(|it| it.exists())
                 .map(AbsPathBuf::assert)
+                .filter_map(|it| it.try_into().ok())
                 .collect()
         }
     }

crates/project_model/src/manifest_path.rs

@@ -0,0 +1,51 @@
+//! See [`ManifestPath`].
+use std::{convert::TryFrom, ops, path::Path};
+
+use paths::{AbsPath, AbsPathBuf};
+
+/// More or less [`AbsPathBuf`] with non-None parent.
+///
+/// We use it to store path to Cargo.toml, as we frequently use the parent dir
+/// as a working directory to spawn various commands, and its nice to not have
+/// to `.unwrap()` everywhere.
+///
+/// This could have been named `AbsNonRootPathBuf`, as we don't enforce that
+/// this stores manifest files in particular, but we only use this for manifests
+/// at the moment in practice.
+#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
+pub struct ManifestPath {
+    file: AbsPathBuf,
+}
+
+impl TryFrom<AbsPathBuf> for ManifestPath {
+    type Error = AbsPathBuf;
+
+    fn try_from(file: AbsPathBuf) -> Result<Self, Self::Error> {
+        if file.parent().is_none() {
+            Err(file)
+        } else {
+            Ok(ManifestPath { file })
+        }
+    }
+}
+
+impl ManifestPath {
+    // Shadow `parent` from `Deref`.
+    pub fn parent(&self) -> &AbsPath {
+        self.file.parent().unwrap()
+    }
+}
+
+impl ops::Deref for ManifestPath {
+    type Target = AbsPath;
+
+    fn deref(&self) -> &Self::Target {
+        &*self.file
+    }
+}
+
+impl AsRef<Path> for ManifestPath {
+    fn as_ref(&self) -> &Path {
+        self.file.as_ref()
+    }
+}

crates/project_model/src/rustc_cfg.rs

@@ -3,11 +3,10 @@
 use std::process::Command;
 
 use anyhow::Result;
-use paths::AbsPath;
 
-use crate::{cfg_flag::CfgFlag, utf8_stdout};
+use crate::{cfg_flag::CfgFlag, utf8_stdout, ManifestPath};
 
-pub(crate) fn get(cargo_toml: Option<&AbsPath>, target: Option<&str>) -> Vec<CfgFlag> {
+pub(crate) fn get(cargo_toml: Option<&ManifestPath>, target: Option<&str>) -> Vec<CfgFlag> {
     let _p = profile::span("rustc_cfg::get");
     let mut res = Vec::with_capacity(6 * 2 + 1);
 
@@ -27,12 +26,12 @@ pub(crate) fn get(cargo_toml: Option<&AbsPath>, target: Option<&str>) -> Vec<Cfg
     res
 }
 
-fn get_rust_cfgs(cargo_toml: Option<&AbsPath>, target: Option<&str>) -> Result<String> {
+fn get_rust_cfgs(cargo_toml: Option<&ManifestPath>, target: Option<&str>) -> Result<String> {
     let cargo_rust_cfgs = match cargo_toml {
         Some(cargo_toml) => {
             let mut cargo_config = Command::new(toolchain::cargo());
             cargo_config
-                .current_dir(cargo_toml.parent().unwrap())
+                .current_dir(cargo_toml.parent())
                 .args(&["-Z", "unstable-options", "rustc", "--print", "cfg"])
                 .env("RUSTC_BOOTSTRAP", "1");
             if let Some(target) = target {