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

src/cargo/core/workspace.rs

@@ -1071,7 +1071,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 +1196,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 +1215,96 @@ 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.
-                    let is_member = self.members().any(|member| member.name() == *dep_name);
+                    // Check if `dep_name` is member of the workspace, but isn't associated with current package.
+                    // Weak can be ignored for this moment.
+                    let is_member = self.members().any(|member| {
+                        self.current_opt() != Some(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
+                        }
                     }
                 }
-            }
-        });
-        ms.collect()
+            })
+            .collect();
+
+        // If any member specific features were not removed while iterating over members
+        // some features will be ignored.
+        assert!(member_specific_features.is_empty());
+
+        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] package `a[..]` does not have a dependency named `a`")
+        .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]