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

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`

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,71 +1216,126 @@ 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 member_id = member.package_id();
-            match self.current_opt() {
-                // The features passed on the command-line only apply to
-                // the "current" package (determined by the cwd).
-                Some(current) if member_id == current.package_id() => {
-                    let feats = CliFeatures {
-                        features: Rc::new(cwd_features.clone()),
-                        all_features: cli_features.all_features,
-                        uses_default_features: cli_features.uses_default_features,
-                    };
-                    Some((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.
-                        //
-                        // The odd behavior here is due to backwards
-                        // compatibility. `--features` and
-                        // `--no-default-features` used to only apply to the
-                        // "current" package. As an extension, this allows
-                        // member-name/feature-name to set member-specific
-                        // features, which should be backwards-compatible.
+        let result: Vec<_> = self
+            .members()
+            .filter_map(|member| {
+                let member_id = member.package_id();
+                match self.current_opt() {
+                    // The features passed on the command-line only apply to
+                    // the "current" package (determined by the cwd).
+                    Some(current) if member_id == current.package_id() => {
                         let feats = CliFeatures {
-                            features: Rc::new(
-                                member_specific_features
-                                    .remove(member.name().as_str())
-                                    .unwrap_or_default(),
-                            ),
-                            uses_default_features: true,
+                            features: Rc::new(cwd_features.clone()),
                             all_features: cli_features.all_features,
+                            uses_default_features: cli_features.uses_default_features,
                         };
+
                         Some((member, feats))
-                    } else {
-                        // This member was not requested on the command-line, skip.
-                        None
+                    }
+                    _ => {
+                        // 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.
+                            //
+                            // The odd behavior here is due to backwards
+                            // compatibility. `--features` and
+                            // `--no-default-features` used to only apply to the
+                            // "current" package. As an extension, this allows
+                            // member-name/feature-name to set member-specific
+                            // features, which should be backwards-compatible.
+                            let feats = CliFeatures {
+                                features: Rc::new(
+                                    member_specific_features
+                                        .remove(member.name().as_str())
+                                        .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))
+                        } else {
+                            // This member was not requested on the command-line, skip.
+                            None
+                        }
                     }
                 }
+            })
+            .collect();
+
+        // If any member specific features were not removed while iterating over members
+        if !member_specific_features.is_empty() {
+            let unknown: Vec<_> = member_specific_features
+                .values()
+                .map(|set| set.iter())
+                .flatten()
+                .sorted_by_key(|feature| feature.to_string())
+                .collect();
+
+            if self.is_virtual() {
+                bail!(
+                    "None of the selected packages contains these features: {}",
+                    unknown.iter().join(", "),
+                );
+            } else {
+                bail!(
+                    "None of the selected packages contains these features: {}, did you mean: {}?",
+                    unknown.iter().join(", "),
+                    unknown
+                        .iter()
+                        .map(|feature| match feature {
+                            // Remove package prefix from feature
+                            FeatureValue::DepFeature {
+                                dep_name: _,
+                                dep_feature,
+                                dep_prefix: false,
+                                weak: _,
+                            } => FeatureValue::new(*dep_feature),
+                            // Member specific features by definition contain only `FeatureValue::DepFeature`
+                            _ => unreachable!(),
+                        })
+                        .join(", ")
+                );
             }
-        });
-        ms.collect()
+        }
+
+        Ok(result)
     }
 }
 

tests/testsuite/package_features.rs

@@ -272,6 +272,51 @@ fn other_member_from_current() {
         .run();
 }
 
+#[cargo_test]
+fn feature_default_resolver() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "a"
+            version = "0.1.0"
+
+            [features]
+            test = []
+            "#,
+        )
+        .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] 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] None of the selected packages contains these features: a/test, did you mean: test?")
+        .run();
+}
+
 #[cargo_test]
 fn virtual_member_slash() {
     // member slash feature syntax
@@ -458,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]