Repository: taiki-e/cargo-hack (language: rust)

Issue:
"dep:dependency" features are not running with `--feature-powerset` and without `--optional-deps`
I have a `Cargo.toml` which basically contains
```toml
[dependencies]
crate_a = { version = "1.33.7", optional = true }
crate_b = { version = "1.33.8" }
crate_c = { version = "1.33.9", optional = true }

[features]
crate_a = ["dep:crate_a", "crate_b/crate_a"]
great_feature = ["dep:crate_c"]
```

- `--feature-powerset` will not run for `--features crate_a`
- `--feature-powerset --optional-deps`, works with `crate_a` but also tries to run `crate_c` which fails because a feature with "dep:crate_c" is specified

Do you see this being fixed somehow? I try to avoid specifying the optional dependencies myself. The optimal solution would probably be that `--feature-powerset` will "simply" pick up `crate-a` as well as it's defined int he `[features]` section.

Without looking at the code I guess that you read all features and exclude them if `--optional-deps` is not specified and the crate is also an optional dependency?



Discussion / hints:
Thanks for the report!

> Without looking at the code I guess that you read all features and exclude them if `--optional-deps` is not specified and the crate is also an optional dependency?

Yes. We need to fix this filter:

https://github.com/taiki-e/cargo-hack/blob/202e6e59d491c9202ce148c9ef423853267226db/src/features.rs#L26-L30

