Auto merge of #4683 - djc:requirements, r=alexcrichton

Introduce Requirements struct to clarify code

`cargo::core::resolver::build_requirements()` previously, in this order:

* Defined local variables to track state
* Called a function to mutate the local variables
* Defined the aforementioned function
* Returned two out of three local variables as a tuple

This PR changes the code so that this is a recast as a struct (appropriately called `Requirements`), which is mutated in a more fine-grained way by its methods and acts as the return type for `build_requirements()`. To me, this seems a lot easier to understand.

This work was done in the context of #1286, but I figured it was easier to start landing some of the refactoring to avoid bitrot and get early (well, earlier) feedback.

Author: bors

src/cargo/core/resolver/mod.rs

@@ -859,55 +859,68 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
     a.patch == b.patch
 }
 
-// Returns a pair of (feature dependencies, all used features)
-//
-// The feature dependencies map is a mapping of package name to list of features
-// enabled. Each package should be enabled, and each package should have the
-// specified set of features enabled.  The boolean indicates whether this
-// package was specifically requested (rather than just requesting features
-// *within* this package).
-//
-// The all used features set is the set of features which this local package had
-// enabled, which is later used when compiling to instruct the code what
-// features were enabled.
-fn build_features<'a>(s: &'a Summary, method: &'a Method)
-                      -> CargoResult<(HashMap<&'a str, (bool, Vec<String>)>, HashSet<&'a str>)> {
-    let mut deps = HashMap::new();
-    let mut used = HashSet::new();
-    let mut visited = HashSet::new();
-    match *method {
-        Method::Everything => {
-            for key in s.features().keys() {
-                add_feature(s, key, &mut deps, &mut used, &mut visited)?;
-            }
-            for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
-                add_feature(s, dep.name(), &mut deps, &mut used,
-                            &mut visited)?;
-            }
+struct Requirements<'a> {
+    summary: &'a Summary,
+    // The deps map is a mapping of package name to list of features enabled.
+    // Each package should be enabled, and each package should have the
+    // specified set of features enabled. The boolean indicates whether this
+    // package was specifically requested (rather than just requesting features
+    // *within* this package).
+    deps: HashMap<&'a str, (bool, Vec<String>)>,
+    // The used features set is the set of features which this local package had
+    // enabled, which is later used when compiling to instruct the code what
+    // features were enabled.
+    used: HashSet<&'a str>,
+    visited: HashSet<&'a str>,
+}
+
+impl<'r> Requirements<'r> {
+    fn new<'a>(summary: &'a Summary) -> Requirements<'a> {
+        Requirements {
+            summary,
+            deps: HashMap::new(),
+            used: HashSet::new(),
+            visited: HashSet::new(),
         }
-        Method::Required { features: requested_features, .. } =>  {
-            for feat in requested_features.iter() {
-                add_feature(s, feat, &mut deps, &mut used, &mut visited)?;
-            }
+    }
+
+    fn require_crate_feature(&mut self, package: &'r str, feat: &'r str) {
+        self.used.insert(package);
+        self.deps.entry(package)
+            .or_insert((false, Vec::new()))
+            .1.push(feat.to_string());
+    }
+
+    fn seen(&mut self, feat: &'r str) -> bool {
+        if self.visited.insert(feat) {
+            self.used.insert(feat);
+            false
+        } else {
+            true
         }
     }
-    match *method {
-        Method::Everything |
-        Method::Required { uses_default_features: true, .. } => {
-            if s.features().get("default").is_some() {
-                add_feature(s, "default", &mut deps, &mut used,
-                            &mut visited)?;
+
+    fn require_dependency(&mut self, pkg: &'r str) {
+        if self.seen(pkg) {
+            return;
+        }
+        self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true;
+    }
+
+    fn require_feature(&mut self, feat: &'r str) -> CargoResult<()> {
+        if self.seen(feat) {
+            return Ok(());
+        }
+        for f in self.summary.features().get(feat).expect("must be a valid feature") {
+            if f == &feat {
+                bail!("Cyclic feature dependency: feature `{}` depends on itself", feat);
             }
+            self.add_feature(f)?;
         }
-        Method::Required { uses_default_features: false, .. } => {}
+        Ok(())
     }
-    return Ok((deps, used));
 
-    fn add_feature<'a>(s: &'a Summary,
-                       feat: &'a str,
-                       deps: &mut HashMap<&'a str, (bool, Vec<String>)>,
-                       used: &mut HashSet<&'a str>,
-                       visited: &mut HashSet<&'a str>) -> CargoResult<()> {
+    fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> {
         if feat.is_empty() { return Ok(()) }
 
         // If this feature is of the form `foo/bar`, then we just lookup package
@@ -919,44 +932,53 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method)
         let feat_or_package = parts.next().unwrap();
         match parts.next() {
             Some(feat) => {
-                let package = feat_or_package;
-                used.insert(package);
-                deps.entry(package)
-                    .or_insert((false, Vec::new()))
-                    .1.push(feat.to_string());
+                self.require_crate_feature(feat_or_package, feat);
             }
             None => {
-                let feat = feat_or_package;
-
-                //if this feature has already been added, then just return Ok
-                if !visited.insert(feat) {
-                    return Ok(());
-                }
-
-                used.insert(feat);
-                match s.features().get(feat) {
-                    Some(recursive) => {
-                        // This is a feature, add it recursively.
-                        for f in recursive {
-                            if f == feat {
-                                bail!("Cyclic feature dependency: feature `{}` depends \
-                                        on itself", feat);
-                            }
-
-                            add_feature(s, f, deps, used, visited)?;
-                        }
-                    }
-                    None => {
-                        // This is a dependency, mark it as explicitly requested.
-                        deps.entry(feat).or_insert((false, Vec::new())).0 = true;
-                    }
+                if self.summary.features().contains_key(feat_or_package) {
+                    self.require_feature(feat_or_package)?;
+                } else {
+                    self.require_dependency(feat_or_package);
                 }
             }
         }
         Ok(())
     }
 }
 
+// Takes requested features for a single package from the input Method and
+// recurses to find all requested features, dependencies and requested
+// dependency features in a Requirements object, returning it to the resolver.
+fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method)
+                                  -> CargoResult<Requirements<'a>> {
+    let mut reqs = Requirements::new(s);
+    match *method {
+        Method::Everything => {
+            for key in s.features().keys() {
+                reqs.require_feature(key)?;
+            }
+            for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
+                reqs.require_dependency(dep.name());
+            }
+        }
+        Method::Required { features: requested_features, .. } =>  {
+            for feat in requested_features.iter() {
+                reqs.add_feature(feat)?;
+            }
+        }
+    }
+    match *method {
+        Method::Everything |
+        Method::Required { uses_default_features: true, .. } => {
+            if s.features().get("default").is_some() {
+                reqs.require_feature("default")?;
+            }
+        }
+        Method::Required { uses_default_features: false, .. } => {}
+    }
+    return Ok(reqs);
+}
+
 impl<'a> Context<'a> {
     // Activate this summary by inserting it into our list of known activations.
     //
@@ -1117,18 +1139,18 @@ impl<'a> Context<'a> {
         let deps = s.dependencies();
         let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
 
-        let (mut feature_deps, used_features) = build_features(s, method)?;
+        let mut reqs = build_requirements(s, method)?;
         let mut ret = Vec::new();
 
         // Next, collect all actually enabled dependencies and their features.
         for dep in deps {
             // Skip optional dependencies, but not those enabled through a feature
-            if dep.is_optional() && !feature_deps.contains_key(dep.name()) {
+            if dep.is_optional() && !reqs.deps.contains_key(dep.name()) {
                 continue
             }
             // So we want this dependency.  Move the features we want from `feature_deps`
             // to `ret`.
-            let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![]));
+            let base = reqs.deps.remove(dep.name()).unwrap_or((false, vec![]));
             if !dep.is_optional() && base.0 {
                 self.warnings.push(
                     format!("Package `{}` does not have feature `{}`. It has a required dependency \
@@ -1151,21 +1173,22 @@ impl<'a> Context<'a> {
         // Any remaining entries in feature_deps are bugs in that the package does not actually
         // have those dependencies.  We classified them as dependencies in the first place
         // because there is no such feature, either.
-        if !feature_deps.is_empty() {
-            let unknown = feature_deps.keys().map(|s| &s[..])
-                                      .collect::<Vec<&str>>();
+        if !reqs.deps.is_empty() {
+            let unknown = reqs.deps.keys()
+                                   .map(|s| &s[..])
+                                   .collect::<Vec<&str>>();
             let features = unknown.join(", ");
             bail!("Package `{}` does not have these features: `{}`",
                     s.package_id(), features)
         }
 
         // Record what list of features is active for this package.
-        if !used_features.is_empty() {
+        if !reqs.used.is_empty() {
             let pkgid = s.package_id();
 
             let set = self.resolve_features.entry(pkgid.clone())
                               .or_insert_with(HashSet::new);
-            for feature in used_features {
+            for feature in reqs.used {
                 if !set.contains(feature) {
                     set.insert(feature.to_string());
                 }