Auto merge of #4834 - aidanhs:aphs-better-backtrack, r=alexcrichton

Make resolution backtracking smarter

There's no need to try every candidate for every dependency when backtracking - instead, only try candidates if they *could* change the eventual failure that caused backtracking in the first place, i.e.
1. if we've backtracked past the parent of the dep that failed
2. the number of activations for the dep has changed (activations are only ever added during resolution I believe)

The two new tests before:
```
$ /usr/bin/time cargo test --test resolve -- --test-threads 1 --nocapture resolving_with_constrained_sibling_
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/deps/resolve-19b2a13e5a19eed8
38.45user 2.16system 0:42.00elapsed 96%CPU (0avgtext+0avgdata 47672maxresident)k
0inputs+1664096outputs (0major+10921minor)pagefaults 0swaps
```
After:
```
$ /usr/bin/time cargo test --test resolve -- --test-threads 1 --nocapture resolving_with_constrained_sibling_
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/deps/resolve-19b2a13e5a19eed8
[...]
0.36user 0.01system 0:00.49elapsed 76%CPU (0avgtext+0avgdata 47464maxresident)k
0inputs+32outputs (0major+11602minor)pagefaults 0swaps
```

You observe the issue yourself with the following (it should fail, but hangs for a while instead - I didn't bother timing it and waiting for it to finish. With this PR it terminates almost instantly):
```
$ cargo new --bin x
     Created binary (application) `x` project
$ /bin/echo -e 'serde = "=1.0.9"\nrust-s3 = "0.8"' >> x/Cargo.toml
$ cd x && cargo build
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Resolving dependency graph...
```

src/cargo/core/resolver/mod.rs

@@ -745,22 +745,38 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>,
     Ok(cx)
 }
 
-// Searches up `backtrack_stack` until it finds a dependency with remaining
-// candidates. Resets `cx` and `remaining_deps` to that level and returns the
+// Looks through the states in `backtrack_stack` for dependencies with
+// remaining candidates. For each one, also checks if rolling back
+// could change the outcome of the failed resolution that caused backtracking
+// in the first place - namely, if we've backtracked past the parent of the
+// failed dep, or the previous number of activations of the failed dep has
+// changed (possibly relaxing version constraints). If the outcome could differ,
+// resets `cx` and `remaining_deps` to that level and returns the
 // next candidate. If all candidates have been exhausted, returns None.
