Resolver test/debug cleanup

crates/resolver-tests/src/lib.rs

@@ -1,7 +1,10 @@
+use std::cell::RefCell;
 use std::cmp::PartialEq;
 use std::cmp::{max, min};
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 use std::fmt;
+use std::fmt::Write;
+use std::rc::Rc;
 use std::time::Instant;
 
 use cargo::core::dependency::Kind;
@@ -17,92 +20,99 @@ use proptest::sample::Index;
 use proptest::string::string_regex;
 use varisat::{self, ExtendFormula};
 
-pub fn resolve(
-    pkg: PackageId,
-    deps: Vec<Dependency>,
-    registry: &[Summary],
-) -> CargoResult<Vec<PackageId>> {
-    resolve_with_config(pkg, deps, registry, None)
+pub fn resolve(deps: Vec<Dependency>, registry: &[Summary]) -> CargoResult<Vec<PackageId>> {
+    resolve_with_config(deps, registry, None)
 }
 
 pub fn resolve_and_validated(
-    pkg: PackageId,
     deps: Vec<Dependency>,
     registry: &[Summary],
-    sat_resolve: Option<&mut SatResolve>,
+    sat_resolve: Option<SatResolve>,
 ) -> CargoResult<Vec<PackageId>> {
-    let should_resolve = if let Some(sat) = sat_resolve {
-        sat.sat_resolve(&deps)
-    } else {
-        SatResolve::new(registry).sat_resolve(&deps)
-    };
-    let resolve = resolve_with_config_raw(pkg, deps, registry, None);
-    assert_eq!(resolve.is_ok(), should_resolve);
-
-    let resolve = resolve?;
-    let mut stack = vec![pkg];
-    let mut used = HashSet::new();
-    let mut links = HashSet::new();
-    while let Some(p) = stack.pop() {
-        assert!(resolve.contains(&p));
-        if used.insert(p) {
-            // in the tests all `links` crates end in `-sys`
-            if p.name().ends_with("-sys") {
-                assert!(links.insert(p.name()));
+    let resolve = resolve_with_config_raw(deps.clone(), registry, None);
+
+    match resolve {
+        Err(e) => {
+            let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry));
+            if sat_resolve.sat_resolve(&deps) {
+                panic!(
+                    "the resolve err but the sat_resolve thinks this will work:\n{}",
+                    sat_resolve.use_packages().unwrap()
+                );
             }
-            stack.extend(resolve.deps(p).map(|(dp, deps)| {
-                for d in deps {
-                    assert!(d.matches_id(dp));
-                }
-                dp
-            }));
+            Err(e)
         }
-    }
-    let out = resolve.sort();
-    assert_eq!(out.len(), used.len());
-
-    let mut pub_deps: HashMap<PackageId, HashSet<_>> = HashMap::new();
-    for &p in out.iter() {
-        // make the list of `p` public dependencies
-        let mut self_pub_dep = HashSet::new();
-        self_pub_dep.insert(p);
-        for (dp, deps) in resolve.deps(p) {
-            if deps.iter().any(|d| d.is_public()) {
-                self_pub_dep.extend(pub_deps[&dp].iter().cloned())
+        Ok(resolve) => {
+            let mut stack = vec![pkg_id("root")];
+            let mut used = HashSet::new();
+            let mut links = HashSet::new();
+            while let Some(p) = stack.pop() {
+                assert!(resolve.contains(&p));
+                if used.insert(p) {
+                    // in the tests all `links` crates end in `-sys`
+                    if p.name().ends_with("-sys") {
+                        assert!(links.insert(p.name()));
+                    }
+                    stack.extend(resolve.deps(p).map(|(dp, deps)| {
+                        for d in deps {
+                            assert!(d.matches_id(dp));
+                        }
+                        dp
+                    }));
+                }
             }
-        }
-        pub_deps.insert(p, self_pub_dep);
+            let out = resolve.sort();
+            assert_eq!(out.len(), used.len());
+
+            let mut pub_deps: HashMap<PackageId, HashSet<_>> = HashMap::new();
+            for &p in out.iter() {
+                // make the list of `p` public dependencies
+                let mut self_pub_dep = HashSet::new();
+                self_pub_dep.insert(p);
+                for (dp, deps) in resolve.deps(p) {
+                    if deps.iter().any(|d| d.is_public()) {
+                        self_pub_dep.extend(pub_deps[&dp].iter().cloned())
+                    }
+                }
+                pub_deps.insert(p, self_pub_dep);
 
-        // check if `p` has a public dependencies conflicts
-        let seen_dep: BTreeSet<_> = resolve
-            .deps(p)
-            .flat_map(|(dp, _)| pub_deps[&dp].iter().cloned())
-            .collect();
-        let seen_dep: Vec<_> = seen_dep.iter().collect();
-        for a in seen_dep.windows(2) {
-            if a[0].name() == a[1].name() {
+                // check if `p` has a public dependencies conflicts
+                let seen_dep: BTreeSet<_> = resolve
+                    .deps(p)
+                    .flat_map(|(dp, _)| pub_deps[&dp].iter().cloned())
+                    .collect();
+                let seen_dep: Vec<_> = seen_dep.iter().collect();
+                for a in seen_dep.windows(2) {
+                    if a[0].name() == a[1].name() {
+                        panic!(
+                            "the package {:?} can publicly see {:?} and {:?}",
+                            p, a[0], a[1]
+                        )
+                    }
+                }
+            }
+            let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry));
+            if !sat_resolve.sat_is_valid_solution(&out) {
                 panic!(
-                    "the package {:?} can publicly see {:?} and {:?}",
-                    p, a[0], a[1]
-                )
+                    "the sat_resolve err but the resolve thinks this will work:\n{:?}",
+                    resolve
+                );
             }
+            Ok(out)
         }
     }
-    Ok(out)
 }
 
 pub fn resolve_with_config(
-    pkg: PackageId,
     deps: Vec<Dependency>,
     registry: &[Summary],
     config: Option<&Config>,
 ) -> CargoResult<Vec<PackageId>> {
-    let resolve = resolve_with_config_raw(pkg, deps, registry, config)?;
+    let resolve = resolve_with_config_raw(deps, registry, config)?;
     Ok(resolve.sort())
 }
 
 pub fn resolve_with_config_raw(
-    pkg: PackageId,
     deps: Vec<Dependency>,
     registry: &[Summary],
     config: Option<&Config>,
@@ -158,7 +168,7 @@ pub fn resolve_with_config_raw(
         used: HashSet::new(),
     };
     let summary = Summary::new(
-        pkg,
+        pkg_id("root"),
         deps,
         &BTreeMap::<String, Vec<String>>::new(),
         None::<String>,
@@ -241,7 +251,9 @@ fn sat_at_most_one_by_key<K: std::hash::Hash + Eq>(
 ///
 /// The SAT library dose not optimize for the newer version,
 /// so the selected packages may not match the real resolver.
-pub struct SatResolve {
+#[derive(Clone)]
+pub struct SatResolve(Rc<RefCell<SatResolveInner>>);
+struct SatResolveInner {
     solver: varisat::Solver<'static>,
     var_for_is_packages_used: HashMap<PackageId, varisat::Var>,
     by_name: HashMap<&'static str, Vec<PackageId>>,
@@ -404,50 +416,86 @@ impl SatResolve {
         solver
             .solve()
             .expect("docs say it can't error in default config");
-
-        SatResolve {
+        SatResolve(Rc::new(RefCell::new(SatResolveInner {
             solver,
             var_for_is_packages_used,
             by_name,
-        }
+        })))
     }
-    pub fn sat_resolve(&mut self, deps: &[Dependency]) -> bool {
+    pub fn sat_resolve(&self, deps: &[Dependency]) -> bool {
+        let mut s = self.0.borrow_mut();
         let mut assumption = vec![];
         let mut this_call = None;
 
         // the starting `deps` need to be satisfied
         for dep in deps.iter() {
             let empty_vec = vec![];
-            let matches: Vec<varisat::Lit> = self
+            let matches: Vec<varisat::Lit> = s
                 .by_name
                 .get(dep.package_name().as_str())
                 .unwrap_or(&empty_vec)
                 .iter()
                 .filter(|&p| dep.matches_id(*p))
-                .map(|p| self.var_for_is_packages_used[p].positive())
+                .map(|p| s.var_for_is_packages_used[p].positive())
                 .collect();
             if matches.is_empty() {
                 return false;
             } else if matches.len() == 1 {
                 assumption.extend_from_slice(&matches)
             } else {
                 if this_call.is_none() {
-                    let new_var = self.solver.new_var();
+                    let new_var = s.solver.new_var();
                     this_call = Some(new_var);
                     assumption.push(new_var.positive());
                 }
                 let mut matches = matches;
                 matches.push(this_call.unwrap().negative());
-                self.solver.add_clause(&matches);
+                s.solver.add_clause(&matches);
+            }
+        }
+
+        s.solver.assume(&assumption);
+
+        s.solver
+            .solve()
+            .expect("docs say it can't error in default config")
+    }
+    pub fn sat_is_valid_solution(&self, pids: &[PackageId]) -> bool {
+        let mut s = self.0.borrow_mut();
+        for p in pids {
+            if p.name().as_str() != "root" && !s.var_for_is_packages_used.contains_key(p) {
+                return false;
             }
         }
+        let assumption: Vec<_> = s
+            .var_for_is_packages_used
+            .iter()
+            .map(|(p, v)| v.lit(pids.contains(p)))
+            .collect();
 
-        self.solver.assume(&assumption);
+        s.solver.assume(&assumption);
 
-        self.solver
+        s.solver
             .solve()
             .expect("docs say it can't error in default config")
     }
+    fn use_packages(&self) -> Option<String> {
+        self.0.borrow().solver.model().map(|lits| {
+            let lits: HashSet<_> = lits
+                .iter()
+                .filter(|l| l.is_positive())
+                .map(|l| l.var())
+                .collect();
+            let mut out = String::new();
+            out.push_str("used:\n");
+            for (p, v) in self.0.borrow().var_for_is_packages_used.iter() {
+                if lits.contains(v) {
+                    writeln!(&mut out, "    {}", p).unwrap();
+                }
+            }
+            out
+        })
+    }
 }
 
 pub trait ToDep {
@@ -856,7 +904,6 @@ fn meta_test_deep_trees_from_strategy() {
         let reg = registry(input.clone());
         for this in input.iter().rev().take(10) {
             let res = resolve(
-                pkg_id("root"),
                 vec![dep_req(&this.name(), &format!("={}", this.version()))],
                 &reg,
             );
@@ -898,7 +945,6 @@ fn meta_test_multiple_versions_strategy() {
         let reg = registry(input.clone());
         for this in input.iter().rev().take(10) {
             let res = resolve(
-                pkg_id("root"),
                 vec![dep_req(&this.name(), &format!("={}", this.version()))],
                 &reg,
             );

src/cargo/core/dependency.rs

@@ -328,6 +328,10 @@ impl Dependency {
     }
 
     pub fn set_kind(&mut self, kind: Kind) -> &mut Dependency {
+        if self.is_public() {
+            // Setting 'public' only makes sense for normal dependencies
+            assert_eq!(kind, Kind::Normal);
+        }
         Rc::make_mut(&mut self.inner).kind = kind;
         self
     }

src/cargo/core/resolver/conflict_cache.rs

@@ -25,6 +25,7 @@ impl ConflictStoreTrie {
         &self,
         is_active: &impl Fn(PackageId) -> Option<usize>,
         must_contain: Option<PackageId>,
+        mut max_age: usize,
     ) -> Option<(&ConflictMap, usize)> {
         match self {
             ConflictStoreTrie::Leaf(c) => {
@@ -43,8 +44,12 @@ impl ConflictStoreTrie {
                 {
                     // If the key is active, then we need to check all of the corresponding subtrie.
                     if let Some(age_this) = is_active(pid) {
+                        if age_this >= max_age && must_contain != Some(pid) {
+                            // not worth looking at, it is to old.
+                            continue;
+                        }
                         if let Some((o, age_o)) =
-                            store.find(is_active, must_contain.filter(|&f| f != pid))
+                            store.find(is_active, must_contain.filter(|&f| f != pid), max_age)
                         {
                             let age = if must_contain == Some(pid) {
                                 // all the results will include `must_contain`
@@ -53,10 +58,11 @@ impl ConflictStoreTrie {
                             } else {
                                 std::cmp::max(age_this, age_o)
                             };
-                            let out_age = out.get_or_insert((o, age)).1;
-                            if out_age > age {
+                            if max_age > age {
                                 // we found one that can jump-back further so replace the out.
                                 out = Some((o, age));
+                                // and dont look at anything older
+                                max_age = age
                             }
                         }
                     }
@@ -152,10 +158,11 @@ impl ConflictCache {
         dep: &Dependency,
         is_active: &impl Fn(PackageId) -> Option<usize>,
         must_contain: Option<PackageId>,
+        max_age: usize,
     ) -> Option<&ConflictMap> {
         self.con_from_dep
             .get(dep)?
-            .find(is_active, must_contain)
+            .find(is_active, must_contain, max_age)
             .map(|(c, _)| c)
     }
     /// Finds any known set of conflicts, if any,
@@ -168,7 +175,7 @@ impl ConflictCache {
         dep: &Dependency,
         must_contain: Option<PackageId>,
     ) -> Option<&ConflictMap> {
-        let out = self.find(dep, &|id| cx.is_active(id), must_contain);
+        let out = self.find(dep, &|id| cx.is_active(id), must_contain, std::usize::MAX);
         if cfg!(debug_assertions) {
             if let Some(c) = &out {
                 assert!(cx.is_conflicting(None, c).is_some());

src/cargo/core/resolver/mod.rs

@@ -208,7 +208,7 @@ fn activate_deps_loop(
     while let Some((just_here_for_the_error_messages, frame)) =
         remaining_deps.pop_most_constrained()
     {
-        let (mut parent, (mut cur, (mut dep, candidates, mut features))) = frame;
+        let (mut parent, (mut dep, candidates, mut features)) = frame;
 
         // If we spend a lot of time here (we shouldn't in most cases) then give
         // a bit of a visual indicator as to what we're doing.
@@ -217,7 +217,7 @@ fn activate_deps_loop(
         trace!(
             "{}[{}]>{} {} candidates",
             parent.name(),
-            cur,
+            cx.age(),
             dep.package_name(),
             candidates.len()
         );
@@ -263,7 +263,7 @@ fn activate_deps_loop(
                 trace!(
                     "{}[{}]>{} -- no candidates",
                     parent.name(),
-                    cur,
+                    cx.age(),
                     dep.package_name()
                 );
 
@@ -308,7 +308,6 @@ fn activate_deps_loop(
                     Some((candidate, has_another, frame)) => {
                         // Reset all of our local variables used with the
                         // contents of `frame` to complete our backtrack.
-                        cur = frame.cur;
                         cx = frame.context;
                         remaining_deps = frame.remaining_deps;
                         remaining_candidates = frame.remaining_candidates;
@@ -353,7 +352,6 @@ fn activate_deps_loop(
             // if we can.
             let backtrack = if has_another {
                 Some(BacktrackFrame {
-                    cur,
                     context: Context::clone(&cx),
                     remaining_deps: remaining_deps.clone(),
                     remaining_candidates: remaining_candidates.clone(),
@@ -376,7 +374,7 @@ fn activate_deps_loop(
             trace!(
                 "{}[{}]>{} trying {}",
                 parent.name(),
-                cur,
+                cx.age(),
                 dep.package_name(),
                 candidate.version()
             );
@@ -409,7 +407,7 @@ fn activate_deps_loop(
                         if let Some(conflicting) = frame
                             .remaining_siblings
                             .clone()
-                            .filter_map(|(_, (ref new_dep, _, _))| {
+                            .filter_map(|(ref new_dep, _, _)| {
                                 past_conflicting_activations.conflicting(&cx, new_dep)
                             })
                             .next()
@@ -526,7 +524,7 @@ fn activate_deps_loop(
                         trace!(
                             "{}[{}]>{} skipping {} ",
                             parent.name(),
-                            cur,
+                            cx.age(),
                             dep.package_name(),
                             pid.version()
                         );
@@ -700,7 +698,6 @@ fn activate(
 
 #[derive(Clone)]
 struct BacktrackFrame {
-    cur: usize,
     context: Context,
     remaining_deps: RemainingDeps,
     remaining_candidates: RemainingCandidates,
@@ -759,7 +756,7 @@ impl RemainingCandidates {
         dep: &Dependency,
         parent: PackageId,
     ) -> Option<(Summary, bool)> {
-        'main: for (_, b) in self.remaining.by_ref() {
+        'main: for b in self.remaining.by_ref() {
             let b_id = b.package_id();
             // The `links` key in the manifest dictates that there's only one
             // package in a dependency graph, globally, with that particular
@@ -905,12 +902,12 @@ fn generalize_conflicting(
                                     // we are imagining that we used other instead
                                     Some(backtrack_critical_age)
                                 } else {
-                                    cx.is_active(id).filter(|&age|
-                                        // we only care about things that are older then critical_age
-                                        age < backtrack_critical_age)
+                                    cx.is_active(id)
                                 }
                             },
                             Some(other.package_id()),
+                            // we only care about things that are newer then critical_age
+                            backtrack_critical_age,
                         )
                         .map(|con| (other.package_id(), con))
                 })
@@ -986,7 +983,12 @@ fn find_candidate(
             .any(|c| *c == ConflictReason::PublicDependency)
     {
         // we dont have abnormal situations. So we can ask `cx` for how far back we need to go.
-        cx.is_conflicting(Some(parent.package_id()), conflicting_activations)
+        let a = cx.is_conflicting(Some(parent.package_id()), conflicting_activations);
+        // If the `conflicting_activations` does not apply to `cx`, then something went very wrong
+        // in building it. But we will just fall back to laboriously trying all possibilities witch
+        // will give us the correct answer so only `assert` if there is a developer to debug it.
+        debug_assert!(a.is_some());
+        a
     } else {
         None
     };

src/cargo/core/resolver/resolve.rs

@@ -49,19 +49,11 @@ impl Resolve {
             .map(|p| {
                 let public_deps = graph
                     .edges(p)
-                    .flat_map(|(dep_package, deps)| {
-                        let id_opt: Option<PackageId> = deps
-                            .iter()
-                            .find(|d| d.kind() == Kind::Normal)
-                            .and_then(|d| {
-                                if d.is_public() {
-                                    Some(*dep_package)
-                                } else {
-                                    None
-                                }
-                            });
-                        id_opt
+                    .filter(|(_, deps)| {
+                        deps.iter()
+                            .any(|d| d.kind() == Kind::Normal && d.is_public())
                     })
+                    .map(|(dep_package, _)| *dep_package)
                     .collect::<HashSet<PackageId>>();
 
                 (*p, public_deps)

src/cargo/core/resolver/types.rs

@@ -146,7 +146,7 @@ impl DepsFrame {
     pub fn flatten<'a>(&'a self) -> impl Iterator<Item = (PackageId, Dependency)> + 'a {
         self.remaining_siblings
             .clone()
-            .map(move |(_, (d, _, _))| (self.parent.package_id(), d))
+            .map(move |(d, _, _)| (self.parent.package_id(), d))
     }
 }
 
@@ -202,7 +202,7 @@ impl RemainingDeps {
         self.data.insert((x, insertion_time));
         self.time += 1;
     }
-    pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> {
+    pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, DepInfo))> {
         while let Some((mut deps_frame, insertion_time)) = self.data.remove_min() {
             let just_here_for_the_error_messages = deps_frame.just_for_error_messages;
 
@@ -325,12 +325,10 @@ impl<T> Iterator for RcVecIter<T>
 where
     T: Clone,
 {
-    type Item = (usize, T);
+    type Item = T;
 
     fn next(&mut self) -> Option<Self::Item> {
-        self.rest
-            .next()
-            .and_then(|i| self.vec.get(i).map(|val| (i, val.clone())))
+        self.rest.next().and_then(|i| self.vec.get(i).cloned())
     }
 
     fn size_hint(&self) -> (usize, Option<usize>) {