Implement -Zpackage-features2

ehuss · ehuss · commit c17bfcbdb2e3 · 2020-04-04T18:04:03.000-07:00
diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs
@@ -330,6 +330,7 @@ pub struct CliUnstable {
     pub avoid_dev_deps: bool,
     pub minimal_versions: bool,
     pub package_features: bool,
+    pub package_features2: bool,
     pub advanced_env: bool,
     pub config_include: bool,
     pub dual_proc_macros: bool,
@@ -404,6 +405,7 @@ impl CliUnstable {
             "avoid-dev-deps" => self.avoid_dev_deps = parse_empty(k, v)?,
             "minimal-versions" => self.minimal_versions = parse_empty(k, v)?,
             "package-features" => self.package_features = parse_empty(k, v)?,
+            "package-features2" => self.package_features2 = parse_empty(k, v)?,
             "advanced-env" => self.advanced_env = parse_empty(k, v)?,
             "config-include" => self.config_include = parse_empty(k, v)?,
             "dual-proc-macros" => self.dual_proc_macros = parse_empty(k, v)?,
diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs
@@ -1,7 +1,8 @@
 use std::cell::RefCell;
 use std::collections::hash_map::{Entry, HashMap};
-use std::collections::{BTreeMap, HashSet};
+use std::collections::{BTreeMap, BTreeSet, HashSet};
 use std::path::{Path, PathBuf};
+use std::rc::Rc;
 use std::slice;
 
 use glob::glob;
@@ -11,7 +12,7 @@ use url::Url;
 use crate::core::features::Features;
 use crate::core::registry::PackageRegistry;
 use crate::core::resolver::features::RequestedFeatures;
-use crate::core::{Dependency, PackageId, PackageIdSpec};
+use crate::core::{Dependency, InternedString, PackageId, PackageIdSpec};
 use crate::core::{EitherManifest, Package, SourceId, VirtualManifest};
 use crate::ops;
 use crate::sources::PathSource;
@@ -877,60 +878,154 @@ impl<'cfg> Workspace<'cfg> {
                 .map(|m| (m, RequestedFeatures::new_all(true)))
                 .collect());
         }
-        if self.config().cli_unstable().package_features {
-            if specs.len() > 1 && !requested_features.features.is_empty() {
-                anyhow::bail!("cannot specify features for more than one package");
+        if self.config().cli_unstable().package_features
+            || self.config().cli_unstable().package_features2
+        {
+            self.members_with_features_pf(specs, requested_features)
+        } else {
+            self.members_with_features_stable(specs, requested_features)
+        }
+    }
+
+    /// New command-line feature selection with -Zpackage-features or -Zpackage-features2.
+    fn members_with_features_pf(
+        &self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<Vec<(&Package, RequestedFeatures)>> {
+        let pf2 = self.config().cli_unstable().package_features2;
+        if specs.len() > 1 && !requested_features.features.is_empty() && !pf2 {
+            anyhow::bail!("cannot specify features for more than one package");
+        }
+        // Keep track of which features matched *any* member, to produce an error
+        // if any of them did not match anywhere.
+        let mut found: BTreeSet<InternedString> = BTreeSet::new();
+
+        // Returns the requested features for the given member.
+        // This filters out any named features that the member does not have.
+        let mut matching_features = |member: &Package| -> RequestedFeatures {
+            // This new behavior is only enabled for -Zpackage-features2
+            if !pf2 {
+                return requested_features.clone();
+            }
+            if requested_features.features.is_empty() || requested_features.all_features {
+                return requested_features.clone();
             }
-            let members: Vec<(&Package, RequestedFeatures)> = self
+            // Only include features this member defines.
+            let summary = member.summary();
+            let member_features = summary.features();
+            let mut features = BTreeSet::new();
+
+            // Checks if a member contains the given feature.
+            let contains = |feature: InternedString| -> bool {
+                member_features.contains_key(&feature)
+                    || summary
+                        .dependencies()
+                        .iter()
+                        .any(|dep| dep.is_optional() && dep.name_in_toml() == feature)
+            };
+
+            for feature in requested_features.features.iter() {
+                let mut split = feature.splitn(2, '/');
+                let split = (split.next().unwrap(), split.next());
+                if let (pkg, Some(pkg_feature)) = split {
+                    let pkg = InternedString::new(pkg);
+                    let pkg_feature = InternedString::new(pkg_feature);
+                    if summary
+                        .dependencies()
+                        .iter()
+                        .any(|dep| dep.name_in_toml() == pkg)
+                    {
+                        // pkg/feat for a dependency.
+                        // Will rely on the dependency resolver to validate `feat`.
+                        features.insert(*feature);
+                        found.insert(*feature);
+                    } else if pkg == member.name() && contains(pkg_feature) {
+                        // member/feat where "feat" is a feature in member.
+                        features.insert(pkg_feature);
+                        found.insert(*feature);
+                    }
+                } else if contains(*feature) {
+                    // feature exists in this member.
+                    features.insert(*feature);
+                    found.insert(*feature);
+                }
+            }
+            RequestedFeatures {
+                features: Rc::new(features),
+                all_features: false,
+                uses_default_features: requested_features.uses_default_features,
+            }
+        };
+
+        let members: Vec<(&Package, RequestedFeatures)> = self
+            .members()
+            .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
+            .map(|m| (m, matching_features(m)))
+            .collect();
+        if members.is_empty() {
+            // `cargo build -p foo`, where `foo` is not a member.
+            // Do not allow any command-line flags (defaults only).
+            if !(requested_features.features.is_empty()
+                && !requested_features.all_features
+                && requested_features.uses_default_features)
+            {
+                anyhow::bail!("cannot specify features for packages outside of workspace");
+            }
+            // Add all members from the workspace so we can ensure `-p nonmember`
+            // is in the resolve graph.
+            return Ok(self
                 .members()
-                .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
-                .map(|m| (m, requested_features.clone()))
+                .map(|m| (m, RequestedFeatures::new_all(false)))
+                .collect());
+        }
+        if pf2 && *requested_features.features != found {
+            let missing: Vec<_> = requested_features
+                .features
+                .difference(&found)
+                .copied()
                 .collect();
-            if members.is_empty() {
-                // `cargo build -p foo`, where `foo` is not a member.
-                // Do not allow any command-line flags (defaults only).
-                if !(requested_features.features.is_empty()
-                    && !requested_features.all_features
-                    && requested_features.uses_default_features)
-                {
-                    anyhow::bail!("cannot specify features for packages outside of workspace");
+            // TODO: typo suggestions would be good here.
+            anyhow::bail!(
+                "none of the selected packages contains these features: {}",
+                missing.join(", ")
+            );
+        }
+        Ok(members)
+    }
+
+    /// This is the current "stable" behavior for command-line feature selection.
+    fn members_with_features_stable(
+        &self,
+        specs: &[PackageIdSpec],
+        requested_features: &RequestedFeatures,
+    ) -> CargoResult<Vec<(&Package, RequestedFeatures)>> {
+        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() => {
+                    Some((member, requested_features.clone()))
                 }
-                // Add all members from the workspace so we can ensure `-p nonmember`
-                // is in the resolve graph.
-                return Ok(self
-                    .members()
-                    .map(|m| (m, RequestedFeatures::new_all(false)))
-                    .collect());
-            }
-            Ok(members)
-        } else {
-            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() => {
-                        Some((member, requested_features.clone()))
-                    }
-                    _ => {
-                        // 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, don't use the local
-                            // `--features`, only allow `--all-features`.
-                            Some((
-                                member,
-                                RequestedFeatures::new_all(requested_features.all_features),
-                            ))
-                        } 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, don't use the local
+                        // `--features`, only allow `--all-features`.
+                        Some((
+                            member,
+                            RequestedFeatures::new_all(requested_features.all_features),
+                        ))
+                    } else {
+                        // This member was not requested on the command-line, skip.
+                        None
                     }
                 }
-            });
-            Ok(ms.collect())
-        }
+            }
+        });
+        Ok(ms.collect())
     }
 }
 
diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs
@@ -62,7 +62,14 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> {
     let mut build_config = BuildConfig::new(config, Some(1), &opts.target, CompileMode::Build)?;
     build_config.requested_profile = opts.requested_profile;
     let target_data = RustcTargetData::new(ws, build_config.requested_kind)?;
-    let resolve_opts = ResolveOpts::everything();
+    // Resolve for default features. In the future, `cargo clean` should be rewritten
+    // so that it doesn't need to guess filename hashes.
+    let resolve_opts = ResolveOpts::new(
+        /*dev_deps*/ true,
+        &[],
+        /*all features*/ false,
+        /*default*/ true,
+    );
     let specs = opts
         .spec
         .iter()
diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs
@@ -288,7 +288,9 @@ pub trait ArgMatchesExt {
         if config.cli_unstable().avoid_dev_deps {
             ws.set_require_optional_deps(false);
         }
-        if ws.is_virtual() && !config.cli_unstable().package_features {
+        let unstable =
+            config.cli_unstable().package_features || config.cli_unstable().package_features2;
+        if ws.is_virtual() && !unstable {
             // --all-features is actually honored. In general, workspaces and
             // feature flags are a bit of a mess right now.
             for flag in &["features", "no-default-features"] {
diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md
@@ -474,7 +474,7 @@ cargo +nightly -Zunstable-options -Zconfig-include --config somefile.toml build
 
 CLI paths are relative to the current working directory.
 
-## Features
+### Features
 * Tracking Issues:
   * [itarget #7914](https://github.com/rust-lang/cargo/issues/7914)
   * [build_dep #7915](https://github.com/rust-lang/cargo/issues/7915)
@@ -549,6 +549,31 @@ The available options are:
 * `compare` — This option compares the resolved features to the old resolver,
   and will print any differences.
 
+### package-features2
+* Tracking Issue: TODO
+
+The `-Zpackage-features2` flag changes the way features can be passed on the
+command-line for a workspace. The normal behavior can be confusing, as the
+features passed are always enabled on the package in the current directory,
+even if that package is not selected with a `-p` flag. Feature flags also do
+not work in the root of a virtual workspace. `-Zpackage-features2` tries to
+make feature flags behave in a more intuitive manner.
+
+* `cargo build -p other_member --features …` — This now only enables the given
+  features as defined in `other_member` (ignores whatever is in the current
+  directory).
+* `cargo build -p a -p b --features …` — This now enables the given features
+  on both `a` and `b`. Not all packages need to define every feature, it only
+  enables matching features. It is still an error if none of the packages
+  define a given feature.
+* `--features` and `--no-default-features` are now allowed in the root of a
+  virtual workspace.
+* `member_name/feature_name` syntax may now be used on the command-line to
+  enable features for a specific member.
+
+The ability to set features for non-workspace members is no longer allowed, as
+the resolver fundamentally does not support that ability.
+
 ### crate-versions
 * Tracking Issue: [#7907](https://github.com/rust-lang/cargo/issues/7907)
 
diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs
@@ -73,6 +73,7 @@ mod offline;
 mod out_dir;
 mod owner;
 mod package;
+mod package_features2;
 mod patch;
 mod path;
 mod paths;
diff --git a/tests/testsuite/package_features2.rs b/tests/testsuite/package_features2.rs
