Public dependency refactor and re-allow backjumping

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")], &reg, None);
+}
+
 #[test]
 fn resolving_with_constrained_sibling_transitive_dep_effects() {
     // When backtracking due to a failed dependency, if Cargo is

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;

src/cargo/core/resolver/context.rs

@@ -23,15 +23,15 @@ 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<PackageId, FeaturesSet>,
     /// get the package that will be linking to a native library by its links attribute
     pub links: im_rc::HashMap<InternedString, PackageId>,
     /// 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<im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>>,
+    pub public_dependency: Option<PublicDependency>,
 
     /// a way to look up for a package in activations what packages required it
     /// and all of the exact deps that it fulfilled.
@@ -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<ActivationsKey, (Summary, ContextAge)>;
 
 /// 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,18 +75,19 @@ 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())
     }
 }
 
 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 {
-                Some(im_rc::HashMap::new())
+                Some(PublicDependency::new())
             } else {
                 None
             },
@@ -109,7 +110,7 @@ impl Context {
         parent: Option<(&Summary, &Dependency)>,
     ) -> ActivateResult<bool> {
         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!(
@@ -181,20 +182,49 @@ 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<ContextAge> {
         self.activations
             .get(&id.as_activations_key())
             .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<ContextAge> {
+        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.
@@ -204,12 +234,12 @@ impl Context {
         conflicting_activations: &ConflictMap,
     ) -> Option<usize> {
         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)
     }
@@ -240,3 +270,165 @@ impl Context {
         graph
     }
 }
+
+impl Graph<PackageId, Rc<Vec<Dependency>>> {
+    pub fn parents_of(&self, p: PackageId) -> impl Iterator<Item = (PackageId, bool)> + '_ {
+        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,
+    ///     the `ContextAge` when it was first visible,
+    ///     and the `ContextAge` when it was first exported.
+    inner: im_rc::HashMap<
+        PackageId,
+        im_rc::HashMap<InternedString, (PackageId, ContextAge, Option<ContextAge>)>,
+    >,
+}
+
+impl PublicDependency {
+    fn new() -> Self {
+        PublicDependency {
+            inner: im_rc::HashMap::new(),
+        }
+    }
+    fn publicly_exports(&self, candidate_pid: PackageId) -> Vec<PackageId> {
+        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.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()
+    }
+    fn publicly_exports_item(
+        &self,
+        candidate_pid: PackageId,
+        target: PackageId,
+    ) -> Option<ContextAge> {
+        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<ContextAge> {
+        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,
+        parent_pid: PackageId,
+        is_public: bool,
+        age: ContextAge,
+        parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
+    ) {
+        // 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
+        // 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, 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().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.
+                            continue;
+                        }
+                        if public {
+                            // Mark that `c` has now bean seen publicly
+                            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, 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`
+                if public {
+                    // if it was public, then we add all of `p`s parents to be checked
+                    stack.extend(parents.parents_of(p));
+                }
+            }
+        }
+    }
+    pub fn can_add_edge(
+        &self,
+        b_id: PackageId,
+        parent: PackageId,
+        is_public: bool,
+        parents: &Graph<PackageId, Rc<Vec<Dependency>>>,
+    ) -> 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
+        // 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, 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((
+                            (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.
+                        // 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
+                    stack.extend(parents.parents_of(p));
+                }
+            }
+        }
+        Ok(())
+    }
+}