cargo-metadata: error out if same dep with differnt names

Previous, `cargo metadata` allows a dependency with different renamed
co-exist. However, its `resolve.nodes.deps` will miss that dependency,
which is wrong. After this commit, `cargo metadata starts erroring out
for that situation.

Author: weihanglo

src/cargo/core/compiler/artifact.rs

@@ -4,7 +4,7 @@ use crate::core::compiler::{Context, CrateType, FileFlavor, Unit};
 use crate::core::dependency::ArtifactKind;
 use crate::core::{Dependency, Target, TargetKind};
 use crate::CargoResult;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::ffi::OsString;
 
 /// Return all environment variables for the given unit-dependencies
@@ -62,21 +62,18 @@ fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str {
 ///
 /// 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],
+pub(crate) fn match_artifacts_kind_with_targets<'t, 'd>(
+    artifact_dep: &'d Dependency,
+    targets: &'t [Target],
     parent_package: &str,
-    mut callback: F,
-) -> CargoResult<()>
-where
-    F: FnMut(&ArtifactKind, &mut dyn Iterator<Item = &'a Target>),
-{
+) -> CargoResult<HashSet<(&'d ArtifactKind, &'t 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 = |kind: &ArtifactKind, filter: &dyn Fn(&&Target) -> bool| {
+        let mut extend = |kind, filter: &dyn Fn(&&Target) -> bool| {
             let mut iter = targets.iter().filter(filter).peekable();
             let found = iter.peek().is_some();
-            callback(kind, &mut iter);
+            out.extend(std::iter::repeat(kind).zip(iter));
             found
         };
         let found = match artifact_kind {
@@ -96,5 +93,5 @@ where
             );
         }
     }
-    Ok(())
+    Ok(out)
 }

src/cargo/core/compiler/unit_dependencies.rs

@@ -555,53 +555,48 @@ fn artifact_targets_to_unit_deps(
     artifact_pkg: &Package,
     dep: &Dependency,
 ) -> CargoResult<Vec<UnitDep>> {
-    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(),
-                ))),
-            }
-        })
-        .collect::<Result<Vec<_>, _>>()?;
+    let ret =
+        match_artifacts_kind_with_targets(dep, artifact_pkg.targets(), parent.pkg.name().as_str())?
+            .into_iter()
+            .map(|(_artifact_kind, target)| target)
+            .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)
 }
 

src/cargo/ops/cargo_output_metadata.rs

@@ -91,16 +91,15 @@ struct Dep {
 struct DepKindInfo {
     kind: DepKind,
     target: Option<Platform>,
+    /// What the manifest calls the crate.
+    ///
+    /// A renamed dependency will show the rename instead of original name.
+    extern_name: InternedString,
 
     // vvvvv The fields below are introduced for `-Z bindeps`.
     /// Artifact's crate type, e.g. staticlib, cdylib, bin...
     #[serde(skip_serializing_if = "Option::is_none")]
     artifact: Option<&'static str>,
-    /// What the manifest calls the crate.
-    ///
-    /// A renamed dependency will show the rename instead of original name.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    extern_name: Option<InternedString>,
     /// Equivalent to `{ target = "…" }` in an artifact dependency requirement.
     ///
     /// * If the target points to a custom target JSON file, the path will be absolute.
@@ -205,9 +204,9 @@ fn build_resolve_graph_r(
     let normalize_id = |id| -> PackageId { *package_map.get_key_value(&id).unwrap().0 };
     let features = resolve.features(pkg_id).to_vec();
 
-    let deps: Vec<Dep> = resolve
-        .deps(pkg_id)
-        .filter(|(_dep_id, deps)| {
+    let deps = {
+        let mut dep_metadatas = Vec::new();
+        let iter = resolve.deps(pkg_id).filter(|(_dep_id, deps)| {
             if requested_kinds == [CompileKind::Host] {
                 true
             } else {
@@ -216,8 +215,8 @@ fn build_resolve_graph_r(
                         .any(|dep| target_data.dep_platform_activated(dep, *kind))
                 })
             }
-        })
-        .filter_map(|(dep_id, deps)| {
+        });
+        for (dep_id, deps) in iter {
             let mut dep_kinds = Vec::new();
 
             let targets = package_map[&dep_id].targets();
@@ -227,30 +226,30 @@ fn build_resolve_graph_r(
                 resolve
                     .extern_crate_name_and_dep_name(pkg_id, dep_id, target)
                     .map(|(ext_crate_name, _)| ext_crate_name)
-                    .ok()
             };
 
-            let lib_target_name = targets.iter().find(|t| t.is_lib()).map(extern_name);
+            let lib_target = targets.iter().find(|t| t.is_lib());
 
             for dep in deps.iter() {
-                let mut include_lib = || {
-                    dep_kinds.push(DepKindInfo {
-                        kind: dep.kind(),
-                        target: dep.platform().cloned(),
-                        artifact: None,
-                        extern_name: lib_target_name,
-                        compile_target: None,
-                        bin_name: None,
-                    });
-                };
-
-                // When we do have a library target, include them in deps if...
-                match (dep.artifact(), lib_target_name) {
-                    // it is also an artifact dep with `{ …, lib = true }`
-                    (Some(a), Some(_)) if a.is_lib() => include_lib(),
-                    // it is not an artifact dep at all
-                    (None, Some(_)) => include_lib(),
-                    _ => {}
+                if let Some(target) = lib_target {
+                    // When we do have a library target, include them in deps if...
+                    let included = match dep.artifact() {
+                        // it is not an artifact dep at all
+                        None => true,
+                        // it is also an artifact dep with `{ …, lib = true }`
+                        Some(a) if a.is_lib() => true,
+                        _ => false,
+                    };
+                    if included {
+                        dep_kinds.push(DepKindInfo {
+                            kind: dep.kind(),
+                            target: dep.platform().cloned(),
+                            extern_name: extern_name(target)?,
+                            artifact: None,
+                            compile_target: None,
+                            bin_name: None,
+                        });
+                    }
                 }
 
                 // No need to proceed if there is no artifact dependency.
@@ -269,48 +268,47 @@ fn build_resolve_graph_r(
                     None => None,
                 };
 
-                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));
+                let target_set =
+                    match_artifacts_kind_with_targets(dep, targets, pkg_id.name().as_str())?;
+                dep_kinds.reserve(target_set.len());
+                for (kind, target) in target_set.into_iter() {
+                    dep_kinds.push(DepKindInfo {
+                        kind: dep.kind(),
+                        target: dep.platform().cloned(),
+                        extern_name: extern_name(target)?,
+                        artifact: Some(kind.crate_type()),
+                        compile_target,
+                        bin_name: target.is_bin().then(|| target.name().to_string()),
+                    })
                 }
             }
 
             dep_kinds.sort();
 
             let pkg = normalize_id(dep_id);
 
-            let dep = match (lib_target_name, dep_kinds.len()) {
-                (Some(name), _) => Some(Dep {
-                    name,
+            let dep = match (lib_target, dep_kinds.len()) {
+                (Some(target), _) => Dep {
+                    name: extern_name(target)?,
                     pkg,
                     dep_kinds,
-                }),
+                },
                 // No lib target exists but contains artifact deps.
-                (None, 1..) => Some(Dep {
+                (None, 1..) => Dep {
                     name: InternedString::new(""),
                     pkg,
                     dep_kinds,
-                }),
+                },
                 // No lib or artifact dep exists.
                 // Ususally this mean parent depending on non-lib bin crate.
-                (None, _) => None,
+                (None, _) => continue,
             };
-            dep.map(Ok)
-        })
-        .collect::<CargoResult<_>>()?;
+
+            dep_metadatas.push(dep)
+        }
+        dep_metadatas
+    };
+
     let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| dep.pkg).collect();
     let to_visit = dumb_deps.clone();
     let node = MetadataResolveNode {

tests/testsuite/metadata.rs

@@ -1632,24 +1632,24 @@ fn workspace_metadata_with_dependencies_and_resolve() {
                             "target": null
                           },
                           {
-                            "artifact": "bin",
-                            "bin_name": "baz-name",
+                            "artifact": "cdylib",
                             "compile_target": "wasm32-unknown-unknown",
-                            "extern_name": "baz_name",
+                            "extern_name": "artifact",
                             "kind": null,
                             "target": null
                           },
                           {
-                            "artifact": "cdylib",
+                            "artifact": "staticlib",
                             "compile_target": "wasm32-unknown-unknown",
                             "extern_name": "artifact",
                             "kind": null,
                             "target": null
                           },
                           {
-                            "artifact": "staticlib",
+                            "artifact": "bin",
+                            "bin_name": "baz-name",
                             "compile_target": "wasm32-unknown-unknown",
-                            "extern_name": "artifact",
+                            "extern_name": "baz_name",
                             "kind": null,
                             "target": null
                           },
@@ -1887,6 +1887,36 @@ fn cargo_metadata_with_invalid_artifact_deps() {
         .run();
 }
 
+#[cargo_test]
+fn cargo_metadata_with_invalid_duplicate_renamed_deps() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.5.0"
+
+                [dependencies]
+                bar = { path = "bar" }
+                baz = { path = "bar", package = "bar" }
+           "#,
+        )
+        .file("src/lib.rs", "")
+        .file("bar/Cargo.toml", &basic_lib_manifest("bar"))
+        .file("bar/src/lib.rs", "")
+        .build();
+
+    p.cargo("metadata")
+        .with_status(101)
+        .with_stderr(
+            "\
+[WARNING] please specify `--format-version` flag explicitly to avoid compatibility problems
+[ERROR] the crate `foo v0.5.0 ([..])` depends on crate `bar v0.5.0 ([..])` multiple times with different names",
+        )
+        .run();
+}
+
 const MANIFEST_OUTPUT: &str = r#"
 {
     "packages": [{