Auto merge of #5300 - djc:namespaced-features, r=alexcrichton

Introduction of namespaced features (see #1286)

I think this basically covers all of the plans from #1286, although it still needs a bunch of tests and documentation updates. Submitting this PR to get some early feedback on the direction.

src/cargo/core/features.rs

@@ -177,6 +177,9 @@ features! {
 
         // Overriding profiles for dependencies.
         [unstable] profile_overrides: bool,
+
+        // Separating the namespaces for features and dependencies
+        [unstable] namespaced_features: bool,
     }
 }
 

src/cargo/core/summary.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 use std::mem;
 use std::rc::Rc;
 
@@ -26,6 +26,7 @@ struct Inner {
     features: FeatureMap,
     checksum: Option<String>,
     links: Option<InternedString>,
+    namespaced_features: bool,
 }
 
 impl Summary {
@@ -34,9 +35,10 @@ impl Summary {
         dependencies: Vec<Dependency>,
         features: BTreeMap<String, Vec<String>>,
         links: Option<String>,
+        namespaced_features: bool,
     ) -> CargoResult<Summary> {
         for dep in dependencies.iter() {
-            if features.get(&*dep.name()).is_some() {
+            if !namespaced_features && features.get(&*dep.name()).is_some() {
                 bail!(
                     "Features and dependencies cannot have the \
                      same name: `{}`",
@@ -50,14 +52,15 @@ impl Summary {
                 )
             }
         }
-        let feature_map = build_feature_map(features, &dependencies)?;
+        let feature_map = build_feature_map(features, &dependencies, namespaced_features)?;
         Ok(Summary {
             inner: Rc::new(Inner {
                 package_id: pkg_id,
                 dependencies,
                 features: feature_map,
                 checksum: None,
                 links: links.map(|l| InternedString::new(&l)),
+                namespaced_features,
             }),
         })
     }
@@ -86,6 +89,9 @@ impl Summary {
     pub fn links(&self) -> Option<InternedString> {
         self.inner.links
     }
+    pub fn namespaced_features(&self) -> bool {
+        self.inner.namespaced_features
+    }
 
     pub fn override_id(mut self, id: PackageId) -> Summary {
         Rc::make_mut(&mut self.inner).package_id = id;
@@ -131,55 +137,179 @@ impl PartialEq for Summary {
 fn build_feature_map(
     features: BTreeMap<String, Vec<String>>,
     dependencies: &[Dependency],
+    namespaced: bool,
 ) -> CargoResult<FeatureMap> {
     use self::FeatureValue::*;
+    let dep_map: HashMap<_, _> = dependencies
+        .iter()
+        .map(|d| (d.name().as_str(), d))
+        .collect();
+
     let mut map = BTreeMap::new();
     for (feature, list) in features.iter() {
+        // If namespaced features is active and the key is the same as that of an
+        // optional dependency, that dependency must be included in the values.
+        // Thus, if a `feature` is found that has the same name as a dependency, we
+        // (a) bail out if the dependency is non-optional, and (b) we track if the
+        // feature requirements include the dependency `crate:feature` in the list.
+        // This is done with the `dependency_found` variable, which can only be
+        // false if features are namespaced and the current feature key is the same
+        // as the name of an optional dependency. If so, it gets set to true during
+        // iteration over the list if the dependency is found in the list.
+        let mut dependency_found = if namespaced {
+            match dep_map.get(feature.as_str()) {
+                Some(ref dep_data) => {
+                    if !dep_data.is_optional() {
+                        bail!(
+                            "Feature `{}` includes the dependency of the same name, but this is \
+                             left implicit in the features included by this feature.\n\
+                             Additionally, the dependency must be marked as optional to be \
+                             included in the feature definition.\n\
+                             Consider adding `crate:{}` to this feature's requirements \
+                             and marking the dependency as `optional = true`",
+                            feature,
+                            feature
+                        )
+                    } else {
+                        false
+                    }
+                }
+                None => true,
+            }
+        } else {
+            true
+        };
+
         let mut values = vec![];
         for dep in list {
-            let val = FeatureValue::build(InternedString::new(dep), |fs| features.contains_key(fs));
-            if let &Feature(_) = &val {
-                values.push(val);
-                continue;
-            }
+            let val = FeatureValue::build(
+                InternedString::new(dep),
+                |fs| features.contains_key(fs),
+                namespaced,
+            );
 
             // Find data for the referenced dependency...
             let dep_data = {
-                let dep_name = match val {
-                    Feature(_) => "",
-                    Crate(ref dep_name) | CrateFeature(ref dep_name, _) => dep_name,
-                };
-                dependencies.iter().find(|d| *d.name() == *dep_name)
+                match val {
+                    Feature(ref dep_name) | Crate(ref dep_name) | CrateFeature(ref dep_name, _) => {
+                        dep_map.get(dep_name.as_str())
+                    }
+                }
             };
+            let is_optional_dep = dep_data.map_or(false, |d| d.is_optional());
+            if let FeatureValue::Crate(ref dep_name) = val {
+                // If we have a dependency value, check if this is the dependency named
+                // the same as the feature that we were looking for.
+                if !dependency_found && feature == dep_name.as_str() {
+                    dependency_found = true;
+                }
+            }
 
-            match (&val, dep_data) {
-                (&Crate(ref dep), Some(d)) => {
-                    if !d.is_optional() {
-                        bail!(
-                            "Feature `{}` depends on `{}` which is not an \
-                             optional dependency.\nConsider adding \
-                             `optional = true` to the dependency",
-                            feature,
-                            dep
-                        )
+            match (&val, dep_data.is_some(), is_optional_dep) {
+                // The value is a feature. If features are namespaced, this just means
+                // it's not prefixed with `crate:`, so we have to check whether the
+                // feature actually exist. If the feature is not defined *and* an optional
+                // dependency of the same name exists, the feature is defined implicitly
+                // here by adding it to the feature map, pointing to the dependency.
+                // If features are not namespaced, it's been validated as a feature already
+                // while instantiating the `FeatureValue` in `FeatureValue::build()`, so
+                // we don't have to do so here.
+                (&Feature(feat), _, true) => {
+                    if namespaced && !features.contains_key(&*feat) {
+                        map.insert(feat.to_string(), vec![FeatureValue::Crate(feat)]);
+                    }
+                }
+                // If features are namespaced and the value is not defined as a feature
+                // and there is no optional dependency of the same name, error out.
+                // If features are not namespaced, there must be an existing feature
+                // here (checked by `FeatureValue::build()`), so it will always be defined.
+                (&Feature(feat), dep_exists, false) => {
+                    if namespaced && !features.contains_key(&*feat) {
+                        if dep_exists {
+                            bail!(
+                                "Feature `{}` includes `{}` which is not defined as a feature.\n\
+                                 A non-optional dependency of the same name is defined; consider \
+                                 adding `optional = true` to its definition",
+                                feature,
+                                feat
+                            )
+                        } else {
+                            bail!(
+                                "Feature `{}` includes `{}` which is not defined as a feature",
+                                feature,
+                                feat
+                            )
+                        }
                     }
                 }
-                (&CrateFeature(ref dep_name, _), None) => bail!(
+                // The value is a dependency. If features are namespaced, it is explicitly
+                // tagged as such (`crate:value`). If features are not namespaced, any value
+                // not recognized as a feature is pegged as a `Crate`. Here we handle the case
+                // where the dependency exists but is non-optional. It branches on namespaced
+                // just to provide the correct string for the crate dependency in the error.
+                (&Crate(ref dep), true, false) => if namespaced {
+                    bail!(
+                        "Feature `{}` includes `crate:{}` which is not an \
+                         optional dependency.\nConsider adding \
+                         `optional = true` to the dependency",
+                        feature,
+                        dep
+                    )
+                } else {
+                    bail!(
+                        "Feature `{}` depends on `{}` which is not an \
+                         optional dependency.\nConsider adding \
+                         `optional = true` to the dependency",
+                        feature,
+                        dep
+                    )
+                },
+                // If namespaced, the value was tagged as a dependency; if not namespaced,
+                // this could be anything not defined as a feature. This handles the case
+                // where no such dependency is actually defined; again, the branch on
+                // namespaced here is just to provide the correct string in the error.
+                (&Crate(ref dep), false, _) => if namespaced {
+                    bail!(
+                        "Feature `{}` includes `crate:{}` which is not a known \
+                         dependency",
+                        feature,
+                        dep
+                    )
+                } else {
+                    bail!(
+                        "Feature `{}` includes `{}` which is neither a dependency nor \
+                         another feature",
+                        feature,
+                        dep
+                    )
+                },
+                (&Crate(_), true, true) => {}
+                // If the value is a feature for one of the dependencies, bail out if no such
+                // dependency is actually defined in the manifest.
+                (&CrateFeature(ref dep, _), false, _) => bail!(
                     "Feature `{}` requires a feature of `{}` which is not a \
                      dependency",
                     feature,
-                    dep_name
-                ),
-                (&Crate(ref dep), None) => bail!(
-                    "Feature `{}` includes `{}` which is neither \
-                     a dependency nor another feature",
-                    feature,
                     dep
                 ),
-                (&CrateFeature(_, _), Some(_)) | (&Feature(_), _) => {}
+                (&CrateFeature(_, _), true, _) => {}
             }
             values.push(val);
         }
+
+        if !dependency_found {
+            // If we have not found the dependency of the same-named feature, we should
+            // bail here.
+            bail!(
+                "Feature `{}` includes the optional dependency of the \
+                 same name, but this is left implicit in the features \
+                 included by this feature.\nConsider adding \
+                 `crate:{}` to this feature's requirements.",
+                feature,
+                feature
+            )
+        }
+
         map.insert(feature.clone(), values);
     }
     Ok(map)
@@ -200,30 +330,42 @@ pub enum FeatureValue {
 }
 
 impl FeatureValue {
-    fn build<T>(feature: InternedString, is_feature: T) -> FeatureValue
+    fn build<T>(feature: InternedString, is_feature: T, namespaced: bool) -> FeatureValue
     where
         T: Fn(&str) -> bool,
     {
-        match feature.find('/') {
-            Some(pos) => {
+        match (feature.find('/'), namespaced) {
+            (Some(pos), _) => {
                 let (dep, dep_feat) = feature.split_at(pos);
                 let dep_feat = &dep_feat[1..];
                 FeatureValue::CrateFeature(InternedString::new(dep), InternedString::new(dep_feat))
             }
-            None if is_feature(&feature) => FeatureValue::Feature(feature),
-            None => FeatureValue::Crate(feature),
+            (None, true) if feature.starts_with("crate:") => {
+                FeatureValue::Crate(InternedString::new(&feature[6..]))
+            }
+            (None, true) => FeatureValue::Feature(feature),
+            (None, false) if is_feature(&feature) => FeatureValue::Feature(feature),
+            (None, false) => FeatureValue::Crate(feature),
         }
     }
 
     pub fn new(feature: InternedString, s: &Summary) -> FeatureValue {
-        Self::build(feature, |fs| s.features().contains_key(fs))
+        Self::build(
+            feature,
+            |fs| s.features().contains_key(fs),
+            s.namespaced_features(),
+        )
     }
 
-    pub fn to_string(&self) -> String {
+    pub fn to_string(&self, s: &Summary) -> String {
         use self::FeatureValue::*;
         match *self {
             Feature(ref f) => f.to_string(),
-            Crate(ref c) => c.to_string(),
+            Crate(ref c) => if s.namespaced_features() {
+                format!("crate:{}", &c)
+            } else {
+                c.to_string()
+            },
             CrateFeature(ref c, ref f) => [c.as_ref(), f.as_ref()].join("/"),
         }
     }

src/cargo/ops/registry.rs

@@ -214,13 +214,14 @@ fn transmit(
         return Ok(());
     }
 
-    let string_features = pkg.summary()
+    let summary = pkg.summary();
+    let string_features = summary
         .features()
         .iter()
         .map(|(feat, values)| {
             (
                 feat.clone(),
-                values.iter().map(|fv| fv.to_string()).collect(),
+                values.iter().map(|fv| fv.to_string(&summary)).collect(),
             )
         })
         .collect::<BTreeMap<String, Vec<String>>>();

src/cargo/sources/registry/index.rs

@@ -153,7 +153,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         let deps = deps.into_iter()
             .map(|dep| dep.into_dep(&self.source_id))
             .collect::<CargoResult<Vec<_>>>()?;
-        let summary = Summary::new(pkgid, deps, features, links)?;
+        let summary = Summary::new(pkgid, deps, features, links, false)?;
         let summary = summary.set_checksum(cksum.clone());
         if self.hashes.contains_key(&name[..]) {
             self.hashes.get_mut(&name[..]).unwrap().insert(vers, cksum);

src/cargo/util/toml/mod.rs

@@ -543,6 +543,8 @@ pub struct TomlProject {
     autoexamples: Option<bool>,
     autotests: Option<bool>,
     autobenches: Option<bool>,
+    #[serde(rename = "namespaced-features")]
+    namespaced_features: Option<bool>,
 
     // package metadata
     description: Option<String>,
@@ -837,12 +839,16 @@ impl TomlManifest {
 
         let exclude = project.exclude.clone().unwrap_or_default();
         let include = project.include.clone().unwrap_or_default();
+        if project.namespaced_features.is_some() {
+            features.require(Feature::namespaced_features())?;
+        }
 
         let summary = Summary::new(
             pkgid,
             deps,
             me.features.clone().unwrap_or_else(BTreeMap::new),
             project.links.clone(),
+            project.namespaced_features.unwrap_or(false),
         )?;
         let metadata = ManifestMetadata {
             description: project.description.clone(),