add some comments

Author: Eh2406

src/cargo/core/resolver/conflict_cache.rs

@@ -2,15 +2,15 @@ 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};
 
 /// This is a trie for storing a large number of sets designed to
 /// 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.

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()

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 {

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,22 +641,22 @@ 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
         }
         None => {
             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()

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>>,