Use enum as the representation of cargo feature

taiki-e · Dec 3, 2020 · 41f9f11 · 41f9f11
1 parent 9e52b0e
commit 41f9f11
Show file tree

Hide file tree

Showing 5 changed files with 180 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,8 @@ This project adheres to [Semantic Versioning](https://semver.org).
 
 ## [Unreleased]
 
+* [Fix an issue that feature combinations exclusion does not work properly when used with `--group-features`.](https://github.com/taiki-e/cargo-hack/pull/99)
+
 ## [0.4.7] - 2020-12-03
 
 No public API changes from 0.4.6.

diff --git a/src/cli.rs b/src/cli.rs
@@ -1,7 +1,7 @@
 use anyhow::{bail, format_err, Error};
 use std::{env, fmt, mem};
 
-use crate::{term, Cargo, Result};
+use crate::{term, Cargo, Feature, Result};
 
 fn print_version() {
     println!("{0} {1}", env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION"))
@@ -213,7 +213,7 @@ pub(crate) struct Args<'a> {
     /// --optional-deps [DEPS]...
     pub(crate) optional_deps: Option<Vec<&'a str>>,
     /// --include-features
-    pub(crate) include_features: Vec<&'a str>,
+    pub(crate) include_features: Vec<Feature>,
     /// --include-deps-features
     pub(crate) include_deps_features: bool,
 
@@ -230,7 +230,7 @@ pub(crate) struct Args<'a> {
     /// --depth <NUM>
     pub(crate) depth: Option<usize>,
     /// --group-features <FEATURES>...
-    pub(crate) group_features: Vec<(Vec<&'a str>, String)>,
+    pub(crate) group_features: Vec<Feature>,
 
     /// --no-default-features
     pub(crate) no_default_features: bool,
@@ -560,18 +560,17 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo) -> Result<Args<'a>
     let depth = depth.map(str::parse::<usize>).transpose()?;
     let group_features =
         group_features.iter().try_fold(Vec::with_capacity(group_features.len()), |mut v, g| {
-            let g: Vec<_> = if g.contains(',') {
-                g.split(',').collect()
+            let g = if g.contains(',') {
+                g.split(',')
             } else if g.contains(' ') {
-                g.split(' ').collect()
+                g.split(' ')
             } else {
                 bail!(
                     "--group-features requires a list of two or more features separated by space \
                      or comma"
                 );
             };
-            let s = g.join(",");
-            v.push((g, s));
+            v.push(Feature::group(g));
             Ok(v)
         })?;
 
@@ -673,7 +672,7 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo) -> Result<Args<'a>
         if optional_deps.as_ref().map_or(false, |d| d.contains(f)) {
             warn!("feature `{}` specified by both --exclude-features and --optional-deps", f);
         }
-        if group_features.iter().any(|(v, _)| v.contains(f)) {
+        if group_features.iter().any(|v| v.matches(f)) {
             warn!("feature `{}` specified by both --exclude-features and --group-features", f);
         }
         if include_features.contains(f) {
@@ -703,7 +702,7 @@ pub(crate) fn parse_args<'a>(raw: &'a RawArgs, cargo: &Cargo) -> Result<Args<'a>
         ignore_unknown_features,
         optional_deps,
         clean_per_run,
-        include_features,
+        include_features: include_features.into_iter().map(Into::into).collect(),
         include_deps_features,
 
         depth,

diff --git a/src/features.rs b/src/features.rs
@@ -1,12 +1,15 @@
-use std::collections::{BTreeMap, BTreeSet};
+use std::{
+    collections::{BTreeMap, BTreeSet},
+    slice,
+};
 
 use crate::{
     metadata::{Dependency, Metadata},
     PackageId,
 };
 
 pub(crate) struct Features {
-    features: Vec<String>,
+    features: Vec<Feature>,
     /// [package features len, package features + optional deps len]
     len: [usize; 2],
 }
@@ -18,11 +21,11 @@ impl Features {
 
         let mut features = Vec::with_capacity(package.features.len());
 
-        for name in package.features.keys().cloned() {
-            features.push(name);
+        for name in package.features.keys() {
+            features.push(name.into());
         }
         for name in package.dependencies.iter().filter_map(Dependency::as_feature) {
-            features.push(name.to_string());
+            features.push(name.into());
         }
         let len = [package.features.len(), features.len()];
 
@@ -37,23 +40,23 @@ impl Features {
             // so I'm not sure if there is a way to find the actual feature name exactly.
             if let Some(d) = package.dependencies.iter().find(|d| d.name == dep_package.name) {
                 let name = d.rename.as_ref().unwrap_or(&d.name);
-                features.extend(dep_package.features.keys().map(|f| format!("{}/{}", name, f)));
+                features.extend(dep_package.features.keys().map(|f| Feature::path(name, f)));
             }
             // TODO: Optional deps of `dep_package`.
         }
 
         Self { features, len }
     }
 
-    pub(crate) fn normal(&self) -> &[String] {
+    pub(crate) fn normal(&self) -> &[Feature] {
         &self.features[..self.len[0]]
     }
 
-    pub(crate) fn optional_deps(&self) -> &[String] {
+    pub(crate) fn optional_deps(&self) -> &[Feature] {
         &self.features[self.len[0]..self.len[1]]
     }
 
-    pub(crate) fn deps_features(&self) -> &[String] {
+    pub(crate) fn deps_features(&self) -> &[Feature] {
         &self.features[self.len[1]..]
     }
 
@@ -62,17 +65,92 @@ impl Features {
     }
 }
 
+/// The representation of Cargo feature.
+#[derive(Debug)]
+pub(crate) enum Feature {
+    /// A feature of the current crate.
+    Normal {
+        /// Feature name. It is considered indivisible.
+        name: String,
+    },
+    /// Grouped features.
+    Group {
+        /// Feature name concatenated with `,`.
+        name: String,
+        /// Original feature list.
+        list: Vec<String>,
+    },
+    /// A feature of a dependency.
+    Path {
+        /// Feature path separated with `/`.
+        name: String,
+        /// Index of `/`.
+        slash: usize,
+    },
+}
+
+impl Feature {
+    pub(crate) fn group(group: impl IntoIterator<Item = impl Into<String>>) -> Self {
+        let list: Vec<_> = group.into_iter().map(Into::into).collect();
+        Feature::Group { name: list.join(","), list }
+    }
+
+    pub(crate) fn path(parent: &str, name: &str) -> Self {
+        Feature::Path { name: format!("{}/{}", parent, name), slash: parent.len() }
+    }
+
+    pub(crate) fn name(&self) -> &str {
+        match self {
+            Feature::Normal { name } | Feature::Group { name, .. } | Feature::Path { name, .. } => {
+                name
+            }
+        }
+    }
+
+    pub(crate) fn as_group(&self) -> &[String] {
+        match self {
+            Feature::Group { list, .. } => list,
+            Feature::Normal { name } | Feature::Path { name, .. } => slice::from_ref(name),
+        }
+    }
+
+    pub(crate) fn matches(&self, s: &str) -> bool {
+        self.as_group().iter().any(|n| **n == *s)
+    }
+}
+
+impl PartialEq<str> for Feature {
+    fn eq(&self, other: &str) -> bool {
+        self.name() == other
+    }
+}
+
+impl<S: Into<String>> From<S> for Feature {
+    fn from(name: S) -> Self {
+        Feature::Normal { name: name.into() }
+    }
+}
+
+impl AsRef<str> for Feature {
+    fn as_ref(&self) -> &str {
+        self.name()
+    }
+}
+
 pub(crate) fn feature_powerset<'a>(
-    features: impl IntoIterator<Item = &'a str>,
+    features: impl IntoIterator<Item = &'a Feature>,
     depth: Option<usize>,
     map: &BTreeMap<String, Vec<String>>,
-) -> Vec<Vec<&'a str>> {
-    let feature_deps = feature_deps(map);
-    let powerset = powerset(features, depth);
-    powerset
+) -> Vec<Vec<&'a Feature>> {
+    let deps_map = feature_deps(map);
+    powerset(features, depth)
         .into_iter()
-        .filter(|a| {
-            !a.iter().filter_map(|b| feature_deps.get(b)).any(|c| a.iter().any(|d| c.contains(d)))
+        .filter(|fs| {
+            !fs.iter().any(|f| {
+                f.as_group().iter().filter_map(|f| deps_map.get(&&**f)).any(|deps| {
+                    fs.iter().any(|f| f.as_group().iter().all(|f| deps.contains(&&**f)))
+                })
+            })
         })
         .collect()
 }
@@ -101,10 +179,10 @@ fn feature_deps(map: &BTreeMap<String, Vec<String>>) -> BTreeMap<&str, BTreeSet<
     feat_deps
 }
 
-fn powerset<T: Clone>(iter: impl IntoIterator<Item = T>, depth: Option<usize>) -> Vec<Vec<T>> {
+fn powerset<T: Copy>(iter: impl IntoIterator<Item = T>, depth: Option<usize>) -> Vec<Vec<T>> {
     iter.into_iter().fold(vec![vec![]], |mut acc, elem| {
         let ext = acc.clone().into_iter().map(|mut curr| {
-            curr.push(elem.clone());
+            curr.push(elem);
             curr
         });
         if let Some(depth) = depth {
@@ -118,7 +196,7 @@ fn powerset<T: Clone>(iter: impl IntoIterator<Item = T>, depth: Option<usize>) -
 
 #[cfg(test)]
 mod tests {
-    use super::{feature_deps, feature_powerset, powerset};
+    use super::{feature_deps, feature_powerset, powerset, Feature};
     use std::{
         collections::{BTreeMap, BTreeSet},
         iter::FromIterator,
@@ -153,8 +231,8 @@ mod tests {
             ("c", set!["a", "b"]),
             ("d", set!["a", "b"])
         ]);
-        let list = vec!["a", "b", "c", "d"];
-        let ps = powerset(list.clone(), None);
+        let list: Vec<Feature> = svec!["a", "b", "c", "d"];
+        let ps = powerset(list.iter().collect::<Vec<_>>(), None);
         assert_eq!(ps, vec![
             vec![],
             vec!["a"],
@@ -173,7 +251,7 @@ mod tests {
             vec!["b", "c", "d"],
             vec!["a", "b", "c", "d"],
         ]);
-        let filtered = feature_powerset(list, None, &map);
+        let filtered = feature_powerset(list.iter().collect::<Vec<_>>(), None, &map);
         assert_eq!(filtered, vec![vec![], vec!["a"], vec!["b"], vec!["c"], vec!["d"], vec![
             "c", "d"
         ]]);

diff --git a/src/main.rs b/src/main.rs
@@ -28,7 +28,8 @@ use anyhow::{bail, Context as _};
 use std::{fmt::Write, fs};
 
 use crate::{
-    cargo::Cargo, context::Context, metadata::PackageId, process::ProcessBuilder, restore::Restore,
+    cargo::Cargo, context::Context, features::Feature, metadata::PackageId,
+    process::ProcessBuilder, restore::Restore,
 };
 
 type Result<T, E = anyhow::Error> = std::result::Result<T, E>;
@@ -76,8 +77,8 @@ enum Kind<'a> {
     NoSubcommand,
     SkipAsPrivate,
     Nomal,
-    Each { features: Vec<&'a str> },
-    Powerset { features: Vec<Vec<&'a str>> },
+    Each { features: Vec<&'a Feature> },
+    Powerset { features: Vec<Vec<&'a Feature>> },
 }
 
 fn determine_kind<'a>(cx: &'a Context<'_>, id: &PackageId, progress: &mut Progress) -> Kind<'a> {
@@ -93,8 +94,9 @@ fn determine_kind<'a>(cx: &'a Context<'_>, id: &PackageId, progress: &mut Progre
     }
 
     let package = cx.packages(id);
-    let filter = |f: &&str| {
-        !cx.exclude_features.contains(f) && !cx.group_features.iter().any(|(g, _)| g.contains(f))
+    let filter = |&f: &&Feature| {
+        !cx.exclude_features.iter().any(|s| f == *s)
+            && !cx.group_features.iter().any(|g| g.matches(f.as_ref()))
     };
     let features = if cx.include_features.is_empty() {
         let feature_list = cx.pkg_features(id);
@@ -105,8 +107,7 @@ fn determine_kind<'a>(cx: &'a Context<'_>, id: &PackageId, progress: &mut Progre
             }
         });
 
-        let mut features: Vec<_> =
-            feature_list.normal().iter().map(String::as_str).filter(filter).collect();
+        let mut features: Vec<_> = feature_list.normal().iter().filter(filter).collect();
 
         if let Some(opt_deps) = &cx.optional_deps {
             opt_deps.iter().for_each(|&d| {
@@ -118,26 +119,22 @@ fn determine_kind<'a>(cx: &'a Context<'_>, id: &PackageId, progress: &mut Progre
                 }
             });
 
-            features.extend(
-                feature_list
-                    .optional_deps()
-                    .iter()
-                    .map(String::as_str)
-                    .filter(|f| filter(f) && (opt_deps.is_empty() || opt_deps.contains(f))),
-            );
+            features.extend(feature_list.optional_deps().iter().filter(|f| {
+                filter(f) && (opt_deps.is_empty() || opt_deps.iter().any(|x| *f == *x))
+            }));
         }
 
         if cx.include_deps_features {
-            features.extend(feature_list.deps_features().iter().map(String::as_str).filter(filter));
+            features.extend(feature_list.deps_features().iter().filter(filter));
         }
 
         if !cx.group_features.is_empty() {
-            features.extend(cx.group_features.iter().map(|(_, s)| &**s));
+            features.extend(cx.group_features.iter());
         }
 
         features
     } else {
-        cx.include_features.iter().copied().filter(filter).collect()
+        cx.include_features.iter().filter(filter).collect()
     };
 
     if cx.each_feature {