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

weihanglo · weihanglo · commit db463e65a09b · 2023-01-07T00:19:48.000Z
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`.
diff --git a/src/cargo/core/compiler/artifact.rs b/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,46 @@ 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,
+/// and provides it via a callback.
+///
+/// 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(())
+}
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/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,87 +555,54 @@ fn artifact_targets_to_unit_deps(
     artifact_pkg: &Package,
     dep: &Dependency,
 ) -> CargoResult<Vec<UnitDep>> {
-    let ret =
-        match_artifacts_kind_with_targets(dep, artifact_pkg.targets(), parent.pkg.name().as_str())?
-            .into_iter()
-            .flat_map(|target| {
-                // We split target libraries into individual units, even though rustc is able
-                // to produce multiple kinds in an single invocation for the sole reason that
-                // each artifact kind has its own output directory, something we can't easily
-                // teach rustc for now.
-                match target.kind() {
-                    TargetKind::Lib(kinds) => Box::new(
-                        kinds
-                            .iter()
-                            .filter(|tk| matches!(tk, CrateType::Cdylib | CrateType::Staticlib))
-                            .map(|target_kind| {
-                                new_unit_dep(
-                                    state,
-                                    parent,
-                                    artifact_pkg,
-                                    target
-                                        .clone()
-                                        .set_kind(TargetKind::Lib(vec![target_kind.clone()])),
-                                    parent_unit_for,
-                                    compile_kind,
-                                    CompileMode::Build,
-                                    dep.artifact(),
-                                )
-                            }),
-                    ) as Box<dyn Iterator<Item = _>>,
-                    _ => Box::new(std::iter::once(new_unit_dep(
-                        state,
-                        parent,
-                        artifact_pkg,
-                        target,
-                        parent_unit_for,
-                        compile_kind,
-                        CompileMode::Build,
-                        dep.artifact(),
-                    ))),
-                }
-            })
-            .collect::<Result<Vec<_>, _>>()?;
-    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())
+    let mut targets = HashSet::new();
+    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
+            // to produce multiple kinds in an single invocation for the sole reason that
+            // each artifact kind has its own output directory, something we can't easily
+            // teach rustc for now.
+            match target.kind() {
+                TargetKind::Lib(kinds) => Box::new(
+                    kinds
+                        .iter()
+                        .filter(|tk| matches!(tk, CrateType::Cdylib | CrateType::Staticlib))
+                        .map(|target_kind| {
+                            new_unit_dep(
+                                state,
+                                parent,
+                                artifact_pkg,
+                                target
+                                    .clone()
+                                    .set_kind(TargetKind::Lib(vec![target_kind.clone()])),
+                                parent_unit_for,
+                                compile_kind,
+                                CompileMode::Build,
+                                dep.artifact(),
+                            )
+                        }),
+                ) as Box<dyn Iterator<Item = _>>,
+                _ => Box::new(std::iter::once(new_unit_dep(
+                    state,
+                    parent,
+                    artifact_pkg,
+                    target,
+                    parent_unit_for,
+                    compile_kind,
+                    CompileMode::Build,
+                    dep.artifact(),
+                ))),
             }
-        };
-        if !found {
-            anyhow::bail!(
-                "dependency `{}` in package `{}` requires a `{}` artifact to be present.",
-                artifact_dep.name_in_toml(),
-                parent_package,
-                artifact_kind
-            );
-        }
-    }
-    Ok(out)
+        })
+        .collect::<Result<Vec<_>, _>>()?;
+    Ok(ret)
 }
 
 /// Returns the dependencies necessary to document a package.
diff --git a/src/cargo/ops/cargo_output_metadata.rs b/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,35 +269,30 @@ fn build_resolve_graph_r(
                     None => None,
                 };
 
-                let mut extend = |kind: &ArtifactKind, filter: &dyn Fn(&&Target) -> bool| {
-                    let iter = targets.iter().filter(filter).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);
-                };
-
-                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())
-                        }
-                    };
+                if let Err(e) = match_artifacts_kind_with_targets(
+                    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()),
+                        }))
+                    },
+                ) {
+                    return Some(Err(e));
                 }
             }
 
             dep_kinds.sort();
 
             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();
-    let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| normalize_id(dep.pkg)).collect();
+        .collect::<CargoResult<_>>()?;
+    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(())
 }
diff --git a/tests/testsuite/metadata.rs b/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": [{
