move compatible to a type

This makes a O(n^2) loop in the hart of the resolver a O(n) loop,
 but n is small and hashing is not free.
So the main reason to do this is to make the code clearer.

Author: Eh2406

src/cargo/core/resolver/context.rs

@@ -1,4 +1,5 @@
 use std::collections::{HashMap, HashSet};
+use std::num::NonZeroU64;
 use std::rc::Rc;
 
 // "ensure" seems to require "bail" be in scope (macro hygiene issue?).
@@ -52,8 +53,36 @@ 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>>>;
+/// find the activated version of a crate based on the name, source, and semver compatibility
+pub type Activations = im_rc::HashMap<(InternedString, SourceId, SemverCompatibility), Summary>;
+
+/// A type that represents when cargo treats two Versions as compatible.
+/// Versions `a` and `b` are compatible if their left-most nonzero digit is the
+/// same.
+#[derive(Clone, Copy, Eq, PartialEq, Hash)]
+pub enum SemverCompatibility {
+    Major(NonZeroU64),
+    Minor(NonZeroU64),
+    Patch(u64),
+}
+
+impl From<&semver::Version> for SemverCompatibility {
+    fn from(ver: &semver::Version) -> Self {
+        if let Some(m) = NonZeroU64::new(ver.major) {
+            return SemverCompatibility::Major(m);
+        }
+        if let Some(m) = NonZeroU64::new(ver.minor) {
+            return SemverCompatibility::Minor(m);
+        }
+        SemverCompatibility::Patch(ver.patch)
+    }
+}
+
+impl PackageId {
+    pub fn as_activations_key(&self) -> (InternedString, SourceId, SemverCompatibility) {
+        (self.name(), self.source_id(), self.version().into())
+    }
+}
 
 impl Context {
     pub fn new(check_public_visible_dependencies: bool) -> Context {
@@ -78,22 +107,27 @@ impl Context {
     /// Returns `true` if this summary with the given method is already activated.
     pub fn flag_activated(&mut self, summary: &Summary, method: &Method<'_>) -> CargoResult<bool> {
         let id = summary.package_id();
-        let prev = self
-            .activations
-            .entry((id.name(), id.source_id()))
-            .or_insert_with(|| Rc::new(Vec::new()));
-        if !prev.iter().any(|c| c == summary) {
-            self.resolve_graph.push(GraphNode::Add(id));
-            if let Some(link) = summary.links() {
-                ensure!(
-                    self.links.insert(link, id).is_none(),
-                    "Attempting to resolve a dependency with more then one crate with the \
-                     links={}.\nThis will not build as is. Consider rebuilding the .lock file.",
-                    &*link
+        match self.activations.entry(id.as_activations_key()) {
+            im_rc::hashmap::Entry::Occupied(o) => {
+                debug_assert_eq!(
+                    o.get(),
+                    summary,
+                    "cargo does not allow two semver compatible versions"
                 );
             }
-            Rc::make_mut(prev).push(summary.clone());
-            return Ok(false);
+            im_rc::hashmap::Entry::Vacant(v) => {
+                self.resolve_graph.push(GraphNode::Add(id));
+                if let Some(link) = summary.links() {
+                    ensure!(
+                        self.links.insert(link, id).is_none(),
+                        "Attempting to resolve a dependency with more then one crate with the \
+                         links={}.\nThis will not build as is. Consider rebuilding the .lock file.",
+                        &*link
+                    );
+                }
+                v.insert(summary.clone());
+                return Ok(false);
+            }
         }
         debug!("checking if {} is already activated", summary.package_id());
         let (features, use_default) = match *method {
@@ -149,17 +183,10 @@ impl Context {
         Ok(deps)
     }
 
-    pub fn prev_active(&self, dep: &Dependency) -> &[Summary] {
-        self.activations
-            .get(&(dep.package_name(), dep.source_id()))
-            .map(|v| &v[..])
-            .unwrap_or(&[])
-    }
-
     pub fn is_active(&self, id: PackageId) -> bool {
         self.activations
-            .get(&(id.name(), id.source_id()))
-            .map(|v| v.iter().any(|s| s.package_id() == id))
+            .get(&id.as_activations_key())
+            .map(|s| s.package_id() == id)
             .unwrap_or(false)
     }
 

src/cargo/core/resolver/mod.rs

@@ -137,7 +137,7 @@ pub fn resolve(
     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()) {
+    for summary in cx.activations.values() {
         let cksum = summary.checksum().map(|s| s.to_string());
         cksums.insert(summary.package_id(), cksum);
     }
@@ -237,13 +237,6 @@ fn activate_deps_loop(
             dep.package_name(),
             candidates.len()
         );
-        trace!(
-            "{}[{}]>{} {} prev activations",
-            parent.name(),
-            cur,
-            dep.package_name(),
-            cx.prev_active(&dep).len()
-        );
 
         let just_here_for_the_error_messages = just_here_for_the_error_messages
             && past_conflicting_activations
@@ -759,16 +752,15 @@ impl RemainingCandidates {
         dep: &Dependency,
         parent: PackageId,
     ) -> Option<(Candidate, bool)> {
-        let prev_active = cx.prev_active(dep);
-
         'main: for (_, b) in self.remaining.by_ref() {
+            let b_id = b.summary.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(&a) = cx.links.get(&link) {
-                    if a != b.summary.package_id() {
+                    if a != b_id {
                         conflicting_prev_active
                             .entry(a)
                             .or_insert_with(|| ConflictReason::Links(link));
@@ -785,10 +777,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) = prev_active
-                .iter()
-                .find(|a| compatible(a.version(), b.summary.version()))
-            {
+            if let Some(a) = cx.activations.get(&b_id.as_activations_key()) {
                 if *a != b.summary {
                     conflicting_prev_active
                         .entry(a.package_id())
@@ -803,11 +792,11 @@ impl RemainingCandidates {
             // all of witch also need to be checked the same way.
             if let Some(public_dependency) = cx.public_dependency.as_ref() {
                 let existing_public_deps: Vec<PackageId> = public_dependency
-                    .get(&b.summary.package_id())
+                    .get(&b_id)
                     .iter()
                     .flat_map(|x| x.values())
                     .filter_map(|x| if x.1 { Some(&x.0) } else { None })
-                    .chain(&Some(b.summary.package_id()))
+                    .chain(&Some(b_id))
                     .cloned()
                     .collect();
                 for t in existing_public_deps {
@@ -851,27 +840,6 @@ impl RemainingCandidates {
     }
 }
 
-// Returns if `a` and `b` are compatible in the semver sense. This is a
-// commutative operation.
-//
-// Versions `a` and `b` are compatible if their left-most nonzero digit is the
-// same.
-fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
-    if a.major != b.major {
-        return false;
-    }
-    if a.major != 0 {
-        return true;
-    }
-    if a.minor != b.minor {
-        return false;
-    }
-    if a.minor != 0 {
-        return true;
-    }
-    a.patch == b.patch
-}
-
 /// Looks through the states in `backtrack_stack` for dependencies with
 /// remaining candidates. For each one, also checks if rolling back
 /// could change the outcome of the failed resolution that caused backtracking
@@ -937,11 +905,8 @@ fn find_candidate(
 }
 
 fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()> {
-    let summaries: HashMap<PackageId, &Summary> = activations
-        .values()
-        .flat_map(|v| v.iter())
-        .map(|s| (s.package_id(), s))
-        .collect();
+    let summaries: HashMap<PackageId, &Summary> =
+        activations.values().map(|s| (s.package_id(), s)).collect();
 
     // Sort packages to produce user friendly deterministic errors.
     let mut all_packages: Vec<_> = resolve.iter().collect();