Auto merge of #5270 - djc:feature-requirements, r=Eh2406

Introduce FeatureValue type to represent features table values

This is the next step towards #1286 (after #5258). The goal here is to have a central place in the code where feature strings are interpreted as (a) a feature, (b) a dependency or (c) a dependency/feature combo, and anchor that interpretation in the type system as an enum.

I've spent quite a bit of effort avoiding extra string allocations, complicating the code a bit; notice in particular the use of `Cow<str>` in `FeatureValue` variants, and the slight workaround in `Context::resolve_features()` and `build_requirements()`. I hope this is all okay.

cc @Eh2406

Author: bors

src/cargo/core/interning.rs

@@ -1,3 +1,5 @@
+use serde::{Serialize, Serializer};
+
 use std::fmt;
 use std::sync::RwLock;
 use std::collections::HashSet;
@@ -95,3 +97,12 @@ impl PartialOrd for InternedString {
         Some(self.cmp(other))
     }
 }
+
+impl Serialize for InternedString {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        serializer.serialize_str(self.inner)
+    }
+}

src/cargo/core/mod.rs

@@ -9,7 +9,7 @@ pub use self::registry::Registry;
 pub use self::resolver::Resolve;
 pub use self::shell::{Shell, Verbosity};
 pub use self::source::{GitReference, Source, SourceId, SourceMap};
-pub use self::summary::Summary;
+pub use self::summary::{FeatureMap, FeatureValue, Summary};
 pub use self::workspace::{Members, Workspace, WorkspaceConfig, WorkspaceRootConfig};
 
 pub mod source;

src/cargo/core/package.rs

@@ -1,5 +1,5 @@
 use std::cell::{Ref, RefCell};
-use std::collections::{BTreeMap, HashMap};
+use std::collections::HashMap;
 use std::fmt;
 use std::hash;
 use std::path::{Path, PathBuf};
@@ -10,7 +10,7 @@ use toml;
 use lazycell::LazyCell;
 
 use core::{Dependency, Manifest, PackageId, SourceId, Target};
-use core::{SourceMap, Summary};
+use core::{FeatureMap, SourceMap, Summary};
 use core::interning::InternedString;
 use util::{internal, lev_distance, Config};
 use util::errors::{CargoResult, CargoResultExt};
@@ -39,7 +39,7 @@ struct SerializedPackage<'a> {
     source: &'a SourceId,
     dependencies: &'a [Dependency],
     targets: &'a [Target],
-    features: &'a BTreeMap<String, Vec<String>>,
+    features: &'a FeatureMap,
     manifest_path: &'a str,
 }
 

src/cargo/core/resolver/context.rs

@@ -1,7 +1,7 @@
 use std::collections::{HashMap, HashSet};
 use std::rc::Rc;
 
-use core::{Dependency, PackageId, SourceId, Summary};
+use core::{Dependency, FeatureValue, PackageId, SourceId, Summary};
 use core::interning::InternedString;
 use util::Graph;
 use util::CargoResult;
@@ -170,7 +170,24 @@ impl Context {
         let deps = s.dependencies();
         let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);
 
-        let mut reqs = build_requirements(s, method)?;
+        // Requested features stored in the Method are stored as string references, but we want to
+        // transform them into FeatureValues here. In order to pass the borrow checker with
+        // storage of the FeatureValues that outlives the Requirements object, we do the
+        // transformation here, and pass the FeatureValues to build_requirements().
+        let values = if let Method::Required {
+            all_features: false,
+            features: requested,
+            ..
+        } = *method
+        {
+            requested
+                .iter()
+                .map(|f| FeatureValue::new(f, s))
+                .collect::<Vec<FeatureValue>>()
+        } else {
+            vec![]
+        };
+        let mut reqs = build_requirements(s, method, &values)?;
         let mut ret = Vec::new();
 
         // Next, collect all actually enabled dependencies and their features.
