Auto merge of #6249 - Eh2406:cleanup, r=alexcrichton

Resolver cleanups and a new fuzz test

This is the commits from my on going work on pub/priv deps that did not relate to that functionality. Then it also adds a fuzz test that minimal-versions does not change whether resolution is possible.

cc #6120

Author: bors

src/cargo/core/resolver/context.rs

@@ -6,8 +6,8 @@ use core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
 use util::CargoResult;
 use util::Graph;
 
-use super::types::RegistryQueryer;
-use super::types::{ActivateResult, ConflictReason, DepInfo, GraphNode, Method, RcList};
+use super::types::{ConflictReason, DepInfo, GraphNode, Method, RcList, RegistryQueryer};
+use super::errors::ActivateResult;
 
 pub use super::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use super::encode::{Metadata, WorkspaceResolve};
@@ -186,11 +186,10 @@ impl Context {
             // name.
             let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
             used_features.insert(dep.name_in_toml());
-            let always_required = !dep.is_optional()
-                && !s
-                    .dependencies()
-                    .iter()
-                    .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
+            let always_required = !dep.is_optional() && !s
+                .dependencies()
+                .iter()
+                .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
             if always_required && base.0 {
                 self.warnings.push(format!(
                     "Package `{}` does not have feature `{}`. It has a required dependency \
@@ -230,11 +229,13 @@ impl Context {
                     "Package `{}` does not have these features: `{}`",
                     s.package_id(),
                     features
-                ).into(),
+                )
+                .into(),
                 Some(p) => (
                     p.package_id().clone(),
                     ConflictReason::MissingFeatures(features),
-                ).into(),
+                )
+                    .into(),
             });
         }
 

src/cargo/core/resolver/encode.rs

@@ -377,7 +377,8 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
             root: None,
             metadata,
             patch,
-        }.serialize(s)
+        }
+        .serialize(s)
     }
 }
 

src/cargo/core/resolver/errors.rs

@@ -4,8 +4,8 @@ use std::fmt;
 use core::{Dependency, PackageId, Registry, Summary};
 use failure::{Error, Fail};
 use semver;
-use util::config::Config;
 use util::lev_distance::lev_distance;
+use util::{CargoError, Config};
 
 use super::context::Context;
 use super::types::{Candidate, ConflictReason};
@@ -49,6 +49,25 @@ impl fmt::Display for ResolveError {
     }
 }
 
+pub type ActivateResult<T> = Result<T, ActivateError>;
+
+pub enum ActivateError {
+    Fatal(CargoError),
+    Conflict(PackageId, ConflictReason),
+}
+
+impl From<::failure::Error> for ActivateError {
+    fn from(t: ::failure::Error) -> Self {
+        ActivateError::Fatal(t)
+    }
+}
+
+impl From<(PackageId, ConflictReason)> for ActivateError {
+    fn from(t: (PackageId, ConflictReason)) -> Self {
+        ActivateError::Conflict(t.0, t.1)
+    }
+}
+
 pub(super) fn activation_error(
     cx: &Context,
     registry: &mut Registry,

src/cargo/core/resolver/mod.rs

@@ -62,21 +62,21 @@ use util::errors::CargoResult;
 use util::profile;
 
 use self::context::{Activations, Context};
-use self::types::{ActivateError, ActivateResult, Candidate, ConflictReason, DepsFrame, GraphNode};
+use self::types::{Candidate, ConflictReason, DepsFrame, GraphNode};
 use self::types::{RcVecIter, RegistryQueryer, RemainingDeps, ResolverProgress};
 
 pub use self::encode::{EncodableDependency, EncodablePackageId, EncodableResolve};
 pub use self::encode::{Metadata, WorkspaceResolve};
+pub use self::errors::{ActivateError, ActivateResult, ResolveError};
 pub use self::resolve::Resolve;
 pub use self::types::Method;
-pub use self::errors::ResolveError;
 
 mod conflict_cache;
 mod context;
 mod encode;
+mod errors;
 mod resolve;
 mod types;
-mod errors;
 
 /// Builds the list of all packages required to build the first argument.
 ///
@@ -403,7 +403,8 @@ fn activate_deps_loop(
                             .clone()
                             .filter_map(|(_, (ref new_dep, _, _))| {
                                 past_conflicting_activations.conflicting(&cx, new_dep)
-                            }).next()
+                            })
+                            .next()
                         {
                             // If one of our deps is known unresolvable
                             // then we will not succeed.
@@ -438,12 +439,15 @@ fn activate_deps_loop(
                                 // for deps related to us
                                 .filter(|&(_, ref other_dep)| {
                                     known_related_bad_deps.contains(other_dep)
-                                }).filter_map(|(other_parent, other_dep)| {
+                                })
+                                .filter_map(|(other_parent, other_dep)| {
                                     past_conflicting_activations
                                         .find_conflicting(&cx, &other_dep, |con| {
                                             con.contains_key(&pid)
-                                        }).map(|con| (other_parent, con))
-                                }).next()
+                                        })
+                                        .map(|con| (other_parent, con))
+                                })
+                                .next()
                             {
                                 let rel = conflict.get(&pid).unwrap().clone();
 
@@ -485,7 +489,8 @@ fn activate_deps_loop(
                                 &parent,
                                 backtracked,
                                 &conflicting_activations,
-                            ).is_none()
+                            )
+                            .is_none()
                         }
                     };
 

