Auto merge of #7174 - alexcrichton:fix-cycle-check, r=Eh2406

Fix detection of cyclic dependencies through `[patch]`

This commit fixes detection of cyclic dependencies through the use of
`[patch]` by ensuring that `matches_id` isn't used because it returns a
false negative for registry dependencies when the dependency
specifications don't match but the resolve edges are still correct.

Closes #7163

Author: bors

src/cargo/core/resolver/encode.rs

@@ -582,7 +582,7 @@ fn encodable_resolve_node(
         None => {
             let mut deps = resolve
                 .deps_not_replaced(id)
-                .map(|id| encodable_package_id(id, state))
+                .map(|(id, _)| encodable_package_id(id, state))
                 .collect::<Vec<_>>();
             deps.sort();
             (None, Some(deps))

src/cargo/core/resolver/mod.rs

@@ -60,7 +60,7 @@ use crate::util::config::Config;
 use crate::util::errors::CargoResult;
 use crate::util::profile;
 
-use self::context::{Activations, Context};
+use self::context::Context;
 use self::dep_cache::RegistryQueryer;
 use self::types::{ConflictMap, ConflictReason, DepsFrame};
 use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress};
@@ -154,7 +154,7 @@ pub fn resolve(
         ResolveVersion::default(),
     );
 
-    check_cycles(&resolve, &cx.activations)?;
+    check_cycles(&resolve)?;
     check_duplicate_pkgs_in_lockfile(&resolve)?;
     trace!("resolved: {:?}", resolve);
 
@@ -1048,27 +1048,21 @@ fn find_candidate(
     None
 }
 
-fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()> {
-    let summaries: HashMap<PackageId, &Summary> = activations
-        .values()
-        .map(|(s, _)| (s.package_id(), s))
-        .collect();
-
+fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
     // Sort packages to produce user friendly deterministic errors.
     let mut all_packages: Vec<_> = resolve.iter().collect();
     all_packages.sort_unstable();
     let mut checked = HashSet::new();
     for pkg in all_packages {
         if !checked.contains(&pkg) {
-            visit(resolve, pkg, &summaries, &mut HashSet::new(), &mut checked)?
+            visit(resolve, pkg, &mut HashSet::new(), &mut checked)?
         }
     }
     return Ok(());
 
     fn visit(
         resolve: &Resolve,
         id: PackageId,
-        summaries: &HashMap<PackageId, &Summary>,
         visited: &mut HashSet<PackageId>,
         checked: &mut HashSet<PackageId>,
     ) -> CargoResult<()> {
@@ -1089,22 +1083,18 @@ fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()>
         // visitation list as we can't induce a cycle through transitive
         // dependencies.
         if checked.insert(id) {
-            let summary = summaries[&id];
-            for dep in resolve.deps_not_replaced(id) {
-                let is_transitive = summary
-                    .dependencies()
-                    .iter()
-                    .any(|d| d.matches_id(dep) && d.is_transitive());
+            for (dep, listings) in resolve.deps_not_replaced(id) {
+                let is_transitive = listings.iter().any(|d| d.is_transitive());
                 let mut empty = HashSet::new();
                 let visited = if is_transitive {
                     &mut *visited
                 } else {
                     &mut empty
                 };
-                visit(resolve, dep, summaries, visited, checked)?;
+                visit(resolve, dep, visited, checked)?;
 
                 if let Some(id) = resolve.replacement(dep) {
-                    visit(resolve, id, summaries, visited, checked)?;
+                    visit(resolve, id, visited, checked)?;
                 }
             }
         }

src/cargo/core/resolver/resolve.rs

@@ -229,13 +229,17 @@ unable to verify that `{0}` is the same as when the lockfile was generated
     }
 
     pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &[Dependency])> {
-        self.graph
-            .edges(&pkg)
-            .map(move |&(id, ref deps)| (self.replacement(id).unwrap_or(id), deps.as_slice()))
+        self.deps_not_replaced(pkg)
+            .map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps))
     }
 
-    pub fn deps_not_replaced<'a>(&'a self, pkg: PackageId) -> impl Iterator<Item = PackageId> + 'a {
-        self.graph.edges(&pkg).map(|&(id, _)| id)
+    pub fn deps_not_replaced(
+        &self,
+        pkg: PackageId,
+    ) -> impl Iterator<Item = (PackageId, &[Dependency])> {
+        self.graph
+            .edges(&pkg)
+            .map(|(id, deps)| (*id, deps.as_slice()))
     }
 
     pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {

src/cargo/ops/cargo_generate_lockfile.rs

@@ -154,7 +154,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
             return;
         }
         set.insert(dep);
-        for dep in resolve.deps_not_replaced(dep) {
+        for (dep, _) in resolve.deps_not_replaced(dep) {
             fill_with_deps(resolve, dep, set, visited);
         }
     }

src/cargo/ops/resolve.rs

@@ -572,7 +572,11 @@ fn register_previous_locks(
     let keep = |id: &PackageId| keep(id) && !avoid_locking.contains(id);
 
     for node in resolve.iter().filter(keep) {
-        let deps = resolve.deps_not_replaced(node).filter(keep).collect();
+        let deps = resolve
+            .deps_not_replaced(node)
+            .map(|p| p.0)
+            .filter(keep)
+            .collect();
         registry.register_lock(node, deps);
     }
 
@@ -582,7 +586,7 @@ fn register_previous_locks(
             return;
         }
         debug!("ignoring any lock pointing directly at {}", node);
-        for dep in resolve.deps_not_replaced(node) {
+        for (dep, _) in resolve.deps_not_replaced(node) {
             add_deps(resolve, dep, set);
         }
     }

tests/testsuite/patch.rs

@@ -1079,3 +1079,58 @@ fn patch_older() {
         )
         .run();
 }
+
+#[cargo_test]
+fn cycle() {
+    Package::new("a", "1.0.0").publish();
+    Package::new("b", "1.0.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [workspace]
+                members = ["a", "b"]
+
+                [patch.crates-io]
+                a = {path="a"}
+                b = {path="b"}
+            "#,
+        )
+        .file(
+            "a/Cargo.toml",
+            r#"
+                [package]
+                name = "a"
+                version = "1.0.0"
+
+                [dependencies]
+                b = "1.0"
+            "#,
+        )
+        .file("a/src/lib.rs", "")
+        .file(
+            "b/Cargo.toml",
+            r#"
+                [package]
+                name = "b"
+                version = "1.0.0"
+
+                [dependencies]
+                a = "1.0"
+            "#,
+        )
+        .file("b/src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[UPDATING] [..]
+error: cyclic package dependency: [..]
+package `[..]`
+    ... which is depended on by `[..]`
+",
+        )
+        .run();
+}