internal: use types to remove some unwraps

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,7 +225,7 @@ struct PackageMetadata {
 
 impl CargoWorkspace {
     pub fn fetch_metadata(
-        cargo_toml: &AbsPath,
+        cargo_toml: &ManifestPath,
         config: &CargoConfig,
         progress: &dyn Fn(String),
     ) -> Result<cargo_metadata::Metadata> {
@@ -249,9 +244,7 @@ impl CargoWorkspace {
                 meta.features(CargoOpt::SomeFeatures(config.features.clone()));
             }
         }
-        if let Some(parent) = cargo_toml.parent() {
+        meta.current_dir(cargo_toml.parent().as_os_str());
-            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) {
@@ -269,21 +262,7 @@ impl CargoWorkspace {
         progress("metadata".to_string());
 
         let meta = meta.exec().with_context(|| {
-            let cwd: Option<AbsPathBuf> =
+            format!("Failed to run `cargo metadata --manifest-path {}`", cargo_toml.display(),)
-                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
-            )
         })?;
 
         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),
+    ProjectJson(ManifestPath),
-    CargoToml(AbsPathBuf),
+    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,16 +88,18 @@ 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);
@@ -100,7 +107,9 @@ impl ProjectManifest {
             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();
             }
@@ -108,13 +117,14 @@ impl ProjectManifest {
             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 {

crates/project_model/src/sysroot.rs

@@ -10,7 +10,7 @@ use anyhow::{format_err, Result};
 use la_arena::{Arena, Idx};
 use paths::{AbsPath, AbsPathBuf};
 
-use crate::utf8_stdout;
+use crate::{utf8_stdout, ManifestPath};
 
 #[derive(Default, Debug, Clone, Eq, PartialEq)]
 pub struct Sysroot {
@@ -22,7 +22,7 @@ pub(crate) type SysrootCrate = Idx<SysrootCrateData>;
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct SysrootCrateData {
     pub name: String,
-    pub root: AbsPathBuf,
+    pub root: ManifestPath,
     pub deps: Vec<SysrootCrate>,
 }
 
@@ -48,20 +48,17 @@ impl Sysroot {
         self.crates.iter().map(|(id, _data)| id)
     }
 
-    pub fn discover(cargo_toml: &AbsPath) -> Result<Sysroot> {
+    pub fn discover(dir: &AbsPath) -> Result<Sysroot> {
-        log::debug!("Discovering sysroot for {}", cargo_toml.display());
+        log::debug!("Discovering sysroot for {}", dir.display());
-        let current_dir = cargo_toml.parent().ok_or_else(|| {
+        let sysroot_dir = discover_sysroot_dir(dir)?;
-            format_err!("Failed to find the parent directory for {}", cargo_toml.display())
+        let sysroot_src_dir = discover_sysroot_src_dir(&sysroot_dir, dir)?;
-        })?;
-        let sysroot_dir = discover_sysroot_dir(current_dir)?;
-        let sysroot_src_dir = discover_sysroot_src_dir(&sysroot_dir, current_dir)?;
         let res = Sysroot::load(&sysroot_src_dir)?;
         Ok(res)
     }
 
-    pub fn discover_rustc(cargo_toml: &AbsPath) -> Option<AbsPathBuf> {
+    pub fn discover_rustc(cargo_toml: &ManifestPath) -> Option<ManifestPath> {
         log::debug!("Discovering rustc source for {}", cargo_toml.display());
-        let current_dir = cargo_toml.parent().unwrap();
+        let current_dir = cargo_toml.parent();
         discover_sysroot_dir(current_dir).ok().and_then(|sysroot_dir| get_rustc_src(&sysroot_dir))
     }
 
@@ -73,6 +70,7 @@ impl Sysroot {
             let root = [format!("{}/src/lib.rs", path), format!("lib{}/lib.rs", path)]
                 .iter()
                 .map(|it| sysroot_src_dir.join(it))
+                .filter_map(|it| ManifestPath::try_from(it).ok())
                 .find(|it| fs::metadata(it).is_ok());
 
             if let Some(root) = root {
@@ -168,8 +166,9 @@ try installing the Rust source the same way you installed rustc",
         })
 }
 
-fn get_rustc_src(sysroot_path: &AbsPath) -> Option<AbsPathBuf> {
+fn get_rustc_src(sysroot_path: &AbsPath) -> Option<ManifestPath> {
     let rustc_src = sysroot_path.join("lib/rustlib/rustc-src/rust/compiler/rustc/Cargo.toml");
+    let rustc_src = ManifestPath::try_from(rustc_src).ok()?;
     log::debug!("Checking for rustc source code: {}", rustc_src.display());
     if fs::metadata(&rustc_src).is_ok() {
         Some(rustc_src)
@@ -185,12 +184,6 @@ fn get_rust_src(sysroot_path: &AbsPath) -> Option<AbsPathBuf> {
     ["library", "src"].iter().map(|it| rust_src.join(it)).find(|it| fs::metadata(it).is_ok())
 }
 
-impl SysrootCrateData {
-    pub fn root_dir(&self) -> &AbsPath {
-        self.root.parent().unwrap()
-    }
-}
-
 const SYSROOT_CRATES: &str = "
 alloc
 core

crates/project_model/src/workspace.rs

@@ -2,7 +2,7 @@
 //! metadata` or `rust-project.json`) into representation stored in the salsa
 //! database -- `CrateGraph`.
 
-use std::{collections::VecDeque, fmt, fs, process::Command};
+use std::{collections::VecDeque, convert::TryFrom, fmt, fs, process::Command};
 
 use anyhow::{format_err, Context, Result};
 use base_db::{CrateDisplayName, CrateGraph, CrateId, CrateName, Edition, Env, FileId, ProcMacro};
@@ -18,8 +18,8 @@ use crate::{
     cfg_flag::CfgFlag,
     rustc_cfg,
     sysroot::SysrootCrate,
-    utf8_stdout, CargoConfig, CargoWorkspace, ProjectJson, ProjectManifest, Sysroot, TargetKind,
+    utf8_stdout, CargoConfig, CargoWorkspace, ManifestPath, ProjectJson, ProjectManifest, Sysroot,
-    WorkspaceBuildScripts,
+    TargetKind, WorkspaceBuildScripts,
 };
 
 pub type CfgOverrides = FxHashMap<String, CfgDiff>;
@@ -123,7 +123,7 @@ impl ProjectWorkspace {
                 let data = serde_json::from_str(&file).with_context(|| {
                     format!("Failed to deserialize json file {}", project_json.display())
                 })?;
-                let project_location = project_json.parent().unwrap().to_path_buf();
+                let project_location = project_json.parent().to_path_buf();
                 let project_json = ProjectJson::new(&project_location, data);
                 ProjectWorkspace::load_inline(project_json, config.target.as_deref())?
             }
@@ -147,7 +147,7 @@ impl ProjectWorkspace {
                 let sysroot = if config.no_sysroot {
                     Sysroot::default()
                 } else {
-                    Sysroot::discover(&cargo_toml).with_context(|| {
+                    Sysroot::discover(cargo_toml.parent()).with_context(|| {
                         format!(
                             "Failed to find sysroot for Cargo.toml file {}. Is rust-src installed?",
                             cargo_toml.display()
@@ -155,13 +155,10 @@ impl ProjectWorkspace {
                     })?
                 };
 
-                let rustc_dir = if let Some(rustc_source) = &config.rustc_source {
+                let rustc_dir = match &config.rustc_source {
-                    match rustc_source {
+                    Some(RustcSource::Path(path)) => ManifestPath::try_from(path.clone()).ok(),
-                        RustcSource::Path(path) => Some(path.clone()),
+                    Some(RustcSource::Discover) => Sysroot::discover_rustc(&cargo_toml),
-                        RustcSource::Discover => Sysroot::discover_rustc(&cargo_toml),
+                    None => None,
-                    }
-                } else {
-                    None
                 };
 
                 let rustc = match rustc_dir {
@@ -206,7 +203,10 @@ impl ProjectWorkspace {
 
     pub fn load_detached_files(detached_files: Vec<AbsPathBuf>) -> Result<ProjectWorkspace> {
         let sysroot = Sysroot::discover(
-            detached_files.first().ok_or_else(|| format_err!("No detached files to load"))?,
+            detached_files
+                .first()
+                .and_then(|it| it.parent())
+                .ok_or_else(|| format_err!("No detached files to load"))?,
         )?;
         let rustc_cfg = rustc_cfg::get(None, None);
         Ok(ProjectWorkspace::DetachedFiles { files: detached_files, sysroot, rustc_cfg })
@@ -253,7 +253,7 @@ impl ProjectWorkspace {
                 .chain(sysroot.as_ref().into_iter().flat_map(|sysroot| {
                     sysroot.crates().map(move |krate| PackageRoot {
                         is_member: false,
-                        include: vec![sysroot[krate].root_dir().to_path_buf()],
+                        include: vec![sysroot[krate].root.parent().to_path_buf()],
                         exclude: Vec::new(),
                     })
                 }))
@@ -270,7 +270,7 @@ impl ProjectWorkspace {
                     .packages()
                     .map(|pkg| {
                         let is_member = cargo[pkg].is_member;
-                        let pkg_root = cargo[pkg].root().to_path_buf();
+                        let pkg_root = cargo[pkg].manifest.parent().to_path_buf();
 
                         let mut include = vec![pkg_root.clone()];
                         include.extend(
@@ -306,13 +306,13 @@ impl ProjectWorkspace {
                     })
                     .chain(sysroot.crates().map(|krate| PackageRoot {
                         is_member: false,
-                        include: vec![sysroot[krate].root_dir().to_path_buf()],
+                        include: vec![sysroot[krate].root.parent().to_path_buf()],
                         exclude: Vec::new(),
                     }))
                     .chain(rustc.into_iter().flat_map(|rustc| {
                         rustc.packages().map(move |krate| PackageRoot {
                             is_member: false,
-                            include: vec![rustc[krate].root().to_path_buf()],
+                            include: vec![rustc[krate].manifest.parent().to_path_buf()],
                             exclude: Vec::new(),
                         })
                     }))
@@ -327,7 +327,7 @@ impl ProjectWorkspace {
                 })
                 .chain(sysroot.crates().map(|krate| PackageRoot {
                     is_member: false,
-                    include: vec![sysroot[krate].root_dir().to_path_buf()],
+                    include: vec![sysroot[krate].root.parent().to_path_buf()],
                     exclude: Vec::new(),
                 }))
                 .collect(),
@@ -855,8 +855,7 @@ fn inject_cargo_env(package: &PackageData, env: &mut Env) {
     // FIXME: Missing variables:
     // CARGO_BIN_NAME, CARGO_BIN_EXE_<name>
 
-    let mut manifest_dir = package.manifest.clone();
+    let manifest_dir = package.manifest.parent();
-    manifest_dir.pop();
     env.set("CARGO_MANIFEST_DIR".into(), manifest_dir.as_os_str().to_string_lossy().into_owned());
 
     // Not always right, but works for common cases.

crates/rust-analyzer/src/cargo_target_spec.rs

@@ -2,7 +2,7 @@
 
 use cfg::{CfgAtom, CfgExpr};
 use ide::{FileId, RunnableKind, TestId};
-use project_model::{self, TargetKind};
+use project_model::{self, ManifestPath, TargetKind};
 use vfs::AbsPathBuf;
 
 use crate::{global_state::GlobalStateSnapshot, Result};
@@ -14,7 +14,7 @@ use crate::{global_state::GlobalStateSnapshot, Result};
 #[derive(Clone)]
 pub(crate) struct CargoTargetSpec {
     pub(crate) workspace_root: AbsPathBuf,
-    pub(crate) cargo_toml: AbsPathBuf,
+    pub(crate) cargo_toml: ManifestPath,
     pub(crate) package: String,
     pub(crate) target: String,
     pub(crate) target_kind: TargetKind,