src/cargo/core/resolver/resolve.rs

@@ -171,6 +171,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated
         self.graph.contains(k)
     }
 
+    pub fn sort(&self) -> Vec<PackageId> {
+        self.graph.sort()
+    }
+
     pub fn iter(&self) -> impl Iterator<Item = &PackageId> {
         self.graph.iter()
     }

src/cargo/core/resolver/types.rs

@@ -6,7 +6,8 @@ use std::time::{Duration, Instant};
 
 use core::interning::InternedString;
 use core::{Dependency, PackageId, PackageIdSpec, Registry, Summary};
-use util::{CargoError, CargoResult, Config};
+use util::errors::CargoResult;
+use util::Config;
 
 pub struct ResolverProgress {
     ticks: u16,
@@ -348,25 +349,6 @@ impl RemainingDeps {
 // (dependency info, candidates, features activated)
 pub type DepInfo = (Dependency, Rc<Vec<Candidate>>, Rc<Vec<InternedString>>);
 
-pub type ActivateResult<T> = Result<T, ActivateError>;
-
-pub enum ActivateError {
-    Fatal(CargoError),
-    Conflict(PackageId, ConflictReason),
-}
-
-impl From<::failure::Error> for ActivateError {
-    fn from(t: ::failure::Error) -> Self {
-        ActivateError::Fatal(t)
-    }
-}
-
-impl From<(PackageId, ConflictReason)> for ActivateError {
-    fn from(t: (PackageId, ConflictReason)) -> Self {
-        ActivateError::Conflict(t.0, t.1)
-    }
-}
-
 /// All possible reasons that a package might fail to activate.
 ///
 /// We maintain a list of conflicts for error reporting as well as backtracking

src/cargo/util/graph.rs

@@ -1,5 +1,5 @@
 use std::borrow::Borrow;
-use std::collections::hash_map::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fmt;
 use std::hash::Hash;
 
@@ -42,6 +42,30 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<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();
+
+        for node in self.nodes.keys() {
+            self.sort_inner_visit(node, &mut ret, &mut marks);
+        }
+
+        ret
+    }
+
+    fn sort_inner_visit(&self, node: &N, dst: &mut Vec<N>, marks: &mut HashSet<N>) {
+        if !marks.insert(node.clone()) {
+            return;
+        }
+
+        for child in self.nodes[node].keys() {
+            self.sort_inner_visit(child, dst, marks);
+        }
+
+        dst.push(node.clone());
+    }
+
     pub fn iter(&self) -> impl Iterator<Item = &N> {
         self.nodes.keys()
     }