Auto merge of #6157 - alexheretic:member-manifest-error, r=alexcrichton

Expose manifest error chain in CargoErrors

Adds new `ManifestError`s to the `CargoError` causal chain. These errors pass on their display, but provide more detail on which manifests are at fault when failing to build a `Workspace`. This is useful for lib users, in particular rls, allowing lookup of a particular manifest file that caused the error. See #6144.

For example a workspace _foo_ where a member _bar_ has an invalid toml manifest would have the error chain:
- failed to parse manifest at `/home/alex/project/foo/bar/Cargo.toml`
  _ManifestError: /home/alex/project/foo/Cargo.toml_
- failed to parse manifest at `/home/alex/project/foo/bar/Cargo.toml`
  _ManifestError: /home/alex/project/foo/bar/Cargo.toml_
- failed to parse manifest at `/home/alex/project/foo/bar/Cargo.toml`
- could not parse input as TOML
- expected a value, found an equals at line 8

This will allow rls to point to a particular workspace member's manifest file when that manifest fails to deserialize, has invalid paths, etc.

This change should not affect binary use.

Author: bors

src/cargo/core/workspace.rs

@@ -13,7 +13,7 @@ use core::{Dependency, PackageIdSpec};
 use core::{EitherManifest, Package, SourceId, VirtualManifest};
 use ops;
 use sources::PathSource;
-use util::errors::{CargoResult, CargoResultExt};
+use util::errors::{CargoResult, CargoResultExt, ManifestError};
 use util::paths;
 use util::toml::read_manifest;
 use util::{Config, Filesystem};
@@ -508,7 +508,8 @@ impl<'cfg> Workspace<'cfg> {
                 .collect::<Vec<_>>()
         };
         for candidate in candidates {
-            self.find_path_deps(&candidate, root_manifest, true)?;
+            self.find_path_deps(&candidate, root_manifest, true)
+                .map_err(|err| ManifestError::new(err, manifest_path.clone()))?;
         }
         Ok(())
     }

src/cargo/ops/cargo_read_manifest.rs

@@ -153,7 +153,7 @@ fn read_nested_packages(
                 "skipping malformed package found at `{}`",
                 path.to_string_lossy()
             );
-            errors.push(err);
+            errors.push(err.into());
             return Ok(());
         }
         Ok(tuple) => tuple,

src/cargo/util/errors.rs

@@ -3,6 +3,7 @@
 use std::fmt;
 use std::process::{ExitStatus, Output};
 use std::str;
+use std::path::PathBuf;
 
 use core::{TargetKind, Workspace};
 use failure::{Context, Error, Fail};
@@ -72,6 +73,68 @@ impl fmt::Display for Internal {
     }
 }
 
+/// Error wrapper related to a particular manifest and providing it's path.
+///
+/// This error adds no displayable info of it's own.
+pub struct ManifestError {
+    cause: Error,
+    manifest: PathBuf,
+}
+
+impl ManifestError {
+    pub fn new<E: Into<Error>>(cause: E, manifest: PathBuf) -> Self {
+        Self {
+            cause: cause.into(),
+            manifest,
+        }
+    }
+
+    pub fn manifest_path(&self) -> &PathBuf {
+        &self.manifest
+    }
+
+    /// Returns an iterator over the `ManifestError` chain of causes.
+    ///
+    /// So if this error was not caused by another `ManifestError` this will be empty.
+    pub fn manifest_causes(&self) -> ManifestCauses {
+        ManifestCauses { current: self }
+    }
+}
+
+impl Fail for ManifestError {
+    fn cause(&self) -> Option<&Fail> {
+        self.cause.as_fail().cause()
+    }
+}
+
+impl fmt::Debug for ManifestError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.cause.fmt(f)
+    }
+}
+
+impl fmt::Display for ManifestError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.cause.fmt(f)
+    }
+}
+
+/// An iterator over the `ManifestError` chain of causes.
+pub struct ManifestCauses<'a> {
+    current: &'a ManifestError,
+}
+
+impl<'a> Iterator for ManifestCauses<'a> {
+    type Item = &'a ManifestError;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.current = self.current.cause.downcast_ref()?;
+        Some(self.current)
+    }
+}
+
+impl<'a> ::std::iter::FusedIterator for ManifestCauses<'a> {}
+
 // =============================================================================
 // Process errors
 #[derive(Debug, Fail)]

src/cargo/util/toml/mod.rs

