Auto merge of #6946 - Eh2406:remove_candidate, r=alexcrichton

Remove Candidate

`Candidate` was a type to record the possibility that a package was replaced using the replacements feature. However there were only two places that cared about this possibility. One was switched to used a lookup table in #6860. This PR switches the other to use that table as well. Making the entire `Candidate` type unnecessary.

The main benefit of this change is a reduction in the cognitive complexity.

Author: bors

src/cargo/core/resolver/dep_cache.rs

@@ -19,7 +19,7 @@ use crate::core::interning::InternedString;
 use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary};
 use crate::util::errors::CargoResult;
 
-use crate::core::resolver::types::{Candidate, ConflictReason, DepInfo, FeaturesSet};
+use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet};
 use crate::core::resolver::{ActivateResult, Method};
 
 pub struct RegistryQueryer<'a> {
@@ -31,14 +31,14 @@ pub struct RegistryQueryer<'a> {
     /// specify minimum dependency versions to be used.
     minimal_versions: bool,
     /// a cache of `Candidate`s that fulfil a `Dependency`
-    registry_cache: HashMap<Dependency, Rc<Vec<Candidate>>>,
+    registry_cache: HashMap<Dependency, Rc<Vec<Summary>>>,
     /// a cache of `Dependency`s that are required for a `Summary`
     summary_cache: HashMap<
         (Option<PackageId>, Summary, Method),
         Rc<(HashSet<InternedString>, Rc<Vec<DepInfo>>)>,
     >,
     /// all the cases we ended up using a supplied replacement
