Auto merge of #7993 - Eh2406:deduplicate-eges, r=ehuss

De-duplicate edges

This is a quick fix for #7985. It is possible to have more than one dependency that connects two packages, if one is a dev and one a regular. The code has use a `Vec` to represent that potential multiplicity. This switches it to a `HashSet` to fix #7985. But if there is only a handful of ways we can have more than one then perhaps we can do something with less indirection/allocations.

Note that #7168 (which was already abandoned) will need to be redesigned for whatever we do for this.

src/cargo/core/resolver/context.rs

@@ -1,6 +1,5 @@
 use std::collections::HashMap;
 use std::num::NonZeroU64;
-use std::rc::Rc;
 
 use anyhow::format_err;
 use log::debug;
@@ -35,7 +34,7 @@ pub struct Context {
 
     /// a way to look up for a package in activations what packages required it
     /// and all of the exact deps that it fulfilled.
-    pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
+    pub parents: Graph<PackageId, im_rc::HashSet<Dependency>>,
 }
 
 /// When backtracking it can be useful to know how far back to go.
@@ -255,8 +254,8 @@ impl Context {
             .collect()
     }
 
-    pub fn graph(&self) -> Graph<PackageId, Vec<Dependency>> {
-        let mut graph: Graph<PackageId, Vec<Dependency>> = Graph::new();
+    pub fn graph(&self) -> Graph<PackageId, std::collections::HashSet<Dependency>> {
+        let mut graph: Graph<PackageId, std::collections::HashSet<Dependency>> = Graph::new();
         self.activations
             .values()
             .for_each(|(r, _)| graph.add(r.package_id()));
@@ -265,14 +264,14 @@ impl Context {
             for (o, e) in self.parents.edges(i) {
                 let old_link = graph.link(*o, *i);
                 assert!(old_link.is_empty());
-                *old_link = e.to_vec();
+                *old_link = e.iter().cloned().collect();
             }
         }
         graph
     }
 }
 
-impl Graph<PackageId, Rc<Vec<Dependency>>> {
+impl Graph<PackageId, im_rc::HashSet<Dependency>> {
     pub fn parents_of(&self, p: PackageId) -> impl Iterator<Item = (PackageId, bool)> + '_ {
         self.edges(&p)
             .map(|(grand, d)| (*grand, d.iter().any(|x| x.is_public())))
@@ -338,7 +337,7 @@ impl PublicDependency {
         parent_pid: PackageId,
         is_public: bool,
         age: ContextAge,
-        parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
+        parents: &Graph<PackageId, im_rc::HashSet<Dependency>>,
     ) {
         // one tricky part is that `candidate_pid` may already be active and
         // have public dependencies of its own. So we not only need to mark
@@ -383,7 +382,7 @@ impl PublicDependency {
         b_id: PackageId,
         parent: PackageId,
         is_public: bool,
-        parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
+        parents: &Graph<PackageId, im_rc::HashSet<Dependency>>,
     ) -> Result<
         (),
         (

src/cargo/core/resolver/mod.rs

@@ -609,12 +609,11 @@ fn activate(
     cx.age += 1;
     if let Some((parent, dep)) = parent {
         let parent_pid = parent.package_id();
-        Rc::make_mut(
-            // add a edge from candidate to parent in the parents graph
-            cx.parents.link(candidate_pid, parent_pid),
-        )
-        // and associate dep with that edge
-        .push(dep.clone());
+        // add a edge from candidate to parent in the parents graph
+        cx.parents
+            .link(candidate_pid, parent_pid)
+            // and associate dep with that edge
+            .insert(dep.clone());
         if let Some(public_dependency) = cx.public_dependency.as_mut() {
             public_dependency.add_edge(
                 candidate_pid,

src/cargo/core/resolver/resolve.rs

@@ -18,7 +18,7 @@ pub struct Resolve {
     /// A graph, whose vertices are packages and edges are dependency specifications
     /// from `Cargo.toml`. We need a `Vec` here because the same package
     /// might be present in both `[dependencies]` and `[build-dependencies]`.
-    graph: Graph<PackageId, Vec<Dependency>>,
+    graph: Graph<PackageId, HashSet<Dependency>>,
     /// Replacements from the `[replace]` table.
     replacements: HashMap<PackageId, PackageId>,
     /// Inverted version of `replacements`.
@@ -70,7 +70,7 @@ pub enum ResolveVersion {
 
 impl Resolve {
     pub fn new(
-        graph: Graph<PackageId, Vec<Dependency>>,
+        graph: Graph<PackageId, HashSet<Dependency>>,
         replacements: HashMap<PackageId, PackageId>,
         features: HashMap<PackageId, Vec<InternedString>>,
         checksums: HashMap<PackageId, Option<String>>,
@@ -264,18 +264,16 @@ unable to verify that `{0}` is the same as when the lockfile was generated
         self.graph.iter().cloned()
     }
 
-    pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &[Dependency])> {
+    pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
         self.deps_not_replaced(pkg)
             .map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps))
     }
 
     pub fn deps_not_replaced(
         &self,
         pkg: PackageId,
-    ) -> impl Iterator<Item = (PackageId, &[Dependency])> {
-        self.graph
-            .edges(&pkg)
-            .map(|(id, deps)| (*id, deps.as_slice()))
+    ) -> impl Iterator<Item = (PackageId, &HashSet<Dependency>)> {
+        self.graph.edges(&pkg).map(|(id, deps)| (*id, deps))
     }
 
     pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {
@@ -325,8 +323,9 @@ unable to verify that `{0}` is the same as when the lockfile was generated
         to: PackageId,
         to_target: &Target,
     ) -> CargoResult<String> {
+        let empty_set: HashSet<Dependency> = HashSet::new();
         let deps = if from == to {
-            &[]
+            &empty_set
         } else {
             self.dependencies_listed(from, to)
         };
@@ -349,7 +348,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated
         Ok(name)
     }
 
-    fn dependencies_listed(&self, from: PackageId, to: PackageId) -> &[Dependency] {
+    fn dependencies_listed(&self, from: PackageId, to: PackageId) -> &HashSet<Dependency> {
         // We've got a dependency on `from` to `to`, but this dependency edge
         // may be affected by [replace]. If the `to` package is listed as the
         // target of a replacement (aka the key of a reverse replacement map)

src/cargo/ops/cargo_output_metadata.rs

@@ -184,11 +184,7 @@ fn build_resolve_graph_r(
             CompileKind::Host => true,
         })
         .filter_map(|(dep_id, deps)| {
-            let mut dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
-            // Duplicates may appear if the same package is used by different
-            // members of a workspace with different features selected.
-            dep_kinds.sort_unstable();
-            dep_kinds.dedup();
+            let dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
             package_map
                 .get(&dep_id)
                 .and_then(|pkg| pkg.targets().iter().find(|t| t.is_lib()))