Fix bug when with resolver = "1" non-virtual package was allowing unknown features

diff --git a/Cargo.toml b/Cargo.toml
index eaf8e8307e85d85b8faa1593c4bbca12fe334163..c7f17c0303908f2494681f0f53b9c7b3c2046845 100644 (file)
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -67,6 +67,7 @@ clap = "2.31.2"
 unicode-width = "0.1.5"
 openssl = { version = '0.10.11', optional = true }
 im-rc = "15.0.0"
+itertools = "0.10.0"
 
 # A noop dependency that changes in the Rust repository, it's a bit of a hack.
 # See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust`

diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
index 9a2d5920a748ea3a4c5c9cf551b932fb5ffa581c..42e7dd0dad6d273e660fc19a62a27e23b3145d4b 100644 (file)
--- a/src/cargo/core/workspace.rs
+++ b/src/cargo/core/workspace.rs
@@ -7,6 +7,7 @@ use std::slice;
 
 use anyhow::{bail, Context as _};
 use glob::glob;
+use itertools::Itertools;
 use log::debug;
 use url::Url;
 
@@ -1071,7 +1072,7 @@ impl<'cfg> Workspace<'cfg> {
         if self.allows_new_cli_feature_behavior() {
             self.members_with_features_new(specs, cli_features)
         } else {
-            Ok(self.members_with_features_old(specs, cli_features))
+            self.members_with_features_old(specs, cli_features)
         }
     }
 
@@ -1196,7 +1197,7 @@ impl<'cfg> Workspace<'cfg> {
         &self,
         specs: &[PackageIdSpec],
         cli_features: &CliFeatures,
-    ) -> Vec<(&Package, CliFeatures)> {
+    ) -> CargoResult<Vec<(&Package, CliFeatures)>> {
         // Split off any features with the syntax `member-name/feature-name` into a map
         // so that those features can be applied directly to those workspace-members.
         let mut member_specific_features: HashMap<InternedString, BTreeSet<FeatureValue>> =
@@ -1215,29 +1216,29 @@ impl<'cfg> Workspace<'cfg> {
                 } => panic!("unexpected dep: syntax {}", feature),
                 FeatureValue::DepFeature {
                     dep_name,
-                    dep_feature,
+                    dep_feature: _,
                     dep_prefix: _,
                     weak: _,
                 } => {
-                    // I think weak can be ignored here.
-                    // * With `--features member?/feat -p member`, the ? doesn't
-                    //   really mean anything (either the member is built or it isn't).
-                    // * With `--features nonmember?/feat`, cwd_features will
-                    //   handle processing it correctly.
+                    // Check if `dep_name` is member of the workspace or package.
+                    // Weak can be ignored for this moment.
                     let is_member = self.members().any(|member| member.name() == *dep_name);
                     if is_member && specs.iter().any(|spec| spec.name() == *dep_name) {
                         member_specific_features
                             .entry(*dep_name)
                             .or_default()
-                            .insert(FeatureValue::Feature(*dep_feature));
+                            .insert(feature.clone());
                     } else {
+                        // With `--features nonmember?/feat`, cwd_features will
+                        // handle processing it correctly.
                         cwd_features.insert(feature.clone());
                     }
                 }
             }
         }
 
