From 7db89eded7453e9b8814ebe4b03b1d7bcf09d1b9 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Fri, 17 Oct 2014 12:23:10 -0700
Subject: [PATCH] Avoid visiting deps too often in resolve

If we're activating an already-active version of a dependency, then there's no
need to actually recurse into it. Instead, we can skip it if we're not enabling
any extra features in it to avoid extraneous recursion.
---
 src/cargo/core/resolver/mod.rs | 50 +++++++++++++++++++++-------------
 tests/resolve.rs               |  8 +-----
 tests/test_cargo_compile.rs    |  4 +--
 3 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 34458472860..8909d1d82bd 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -221,35 +221,47 @@ fn activate_deps<R: Registry>(cx: Context,
         log!(5, "{}[{}]>{} trying {}", parent.get_name(), cur, dep.get_name(),
              candidate.get_version());
         let mut my_cx = cx.clone();
-        {
+        let early_return = {
             my_cx.resolve.graph.link(parent.get_package_id().clone(),
                                      candidate.get_package_id().clone());
             let prev = match my_cx.activations.entry(key.clone()) {
                 Occupied(e) => e.into_mut(),
                 Vacant(e) => e.set(Vec::new()),
             };
-            if !prev.iter().any(|c| c == candidate) {
+            if prev.iter().any(|c| c == candidate) {
+                match cx.resolve.features(candidate.get_package_id()) {
+                    Some(prev_features) => {
+                        features.iter().all(|f| prev_features.contains(f))
+                    }
+                    None => features.len() == 0,
+                }
+            } else {
                 my_cx.resolve.graph.add(candidate.get_package_id().clone(), []);
                 prev.push(candidate.clone());
+                false
             }
-        }
+        };
 
-        // Dependency graphs are required to be a DAG. Non-transitive
-        // dependencies (dev-deps), however, can never introduce a cycle, so we
-        // skip them.
-        if dep.is_transitive() &&
-           !cx.visited.borrow_mut().insert(candidate.get_package_id().clone()) {
-            return Err(human(format!("cyclic package dependency: package `{}` \
-                                      depends on itself",
-                                     candidate.get_package_id())))
-        }
-        let my_cx = try!(activate(my_cx, registry, &**candidate, method));
-        if dep.is_transitive() {
-            cx.visited.borrow_mut().remove(candidate.get_package_id());
-        }
-        let my_cx = match my_cx {
-            Ok(cx) => cx,
-            Err(e) => { last_err = Some(e); continue }
+        let my_cx = if early_return {
+            my_cx
+        } else {
+            // Dependency graphs are required to be a DAG. Non-transitive
+            // dependencies (dev-deps), however, can never introduce a cycle, so we
+            // skip them.
+            if dep.is_transitive() &&
+               !cx.visited.borrow_mut().insert(candidate.get_package_id().clone()) {
+                return Err(human(format!("cyclic package dependency: package `{}` \
+                                          depends on itself",
+                                         candidate.get_package_id())))
+            }
+            let my_cx = try!(activate(my_cx, registry, &**candidate, method));
+            if dep.is_transitive() {
+                cx.visited.borrow_mut().remove(candidate.get_package_id());
+            }
+            match my_cx {
+                Ok(cx) => cx,
+                Err(e) => { last_err = Some(e); continue }
+            }
         };
         match try!(activate_deps(my_cx, registry, parent, deps, cur + 1)) {
             Ok(cx) => return Ok(Ok(cx)),
diff --git a/tests/resolve.rs b/tests/resolve.rs
index aa07288197d..30c8c04fca8 100644
--- a/tests/resolve.rs
+++ b/tests/resolve.rs
@@ -345,13 +345,7 @@ fn resolving_cycle() {
         pkg!("foo" => ["foo"]),
     ));
 
-    let res = resolve(pkg_id("root"), vec![
+    let _ = resolve(pkg_id("root"), vec![
         dep_req("foo", "1"),
     ], &mut reg);
-    assert!(res.is_err());
-
-    assert_eq!(res.to_string().as_slice(), "Err(\
-cyclic package dependency: package `foo v1.0.0 (registry http://example.com/)` \
-depends on itself\
-)");
 }
diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs
index 83164d13c0a..d55b2e0afb9 100644
--- a/tests/test_cargo_compile.rs
+++ b/tests/test_cargo_compile.rs
@@ -1091,9 +1091,7 @@ test!(self_dependency {
         "#)
         .file("src/test.rs", "fn main() {}");
     assert_that(p.cargo_process("build"),
-                execs().with_status(101).with_stderr("\
-cyclic package dependency: package `test v0.0.0 ([..])` depends on itself
-"));
+                execs().with_status(0));
 })
 
 #[cfg(not(windows))]