Skip to content

Commit

Permalink
fix(resolver): Remove unused public-deps error handling
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@epage
epage committed Nov 22, 2023
1 parent d5d9c35 commit 2f10699
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 511 deletions.
30 changes: 1 addition & 29 deletions crates/resolver-tests/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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.
Expand Down
225 changes: 2 additions & 223 deletions crates/resolver-tests/tests/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 2f10699

Please sign in to comment.