@@ -19,7 +19,7 @@ use core::{Dependency, Manifest, PackageId, Summary, Target};
 use core::{Edition, EitherManifest, Feature, Features, VirtualManifest};
 use core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, WorkspaceRootConfig};
 use sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY};
-use util::errors::{CargoError, CargoResult, CargoResultExt};
+use util::errors::{CargoError, CargoResult, CargoResultExt, ManifestError};
 use util::paths;
 use util::{self, Config, ToUrl};
 
@@ -30,17 +30,17 @@ pub fn read_manifest(
     path: &Path,
     source_id: &SourceId,
     config: &Config,
-) -> CargoResult<(EitherManifest, Vec<PathBuf>)> {
+) -> Result<(EitherManifest, Vec<PathBuf>), ManifestError> {
     trace!(
         "read_manifest; path={}; source-id={}",
         path.display(),
         source_id
     );
-    let contents = paths::read(path)?;
+    let contents = paths::read(path).map_err(|err| ManifestError::new(err, path.into()))?;
 
-    let ret = do_read_manifest(&contents, path, source_id, config)
-        .chain_err(|| format!("failed to parse manifest at `{}`", path.display()))?;
-    Ok(ret)
+    do_read_manifest(&contents, path, source_id, config)
+        .chain_err(|| format!("failed to parse manifest at `{}`", path.display()))
+        .map_err(|err| ManifestError::new(err, path.into()))
 }
 
 fn do_read_manifest(

tests/testsuite/main.rs

@@ -66,6 +66,7 @@ mod jobserver;
 mod local_registry;
 mod lockfile_compat;
 mod login;
+mod member_errors;
 mod metabuild;
 mod metadata;
 mod net_config;

tests/testsuite/member_errors.rs

@@ -0,0 +1,104 @@
+use cargo::core::Workspace;
+use cargo::util::{config::Config, errors::ManifestError};
+
+use support::project;
+
+/// Tests inclusion of a `ManifestError` pointing to a member manifest
+/// when that manifest fails to deserialize.
+#[test]
+fn toml_deserialize_manifest_error() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            bar = { path = "bar" }
+
+            [workspace]
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+            [project]
+            name = "bar"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            foobar == "0.55"
+        "#,
+        )
+        .file("bar/src/main.rs", "fn main() {}")
+        .build();
+
+    let root_manifest_path = p.root().join("Cargo.toml");
+    let member_manifest_path = p.root().join("bar").join("Cargo.toml");
+
+    let error = Workspace::new(&root_manifest_path, &Config::default().unwrap()).unwrap_err();
+    eprintln!("{:?}", error);
+
+    let manifest_err: &ManifestError = error.downcast_ref().expect("Not a ManifestError");
+    assert_eq!(manifest_err.manifest_path(), &root_manifest_path);
+
+    let causes: Vec<_> = manifest_err.manifest_causes().collect();
+    assert_eq!(causes.len(), 1, "{:?}", causes);
+    assert_eq!(causes[0].manifest_path(), &member_manifest_path);
+}
+
+/// Tests inclusion of a `ManifestError` pointing to a member manifest
+/// when that manifest has an invalid dependency path.
+#[test]
+fn member_manifest_path_io_error() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            bar = { path = "bar" }
+
+            [workspace]
+        "#,
+        )
+        .file("src/main.rs", "fn main() {}")
+        .file(
+            "bar/Cargo.toml",
+            r#"
+            [project]
+            name = "bar"
+            version = "0.1.0"
+            authors = []
+
+            [dependencies]
+            foobar = { path = "nosuch" }
+        "#,
+        )
+        .file("bar/src/main.rs", "fn main() {}")
+        .build();
+
+    let root_manifest_path = p.root().join("Cargo.toml");
+    let member_manifest_path = p.root().join("bar").join("Cargo.toml");
+    let missing_manifest_path = p.root().join("bar").join("nosuch").join("Cargo.toml");
+
+    let error = Workspace::new(&root_manifest_path, &Config::default().unwrap()).unwrap_err();
+    eprintln!("{:?}", error);
+
+    let manifest_err: &ManifestError = error.downcast_ref().expect("Not a ManifestError");
+    assert_eq!(manifest_err.manifest_path(), &root_manifest_path);
+
+    let causes: Vec<_> = manifest_err.manifest_causes().collect();
+    assert_eq!(causes.len(), 2, "{:?}", causes);
+    assert_eq!(causes[0].manifest_path(), &member_manifest_path);
+    assert_eq!(causes[1].manifest_path(), &missing_manifest_path);
+}