Redesign features

src/cargo/core/feature.rs

@@ -0,0 +1,34 @@
+#[derive(Debug, Clone)]
+pub struct Feature {
+    dependencies: Vec<String>,
+    features: Vec<String>,
+}
+
+impl Feature {
+    pub fn new() -> Feature {
+        Feature {
+            dependencies: vec![],
+            features: vec![],
+        }
+    }
+
+    pub fn dependencies(&self) -> &[String] {
+        &self.dependencies
+    }
+
+    pub fn features(&self) -> &[String] {
+        &self.features
+    }
+
+    /// Sets the list of dependencies required by this feature
+    pub fn set_dependencies(mut self, dependencies: Vec<String>) -> Feature {
+        self.dependencies = dependencies;
+        self
+    }
+
+    /// Sets the list of features that this feature depends on
+    pub fn set_features(mut self, features: Vec<String>) -> Feature {
+        self.features = features;
+        self
+    }
+}

src/cargo/core/mod.rs

@@ -8,6 +8,7 @@ pub use self::resolver::Resolve;
 pub use self::shell::{Shell, MultiShell, ShellConfig};
 pub use self::source::{Source, SourceId, SourceMap, SourceSet, GitReference};
 pub use self::summary::Summary;
+pub use self::feature::Feature;
 
 pub mod source;
 pub mod package;
@@ -18,4 +19,5 @@ pub mod resolver;
 pub mod summary;
 pub mod shell;
 pub mod registry;
+pub mod feature;
 mod package_id_spec;

src/cargo/core/resolver/mod.rs

@@ -6,7 +6,7 @@ use std::fmt;
 use std::rc::Rc;
 use semver;
 
-use core::{PackageId, Registry, SourceId, Summary, Dependency};
+use core::{PackageId, Registry, SourceId, Summary, Dependency, Feature};
 use core::PackageIdSpec;
 use util::{CargoResult, Graph, human, ChainError, CargoError};
 use util::profile;
