From c3e67c31bcb56655deac51925b0940c2fa12b918 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Tue, 9 Oct 2018 17:21:46 -0400
Subject: [PATCH 01/13] gen public deps

---
 src/cargo/core/dependency.rs        | 12 +++++
 tests/testsuite/support/resolver.rs | 80 +++++++++++++++++------------
 2 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs
index e2a27e0431e..b779d77c693 100644
--- a/src/cargo/core/dependency.rs
+++ b/src/cargo/core/dependency.rs
@@ -40,6 +40,7 @@ struct Inner {
     explicit_name_in_toml: Option<InternedString>,
 
     optional: bool,
+    public: bool,
     default_features: bool,
     features: Vec<InternedString>,
 
@@ -217,6 +218,7 @@ impl Dependency {
                 kind: Kind::Normal,
                 only_match_name: true,
                 optional: false,
+                public: false,
                 features: Vec::new(),
                 default_features: true,
                 specified_req: false,
@@ -293,6 +295,16 @@ impl Dependency {
         self.inner.kind
     }
 
+    pub fn is_public(&self) -> bool {
+        self.inner.public
+    }
+
+    /// Sets whether the dependency is public.
+    pub fn set_public(&mut self, public: bool) -> &mut Dependency {
+        Rc::make_mut(&mut self.inner).public = public;
+        self
+    }
+
     pub fn specified_req(&self) -> bool {
         self.inner.specified_req
     }
diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs
index 1db28a16953..99ebc3a9d59 100644
--- a/tests/testsuite/support/resolver.rs
+++ b/tests/testsuite/support/resolver.rs
@@ -250,9 +250,10 @@ pub fn dep(name: &str) -> Dependency {
 pub fn dep_req(name: &str, req: &str) -> Dependency {
     Dependency::parse_no_deprecated(name, Some(req), registry_loc()).unwrap()
 }
-pub fn dep_req_kind(name: &str, req: &str, kind: Kind) -> Dependency {
+pub fn dep_req_kind(name: &str, req: &str, kind: Kind, public: bool) -> Dependency {
     let mut dep = dep_req(name, req);
     dep.set_kind(kind);
+    dep.set_public(public);
     dep
 }
 
@@ -297,9 +298,12 @@ impl fmt::Debug for PrettyPrintRegistry {
             } else {
                 write!(f, "pkg!((\"{}\", \"{}\") => [", s.name(), s.version())?;
                 for d in s.dependencies() {
-                    if d.kind() == Kind::Normal && &d.version_req().to_string() == "*" {
+                    if d.kind() == Kind::Normal
+                        && &d.version_req().to_string() == "*"
+                        && !d.is_public()
+                    {
                         write!(f, "dep(\"{}\"),", d.name_in_toml())?;
-                    } else if d.kind() == Kind::Normal {
+                    } else if d.kind() == Kind::Normal && !d.is_public() {
                         write!(
                             f,
                             "dep_req(\"{}\", \"{}\"),",
@@ -309,14 +313,15 @@ impl fmt::Debug for PrettyPrintRegistry {
                     } else {
                         write!(
                             f,
-                            "dep_req_kind(\"{}\", \"{}\", {}),",
+                            "dep_req_kind(\"{}\", \"{}\", {}, {}),",
                             d.name_in_toml(),
                             d.version_req(),
                             match d.kind() {
                                 Kind::Development => "Kind::Development",
                                 Kind::Build => "Kind::Build",
                                 Kind::Normal => "Kind::Normal",
-                            }
+                            },
+                            d.is_public()
                         )?;
                     }
                 }
@@ -341,8 +346,10 @@ fn meta_test_deep_pretty_print_registry() {
                 pkg!(("bar", "2.0.0") => [dep_req("baz", "=1.0.1")]),
                 pkg!(("baz", "1.0.2") => [dep_req("other", "2")]),
                 pkg!(("baz", "1.0.1")),
-                pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Build)]),
-                pkg!(("cat", "1.0.2") => [dep_req_kind("other", "2", Kind::Development)]),
+                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, true)]),
+                pkg!(("cat", "1.0.5") => [dep_req_kind("other", "2", Kind::Development, true)]),
                 pkg!(("dep_req", "1.0.0")),
                 pkg!(("dep_req", "2.0.0")),
             ])
