Auto merge of #6995 - Eh2406:new-test-is-worng, r=@alexcrichton

the testing SAT solver was messed up by a refactor

This fixes a mistake in #6980 introduced in [this commit](c68334f#diff-4317936c037e49f70800a86656c67569L308).
This also reverts [8458661](8458661) with some test cases to show that it was wrong.

This only causes problems when proptest is set to make public dependencies (witch is not true on master) and it gens a `reverse_alphabetical` example. Despite the low impact of these bugs, I would like it to be left incorrect as short as possible.

Author: bors

src/cargo/core/resolver/context.rs

@@ -57,7 +57,7 @@ pub type Activations =
 /// A type that represents when cargo treats two Versions as compatible.
 /// Versions `a` and `b` are compatible if their left-most nonzero digit is the
 /// same.
-#[derive(Clone, Copy, Eq, PartialEq, Hash)]
+#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
 pub enum SemverCompatibility {
     Major(NonZeroU64),
     Minor(NonZeroU64),

tests/testsuite/resolve.rs

@@ -357,7 +357,7 @@ fn public_dependency_skipping() {
     ];
     let reg = registry(input);
 
-    resolve(pkg_id("root"), vec![dep("c")], &reg).unwrap();
+    resolve_and_validated(pkg_id("root"), vec![dep("c")], &reg, None).unwrap();
 }
 
 #[test]
@@ -377,7 +377,50 @@ fn public_dependency_skipping_in_backtracking() {
     ];
     let reg = registry(input);
 
-    resolve(pkg_id("root"), vec![dep("C")], &reg).unwrap();
+    resolve_and_validated(pkg_id("root"), vec![dep("C")], &reg, None).unwrap();
+}
+
+#[test]
+fn public_sat_topological_order() {
+    let input = vec![
+        pkg!(("a", "0.0.1")),
+        pkg!(("a", "0.0.0")),
+        pkg!(("b", "0.0.1") => [dep_req_kind("a", "= 0.0.1", Kind::Normal, true),]),
+        pkg!(("b", "0.0.0") => [dep("bad"),]),
+        pkg!("A" => [dep_req("a", "= 0.0.0"),dep_req_kind("b", "*", Kind::Normal, true)]),
+    ];
+
+    let reg = registry(input);
+    assert!(resolve_and_validated(pkg_id("root"), vec![dep("A")], &reg, None).is_err());
+}
+
+#[test]
+fn public_sat_unused_makes_things_pub() {
+    let input = vec![
+        pkg!(("a", "0.0.1")),
+        pkg!(("a", "0.0.0")),
+        pkg!(("b", "8.0.1") => [dep_req_kind("a", "= 0.0.1", Kind::Normal, true),]),
+        pkg!(("b", "8.0.0") => [dep_req("a", "= 0.0.1"),]),
+        pkg!("c" => [dep_req("b", "= 8.0.0"),dep_req("a", "= 0.0.0"),]),
+    ];
+    let reg = registry(input);
+
+    resolve_and_validated(pkg_id("root"), vec![dep("c")], &reg, None).unwrap();
+}
+
+#[test]
+fn public_sat_unused_makes_things_pub_2() {
+    let input = vec![
+        pkg!(("c", "0.0.2")),
+        pkg!(("c", "0.0.1")),
+        pkg!(("a-sys", "0.0.2")),
+        pkg!(("a-sys", "0.0.1") => [dep_req_kind("c", "= 0.0.1", Kind::Normal, true),]),
+        pkg!("P" => [dep_req_kind("a-sys", "*", Kind::Normal, true),dep_req("c", "= 0.0.1"),]),
+        pkg!("A" => [dep("P"),dep_req("c", "= 0.0.2"),]),
+    ];
+    let reg = registry(input);
+
+    resolve_and_validated(pkg_id("root"), vec![dep("A")], &reg, None).unwrap();
 }
 
 #[test]

tests/testsuite/support/resolver.rs

@@ -203,10 +203,12 @@ fn sat_at_most_one(solver: &mut impl varisat::ExtendFormula, vars: &[varisat::Va
         return;
     } else if vars.len() == 2 {
         solver.add_clause(&[vars[0].negative(), vars[1].negative()]);
+        return;
     } else if vars.len() == 3 {
         solver.add_clause(&[vars[0].negative(), vars[1].negative()]);
         solver.add_clause(&[vars[0].negative(), vars[2].negative()]);
         solver.add_clause(&[vars[1].negative(), vars[2].negative()]);
+        return;
     }
     // use the "Binary Encoding" from
     // https://www.it.uu.se/research/group/astra/ModRef10/papers/Alan%20M.%20Frisch%20and%20Paul%20A.%20Giannoros.%20SAT%20Encodings%20of%20the%20At-Most-k%20Constraint%20-%20ModRef%202010.pdf
@@ -305,6 +307,7 @@ impl SatResolve {
                     .iter()
                     .filter(|&p| dep.matches_id(*p))
                 {
+                    graph.link(p.package_id(), m);
                     by_key
                         .entry(m.as_activations_key())
                         .or_default()
@@ -374,12 +377,12 @@ impl SatResolve {
         // we already ensure there is only one version for each `activations_key` so we can think of
         // `can_see` as being in terms of a set of `activations_key`s
         // and if `p` `publicly_exports` `export` then it `can_see` `export`
-        let mut can_see: HashMap<_, HashMap<_, varisat::Var>> = publicly_exports.clone();
+        let mut can_see: HashMap<_, HashMap<_, varisat::Var>> = HashMap::new();
 
         // if `p` has a `dep` that selected `ver` then it `can_see` all the things that the selected version `publicly_exports`
         for (&p, deps) in version_selected_for.iter() {
-            let p_can_see = can_see.entry(p.as_activations_key()).or_default();
-            for (_, versions) in deps.iter().filter(|(d, _)| !d.is_public()) {
+            let p_can_see = can_see.entry(p).or_default();
+            for (_, versions) in deps.iter() {
                 for (&ver, sel) in versions {
                     for (&export_pid, &export_var) in publicly_exports[&ver].iter() {
                         let our_var = p_can_see.entry(export_pid).or_insert_with(|| cnf.new_var());