From bf28b0f7ade99fb0d99a54078447e5e62c776e12 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 26 Apr 2019 15:10:29 -0400 Subject: [PATCH 1/3] add comments to `generalize_conflicting` and use more general logic --- src/cargo/core/resolver/conflict_cache.rs | 68 +++++++---------------- src/cargo/core/resolver/mod.rs | 58 +++++++++++++++---- 2 files changed, 68 insertions(+), 58 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index d652e5bbe3b..6b9d52b18d1 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -18,20 +18,17 @@ enum ConflictStoreTrie { impl ConflictStoreTrie { /// Finds any known set of conflicts, if any, - /// which are activated in `cx` and contain `PackageId` specified. + /// where all elements return some from `is_active` and contain `PackageId` specified. /// If more then one are activated, then it will return /// one that will allow for the most jump-back. - fn find_conflicting( + fn find( &self, - cx: &Context, + is_active: &impl Fn(PackageId) -> Option, must_contain: Option, ) -> Option<(&ConflictMap, usize)> { match self { ConflictStoreTrie::Leaf(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).is_some()); Some((c, 0)) } else { // We did not find `must_contain`, so we need to keep looking. @@ -45,9 +42,9 @@ impl ConflictStoreTrie { .unwrap_or_else(|| m.range(..)) { // If the key is active, then we need to check all of the corresponding subtrie. - if let Some(age_this) = cx.is_active(pid) { + if let Some(age_this) = is_active(pid) { if let Some((o, age_o)) = - store.find_conflicting(cx, must_contain.filter(|&f| f != pid)) + store.find(is_active, must_contain.filter(|&f| f != pid)) { let age = if must_contain == Some(pid) { // all the results will include `must_contain` @@ -102,28 +99,6 @@ impl ConflictStoreTrie { *self = ConflictStoreTrie::Leaf(con) } } - - fn contains(&self, mut iter: impl Iterator, con: &ConflictMap) -> bool { - match (self, iter.next()) { - (ConflictStoreTrie::Leaf(c), None) => { - if cfg!(debug_assertions) { - let a: Vec<_> = con.keys().collect(); - let b: Vec<_> = c.keys().collect(); - assert_eq!(a, b); - } - true - } - (ConflictStoreTrie::Leaf(_), Some(_)) => false, - (ConflictStoreTrie::Node(_), None) => false, - (ConflictStoreTrie::Node(m), Some(n)) => { - if let Some(next) = m.get(&n) { - next.contains(iter, con) - } else { - false - } - } - } - } } pub(super) struct ConflictCache { @@ -172,6 +147,17 @@ impl ConflictCache { dep_from_pid: HashMap::new(), } } + pub fn find( + &self, + dep: &Dependency, + is_active: &impl Fn(PackageId) -> Option, + must_contain: Option, + ) -> Option<&ConflictMap> { + self.con_from_dep + .get(dep)? + .find(is_active, must_contain) + .map(|(c, _)| c) + } /// Finds any known set of conflicts, if any, /// which are activated in `cx` and contain `PackageId` specified. /// If more then one are activated, then it will return @@ -182,14 +168,11 @@ impl ConflictCache { dep: &Dependency, must_contain: Option, ) -> Option<&ConflictMap> { - let out = self - .con_from_dep - .get(dep)? - .find_conflicting(cx, must_contain) - .map(|(c, _)| c); + let out = self.find(dep, &|id| cx.is_active(id), must_contain); if cfg!(debug_assertions) { - if let Some(f) = must_contain { - if let Some(c) = &out { + if let Some(c) = &out { + assert!(cx.is_conflicting(None, c).is_some()); + if let Some(f) = must_contain { assert!(c.contains_key(&f)); } } @@ -229,17 +212,6 @@ impl ConflictCache { } } - /// Check if a conflict was previously added of the form: - /// `dep` is known to be unresolvable if - /// all the `PackageId` entries are activated. - pub fn contains(&self, dep: &Dependency, con: &ConflictMap) -> bool { - if let Some(cst) = self.con_from_dep.get(dep) { - cst.contains(con.keys().cloned(), con) - } else { - false - } - } - pub fn dependencies_conflicting_with(&self, pid: PackageId) -> Option<&HashSet> { self.dep_from_pid.get(&pid) } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index a6a589da8be..d30561a7b08 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -895,24 +895,62 @@ fn generalize_conflicting( // A dep is equivalent to one of the things it can resolve to. // Thus, if all the things it can resolve to have already ben determined // to be conflicting, then we can just say that we conflict with the parent. - if registry + if let Some(others) = registry .query(critical_parents_dep) .expect("an already used dep now error!?") .iter() .rev() // the last one to be tried is the least likely to be in the cache, so start with that. - .all(|other| { - let mut con = conflicting_activations.clone(); - con.remove(&backtrack_critical_id); - con.insert( - other.summary.package_id(), - backtrack_critical_reason.clone(), - ); - past_conflicting_activations.contains(dep, &con) + .map(|other| { + past_conflicting_activations + .find( + dep, + &|id| { + if id == other.summary.package_id() { + // 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) + } + }, + Some(other.summary.package_id()), + ) + .map(|con| (other.summary.package_id(), con)) }) + .collect::>>() { let mut con = conflicting_activations.clone(); - con.remove(&backtrack_critical_id); + // It is always valid to combine previously inserted conflicts. + // A, B are both known bad states each that can never be activated. + // A + B is redundant but cant be activated, as if + // A + B is active then A is active and we know that is not ok. + for (_, other) in &others { + con.extend(other.iter().map(|(&id, re)| (id, re.clone()))); + } + // Now that we have this combined conflict, we can do a substitution: + // A dep is equivalent to one of the things it can resolve to. + // So we can remove all the things that it resolves to and replace with the parent. + for (other_id, _) in &others { + con.remove(other_id); + } con.insert(*critical_parent, backtrack_critical_reason); + + #[cfg(debug_assertions)] + { + // the entire point is to find an older conflict, so let's make sure we did + let new_age = con + .keys() + .map(|&c| cx.is_active(c).expect("not currently active!?")) + .max() + .unwrap(); + assert!( + new_age < backtrack_critical_age, + "new_age {} < backtrack_critical_age {}", + new_age, + backtrack_critical_age + ); + } past_conflicting_activations.insert(dep, &con); return Some(con); } From 45411e2d99b4da3024911804a02f4fabe3db9192 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 2 May 2019 11:13:25 -0400 Subject: [PATCH 2/3] if found a case that causes a panic and did not use all of the input. lets print the part of the input that was used. Helpful in minimization. --- tests/testsuite/support/resolver.rs | 35 +++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs index fd5dc22db02..7c8ce7f2fc1 100644 --- a/tests/testsuite/support/resolver.rs +++ b/tests/testsuite/support/resolver.rs @@ -73,7 +73,10 @@ pub fn resolve_with_config_raw( registry: &[Summary], config: Option<&Config>, ) -> CargoResult { - struct MyRegistry<'a>(&'a [Summary]); + struct MyRegistry<'a> { + list: &'a [Summary], + used: HashSet, + }; impl<'a> Registry for MyRegistry<'a> { fn query( &mut self, @@ -81,8 +84,9 @@ pub fn resolve_with_config_raw( f: &mut dyn FnMut(Summary), fuzzy: bool, ) -> CargoResult<()> { - for summary in self.0.iter() { + for summary in self.list.iter() { if fuzzy || dep.matches(summary) { + self.used.insert(summary.package_id()); f(summary.clone()); } } @@ -97,7 +101,28 @@ pub fn resolve_with_config_raw( false } } - let mut registry = MyRegistry(registry); + impl<'a> Drop for MyRegistry<'a> { + fn drop(&mut self) { + if std::thread::panicking() && self.list.len() != self.used.len() { + // we found a case that causes a panic and did not use all of the input. + // lets print the part of the input that was used for minimization. + println!( + "{:?}", + PrettyPrintRegistry( + self.list + .iter() + .filter(|s| { self.used.contains(&s.package_id()) }) + .cloned() + .collect() + ) + ); + } + } + } + let mut registry = MyRegistry { + list: registry, + used: HashSet::new(), + }; let summary = Summary::new( pkg, deps, @@ -348,8 +373,6 @@ fn meta_test_deep_pretty_print_registry() { pkg!(("baz", "1.0.1")), pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Build, false)]), pkg!(("cat", "1.0.3") => [dep_req_kind("other", "2", Kind::Development, false)]), - pkg!(("cat", "1.0.4") => [dep_req_kind("other", "2", Kind::Build, false)]), - pkg!(("cat", "1.0.5") => [dep_req_kind("other", "2", Kind::Development, false)]), pkg!(("dep_req", "1.0.0")), pkg!(("dep_req", "2.0.0")), ]) @@ -363,8 +386,6 @@ fn meta_test_deep_pretty_print_registry() { pkg!((\"baz\", \"1.0.1\")),\ pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Build, false),]),\ pkg!((\"cat\", \"1.0.3\") => [dep_req_kind(\"other\", \"^2\", Kind::Development, false),]),\ - pkg!((\"cat\", \"1.0.4\") => [dep_req_kind(\"other\", \"^2\", Kind::Build, false),]),\ - pkg!((\"cat\", \"1.0.5\") => [dep_req_kind(\"other\", \"^2\", Kind::Development, false),]),\ pkg!((\"dep_req\", \"1.0.0\")),\ pkg!((\"dep_req\", \"2.0.0\")),]" ) From d11e42cd8f8d804790cb48dd3bd4a24dc7a2c4cc Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 6 May 2019 16:12:36 -0400 Subject: [PATCH 3/3] nit --- src/cargo/core/resolver/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index d30561a7b08..f44cfb514cd 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -936,8 +936,7 @@ fn generalize_conflicting( } con.insert(*critical_parent, backtrack_critical_reason); - #[cfg(debug_assertions)] - { + if cfg!(debug_assertions) { // the entire point is to find an older conflict, so let's make sure we did let new_age = con .keys()