fix(resolver): Remove unused public-deps error handling

To implement rust-lang/rfcs#3516, we need to decouple the resolver's
behavior from the unstable flag.  Since the code path is now dead, I
went ahead and removed it.

Author: epage

crates/resolver-tests/src/lib.rs

@@ -3,7 +3,7 @@
 use std::cell::RefCell;
 use std::cmp::PartialEq;
 use std::cmp::{max, min};
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::fmt;
 use std::fmt::Write;
 use std::rc::Rc;
@@ -70,33 +70,6 @@ pub fn resolve_and_validated(
             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() {
-                        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!(
@@ -201,7 +174,6 @@ pub fn resolve_with_config_raw(
         &mut registry,
         &version_prefs,
         Some(config),
-        true,
     );
 
     // The largest test in our suite takes less then 30 sec.

crates/resolver-tests/tests/resolve.rs

@@ -6,8 +6,8 @@ use cargo::util::Config;
 use cargo_util::is_ci;
 
 use resolver_tests::{
-    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names,
-    pkg, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated,
+    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_id,
+    pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated,
     resolve_with_config, PrettyPrintRegistry, SatResolve, ToDep, ToPkgId,
 };
 
@@ -287,192 +287,6 @@ proptest! {
     }
 }
 
-#[test]
-#[should_panic(expected = "pub dep")] // The error handling is not yet implemented.
-fn pub_fail() {
-    let input = vec![
-        pkg!(("a", "0.0.4")),
-        pkg!(("a", "0.0.5")),
-        pkg!(("e", "0.0.6") => [dep_req_kind("a", "<= 0.0.4", DepKind::Normal, true),]),
-        pkg!(("kB", "0.0.3") => [dep_req("a", ">= 0.0.5"),dep("e"),]),
-    ];
-    let reg = registry(input);
-    assert!(resolve_and_validated(vec![dep("kB")], &reg, None).is_err());
-}
-
-#[test]
-fn basic_public_dependency() {
-    let reg = registry(vec![
-        pkg!(("A", "0.1.0")),
-        pkg!(("A", "0.2.0")),
-        pkg!("B" => [dep_req_kind("A", "0.1", DepKind::Normal, true)]),
-        pkg!("C" => [dep("A"), dep("B")]),
-    ]);
-
-    let res = resolve_and_validated(vec![dep("C")], &reg, None).unwrap();
-    assert_same(
-        &res,
-        &names(&[
-            ("root", "1.0.0"),
-            ("C", "1.0.0"),
-            ("B", "1.0.0"),
-            ("A", "0.1.0"),
-        ]),
-    );
-}
-
-#[test]
-fn public_dependency_filling_in() {
-    // The resolver has an optimization where if a candidate to resolve a dependency
-    // has already bean activated then we skip looking at the candidates dependencies.
-    // However, we have to be careful as the new path may make pub dependencies invalid.
-
-    // Triggering this case requires dependencies to be resolved in a specific order.
-    // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations:
-    // 1. `d`'s dep on `c` is resolved
-    // 2. `d`'s dep on `a` is resolved with `0.1.1`
-    // 3. `c`'s dep on `b` is resolved with `0.0.2`
-    // 4. `b`'s dep on `a` is resolved with `0.0.6` no pub dev conflict as `b` is private to `c`
-    // 5. `d`'s dep on `b` is resolved with `0.0.2` triggering the optimization.
-    // Do we notice that `d` has a pub dep conflict on `a`? Lets try it and see.
-    let reg = registry(vec![
-        pkg!(("a", "0.0.6")),
-        pkg!(("a", "0.1.1")),
-        pkg!(("b", "0.0.0") => [dep("bad")]),
-        pkg!(("b", "0.0.1") => [dep("bad")]),
-        pkg!(("b", "0.0.2") => [dep_req_kind("a", "=0.0.6", DepKind::Normal, true)]),
-        pkg!("c" => [dep_req("b", ">=0.0.1")]),
-        pkg!("d" => [dep("c"), dep("a"), dep("b")]),
-    ]);
-
-    let res = resolve_and_validated(vec![dep("d")], &reg, None).unwrap();
-    assert_same(
-        &res,
-        &names(&[
-            ("root", "1.0.0"),
-            ("d", "1.0.0"),
-            ("c", "1.0.0"),
-            ("b", "0.0.2"),
-            ("a", "0.0.6"),
-        ]),
-    );
-}
-
-#[test]
-fn public_dependency_filling_in_and_update() {
-    // The resolver has an optimization where if a candidate to resolve a dependency
-    // has already bean activated then we skip looking at the candidates dependencies.
-    // However, we have to be careful as the new path may make pub dependencies invalid.
-
-    // Triggering this case requires dependencies to be resolved in a specific order.
-    // Fuzzing found this unintuitive case, that triggers this unfortunate order of operations:
-    // 1. `D`'s dep on `B` is resolved
-    // 2. `D`'s dep on `C` is resolved
-    // 3. `B`'s dep on `A` is resolved with `0.0.0`
-    // 4. `C`'s dep on `B` triggering the optimization.
-    // So did we add `A 0.0.0` to the deps `C` can see?
-    // Or are we going to  resolve `C`'s dep on `A` with `0.0.2`?
-    // Lets try it and see.
-    let reg = registry(vec![
-        pkg!(("A", "0.0.0")),
-        pkg!(("A", "0.0.2")),
-        pkg!("B" => [dep_req_kind("A", "=0.0.0", DepKind::Normal, true),]),
-        pkg!("C" => [dep("A"),dep("B")]),
-        pkg!("D" => [dep("B"),dep("C")]),
-    ]);
-    let res = resolve_and_validated(vec![dep("D")], &reg, None).unwrap();
-    assert_same(
-        &res,
-        &names(&[
-            ("root", "1.0.0"),
-            ("D", "1.0.0"),
-            ("C", "1.0.0"),
-            ("B", "1.0.0"),
-            ("A", "0.0.0"),
-        ]),
-    );
-}
-
-#[test]
-fn public_dependency_skipping() {
-    // When backtracking due to a failed dependency, if Cargo is
-    // trying to be clever and skip irrelevant dependencies, care must
-    // the effects of pub dep must be accounted for.
-    let input = vec![
-        pkg!(("a", "0.2.0")),
-        pkg!(("a", "2.0.0")),
-        pkg!(("b", "0.0.0") => [dep("bad")]),
-        pkg!(("b", "0.2.1") => [dep_req_kind("a", "0.2.0", DepKind::Normal, true)]),
-        pkg!("c" => [dep("a"),dep("b")]),
-    ];
-    let reg = registry(input);
-
-    resolve_and_validated(vec![dep("c")], &reg, None).unwrap();
-}
-
-#[test]
-fn public_dependency_skipping_in_backtracking() {
-    // When backtracking due to a failed dependency, if Cargo is
-    // trying to be clever and skip irrelevant dependencies, care must
-    // the effects of pub dep must be accounted for.
-    let input = vec![
-        pkg!(("A", "0.0.0") => [dep("bad")]),
-        pkg!(("A", "0.0.1") => [dep("bad")]),
-        pkg!(("A", "0.0.2") => [dep("bad")]),
-        pkg!(("A", "0.0.3") => [dep("bad")]),
-        pkg!(("A", "0.0.4")),
-        pkg!(("A", "0.0.5")),
-        pkg!("B" => [dep_req_kind("A", ">= 0.0.3", DepKind::Normal, true)]),
-        pkg!("C" => [dep_req("A", "<= 0.0.4"), dep("B")]),
-    ];
-    let reg = registry(input);
-
-    resolve_and_validated(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", DepKind::Normal, true),]),
-        pkg!(("b", "0.0.0") => [dep("bad"),]),
-        pkg!("A" => [dep_req("a", "= 0.0.0"),dep_req_kind("b", "*", DepKind::Normal, true)]),
-    ];
-
-    let reg = registry(input);
-    assert!(resolve_and_validated(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", DepKind::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(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", DepKind::Normal, true),]),
-        pkg!("P" => [dep_req_kind("a-sys", "*", DepKind::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(vec![dep("A")], &reg, None).unwrap();
-}
-
 #[test]
 #[should_panic(expected = "assertion failed: !name.is_empty()")]
 fn test_dependency_with_empty_name() {
@@ -1115,41 +929,6 @@ fn resolving_with_constrained_sibling_backtrack_activation() {
     );
 }
 
-#[test]
-fn resolving_with_public_constrained_sibling() {
-    // 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", "<=60")]),
-        pkg!(("bar", "1.0.0") => [dep_req_kind("constrained", ">=60", DepKind::Normal, true)]),
-    ];
-    // 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!("{}.0.0", i);
-        reglist.push(pkg!(("constrained", vsn.clone())));
-    }
-    let reg = registry(reglist);
-
-    let _ = resolve_and_validated(vec![dep_req("foo", "1")], &reg, None);
-}
-
 #[test]
 fn resolving_with_constrained_sibling_transitive_dep_effects() {
     // When backtracking due to a failed dependency, if Cargo is