From 1d43c199688699e228f9b9f9c012a6d92f7bfe7c Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 24 May 2019 17:40:38 -0400 Subject: [PATCH 1/6] move `public_dependency`s code to methods for better organization Note: this commit does not change code, just moves it --- src/cargo/core/resolver/context.rs | 112 ++++++++++++++++++++++++++++- src/cargo/core/resolver/mod.rs | 82 ++------------------- 2 files changed, 115 insertions(+), 79 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 991b746e3c2..8c3cc1e54bf 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -30,8 +30,7 @@ pub struct Context { pub links: im_rc::HashMap, /// for each package the list of names it can see, /// then for each name the exact version that name represents and weather the name is public. - pub public_dependency: - Option>>, + pub public_dependency: Option, /// a way to look up for a package in activations what packages required it /// and all of the exact deps that it fulfilled. @@ -86,7 +85,7 @@ impl Context { resolve_features: im_rc::HashMap::new(), links: im_rc::HashMap::new(), public_dependency: if check_public_visible_dependencies { - Some(im_rc::HashMap::new()) + Some(PublicDependency::new()) } else { None }, @@ -240,3 +239,110 @@ impl Context { graph } } + +#[derive(Clone, Debug, Default)] +pub struct PublicDependency { + inner: im_rc::HashMap>, +} + +impl PublicDependency { + fn new() -> Self { + PublicDependency { + inner: im_rc::HashMap::new(), + } + } + pub fn add_edge( + &mut self, + candidate_pid: PackageId, + parent_pid: PackageId, + dep: &Dependency, + parents: &Graph>>, + ) { + // one tricky part is that `candidate_pid` may already be active and + // have public dependencies of its own. So we not only need to mark + // `candidate_pid` as visible to its parents but also all of its existing + // public dependencies. + let existing_public_deps: Vec = self + .inner + .get(&candidate_pid) + .iter() + .flat_map(|x| x.values()) + .filter_map(|x| if x.1 { Some(&x.0) } else { None }) + .chain(&Some(candidate_pid)) + .cloned() + .collect(); + for c in existing_public_deps { + // for each (transitive) parent that can newly see `t` + let mut stack = vec![(parent_pid, dep.is_public())]; + while let Some((p, public)) = stack.pop() { + match self.inner.entry(p).or_default().entry(c.name()) { + im_rc::hashmap::Entry::Occupied(mut o) => { + // the (transitive) parent can already see something by `c`s name, it had better be `c`. + assert_eq!(o.get().0, c); + if o.get().1 { + // The previous time the parent saw `c`, it was a public dependency. + // So all of its parents already know about `c` + // and we can save some time by stopping now. + continue; + } + if public { + // Mark that `c` has now bean seen publicly + o.insert((c, public)); + } + } + im_rc::hashmap::Entry::Vacant(v) => { + // The (transitive) parent does not have anything by `c`s name, + // so we add `c`. + v.insert((c, public)); + } + } + // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` + if public { + // if it was public, then we add all of `p`s parents to be checked + for &(grand, ref d) in parents.edges(&p) { + stack.push((grand, d.iter().any(|x| x.is_public()))); + } + } + } + } + } + pub fn can_add_edge( + &self, + b_id: PackageId, + parent: PackageId, + dep: &Dependency, + parents: &Graph>>, + ) -> Result<(), (PackageId, ConflictReason)> { + let existing_public_deps: Vec = self + .inner + .get(&b_id) + .iter() + .flat_map(|x| x.values()) + .filter_map(|x| if x.1 { Some(&x.0) } else { None }) + .chain(&Some(b_id)) + .cloned() + .collect(); + for t in existing_public_deps { + // for each (transitive) parent that can newly see `t` + let mut stack = vec![(parent, dep.is_public())]; + while let Some((p, public)) = stack.pop() { + // TODO: dont look at the same thing more then once + if let Some(o) = self.inner.get(&p).and_then(|x| x.get(&t.name())) { + if o.0 != t { + // the (transitive) parent can already see a different version by `t`s name. + // So, adding `b` will cause `p` to have a public dependency conflict on `t`. + return Err((p, ConflictReason::PublicDependency)); + } + } + // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` + if public { + // if it was public, then we add all of `p`s parents to be checked + for &(grand, ref d) in parents.edges(&p) { + stack.push((grand, d.iter().any(|x| x.is_public()))); + } + } + } + } + Ok(()) + } +} diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4aaa7eeafed..0d02515d1a1 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -603,52 +603,7 @@ fn activate( // and associate dep with that edge .push(dep.clone()); if let Some(public_dependency) = cx.public_dependency.as_mut() { - // one tricky part is that `candidate_pid` may already be active and - // have public dependencies of its own. So we not only need to mark - // `candidate_pid` as visible to its parents but also all of its existing - // public dependencies. - let existing_public_deps: Vec = public_dependency - .get(&candidate_pid) - .iter() - .flat_map(|x| x.values()) - .filter_map(|x| if x.1 { Some(&x.0) } else { None }) - .chain(&Some(candidate_pid)) - .cloned() - .collect(); - for c in existing_public_deps { - // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent_pid, dep.is_public())]; - while let Some((p, public)) = stack.pop() { - match public_dependency.entry(p).or_default().entry(c.name()) { - im_rc::hashmap::Entry::Occupied(mut o) => { - // the (transitive) parent can already see something by `c`s name, it had better be `c`. - assert_eq!(o.get().0, c); - if o.get().1 { - // The previous time the parent saw `c`, it was a public dependency. - // So all of its parents already know about `c` - // and we can save some time by stopping now. - continue; - } - if public { - // Mark that `c` has now bean seen publicly - o.insert((c, public)); - } - } - im_rc::hashmap::Entry::Vacant(v) => { - // The (transitive) parent does not have anything by `c`s name, - // so we add `c`. - v.insert((c, public)); - } - } - // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` - if public { - // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in cx.parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); - } - } - } - } + public_dependency.add_edge(candidate_pid, parent_pid, dep, &cx.parents); } } @@ -762,7 +717,7 @@ impl RemainingCandidates { dep: &Dependency, parent: PackageId, ) -> Option<(Summary, bool)> { - 'main: for b in self.remaining.by_ref() { + for b in self.remaining.by_ref() { let b_id = b.package_id(); // The `links` key in the manifest dictates that there's only one // package in a dependency graph, globally, with that particular @@ -801,35 +756,10 @@ impl RemainingCandidates { // activated and have public dependants of its own, // all of witch also need to be checked the same way. if let Some(public_dependency) = cx.public_dependency.as_ref() { - let existing_public_deps: Vec = public_dependency - .get(&b_id) - .iter() - .flat_map(|x| x.values()) - .filter_map(|x| if x.1 { Some(&x.0) } else { None }) - .chain(&Some(b_id)) - .cloned() - .collect(); - for t in existing_public_deps { - // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent, dep.is_public())]; - while let Some((p, public)) = stack.pop() { - // TODO: dont look at the same thing more then once - if let Some(o) = public_dependency.get(&p).and_then(|x| x.get(&t.name())) { - if o.0 != t { - // the (transitive) parent can already see a different version by `t`s name. - // So, adding `b` will cause `p` to have a public dependency conflict on `t`. - conflicting_prev_active.insert(p, ConflictReason::PublicDependency); - continue 'main; - } - } - // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` - if public { - // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in cx.parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); - } - } - } + if let Err((p, c)) = public_dependency.can_add_edge(b_id, parent, dep, &cx.parents) + { + conflicting_prev_active.insert(p, c); + continue; } } From 2d2973487b0aebd21d2ee8b54d6e977e010926b7 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 25 May 2019 20:02:29 -0400 Subject: [PATCH 2/6] Some cleanups --- src/cargo/core/resolver/context.rs | 69 ++++++++++++++++-------------- src/cargo/core/resolver/mod.rs | 5 ++- 2 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 8c3cc1e54bf..abe07f1822e 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -240,8 +240,17 @@ impl Context { } } +impl Graph>> { + pub fn parents_of(&self, p: PackageId) -> impl Iterator + '_ { + self.edges(&p) + .map(|(grand, d)| (*grand, d.iter().any(|x| x.is_public()))) + } +} + #[derive(Clone, Debug, Default)] pub struct PublicDependency { + /// For each active package the set of all the names it can see, + /// for each name the exact package that name resolves to and whether it exports that visibility. inner: im_rc::HashMap>, } @@ -251,29 +260,30 @@ impl PublicDependency { inner: im_rc::HashMap::new(), } } + fn publicly_exports(&self, candidate_pid: PackageId) -> Vec { + self.inner + .get(&candidate_pid) // if we have seen it before + .iter() + .flat_map(|x| x.values()) // all the things we have stored + .filter(|x| x.1) // as publicly exported + .map(|x| x.0) + .chain(Some(candidate_pid)) // but even if not we know that everything exports itself + .collect() + } pub fn add_edge( &mut self, candidate_pid: PackageId, parent_pid: PackageId, - dep: &Dependency, + is_public: bool, parents: &Graph>>, ) { // one tricky part is that `candidate_pid` may already be active and // have public dependencies of its own. So we not only need to mark // `candidate_pid` as visible to its parents but also all of its existing - // public dependencies. - let existing_public_deps: Vec = self - .inner - .get(&candidate_pid) - .iter() - .flat_map(|x| x.values()) - .filter_map(|x| if x.1 { Some(&x.0) } else { None }) - .chain(&Some(candidate_pid)) - .cloned() - .collect(); - for c in existing_public_deps { + // publicly exported dependencies. + for c in self.publicly_exports(candidate_pid) { // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent_pid, dep.is_public())]; + let mut stack = vec![(parent_pid, is_public)]; while let Some((p, public)) = stack.pop() { match self.inner.entry(p).or_default().entry(c.name()) { im_rc::hashmap::Entry::Occupied(mut o) => { @@ -299,9 +309,7 @@ impl PublicDependency { // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); - } + stack.extend(parents.parents_of(p)); } } } @@ -310,21 +318,16 @@ impl PublicDependency { &self, b_id: PackageId, parent: PackageId, - dep: &Dependency, + is_public: bool, parents: &Graph>>, ) -> Result<(), (PackageId, ConflictReason)> { - let existing_public_deps: Vec = self - .inner - .get(&b_id) - .iter() - .flat_map(|x| x.values()) - .filter_map(|x| if x.1 { Some(&x.0) } else { None }) - .chain(&Some(b_id)) - .cloned() - .collect(); - for t in existing_public_deps { + // one tricky part is that `candidate_pid` may already be active and + // have public dependencies of its own. So we not only need to check + // `b_id` as visible to its parents but also all of its existing + // publicly exported dependencies. + for t in self.publicly_exports(b_id) { // for each (transitive) parent that can newly see `t` - let mut stack = vec![(parent, dep.is_public())]; + let mut stack = vec![(parent, is_public)]; while let Some((p, public)) = stack.pop() { // TODO: dont look at the same thing more then once if let Some(o) = self.inner.get(&p).and_then(|x| x.get(&t.name())) { @@ -333,13 +336,17 @@ impl PublicDependency { // So, adding `b` will cause `p` to have a public dependency conflict on `t`. return Err((p, ConflictReason::PublicDependency)); } + if o.1 { + // The previous time the parent saw `t`, it was a public dependency. + // So all of its parents already know about `t` + // and we can save some time by stopping now. + continue; + } } // if `b` was a private dependency of `p` then `p` parents can't see `t` thru `p` if public { // if it was public, then we add all of `p`s parents to be checked - for &(grand, ref d) in parents.edges(&p) { - stack.push((grand, d.iter().any(|x| x.is_public()))); - } + stack.extend(parents.parents_of(p)); } } } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 0d02515d1a1..d39b3ef19dc 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -603,7 +603,7 @@ fn activate( // and associate dep with that edge .push(dep.clone()); if let Some(public_dependency) = cx.public_dependency.as_mut() { - public_dependency.add_edge(candidate_pid, parent_pid, dep, &cx.parents); + public_dependency.add_edge(candidate_pid, parent_pid, dep.is_public(), &cx.parents); } } @@ -756,7 +756,8 @@ impl RemainingCandidates { // activated and have public dependants of its own, // all of witch also need to be checked the same way. if let Some(public_dependency) = cx.public_dependency.as_ref() { - if let Err((p, c)) = public_dependency.can_add_edge(b_id, parent, dep, &cx.parents) + if let Err((p, c)) = + public_dependency.can_add_edge(b_id, parent, dep.is_public(), &cx.parents) { conflicting_prev_active.insert(p, c); continue; From a8a79b4246a88c1ad60f25fa59a9dc980f0126dc Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 26 May 2019 23:55:42 -0400 Subject: [PATCH 3/6] change `age` to count edges not just activations A pub dep conflict can be made by connecting two already activated pids --- src/cargo/core/resolver/context.rs | 11 +++-------- src/cargo/core/resolver/mod.rs | 11 ++++++----- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index abe07f1822e..925dfe50360 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -23,6 +23,7 @@ pub use super::resolve::Resolve; // possible. #[derive(Clone)] pub struct Context { + pub age: ContextAge, pub activations: Activations, /// list the features that are activated for each package pub resolve_features: im_rc::HashMap, @@ -82,6 +83,7 @@ impl PackageId { impl Context { pub fn new(check_public_visible_dependencies: bool) -> Context { Context { + age: 0, resolve_features: im_rc::HashMap::new(), links: im_rc::HashMap::new(), public_dependency: if check_public_visible_dependencies { @@ -108,7 +110,7 @@ impl Context { parent: Option<(&Summary, &Dependency)>, ) -> ActivateResult { let id = summary.package_id(); - let age: ContextAge = self.age(); + let age: ContextAge = self.age; match self.activations.entry(id.as_activations_key()) { im_rc::hashmap::Entry::Occupied(o) => { debug_assert_eq!( @@ -180,13 +182,6 @@ impl Context { }) } - /// Returns the `ContextAge` of this `Context`. - /// For now we use (len of activations) as the age. - /// See the `ContextAge` docs for more details. - pub fn age(&self) -> ContextAge { - self.activations.len() - } - /// If the package is active returns the `ContextAge` when it was added pub fn is_active(&self, id: PackageId) -> Option { self.activations diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index d39b3ef19dc..1592db131dc 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -218,7 +218,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} {} candidates", parent.name(), - cx.age(), + cx.age, dep.package_name(), candidates.len() ); @@ -264,7 +264,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} -- no candidates", parent.name(), - cx.age(), + cx.age, dep.package_name() ); @@ -375,7 +375,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} trying {}", parent.name(), - cx.age(), + cx.age, dep.package_name(), candidate.version() ); @@ -525,7 +525,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} skipping {} ", parent.name(), - cx.age(), + cx.age, dep.package_name(), pid.version() ); @@ -594,6 +594,7 @@ fn activate( opts: ResolveOpts, ) -> ActivateResult> { let candidate_pid = candidate.package_id(); + cx.age += 1; if let Some((parent, dep)) = parent { let parent_pid = parent.package_id(); Rc::make_mut( @@ -947,7 +948,7 @@ fn find_candidate( // make any progress. As a result if we hit this condition we can // completely skip this backtrack frame and move on to the next. if let Some(age) = age { - if frame.context.age() > age { + if frame.context.age >= age { trace!( "{} = \"{}\" skip as not solving {}: {:?}", frame.dep.package_name(), From 4a2dcf486e41bdb6cc61adbf5a583ca8a96bfced Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 28 May 2019 14:40:26 -0400 Subject: [PATCH 4/6] change `PublicDependency` to store the `age` for each edge --- src/cargo/core/resolver/context.rs | 21 ++++++++++++++------- src/cargo/core/resolver/mod.rs | 8 +++++++- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 925dfe50360..6802b3daf02 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -245,8 +245,13 @@ impl Graph>> { #[derive(Clone, Debug, Default)] pub struct PublicDependency { /// For each active package the set of all the names it can see, - /// for each name the exact package that name resolves to and whether it exports that visibility. - inner: im_rc::HashMap>, + /// for each name the exact package that name resolves to, + /// the `ContextAge` when it was first visible, + /// and the `ContextAge` when it was first exported. + inner: im_rc::HashMap< + PackageId, + im_rc::HashMap)>, + >, } impl PublicDependency { @@ -260,7 +265,7 @@ impl PublicDependency { .get(&candidate_pid) // if we have seen it before .iter() .flat_map(|x| x.values()) // all the things we have stored - .filter(|x| x.1) // as publicly exported + .filter(|x| x.2.is_some()) // as publicly exported .map(|x| x.0) .chain(Some(candidate_pid)) // but even if not we know that everything exports itself .collect() @@ -270,6 +275,7 @@ impl PublicDependency { candidate_pid: PackageId, parent_pid: PackageId, is_public: bool, + age: ContextAge, parents: &Graph>>, ) { // one tricky part is that `candidate_pid` may already be active and @@ -284,7 +290,7 @@ impl PublicDependency { im_rc::hashmap::Entry::Occupied(mut o) => { // the (transitive) parent can already see something by `c`s name, it had better be `c`. assert_eq!(o.get().0, c); - if o.get().1 { + if o.get().2.is_some() { // The previous time the parent saw `c`, it was a public dependency. // So all of its parents already know about `c` // and we can save some time by stopping now. @@ -292,13 +298,14 @@ impl PublicDependency { } if public { // Mark that `c` has now bean seen publicly - o.insert((c, public)); + let old_age = o.get().1; + o.insert((c, old_age, if public { Some(age) } else { None })); } } im_rc::hashmap::Entry::Vacant(v) => { // The (transitive) parent does not have anything by `c`s name, // so we add `c`. - v.insert((c, public)); + v.insert((c, age, if public { Some(age) } else { None })); } } // if `candidate_pid` was a private dependency of `p` then `p` parents can't see `c` thru `p` @@ -331,7 +338,7 @@ impl PublicDependency { // So, adding `b` will cause `p` to have a public dependency conflict on `t`. return Err((p, ConflictReason::PublicDependency)); } - if o.1 { + if o.2.is_some() { // The previous time the parent saw `t`, it was a public dependency. // So all of its parents already know about `t` // and we can save some time by stopping now. diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 1592db131dc..f92fbb0f81e 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -604,7 +604,13 @@ fn activate( // and associate dep with that edge .push(dep.clone()); if let Some(public_dependency) = cx.public_dependency.as_mut() { - public_dependency.add_edge(candidate_pid, parent_pid, dep.is_public(), &cx.parents); + public_dependency.add_edge( + candidate_pid, + parent_pid, + dep.is_public(), + cx.age, + &cx.parents, + ); } } From b48616fb83d0c5a17152dacdcc901b4ef1dd418b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 25 Jun 2019 17:28:06 -0400 Subject: [PATCH 5/6] allow `find_candidate` to backtrack from a `PublicDependency` --- src/cargo/core/resolver/conflict_cache.rs | 4 +- src/cargo/core/resolver/context.rs | 101 +++++++++++++++++++--- src/cargo/core/resolver/mod.rs | 20 +++-- src/cargo/core/resolver/types.rs | 13 ++- 4 files changed, 114 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index b88ad6825e7..8f629114455 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use log::trace; -use super::types::{ConflictMap, ConflictReason}; +use super::types::ConflictMap; use crate::core::resolver::Context; use crate::core::{Dependency, PackageId}; @@ -194,7 +194,7 @@ impl ConflictCache { /// `dep` is known to be unresolvable if /// all the `PackageId` entries are activated. pub fn insert(&mut self, dep: &Dependency, con: &ConflictMap) { - if con.values().any(|c| *c == ConflictReason::PublicDependency) { + if con.values().any(|c| c.is_public_dependency()) { // TODO: needs more info for back jumping // for now refuse to cache it. return; diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 6802b3daf02..535583797ae 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -49,13 +49,13 @@ pub type ContextAge = usize; /// By storing this in a hash map we ensure that there is only one /// semver compatible version of each crate. /// This all so stores the `ContextAge`. -pub type Activations = - im_rc::HashMap<(InternedString, SourceId, SemverCompatibility), (Summary, ContextAge)>; +pub type ActivationsKey = (InternedString, SourceId, SemverCompatibility); +pub type Activations = im_rc::HashMap; /// 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, Debug)] +#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug, PartialOrd, Ord)] pub enum SemverCompatibility { Major(NonZeroU64), Minor(NonZeroU64), @@ -75,7 +75,7 @@ impl From<&semver::Version> for SemverCompatibility { } impl PackageId { - pub fn as_activations_key(self) -> (InternedString, SourceId, SemverCompatibility) { + pub fn as_activations_key(self) -> ActivationsKey { (self.name(), self.source_id(), self.version().into()) } } @@ -189,6 +189,42 @@ impl Context { .and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None }) } + /// If the conflict reason on the package still applies returns the `ContextAge` when it was added + pub fn still_applies(&self, id: PackageId, reason: &ConflictReason) -> Option { + self.is_active(id).and_then(|mut max| { + match reason { + ConflictReason::PublicDependency(name) => { + if &id == name { + return Some(max); + } + max = std::cmp::max(max, self.is_active(*name)?); + max = std::cmp::max( + max, + self.public_dependency + .as_ref() + .unwrap() + .can_see_item(*name, id)?, + ); + } + ConflictReason::PubliclyExports(name) => { + if &id == name { + return Some(max); + } + max = std::cmp::max(max, self.is_active(*name)?); + max = std::cmp::max( + max, + self.public_dependency + .as_ref() + .unwrap() + .publicly_exports_item(*name, id)?, + ); + } + _ => {} + } + Some(max) + }) + } + /// Checks whether all of `parent` and the keys of `conflicting activations` /// are still active. /// If so returns the `ContextAge` when the newest one was added. @@ -198,12 +234,12 @@ impl Context { conflicting_activations: &ConflictMap, ) -> Option { let mut max = 0; - for &id in conflicting_activations.keys().chain(parent.as_ref()) { - if let Some(age) = self.is_active(id) { - max = std::cmp::max(max, age); - } else { - return None; - } + if let Some(parent) = parent { + max = std::cmp::max(max, self.is_active(parent)?); + } + + for (id, reason) in conflicting_activations.iter() { + max = std::cmp::max(max, self.still_applies(*id, reason)?); } Some(max) } @@ -270,6 +306,31 @@ impl PublicDependency { .chain(Some(candidate_pid)) // but even if not we know that everything exports itself .collect() } + fn publicly_exports_item( + &self, + candidate_pid: PackageId, + target: PackageId, + ) -> Option { + debug_assert_ne!(candidate_pid, target); + let out = self + .inner + .get(&candidate_pid) + .and_then(|names| names.get(&target.name())) + .filter(|(p, _, _)| *p == target) + .and_then(|(_, _, age)| *age); + debug_assert_eq!( + out.is_some(), + self.publicly_exports(candidate_pid).contains(&target) + ); + out + } + pub fn can_see_item(&self, candidate_pid: PackageId, target: PackageId) -> Option { + self.inner + .get(&candidate_pid) + .and_then(|names| names.get(&target.name())) + .filter(|(p, _, _)| *p == target) + .map(|(_, age, _)| *age) + } pub fn add_edge( &mut self, candidate_pid: PackageId, @@ -322,7 +383,13 @@ impl PublicDependency { parent: PackageId, is_public: bool, parents: &Graph>>, - ) -> Result<(), (PackageId, ConflictReason)> { + ) -> Result< + (), + ( + ((PackageId, ConflictReason), (PackageId, ConflictReason)), + Option<(PackageId, ConflictReason)>, + ), + > { // one tricky part is that `candidate_pid` may already be active and // have public dependencies of its own. So we not only need to check // `b_id` as visible to its parents but also all of its existing @@ -336,7 +403,17 @@ impl PublicDependency { if o.0 != t { // the (transitive) parent can already see a different version by `t`s name. // So, adding `b` will cause `p` to have a public dependency conflict on `t`. - return Err((p, ConflictReason::PublicDependency)); + return Err(( + (o.0, ConflictReason::PublicDependency(p)), // p can see the other version and + (parent, ConflictReason::PublicDependency(p)), // p can see us + )) + .map_err(|e| { + if t == b_id { + (e, None) + } else { + (e, Some((t, ConflictReason::PubliclyExports(b_id)))) + } + }); } if o.2.is_some() { // The previous time the parent saw `t`, it was a public dependency. diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index f92fbb0f81e..d5eb0ccf0de 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -763,10 +763,14 @@ impl RemainingCandidates { // activated and have public dependants of its own, // all of witch also need to be checked the same way. if let Some(public_dependency) = cx.public_dependency.as_ref() { - if let Err((p, c)) = + if let Err(((c1, c2), c3)) = public_dependency.can_add_edge(b_id, parent, dep.is_public(), &cx.parents) { - conflicting_prev_active.insert(p, c); + conflicting_prev_active.insert(c1.0, c1.1); + conflicting_prev_active.insert(c2.0, c2.1); + if let Some(c3) = c3 { + conflicting_prev_active.insert(c3.0, c3.1); + } continue; } } @@ -812,6 +816,10 @@ fn generalize_conflicting( let backtrack_critical_reason: ConflictReason = conflicting_activations[&backtrack_critical_id].clone(); + if backtrack_critical_reason.is_public_dependency() { + return None; + } + if cx .parents .is_path_from_to(&parent.package_id(), &backtrack_critical_id) @@ -919,13 +927,7 @@ fn find_candidate( // The abnormal situations are things that do not put all of the reasons in `conflicting_activations`: // If we backtracked we do not know how our `conflicting_activations` related to // the cause of that backtrack, so we do not update it. - // If we had a PublicDependency conflict, then we do not yet have a compact way to - // represent all the parts of the problem, so `conflicting_activations` is incomplete. - let age = if !backtracked - && !conflicting_activations - .values() - .any(|c| *c == ConflictReason::PublicDependency) - { + let age = if !backtracked { // we dont have abnormal situations. So we can ask `cx` for how far back we need to go. let a = cx.is_conflicting(Some(parent.package_id()), conflicting_activations); // If the `conflicting_activations` does not apply to `cx`, then something went very wrong diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index 881869ef16f..d18557a7a6d 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -286,7 +286,8 @@ pub enum ConflictReason { // TODO: needs more info for `activation_error` // TODO: needs more info for `find_candidate` /// pub dep error - PublicDependency, + PublicDependency(PackageId), + PubliclyExports(PackageId), } impl ConflictReason { @@ -310,6 +311,16 @@ impl ConflictReason { } false } + + pub fn is_public_dependency(&self) -> bool { + if let ConflictReason::PublicDependency(_) = *self { + return true; + } + if let ConflictReason::PubliclyExports(_) = *self { + return true; + } + false + } } /// A list of packages that have gotten in the way of resolving a dependency. From 0750caffe816a3b64d4ec34e638e9fa472200a6b Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 13 Sep 2019 15:22:43 -0400 Subject: [PATCH 6/6] add a test to show the new performance --- crates/resolver-tests/tests/resolve.rs | 35 ++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 362a6dc9b9b..07c7e3e39ea 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -1047,6 +1047,41 @@ 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", Kind::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")], ®, None); +} + #[test] fn resolving_with_constrained_sibling_transitive_dep_effects() { // When backtracking due to a failed dependency, if Cargo is