@@ -269,8 +286,12 @@ impl Context {
 fn build_requirements<'a, 'b: 'a>(
     s: &'a Summary,
     method: &'b Method,
+    requested: &'a [FeatureValue],
 ) -> CargoResult<Requirements<'a>> {
     let mut reqs = Requirements::new(s);
+    for fv in requested.iter() {
+        reqs.require_value(fv)?;
+    }
     match *method {
         Method::Everything
         | Method::Required {
@@ -283,12 +304,7 @@ fn build_requirements<'a, 'b: 'a>(
                 reqs.require_dependency(dep.name().as_str());
             }
         }
-        Method::Required {
-            features: requested_features,
-            ..
-        } => for feat in requested_features.iter() {
-            reqs.add_feature(feat)?;
-        },
+        _ => {} // Explicitly requested features are handled through `requested`
     }
     match *method {
         Method::Everything
@@ -359,50 +375,34 @@ impl<'r> Requirements<'r> {
     }
 
     fn require_feature(&mut self, feat: &'r str) -> CargoResult<()> {
-        if self.seen(feat) {
+        if feat.is_empty() || self.seen(feat) {
             return Ok(());
         }
-        for f in self.summary
+        for fv in self.summary
             .features()
             .get(feat)
             .expect("must be a valid feature")
         {
-            if f == feat {
-                bail!(
+            match *fv {
+                FeatureValue::Feature(ref dep_feat) if **dep_feat == *feat => bail!(
                     "Cyclic feature dependency: feature `{}` depends on itself",
                     feat
-                );
+                ),
+                _ => {}
             }
-            self.add_feature(f)?;
+            self.require_value(&fv)?;
         }
         Ok(())
     }
 
-    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
-        // `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(2, '/');
-        let feat_or_package = parts.next().unwrap();
-        match parts.next() {
-            Some(feat) => {
-                self.require_crate_feature(feat_or_package, feat);
-            }
-            None => {
-                if self.summary.features().contains_key(feat_or_package) {
-                    self.require_feature(feat_or_package)?;
-                } else {
-                    self.require_dependency(feat_or_package);
-                }
+    fn require_value(&mut self, fv: &'r FeatureValue) -> CargoResult<()> {
+        match *fv {
+            FeatureValue::Feature(ref feat) => self.require_feature(feat),
+            FeatureValue::Crate(ref dep) => Ok(self.require_dependency(dep)),
+            FeatureValue::CrateFeature(ref dep, ref dep_feat) => {
+                Ok(self.require_crate_feature(dep, dep_feat))
             }
         }
-        Ok(())
     }
 }
 

src/cargo/core/summary.rs

@@ -21,7 +21,7 @@ pub struct Summary {
 struct Inner {
     package_id: PackageId,
     dependencies: Vec<Dependency>,
-    features: BTreeMap<String, Vec<String>>,
+    features: FeatureMap,
     checksum: Option<String>,
     links: Option<InternedString>,
 }
@@ -48,47 +48,12 @@ impl Summary {
                 )
             }
         }
-        for (feature, list) in features.iter() {
-            for dep in list.iter() {
-                let mut parts = dep.splitn(2, '/');
-                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;
-                        }
-                        bail!(
-                            "Feature `{}` depends on `{}` which is not an \
-                             optional dependency.\nConsider adding \
-                             `optional = true` to the dependency",
-                            feature,
-                            dep
-                        )
-                    }
-                    None if is_reexport => bail!(
-                        "Feature `{}` requires a feature of `{}` which is not a \
-                         dependency",
-                        feature,
-                        dep
-                    ),
-                    None => bail!(
-                        "Feature `{}` includes `{}` which is neither \
-                         a dependency nor another feature",
-                        feature,
-                        dep
-                    ),
-                }
-            }
-        }
+        let feature_map = build_feature_map(features, &dependencies)?;
         Ok(Summary {
             inner: Rc::new(Inner {
                 package_id: pkg_id,
                 dependencies,
-                features,
+                features: feature_map,
                 checksum: None,
                 links: links.map(|l| InternedString::new(&l)),
             }),
@@ -110,7 +75,7 @@ impl Summary {
     pub fn dependencies(&self) -> &[Dependency] {
         &self.inner.dependencies
     }
-    pub fn features(&self) -> &BTreeMap<String, Vec<String>> {
+    pub fn features(&self) -> &FeatureMap {
         &self.inner.features
     }
     pub fn checksum(&self) -> Option<&str> {
@@ -158,3 +123,108 @@ impl PartialEq for Summary {
         self.inner.package_id == other.inner.package_id
     }
 }
+
+// Checks features for errors, bailing out a CargoResult:Err if invalid,
+// and creates FeatureValues for each feature.
+fn build_feature_map(
+    features: BTreeMap<String, Vec<String>>,
+    dependencies: &Vec<Dependency>,
+) -> CargoResult<FeatureMap> {
+    use self::FeatureValue::*;
+    let mut map = BTreeMap::new();
+    for (feature, list) in features.iter() {
+        let mut values = vec![];
+        for dep in list {
+            let val = FeatureValue::build(dep, |fs| (&features).get(fs).is_some());
+            if let &Feature(_) = &val {
+                values.push(val);
+                continue;
+            }
+
+            // 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, 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
+                        )
+                    }
+                }
+                (&CrateFeature(ref dep_name, _), None) => 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(_), _) => {}
+            }
+            values.push(val);
+        }
+        map.insert(feature.clone(), values);
+    }
+    Ok(map)
+}
+
+/// FeatureValue represents the types of dependencies a feature can have:
+///
+/// * Another feature
+/// * An optional dependency
+/// * A feature in a depedency
+///
+/// The selection between these 3 things happens as part of the construction of the FeatureValue.
+#[derive(Clone, Debug, Serialize)]
+pub enum FeatureValue {
+    Feature(InternedString),
+    Crate(InternedString),
+    CrateFeature(InternedString, InternedString),
+}
+
+impl FeatureValue {
+    fn build<T>(feature: &str, is_feature: T) -> FeatureValue
+    where
+        T: Fn(&str) -> bool,
+    {
+        match feature.find('/') {
+            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(InternedString::new(feature)),
+            None => FeatureValue::Crate(InternedString::new(feature)),
+        }
+    }
+
+    pub fn new(feature: &str, s: &Summary) -> FeatureValue {
+        Self::build(feature, |fs| s.features().get(fs).is_some())
+    }
+
+    pub fn to_string(&self) -> String {
+        use self::FeatureValue::*;
+        match *self {
+            Feature(ref f) => f.to_string(),
+            Crate(ref c) => c.to_string(),
+            CrateFeature(ref c, ref f) => [c.as_ref(), f.as_ref()].join("/"),
+        }
+    }
+}
+
+pub type FeatureMap = BTreeMap<String, Vec<FeatureValue>>;

src/cargo/ops/registry.rs

@@ -1,4 +1,5 @@
 use std::{cmp, env};
+use std::collections::BTreeMap;
 use std::fs::{self, File};
 use std::iter::repeat;
 use std::time::Duration;
@@ -213,12 +214,23 @@ fn transmit(
         return Ok(());
     }
 
+    let string_features = pkg.summary()
+        .features()
+        .iter()
+        .map(|(feat, values)| {
+            (
+                feat.clone(),
+                values.iter().map(|fv| fv.to_string()).collect(),
+            )
+        })
+        .collect::<BTreeMap<String, Vec<String>>>();
+
     let publish = registry.publish(
         &NewCrate {
             name: pkg.name().to_string(),
             vers: pkg.version().to_string(),
             deps,
-            features: pkg.summary().features().clone(),
+            features: string_features,
             authors: authors.clone(),
             description: description.clone(),
             homepage: homepage.clone(),

tests/testsuite/metadata.rs

@@ -192,7 +192,9 @@ optional_feat = []
                 ],
                 "features": {
                   "default": [
-                    "default_feat"
+                    {
+                       "Feature": "default_feat"
+                    }
                   ],
                   "default_feat": [],
                   "optional_feat": []