@@ -33,10 +33,12 @@ pub struct Resolve {
 #[derive(Copy)]
 pub enum Method<'a> {
     Everything,
-    Required(/* dev_deps = */ bool,
-             /* features = */ &'a [String],
-             /* uses_default_features = */ bool,
-             /* target_platform = */ Option<&'a str>),
+    Required {
+        dev_deps: bool,
+        features: &'a [String],
+        uses_default_features: bool,
+        target_platform: Option<&'a str>
+    },
 }
 
 impl Resolve {
@@ -170,7 +172,7 @@ fn activate(mut cx: Box<Context>,
 
     // Extracting the platform request.
     let platform = match method {
-        Method::Required(_, _, _, platform) => platform,
+        Method::Required { target_platform, .. } => target_platform,
         Method::Everything => None,
     };
 
@@ -230,7 +232,7 @@ fn flag_activated(cx: &mut Context,
     }
     debug!("checking if {} is already activated", summary.package_id());
     let features = match *method {
-        Method::Required(_, features, _, _) => features,
+        Method::Required { features, .. } => features,
         Method::Everything => return false,
     };
     match cx.resolve.features(id) {
@@ -247,8 +249,12 @@ fn activate_deps<'a>(cx: Box<Context>,
                      cur: usize) -> CargoResult<CargoResult<Box<Context>>> {
     if cur == deps.len() { return Ok(Ok(cx)) }
     let (dep, ref candidates, ref features) = deps[cur];
-    let method = Method::Required(false, &features,
-                                  dep.uses_default_features(), platform);
+    let method = Method::Required {
+        dev_deps: false,
+        features: &features,
+        uses_default_features: dep.uses_default_features(),
+        target_platform: platform
+    };
 
     let key = (dep.name().to_string(), dep.source_id().clone());
     let prev_active = cx.activations.get(&key)
@@ -424,7 +430,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
                                                (&'a Dependency, Vec<String>)>> {
     let dev_deps = match method {
         Method::Everything => true,
-        Method::Required(dev_deps, _, _, _) => dev_deps,
+        Method::Required { dev_deps, .. } => dev_deps,
     };
 
     // First, filter by dev-dependencies
@@ -434,7 +440,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
     // Second, ignoring dependencies that should not be compiled for this platform
     let deps = deps.filter(|d| {
         match method {
-            Method::Required(_, _, _, Some(ref platform)) => {
+            Method::Required { target_platform: Some(ref platform), .. } => {
                 d.is_active_for_platform(platform)
             },
             _ => true
@@ -452,7 +458,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
             continue
         }
         let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]);
-        for feature in dep.features().iter() {
+        for feature in dep.features() {
             base.push(feature.clone());
             if feature.contains("/") {
                 return Err(human(format!("features in dependencies \
@@ -464,19 +470,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
         ret.insert(dep.name(), (dep, base));
     }
 
-    // All features can only point to optional dependencies, in which case they
-    // should have all been weeded out by the above iteration. Any remaining
-    // features are bugs in that the package does not actually have those
-    // features.
-    if feature_deps.len() > 0 {
-        let unknown = feature_deps.keys().map(|s| s.as_slice())
-                                  .collect::<Vec<&str>>();
-        if unknown.len() > 0 {
-            let features = unknown.connect(", ");
-            return Err(human(format!("Package `{}` does not have these features: \
-                                      `{}`", parent.package_id(), features)))
-        }
-    }
+    assert!(feature_deps.is_empty(), "ended up with leftover features: {:?}", feature_deps);
 
     // Record what list of features is active for this package.
     if used_features.len() > 0 {
@@ -506,78 +500,66 @@ fn build_features(s: &Summary, method: Method)
     let mut visited = HashSet::new();
     match method {
         Method::Everything => {
-            for key in s.features().keys() {
-                try!(add_feature(s, key, &mut deps, &mut used, &mut visited));
-            }
             for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
-                try!(add_feature(s, dep.name(), &mut deps, &mut used,
-                                 &mut visited));
+                deps.insert(dep.name().to_string(), vec![]);
+            }
+            for (name, feat) in s.features() {
+                try!(add_feature(s, name, feat, &mut deps, &mut used, &mut visited));
             }
         }
-        Method::Required(_, requested_features, _, _) =>  {
-            for feat in requested_features.iter() {
-                try!(add_feature(s, feat, &mut deps, &mut used, &mut visited));
+        Method::Required { features: requested_features, .. } =>  {
+            let (good, bad): (Vec<_>, Vec<_>) = requested_features.iter()
+                .filter(|f| !f.is_empty())
+                .partition(|f| s.features().contains_key(*f));
+            if !bad.is_empty() {
+                return Err(human(format!("Package `{}` does not have these features: \
+                                          `{}`", s.package_id(), bad.connect(","))))
+            }
+            for (name, feat) in good.iter().map(|feat| (feat, &s.features()[**feat])) {
+                try!(add_feature(s, name, feat, &mut deps, &mut used, &mut visited));
             }
         }
     }
     match method {
-        Method::Everything | Method::Required(_, _, true, _) => {
-            if s.features().get("default").is_some() &&
-               !visited.contains("default") {
-                try!(add_feature(s, "default", &mut deps, &mut used,
-                                 &mut visited));
+        Method::Everything | Method::Required { uses_default_features: true, .. } => {
+            match s.features().get("default") {
+                Some(feat) if !visited.contains("default") => {
+                    try!(add_feature(s, "default", feat, &mut deps, &mut used, &mut visited));
+                }
+                _ => {}
             }
         }
         _ => {}
     }
     return Ok((deps, used));
 
-    fn add_feature(s: &Summary, feat: &str,
+    fn add_feature(s: &Summary, name: &str, feat: &Feature,
                    deps: &mut HashMap<String, Vec<String>>,
                    used: &mut HashSet<String>,
                    visited: &mut HashSet<String>) -> CargoResult<()> {
-        if feat.is_empty() { return Ok(()) }
-
-        // If this feature is of the form `foo/bar`, then we just lookup package
-        // `foo` and enable its feature `bar`. Otherwise this feature is of the
-        // form `foo` and we need to recurse to enable the feature `foo` for our
-        // own package, which may end up enabling more features or just enabling
-        // a dependency.
-        let mut parts = feat.splitn(1, '/');
-        let feat_or_package = parts.next().unwrap();
-        match parts.next() {
-            Some(feat) => {
-                let package = feat_or_package;
-                match deps.entry(package.to_string()) {
-                    Occupied(e) => e.into_mut(),
-                    Vacant(e) => e.insert(Vec::new()),
-                }.push(feat.to_string());
+        used.insert(name.to_string());
+
+        for dep in feat.dependencies() {
+            let mut parts = dep.splitn(1, '/');
+            let package = parts.next().unwrap();
+            let feats = match deps.entry(package.to_string()) {
+                Occupied(e) => e.into_mut(),
+                Vacant(e) => e.insert(Vec::new()),
+            };
+            if let Some(feat) = parts.next() {
+                feats.push(feat.to_string());
             }
-            None => {
-                let feat = feat_or_package;
-                if !visited.insert(feat.to_string()) {
-                    return Err(human(format!("Cyclic feature dependency: \
-                                              feature `{}` depends on itself",
-                                              feat)))
-                }
-                used.insert(feat.to_string());
-                match s.features().get(feat) {
-                    Some(recursive) => {
-                        for f in recursive.iter() {
-                            try!(add_feature(s, f, deps, used,
-                                             visited));
-                        }
-                    }
-                    None => {
-                        match deps.entry(feat.to_string()) {
-                            Occupied(..) => {} // already activated
-                            Vacant(e) => { e.insert(Vec::new()); }
-                        }
-                    }
-                }
-                visited.remove(&feat.to_string());
+        }
+
+        for subfeat in feat.features() {
+            if !visited.insert(subfeat.to_string()) {
+                return Err(human(format!("Cyclic feature dependency: feature \
+                                         `{}` depends on itself", subfeat)));
             }
+            try!(add_feature(s, subfeat, &s.features()[*subfeat], deps, used, visited));
+            visited.remove(subfeat);
         }
+
         Ok(())
     }
 }

src/cargo/core/summary.rs

@@ -2,7 +2,7 @@ use std::collections::HashMap;
 use std::mem;
 
 use semver::Version;
-use core::{Dependency, PackageId, SourceId};
+use core::{Dependency, PackageId, SourceId, Feature};
 
 use util::{CargoResult, human};
 
@@ -14,30 +14,25 @@ use util::{CargoResult, human};
 pub struct Summary {
     package_id: PackageId,
     dependencies: Vec<Dependency>,
-    features: HashMap<String, Vec<String>>,
+    features: HashMap<String, Feature>,
 }
 
 impl Summary {
     pub fn new(pkg_id: PackageId,
                dependencies: Vec<Dependency>,
-               features: HashMap<String, Vec<String>>) -> CargoResult<Summary> {
-        for dep in dependencies.iter() {
-            if features.get(dep.name()).is_some() {
-                return Err(human(format!("Features and dependencies cannot have \
-                                          the same name: `{}`", dep.name())))
-            }
+               features: HashMap<String, Feature>) -> CargoResult<Summary> {
+        for dep in &dependencies {
             if dep.is_optional() && !dep.is_transitive() {
                 return Err(human(format!("Dev-dependencies are not allowed \
                                           to be optional: `{}`",
                                           dep.name())))
             }
         }
-        for (feature, list) in features.iter() {
-            for dep in list.iter() {
+        for (feature, desc) in &features {
+            for dep in desc.dependencies() {
                 let mut parts = dep.splitn(1, '/');
                 let dep = parts.next().unwrap();
                 let is_reexport = parts.next().is_some();
-                if !is_reexport && features.get(dep).is_some() { continue }
                 match dependencies.iter().find(|d| d.name() == dep) {
                     Some(d) => {
                         if d.is_optional() || is_reexport { continue }
@@ -47,19 +42,20 @@ impl Summary {
                                                   `optional = true` to the \
                                                   dependency", feature, dep)))
                     }
-                    None if is_reexport => {
-                        return Err(human(format!("Feature `{}` requires `{}` \
-                                                  which is not an optional \
-                                                  dependency", feature, dep)))
-                    }
                     None => {
-                        return Err(human(format!("Feature `{}` includes `{}` \
-                                                  which is neither a dependency \
-                                                  nor another feature",
+                        return Err(human(format!("Feature `{}` requires `{}` \
+                                                  which is not a dependency",
                                                   feature, dep)))
                     }
                 }
             }
+            for subfeat in desc.features() {
+                if !features.contains_key(subfeat) {
+                    return Err(human(format!("Feature `{}` requires `{}` which \
+                                              is not a feature", feature,
+                                              subfeat)));
+                }
+            }
         }
         Ok(Summary {
             package_id: pkg_id,
@@ -73,7 +69,7 @@ impl Summary {
     pub fn version(&self) -> &Version { self.package_id().version() }
     pub fn source_id(&self) -> &SourceId { self.package_id.source_id() }
     pub fn dependencies(&self) -> &[Dependency] { &self.dependencies }
-    pub fn features(&self) -> &HashMap<String, Vec<String>> { &self.features }
+    pub fn features(&self) -> &HashMap<String, Feature> { &self.features }
 
     pub fn override_id(mut self, id: PackageId) -> Summary {
         self.package_id = id;

src/cargo/ops/cargo_compile.rs

@@ -109,8 +109,12 @@ pub fn compile_pkg(package: &Package, options: &CompileOptions)
         try!(registry.add_overrides(override_ids));
 
         let platform = target.as_ref().map(|e| e.as_slice()).or(Some(rustc_host.as_slice()));
-        let method = Method::Required(dev_deps, &features,
-                                      !no_default_features, platform);
+        let method = Method::Required {
+            dev_deps: dev_deps,
+            features: &features,
+            uses_default_features: !no_default_features,
+            target_platform: platform
+        };
         let resolved_with_overrides =
                 try!(ops::resolve_with_previous(&mut registry, package, method,
                                                 Some(&resolve), None));