-    used_replacements: HashMap<PackageId, PackageId>,
+    used_replacements: HashMap<PackageId, Summary>,
 }
 
 impl<'a> RegistryQueryer<'a> {
@@ -60,7 +60,11 @@ impl<'a> RegistryQueryer<'a> {
     }
 
     pub fn used_replacement_for(&self, p: PackageId) -> Option<(PackageId, PackageId)> {
-        self.used_replacements.get(&p).map(|&r| (p, r))
+        self.used_replacements.get(&p).map(|r| (p, r.package_id()))
+    }
+
+    pub fn replacement_summary(&self, p: PackageId) -> Option<&Summary> {
+        self.used_replacements.get(&p)
     }
 
     /// Queries the `registry` to return a list of candidates for `dep`.
@@ -69,7 +73,7 @@ impl<'a> RegistryQueryer<'a> {
     /// any candidates are returned which match an override then the override is
     /// applied by performing a second query for what the override should
     /// return.
-    pub fn query(&mut self, dep: &Dependency) -> CargoResult<Rc<Vec<Candidate>>> {
+    pub fn query(&mut self, dep: &Dependency) -> CargoResult<Rc<Vec<Summary>>> {
         if let Some(out) = self.registry_cache.get(dep).cloned() {
             return Ok(out);
         }
@@ -78,16 +82,11 @@ impl<'a> RegistryQueryer<'a> {
         self.registry.query(
             dep,
             &mut |s| {
-                ret.push(Candidate {
-                    summary: s,
-                    replace: None,
-                });
+                ret.push(s);
             },
             false,
         )?;
-        for candidate in ret.iter_mut() {
-            let summary = &candidate.summary;
-
+        for summary in ret.iter_mut() {
             let mut potential_matches = self
                 .replacements
                 .iter()
@@ -157,25 +156,22 @@ impl<'a> RegistryQueryer<'a> {
             for dep in summary.dependencies() {
                 debug!("\t{} => {}", dep.package_name(), dep.version_req());
             }
-            if let Some(r) = &replace {
-                self.used_replacements
-                    .insert(summary.package_id(), r.package_id());
+            if let Some(r) = replace {
+                self.used_replacements.insert(summary.package_id(), r);
             }
-
-            candidate.replace = replace;
         }
 
         // When we attempt versions for a package we'll want to do so in a
         // sorted fashion to pick the "best candidates" first. Currently we try
         // prioritized summaries (those in `try_to_use`) and failing that we
         // list everything from the maximum version to the lowest version.
         ret.sort_unstable_by(|a, b| {
-            let a_in_previous = self.try_to_use.contains(&a.summary.package_id());
-            let b_in_previous = self.try_to_use.contains(&b.summary.package_id());
+            let a_in_previous = self.try_to_use.contains(&a.package_id());
+            let b_in_previous = self.try_to_use.contains(&b.package_id());
             let previous_cmp = a_in_previous.cmp(&b_in_previous).reverse();
             match previous_cmp {
                 Ordering::Equal => {
-                    let cmp = a.summary.version().cmp(b.summary.version());
+                    let cmp = a.version().cmp(b.version());
                     if self.minimal_versions {
                         // Lower version ordered first.
                         cmp

src/cargo/core/resolver/errors.rs

@@ -7,7 +7,7 @@ use failure::{Error, Fail};
 use semver;
 
 use super::context::Context;
-use super::types::{Candidate, ConflictMap, ConflictReason};
+use super::types::{ConflictMap, ConflictReason};
 
 /// Error during resolution providing a path of `PackageId`s.
 pub struct ResolveError {
@@ -74,7 +74,7 @@ pub(super) fn activation_error(
     parent: &Summary,
     dep: &Dependency,
     conflicting_activations: &ConflictMap,
-    candidates: &[Candidate],
+    candidates: &[Summary],
     config: Option<&Config>,
 ) -> ResolveError {
     let to_resolve_err = |err| {
@@ -101,7 +101,7 @@ pub(super) fn activation_error(
         msg.push_str(
             &candidates
                 .iter()
-                .map(|v| v.summary.version())
+                .map(|v| v.version())
                 .map(|v| v.to_string())
                 .collect::<Vec<_>>()
                 .join(", "),

src/cargo/core/resolver/mod.rs

@@ -62,7 +62,7 @@ use crate::util::profile;
 
 use self::context::{Activations, Context};
 use self::dep_cache::RegistryQueryer;
-use self::types::{Candidate, ConflictMap, ConflictReason, DepsFrame};
+use self::types::{ConflictMap, ConflictReason, DepsFrame};
 use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
@@ -181,11 +181,7 @@ fn activate_deps_loop(
     // Activate all the initial summaries to kick off some work.
     for &(ref summary, ref method) in summaries {
         debug!("initial activation: {}", summary.package_id());
-        let candidate = Candidate {
-            summary: summary.clone(),
-            replace: None,
-        };
-        let res = activate(&mut cx, registry, None, candidate, method.clone());
+        let res = activate(&mut cx, registry, None, summary.clone(), method.clone());
         match res {
             Ok(Some((frame, _))) => remaining_deps.push(frame),
             Ok(None) => (),
@@ -370,7 +366,7 @@ fn activate_deps_loop(
                 None
             };
 
-            let pid = candidate.summary.package_id();
+            let pid = candidate.package_id();
             let method = Method::Required {
                 dev_deps: false,
                 features: Rc::clone(&features),
@@ -382,7 +378,7 @@ fn activate_deps_loop(
                 parent.name(),
                 cur,
                 dep.package_name(),
-                candidate.summary.version()
+                candidate.version()
             );
             let res = activate(&mut cx, registry, Some((&parent, &dep)), candidate, method);
 
@@ -595,10 +591,10 @@ fn activate(
     cx: &mut Context,
     registry: &mut RegistryQueryer<'_>,
     parent: Option<(&Summary, &Dependency)>,
-    candidate: Candidate,
+    candidate: Summary,
     method: Method,
 ) -> ActivateResult<Option<(DepsFrame, Duration)>> {
-    let candidate_pid = candidate.summary.package_id();
+    let candidate_pid = candidate.package_id();
     if let Some((parent, dep)) = parent {
         let parent_pid = parent.package_id();
         Rc::make_mut(
@@ -657,9 +653,9 @@ fn activate(
         }
     }
 
-    let activated = cx.flag_activated(&candidate.summary, &method)?;
+    let activated = cx.flag_activated(&candidate, &method)?;
 
-    let candidate = match candidate.replace {
+    let candidate = match registry.replacement_summary(candidate_pid) {
         Some(replace) => {
             if cx.flag_activated(&replace, &method)? && activated {
                 return Ok(None);
@@ -669,14 +665,14 @@ fn activate(
                 replace.package_id(),
                 candidate_pid
             );
-            replace
+            replace.clone()
         }
         None => {
             if activated {
                 return Ok(None);
             }
             trace!("activating {}", candidate_pid);
-            candidate.summary
+            candidate
         }
     };
 
@@ -727,13 +723,13 @@ struct BacktrackFrame {
 /// filtered out, and as they are filtered the causes will be added to `conflicting_prev_active`.
 #[derive(Clone)]
 struct RemainingCandidates {
-    remaining: RcVecIter<Candidate>,
+    remaining: RcVecIter<Summary>,
     // This is a inlined peekable generator
-    has_another: Option<Candidate>,
+    has_another: Option<Summary>,
 }
 
 impl RemainingCandidates {
-    fn new(candidates: &Rc<Vec<Candidate>>) -> RemainingCandidates {
+    fn new(candidates: &Rc<Vec<Summary>>) -> RemainingCandidates {
         RemainingCandidates {
             remaining: RcVecIter::new(Rc::clone(candidates)),
             has_another: None,
@@ -762,14 +758,14 @@ impl RemainingCandidates {
         cx: &Context,
         dep: &Dependency,
         parent: PackageId,
-    ) -> Option<(Candidate, bool)> {
+    ) -> Option<(Summary, bool)> {
         'main: for (_, b) in self.remaining.by_ref() {
-            let b_id = b.summary.package_id();
+            let b_id = b.package_id();
             // The `links` key in the manifest dictates that there's only one
             // package in a dependency graph, globally, with that particular
             // `links` key. If this candidate links to something that's already
             // linked to by a different package then we've gotta skip this.
-            if let Some(link) = b.summary.links() {
+            if let Some(link) = b.links() {
                 if let Some(&a) = cx.links.get(&link) {
                     if a != b_id {
                         conflicting_prev_active
@@ -789,7 +785,7 @@ impl RemainingCandidates {
             // Here we throw out our candidate if it's *compatible*, yet not
             // equal, to all previously activated versions.
             if let Some((a, _)) = cx.activations.get(&b_id.as_activations_key()) {
-                if *a != b.summary {
+                if *a != b {
                     conflicting_prev_active
                         .entry(a.package_id())
                         .or_insert(ConflictReason::Semver);
@@ -905,7 +901,7 @@ fn generalize_conflicting(
                         .find(
                             dep,
                             &|id| {
-                                if id == other.summary.package_id() {
+                                if id == other.package_id() {
                                     // we are imagining that we used other instead
                                     Some(backtrack_critical_age)
                                 } else {
@@ -914,9 +910,9 @@ fn generalize_conflicting(
                                         age < backtrack_critical_age)
                                 }
                             },
-                            Some(other.summary.package_id()),
+                            Some(other.package_id()),
                         )
-                        .map(|con| (other.summary.package_id(), con))
+                        .map(|con| (other.package_id(), con))
                 })
                 .collect::<Option<Vec<(PackageId, &ConflictMap)>>>()
             {
@@ -973,7 +969,7 @@ fn find_candidate(
     parent: &Summary,
     backtracked: bool,
     conflicting_activations: &ConflictMap,
-) -> Option<(Candidate, bool, BacktrackFrame)> {
+) -> Option<(Summary, bool, BacktrackFrame)> {
     // 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. Normally, that means that all of those reasons

src/cargo/core/resolver/types.rs

@@ -122,12 +122,6 @@ impl Method {
     }
 }
 
-#[derive(Clone)]
-pub struct Candidate {
-    pub summary: Summary,
-    pub replace: Option<Summary>,
-}
-
 #[derive(Clone)]
 pub struct DepsFrame {
     pub parent: Summary,
@@ -231,7 +225,7 @@ impl RemainingDeps {
 /// Information about the dependencies for a crate, a tuple of:
 ///
 /// (dependency info, candidates, features activated)
-pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, FeaturesSet);
+pub type DepInfo = (Dependency, Rc<Vec<Summary>>, FeaturesSet);
 
 /// All possible reasons that a package might fail to activate.
 ///