Take feature namespace into account while building summary (fixes #1286)

Here's an attempt at a table to cover the different cases:

Feature
    Old (must be in features table)
        Continue
    Namespaced (might be stray value)
        In features table: Check that Crate dependency is in the list
        -> Non-optional dependency: Bail [PREVIOUSLY: bailed for non-optional dependency]
        -> Optional dependency: Insert feature of this name
        -> Else: Bail [PREVIOUSLY: bailed for unknown dependency or feature]

Crate
    Old (might be stray value)
        Non-optional dependency: Bail
        No dependency found: Bail
    Namespaced
        Non-optional dependency: Bail
        No dependency found: Bail

CrateFeature
    Old
        No dependency found: Bail
    Namespaced
        No dependency found: Bail

Author: djc

src/cargo/core/summary.rs

@@ -38,7 +38,7 @@ impl Summary {
         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: `{}`",
@@ -52,7 +52,7 @@ 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,
@@ -137,6 +137,7 @@ 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
@@ -146,52 +147,169 @@ fn build_feature_map(
 
     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 = {
                 match val {
-                    Feature(_) => None,
-                    Crate(ref dep_name) | CrateFeature(ref dep_name, _) => {
+                    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)]);
                     }
                 }
-                (&CrateFeature(ref dep_name, _), None) => bail!(
+                // 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
+                            )
+                        }
+                    }
+                }
+                // 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)
@@ -212,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>>>();