Auto merge of #5415 - alexcrichton:rename-same-dep, r=matklad

Fix renaming crates as they come from 2 sources

Previously there was a verification in manifest parsing that the same dependency
must come from the same source, but this erroneously triggered an error to get
emitted when the `package` key was used to rename crates. The first change here
was to update that clause to key off the `rename` field rather than the `name`
field.

Afterwards, though, this exposed an existing bug in the implementation. During
compilation we have a `Resolve` which is a graph of crates, but we don't know
*why* each edge in the dependency graph exists. In other words we don't know,
when looking at an edge of the graph, what `Dependency` caused that edge to be
drawn. We need to know this when passing `--extern` flags because the
`Dependency` is what lists what's being renamed.

This commit then primarily refactors `Resolve::deps` from an iterator of package
ids to an iterator of a tuples. The first element is the package id from before
and the second element is a list of `Dependency` directives which caused the
edge to ber driven.

This refactoring cleaned up a few places in the backend where we had to work
around the lack of this knowledge. Namely this also fixes the extra test added
here.

Closes #5413

Author: bors

src/cargo/core/compiler/context/unit_dependencies.rs

@@ -64,54 +64,55 @@ fn compute_deps<'a, 'b, 'cfg>(
 
     let id = unit.pkg.package_id();
     let deps = cx.resolve.deps(id);
