Introduce Requirements struct to clarify code

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