+// Read https://github.com/rust-lang/cargo/pull/4834#issuecomment-362871537 for
+// a more detailed explanation of the logic here.
 fn find_candidate<'a>(backtrack_stack: &mut Vec<BacktrackFrame<'a>>,
                       cx: &mut Context<'a>,
                       remaining_deps: &mut BinaryHeap<DepsFrame>,
                       parent: &mut Summary,
                       cur: &mut usize,
                       dep: &mut Dependency,
                       features: &mut Rc<Vec<String>>) -> Option<Candidate> {
+    let num_dep_prev_active = cx.prev_active(dep).len();
     while let Some(mut frame) = backtrack_stack.pop() {
         let (next, has_another) = {
             let prev_active = frame.context_backup.prev_active(&frame.dep);
             (frame.remaining_candidates.next(prev_active),
              frame.remaining_candidates.clone().next(prev_active).is_some())
         };
+        let cur_num_dep_prev_active = frame.context_backup.prev_active(dep).len();
+        // Activations should monotonically decrease during backtracking
+        assert!(cur_num_dep_prev_active <= num_dep_prev_active);
+        let maychange = !frame.context_backup.is_active(parent) ||
+            cur_num_dep_prev_active != num_dep_prev_active;
+        if !maychange {
+            continue
+        }
         if let Some(candidate) = next {
             *cur = frame.cur;
             if has_another {
@@ -1178,6 +1194,14 @@ impl<'a> Context<'a> {
             .unwrap_or(&[])
     }
 
+    fn is_active(&mut self, summary: &Summary) -> bool {
+        let id = summary.package_id();
+        self.activations.get(id.name())
+            .and_then(|v| v.get(id.source_id()))
+            .map(|v| v.iter().any(|s| s == summary))
+            .unwrap_or(false)
+    }
+
     /// Return all dependencies and the features we want from them.
     fn resolve_features<'b>(&mut self,
                             s: &'b Summary,

tests/resolve.rs

@@ -74,6 +74,13 @@ impl ToPkgId for (&'static str, &'static str) {
     }
 }
 
+impl ToPkgId for (&'static str, String) {
+    fn to_pkgid(&self) -> PackageId {
+        let (name, ref vers) = *self;
+        PackageId::new(name, vers, &registry_loc()).unwrap()
+    }
+}
+
 macro_rules! pkg {
     ($pkgid:expr => [$($deps:expr),+]) => ({
         let d: Vec<Dependency> = vec![$($deps.to_dep()),+];
@@ -364,6 +371,137 @@ fn resolving_with_deep_backtracking() {
                                        ("baz", "1.0.1")])));
 }
 
+#[test]
+fn resolving_with_constrained_sibling_backtrack_parent() {
+    // There is no point in considering all of the backtrack_trap{1,2}
+    // candidates since they can't change the result of failing to
+    // resolve 'constrained'. Cargo should (ideally) skip past them and resume
+    // resolution once the activation of the parent, 'bar', is rolled back.
+    // Note that the traps are slightly more constrained to make sure they
+    // get picked first.
+    let mut reglist = vec![
+        pkg!(("foo", "1.0.0") => [dep_req("bar", "1.0"),
+                                  dep_req("constrained", "=1.0.0")]),
+
+        pkg!(("bar", "1.0.0") => [dep_req("backtrack_trap1", "1.0.2"),
+                                  dep_req("backtrack_trap2", "1.0.2"),
+                                  dep_req("constrained", "1.0.0")]),
+        pkg!(("constrained", "1.0.0")),
+        pkg!(("backtrack_trap1", "1.0.0")),
+        pkg!(("backtrack_trap2", "1.0.0")),
+    ];
+    // Bump this to make the test harder - it adds more versions of bar that will
+    // fail to resolve, and more versions of the traps to consider.
+    const NUM_BARS_AND_TRAPS: usize = 50; // minimum 2
+    for i in 1..NUM_BARS_AND_TRAPS {
+        let vsn = format!("1.0.{}", i);
+        reglist.push(pkg!(("bar", vsn.clone()) => [dep_req("backtrack_trap1", "1.0.2"),
+                                                   dep_req("backtrack_trap2", "1.0.2"),
+                                                   dep_req("constrained", "1.0.1")]));
+        reglist.push(pkg!(("backtrack_trap1", vsn.clone())));
+        reglist.push(pkg!(("backtrack_trap2", vsn.clone())));
+        reglist.push(pkg!(("constrained", vsn.clone())));
+    }
+    let reg = registry(reglist);
+
+    let res = resolve(&pkg_id("root"), vec![
+        dep_req("foo", "1"),
+    ], &reg).unwrap();
+
+    assert_that(&res, contains(names(&[("root", "1.0.0"),
+                                       ("foo", "1.0.0"),
+                                       ("bar", "1.0.0"),
+                                       ("constrained", "1.0.0")])));
+}
+
+#[test]
+fn resolving_with_constrained_sibling_backtrack_activation() {
+    // It makes sense to resolve most-constrained deps first, but
+    // with that logic the backtrack traps here come between the two
+    // attempted resolutions of 'constrained'. When backtracking,
+    // cargo should skip past them and resume resolution once the
+    // number of activations for 'constrained' changes.
+    let mut reglist = vec![
+        pkg!(("foo", "1.0.0") => [dep_req("bar", "=1.0.0"),
+                                  dep_req("backtrack_trap1", "1.0"),
+                                  dep_req("backtrack_trap2", "1.0"),
+                                  dep_req("constrained", "<=1.0.60")]),
+        pkg!(("bar", "1.0.0") => [dep_req("constrained", ">=1.0.60")]),
+    ];
+    // Bump these to make the test harder, but you'll also need to
+    // change the version constraints on `constrained` above. To correctly
+    // exercise Cargo, the relationship between the values is:
+    // NUM_CONSTRAINED - vsn < NUM_TRAPS < vsn
+    // to make sure the traps are resolved between `constrained`.
+    const NUM_TRAPS: usize = 45; // min 1
+    const NUM_CONSTRAINED: usize = 100; // min 1
+    for i in 0..NUM_TRAPS {
+        let vsn = format!("1.0.{}", i);
+        reglist.push(pkg!(("backtrack_trap1", vsn.clone())));
+        reglist.push(pkg!(("backtrack_trap2", vsn.clone())));
+    }
+    for i in 0..NUM_CONSTRAINED {
+        let vsn = format!("1.0.{}", i);
+        reglist.push(pkg!(("constrained", vsn.clone())));
+    }
+    let reg = registry(reglist);
+
+    let res = resolve(&pkg_id("root"), vec![
+        dep_req("foo", "1"),
+    ], &reg).unwrap();
+
+    assert_that(&res, contains(names(&[("root", "1.0.0"),
+                                       ("foo", "1.0.0"),
+                                       ("bar", "1.0.0"),
+                                       ("constrained", "1.0.60")])));
+}
+
+#[test]
+fn resolving_with_constrained_sibling_transitive_dep_effects() {
+    // When backtracking due to a failed dependency, if Cargo is
+    // trying to be clever and skip irrelevant dependencies, care must
+    // be taken to not miss the transitive effects of alternatives. E.g.
+    // in the right-to-left resolution of the graph below, B may
+    // affect whether D is successfully resolved.
+    //
+    //    A
+    //  / | \
+    // B  C  D
+    // |  |
+    // C  D
+    let reg = registry(vec![
+        pkg!(("A", "1.0.0") => [dep_req("B", "1.0"),
+                                dep_req("C", "1.0"),
+                                dep_req("D", "1.0.100")]),
+
+        pkg!(("B", "1.0.0") => [dep_req("C", ">=1.0.0")]),
+        pkg!(("B", "1.0.1") => [dep_req("C", ">=1.0.1")]),
+
+        pkg!(("C", "1.0.0") => [dep_req("D", "1.0.0")]),
+        pkg!(("C", "1.0.1") => [dep_req("D", ">=1.0.1,<1.0.100")]),
+        pkg!(("C", "1.0.2") => [dep_req("D", ">=1.0.2,<1.0.100")]),
+
+        pkg!(("D", "1.0.0")),
+        pkg!(("D", "1.0.1")),
+        pkg!(("D", "1.0.2")),
+        pkg!(("D", "1.0.100")),
+        pkg!(("D", "1.0.101")),
+        pkg!(("D", "1.0.102")),
+        pkg!(("D", "1.0.103")),
+        pkg!(("D", "1.0.104")),
+        pkg!(("D", "1.0.105")),
+    ]);
+
+    let res = resolve(&pkg_id("root"), vec![
+        dep_req("A", "1"),
+    ], &reg).unwrap();
+
+    assert_that(&res, contains(names(&[("A", "1.0.0"),
+                                       ("B", "1.0.0"),
+                                       ("C", "1.0.0"),
+                                       ("D", "1.0.105")])));
+}
+
 #[test]
 fn resolving_but_no_exists() {
     let reg = registry(vec![