-    let mut ret = deps.filter(|dep| {
-        unit.pkg
-            .dependencies()
-            .iter()
-            .filter(|d| d.name() == dep.name() && d.version_req().matches(dep.version()))
-            .any(|d| {
+    let mut ret = deps
+        .filter(|&(_id, deps)| {
+            assert!(deps.len() > 0);
+            deps.iter().any(|dep| {
                 // If this target is a build command, then we only want build
                 // dependencies, otherwise we want everything *other than* build
                 // dependencies.
-                if unit.target.is_custom_build() != d.is_build() {
+                if unit.target.is_custom_build() != dep.is_build() {
                     return false;
                 }
 
                 // If this dependency is *not* a transitive dependency, then it
                 // only applies to test/example targets
-                if !d.is_transitive() && !unit.target.is_test() && !unit.target.is_example()
+                if !dep.is_transitive() && !unit.target.is_test() && !unit.target.is_example()
                     && !unit.profile.test
                 {
                     return false;
                 }
 
                 // If this dependency is only available for certain platforms,
                 // make sure we're only enabling it for that platform.
-                if !cx.dep_platform_activated(d, unit.kind) {
+                if !cx.dep_platform_activated(dep, unit.kind) {
                     return false;
                 }
 
                 // If the dependency is optional, then we're only activating it
                 // if the corresponding feature was activated
-                if d.is_optional() && !cx.resolve.features(id).contains(&*d.name()) {
+                if dep.is_optional() && !cx.resolve.features(id).contains(&*dep.name()) {
                     return false;
                 }
 
                 // If we've gotten past all that, then this dependency is
                 // actually used!
                 true
             })
-    }).filter_map(|id| match cx.get_package(id) {
-            Ok(pkg) => pkg.targets().iter().find(|t| t.is_lib()).map(|t| {
-                let unit = Unit {
-                    pkg,
-                    target: t,
-                    profile: lib_or_check_profile(unit, t, cx),
-                    kind: unit.kind.for_target(t),
-                };
-                Ok(unit)
-            }),
-            Err(e) => Some(Err(e)),
+        })
+        .filter_map(|(id, _)| {
+            match cx.get_package(id) {
+                Ok(pkg) => pkg.targets().iter().find(|t| t.is_lib()).map(|t| {
+                    let unit = Unit {
+                        pkg,
+                        target: t,
+                        profile: lib_or_check_profile(unit, t, cx),
+                        kind: unit.kind.for_target(t),
+                    };
+                    Ok(unit)
+                }),
+                Err(e) => Some(Err(e)),
+            }
         })
         .collect::<CargoResult<Vec<_>>>()?;
 
@@ -205,17 +206,15 @@ fn compute_deps_doc<'a, 'cfg>(
 ) -> CargoResult<Vec<Unit<'a>>> {
     let deps = cx.resolve
         .deps(unit.pkg.package_id())
-        .filter(|dep| {
-            unit.pkg
-                .dependencies()
-                .iter()
-                .filter(|d| d.name() == dep.name() && d.version_req().matches(dep.version()))
-                .any(|dep| match dep.kind() {
+        .filter(|&(_id, deps)| {
+            deps.iter().any(|dep| {
+                match dep.kind() {
                     DepKind::Normal => cx.dep_platform_activated(dep, unit.kind),
                     _ => false,
-                })
+                }
+            })
         })
-        .map(|dep| cx.get_package(dep));
+        .map(|(id, _deps)| cx.get_package(id));
 
     // To document a library, we depend on dependencies actually being
     // built. If we're documenting *all* libraries, then we also depend on

src/cargo/core/compiler/mod.rs

@@ -1094,23 +1094,30 @@ fn build_deps_args<'a, 'cfg>(
             }
             let mut v = OsString::new();
 
-            // Unfortunately right now Cargo doesn't have a great way to get a
-            // 1:1 mapping of entries in `dependencies()` to the actual crate
-            // we're depending on. Instead we're left to do some guesswork here
-            // to figure out what `Dependency` the `dep` unit corresponds to in
-            // `current` to see if we're renaming it.
-            //
-            // This I believe mostly works out for now, but we'll likely want
-            // to tighten up this in the future.
-            let name = current
-                .pkg
-                .dependencies()
-                .iter()
-                .filter(|d| d.matches_ignoring_source(dep.pkg.package_id()))
-                .filter_map(|d| d.rename())
-                .next();
-
-            v.push(name.unwrap_or(&dep.target.crate_name()));
+            let deps = {
+                let a = current.pkg.package_id();
+                let b = dep.pkg.package_id();
+                if a == b {
+                    &[]
+                } else {
+                    cx.resolve.dependencies_listed(a, b)
+                }
+            };
+
+            let crate_name = dep.target.crate_name();
+            let mut names = deps.iter()
+                .map(|d| d.rename().unwrap_or(&crate_name));
+            let name = names.next().unwrap_or(&crate_name);
+            for n in names {
+                if n == name {
+                    continue
+                }
+                bail!("multiple dependencies listed for the same crate must \
+                       all have the same name, but the dependency on `{}` \
+                       is listed as having different names", dep.pkg.package_id());
+            }
+
+            v.push(name);
             v.push("=");
             v.push(cx.files().out_dir(dep));
             v.push(&path::MAIN_SEPARATOR.to_string());

src/cargo/core/resolver/context.rs

@@ -269,17 +269,25 @@ impl Context {
         replacements
     }
 
-    pub fn graph(&self) -> Graph<PackageId> {
+    pub fn graph(&self)
+        -> (Graph<PackageId>, HashMap<(PackageId, PackageId), Vec<Dependency>>)
+    {
         let mut graph = Graph::new();
+        let mut deps = HashMap::new();
         let mut cur = &self.resolve_graph;
         while let Some(ref node) = cur.head {
             match node.0 {
-                GraphNode::Add(ref p) => graph.add(p.clone(), &[]),
-                GraphNode::Link(ref a, ref b) => graph.link(a.clone(), b.clone()),
+                GraphNode::Add(ref p) => graph.add(p.clone()),
+                GraphNode::Link(ref a, ref b, ref dep) => {
+                    graph.link(a.clone(), b.clone());
+                    deps.entry((a.clone(), b.clone()))
+                        .or_insert(Vec::new())
+                        .push(dep.clone());
+                }
             }
             cur = &node.1;
         }
-        graph
+        (graph, deps)
     }
 }
 

src/cargo/core/resolver/encode.rs

@@ -95,7 +95,7 @@ impl EncodableResolve {
             let mut g = Graph::new();
 
             for &(ref id, _) in live_pkgs.values() {
-                g.add(id.clone(), &[]);
+                g.add(id.clone());
             }
 
             for &(ref id, pkg) in live_pkgs.values() {
@@ -177,15 +177,15 @@ impl EncodableResolve {
             unused_patches.push(id);
         }
 
-        Ok(Resolve {
-            graph: g,
-            empty_features: HashSet::new(),
-            features: HashMap::new(),
+        Ok(Resolve::new(
+            g,
+            HashMap::new(),
             replacements,
+            HashMap::new(),
             checksums,
             metadata,
             unused_patches,
-        })
+        ))
     }
 }
 
@@ -347,17 +347,17 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
     where
         S: ser::Serializer,
     {
-        let mut ids: Vec<&PackageId> = self.resolve.graph.iter().collect();
+        let mut ids: Vec<_> = self.resolve.iter().collect();
         ids.sort();
 
         let encodable = ids.iter()
             .filter_map(|&id| Some(encodable_resolve_node(id, self.resolve)))
             .collect::<Vec<_>>();
 
-        let mut metadata = self.resolve.metadata.clone();
+        let mut metadata = self.resolve.metadata().clone();
 
         for id in ids.iter().filter(|id| !id.source_id().is_path()) {
-            let checksum = match self.resolve.checksums[*id] {
+            let checksum = match self.resolve.checksums()[*id] {
                 Some(ref s) => &s[..],
                 None => "<none>",
             };
@@ -398,10 +398,7 @@ fn encodable_resolve_node(id: &PackageId, resolve: &Resolve) -> EncodableDepende
         Some(id) => (Some(encodable_package_id(id)), None),
         None => {
             let mut deps = resolve
-                .graph
-                .edges(id)
-                .into_iter()
-                .flat_map(|a| a)
+                .deps_not_replaced(id)
                 .map(encodable_package_id)
                 .collect::<Vec<_>>();
             deps.sort();