-        let ms = self.members().filter_map(|member| {
+        let mut result = Vec::new();
+        for member in self.members() {
             let member_id = member.package_id();
             match self.current_opt() {
                 // The features passed on the command-line only apply to
@@ -1248,13 +1249,29 @@ impl<'cfg> Workspace<'cfg> {
                         all_features: cli_features.all_features,
                         uses_default_features: cli_features.uses_default_features,
                     };
-                    Some((member, feats))
+
+                    // If any member specific features were passed to non-virtual package, it's error
+                    if !member_specific_features.is_empty() {
+                        let invalid: Vec<_> = member_specific_features
+                            .values()
+                            .map(|set| set.iter())
+                            .flatten()
+                            .map(|feature| feature.to_string())
+                            .sorted()
+                            .collect();
+
+                        bail!(
+                            "Member specific features with `pkg/feat` syntax are dissalowed outside of workspace with `resolver = \"1\", remove: {}", 
+                            invalid.join(", ")
+                        );
+                    }
+
+                    result.push((member, feats))
                 }
                 _ => {
                     // Ignore members that are not enabled on the command-line.
                     if specs.iter().any(|spec| spec.matches(member_id)) {
-                        // -p for a workspace member that is not the "current"
-                        // one.
+                        // -p for a workspace member that is not the "current" one.
                         //
                         // The odd behavior here is due to backwards
                         // compatibility. `--features` and
@@ -1266,20 +1283,36 @@ impl<'cfg> Workspace<'cfg> {
                             features: Rc::new(
                                 member_specific_features
                                     .remove(member.name().as_str())
-                                    .unwrap_or_default(),
+                                    .unwrap_or_default()
+                                    .into_iter()
+                                    .map(|feature| match feature {
+                                        // I think weak can be ignored here.
+                                        // With `--features member?/feat -p member`, the ? doesn't
+                                        // really mean anything (either the member is built or it isn't).
+                                        FeatureValue::DepFeature {
+                                            dep_name: _,
+                                            dep_feature,
+                                            dep_prefix: false,
+                                            weak: _,
+                                        } => FeatureValue::new(dep_feature),
+                                        // Member specific features by definition contain only `FeatureValue::DepFeature`
+                                        _ => unreachable!(),
+                                    })
+                                    .collect(),
                             ),
                             uses_default_features: true,
                             all_features: cli_features.all_features,
                         };
-                        Some((member, feats))
+
+                        result.push((member, feats))
                     } else {
                         // This member was not requested on the command-line, skip.
-                        None
                     }
                 }
             }
-        });
-        ms.collect()
+        }
+
+        Ok(result)
     }
 }
 

diff --git a/tests/testsuite/package_features.rs b/tests/testsuite/package_features.rs
index dd9cd21279b431ea766f1b60b8300104daed44d7..bb2880cc43cbf4c2c9b0fcaf96675072278530d1 100644 (file)
--- a/tests/testsuite/package_features.rs
+++ b/tests/testsuite/package_features.rs
@@ -215,7 +215,7 @@ fn other_member_from_current() {
             name = "bar"
             version = "0.1.0"
 
-            [features]
+            [features]            
             f1 = []
             f2 = []
             f3 = []
@@ -273,8 +273,8 @@ fn other_member_from_current() {
 }
 
 #[cargo_test]
-fn virtual_typo_member_feature_default_resolver() {
-    project()
+fn feature_default_resolver() {
+    let p = project()
         .file(
             "Cargo.toml",
             r#"
@@ -283,17 +283,37 @@ fn virtual_typo_member_feature_default_resolver() {
             version = "0.1.0"
 
             [features]
-            deny-warnings = []
+            test = []
             "#,
         )
-        .file("src/lib.rs", "")
-        .build()
-        .cargo("check --features a/deny-warning")
+        .file(
+            "src/main.rs",
+            r#"
+                fn main() {
+                    if cfg!(feature = "test") {
+                        println!("feature set");
+                    }
+                }
+            "#,
+        )
+        .build();
+
+    p.cargo("check --features testt")
         .masquerade_as_nightly_cargo()
         .with_status(101)
-        .with_stderr(
-            "[ERROR] none of the selected packages contains these features: a/deny-warning",
-        )
+        .with_stderr("[ERROR] Package `a[..]` does not have the feature `testt`")
+        .run();
+
+    p.cargo("run --features test")
+        .masquerade_as_nightly_cargo()
+        .with_status(0)
+        .with_stdout("feature set")
+        .run();
+
+    p.cargo("run --features a/test")
+        .masquerade_as_nightly_cargo()
+        .with_status(101)
+        .with_stderr("[ERROR] Member specific features with `pkg/feat` syntax are dissalowed outside of workspace with `resolver = \"1\", remove: a/test")
         .run();
 }
 
@@ -483,6 +503,12 @@ fn resolver1_member_features() {
         .cwd("member2")
         .with_stdout("m1-feature set")
         .run();
+
+    p.cargo("check -p member1 --features member1/m2-feature")
+        .cwd("member2")
+        .with_status(101)
+        .with_stderr("[ERROR] Package `member1[..]` does not have the feature `m2-feature`")
+        .run();
 }
 
 #[cargo_test]