(This incomplete filter was originally added to fix breakage after namespaced and weak dependency features stabilization changed the `cargo metadata` and [caused breakage in some projects](https://github.com/taiki-e/cargo-hack/issues/145).)

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cde74c00..2ed588e6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,8 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com
 
 ## [Unreleased]
 
+- Support namespaced features (features with `dep:` prefix). ([#154](https://github.com/taiki-e/cargo-hack/pull/154))
+
 - Add metadata for cargo binstall.
 
 ## [0.5.14] - 2022-06-02
diff --git a/src/context.rs b/src/context.rs
index cc2293fe..b4e0bac0 100644
--- a/src/context.rs
+++ b/src/context.rs
@@ -46,16 +46,21 @@ impl Context {
             bail!("--include-deps-features requires Cargo 1.41 or later");
         }
 
+        let mut manifests = HashMap::with_capacity(metadata.workspace_members.len());
         let mut pkg_features = HashMap::with_capacity(metadata.workspace_members.len());
+
         for id in &metadata.workspace_members {
-            let features = Features::new(&metadata, id);
+            let manifest_path = &metadata.packages[id].manifest_path;
+            let manifest = Manifest::new(manifest_path)?;
+            let features = Features::new(&metadata, &manifest, id);
+            manifests.insert(id.clone(), manifest);
             pkg_features.insert(id.clone(), features);
         }
 
         let mut this = Self {
             args,
             metadata,
-            manifests: HashMap::new(),
+            manifests,
             pkg_features,
             cargo: cargo.into(),
             restore,
@@ -63,17 +68,6 @@ impl Context {
             version_range: None,
         };
 
-        // Only a few options require information from cargo manifest.
-        // If manifest information is not required, do not read and parse them.
-        if this.require_manifest_info() {
-            this.manifests.reserve(this.metadata.workspace_members.len());
-            for id in &this.metadata.workspace_members {
-                let manifest_path = &this.metadata.packages[id].manifest_path;
-                let manifest = Manifest::new(manifest_path)?;
-                this.manifests.insert(id.clone(), manifest);
-            }
-        }
-
         this.version_range = this
             .args
             .version_range
@@ -103,7 +97,6 @@ impl Context {
     }
 
     pub(crate) fn manifests(&self, id: &PackageId) -> &Manifest {
-        debug_assert!(self.require_manifest_info());
         &self.manifests[id]
     }
 
@@ -140,14 +133,6 @@ impl Context {
         }
     }
 
-    /// Return `true` if options that require information from cargo manifest is specified.
-    pub(crate) fn require_manifest_info(&self) -> bool {
-        (self.metadata.cargo_version < 39 && self.ignore_private)
-            || (self.metadata.cargo_version < 58 && self.args.version_range.is_some())
-            || self.no_dev_deps
-            || self.remove_dev_deps
-    }
-
     pub(crate) fn cargo(&self) -> ProcessBuilder<'_> {
         cmd!(&self.cargo)
     }
diff --git a/src/features.rs b/src/features.rs
index 9243a3f3..485c879b 100644
--- a/src/features.rs
+++ b/src/features.rs
@@ -3,7 +3,7 @@ use std::{
     slice,
 };
 
-use crate::{metadata::Metadata, PackageId};
+use crate::{manifest::Manifest, metadata::Metadata, PackageId};
 
 #[derive(Debug)]
 pub(crate) struct Features {
@@ -13,15 +13,26 @@ pub(crate) struct Features {
 }
 
 impl Features {
-    pub(crate) fn new(metadata: &Metadata, id: &PackageId) -> Self {
+    pub(crate) fn new(metadata: &Metadata, manifest: &Manifest, id: &PackageId) -> Self {
         let package = &metadata.packages[id];
         let node = &metadata.resolve.nodes[id];
 
         let mut features = Vec::with_capacity(package.features.len());
         let mut optional_deps = vec![];
+        let mut namespaced_features = vec![]; // features with `dep:` prefix
 
+        // package.features.values() does not provide a way to determine the `dep:` specified by the user.
+        for names in manifest.features.values() {
+            for name in names {
+                if let Some(name) = name.strip_prefix("dep:") {
+                    namespaced_features.push(name);
+                }
+            }
+        }
         for name in package.optional_deps() {
-            optional_deps.push(name);
+            if !namespaced_features.contains(&name) {
+                optional_deps.push(name);
+            }
         }
         for name in package.features.keys() {
             if !optional_deps.contains(&&**name) {
diff --git a/src/manifest.rs b/src/manifest.rs
index ad87fb71..9d3eb66f 100644
--- a/src/manifest.rs
+++ b/src/manifest.rs
@@ -1,4 +1,4 @@
-use std::path::Path;
+use std::{collections::BTreeMap, path::Path};
 
 use anyhow::{format_err, Context as _, Result};
 
@@ -12,6 +12,7 @@ pub(crate) struct Manifest {
     pub(crate) raw: String,
     pub(crate) doc: toml_edit::Document,
     pub(crate) package: Package,
+    pub(crate) features: BTreeMap<String, Vec<String>>,
 }
 
 impl Manifest {
@@ -23,7 +24,10 @@ impl Manifest {
         let package = Package::from_table(&doc).map_err(|s| {
             format_err!("failed to parse `{}` field from manifest `{}`", s, path.display())
         })?;
-        Ok(Self { raw, doc, package })
+        let features = Features::from_table(&doc).map_err(|s| {
+            format_err!("failed to parse `{}` field from manifest `{}`", s, path.display())
+        })?;
+        Ok(Self { raw, doc, package, features })
     }
 
     pub(crate) fn remove_dev_deps(&self) -> String {
@@ -62,6 +66,31 @@ impl Package {
     }
 }
 
+struct Features {}
+
+impl Features {
+    fn from_table(doc: &toml_edit::Document) -> ParseResult<BTreeMap<String, Vec<String>>> {
+        let features = match doc.get("features") {
+            Some(features) => features.as_table().ok_or("features")?,
+            None => return Ok(BTreeMap::new()),
+        };
+        let mut res = BTreeMap::new();
+        for (name, values) in features {
+            res.insert(
+                name.to_owned(),
+                values
+                    .as_array()
+                    .ok_or("features")?
+                    .into_iter()
+                    .flat_map(toml_edit::Value::as_str)
+                    .map(str::to_owned)
+                    .collect(),
+            );
+        }
+        Ok(res)
+    }
+}
+
 fn remove_dev_deps(doc: &mut toml_edit::Document) {
     const KEY: &str = "dev-dependencies";
     let table = doc.as_table_mut();


Test patch:
diff --git a/tests/fixtures/namespaced_features/Cargo.toml b/tests/fixtures/namespaced_features/Cargo.toml
new file mode 100644
index 00000000..ad9cea88
--- /dev/null
+++ b/tests/fixtures/namespaced_features/Cargo.toml
@@ -0,0 +1,16 @@
+[package]
+name = "namespaced_features"
+version = "0.0.0"
+edition = "2021"
+publish = false
+
+[workspace]
+resolver = "2"
+
+[features]
+easytime = ["dep:easytime"]
+
+[dependencies]
+easytime = { version = "0.2", optional = true, default-features = false }
+
+[dev-dependencies]
diff --git a/tests/fixtures/namespaced_features/src/lib.rs b/tests/fixtures/namespaced_features/src/lib.rs
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/test.rs b/tests/test.rs
index 99236787..61062ca1 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -1393,3 +1393,15 @@ fn keep_going() {
             ",
         ));
 }
+
+#[test]
+fn namespaced_features() {
+    cargo_hack(["check", "--feature-powerset"])
+        .assert_success2("namespaced_features", Some(60))
+        .stderr_contains(
+            "
+            running `cargo check --no-default-features` on namespaced_features (1/2)
+            running `cargo check --no-default-features --features easytime` on namespaced_features (2/2)
+            ",
+        );
+}