From 8c7f6af4e8d2dec25474d0ac9c7916a4796faedf Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 14 Nov 2018 17:31:49 -0500 Subject: [PATCH 1/6] I think this shrinks better. --- tests/testsuite/support/resolver.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs index 758db0ec2f6..9dca8fcfd8e 100644 --- a/tests/testsuite/support/resolver.rs +++ b/tests/testsuite/support/resolver.rs @@ -355,8 +355,13 @@ pub fn registry_strategy( ) -> impl Strategy { let name = string_regex("[A-Za-z][A-Za-z0-9_-]*(-sys)?").unwrap(); - let raw_version = [..max_versions; 3]; - let version_from_raw = |v: &[usize; 3]| format!("{}.{}.{}", v[0], v[1], v[2]); + let raw_version = ..max_versions.pow(3); + let version_from_raw = move |r: usize| { + let major = ((r / max_versions) / max_versions) % max_versions; + let minor = (r / max_versions) % max_versions; + let patch = r % max_versions; + format!("{}.{}.{}", major, minor, patch) + }; // If this is false than the crate will depend on the nonexistent "bad" // instead of the complex set we generated for it. @@ -365,7 +370,7 @@ pub fn registry_strategy( let list_of_versions = btree_map(raw_version, allow_deps, 1..=max_versions).prop_map(move |ver| { ver.into_iter() - .map(|a| (version_from_raw(&a.0), a.1)) + .map(|a| (version_from_raw(a.0), a.1)) .collect::>() }); From 3bd1005c0da0bab2d0be3e43f5587b0a0123a87b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 1 Dec 2018 10:04:03 -0500 Subject: [PATCH 2/6] regain determinism --- src/cargo/core/resolver/conflict_cache.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 29aac8fd2ed..479c3029d93 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -11,7 +11,7 @@ enum ConflictStoreTrie { Leaf(BTreeMap), /// a Node is a map from an element to a subTrie where /// all the Sets in the subTrie contains that element. - Node(HashMap), + Node(BTreeMap), } impl ConflictStoreTrie { @@ -59,7 +59,7 @@ impl ConflictStoreTrie { if let Some(pid) = iter.next() { if let ConflictStoreTrie::Node(p) = self { p.entry(pid) - .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new())) + .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) .insert(iter, con); } // else, We already have a subset of this in the ConflictStore } else { @@ -159,7 +159,7 @@ impl ConflictCache { pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap) { self.con_from_dep .entry(dep.clone()) - .or_insert_with(|| ConflictStoreTrie::Node(HashMap::new())) + .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new())) .insert(con.keys().cloned(), con.clone()); trace!( From b5453f391235b889e853ed30eb427c804be1d592 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 1 Dec 2018 10:17:26 -0500 Subject: [PATCH 3/6] inline the filter --- src/cargo/core/resolver/conflict_cache.rs | 28 ++++++++++------------- src/cargo/core/resolver/mod.rs | 4 +--- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 479c3029d93..04424dfc420 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -17,17 +17,14 @@ enum ConflictStoreTrie { impl ConflictStoreTrie { /// Finds any known set of conflicts, if any, /// which are activated in `cx` and pass the `filter` specified? - fn find_conflicting( + fn find_conflicting( &self, cx: &Context, - filter: &F, - ) -> Option<&BTreeMap> - where - for<'r> F: Fn(&'r &BTreeMap) -> bool, - { + must_contain: Option, + ) -> Option<&BTreeMap> { match self { ConflictStoreTrie::Leaf(c) => { - if filter(&c) { + if must_contain.map(|f| c.contains_key(&f)).unwrap_or(true) { // is_conflicting checks that all the elements are active, // but we have checked each one by the recursion of this function. debug_assert!(cx.is_conflicting(None, c)); @@ -40,7 +37,7 @@ impl ConflictStoreTrie { for (&pid, store) in m { // if the key is active then we need to check all of the corresponding subTrie. if cx.is_active(pid) { - if let Some(o) = store.find_conflicting(cx, filter) { + if let Some(o) = store.find_conflicting(cx, must_contain) { return Some(o); } } // else, if it is not active then there is no way any of the corresponding @@ -134,23 +131,22 @@ impl ConflictCache { } /// Finds any known set of conflicts, if any, /// which are activated in `cx` and pass the `filter` specified? - pub fn find_conflicting( + pub fn find_conflicting( &self, cx: &Context, dep: &Dependency, - filter: F, - ) -> Option<&BTreeMap> - where - for<'r> F: Fn(&'r &BTreeMap) -> bool, - { - self.con_from_dep.get(dep)?.find_conflicting(cx, &filter) + must_contain: Option, + ) -> Option<&BTreeMap> { + self.con_from_dep + .get(dep)? + .find_conflicting(cx, must_contain) } pub fn conflicting( &self, cx: &Context, dep: &Dependency, ) -> Option<&BTreeMap> { - self.find_conflicting(cx, dep, |_| true) + self.find_conflicting(cx, dep, None) } /// Add to the cache a conflict of the form: diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index afe289f7d06..6df605cb1b7 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -442,9 +442,7 @@ fn activate_deps_loop( }) .filter_map(|(other_parent, other_dep)| { past_conflicting_activations - .find_conflicting(&cx, &other_dep, |con| { - con.contains_key(&pid) - }) + .find_conflicting(&cx, &other_dep, Some(pid)) .map(|con| (other_parent, con)) }) .next() From 50af11758740ac87e1eda0c1192369b5f978ef61 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 1 Dec 2018 10:42:56 -0500 Subject: [PATCH 4/6] we only need to search the part of the Trie that may contain the filler --- src/cargo/core/resolver/conflict_cache.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 04424dfc420..79173202ba0 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -24,20 +24,21 @@ impl ConflictStoreTrie { ) -> Option<&BTreeMap> { match self { ConflictStoreTrie::Leaf(c) => { - if must_contain.map(|f| c.contains_key(&f)).unwrap_or(true) { - // is_conflicting checks that all the elements are active, - // but we have checked each one by the recursion of this function. - debug_assert!(cx.is_conflicting(None, c)); - Some(c) - } else { - None - } + // is_conflicting checks that all the elements are active, + // but we have checked each one by the recursion of this function. + debug_assert!(cx.is_conflicting(None, c)); + Some(c) } ConflictStoreTrie::Node(m) => { - for (&pid, store) in m { + for (&pid, store) in must_contain + .map(|f| m.range(..=f)) + .unwrap_or_else(|| m.range(..)) + { // if the key is active then we need to check all of the corresponding subTrie. if cx.is_active(pid) { - if let Some(o) = store.find_conflicting(cx, must_contain) { + if let Some(o) = + store.find_conflicting(cx, must_contain.filter(|&f| f == pid)) + { return Some(o); } } // else, if it is not active then there is no way any of the corresponding From 2814ca2b191e7480435171503c6bd72e236f402a Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 1 Dec 2018 15:01:34 -0500 Subject: [PATCH 5/6] fuzzer found a bad case --- tests/testsuite/resolve.rs | 40 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index e9644e628d5..ce66ce06288 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -1201,3 +1201,43 @@ fn large_conflict_cache() { let reg = registry(input); let _ = resolve(&pkg_id("root"), root_deps, ®); } + +#[test] +fn conflict_store_bug() { + let input = vec![ + pkg!(("A", "0.0.3")), + pkg!(("A", "0.0.5")), + pkg!(("A", "0.0.9") => [dep("bad"),]), + pkg!(("A", "0.0.10") => [dep("bad"),]), + pkg!(("L-sys", "0.0.1") => [dep("bad"),]), + pkg!(("L-sys", "0.0.5")), + pkg!(("R", "0.0.4") => [ + dep_req("L-sys", "= 0.0.5"), + ]), + pkg!(("R", "0.0.6")), + pkg!(("a-sys", "0.0.5")), + pkg!(("a-sys", "0.0.11")), + pkg!(("c", "0.0.12") => [ + dep_req("R", ">= 0.0.3, <= 0.0.4"), + ]), + pkg!(("c", "0.0.13") => [ + dep_req("a-sys", ">= 0.0.8, <= 0.0.11"), + ]), + pkg!(("c0", "0.0.6") => [ + dep_req("L-sys", "<= 0.0.2"), + ]), + pkg!(("c0", "0.0.10") => [ + dep_req("A", ">= 0.0.9, <= 0.0.10"), + dep_req("a-sys", "= 0.0.5"), + ]), + pkg!("j" => [ + dep_req("A", ">= 0.0.3, <= 0.0.5"), + dep_req("R", ">=0.0.4, <= 0.0.6"), + dep_req("c", ">= 0.0.9"), + dep_req("c0", ">= 0.0.6"), + ]), + ]; + + let reg = registry(input.clone()); + let _ = resolve_and_validated(&pkg_id("root"), vec![dep("j")], ®); +} From eeaebfcdfce39d1701d27015cca8b717996d0047 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 1 Dec 2018 19:08:12 -0500 Subject: [PATCH 6/6] oops... --- src/cargo/core/resolver/conflict_cache.rs | 28 +++++++++++++++++------ 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 79173202ba0..aacab60be18 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -24,10 +24,15 @@ impl ConflictStoreTrie { ) -> Option<&BTreeMap> { match self { ConflictStoreTrie::Leaf(c) => { - // is_conflicting checks that all the elements are active, - // but we have checked each one by the recursion of this function. - debug_assert!(cx.is_conflicting(None, c)); - Some(c) + if must_contain.is_none() { + // is_conflicting checks that all the elements are active, + // but we have checked each one by the recursion of this function. + debug_assert!(cx.is_conflicting(None, c)); + Some(c) + } else { + // we did not find `must_contain` so we need to keep looking. + None + } } ConflictStoreTrie::Node(m) => { for (&pid, store) in must_contain @@ -37,7 +42,7 @@ impl ConflictStoreTrie { // if the key is active then we need to check all of the corresponding subTrie. if cx.is_active(pid) { if let Some(o) = - store.find_conflicting(cx, must_contain.filter(|&f| f == pid)) + store.find_conflicting(cx, must_contain.filter(|&f| f != pid)) { return Some(o); } @@ -138,9 +143,18 @@ impl ConflictCache { dep: &Dependency, must_contain: Option, ) -> Option<&BTreeMap> { - self.con_from_dep + let out = self + .con_from_dep .get(dep)? - .find_conflicting(cx, must_contain) + .find_conflicting(cx, must_contain); + if cfg!(debug_assertions) { + if let Some(f) = must_contain { + if let Some(c) = &out { + assert!(c.contains_key(&f)); + } + } + } + out } pub fn conflicting( &self,