Auto merge of #5390 - matklad:new-feature-behavior, r=alexcrichton

Enable new behavior of `--feature`

So far, the feedback on https://internals.rust-lang.org/t/help-us-test-the-breaking-bug-fix-to-cargo-features/7317 has been positive, so here's a PR to try this in nightly.

Note that the logic is slightly tweak for the case `cargo build -p not-a-workspace-member`: we want not only to resolve all ws members in this case, but to enable all of their features as well!

As a sanity check, this seems to be forward compatible with further improvements to features:

1) when we solve the grand bug of features being unified across the whole workspace, `cargo build -p not-a-member` would hopefully just work without additional contortions.
2) we might add a way to specify features per package, like `cargo build -p foo -p bar --features "foo/serde bar/serde"`

Author: bors

src/cargo/core/features.rs

@@ -292,7 +292,6 @@ pub struct CliUnstable {
     pub no_index_update: bool,
     pub avoid_dev_deps: bool,
     pub minimal_versions: bool,
-    pub package_features: bool,
 }
 
 impl CliUnstable {
@@ -326,7 +325,6 @@ impl CliUnstable {
             "no-index-update" => self.no_index_update = true,
             "avoid-dev-deps" => self.avoid_dev_deps = true,
             "minimal-versions" => self.minimal_versions = true,
-            "package-features" => self.package_features = true,
             _ => bail!("unknown `-Z` flag specified: {}", k),
         }
 

src/cargo/core/resolver/types.rs

@@ -155,7 +155,7 @@ impl<'a> RegistryQueryer<'a> {
     }
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Eq, PartialEq)]
 pub enum Method<'a> {
     Everything, // equivalent to Required { dev_deps: true, all_features: true, .. }
     Required {

src/cargo/core/workspace.rs

@@ -117,6 +117,7 @@ pub struct WorkspaceRootConfig {
 
 /// An iterator over the member packages of a workspace, returned by
 /// `Workspace::members`
+#[derive(Clone)]
 pub struct Members<'a, 'cfg: 'a> {
     ws: &'a Workspace<'cfg>,
     iter: slice::Iter<'a, PathBuf>,

src/cargo/ops/resolve.rs

@@ -217,86 +217,43 @@ pub fn resolve_with_previous<'a, 'cfg>(
         registry.add_sources(&[member.package_id().source_id().clone()])?;
     }
 
-    let mut summaries = Vec::new();
-    if ws.config().cli_unstable().package_features {
-        let mut members = Vec::new();
-        match method {
-            Method::Everything => members.extend(ws.members()),
-            Method::Required {
-                features,
-                all_features,
-                uses_default_features,
-                ..
-            } => {
-                if specs.len() > 1 && !features.is_empty() {
-                    bail!("cannot specify features for more than one package");
-                }
-                members.extend(
-                    ws.members()
-                        .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id()))),
-                );
+    let method = match method {
+        Method::Everything => Method::Everything,
+        Method::Required {
+            features,
+            all_features,
+            uses_default_features,
+            ..
+        } => {
+            if specs.len() > 1 && !features.is_empty() {
+                bail!("cannot specify features for more than one package");
+            }
+            let members_requested = ws.members()
+                .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
+                .count();
+            if members_requested == 0 {
                 // Edge case: running `cargo build -p foo`, where `foo` is not a member
-                // of current workspace. Add all packages from workspace to get `foo`
-                // into the resolution graph.
-                if members.is_empty() {
-                    if !(features.is_empty() && !all_features && uses_default_features) {
-                        bail!("cannot specify features for packages outside of workspace");
-                    }
-                    members.extend(ws.members());
+                // of current workspace. Resolve whole workspace to get `foo` into the
+                // resolution graph.
+                if !(features.is_empty() && !all_features && uses_default_features) {
+                    bail!("cannot specify features for packages outside of workspace");
                 }
+                Method::Everything
+            } else {
+                method
             }
         }
-        for member in members {
-            let summary = registry.lock(member.summary().clone());
-            summaries.push((summary, method))
-        }
-    } else {
-        for member in ws.members() {
-            let method_to_resolve = match method {
-                // When everything for a workspace we want to be sure to resolve all
-                // members in the workspace, so propagate the `Method::Everything`.
-                Method::Everything => Method::Everything,
-
-                // If we're not resolving everything though then we're constructing the
-                // exact crate graph we're going to build. Here we don't necessarily
-                // want to keep around all workspace crates as they may not all be
-                // built/tested.
-                //
-                // Additionally, the `method` specified represents command line
-                // flags, which really only matters for the current package
-                // (determined by the cwd). If other packages are specified (via
-                // `-p`) then the command line flags like features don't apply to
-                // them.
-                //
-                // As a result, if this `member` is the current member of the
-                // workspace, then we use `method` specified. Otherwise we use a
-                // base method with no features specified but using default features
-                // for any other packages specified with `-p`.
-                Method::Required { dev_deps, .. } => {
-                    let base = Method::Required {
-                        dev_deps,
-                        features: &[],
-                        all_features: false,
-                        uses_default_features: true,
-                    };
-                    let member_id = member.package_id();
-                    match ws.current_opt() {
-                        Some(current) if member_id == current.package_id() => method,
-                        _ => {
-                            if specs.iter().any(|spec| spec.matches(member_id)) {
-                                base
-                            } else {
-                                continue;
-                            }
-                        }
-                    }
-                }
-            };
+    };
 
+    let summaries = ws.members()
+        .filter(|m| {
+            method == Method::Everything || specs.iter().any(|spec| spec.matches(m.package_id()))
+        })
+        .map(|member| {
             let summary = registry.lock(member.summary().clone());
-            summaries.push((summary, method_to_resolve));
-        }
-    };
+            (summary, method)
+        })
+        .collect::<Vec<_>>();
 
     let root_replace = ws.root_replace();
 

tests/testsuite/features.rs

@@ -1674,46 +1674,42 @@ fn combining_features_and_package() {
         .build();
 
     assert_that(
-        p.cargo("build -Z package-features --all --features main")
+        p.cargo("build --all --features main")
             .masquerade_as_nightly_cargo(),
-        execs().with_status(101).with_stderr_contains(
-            "\
-             [ERROR] cannot specify features for more than one package",
-        ),
+        execs()
+            .with_status(101)
+            .with_stderr_contains("[ERROR] cannot specify features for more than one package"),
     );
 
     assert_that(
-        p.cargo("build -Z package-features --package dep --features main")
+        p.cargo("build --package dep --features main")
             .masquerade_as_nightly_cargo(),
         execs().with_status(101).with_stderr_contains(
-            "\
-             [ERROR] cannot specify features for packages outside of workspace",
+            "[ERROR] cannot specify features for packages outside of workspace",
         ),
     );
     assert_that(
-        p.cargo("build -Z package-features --package dep --all-features")
+        p.cargo("build --package dep --all-features")
             .masquerade_as_nightly_cargo(),
         execs().with_status(101).with_stderr_contains(
-            "\
-             [ERROR] cannot specify features for packages outside of workspace",
+            "[ERROR] cannot specify features for packages outside of workspace",
         ),
     );
     assert_that(
-        p.cargo("build -Z package-features --package dep --no-default-features")
+        p.cargo("build --package dep --no-default-features")
             .masquerade_as_nightly_cargo(),
         execs().with_status(101).with_stderr_contains(
-            "\
-             [ERROR] cannot specify features for packages outside of workspace",
+            "[ERROR] cannot specify features for packages outside of workspace",
         ),
     );
 
     assert_that(
-        p.cargo("build -Z package-features --all --all-features")
+        p.cargo("build --all --all-features")
             .masquerade_as_nightly_cargo(),
         execs().with_status(0),
     );
     assert_that(
-        p.cargo("run -Z package-features --package foo --features main")
+        p.cargo("run --package foo --features main")
             .masquerade_as_nightly_cargo(),
         execs().with_status(0),
     );

tests/testsuite/test.rs

@@ -2867,13 +2867,7 @@ fn selective_test_optional_dep() {
     let p = p.build();
 
     assert_that(
-        p.cargo("test")
-            .arg("-v")
-            .arg("--no-run")
-            .arg("--features")
-            .arg("a")
-            .arg("-p")
-            .arg("a"),
+        p.cargo("test -v --no-run --package a"),
         execs().with_status(0).with_stderr(
             "\
 [COMPILING] a v0.0.1 ([..])
@@ -3060,6 +3054,8 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
             version = "0.1.0"
             authors = []
 
+            [workspace]
+
             [features]
             default = ["feature_a/default"]
             nightly = ["feature_a/nightly"]
@@ -3137,10 +3133,7 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
     let p = p.build();
 
     assert_that(
-        p.cargo("test")
-            .arg("--package")
-            .arg("feature_a")
-            .arg("--verbose"),
+        p.cargo("test --package feature_a --verbose"),
         execs().with_status(0).with_stderr_contains(
             "\
 [DOCTEST] feature_a
@@ -3149,7 +3142,7 @@ fn pass_correct_cfgs_flags_to_rustdoc() {
     );
 
     assert_that(
-        p.cargo("test").arg("--verbose"),
+        p.cargo("test --verbose"),
         execs().with_status(0).with_stderr_contains(
             "\
 [DOCTEST] foo
@@ -3199,45 +3192,6 @@ fn test_release_ignore_panic() {
     assert_that(p.cargo("bench").arg("-v"), execs().with_status(0));
 }
 
-#[test]
-fn test_many_with_features() {
-    let p = project("foo")
-        .file(
-            "Cargo.toml",
-            r#"
-            [package]
-            name = "foo"
-            version = "0.0.1"
-            authors = []
-
-            [dependencies]
-            a = { path = "a" }
-
-            [features]
-            foo = []
-
-            [workspace]
-        "#,
-        )
-        .file("src/lib.rs", "")
-        .file(
-            "a/Cargo.toml",
-            r#"
-            [package]
-            name = "a"
-            version = "0.0.1"
-            authors = []
-        "#,
-        )
-        .file("a/src/lib.rs", "")
-        .build();
-
-    assert_that(
-        p.cargo("test -v -p a -p foo --features foo"),
-        execs().with_status(0),
-    );
-}
-
 #[test]
 fn test_all_workspace() {
     let p = project("foo")