cargo-metadata: error out when encountering invalid artifact kind syntax

This refactor reuse the logic of
`core::compiler::unit_dependencies::match_artifacts_kind_with_targets`
to emit error if there is any syntax error in `ArtifactKind`.

It also put `match_artifacts_kind_with_targets` to a better place `core::compiler::artifact`.

src/cargo/core/compiler/artifact.rs

@@ -1,7 +1,8 @@
 /// Generate artifact information from unit dependencies for configuring the compiler environment.
 use crate::core::compiler::unit_graph::UnitDep;
 use crate::core::compiler::{Context, CrateType, FileFlavor, Unit};
-use crate::core::TargetKind;
+use crate::core::dependency::ArtifactKind;
+use crate::core::{Dependency, Target, TargetKind};
 use crate::CargoResult;
 use std::collections::HashMap;
 use std::ffi::OsString;
@@ -55,3 +56,45 @@ fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str {
         invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid),
     }
 }
+
+/// Given a dependency with an artifact `artifact_dep` and a set of available `targets`
+/// of its package, find a target for each kind of artifacts that are to be built.
+///
+/// Failure to match any target results in an error mentioning the parent manifests
+/// `parent_package` name.
+pub(crate) fn match_artifacts_kind_with_targets<'a, F>(
+    artifact_dep: &Dependency,
+    targets: &'a [Target],
+    parent_package: &str,
+    mut callback: F,
+) -> CargoResult<()>
+where
+    F: FnMut(&ArtifactKind, &mut dyn Iterator<Item = &'a Target>),
+{
+    let artifact_requirements = artifact_dep.artifact().expect("artifact present");
+    for artifact_kind in artifact_requirements.kinds() {
+        let mut extend = |kind: &ArtifactKind, filter: &dyn Fn(&&Target) -> bool| {
+            let mut iter = targets.iter().filter(filter).peekable();
+            let found = iter.peek().is_some();
+            callback(kind, &mut iter);
+            found
+        };
+        let found = match artifact_kind {
+            ArtifactKind::Cdylib => extend(artifact_kind, &|t| t.is_cdylib()),
+            ArtifactKind::Staticlib => extend(artifact_kind, &|t| t.is_staticlib()),
+            ArtifactKind::AllBinaries => extend(artifact_kind, &|t| t.is_bin()),
+            ArtifactKind::SelectedBinary(bin_name) => extend(artifact_kind, &|t| {
+                t.is_bin() && t.name() == bin_name.as_str()
+            }),
+        };
+        if !found {
+            anyhow::bail!(
+                "dependency `{}` in package `{}` requires a `{}` artifact to be present.",
+                artifact_dep.name_in_toml(),
+                parent_package,
+                artifact_kind
+            );
+        }
+    }
+    Ok(())
+}

src/cargo/core/compiler/unit_dependencies.rs

@@ -19,11 +19,12 @@ use std::collections::{HashMap, HashSet};
 
 use log::trace;
 
+use crate::core::compiler::artifact::match_artifacts_kind_with_targets;
 use crate::core::compiler::unit_graph::{UnitDep, UnitGraph};
 use crate::core::compiler::{
     CompileKind, CompileMode, CrateType, RustcTargetData, Unit, UnitInterner,
 };
-use crate::core::dependency::{Artifact, ArtifactKind, ArtifactTarget, DepKind};
+use crate::core::dependency::{Artifact, ArtifactTarget, DepKind};
 use crate::core::profiles::{Profile, Profiles, UnitFor};
 use crate::core::resolver::features::{FeaturesFor, ResolvedFeatures};
 use crate::core::resolver::Resolve;
@@ -554,8 +555,14 @@ fn artifact_targets_to_unit_deps(
     artifact_pkg: &Package,
     dep: &Dependency,
 ) -> CargoResult<Vec<UnitDep>> {
-    let ret =
+    let mut targets = HashSet::new();
-        match_artifacts_kind_with_targets(dep, artifact_pkg.targets(), parent.pkg.name().as_str())?
+    match_artifacts_kind_with_targets(
+        dep,
+        artifact_pkg.targets(),
+        parent.pkg.name().as_str(),
+        |_, iter| targets.extend(iter),
+    )?;
+    let ret = targets
         .into_iter()
         .flat_map(|target| {
             // We split target libraries into individual units, even though rustc is able
@@ -598,45 +605,6 @@ fn artifact_targets_to_unit_deps(
     Ok(ret)
 }
 
-/// Given a dependency with an artifact `artifact_dep` and a set of available `targets`
-/// of its package, find a target for each kind of artifacts that are to be built.
-///
-/// Failure to match any target results in an error mentioning the parent manifests
-/// `parent_package` name.
-fn match_artifacts_kind_with_targets<'a>(
-    artifact_dep: &Dependency,
-    targets: &'a [Target],
-    parent_package: &str,
-) -> CargoResult<HashSet<&'a Target>> {
-    let mut out = HashSet::new();
-    let artifact_requirements = artifact_dep.artifact().expect("artifact present");
-    for artifact_kind in artifact_requirements.kinds() {
-        let mut extend = |filter: &dyn Fn(&&Target) -> bool| {
-            let mut iter = targets.iter().filter(filter).peekable();
-            let found = iter.peek().is_some();
-            out.extend(iter);
-            found
-        };
-        let found = match artifact_kind {
-            ArtifactKind::Cdylib => extend(&|t| t.is_cdylib()),
-            ArtifactKind::Staticlib => extend(&|t| t.is_staticlib()),
-            ArtifactKind::AllBinaries => extend(&|t| t.is_bin()),
-            ArtifactKind::SelectedBinary(bin_name) => {
-                extend(&|t| t.is_bin() && t.name() == bin_name.as_str())
-            }
-        };
-        if !found {
-            anyhow::bail!(
-                "dependency `{}` in package `{}` requires a `{}` artifact to be present.",
-                artifact_dep.name_in_toml(),
-                parent_package,
-                artifact_kind
-            );
-        }
-    }
-    Ok(out)
-}
-
 /// Returns the dependencies necessary to document a package.
 fn compute_deps_doc(
     unit: &Unit,

src/cargo/ops/cargo_output_metadata.rs

@@ -1,8 +1,9 @@
+use crate::core::compiler::artifact::match_artifacts_kind_with_targets;
 use crate::core::compiler::{CompileKind, RustcTargetData};
-use crate::core::dependency::{ArtifactKind, DepKind};
+use crate::core::dependency::DepKind;
 use crate::core::package::SerializedPackage;
 use crate::core::resolver::{features::CliFeatures, HasDevUnits, Resolve};
-use crate::core::{Package, PackageId, Target, Workspace};
+use crate::core::{Package, PackageId, Workspace};
 use crate::ops::{self, Packages};
 use crate::util::interning::InternedString;
 use crate::util::CargoResult;
@@ -160,7 +161,7 @@ fn build_resolve_graph(
             &package_map,
             &target_data,
             &requested_kinds,
-        );
+        )?;
     }
     // Get a Vec of Packages.
     let actual_packages = package_map
@@ -183,9 +184,9 @@ fn build_resolve_graph_r(
     package_map: &BTreeMap<PackageId, Package>,
     target_data: &RustcTargetData<'_>,
     requested_kinds: &[CompileKind],
-) {
+) -> CargoResult<()> {
     if node_map.contains_key(&pkg_id) {
-        return;
+        return Ok(());
     }
     // This normalizes the IDs so that they are consistent between the
     // `packages` array and the `resolve` map. This is a bit of a hack to
@@ -268,27 +269,22 @@ fn build_resolve_graph_r(
                     None => None,
                 };
 
-                let mut extend = |kind: &ArtifactKind, filter: &dyn Fn(&&Target) -> bool| {
+                if let Err(e) = match_artifacts_kind_with_targets(
-                    let iter = targets.iter().filter(filter).map(|target| DepKindInfo {
+                    dep,
+                    targets,
+                    pkg_id.name().as_str(),
+                    |kind, targets| {
+                        dep_kinds.extend(targets.map(|target| DepKindInfo {
                             kind: dep.kind(),
                             target: dep.platform().cloned(),
                             artifact: Some(kind.crate_type()),
                             extern_name: extern_name(target),
                             compile_target,
                             bin_name: target.is_bin().then(|| target.name().to_string()),
-                    });
+                        }))
-                    dep_kinds.extend(iter);
+                    },
-                };
+                ) {
-
+                    return Some(Err(e));
-                for kind in artifact_requirements.kinds() {
-                    match kind {
-                        ArtifactKind::Cdylib => extend(kind, &|t| t.is_cdylib()),
-                        ArtifactKind::Staticlib => extend(kind, &|t| t.is_staticlib()),
-                        ArtifactKind::AllBinaries => extend(kind, &|t| t.is_bin()),
-                        ArtifactKind::SelectedBinary(bin_name) => {
-                            extend(kind, &|t| t.is_bin() && t.name() == bin_name.as_str())
-                        }
-                    };
                 }
             }
 
@@ -296,7 +292,7 @@ fn build_resolve_graph_r(
 
             let pkg = normalize_id(dep_id);
 
-            match (lib_target_name, dep_kinds.len()) {
+            let dep = match (lib_target_name, dep_kinds.len()) {
                 (Some(name), _) => Some(Dep {
                     name,
                     pkg,
@@ -311,10 +307,11 @@ fn build_resolve_graph_r(
                 // No lib or artifact dep exists.
                 // Ususally this mean parent depending on non-lib bin crate.
                 (None, _) => None,
-            }
+            };
+            dep.map(Ok)
         })
-        .collect();
+        .collect::<CargoResult<_>>()?;
-    let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| normalize_id(dep.pkg)).collect();
+    let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| dep.pkg).collect();
     let to_visit = dumb_deps.clone();
     let node = MetadataResolveNode {
         id: normalize_id(pkg_id),
@@ -331,6 +328,8 @@ fn build_resolve_graph_r(
             package_map,
             target_data,
             requested_kinds,
-        );
+        )?;
     }
+
+    Ok(())
 }

tests/testsuite/metadata.rs

@@ -1857,6 +1857,36 @@ Caused by:
         .run();
 }
 
+#[cargo_test]
+fn cargo_metadata_with_invalid_artifact_deps() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.5.0"
+
+                [dependencies]
+                artifact = { path = "artifact", artifact = "bin:notfound" }
+           "#,
+        )
+        .file("src/lib.rs", "")
+        .file("artifact/Cargo.toml", &basic_bin_manifest("artifact"))
+        .file("artifact/src/main.rs", "fn main() {}")
+        .build();
+
+    p.cargo("metadata -Z bindeps")
+        .masquerade_as_nightly_cargo(&["bindeps"])
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] please specify `--format-version` flag explicitly to avoid compatibility problems
+[ERROR] dependency `artifact` in package `foo` requires a `bin:notfound` artifact to be present.",
+        )
+        .run();
+}
+
 const MANIFEST_OUTPUT: &str = r#"
 {
     "packages": [{