@@ -354,8 +361,10 @@ fn meta_test_deep_pretty_print_registry() {
          pkg!((\"bar\", \"2.0.0\") => [dep_req(\"baz\", \"= 1.0.1\"),]),\
          pkg!((\"baz\", \"1.0.2\") => [dep_req(\"other\", \"^2\"),]),\
          pkg!((\"baz\", \"1.0.1\")),\
-         pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Build),]),\
-         pkg!((\"cat\", \"1.0.2\") => [dep_req_kind(\"other\", \"^2\", Kind::Development),]),\
+         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, true),]),\
+         pkg!((\"cat\", \"1.0.5\") => [dep_req_kind(\"other\", \"^2\", Kind::Development, true),]),\
          pkg!((\"dep_req\", \"1.0.0\")),\
          pkg!((\"dep_req\", \"2.0.0\")),]"
     )
@@ -405,7 +414,13 @@ pub fn registry_strategy(
     let max_deps = max_versions * (max_crates * (max_crates - 1)) / shrinkage;
 
     let raw_version_range = (any::<Index>(), any::<Index>());
-    let raw_dependency = (any::<Index>(), any::<Index>(), raw_version_range, 0..=1);
+    let raw_dependency = (
+        any::<Index>(),
+        any::<Index>(),
+        raw_version_range,
+        0..=1,
+        any::<bool>(),
+    );
 
     fn order_index(a: Index, b: Index, size: usize) -> (usize, usize) {
         let (a, b) = (a.index(size), b.index(size));
@@ -432,7 +447,7 @@ pub fn registry_strategy(
                     .collect();
                 let len_all_pkgid = list_of_pkgid.len();
                 let mut dependency_by_pkgid = vec![vec![]; len_all_pkgid];
-                for (a, b, (c, d), k) in raw_dependencies {
+                for (a, b, (c, d), k, p) in raw_dependencies {
                     let (a, b) = order_index(a, b, len_all_pkgid);
                     let (a, b) = if reverse_alphabetical { (b, a) } else { (a, b) };
                     let ((dep_name, _), _) = list_of_pkgid[a];
@@ -443,27 +458,28 @@ pub fn registry_strategy(
                     let s_last_index = s.len() - 1;
                     let (c, d) = order_index(c, d, s.len());
 
-                    dependency_by_pkgid[b].push(dep_req_kind(
-                        &dep_name,
-                        &if c == 0 && d == s_last_index {
-                            "*".to_string()
-                        } else if c == 0 {
-                            format!("<={}", s[d].0)
-                        } else if d == s_last_index {
-                            format!(">={}", s[c].0)
-                        } else if c == d {
-                            format!("={}", s[c].0)
-                        } else {
-                            format!(">={}, <={}", s[c].0, s[d].0)
-                        },
-                        match k {
-                            0 => Kind::Normal,
-                            1 => Kind::Build,
-                            // => Kind::Development, // Development has not impact so don't gen
-                            _ => panic!("bad index for Kind"),
-                        },
-                    ))
-                }
+                dependency_by_pkgid[b].push(dep_req_kind(
+                    &dep_name,
+                    &if c == 0 && d == s_last_index {
+                        "*".to_string()
+                    } else if c == 0 {
+                        format!("<={}", s[d].0)
+                    } else if d == s_last_index {
+                        format!(">={}", s[c].0)
+                    } else if c == d {
+                        format!("={}", s[c].0)
+                    } else {
+                        format!(">={}, <={}", s[c].0, s[d].0)
+                    },
+                    match k {
+                        0 => Kind::Normal,
+                        1 => Kind::Build,
+                        // => Kind::Development, // Development has not impact so don't gen
+                        _ => panic!("bad index for Kind"),
+                    },
+                    p,
+                ))
+            }
 
                 PrettyPrintRegistry(
                     list_of_pkgid

From 363105f950e897f54c2f59604420819067dcab89 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Sat, 13 Oct 2018 17:34:13 -0400
Subject: [PATCH 02/13] minimized an example

---
 tests/testsuite/resolve.rs | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs
index 84aa81304a5..027e8aeb3bf 100644
--- a/tests/testsuite/resolve.rs
+++ b/tests/testsuite/resolve.rs
@@ -7,8 +7,8 @@ use cargo::util::Config;
 use crate::support::project;
 use crate::support::registry::Package;
 use crate::support::resolver::{
-    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, loc_names, names, pkg, pkg_dep,
-    pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated,
+    assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names,
+    pkg, pkg_dep, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated,
     resolve_with_config, PrettyPrintRegistry, ToDep, ToPkgId,
 };
 
@@ -235,6 +235,27 @@ proptest! {
     }
 }
 
+#[test]
+fn public_dependency() {
+    let reg = registry(vec![
+        pkg!(("A", "0.1.0")),
+        pkg!(("A", "0.2.0")),
+        pkg!("B" => [dep_req_kind("A", "0.1", Kind::Normal, true)]),
+        pkg!("C" => [dep_req("A", "*"), dep_req("B", "*")]),
+    ]);
+
+    let res = resolve_and_validated(&pkg_id("root"), vec![dep("C")], &reg).unwrap();
+    assert_same(
+        &res,
+        &names(&[
+            ("root", "1.0.0"),
+            ("C", "1.0.0"),
+            ("B", "1.0.0"),
+            ("A", "0.1.0"),
+        ]),
+    );
+}
+
 #[test]
 #[should_panic(expected = "assertion failed: !name.is_empty()")]
 fn test_dependency_with_empty_name() {

From 8c4c3804fe5b68cffb516c728908e5f86affcd12 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Mon, 8 Oct 2018 12:07:20 -0400
Subject: [PATCH 03/13] maintain a graph of parents so we can quickly walk to
 the root

---
 src/cargo/core/resolver/context.rs |  5 +++
 src/cargo/core/resolver/errors.rs  | 21 +++++++-----
 src/cargo/core/resolver/mod.rs     |  5 +++
 src/cargo/core/resolver/resolve.rs |  7 ++--
 src/cargo/util/graph.rs            | 54 ++++++++++++++++++++----------
 5 files changed, 62 insertions(+), 30 deletions(-)

diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index eb13dfa08e2..ef8b548a11d 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -28,6 +28,10 @@ pub struct Context {
     pub resolve_features: im_rc::HashMap<PackageId, Rc<HashSet<InternedString>>>,
     pub links: im_rc::HashMap<InternedString, PackageId>,
 
+    // This is somewhat redundant with the `resolve_graph` that stores the same data,
+    //   but for querying in the opposite order.
+    pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
+
     // These are two cheaply-cloneable lists (O(1) clone) which are effectively
     // hash maps but are built up as "construction lists". We'll iterate these
     // at the very end and actually construct the map that we're making.
@@ -46,6 +50,7 @@ impl Context {
             resolve_graph: RcList::new(),
             resolve_features: im_rc::HashMap::new(),
             links: im_rc::HashMap::new(),
+            parents: Graph::new(),
             resolve_replacements: RcList::new(),
             activations: im_rc::HashMap::new(),
             warnings: RcList::new(),
diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs
index 20c8d888776..976144b44d6 100644
--- a/src/cargo/core/resolver/errors.rs
+++ b/src/cargo/core/resolver/errors.rs
@@ -77,12 +77,11 @@ pub(super) fn activation_error(
     candidates: &[Candidate],
     config: Option<&Config>,
 ) -> ResolveError {
-    let graph = cx.graph();
     let to_resolve_err = |err| {
         ResolveError::new(
             err,
-            graph
-                .path_to_top(&parent.package_id())
+            cx.parents
+                .path_to_bottom(&parent.package_id())
                 .into_iter()
                 .cloned()
                 .collect(),
@@ -92,7 +91,9 @@ pub(super) fn activation_error(
     if !candidates.is_empty() {
         let mut msg = format!("failed to select a version for `{}`.", dep.package_name());
         msg.push_str("\n    ... required by ");
-        msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
+        msg.push_str(&describe_path(
+            &cx.parents.path_to_bottom(&parent.package_id()),
+        ));
 
         msg.push_str("\nversions that meet the requirements `");
         msg.push_str(&dep.version_req().to_string());
@@ -123,7 +124,7 @@ pub(super) fn activation_error(
                 msg.push_str(link);
                 msg.push_str("` as well:\n");
             }
-            msg.push_str(&describe_path(&graph.path_to_top(p)));
+            msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
         }
 
         let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
@@ -154,7 +155,7 @@ pub(super) fn activation_error(
 
         for &(p, _) in other_errors.iter() {
             msg.push_str("\n\n  previously selected ");
-            msg.push_str(&describe_path(&graph.path_to_top(p)));
+            msg.push_str(&describe_path(&cx.parents.path_to_bottom(p)));
         }
 
         msg.push_str("\n\nfailed to select a version for `");
@@ -204,7 +205,9 @@ pub(super) fn activation_error(
             registry.describe_source(dep.source_id()),
         );
         msg.push_str("required by ");
-        msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
+        msg.push_str(&describe_path(
+            &cx.parents.path_to_bottom(&parent.package_id()),
+        ));
 
         // If we have a path dependency with a locked version, then this may
         // indicate that we updated a sub-package and forgot to run `cargo
@@ -265,7 +268,9 @@ pub(super) fn activation_error(
             msg.push_str("\n");
         }
         msg.push_str("required by ");
-        msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
+        msg.push_str(&describe_path(
+            &cx.parents.path_to_bottom(&parent.package_id()),
+        ));
 
         msg
     };
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index ba1ef7d0744..e15d8204bb6 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -591,6 +591,11 @@ fn activate(
             candidate.summary.package_id(),
             dep.clone(),
         ));
+        Rc::make_mut(
+            cx.parents
+                .link(candidate.summary.package_id(), parent.package_id()),
+        )
+        .push(dep.clone());
     }
 
     let activated = cx.flag_activated(&candidate.summary, method)?;
diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs
index 87117af254a..04261d1f398 100644
--- a/src/cargo/core/resolver/resolve.rs
+++ b/src/cargo/core/resolver/resolve.rs
@@ -1,7 +1,6 @@
 use std::borrow::Borrow;
 use std::collections::{HashMap, HashSet};
 use std::fmt;
-use std::hash::Hash;
 use std::iter::FromIterator;
 
 use url::Url;
@@ -163,7 +162,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated
     pub fn contains<Q: ?Sized>(&self, k: &Q) -> bool
     where
         PackageId: Borrow<Q>,
-        Q: Hash + Eq,
+        Q: Ord + Eq,
     {
         self.graph.contains(k)
     }
@@ -179,11 +178,11 @@ unable to verify that `{0}` is the same as when the lockfile was generated
     pub fn deps(&self, pkg: PackageId) -> impl Iterator<Item = (PackageId, &[Dependency])> {
         self.graph
             .edges(&pkg)
-            .map(move |(&id, deps)| (self.replacement(id).unwrap_or(id), deps.as_slice()))
+            .map(move |&(id, ref deps)| (self.replacement(id).unwrap_or(id), deps.as_slice()))
     }
 
     pub fn deps_not_replaced<'a>(&'a self, pkg: PackageId) -> impl Iterator<Item = PackageId> + 'a {
-        self.graph.edges(&pkg).map(|(&id, _)| id)
+        self.graph.edges(&pkg).map(|&(id, _)| id)
     }
 
     pub fn replacement(&self, pkg: PackageId) -> Option<PackageId> {
diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs
index 48fc0a227f0..603edf14f9c 100644
--- a/src/cargo/util/graph.rs
+++ b/src/cargo/util/graph.rs
@@ -1,27 +1,28 @@
 use std::borrow::Borrow;
-use std::collections::{HashMap, HashSet};
+use std::collections::BTreeSet;
 use std::fmt;
-use std::hash::Hash;
 
-pub struct Graph<N, E> {
-    nodes: HashMap<N, HashMap<N, E>>,
+use im_rc;
+
+pub struct Graph<N: Clone, E: Clone> {
+    nodes: im_rc::OrdMap<N, im_rc::OrdMap<N, E>>,
 }
 
-impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
+impl<N: Eq + Ord + Clone, E: Default + Clone> Graph<N, E> {
     pub fn new() -> Graph<N, E> {
         Graph {
-            nodes: HashMap::new(),
+            nodes: im_rc::OrdMap::new(),
         }
     }
 
     pub fn add(&mut self, node: N) {
-        self.nodes.entry(node).or_insert_with(HashMap::new);
+        self.nodes.entry(node).or_insert_with(im_rc::OrdMap::new);
     }
 
     pub fn link(&mut self, node: N, child: N) -> &mut E {
         self.nodes
             .entry(node)
-            .or_insert_with(HashMap::new)
+            .or_insert_with(im_rc::OrdMap::new)
             .entry(child)
             .or_insert_with(Default::default)
     }
@@ -29,7 +30,7 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
     pub fn contains<Q: ?Sized>(&self, k: &Q) -> bool
     where
         N: Borrow<Q>,
-        Q: Hash + Eq,
+        Q: Ord + Eq,
     {
         self.nodes.contains_key(k)
     }
@@ -38,14 +39,14 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
         self.nodes.get(from)?.get(to)
     }
 
-    pub fn edges(&self, from: &N) -> impl Iterator<Item = (&N, &E)> {
+    pub fn edges(&self, from: &N) -> impl Iterator<Item = &(N, E)> {
         self.nodes.get(from).into_iter().flat_map(|x| x.iter())
     }
 
     /// A topological sort of the `Graph`
     pub fn sort(&self) -> Vec<N> {
         let mut ret = Vec::new();
-        let mut marks = HashSet::new();
+        let mut marks = BTreeSet::new();
 
         for node in self.nodes.keys() {
             self.sort_inner_visit(node, &mut ret, &mut marks);
@@ -54,7 +55,7 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
         ret
     }
 
-    fn sort_inner_visit(&self, node: &N, dst: &mut Vec<N>, marks: &mut HashSet<N>) {
+    fn sort_inner_visit(&self, node: &N, dst: &mut Vec<N>, marks: &mut BTreeSet<N>) {
         if !marks.insert(node.clone()) {
             return;
         }
@@ -70,6 +71,23 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
         self.nodes.keys()
     }
 
+    /// Resolves one of the paths from the given dependent package down to
+    /// a leaf.
+    pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> {
+        let mut result = vec![pkg];
+        while let Some(p) = self.nodes.get(pkg).and_then(|p| {
+            p.iter()
+                // Note that we can have "cycles" introduced through dev-dependency
+                // edges, so make sure we don't loop infinitely.
+                .find(|&(node, _)| !result.contains(&node))
+                .map(|(ref p, _)| p)
+        }) {
+            result.push(p);
+            pkg = p;
+        }
+        result
+    }
+
     /// Resolves one of the paths from the given dependent package up to
     /// the root.
     pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> {
@@ -84,7 +102,7 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
                 // Note that we can have "cycles" introduced through dev-dependency
                 // edges, so make sure we don't loop infinitely.
                 .find(|&(node, _)| !res.contains(&node))
-                .map(|p| p.0)
+                .map(|(ref p, _)| p)
         };
         while let Some(p) = first_pkg_depending_on(pkg, &result) {
             result.push(p);
@@ -94,13 +112,13 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {
     }
 }
 
-impl<N: Eq + Hash + Clone, E: Default> Default for Graph<N, E> {
+impl<N: Eq + Ord + Clone, E: Default + Clone> Default for Graph<N, E> {
     fn default() -> Graph<N, E> {
         Graph::new()
     }
 }
 
-impl<N: fmt::Display + Eq + Hash, E> fmt::Debug for Graph<N, E> {
+impl<N: fmt::Display + Eq + Ord + Clone, E: Clone> fmt::Debug for Graph<N, E> {
     fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
         writeln!(fmt, "Graph {{")?;
 
@@ -118,14 +136,14 @@ impl<N: fmt::Display + Eq + Hash, E> fmt::Debug for Graph<N, E> {
     }
 }
 
-impl<N: Eq + Hash, E: Eq> PartialEq for Graph<N, E> {
+impl<N: Eq + Ord + Clone, E: Eq + Clone> PartialEq for Graph<N, E> {
     fn eq(&self, other: &Graph<N, E>) -> bool {
         self.nodes.eq(&other.nodes)
     }
 }
-impl<N: Eq + Hash, E: Eq> Eq for Graph<N, E> {}
+impl<N: Eq + Ord + Clone, E: Eq + Clone> Eq for Graph<N, E> {}
 
-impl<N: Eq + Hash + Clone, E: Clone> Clone for Graph<N, E> {
+impl<N: Eq + Ord + Clone, E: Clone> Clone for Graph<N, E> {
     fn clone(&self) -> Graph<N, E> {
         Graph {
             nodes: self.nodes.clone(),

From 3e0a07f872ccf12f1066374a60968dc4b12b1517 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Mon, 15 Oct 2018 13:30:51 -0400
Subject: [PATCH 04/13] get the one test to pass

---
 src/cargo/core/resolver/context.rs |  3 ++
 src/cargo/core/resolver/mod.rs     | 58 ++++++++++++++++++++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index ef8b548a11d..bc1ae6f6776 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -27,6 +27,8 @@ pub struct Context {
     pub activations: Activations,
     pub resolve_features: im_rc::HashMap<PackageId, Rc<HashSet<InternedString>>>,
     pub links: im_rc::HashMap<InternedString, PackageId>,
+    pub public_dependency:
+        im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>,
 
     // This is somewhat redundant with the `resolve_graph` that stores the same data,
     //   but for querying in the opposite order.
@@ -50,6 +52,7 @@ impl Context {
             resolve_graph: RcList::new(),
             resolve_features: im_rc::HashMap::new(),
             links: im_rc::HashMap::new(),
+            public_dependency: im_rc::HashMap::new(),
             parents: Graph::new(),
             resolve_replacements: RcList::new(),
             activations: im_rc::HashMap::new(),
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index e15d8204bb6..fbfb13e823b 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -257,7 +257,12 @@ fn activate_deps_loop(
         let mut backtracked = false;
 
         loop {
-            let next = remaining_candidates.next(&mut conflicting_activations, &cx, &dep);
+            let next = remaining_candidates.next(
+                &mut conflicting_activations,
+                &cx,
+                &dep,
+                parent.package_id(),
+            );
 
             let (candidate, has_another) = next.ok_or(()).or_else(|_| {
                 // If we get here then our `remaining_candidates` was just
@@ -596,6 +601,33 @@ fn activate(
                 .link(candidate.summary.package_id(), parent.package_id()),
         )
         .push(dep.clone());
+        let mut stack = vec![(parent.package_id(), dep.is_public())];
+        while let Some((p, public)) = stack.pop() {
+            match cx
+                .public_dependency
+                .entry(p)
+                .or_default()
+                .entry(candidate.summary.name())
+            {
+                im_rc::hashmap::Entry::Occupied(mut o) => {
+                    assert_eq!(o.get().0, candidate.summary.package_id());
+                    if o.get().1 {
+                        continue;
+                    }
+                    if public {
+                        o.insert((candidate.summary.package_id(), public));
+                    }
+                }
+                im_rc::hashmap::Entry::Vacant(v) => {
+                    v.insert((candidate.summary.package_id(), public));
+                }
+            }
+            if public {
+                for &(grand, ref d) in cx.parents.edges(&p) {
+                    stack.push((grand, d.iter().any(|x| x.is_public())));
+                }
+            }
+        }
     }
 
     let activated = cx.flag_activated(&candidate.summary, method)?;
@@ -692,10 +724,11 @@ impl RemainingCandidates {
         conflicting_prev_active: &mut BTreeMap<PackageId, ConflictReason>,
         cx: &Context,
         dep: &Dependency,
+        parent: PackageId,
     ) -> Option<(Candidate, bool)> {
         let prev_active = cx.prev_active(dep);
 
-        for (_, b) in self.remaining.by_ref() {
+        'main: for (_, b) in self.remaining.by_ref() {
             // The `links` key in the manifest dictates that there's only one
             // package in a dependency graph, globally, with that particular
             // `links` key. If this candidate links to something that's already
@@ -731,6 +764,26 @@ impl RemainingCandidates {
                 }
             }
 
+            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) = cx
+                    .public_dependency
+                    .get(&p)
+                    .and_then(|x| x.get(&b.summary.name()))
+                {
+                    if o.0 != b.summary.package_id() {
+                        // TODO: conflicting_prev_active
+                        continue 'main;
+                    }
+                }
+                if public {
+                    for &(grand, ref d) in cx.parents.edges(&p) {
+                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    }
+                }
+            }
+
             // Well if we made it this far then we've got a valid dependency. We
             // want this iterator to be inherently "peekable" so we don't
             // necessarily return the item just yet. Instead we stash it away to
@@ -789,6 +842,7 @@ fn find_candidate(
             &mut frame.conflicting_activations,
             &frame.context,
             &frame.dep,
+            frame.parent.package_id(),
         );
         let (candidate, has_another) = match next {
             Some(pair) => pair,

From 38c5819ab041f0fb06fe68d74e1255a1a06db018 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Tue, 16 Oct 2018 13:52:25 -0400
Subject: [PATCH 05/13] add a harder test.

---
 src/cargo/core/resolver/mod.rs | 84 +++++++++++++++++++---------------
 tests/testsuite/resolve.rs     | 76 +++++++++++++++++++++++++++++-
 2 files changed, 121 insertions(+), 39 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index fbfb13e823b..6d3cc857065 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -601,30 +601,36 @@ fn activate(
                 .link(candidate.summary.package_id(), parent.package_id()),
         )
         .push(dep.clone());
-        let mut stack = vec![(parent.package_id(), dep.is_public())];
-        while let Some((p, public)) = stack.pop() {
-            match cx
-                .public_dependency
-                .entry(p)
-                .or_default()
-                .entry(candidate.summary.name())
-            {
-                im_rc::hashmap::Entry::Occupied(mut o) => {
-                    assert_eq!(o.get().0, candidate.summary.package_id());
-                    if o.get().1 {
-                        continue;
+        let cs: Vec<PackageId> = cx
+            .public_dependency
+            .get(&candidate.summary.package_id())
+            .iter()
+            .flat_map(|x| x.values())
+            .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+            .chain(Some(candidate.summary.package_id()).iter())
+            .cloned()
+            .collect();
+        for c in cs {
+            let mut stack = vec![(parent.package_id(), dep.is_public())];
+            while let Some((p, public)) = stack.pop() {
+                match cx.public_dependency.entry(p).or_default().entry(c.name()) {
+                    im_rc::hashmap::Entry::Occupied(mut o) => {
+                        assert_eq!(o.get().0, c);
+                        if o.get().1 {
+                            continue;
+                        }
+                        if public {
+                            o.insert((c, public));
+                        }
                     }
-                    if public {
-                        o.insert((candidate.summary.package_id(), public));
+                    im_rc::hashmap::Entry::Vacant(v) => {
+                        v.insert((c, public));
                     }
                 }
-                im_rc::hashmap::Entry::Vacant(v) => {
-                    v.insert((candidate.summary.package_id(), public));
-                }
-            }
-            if public {
-                for &(grand, ref d) in cx.parents.edges(&p) {
-                    stack.push((grand, d.iter().any(|x| x.is_public())));
+                if public {
+                    for &(grand, ref d) in cx.parents.edges(&p) {
+                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    }
                 }
             }
         }
@@ -763,23 +769,27 @@ impl RemainingCandidates {
                     continue;
                 }
             }
-
-            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) = cx
-                    .public_dependency
-                    .get(&p)
-                    .and_then(|x| x.get(&b.summary.name()))
-                {
-                    if o.0 != b.summary.package_id() {
-                        // TODO: conflicting_prev_active
-                        continue 'main;
+            for &t in cx
+                .public_dependency
+                .get(&b.summary.package_id())
+                .iter()
+                .flat_map(|x| x.values())
+                .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+                .chain(Some(b.summary.package_id()).iter())
+            {
+                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) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name())) {
+                        if o.0 != t {
+                            // TODO: conflicting_prev_active
+                            continue 'main;
+                        }
                     }
-                }
-                if public {
-                    for &(grand, ref d) in cx.parents.edges(&p) {
-                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    if public {
+                        for &(grand, ref d) in cx.parents.edges(&p) {
+                            stack.push((grand, d.iter().any(|x| x.is_public())));
+                        }
                     }
                 }
             }
diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs
index 027e8aeb3bf..6b39754d808 100644
--- a/tests/testsuite/resolve.rs
+++ b/tests/testsuite/resolve.rs
@@ -236,12 +236,12 @@ proptest! {
 }
 
 #[test]
-fn public_dependency() {
+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", Kind::Normal, true)]),
-        pkg!("C" => [dep_req("A", "*"), dep_req("B", "*")]),
+        pkg!("C" => [dep("A"), dep("B")]),
     ]);
 
     let res = resolve_and_validated(&pkg_id("root"), vec![dep("C")], &reg).unwrap();
@@ -256,6 +256,78 @@ fn public_dependency() {
     );
 }
 
+#[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", Kind::Normal, true)]),
+        pkg!("c" => [dep_req("b", ">=0.0.1")]),
+        pkg!("d" => [dep("c"), dep("a"), dep("b")]),
+    ]);
+
+    let res = resolve_and_validated(&pkg_id("root"), vec![dep("d")], &reg).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", Kind::Normal, true),]),
+        pkg!("C" => [dep("A"),dep("B")]),
+        pkg!("D" => [dep("B"),dep("C")]),
+    ]);
+    let res = resolve_and_validated(&pkg_id("root"), vec![dep("D")], &reg).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]
 #[should_panic(expected = "assertion failed: !name.is_empty()")]
 fn test_dependency_with_empty_name() {

From ed09ea2d68111a4f01601feb1c7700556dec1d11 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Wed, 17 Oct 2018 11:08:59 -0400
Subject: [PATCH 06/13] disable back jumping on pub dep error

---
 src/cargo/core/resolver/conflict_cache.rs |  5 +++
 src/cargo/core/resolver/mod.rs            |  5 ++-
 src/cargo/core/resolver/types.rs          |  5 +++
 tests/testsuite/resolve.rs                | 37 +++++++++++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs
index 6e14dade150..9bd71e9c0f8 100644
--- a/src/cargo/core/resolver/conflict_cache.rs
+++ b/src/cargo/core/resolver/conflict_cache.rs
@@ -173,6 +173,11 @@ impl ConflictCache {
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated.
     pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
+        if con.values().any(|c| *c == ConflictReason::PublicDependency) {
+            // TODO: needs more info for back jumping
+            // for now refuse to cache it.
+            return;
+        }
         self.con_from_dep
             .entry(dep.clone())
             .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 6d3cc857065..87e08596e61 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -782,7 +782,7 @@ impl RemainingCandidates {
                     // TODO: dont look at the same thing more then once
                     if let Some(o) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name())) {
                         if o.0 != t {
-                            // TODO: conflicting_prev_active
+                            conflicting_prev_active.insert(p, ConflictReason::PublicDependency);
                             continue 'main;
                         }
                     }
@@ -868,6 +868,9 @@ 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 !backtracked
+            && !conflicting_activations
+                .values()
+                .any(|c| *c == ConflictReason::PublicDependency)
             && frame
                 .context
                 .is_conflicting(Some(parent.package_id()), conflicting_activations)
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
index cdef344805b..84addf8bd31 100644
--- a/src/cargo/core/resolver/types.rs
+++ b/src/cargo/core/resolver/types.rs
@@ -402,6 +402,11 @@ pub enum ConflictReason {
     /// candidate. For example we tried to activate feature `foo` but the
     /// candidate we're activating didn't actually have the feature `foo`.
     MissingFeatures(String),
+
+    // TODO: needs more info for errors maneges
+    // TODO: needs more info for back jumping
+    /// pub dep errore
+    PublicDependency,
 }
 
 impl ConflictReason {
diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs
index 6b39754d808..33c25e3fe54 100644
--- a/tests/testsuite/resolve.rs
+++ b/tests/testsuite/resolve.rs
@@ -328,6 +328,43 @@ fn public_dependency_filling_in_and_update() {
     );
 }
 
+#[test]
+fn public_dependency_skiping() {
+    // 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", Kind::Normal, true)]),
+        pkg!("c" => [dep("a"),dep("b")]),
+    ];
+    let reg = registry(input.clone());
+
+    resolve(&pkg_id("root"), vec![dep("c")], &reg).unwrap();
+}
+
+#[test]
+fn public_dependency_skiping_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", Kind::Normal, true)]),
+        pkg!("C" => [dep_req("A", "<= 0.0.4"), dep("B")]),
+    ];
+    let reg = registry(input.clone());
+
+    resolve(&pkg_id("root"), vec![dep("C")], &reg).unwrap();
+}
+
 #[test]
 #[should_panic(expected = "assertion failed: !name.is_empty()")]
 fn test_dependency_with_empty_name() {

From ac9ba10f1c97ec7faf815826795873637684786d Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Wed, 14 Nov 2018 12:32:10 -0500
Subject: [PATCH 07/13] hide the details of the type for Conflict

---
 src/cargo/core/resolver/conflict_cache.rs | 26 ++++++-----------------
 src/cargo/core/resolver/context.rs        |  6 +++---
 src/cargo/core/resolver/errors.rs         |  5 ++---
 src/cargo/core/resolver/mod.rs            | 10 ++++-----
 src/cargo/core/resolver/types.rs          |  4 +++-
 5 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs
index 9bd71e9c0f8..9954124854a 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::ConflictReason;
+use super::types::{Conflict, ConflictReason};
 use crate::core::resolver::Context;
 use crate::core::{Dependency, PackageId};
 
@@ -10,7 +10,7 @@ use crate::core::{Dependency, PackageId};
 /// efficiently see if any of the stored sets are a subset of a search set.
 enum ConflictStoreTrie {
     /// One of the stored sets.
-    Leaf(BTreeMap<PackageId, ConflictReason>),
+    Leaf(Conflict),
     /// A map from an element to a subtrie where
     /// all the sets in the subtrie contains that element.
     Node(BTreeMap<PackageId, ConflictStoreTrie>),
@@ -19,11 +19,7 @@ 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(
-        &self,
-        cx: &Context,
-        must_contain: Option<PackageId>,
-    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
+    fn find_conflicting(&self, cx: &Context, must_contain: Option<PackageId>) -> Option<&Conflict> {
         match self {
             ConflictStoreTrie::Leaf(c) => {
                 if must_contain.is_none() {
@@ -57,11 +53,7 @@ impl ConflictStoreTrie {
         }
     }
 
-    fn insert(
-        &mut self,
-        mut iter: impl Iterator<Item = PackageId>,
-        con: BTreeMap<PackageId, ConflictReason>,
-    ) {
+    fn insert(&mut self, mut iter: impl Iterator<Item = PackageId>, con: Conflict) {
         if let Some(pid) = iter.next() {
             if let ConflictStoreTrie::Node(p) = self {
                 p.entry(pid)
@@ -147,7 +139,7 @@ impl ConflictCache {
         cx: &Context,
         dep: &Dependency,
         must_contain: Option<PackageId>,
-    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
+    ) -> Option<&Conflict> {
         let out = self
             .con_from_dep
             .get(dep)?
@@ -161,18 +153,14 @@ impl ConflictCache {
         }
         out
     }
-    pub fn conflicting(
-        &self,
-        cx: &Context,
-        dep: &Dependency,
-    ) -> Option<&BTreeMap<PackageId, ConflictReason>> {
+    pub fn conflicting(&self, cx: &Context, dep: &Dependency) -> Option<&Conflict> {
         self.find_conflicting(cx, dep, None)
     }
 
     /// Adds to the cache a conflict of the form:
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated.
-    pub fn insert(&mut self, dep: &Dependency, con: &BTreeMap<PackageId, ConflictReason>) {
+    pub fn insert(&mut self, dep: &Dependency, con: &Conflict) {
         if con.values().any(|c| *c == ConflictReason::PublicDependency) {
             // TODO: needs more info for back jumping
             // for now refuse to cache it.
diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index bc1ae6f6776..d1516f048bf 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -1,4 +1,4 @@
-use std::collections::{BTreeMap, HashMap, HashSet};
+use std::collections::{HashMap, HashSet};
 use std::rc::Rc;
 
 // "ensure" seems to require "bail" be in scope (macro hygiene issue?).
@@ -12,7 +12,7 @@ use crate::util::CargoResult;
 use crate::util::Graph;
 
 use super::errors::ActivateResult;
-use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
+use super::types::{Conflict, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -155,7 +155,7 @@ impl Context {
     pub fn is_conflicting(
         &self,
         parent: Option<PackageId>,
-        conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
+        conflicting_activations: &Conflict,
     ) -> bool {
         conflicting_activations
             .keys()
diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs
index 976144b44d6..3b42e042654 100644
--- a/src/cargo/core/resolver/errors.rs
+++ b/src/cargo/core/resolver/errors.rs
@@ -1,4 +1,3 @@
-use std::collections::BTreeMap;
 use std::fmt;
 
 use crate::core::{Dependency, PackageId, Registry, Summary};
@@ -8,7 +7,7 @@ use failure::{Error, Fail};
 use semver;
 
 use super::context::Context;
-use super::types::{Candidate, ConflictReason};
+use super::types::{Candidate, Conflict, ConflictReason};
 
 /// Error during resolution providing a path of `PackageId`s.
 pub struct ResolveError {
@@ -73,7 +72,7 @@ pub(super) fn activation_error(
     registry: &mut dyn Registry,
     parent: &Summary,
     dep: &Dependency,
-    conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
+    conflicting_activations: &Conflict,
     candidates: &[Candidate],
     config: Option<&Config>,
 ) -> ResolveError {
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 87e08596e61..dfe618ab2ca 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -62,7 +62,7 @@ use crate::util::errors::CargoResult;
 use crate::util::profile;
 
 use self::context::{Activations, Context};
-use self::types::{Candidate, ConflictReason, DepsFrame, GraphNode};
+use self::types::{Candidate, Conflict, ConflictReason, DepsFrame, GraphNode};
 use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
@@ -247,7 +247,7 @@ fn activate_deps_loop(
         //
         // This is a map of package ID to a reason why that packaged caused a
         // conflict for us.
-        let mut conflicting_activations = BTreeMap::new();
+        let mut conflicting_activations = Conflict::new();
 
         // When backtracking we don't fully update `conflicting_activations`
         // especially for the cases that we didn't make a backtrack frame in the
@@ -680,7 +680,7 @@ struct BacktrackFrame {
     parent: Summary,
     dep: Dependency,
     features: Rc<Vec<InternedString>>,
-    conflicting_activations: BTreeMap<PackageId, ConflictReason>,
+    conflicting_activations: Conflict,
 }
 
 /// A helper "iterator" used to extract candidates within a current `Context` of
@@ -727,7 +727,7 @@ impl RemainingCandidates {
     /// original list for the reason listed.
     fn next(
         &mut self,
-        conflicting_prev_active: &mut BTreeMap<PackageId, ConflictReason>,
+        conflicting_prev_active: &mut Conflict,
         cx: &Context,
         dep: &Dependency,
         parent: PackageId,
@@ -845,7 +845,7 @@ fn find_candidate(
     backtrack_stack: &mut Vec<BacktrackFrame>,
     parent: &Summary,
     backtracked: bool,
-    conflicting_activations: &BTreeMap<PackageId, ConflictReason>,
+    conflicting_activations: &Conflict,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
         let next = frame.remaining_candidates.next(
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
index 84addf8bd31..3804894abb0 100644
--- a/src/cargo/core/resolver/types.rs
+++ b/src/cargo/core/resolver/types.rs
@@ -1,5 +1,5 @@
 use std::cmp::Ordering;
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::ops::Range;
 use std::rc::Rc;
 use std::time::{Duration, Instant};
@@ -425,6 +425,8 @@ impl ConflictReason {
     }
 }
 
+pub type Conflict = BTreeMap<PackageId, ConflictReason>;
+
 pub struct RcVecIter<T> {
     vec: Rc<Vec<T>>,
     rest: Range<usize>,

From 9b8b12c1f85082e623a4dc8c7b248d7b757efcde Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Mon, 11 Feb 2019 12:00:36 -0500
Subject: [PATCH 08/13] disable fuzzing of pub/priv deps so we can merge

partial removal of 8052d756aa97efac332cefcf1c5d98871b43f1bd
---
 tests/testsuite/support/resolver.rs | 47 +++++++++++++++--------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs
index 99ebc3a9d59..3ce9df8a10c 100644
--- a/tests/testsuite/support/resolver.rs
+++ b/tests/testsuite/support/resolver.rs
@@ -419,7 +419,8 @@ pub fn registry_strategy(
         any::<Index>(),
         raw_version_range,
         0..=1,
-        any::<bool>(),
+        Just(false),
+        // TODO: ^ this needs to be set back to `any::<bool>()` and work before public & private dependencies can stabilize
     );
 
     fn order_index(a: Index, b: Index, size: usize) -> (usize, usize) {
@@ -458,28 +459,28 @@ pub fn registry_strategy(
                     let s_last_index = s.len() - 1;
                     let (c, d) = order_index(c, d, s.len());
 
-                dependency_by_pkgid[b].push(dep_req_kind(
-                    &dep_name,
-                    &if c == 0 && d == s_last_index {
-                        "*".to_string()
-                    } else if c == 0 {
-                        format!("<={}", s[d].0)
-                    } else if d == s_last_index {
-                        format!(">={}", s[c].0)
-                    } else if c == d {
-                        format!("={}", s[c].0)
-                    } else {
-                        format!(">={}, <={}", s[c].0, s[d].0)
-                    },
-                    match k {
-                        0 => Kind::Normal,
-                        1 => Kind::Build,
-                        // => Kind::Development, // Development has not impact so don't gen
-                        _ => panic!("bad index for Kind"),
-                    },
-                    p,
-                ))
-            }
+                    dependency_by_pkgid[b].push(dep_req_kind(
+                        &dep_name,
+                        &if c == 0 && d == s_last_index {
+                            "*".to_string()
+                        } else if c == 0 {
+                            format!("<={}", s[d].0)
+                        } else if d == s_last_index {
+                            format!(">={}", s[c].0)
+                        } else if c == d {
+                            format!("={}", s[c].0)
+                        } else {
+                            format!(">={}, <={}", s[c].0, s[d].0)
+                        },
+                        match k {
+                            0 => Kind::Normal,
+                            1 => Kind::Build,
+                            // => Kind::Development, // Development has no impact so don't gen
+                            _ => panic!("bad index for Kind"),
+                        },
+                        p,
+                    ))
+                }
 
                 PrettyPrintRegistry(
                     list_of_pkgid

From c1ca055e0ba7beb7f9a348284fd40b4c5a7a105f Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Tue, 12 Feb 2019 14:38:24 -0500
Subject: [PATCH 09/13] add some comments

---
 src/cargo/core/resolver/conflict_cache.rs | 18 +++++---
 src/cargo/core/resolver/context.rs        | 13 +++++-
 src/cargo/core/resolver/errors.rs         |  4 +-
 src/cargo/core/resolver/mod.rs            | 53 ++++++++++++++---------
 src/cargo/core/resolver/types.rs          |  2 +-
 5 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs
index 9954124854a..b502e86d156 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::{Conflict, ConflictReason};
+use super::types::{ConflictMap, ConflictReason};
 use crate::core::resolver::Context;
 use crate::core::{Dependency, PackageId};
 
@@ -10,7 +10,7 @@ use crate::core::{Dependency, PackageId};
 /// efficiently see if any of the stored sets are a subset of a search set.
 enum ConflictStoreTrie {
     /// One of the stored sets.
-    Leaf(Conflict),
+    Leaf(ConflictMap),
     /// A map from an element to a subtrie where
     /// all the sets in the subtrie contains that element.
     Node(BTreeMap<PackageId, ConflictStoreTrie>),
@@ -19,7 +19,11 @@ 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(&self, cx: &Context, must_contain: Option<PackageId>) -> Option<&Conflict> {
+    fn find_conflicting(
+        &self,
+        cx: &Context,
+        must_contain: Option<PackageId>,
+    ) -> Option<&ConflictMap> {
         match self {
             ConflictStoreTrie::Leaf(c) => {
                 if must_contain.is_none() {
@@ -53,7 +57,7 @@ impl ConflictStoreTrie {
         }
     }
 
-    fn insert(&mut self, mut iter: impl Iterator<Item = PackageId>, con: Conflict) {
+    fn insert(&mut self, mut iter: impl Iterator<Item = PackageId>, con: ConflictMap) {
         if let Some(pid) = iter.next() {
             if let ConflictStoreTrie::Node(p) = self {
                 p.entry(pid)
@@ -139,7 +143,7 @@ impl ConflictCache {
         cx: &Context,
         dep: &Dependency,
         must_contain: Option<PackageId>,
-    ) -> Option<&Conflict> {
+    ) -> Option<&ConflictMap> {
         let out = self
             .con_from_dep
             .get(dep)?
@@ -153,14 +157,14 @@ impl ConflictCache {
         }
         out
     }
-    pub fn conflicting(&self, cx: &Context, dep: &Dependency) -> Option<&Conflict> {
+    pub fn conflicting(&self, cx: &Context, dep: &Dependency) -> Option<&ConflictMap> {
         self.find_conflicting(cx, dep, None)
     }
 
     /// Adds to the cache a conflict of the form:
     /// `dep` is known to be unresolvable if
     /// all the `PackageId` entries are activated.
-    pub fn insert(&mut self, dep: &Dependency, con: &Conflict) {
+    pub fn insert(&mut self, dep: &Dependency, con: &ConflictMap) {
         if con.values().any(|c| *c == ConflictReason::PublicDependency) {
             // TODO: needs more info for back jumping
             // for now refuse to cache it.
diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index d1516f048bf..5d417d2def6 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -12,7 +12,9 @@ use crate::util::CargoResult;
 use crate::util::Graph;
 
 use super::errors::ActivateResult;
-use super::types::{Conflict, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
+use super::types::{
+    ConflictMap, ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer,
+};
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -25,13 +27,19 @@ pub use super::resolve::Resolve;
 #[derive(Clone)]
 pub struct Context {
     pub activations: Activations,
+    /// list the features that are activated for each package
     pub resolve_features: im_rc::HashMap<PackageId, Rc<HashSet<InternedString>>>,
+    /// 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:
         im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>,
 
     // This is somewhat redundant with the `resolve_graph` that stores the same data,
     //   but for querying in the opposite order.
+    /// a way to look up for a package in activations what packages required it
+    /// and all of the exact deps that it fulfilled.
     pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,
 
     // These are two cheaply-cloneable lists (O(1) clone) which are effectively
@@ -44,6 +52,7 @@ pub struct Context {
     pub warnings: RcList<String>,
 }
 
+/// list all the activated versions of a particular crate name from a source
 pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;
 
 impl Context {
@@ -155,7 +164,7 @@ impl Context {
     pub fn is_conflicting(
         &self,
         parent: Option<PackageId>,
-        conflicting_activations: &Conflict,
+        conflicting_activations: &ConflictMap,
     ) -> bool {
         conflicting_activations
             .keys()
diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs
index 3b42e042654..071423690de 100644
--- a/src/cargo/core/resolver/errors.rs
+++ b/src/cargo/core/resolver/errors.rs
@@ -7,7 +7,7 @@ use failure::{Error, Fail};
 use semver;
 
 use super::context::Context;
-use super::types::{Candidate, Conflict, ConflictReason};
+use super::types::{Candidate, ConflictMap, ConflictReason};
 
 /// Error during resolution providing a path of `PackageId`s.
 pub struct ResolveError {
@@ -72,7 +72,7 @@ pub(super) fn activation_error(
     registry: &mut dyn Registry,
     parent: &Summary,
     dep: &Dependency,
-    conflicting_activations: &Conflict,
+    conflicting_activations: &ConflictMap,
     candidates: &[Candidate],
     config: Option<&Config>,
 ) -> ResolveError {
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index dfe618ab2ca..d837a487d50 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -62,7 +62,7 @@ use crate::util::errors::CargoResult;
 use crate::util::profile;
 
 use self::context::{Activations, Context};
-use self::types::{Candidate, Conflict, ConflictReason, DepsFrame, GraphNode};
+use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame, GraphNode};
 use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
@@ -247,7 +247,7 @@ fn activate_deps_loop(
         //
         // This is a map of package ID to a reason why that packaged caused a
         // conflict for us.
-        let mut conflicting_activations = Conflict::new();
+        let mut conflicting_activations = ConflictMap::new();
 
         // When backtracking we don't fully update `conflicting_activations`
         // especially for the cases that we didn't make a backtrack frame in the
@@ -590,28 +590,28 @@ fn activate(
     candidate: Candidate,
     method: &Method<'_>,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
+    let candidate_pid = candidate.summary.package_id();
     if let Some((parent, dep)) = parent {
-        cx.resolve_graph.push(GraphNode::Link(
-            parent.package_id(),
-            candidate.summary.package_id(),
-            dep.clone(),
-        ));
+        let parent_pid = parent.package_id();
+        cx.resolve_graph
+            .push(GraphNode::Link(parent_pid, candidate_pid, dep.clone()));
         Rc::make_mut(
-            cx.parents
-                .link(candidate.summary.package_id(), parent.package_id()),
+            // add a edge from candidate to parent in the parents graph
+            cx.parents.link(candidate_pid, parent_pid),
         )
+        // and associate dep with that edge
         .push(dep.clone());
         let cs: Vec<PackageId> = cx
             .public_dependency
-            .get(&candidate.summary.package_id())
+            .get(&candidate_pid)
             .iter()
             .flat_map(|x| x.values())
             .filter_map(|x| if x.1 { Some(&x.0) } else { None })
-            .chain(Some(candidate.summary.package_id()).iter())
+            .chain(&Some(candidate_pid))
             .cloned()
             .collect();
         for c in cs {
-            let mut stack = vec![(parent.package_id(), dep.is_public())];
+            let mut stack = vec![(parent_pid, dep.is_public())];
             while let Some((p, public)) = stack.pop() {
                 match cx.public_dependency.entry(p).or_default().entry(c.name()) {
                     im_rc::hashmap::Entry::Occupied(mut o) => {
@@ -641,14 +641,14 @@ fn activate(
     let candidate = match candidate.replace {
         Some(replace) => {
             cx.resolve_replacements
-                .push((candidate.summary.package_id(), replace.package_id()));
+                .push((candidate_pid, replace.package_id()));
             if cx.flag_activated(&replace, method)? && activated {
                 return Ok(None);
             }
             trace!(
                 "activating {} (replacing {})",
                 replace.package_id(),
-                candidate.summary.package_id()
+                candidate_pid
             );
             replace
         }
@@ -656,7 +656,7 @@ fn activate(
             if activated {
                 return Ok(None);
             }
-            trace!("activating {}", candidate.summary.package_id());
+            trace!("activating {}", candidate_pid);
             candidate.summary
         }
     };
@@ -680,7 +680,7 @@ struct BacktrackFrame {
     parent: Summary,
     dep: Dependency,
     features: Rc<Vec<InternedString>>,
-    conflicting_activations: Conflict,
+    conflicting_activations: ConflictMap,
 }
 
 /// A helper "iterator" used to extract candidates within a current `Context` of
@@ -727,7 +727,7 @@ impl RemainingCandidates {
     /// original list for the reason listed.
     fn next(
         &mut self,
-        conflicting_prev_active: &mut Conflict,
+        conflicting_prev_active: &mut ConflictMap,
         cx: &Context,
         dep: &Dependency,
         parent: PackageId,
@@ -769,13 +769,18 @@ impl RemainingCandidates {
                     continue;
                 }
             }
+            // We may still have to reject do to a public dependency conflict. If one of any of our
+            // ancestors that can see us already knows about a different crate with this name then
+            // we have to reject this candidate. Additionally this candidate may already have been
+            // activated and have public dependants of its own,
+            // all of witch also need to be checked the same way.
             for &t in cx
                 .public_dependency
                 .get(&b.summary.package_id())
                 .iter()
                 .flat_map(|x| x.values())
                 .filter_map(|x| if x.1 { Some(&x.0) } else { None })
-                .chain(Some(b.summary.package_id()).iter())
+                .chain(&Some(b.summary.package_id()))
             {
                 let mut stack = vec![(parent, dep.is_public())];
                 while let Some((p, public)) = stack.pop() {
@@ -845,7 +850,7 @@ fn find_candidate(
     backtrack_stack: &mut Vec<BacktrackFrame>,
     parent: &Summary,
     backtracked: bool,
-    conflicting_activations: &Conflict,
+    conflicting_activations: &ConflictMap,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
         let next = frame.remaining_candidates.next(
@@ -860,13 +865,19 @@ fn find_candidate(
         };
         // When we're calling this method we know that `parent` failed to
         // activate. That means that some dependency failed to get resolved for
-        // whatever reason, and all of those reasons (plus maybe some extras)
-        // are listed in `conflicting_activations`.
+        // whatever reason. Normally, that means that all of those reasons
+        // (plus maybe some extras) are listed in `conflicting_activations`.
         //
         // This means that if all members of `conflicting_activations` are still
         // active in this back up we know that we're guaranteed to not actually
         // make any progress. As a result if we hit this condition we can
         // completely skip this backtrack frame and move on to the next.
+        //
+        // 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.
         if !backtracked
             && !conflicting_activations
                 .values()
diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs
index 3804894abb0..452032597c2 100644
--- a/src/cargo/core/resolver/types.rs
+++ b/src/cargo/core/resolver/types.rs
@@ -425,7 +425,7 @@ impl ConflictReason {
     }
 }
 
-pub type Conflict = BTreeMap<PackageId, ConflictReason>;
+pub type ConflictMap = BTreeMap<PackageId, ConflictReason>;
 
 pub struct RcVecIter<T> {
     vec: Rc<Vec<T>>,

From a81fcb2fb7346bbe1edacca6d1cb460d7af62967 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Mon, 18 Feb 2019 13:50:54 -0500
Subject: [PATCH 10/13] disable public dependency checks with renamed and
 'cfg({})'

---
 src/cargo/core/resolver/mod.rs | 106 +++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index d837a487d50..2064b669d79 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -601,35 +601,42 @@ fn activate(
         )
         // and associate dep with that edge
         .push(dep.clone());
-        let cs: Vec<PackageId> = cx
-            .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 cs {
-            let mut stack = vec![(parent_pid, dep.is_public())];
-            while let Some((p, public)) = stack.pop() {
-                match cx.public_dependency.entry(p).or_default().entry(c.name()) {
-                    im_rc::hashmap::Entry::Occupied(mut o) => {
-                        assert_eq!(o.get().0, c);
-                        if o.get().1 {
-                            continue;
+        // For now the interaction of public dependency conflicts with renamed and
+        // 'cfg({})' dependencies is not clear. So we disable public checks if the
+        // other functionality is used.
+        // TODO: this disable is not a long term solution.
+        //       This needs to be removed before public dependencies are stabilized!
+        if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
+            let cs: Vec<PackageId> = cx
+                .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 cs {
+                let mut stack = vec![(parent_pid, dep.is_public())];
+                while let Some((p, public)) = stack.pop() {
+                    match cx.public_dependency.entry(p).or_default().entry(c.name()) {
+                        im_rc::hashmap::Entry::Occupied(mut o) => {
+                            assert_eq!(o.get().0, c);
+                            if o.get().1 {
+                                continue;
+                            }
+                            if public {
+                                o.insert((c, public));
+                            }
                         }
-                        if public {
-                            o.insert((c, public));
+                        im_rc::hashmap::Entry::Vacant(v) => {
+                            v.insert((c, public));
                         }
                     }
-                    im_rc::hashmap::Entry::Vacant(v) => {
-                        v.insert((c, public));
-                    }
-                }
-                if public {
-                    for &(grand, ref d) in cx.parents.edges(&p) {
-                        stack.push((grand, d.iter().any(|x| x.is_public())));
+                    if public {
+                        for &(grand, ref d) in cx.parents.edges(&p) {
+                            stack.push((grand, d.iter().any(|x| x.is_public())));
+                        }
                     }
                 }
             }
@@ -774,26 +781,35 @@ impl RemainingCandidates {
             // we have to reject this candidate. Additionally this candidate may already have been
             // activated and have public dependants of its own,
             // all of witch also need to be checked the same way.
-            for &t in cx
-                .public_dependency
-                .get(&b.summary.package_id())
-                .iter()
-                .flat_map(|x| x.values())
-                .filter_map(|x| if x.1 { Some(&x.0) } else { None })
-                .chain(&Some(b.summary.package_id()))
-            {
-                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) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name())) {
-                        if o.0 != t {
-                            conflicting_prev_active.insert(p, ConflictReason::PublicDependency);
-                            continue 'main;
+
+            // For now the interaction of public dependency conflicts with renamed and
+            // 'cfg({})' dependencies is not clear. So we disable public checks if the
+            // other functionality is used.
+            // TODO: this disable is not a long term solution.
+            //       This needs to be removed before public dependencies are stabilized!
+            if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
+                for &t in cx
+                    .public_dependency
+                    .get(&b.summary.package_id())
+                    .iter()
+                    .flat_map(|x| x.values())
+                    .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+                    .chain(&Some(b.summary.package_id()))
+                {
+                    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) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name()))
+                        {
+                            if o.0 != t {
+                                conflicting_prev_active.insert(p, ConflictReason::PublicDependency);
+                                continue 'main;
+                            }
                         }
-                    }
-                    if public {
-                        for &(grand, ref d) in cx.parents.edges(&p) {
-                            stack.push((grand, d.iter().any(|x| x.is_public())));
+                        if public {
+                            for &(grand, ref d) in cx.parents.edges(&p) {
+                                stack.push((grand, d.iter().any(|x| x.is_public())));
+                            }
                         }
                     }
                 }

From 9e437ac5fe648ad15bf964c7e5a7f6d0a83d95e7 Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Fri, 1 Mar 2019 15:20:03 -0500
Subject: [PATCH 11/13] add more comments

---
 src/cargo/core/resolver/mod.rs | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 2064b669d79..bed319885a6 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -607,7 +607,11 @@ fn activate(
         // TODO: this disable is not a long term solution.
         //       This needs to be removed before public dependencies are stabilized!
         if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
-            let cs: Vec<PackageId> = cx
+            // 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<PackageId> = cx
                 .public_dependency
                 .get(&candidate_pid)
                 .iter()
@@ -616,24 +620,34 @@ fn activate(
                 .chain(&Some(candidate_pid))
                 .cloned()
                 .collect();
-            for c in cs {
+            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 cx.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())));
                         }
@@ -788,25 +802,32 @@ impl RemainingCandidates {
             // TODO: this disable is not a long term solution.
             //       This needs to be removed before public dependencies are stabilized!
             if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
-                for &t in cx
+                let existing_public_deps: Vec<PackageId> = cx
                     .public_dependency
                     .get(&b.summary.package_id())
                     .iter()
                     .flat_map(|x| x.values())
                     .filter_map(|x| if x.1 { Some(&x.0) } else { None })
                     .chain(&Some(b.summary.package_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) = cx.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())));
                             }

From d016ceb2b565d527a3ff154300f09b94f03af9ce Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Mon, 4 Mar 2019 17:31:49 -0500
Subject: [PATCH 12/13] add a `check_public_visible_dependencies` flag

---
 src/cargo/core/resolver/mod.rs      | 61 +++++++++++++++++++++--------
 src/cargo/ops/resolve.rs            |  1 +
 tests/testsuite/support/resolver.rs |  1 +
 3 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index bed319885a6..45b915a14d7 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -107,6 +107,17 @@ mod types;
 ///
 /// * `print_warnings` - whether or not to print backwards-compatibility
 ///   warnings and such
+///
+/// * `check_public_visible_dependencies` - a flag for whether to enforce the restrictions
+///     introduced in the "public & private dependencies" RFC (1977). The current implementation
+///     makes sure that there is only one version of each name visible to each package.
+///
+///     But there are 2 stable ways to directly depend on different versions of the same name.
+///     1. Use the renamed dependencies functionality
+///     2. Use 'cfg({})' dependencies functionality
+///
+///     When we have a decision for how to implement is without breaking existing functionality
+///     this flag can be removed.
 pub fn resolve(
     summaries: &[(Summary, Method<'_>)],
     replacements: &[(PackageIdSpec, Dependency)],
@@ -114,6 +125,7 @@ pub fn resolve(
     try_to_use: &HashSet<PackageId>,
     config: Option<&Config>,
     print_warnings: bool,
+    check_public_visible_dependencies: bool,
 ) -> CargoResult<Resolve> {
     let cx = Context::new();
     let _p = profile::start("resolving");
@@ -122,7 +134,13 @@ pub fn resolve(
         None => false,
     };
     let mut registry = RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions);
-    let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
+    let cx = activate_deps_loop(
+        cx,
+        &mut registry,
+        summaries,
+        config,
+        check_public_visible_dependencies,
+    )?;
 
     let mut cksums = HashMap::new();
     for summary in cx.activations.values().flat_map(|v| v.iter()) {
@@ -170,6 +188,7 @@ fn activate_deps_loop(
     registry: &mut RegistryQueryer<'_>,
     summaries: &[(Summary, Method<'_>)],
     config: Option<&Config>,
+    check_public_visible_dependencies: bool,
 ) -> CargoResult<Context> {
     let mut backtrack_stack = Vec::new();
     let mut remaining_deps = RemainingDeps::new();
@@ -185,7 +204,14 @@ fn activate_deps_loop(
             summary: summary.clone(),
             replace: None,
         };
-        let res = activate(&mut cx, registry, None, candidate, method);
+        let res = activate(
+            &mut cx,
+            registry,
+            None,
+            candidate,
+            method,
+            check_public_visible_dependencies,
+        );
         match res {
             Ok(Some((frame, _))) => remaining_deps.push(frame),
             Ok(None) => (),
@@ -262,6 +288,7 @@ fn activate_deps_loop(
                 &cx,
                 &dep,
                 parent.package_id(),
+                check_public_visible_dependencies,
             );
 
             let (candidate, has_another) = next.ok_or(()).or_else(|_| {
@@ -301,6 +328,7 @@ fn activate_deps_loop(
                     &parent,
                     backtracked,
                     &conflicting_activations,
+                    check_public_visible_dependencies,
                 ) {
                     Some((candidate, has_another, frame)) => {
                         // Reset all of our local variables used with the
@@ -377,7 +405,14 @@ fn activate_deps_loop(
                 dep.package_name(),
                 candidate.summary.version()
             );
-            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &method);
+            let res = activate(
+                &mut cx,
+                registry,
+                Some((&parent, &dep)),
+                candidate,
+                &method,
+                check_public_visible_dependencies,
+            );
 
             let successfully_activated = match res {
                 // Success! We've now activated our `candidate` in our context
@@ -492,6 +527,7 @@ fn activate_deps_loop(
                                 &parent,
                                 backtracked,
                                 &conflicting_activations,
+                                check_public_visible_dependencies,
                             )
                             .is_none()
                         }
@@ -589,6 +625,7 @@ fn activate(
     parent: Option<(&Summary, &Dependency)>,
     candidate: Candidate,
     method: &Method<'_>,
+    check_public_visible_dependencies: bool,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
     let candidate_pid = candidate.summary.package_id();
     if let Some((parent, dep)) = parent {
@@ -601,12 +638,7 @@ fn activate(
         )
         // and associate dep with that edge
         .push(dep.clone());
-        // For now the interaction of public dependency conflicts with renamed and
-        // 'cfg({})' dependencies is not clear. So we disable public checks if the
-        // other functionality is used.
-        // TODO: this disable is not a long term solution.
-        //       This needs to be removed before public dependencies are stabilized!
-        if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
+        if check_public_visible_dependencies {
             // 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
@@ -752,6 +784,7 @@ impl RemainingCandidates {
         cx: &Context,
         dep: &Dependency,
         parent: PackageId,
+        check_public_visible_dependencies: bool,
     ) -> Option<(Candidate, bool)> {
         let prev_active = cx.prev_active(dep);
 
@@ -795,13 +828,7 @@ impl RemainingCandidates {
             // we have to reject this candidate. Additionally this candidate may already have been
             // activated and have public dependants of its own,
             // all of witch also need to be checked the same way.
-
-            // For now the interaction of public dependency conflicts with renamed and
-            // 'cfg({})' dependencies is not clear. So we disable public checks if the
-            // other functionality is used.
-            // TODO: this disable is not a long term solution.
-            //       This needs to be removed before public dependencies are stabilized!
-            if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {
+            if check_public_visible_dependencies {
                 let existing_public_deps: Vec<PackageId> = cx
                     .public_dependency
                     .get(&b.summary.package_id())
@@ -888,6 +915,7 @@ fn find_candidate(
     parent: &Summary,
     backtracked: bool,
     conflicting_activations: &ConflictMap,
+    check_public_visible_dependencies: bool,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
         let next = frame.remaining_candidates.next(
@@ -895,6 +923,7 @@ fn find_candidate(
             &frame.context,
             &frame.dep,
             frame.parent.package_id(),
+            check_public_visible_dependencies,
         );
         let (candidate, has_another) = match next {
             Some(pair) => pair,
diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index 5d87bc4d2d7..f14abf1821d 100644
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -338,6 +338,7 @@ pub fn resolve_with_previous<'cfg>(
         &try_to_use,
         Some(ws.config()),
         warn,
+        false, // TODO: use "public and private dependencies" feature flag
     )?;
     resolved.register_used_patches(registry.patches());
     if register_patches {
diff --git a/tests/testsuite/support/resolver.rs b/tests/testsuite/support/resolver.rs
index 3ce9df8a10c..bd2fb75b622 100644
--- a/tests/testsuite/support/resolver.rs
+++ b/tests/testsuite/support/resolver.rs
@@ -115,6 +115,7 @@ pub fn resolve_with_config_raw(
         &HashSet::new(),
         config,
         false,
+        true,
     );
 
     // The largest test in our suite takes less then 30 sec.

From 12e474b03f587a82a7ea038fcadcb17c8056acfe Mon Sep 17 00:00:00 2001
From: Eh2406 <YeomanYaacov@gmail.com>
Date: Tue, 5 Mar 2019 12:58:40 -0500
Subject: [PATCH 13/13] move `check_public_visible_dependencies` to `Context`

---
 src/cargo/core/resolver/conflict_cache.rs |  2 +-
 src/cargo/core/resolver/context.rs        | 10 +++--
 src/cargo/core/resolver/mod.rs            | 51 +++++------------------
 3 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs
index b502e86d156..58beb6da17f 100644
--- a/src/cargo/core/resolver/conflict_cache.rs
+++ b/src/cargo/core/resolver/conflict_cache.rs
@@ -64,7 +64,7 @@ impl ConflictStoreTrie {
                     .or_insert_with(|| ConflictStoreTrie::Node(BTreeMap::new()))
                     .insert(iter, con);
             }
-            // Else, we already have a subset of this in the `ConflictStore`.
+        // Else, we already have a subset of this in the `ConflictStore`.
         } else {
             // We are at the end of the set we are adding, there are three cases for what to do
             // next:
diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs
index 5d417d2def6..0c8b69ebf6f 100644
--- a/src/cargo/core/resolver/context.rs
+++ b/src/cargo/core/resolver/context.rs
@@ -34,7 +34,7 @@ pub struct Context {
     /// 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:
-        im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>,
+        Option<im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>>,
 
     // This is somewhat redundant with the `resolve_graph` that stores the same data,
     //   but for querying in the opposite order.
@@ -56,12 +56,16 @@ pub struct Context {
 pub type Activations = im_rc::HashMap<(InternedString, SourceId), Rc<Vec<Summary>>>;
 
 impl Context {
-    pub fn new() -> Context {
+    pub fn new(check_public_visible_dependencies: bool) -> Context {
         Context {
             resolve_graph: RcList::new(),
             resolve_features: im_rc::HashMap::new(),
             links: im_rc::HashMap::new(),
-            public_dependency: im_rc::HashMap::new(),
+            public_dependency: if check_public_visible_dependencies {
+                Some(im_rc::HashMap::new())
+            } else {
+                None
+            },
             parents: Graph::new(),
             resolve_replacements: RcList::new(),
             activations: im_rc::HashMap::new(),
diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 45b915a14d7..a0b57173af1 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -127,20 +127,14 @@ pub fn resolve(
     print_warnings: bool,
     check_public_visible_dependencies: bool,
 ) -> CargoResult<Resolve> {
-    let cx = Context::new();
+    let cx = Context::new(check_public_visible_dependencies);
     let _p = profile::start("resolving");
     let minimal_versions = match config {
         Some(config) => config.cli_unstable().minimal_versions,
         None => false,
     };
     let mut registry = RegistryQueryer::new(registry, replacements, try_to_use, minimal_versions);
-    let cx = activate_deps_loop(
-        cx,
-        &mut registry,
-        summaries,
-        config,
-        check_public_visible_dependencies,
-    )?;
+    let cx = activate_deps_loop(cx, &mut registry, summaries, config)?;
 
     let mut cksums = HashMap::new();
     for summary in cx.activations.values().flat_map(|v| v.iter()) {
@@ -188,7 +182,6 @@ fn activate_deps_loop(
     registry: &mut RegistryQueryer<'_>,
     summaries: &[(Summary, Method<'_>)],
     config: Option<&Config>,
-    check_public_visible_dependencies: bool,
 ) -> CargoResult<Context> {
     let mut backtrack_stack = Vec::new();
     let mut remaining_deps = RemainingDeps::new();
@@ -204,14 +197,7 @@ fn activate_deps_loop(
             summary: summary.clone(),
             replace: None,
         };
-        let res = activate(
-            &mut cx,
-            registry,
-            None,
-            candidate,
-            method,
-            check_public_visible_dependencies,
-        );
+        let res = activate(&mut cx, registry, None, candidate, method);
         match res {
             Ok(Some((frame, _))) => remaining_deps.push(frame),
             Ok(None) => (),
@@ -288,7 +274,6 @@ fn activate_deps_loop(
                 &cx,
                 &dep,
                 parent.package_id(),
-                check_public_visible_dependencies,
             );
 
             let (candidate, has_another) = next.ok_or(()).or_else(|_| {
@@ -328,7 +313,6 @@ fn activate_deps_loop(
                     &parent,
                     backtracked,
                     &conflicting_activations,
-                    check_public_visible_dependencies,
                 ) {
                     Some((candidate, has_another, frame)) => {
                         // Reset all of our local variables used with the
@@ -405,14 +389,7 @@ fn activate_deps_loop(
                 dep.package_name(),
                 candidate.summary.version()
             );
-            let res = activate(
-                &mut cx,
-                registry,
-                Some((&parent, &dep)),
-                candidate,
-                &method,
-                check_public_visible_dependencies,
-            );
+            let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, &method);
 
             let successfully_activated = match res {
                 // Success! We've now activated our `candidate` in our context
@@ -527,7 +504,6 @@ fn activate_deps_loop(
                                 &parent,
                                 backtracked,
                                 &conflicting_activations,
-                                check_public_visible_dependencies,
                             )
                             .is_none()
                         }
@@ -625,7 +601,6 @@ fn activate(
     parent: Option<(&Summary, &Dependency)>,
     candidate: Candidate,
     method: &Method<'_>,
-    check_public_visible_dependencies: bool,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
     let candidate_pid = candidate.summary.package_id();
     if let Some((parent, dep)) = parent {
@@ -638,13 +613,12 @@ fn activate(
         )
         // and associate dep with that edge
         .push(dep.clone());
-        if check_public_visible_dependencies {
+        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<PackageId> = cx
-                .public_dependency
+            let existing_public_deps: Vec<PackageId> = public_dependency
                 .get(&candidate_pid)
                 .iter()
                 .flat_map(|x| x.values())
@@ -656,7 +630,7 @@ fn activate(
                 // 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 cx.public_dependency.entry(p).or_default().entry(c.name()) {
+                    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);
@@ -784,7 +758,6 @@ impl RemainingCandidates {
         cx: &Context,
         dep: &Dependency,
         parent: PackageId,
-        check_public_visible_dependencies: bool,
     ) -> Option<(Candidate, bool)> {
         let prev_active = cx.prev_active(dep);
 
@@ -828,9 +801,8 @@ impl RemainingCandidates {
             // we have to reject this candidate. Additionally this candidate may already have been
             // activated and have public dependants of its own,
             // all of witch also need to be checked the same way.
-            if check_public_visible_dependencies {
-                let existing_public_deps: Vec<PackageId> = cx
-                    .public_dependency
+            if let Some(public_dependency) = cx.public_dependency.as_ref() {
+                let existing_public_deps: Vec<PackageId> = public_dependency
                     .get(&b.summary.package_id())
                     .iter()
                     .flat_map(|x| x.values())
@@ -843,8 +815,7 @@ impl RemainingCandidates {
                     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) = cx.public_dependency.get(&p).and_then(|x| x.get(&t.name()))
-                        {
+                        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`.
@@ -915,7 +886,6 @@ fn find_candidate(
     parent: &Summary,
     backtracked: bool,
     conflicting_activations: &ConflictMap,
-    check_public_visible_dependencies: bool,
 ) -> Option<(Candidate, bool, BacktrackFrame)> {
     while let Some(mut frame) = backtrack_stack.pop() {
         let next = frame.remaining_candidates.next(
@@ -923,7 +893,6 @@ fn find_candidate(
             &frame.context,
             &frame.dep,
             frame.parent.package_id(),
-            check_public_visible_dependencies,
         );
         let (candidate, has_another) = match next {
             Some(pair) => pair,