From 2611f5c73c84030086bb429304ae468753693e24 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 19 Oct 2020 16:51:02 -0700 Subject: [PATCH 1/9] Move namespaced features tests to a separate file. --- tests/testsuite/features.rs | 271 ------------------------ tests/testsuite/features_namespaced.rs | 274 +++++++++++++++++++++++++ tests/testsuite/main.rs | 1 + 3 files changed, 275 insertions(+), 271 deletions(-) create mode 100644 tests/testsuite/features_namespaced.rs diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index bb35bad4e02..dc346e7fed7 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -1353,277 +1353,6 @@ fn many_cli_features_comma_and_space_delimited() { .run(); } -#[cargo_test] -fn namespaced_invalid_feature() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - bar = ["baz"] - "#, - ) - .file("src/main.rs", "") - .build(); - - p.cargo("build") - .masquerade_as_nightly_cargo() - .with_status(101) - .with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `bar` includes `baz` which is not defined as a feature -", - ) - .run(); -} - -#[cargo_test] -fn namespaced_invalid_dependency() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - bar = ["crate:baz"] - "#, - ) - .file("src/main.rs", "") - .build(); - - p.cargo("build") - .masquerade_as_nightly_cargo() - .with_status(101) - .with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `bar` includes `crate:baz` which is not a known dependency -", - ) - .run(); -} - -#[cargo_test] -fn namespaced_non_optional_dependency() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - bar = ["crate:baz"] - - [dependencies] - baz = "0.1" - "#, - ) - .file("src/main.rs", "") - .build(); - - p.cargo("build") - .masquerade_as_nightly_cargo() - .with_status(101) - .with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `bar` includes `crate:baz` which is not an optional dependency. - Consider adding `optional = true` to the dependency -", - ) - .run(); -} - -#[cargo_test] -fn namespaced_implicit_feature() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - bar = ["baz"] - - [dependencies] - baz = { version = "0.1", optional = true } - "#, - ) - .file("src/main.rs", "fn main() {}") - .build(); - - p.cargo("build").masquerade_as_nightly_cargo().run(); -} - -#[cargo_test] -fn namespaced_shadowed_dep() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - baz = [] - - [dependencies] - baz = { version = "0.1", optional = true } - "#, - ) - .file("src/main.rs", "fn main() {}") - .build(); - - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `baz` includes the optional dependency of the same name, but this is left implicit in the features included by this feature. - Consider adding `crate:baz` to this feature's requirements. -", - ) - .run(); -} - -#[cargo_test] -fn namespaced_shadowed_non_optional() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - baz = [] - - [dependencies] - baz = "0.1" - "#, - ) - .file("src/main.rs", "fn main() {}") - .build(); - - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `baz` includes the dependency of the same name, but this is left implicit in the features included by this feature. - Additionally, the dependency must be marked as optional to be included in the feature definition. - Consider adding `crate:baz` to this feature's requirements and marking the dependency as `optional = true` -", - ) - .run(); -} - -#[cargo_test] -fn namespaced_implicit_non_optional() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - bar = ["baz"] - - [dependencies] - baz = "0.1" - "#, - ) - .file("src/main.rs", "fn main() {}") - .build(); - - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `bar` includes `baz` which is not defined as a feature. - A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition -", - ).run(); -} - -#[cargo_test] -fn namespaced_same_name() { - let p = project() - .file( - "Cargo.toml", - r#" - cargo-features = ["namespaced-features"] - - [project] - name = "foo" - version = "0.0.1" - authors = [] - namespaced-features = true - - [features] - baz = ["crate:baz"] - - [dependencies] - baz = { version = "0.1", optional = true } - "#, - ) - .file("src/main.rs", "fn main() {}") - .build(); - - p.cargo("build").masquerade_as_nightly_cargo().run(); -} - #[cargo_test] fn only_dep_is_optional() { Package::new("bar", "0.1.0").publish(); diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs new file mode 100644 index 00000000000..5cee06d01b5 --- /dev/null +++ b/tests/testsuite/features_namespaced.rs @@ -0,0 +1,274 @@ +//! Tests for namespaced features. + +use cargo_test_support::project; + +#[cargo_test] +fn namespaced_invalid_feature() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + bar = ["baz"] + "#, + ) + .file("src/main.rs", "") + .build(); + + p.cargo("build") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `bar` includes `baz` which is not defined as a feature +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_invalid_dependency() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + bar = ["crate:baz"] + "#, + ) + .file("src/main.rs", "") + .build(); + + p.cargo("build") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `bar` includes `crate:baz` which is not a known dependency +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_non_optional_dependency() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + bar = ["crate:baz"] + + [dependencies] + baz = "0.1" + "#, + ) + .file("src/main.rs", "") + .build(); + + p.cargo("build") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `bar` includes `crate:baz` which is not an optional dependency. + Consider adding `optional = true` to the dependency +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_implicit_feature() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + bar = ["baz"] + + [dependencies] + baz = { version = "0.1", optional = true } + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build").masquerade_as_nightly_cargo().run(); +} + +#[cargo_test] +fn namespaced_shadowed_dep() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + baz = [] + + [dependencies] + baz = { version = "0.1", optional = true } + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `baz` includes the optional dependency of the same name, but this is left implicit in the features included by this feature. + Consider adding `crate:baz` to this feature's requirements. +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_shadowed_non_optional() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + baz = [] + + [dependencies] + baz = "0.1" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `baz` includes the dependency of the same name, but this is left implicit in the features included by this feature. + Additionally, the dependency must be marked as optional to be included in the feature definition. + Consider adding `crate:baz` to this feature's requirements and marking the dependency as `optional = true` +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_implicit_non_optional() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + bar = ["baz"] + + [dependencies] + baz = "0.1" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + Feature `bar` includes `baz` which is not defined as a feature. + A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition +", + ).run(); +} + +#[cargo_test] +fn namespaced_same_name() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["namespaced-features"] + + [project] + name = "foo" + version = "0.0.1" + authors = [] + namespaced-features = true + + [features] + baz = ["crate:baz"] + + [dependencies] + baz = { version = "0.1", optional = true } + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build").masquerade_as_nightly_cargo().run(); +} diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 2b1767d41e9..5c55e2cb4b4 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -45,6 +45,7 @@ mod edition; mod error; mod features; mod features2; +mod features_namespaced; mod fetch; mod fix; mod freshness; From 36c69a18fe8af5c07c9ecf6234f0c560bcc415a2 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 19 Oct 2020 16:54:45 -0700 Subject: [PATCH 2/9] Rename features() to unstable_features(). To avoid confusion with the...other thing called "features". --- src/cargo/core/compiler/mod.rs | 2 +- src/cargo/core/features.rs | 2 +- src/cargo/core/manifest.rs | 15 ++++++++------- src/cargo/core/workspace.rs | 7 ++++--- src/cargo/ops/cargo_clean.rs | 7 ++++++- src/cargo/ops/cargo_compile.rs | 2 +- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/ops/resolve.rs | 4 +++- 8 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 0fc9a8145d9..a408c089e03 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -1034,7 +1034,7 @@ pub fn extern_args( if unit .pkg .manifest() - .features() + .unstable_features() .require(Feature::public_dependency()) .is_ok() && !dep.public diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 5b3fbce1977..642e6efec18 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -25,7 +25,7 @@ //! use core::{Feature, Features}; //! //! let feature = Feature::launch_into_space(); -//! package.manifest().features().require(feature).chain_err(|| { +//! package.manifest().unstable_features().require(feature).chain_err(|| { //! "launching Cargo into space right now is unstable and may result in \ //! unintended damage to your codebase, use with caution" //! })?; diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index dc69fbc08e3..31c56f3d658 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -42,7 +42,7 @@ pub struct Manifest { patch: HashMap>, workspace: WorkspaceConfig, original: Rc, - features: Features, + unstable_features: Features, edition: Edition, im_a_teapot: Option, default_run: Option, @@ -371,7 +371,7 @@ impl Manifest { replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, workspace: WorkspaceConfig, - features: Features, + unstable_features: Features, edition: Edition, im_a_teapot: Option, default_run: Option, @@ -393,7 +393,7 @@ impl Manifest { replace, patch, workspace, - features, + unstable_features, edition, original, im_a_teapot, @@ -467,8 +467,9 @@ impl Manifest { &self.workspace } - pub fn features(&self) -> &Features { - &self.features + /// Unstable, nightly features that are enabled in this manifest. + pub fn unstable_features(&self) -> &Features { + &self.unstable_features } /// The style of resolver behavior to use, declared with the `resolver` field. @@ -487,7 +488,7 @@ impl Manifest { pub fn feature_gate(&self) -> CargoResult<()> { if self.im_a_teapot.is_some() { - self.features + self.unstable_features .require(Feature::test_dummy_unstable()) .chain_err(|| { anyhow::format_err!( @@ -578,7 +579,7 @@ impl VirtualManifest { &self.warnings } - pub fn features(&self) -> &Features { + pub fn unstable_features(&self) -> &Features { &self.features } diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index b53329015d3..75fb598da8d 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -625,10 +625,11 @@ impl<'cfg> Workspace<'cfg> { Ok(()) } - pub fn features(&self) -> &Features { + /// Returns the unstable nightly-only features enabled via `cargo-features` in the manifest. + pub fn unstable_features(&self) -> &Features { match self.root_maybe() { - MaybePackage::Package(p) => p.manifest().features(), - MaybePackage::Virtual(vm) => vm.features(), + MaybePackage::Package(p) => p.manifest().unstable_features(), + MaybePackage::Virtual(vm) => vm.unstable_features(), } } diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 5efa976f04d..68f82b3c146 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -34,7 +34,12 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> { return rm_rf(&target_dir.into_path_unlocked(), config); } - let profiles = Profiles::new(ws.profiles(), config, opts.requested_profile, ws.features())?; + let profiles = Profiles::new( + ws.profiles(), + config, + opts.requested_profile, + ws.unstable_features(), + )?; if opts.profile_specified { // After parsing profiles we know the dir-name of the profile, if a profile diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index f1464b7337c..926553ab9b5 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -424,7 +424,7 @@ pub fn create_bcx<'a, 'cfg>( ws.profiles(), config, build_config.requested_profile, - ws.features(), + ws.unstable_features(), )?; profiles.validate_packages( ws.profiles(), diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 5dc077c4b4d..a32c67004dc 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -681,7 +681,7 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car let rustc_args = if pkg .manifest() - .features() + .unstable_features() .require(Feature::public_dependency()) .is_ok() { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 05b3f5961e3..fe695fd5794 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -352,7 +352,9 @@ pub fn resolve_with_previous<'cfg>( registry, &try_to_use, Some(ws.config()), - ws.features().require(Feature::public_dependency()).is_ok(), + ws.unstable_features() + .require(Feature::public_dependency()) + .is_ok(), )?; resolved.register_used_patches(®istry.patches()); if register_patches { From dca0b1156651a94a2ffd93d9c12c670727309a94 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 19 Oct 2020 17:23:31 -0700 Subject: [PATCH 3/9] Slightly rewrite how conflicting activation errors are processed. I'm not sure why the original code partitioned the errors in the way that it did. I think it is better to exhaustively match all the reasons, so that when new reasons are added, it will be clear that this code needs to be updated. I also think it simplifies the code a little. --- src/cargo/core/resolver/errors.rs | 132 +++++++++++++++--------------- 1 file changed, 66 insertions(+), 66 deletions(-) diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index d39ee7c87c7..9ad230f7f80 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -108,76 +108,76 @@ pub(super) fn activation_error( let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect(); conflicting_activations.sort_unstable(); - let (links_errors, mut other_errors): (Vec<_>, Vec<_>) = conflicting_activations - .drain(..) - .rev() - .partition(|&(_, r)| r.is_links()); - - for &(p, r) in links_errors.iter() { - if let ConflictReason::Links(ref link) = *r { - msg.push_str("\n\nthe package `"); - msg.push_str(&*dep.package_name()); - msg.push_str("` links to the native library `"); - msg.push_str(link); - msg.push_str("`, but it conflicts with a previous package which links to `"); - msg.push_str(link); - msg.push_str("` as well:\n"); - } - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); - } - - let (features_errors, mut other_errors): (Vec<_>, Vec<_>) = other_errors - .drain(..) - .partition(|&(_, r)| r.is_missing_features()); - - for &(p, r) in features_errors.iter() { - if let ConflictReason::MissingFeatures(ref features) = *r { - msg.push_str("\n\nthe package `"); - msg.push_str(&*p.name()); - msg.push_str("` depends on `"); - msg.push_str(&*dep.package_name()); - msg.push_str("`, with features: `"); - msg.push_str(features); - msg.push_str("` but `"); - msg.push_str(&*dep.package_name()); - msg.push_str("` does not have these features.\n"); + // This is reversed to show the newest versions first. I don't know if there is + // a strong reason to do this, but that is how the code previously worked + // (see https://github.com/rust-lang/cargo/pull/5037) and I don't feel like changing it. + conflicting_activations.reverse(); + // Flag used for grouping all semver errors together. + let mut has_semver = false; + + for (p, r) in &conflicting_activations { + match r { + ConflictReason::Semver => { + has_semver = true; + } + ConflictReason::Links(link) => { + msg.push_str("\n\nthe package `"); + msg.push_str(&*dep.package_name()); + msg.push_str("` links to the native library `"); + msg.push_str(&link); + msg.push_str("`, but it conflicts with a previous package which links to `"); + msg.push_str(&link); + msg.push_str("` as well:\n"); + msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + } + ConflictReason::MissingFeatures(features) => { + msg.push_str("\n\nthe package `"); + msg.push_str(&*p.name()); + msg.push_str("` depends on `"); + msg.push_str(&*dep.package_name()); + msg.push_str("`, with features: `"); + msg.push_str(features); + msg.push_str("` but `"); + msg.push_str(&*dep.package_name()); + msg.push_str("` does not have these features.\n"); + // p == parent so the full path is redundant. + } + ConflictReason::RequiredDependencyAsFeatures(features) => { + msg.push_str("\n\nthe package `"); + msg.push_str(&*p.name()); + msg.push_str("` depends on `"); + msg.push_str(&*dep.package_name()); + msg.push_str("`, with features: `"); + msg.push_str(features); + msg.push_str("` but `"); + msg.push_str(&*dep.package_name()); + msg.push_str("` does not have these features.\n"); + msg.push_str( + " It has a required dependency with that name, \ + but only optional dependencies can be used as features.\n", + ); + // p == parent so the full path is redundant. + } + ConflictReason::PublicDependency(pkg_id) => { + // TODO: This needs to be implemented. + unimplemented!("pub dep {:?}", pkg_id); + } + ConflictReason::PubliclyExports(pkg_id) => { + // TODO: This needs to be implemented. + unimplemented!("pub exp {:?}", pkg_id); + } } - // p == parent so the full path is redundant. } - let (required_dependency_as_features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors - .drain(..) - .partition(|&(_, r)| r.is_required_dependency_as_features()); - - for &(p, r) in required_dependency_as_features_errors.iter() { - if let ConflictReason::RequiredDependencyAsFeatures(ref features) = *r { - msg.push_str("\n\nthe package `"); - msg.push_str(&*p.name()); - msg.push_str("` depends on `"); - msg.push_str(&*dep.package_name()); - msg.push_str("`, with features: `"); - msg.push_str(features); - msg.push_str("` but `"); - msg.push_str(&*dep.package_name()); - msg.push_str("` does not have these features.\n"); - msg.push_str( - " It has a required dependency with that name, \ - but only optional dependencies can be used as features.\n", - ); + if has_semver { + // Group these errors together. + msg.push_str("\n\nall possible versions conflict with previously selected packages."); + for (p, r) in &conflicting_activations { + if let ConflictReason::Semver = r { + msg.push_str("\n\n previously selected "); + msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + } } - // p == parent so the full path is redundant. - } - - if !other_errors.is_empty() { - msg.push_str( - "\n\nall possible versions conflict with \ - previously selected packages.", - ); - } - - for &(p, _) in other_errors.iter() { - msg.push_str("\n\n previously selected "); - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); } msg.push_str("\n\nfailed to select a version for `"); From bcfdf9fbad78cf0e9ec52e84ce68f7b2a5aa19da Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 20 Oct 2020 11:15:48 -0700 Subject: [PATCH 4/9] New namespaced features implementation. --- crates/cargo-test-support/src/lib.rs | 2 +- crates/resolver-tests/src/lib.rs | 30 +- crates/resolver-tests/tests/resolve.rs | 1 + src/bin/cargo/cli.rs | 15 +- src/cargo/core/compiler/unit_dependencies.rs | 14 +- src/cargo/core/features.rs | 5 +- src/cargo/core/package.rs | 14 +- src/cargo/core/resolver/dep_cache.rs | 294 ++++--- src/cargo/core/resolver/errors.rs | 20 +- src/cargo/core/resolver/features.rs | 114 ++- src/cargo/core/resolver/types.rs | 10 +- src/cargo/core/summary.rs | 467 +++++----- src/cargo/ops/cargo_compile.rs | 60 +- src/cargo/ops/registry.rs | 2 +- src/cargo/ops/tree/graph.rs | 41 +- src/cargo/sources/registry/index.rs | 7 +- src/cargo/util/toml/mod.rs | 25 +- src/doc/src/reference/unstable.md | 63 +- tests/testsuite/features.rs | 34 +- tests/testsuite/features2.rs | 6 +- tests/testsuite/features_namespaced.rs | 862 +++++++++++++++++-- tests/testsuite/help.rs | 2 +- tests/testsuite/package_features.rs | 2 +- tests/testsuite/rename_deps.rs | 2 +- 24 files changed, 1503 insertions(+), 589 deletions(-) diff --git a/crates/cargo-test-support/src/lib.rs b/crates/cargo-test-support/src/lib.rs index 1973b5c69e5..bc02bf6b9db 100644 --- a/crates/cargo-test-support/src/lib.rs +++ b/crates/cargo-test-support/src/lib.rs @@ -1218,7 +1218,7 @@ enum MatchKind { /// Compares a line with an expected pattern. /// - Use `[..]` as a wildcard to match 0 or more characters on the same line -/// (similar to `.*` in a regex). +/// (similar to `.*` in a regex). It is non-greedy. /// - Use `[EXE]` to optionally add `.exe` on Windows (empty string on other /// platforms). /// - There is a wide range of macros (such as `[COMPILING]` or `[WARNING]`) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index a1cdb9d11a2..b985aebaaf7 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -170,14 +170,7 @@ pub fn resolve_with_config_raw( list: registry, used: HashSet::new(), }; - let summary = Summary::new( - pkg_id("root"), - deps, - &BTreeMap::>::new(), - None::<&String>, - false, - ) - .unwrap(); + let summary = Summary::new(pkg_id("root"), deps, &BTreeMap::new(), None::<&String>).unwrap(); let opts = ResolveOpts::everything(); let start = Instant::now(); let resolve = resolver::resolve( @@ -571,14 +564,7 @@ pub fn pkg_dep(name: T, dep: Vec) -> Summary { } else { None }; - Summary::new( - name.to_pkgid(), - dep, - &BTreeMap::>::new(), - link, - false, - ) - .unwrap() + Summary::new(name.to_pkgid(), dep, &BTreeMap::new(), link).unwrap() } pub fn pkg_id(name: &str) -> PackageId { @@ -599,14 +585,7 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary { } else { None }; - Summary::new( - pkg_id_loc(name, loc), - Vec::new(), - &BTreeMap::>::new(), - link, - false, - ) - .unwrap() + Summary::new(pkg_id_loc(name, loc), Vec::new(), &BTreeMap::new(), link).unwrap() } pub fn remove_dep(sum: &Summary, ind: usize) -> Summary { @@ -616,9 +595,8 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary { Summary::new( sum.package_id(), deps, - &BTreeMap::>::new(), + &BTreeMap::new(), sum.links().map(|a| a.as_str()), - sum.namespaced_features(), ) .unwrap() } diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 337f422ab65..c2ee408f485 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -228,6 +228,7 @@ proptest! { } #[test] +#[should_panic(expected = "pub dep")] // The error handling is not yet implemented. fn pub_fail() { let input = vec![ pkg!(("a", "0.0.4")), diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 83303e50cca..4c18f3e68a6 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -35,13 +35,14 @@ pub fn main(config: &mut Config) -> CliResult { " Available unstable (nightly-only) flags: - -Z avoid-dev-deps -- Avoid installing dev-dependencies if possible - -Z minimal-versions -- Install minimal dependency versions instead of maximum - -Z no-index-update -- Do not update the registry, avoids a network request for benchmarking - -Z unstable-options -- Allow the usage of unstable options - -Z timings -- Display concurrency information - -Z doctest-xcompile -- Compile and run doctests for non-host target using runner config - -Z terminal-width -- Provide a terminal width to rustc for error truncation + -Z avoid-dev-deps -- Avoid installing dev-dependencies if possible + -Z minimal-versions -- Install minimal dependency versions instead of maximum + -Z no-index-update -- Do not update the registry, avoids a network request for benchmarking + -Z unstable-options -- Allow the usage of unstable options + -Z timings -- Display concurrency information + -Z doctest-xcompile -- Compile and run doctests for non-host target using runner config + -Z terminal-width -- Provide a terminal width to rustc for error truncation + -Z namespaced-features -- Allow features with `crate:` prefix Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" ); diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 465f841a807..b6ae6df45ec 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -713,6 +713,16 @@ impl<'a, 'cfg> State<'a, 'cfg> { features.activated_features(pkg_id, features_for) } + fn is_dep_activated( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + dep_name: InternedString, + ) -> bool { + self.features() + .is_dep_activated(pkg_id, features_for, dep_name) + } + fn get(&self, id: PackageId) -> &'a Package { self.package_set .get_one(id) @@ -738,9 +748,7 @@ impl<'a, 'cfg> State<'a, 'cfg> { // did not enable it, don't include it. if dep.is_optional() { let features_for = unit_for.map_to_features_for(); - - let feats = self.activated_features(pkg_id, features_for); - if !feats.contains(&dep.name_in_toml()) { + if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) { return false; } } diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 642e6efec18..5d3aa4db8cf 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -197,9 +197,6 @@ features! { // Overriding profiles for dependencies. [stable] profile_overrides: bool, - // Separating the namespaces for features and dependencies - [unstable] namespaced_features: bool, - // "default-run" manifest option, [stable] default_run: bool, @@ -360,6 +357,7 @@ pub struct CliUnstable { pub multitarget: bool, pub rustdoc_map: bool, pub terminal_width: Option>, + pub namespaced_features: bool, } fn deserialize_build_std<'de, D>(deserializer: D) -> Result>, D::Error> @@ -465,6 +463,7 @@ impl CliUnstable { "multitarget" => self.multitarget = parse_empty(k, v)?, "rustdoc-map" => self.rustdoc_map = parse_empty(k, v)?, "terminal-width" => self.terminal_width = Some(parse_usize_opt(v)?), + "namespaced-features" => self.namespaced_features = parse_empty(k, v)?, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 27e112f6f61..90722692b34 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,6 +1,6 @@ use std::cell::{Cell, Ref, RefCell, RefMut}; use std::cmp::Ordering; -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt; use std::hash; use std::mem; @@ -23,7 +23,7 @@ use crate::core::dependency::DepKind; use crate::core::resolver::{HasDevUnits, Resolve}; use crate::core::source::MaybePackage; use crate::core::{Dependency, Manifest, PackageId, SourceId, Target}; -use crate::core::{FeatureMap, SourceMap, Summary, Workspace}; +use crate::core::{SourceMap, Summary, Workspace}; use crate::ops; use crate::util::config::PackageCacheLock; use crate::util::errors::{CargoResult, CargoResultExt, HttpNot200}; @@ -87,7 +87,7 @@ struct SerializedPackage<'a> { source: SourceId, dependencies: &'a [Dependency], targets: Vec<&'a Target>, - features: &'a FeatureMap, + features: &'a BTreeMap>, manifest_path: &'a Path, metadata: Option<&'a toml::Value>, publish: Option<&'a Vec>, @@ -131,6 +131,12 @@ impl ser::Serialize for Package { .iter() .filter(|t| t.src_path().is_path()) .collect(); + let empty_feats = BTreeMap::new(); + let features = self + .manifest() + .original() + .features() + .unwrap_or(&empty_feats); SerializedPackage { name: &*package_id.name(), @@ -142,7 +148,7 @@ impl ser::Serialize for Package { source: summary.source_id(), dependencies: summary.dependencies(), targets, - features: summary.features(), + features, manifest_path: self.manifest_path(), metadata: self.manifest().custom_metadata(), authors, diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 4dd9e7c94a3..8a598d79b22 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -12,7 +12,7 @@ use crate::core::resolver::context::Context; use crate::core::resolver::errors::describe_path; use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet}; -use crate::core::resolver::{ActivateResult, ResolveOpts}; +use crate::core::resolver::{ActivateError, ActivateResult, ResolveOpts}; use crate::core::{Dependency, FeatureValue, PackageId, PackageIdSpec, Registry, Summary}; use crate::core::{GitReference, SourceId}; use crate::util::errors::{CargoResult, CargoResultExt}; @@ -307,10 +307,10 @@ pub fn resolve_features<'b>( let deps = s.dependencies(); let deps = deps.iter().filter(|d| d.is_transitive() || opts.dev_deps); - let reqs = build_requirements(s, opts)?; + let reqs = build_requirements(parent, s, opts)?; let mut ret = Vec::new(); - let mut used_features = HashSet::new(); - let default_dep = (false, BTreeSet::new()); + let default_dep = BTreeSet::new(); + let mut valid_dep_names = HashSet::new(); // Next, collect all actually enabled dependencies and their features. for dep in deps { @@ -319,33 +319,15 @@ pub fn resolve_features<'b>( if dep.is_optional() && !reqs.deps.contains_key(&dep.name_in_toml()) { continue; } + valid_dep_names.insert(dep.name_in_toml()); // So we want this dependency. Move the features we want from // `feature_deps` to `ret` and register ourselves as using this // name. - let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep); - used_features.insert(dep.name_in_toml()); - let always_required = !dep.is_optional() - && !s - .dependencies() - .iter() - .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml()); - if always_required && base.0 { - return Err(match parent { - None => anyhow::format_err!( - "Package `{}` does not have feature `{}`. It has a required dependency \ - with that name, but only optional dependencies can be used as features.", - s.package_id(), - dep.name_in_toml() - ) - .into(), - Some(p) => ( - p, - ConflictReason::RequiredDependencyAsFeatures(dep.name_in_toml()), - ) - .into(), - }); - } - let mut base = base.1.clone(); + let mut base = reqs + .deps + .get(&dep.name_in_toml()) + .unwrap_or(&default_dep) + .clone(); base.extend(dep.features().iter()); for feature in base.iter() { if feature.contains('/') { @@ -359,74 +341,88 @@ pub fn resolve_features<'b>( ret.push((dep.clone(), Rc::new(base))); } - // Any entries in `reqs.dep` which weren't used are bugs in that the - // package does not actually have those dependencies. We classified - // them as dependencies in the first place because there is no such - // feature, either. - let remaining = reqs - .deps - .keys() - .cloned() - .filter(|s| !used_features.contains(s)) - .collect::>(); - if !remaining.is_empty() { - let features = remaining.join(", "); - return Err(match parent { - None => anyhow::format_err!( - "Package `{}` does not have these features: `{}`", - s.package_id(), - features - ) - .into(), - Some(p) => (p, ConflictReason::MissingFeatures(features)).into(), - }); + // This is a special case for command-line `--features + // crate_name/feat_name` where `crate_name` does not exist. All other + // validation is done either in `build_requirements` or + // `build_feature_map`. + for dep_name in reqs.deps.keys() { + if !valid_dep_names.contains(dep_name) { + let e = RequirementError::MissingDependency(*dep_name); + return Err(e.into_activate_error(parent, s)); + } } - Ok((reqs.into_used(), ret)) + Ok((reqs.into_features(), ret)) } /// Takes requested features for a single package from the input `ResolveOpts` and /// recurses to find all requested features, dependencies and requested /// dependency features in a `Requirements` object, returning it to the resolver. fn build_requirements<'a, 'b: 'a>( + parent: Option, s: &'a Summary, opts: &'b ResolveOpts, -) -> CargoResult> { +) -> ActivateResult> { let mut reqs = Requirements::new(s); if opts.features.all_features { for key in s.features().keys() { - reqs.require_feature(*key)?; - } - for dep in s.dependencies().iter().filter(|d| d.is_optional()) { - reqs.require_dependency(dep.name_in_toml()); + if let Err(e) = reqs.require_feature(*key) { + return Err(e.into_activate_error(parent, s)); + } } } else { for &f in opts.features.features.iter() { - reqs.require_value(&FeatureValue::new(f, s))?; + let fv = FeatureValue::new(f); + if fv.is_explicit_crate() { + return Err(ActivateError::Fatal(anyhow::format_err!( + "feature value `{}` is not allowed to use explicit `crate:` syntax", + fv + ))); + } + if let Err(e) = reqs.require_value(&fv) { + return Err(e.into_activate_error(parent, s)); + } } } if opts.features.uses_default_features && s.features().contains_key("default") { - reqs.require_feature(InternedString::new("default"))?; + if let Err(e) = reqs.require_feature(InternedString::new("default")) { + return Err(e.into_activate_error(parent, s)); + } } Ok(reqs) } +/// Set of feature and dependency requirements for a package. +#[derive(Debug)] struct Requirements<'a> { summary: &'a Summary, - // The deps map is a mapping of package name to list of features enabled. - // Each package should be enabled, and each package should have the - // specified set of features enabled. The boolean indicates whether this - // package was specifically requested (rather than just requesting features - // *within* this package). - deps: HashMap)>, - // The used features set is the set of features which this local package had - // enabled, which is later used when compiling to instruct the code what - // features were enabled. - used: HashSet, - visited: HashSet, + /// The deps map is a mapping of dependency name to list of features enabled. + /// + /// The resolver will activate all of these dependencies, with the given + /// features enabled. + deps: HashMap>, + /// The set of features enabled on this package which is later used when + /// compiling to instruct the code what features were enabled. + features: HashSet, +} + +/// An error for a requirement. +/// +/// This will later be converted to an `ActivateError` depending on whether or +/// not this is a dependency or a root package. +enum RequirementError { + /// The package does not have the requested feature. + MissingFeature(InternedString), + /// The package does not have the requested dependency. + MissingDependency(InternedString), + /// A feature has a direct cycle to itself. + /// + /// Note that cycles through multiple features are allowed (but perhaps + /// they shouldn't be?). + Cycle(InternedString), } impl Requirements<'_> { @@ -434,80 +430,146 @@ impl Requirements<'_> { Requirements { summary, deps: HashMap::new(), - used: HashSet::new(), - visited: HashSet::new(), + features: HashSet::new(), } } - fn into_used(self) -> HashSet { - self.used + fn into_features(self) -> HashSet { + self.features } - fn require_crate_feature(&mut self, package: InternedString, feat: InternedString) { + fn require_crate_feature( + &mut self, + package: InternedString, + feat: InternedString, + explicit: bool, + ) -> Result<(), RequirementError> { // If `package` is indeed an optional dependency then we activate the // feature named `package`, but otherwise if `package` is a required // dependency then there's no feature associated with it. - if self - .summary - .dependencies() - .iter() - .any(|dep| dep.name_in_toml() == package && dep.is_optional()) + if !explicit + && self + .summary + .dependencies() + .iter() + .any(|dep| dep.name_in_toml() == package && dep.is_optional()) { - self.used.insert(package); - } - self.deps - .entry(package) - .or_insert((false, BTreeSet::new())) - .1 - .insert(feat); - } - - fn seen(&mut self, feat: InternedString) -> bool { - if self.visited.insert(feat) { - self.used.insert(feat); - false - } else { - true + self.require_feature(package)?; } + self.deps.entry(package).or_default().insert(feat); + Ok(()) } fn require_dependency(&mut self, pkg: InternedString) { - if self.seen(pkg) { - return; - } - self.deps.entry(pkg).or_insert((false, BTreeSet::new())).0 = true; + self.deps.entry(pkg).or_default(); } - fn require_feature(&mut self, feat: InternedString) -> CargoResult<()> { - if feat.is_empty() || self.seen(feat) { + fn require_feature(&mut self, feat: InternedString) -> Result<(), RequirementError> { + if !self.features.insert(feat) { + // Already seen this feature. return Ok(()); } - for fv in self - .summary - .features() - .get(&feat) - .expect("must be a valid feature") - { - match *fv { - FeatureValue::Feature(ref dep_feat) if **dep_feat == *feat => anyhow::bail!( - "cyclic feature dependency: feature `{}` depends on itself", - feat - ), - _ => {} + + let fvs = match self.summary.features().get(&feat) { + Some(fvs) => fvs, + None => return Err(RequirementError::MissingFeature(feat)), + }; + + for fv in fvs { + if let FeatureValue::Feature(dep_feat) = fv { + if *dep_feat == feat { + return Err(RequirementError::Cycle(feat)); + } } self.require_value(fv)?; } Ok(()) } - fn require_value(&mut self, fv: &FeatureValue) -> CargoResult<()> { + fn require_value(&mut self, fv: &FeatureValue) -> Result<(), RequirementError> { match fv { FeatureValue::Feature(feat) => self.require_feature(*feat)?, - FeatureValue::Crate(dep) => self.require_dependency(*dep), - FeatureValue::CrateFeature(dep, dep_feat) => { - self.require_crate_feature(*dep, *dep_feat) - } + FeatureValue::Crate { dep_name } => self.require_dependency(*dep_name), + FeatureValue::CrateFeature { + dep_name, + dep_feature, + explicit, + } => self.require_crate_feature(*dep_name, *dep_feature, *explicit)?, }; Ok(()) } } + +impl RequirementError { + fn into_activate_error(self, parent: Option, summary: &Summary) -> ActivateError { + match self { + RequirementError::MissingFeature(feat) => { + let deps: Vec<_> = summary + .dependencies() + .iter() + .filter(|dep| dep.name_in_toml() == feat) + .collect(); + if deps.is_empty() { + return match parent { + None => ActivateError::Fatal(anyhow::format_err!( + "Package `{}` does not have the feature `{}`", + summary.package_id(), + feat + )), + Some(p) => ActivateError::Conflict( + p, + ConflictReason::MissingFeatures(feat.to_string()), + ), + }; + } + if deps.iter().any(|dep| dep.is_optional()) { + match parent { + None => ActivateError::Fatal(anyhow::format_err!( + "Package `{}` does not have feature `{}`. It has an optional dependency \ + with that name, but that dependency uses the \"crate:\" \ + syntax in the features table, so it does not have an implicit feature with that name.", + summary.package_id(), + feat + )), + Some(p) => ActivateError::Conflict( + p, + ConflictReason::NonImplicitDependencyAsFeature(feat), + ), + } + } else { + match parent { + None => ActivateError::Fatal(anyhow::format_err!( + "Package `{}` does not have feature `{}`. It has a required dependency \ + with that name, but only optional dependencies can be used as features.", + summary.package_id(), + feat + )), + Some(p) => ActivateError::Conflict( + p, + ConflictReason::RequiredDependencyAsFeature(feat), + ), + } + } + } + RequirementError::MissingDependency(dep_name) => { + match parent { + None => ActivateError::Fatal(anyhow::format_err!( + "package `{}` does not have a dependency named `{}`", + summary.package_id(), + dep_name + )), + // This code path currently isn't used, since `foo/bar` + // and `crate:` syntax is not allowed in a dependency. + Some(p) => ActivateError::Conflict( + p, + ConflictReason::MissingFeatures(dep_name.to_string()), + ), + } + } + RequirementError::Cycle(feat) => ActivateError::Fatal(anyhow::format_err!( + "cyclic feature dependency: feature `{}` depends on itself", + feat + )), + } + } +} diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index 9ad230f7f80..8f5dc7cd0b4 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -142,7 +142,7 @@ pub(super) fn activation_error( msg.push_str("` does not have these features.\n"); // p == parent so the full path is redundant. } - ConflictReason::RequiredDependencyAsFeatures(features) => { + ConflictReason::RequiredDependencyAsFeature(features) => { msg.push_str("\n\nthe package `"); msg.push_str(&*p.name()); msg.push_str("` depends on `"); @@ -158,6 +158,24 @@ pub(super) fn activation_error( ); // p == parent so the full path is redundant. } + ConflictReason::NonImplicitDependencyAsFeature(features) => { + msg.push_str("\n\nthe package `"); + msg.push_str(&*p.name()); + msg.push_str("` depends on `"); + msg.push_str(&*dep.package_name()); + msg.push_str("`, with features: `"); + msg.push_str(features); + msg.push_str("` but `"); + msg.push_str(&*dep.package_name()); + msg.push_str("` does not have these features.\n"); + msg.push_str( + " It has an optional dependency with that name, \ + but but that dependency uses the \"crate:\" \ + syntax in the features table, so it does not have an \ + implicit feature with that name.\n", + ); + // p == parent so the full path is redundant. + } ConflictReason::PublicDependency(pkg_id) => { // TODO: This needs to be implemented. unimplemented!("pub dep {:?}", pkg_id); diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 026f0af51f3..113d18461cd 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -57,8 +57,18 @@ type ActivateMap = HashMap<(PackageId, bool), BTreeSet>; /// Set of all activated features for all packages in the resolve graph. pub struct ResolvedFeatures { activated_features: ActivateMap, + /// Optional dependencies that should be built. + /// + /// The value is the `name_in_toml` of the dependencies. + activated_dependencies: ActivateMap, /// This is only here for legacy support when `-Zfeatures` is not enabled. - legacy: Option>>, + /// + /// This is the set of features enabled for each package. + legacy_features: Option>>, + /// This is only here for legacy support when `-Zfeatures` is not enabled. + /// + /// This is the set of optional dependencies enabled for each package. + legacy_dependencies: Option>>, opts: FeatureOpts, } @@ -227,6 +237,30 @@ impl ResolvedFeatures { .expect("activated_features for invalid package") } + /// Returns if the given dependency should be included. + /// + /// This handles dependencies disabled via `cfg` expressions and optional + /// dependencies which are not enabled. + pub fn is_dep_activated( + &self, + pkg_id: PackageId, + features_for: FeaturesFor, + dep_name: InternedString, + ) -> bool { + if let Some(legacy) = &self.legacy_dependencies { + legacy + .get(&pkg_id) + .map(|deps| deps.contains(&dep_name)) + .unwrap_or(false) + } else { + let is_build = self.opts.decouple_host_deps && features_for == FeaturesFor::HostDep; + self.activated_dependencies + .get(&(pkg_id, is_build)) + .map(|deps| deps.contains(&dep_name)) + .unwrap_or(false) + } + } + /// Variant of `activated_features` that returns `None` if this is /// not a valid pkg_id/is_build combination. Used in places which do /// not know which packages are activated (like `cargo clean`). @@ -243,7 +277,7 @@ impl ResolvedFeatures { pkg_id: PackageId, features_for: FeaturesFor, ) -> CargoResult> { - if let Some(legacy) = &self.legacy { + if let Some(legacy) = &self.legacy_features { Ok(legacy.get(&pkg_id).map_or_else(Vec::new, |v| v.clone())) } else { let is_build = self.opts.decouple_host_deps && features_for == FeaturesFor::HostDep; @@ -267,6 +301,8 @@ pub struct FeatureResolver<'a, 'cfg> { opts: FeatureOpts, /// Map of features activated for each package. activated_features: ActivateMap, + /// Map of optional dependencies activated for each package. + activated_dependencies: ActivateMap, /// Keeps track of which packages have had its dependencies processed. /// Used to avoid cycles, and to speed up processing. processed_deps: HashSet<(PackageId, bool)>, @@ -294,7 +330,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Legacy mode. return Ok(ResolvedFeatures { activated_features: HashMap::new(), - legacy: Some(resolve.features_clone()), + activated_dependencies: HashMap::new(), + legacy_features: Some(resolve.features_clone()), + legacy_dependencies: Some(compute_legacy_deps(resolve)), opts, }); } @@ -306,6 +344,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { package_set, opts, activated_features: HashMap::new(), + activated_dependencies: HashMap::new(), processed_deps: HashSet::new(), }; r.do_resolve(specs, requested_features)?; @@ -315,7 +354,9 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } Ok(ResolvedFeatures { activated_features: r.activated_features, - legacy: None, + activated_dependencies: r.activated_dependencies, + legacy_features: None, + legacy_dependencies: None, opts: r.opts, }) } @@ -402,9 +443,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { FeatureValue::Feature(f) => { self.activate_rec(pkg_id, *f, for_host)?; } - FeatureValue::Crate(dep_name) => { - // Activate the feature name on self. - self.activate_rec(pkg_id, *dep_name, for_host)?; + FeatureValue::Crate { dep_name } => { + // Mark this dependency as activated. + self.activated_dependencies + .entry((pkg_id, self.opts.decouple_host_deps && for_host)) + .or_default() + .insert(*dep_name); // Activate the optional dep. for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) { for (dep, dep_for_host) in deps { @@ -416,7 +460,11 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } } } - FeatureValue::CrateFeature(dep_name, dep_feature) => { + FeatureValue::CrateFeature { + dep_name, + dep_feature, + explicit, + } => { // Activate a feature within a dependency. for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) { for (dep, dep_for_host) in deps { @@ -425,12 +473,20 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } if dep.is_optional() { // Activate the crate on self. - let fv = FeatureValue::Crate(*dep_name); + let fv = FeatureValue::Crate { + dep_name: *dep_name, + // explicit: *explicit, + }; self.activate_fv(pkg_id, &fv, for_host)?; + if !explicit { + // To retain compatibility with old behavior, + // this also enables a feature of the same + // name. + self.activate_rec(pkg_id, *dep_name, for_host)?; + } } // Activate the feature on the dependency. - let summary = self.resolve.summary(dep_pkg_id); - let fv = FeatureValue::new(*dep_feature, summary); + let fv = FeatureValue::new(*dep_feature); self.activate_fv(dep_pkg_id, &fv, dep_for_host)?; } } @@ -484,7 +540,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { let mut result: Vec = dep .features() .iter() - .map(|f| FeatureValue::new(*f, summary)) + .map(|f| FeatureValue::new(*f)) .collect(); let default = InternedString::new("default"); if dep.uses_default_features() && feature_map.contains_key(&default) { @@ -502,28 +558,16 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { let summary = self.resolve.summary(pkg_id); let feature_map = summary.features(); if requested_features.all_features { - let mut fvs: Vec = feature_map + feature_map .keys() .map(|k| FeatureValue::Feature(*k)) - .collect(); - // Add optional deps. - // Top-level requested features can never apply to - // build-dependencies, so for_host is `false` here. - for (_dep_pkg_id, deps) in self.deps(pkg_id, false) { - for (dep, _dep_for_host) in deps { - if dep.is_optional() { - // This may result in duplicates, but that should be ok. - fvs.push(FeatureValue::Crate(dep.name_in_toml())); - } - } - } - fvs + .collect() } else { let mut result: Vec = requested_features .features .as_ref() .iter() - .map(|f| FeatureValue::new(*f, summary)) + .map(|f| FeatureValue::new(*f)) .collect(); let default = InternedString::new("default"); if requested_features.uses_default_features && feature_map.contains_key(&default) { @@ -612,3 +656,19 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { .proc_macro() } } + +/// Computes a map of PackageId to the set of optional dependencies that are +/// enabled for that dep (when the new resolver is not enabled). +fn compute_legacy_deps(resolve: &Resolve) -> HashMap> { + let mut result: HashMap> = HashMap::new(); + for pkg_id in resolve.iter() { + for (_dep_id, deps) in resolve.deps(pkg_id) { + for dep in deps { + if dep.is_optional() { + result.entry(pkg_id).or_default().insert(dep.name_in_toml()); + } + } + } + } + result +} diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index dabe8c094e1..fd53e856cb7 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -290,11 +290,15 @@ pub enum ConflictReason { /// candidate we're activating didn't actually have the feature `foo`. MissingFeatures(String), - /// A dependency listed features that ended up being a required dependency. + /// A dependency listed a feature that ended up being a required dependency. /// For example we tried to activate feature `foo` but the /// candidate we're activating didn't actually have the feature `foo` /// it had a dependency `foo` instead. - RequiredDependencyAsFeatures(InternedString), + RequiredDependencyAsFeature(InternedString), + + /// A dependency listed a feature for an optional dependency, but that + /// optional dependency is "hidden" using namespaced `crate:` syntax. + NonImplicitDependencyAsFeature(InternedString), // TODO: needs more info for `activation_error` // TODO: needs more info for `find_candidate` @@ -319,7 +323,7 @@ impl ConflictReason { } pub fn is_required_dependency_as_features(&self) -> bool { - if let ConflictReason::RequiredDependencyAsFeatures(_) = *self { + if let ConflictReason::RequiredDependencyAsFeature(_) = *self { return true; } false diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 70774577a8a..0ca8e2cbc64 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -1,17 +1,13 @@ -use std::borrow::Borrow; -use std::collections::{BTreeMap, HashMap}; -use std::fmt::Display; -use std::hash::{Hash, Hasher}; -use std::mem; -use std::rc::Rc; - -use serde::{Serialize, Serializer}; - use crate::core::{Dependency, PackageId, SourceId}; use crate::util::interning::InternedString; -use semver::Version; - use crate::util::CargoResult; +use anyhow::bail; +use semver::Version; +use std::collections::{BTreeMap, HashMap, HashSet}; +use std::fmt; +use std::hash::{Hash, Hasher}; +use std::mem; +use std::rc::Rc; /// Subset of a `Manifest`. Contains only the most important information about /// a package. @@ -27,39 +23,33 @@ struct Inner { package_id: PackageId, dependencies: Vec, features: Rc, + has_namespaced_features: bool, + has_overlapping_features: Option, checksum: Option, links: Option, - namespaced_features: bool, } impl Summary { - pub fn new( + pub fn new( pkg_id: PackageId, dependencies: Vec, - features: &BTreeMap>>, + features: &BTreeMap>, links: Option>, - namespaced_features: bool, - ) -> CargoResult - where - K: Borrow + Ord + Display, - { + ) -> CargoResult { + let mut has_overlapping_features = None; for dep in dependencies.iter() { - let feature = dep.name_in_toml(); - if !namespaced_features && features.get(&*feature).is_some() { - anyhow::bail!( - "Features and dependencies cannot have the \ - same name: `{}`", - feature - ) + let dep_name = dep.name_in_toml(); + if features.contains_key(&dep_name) { + has_overlapping_features = Some(dep_name); } if dep.is_optional() && !dep.is_transitive() { - anyhow::bail!( - "Dev-dependencies are not allowed to be optional: `{}`", - feature + bail!( + "dev-dependencies are not allowed to be optional: `{}`", + dep_name ) } } - let feature_map = build_feature_map(features, &dependencies, namespaced_features)?; + let (feature_map, has_namespaced_features) = build_feature_map(features, &dependencies)?; Ok(Summary { inner: Rc::new(Inner { package_id: pkg_id, @@ -67,7 +57,8 @@ impl Summary { features: Rc::new(feature_map), checksum: None, links: links.map(|l| l.into()), - namespaced_features, + has_namespaced_features, + has_overlapping_features, }), }) } @@ -90,15 +81,33 @@ impl Summary { pub fn features(&self) -> &FeatureMap { &self.inner.features } + + /// Returns an error if this Summary is using an unstable feature that is + /// not enabled. + pub fn unstable_gate(&self, namespaced_features: bool) -> CargoResult<()> { + if !namespaced_features { + if self.inner.has_namespaced_features { + bail!( + "namespaced features with the `crate:` prefix are only allowed on \ + the nightly channel and requires the `-Z namespaced-features` flag on the command-line" + ); + } + if let Some(dep_name) = self.inner.has_overlapping_features { + bail!( + "features and dependencies cannot have the same name: `{}`", + dep_name + ) + } + } + Ok(()) + } + pub fn checksum(&self) -> Option<&str> { self.inner.checksum.as_deref() } pub fn links(&self) -> Option { self.inner.links } - pub fn namespaced_features(&self) -> bool { - self.inner.namespaced_features - } pub fn override_id(mut self, id: PackageId) -> Summary { Rc::make_mut(&mut self.inner).package_id = id; @@ -145,16 +154,16 @@ impl Hash for Summary { } } -// Checks features for errors, bailing out a CargoResult:Err if invalid, -// and creates FeatureValues for each feature. -fn build_feature_map( - features: &BTreeMap>>, +/// Checks features for errors, bailing out a CargoResult:Err if invalid, +/// and creates FeatureValues for each feature. +/// +/// The returned `bool` indicates whether or not the `[features]` table +/// included a `crate:` prefixed namespaced feature (used for gating on +/// nightly). +fn build_feature_map( + features: &BTreeMap>, dependencies: &[Dependency], - namespaced: bool, -) -> CargoResult -where - K: Borrow + Ord + Display, -{ +) -> CargoResult<(FeatureMap, bool)> { use self::FeatureValue::*; let mut dep_map = HashMap::new(); for dep in dependencies.iter() { @@ -164,54 +173,62 @@ where .push(dep); } - let mut map = BTreeMap::new(); - for (feature, list) in features.iter() { - // If namespaced features is active and the key is the same as that of an - // optional dependency, that dependency must be included in the values. - // Thus, if a `feature` is found that has the same name as a dependency, we - // (a) bail out if the dependency is non-optional, and (b) we track if the - // feature requirements include the dependency `crate:feature` in the list. - // This is done with the `dependency_found` variable, which can only be - // false if features are namespaced and the current feature key is the same - // as the name of an optional dependency. If so, it gets set to true during - // iteration over the list if the dependency is found in the list. - let mut dependency_found = if namespaced { - match dep_map.get(feature.borrow()) { - Some(dep_data) => { - if !dep_data.iter().any(|d| d.is_optional()) { - anyhow::bail!( - "Feature `{}` includes the dependency of the same name, but this is \ - left implicit in the features included by this feature.\n\ - Additionally, the dependency must be marked as optional to be \ - included in the feature definition.\n\ - Consider adding `crate:{}` to this feature's requirements \ - and marking the dependency as `optional = true`", - feature, - feature - ) - } else { - false - } - } - None => true, - } - } else { - true + let mut map: FeatureMap = features + .iter() + .map(|(feature, list)| { + let fvs: Vec<_> = list + .iter() + .map(|feat_value| FeatureValue::new(*feat_value)) + .collect(); + (*feature, fvs) + }) + .collect(); + let has_namespaced_features = map.values().flatten().any(|fv| fv.is_explicit_crate()); + + // Add implicit features for optional dependencies if they weren't + // explicitly listed anywhere. + let explicitly_listed: HashSet<_> = map + .values() + .flatten() + .filter_map(|fv| match fv { + Crate { dep_name } + | CrateFeature { + dep_name, + explicit: true, + .. + } => Some(*dep_name), + _ => None, + }) + .collect(); + for dep in dependencies { + if !dep.is_optional() { + continue; + } + let dep_name_in_toml = dep.name_in_toml(); + if features.contains_key(&dep_name_in_toml) || explicitly_listed.contains(&dep_name_in_toml) + { + continue; + } + let fv = Crate { + dep_name: dep_name_in_toml, }; + map.insert(dep_name_in_toml, vec![fv]); + } - let mut values = vec![]; - for dep in list { - let val = FeatureValue::build( - InternedString::new(dep.as_ref()), - |fs| features.contains_key(fs.as_str()), - namespaced, + // Validate features are listed properly. + for (feature, fvs) in &map { + if feature.starts_with("crate:") { + bail!( + "feature named `{}` is not allowed to start with `crate:`", + feature ); - + } + for fv in fvs { // Find data for the referenced dependency... let dep_data = { - match val { - Feature(ref dep_name) | Crate(ref dep_name) | CrateFeature(ref dep_name, _) => { - dep_map.get(dep_name.as_str()) + match fv { + Feature(dep_name) | Crate { dep_name, .. } | CrateFeature { dep_name, .. } => { + dep_map.get(dep_name) } } }; @@ -219,198 +236,160 @@ where .iter() .flat_map(|d| d.iter()) .any(|d| d.is_optional()); - if let FeatureValue::Crate(ref dep_name) = val { - // If we have a dependency value, check if this is the dependency named - // the same as the feature that we were looking for. - if !dependency_found && feature.borrow() == dep_name.as_str() { - dependency_found = true; - } - } - - match (&val, dep_data.is_some(), is_optional_dep) { - // The value is a feature. If features are namespaced, this just means - // it's not prefixed with `crate:`, so we have to check whether the - // feature actually exist. If the feature is not defined *and* an optional - // dependency of the same name exists, the feature is defined implicitly - // here by adding it to the feature map, pointing to the dependency. - // If features are not namespaced, it's been validated as a feature already - // while instantiating the `FeatureValue` in `FeatureValue::build()`, so - // we don't have to do so here. - (&Feature(feat), _, true) => { - if namespaced && !features.contains_key(&*feat) { - map.insert(feat, vec![FeatureValue::Crate(feat)]); - } - } - // If features are namespaced and the value is not defined as a feature - // and there is no optional dependency of the same name, error out. - // If features are not namespaced, there must be an existing feature - // here (checked by `FeatureValue::build()`), so it will always be defined. - (&Feature(feat), dep_exists, false) => { - if namespaced && !features.contains_key(&*feat) { - if dep_exists { - anyhow::bail!( - "Feature `{}` includes `{}` which is not defined as a feature.\n\ - A non-optional dependency of the same name is defined; consider \ - adding `optional = true` to its definition", + let is_any_dep = dep_data.is_some(); + match fv { + Feature(f) => { + if !features.contains_key(f) { + if !is_any_dep { + bail!( + "feature `{}` includes `{}` which is neither a dependency \ + nor another feature", feature, - feat - ) + fv + ); + } + if is_optional_dep { + if !map.contains_key(f) { + bail!( + "feature `{}` includes `{}`, but `{}` is an \ + optional dependency without an implicit feature\n\ + Use `crate:{}` to enable the dependency.", + feature, + fv, + f, + f + ); + } } else { - anyhow::bail!( - "Feature `{}` includes `{}` which is not defined as a feature", - feature, - feat - ) + bail!("feature `{}` includes `{}`, but `{}` is not an optional dependency\n\ + A non-optional dependency of the same name is defined; \ + consider adding `optional = true` to its definition.", + feature, fv, f); } } } - // The value is a dependency. If features are namespaced, it is explicitly - // tagged as such (`crate:value`). If features are not namespaced, any value - // not recognized as a feature is pegged as a `Crate`. Here we handle the case - // where the dependency exists but is non-optional. It branches on namespaced - // just to provide the correct string for the crate dependency in the error. - (&Crate(ref dep), true, false) => { - if namespaced { - anyhow::bail!( - "Feature `{}` includes `crate:{}` which is not an \ - optional dependency.\nConsider adding \ - `optional = true` to the dependency", + Crate { dep_name } => { + if !is_any_dep { + bail!( + "feature `{}` includes `{}`, but `{}` is not listed as a dependency", feature, - dep - ) - } else { - anyhow::bail!( - "Feature `{}` depends on `{}` which is not an \ - optional dependency.\nConsider adding \ - `optional = true` to the dependency", + fv, + dep_name + ); + } + if !is_optional_dep { + bail!( + "feature `{}` includes `{}`, but `{}` is not an optional dependency\n\ + A non-optional dependency of the same name is defined; \ + consider adding `optional = true` to its definition.", feature, - dep - ) + fv, + dep_name + ); } } - // If namespaced, the value was tagged as a dependency; if not namespaced, - // this could be anything not defined as a feature. This handles the case - // where no such dependency is actually defined; again, the branch on - // namespaced here is just to provide the correct string in the error. - (&Crate(ref dep), false, _) => { - if namespaced { - anyhow::bail!( - "Feature `{}` includes `crate:{}` which is not a known \ - dependency", - feature, - dep - ) - } else { - anyhow::bail!( - "Feature `{}` includes `{}` which is neither a dependency nor \ - another feature", + CrateFeature { dep_name, .. } => { + // Validation of the feature name will be performed in the resolver. + if !is_any_dep { + bail!( + "feature `{}` includes `{}`, but `{}` is not a dependency", feature, - dep - ) + fv, + dep_name + ); } } - (&Crate(_), true, true) => {} - // If the value is a feature for one of the dependencies, bail out if no such - // dependency is actually defined in the manifest. - (&CrateFeature(ref dep, _), false, _) => anyhow::bail!( - "Feature `{}` requires a feature of `{}` which is not a \ - dependency", - feature, - dep - ), - (&CrateFeature(_, _), true, _) => {} } - values.push(val); - } - - if !dependency_found { - // If we have not found the dependency of the same-named feature, we should - // bail here. - anyhow::bail!( - "Feature `{}` includes the optional dependency of the \ - same name, but this is left implicit in the features \ - included by this feature.\nConsider adding \ - `crate:{}` to this feature's requirements.", - feature, - feature - ) } + } - map.insert(InternedString::new(feature.borrow()), values); + // Make sure every optional dep is mentioned at least once. + let used: HashSet<_> = map + .values() + .flatten() + .filter_map(|fv| match fv { + Crate { dep_name } | CrateFeature { dep_name, .. } => Some(dep_name), + _ => None, + }) + .collect(); + if let Some(dep) = dependencies + .iter() + .find(|dep| dep.is_optional() && !used.contains(&dep.name_in_toml())) + { + bail!( + "optional dependency `{}` is not included in any feature\n\ + Make sure that `crate:{}` is included in one of features in the [features] table.", + dep.name_in_toml(), + dep.name_in_toml(), + ); } - Ok(map) + + Ok((map, has_namespaced_features)) } -/// FeatureValue represents the types of dependencies a feature can have: -/// -/// * Another feature -/// * An optional dependency -/// * A feature in a dependency -/// -/// The selection between these 3 things happens as part of the construction of the FeatureValue. +/// FeatureValue represents the types of dependencies a feature can have. #[derive(Clone, Debug)] pub enum FeatureValue { + /// A feature enabling another feature. Feature(InternedString), - Crate(InternedString), - CrateFeature(InternedString, InternedString), + /// A feature enabling a dependency with `crate:dep_name` syntax. + Crate { dep_name: InternedString }, + /// A feature enabling a feature on a dependency with `crate_name/feat_name` syntax. + CrateFeature { + dep_name: InternedString, + dep_feature: InternedString, + /// If this is true, then the feature used the `crate:` prefix, which + /// prevents enabling the feature named `dep_name`. + explicit: bool, + }, } impl FeatureValue { - fn build(feature: InternedString, is_feature: T, namespaced: bool) -> FeatureValue - where - T: Fn(InternedString) -> bool, - { - match (feature.find('/'), namespaced) { - (Some(pos), _) => { + pub fn new(feature: InternedString) -> FeatureValue { + match feature.find('/') { + Some(pos) => { let (dep, dep_feat) = feature.split_at(pos); let dep_feat = &dep_feat[1..]; - FeatureValue::CrateFeature(InternedString::new(dep), InternedString::new(dep_feat)) - } - (None, true) if feature.starts_with("crate:") => { - FeatureValue::Crate(InternedString::new(&feature[6..])) - } - (None, true) => FeatureValue::Feature(feature), - (None, false) if is_feature(feature) => FeatureValue::Feature(feature), - (None, false) => FeatureValue::Crate(feature), - } - } - - pub fn new(feature: InternedString, s: &Summary) -> FeatureValue { - Self::build( - feature, - |fs| s.features().contains_key(&fs), - s.namespaced_features(), - ) - } - - pub fn to_string(&self, s: &Summary) -> String { - use self::FeatureValue::*; - match *self { - Feature(ref f) => f.to_string(), - Crate(ref c) => { - if s.namespaced_features() { - format!("crate:{}", &c) + let (dep, explicit) = if dep.starts_with("crate:") { + (&dep[6..], true) } else { - c.to_string() + (dep, false) + }; + FeatureValue::CrateFeature { + dep_name: InternedString::new(dep), + dep_feature: InternedString::new(dep_feat), + explicit, } } - CrateFeature(ref c, ref f) => [c.as_ref(), f.as_ref()].join("/"), + None if feature.starts_with("crate:") => FeatureValue::Crate { + dep_name: InternedString::new(&feature[6..]), + }, + None => FeatureValue::Feature(feature), } } + + /// Returns `true` if this feature explicitly used `crate:` syntax. + pub fn is_explicit_crate(&self) -> bool { + matches!(self, FeatureValue::Crate{..} | FeatureValue::CrateFeature{explicit:true, ..}) + } } -impl Serialize for FeatureValue { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { +impl fmt::Display for FeatureValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use self::FeatureValue::*; - match *self { - Feature(ref f) => serializer.serialize_str(f), - Crate(ref c) => serializer.serialize_str(c), - CrateFeature(ref c, ref f) => { - serializer.serialize_str(&[c.as_ref(), f.as_ref()].join("/")) - } + match self { + Feature(feat) => write!(f, "{}", feat), + Crate { dep_name } => write!(f, "crate:{}", dep_name), + CrateFeature { + dep_name, + dep_feature, + explicit: true, + } => write!(f, "crate:{}/{}", dep_name, dep_feature), + CrateFeature { + dep_name, + dep_feature, + explicit: false, + } => write!(f, "{}/{}", dep_name, dep_feature), } } } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 926553ab9b5..95d4e51f930 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -1006,8 +1006,9 @@ fn generate_targets( { let unavailable_features = match target.required_features() { Some(rf) => { - warn_on_missing_features( + validate_required_features( workspace_resolve, + target.name(), rf, pkg.summary(), &mut config.shell(), @@ -1042,8 +1043,13 @@ fn generate_targets( Ok(units.into_iter().collect()) } -fn warn_on_missing_features( +/// Warns if a target's required-features references a feature that doesn't exist. +/// +/// This is a warning because historically this was not validated, and it +/// would cause too much breakage to make it an error. +fn validate_required_features( resolve: &Option, + target_name: &str, required_features: &[String], summary: &Summary, shell: &mut Shell, @@ -1054,47 +1060,55 @@ fn warn_on_missing_features( }; for feature in required_features { - match FeatureValue::new(feature.into(), summary) { - // No need to do anything here, since the feature must exist to be parsed as such - FeatureValue::Feature(_) => {} - // Possibly mislabeled feature that was not found - FeatureValue::Crate(krate) => { - if !summary - .dependencies() - .iter() - .any(|dep| dep.name_in_toml() == krate && dep.is_optional()) - { + let fv = FeatureValue::new(feature.into()); + match &fv { + FeatureValue::Feature(f) => { + if !summary.features().contains_key(f) { shell.warn(format!( - "feature `{}` is not present in [features] section.", - krate + "invalid feature `{}` in required-features of target `{}`: \ + `{}` is not present in [features] section", + fv, target_name, fv ))?; } } + FeatureValue::Crate { .. } | FeatureValue::CrateFeature { explicit: true, .. } => { + anyhow::bail!( + "invalid feature `{}` in required-features of target `{}`: \ + explicit `crate:` feature values are not allowed in required-features", + fv, + target_name + ); + } // Handling of dependent_crate/dependent_crate_feature syntax - FeatureValue::CrateFeature(krate, feature) => { + FeatureValue::CrateFeature { + dep_name, + dep_feature, + explicit: false, + } => { match resolve .deps(summary.package_id()) - .find(|(_dep_id, deps)| deps.iter().any(|dep| dep.name_in_toml() == krate)) + .find(|(_dep_id, deps)| deps.iter().any(|dep| dep.name_in_toml() == *dep_name)) { Some((dep_id, _deps)) => { let dep_summary = resolve.summary(dep_id); - if !dep_summary.features().contains_key(&feature) + if !dep_summary.features().contains_key(dep_feature) && !dep_summary .dependencies() .iter() - .any(|dep| dep.name_in_toml() == feature && dep.is_optional()) + .any(|dep| dep.name_in_toml() == *dep_feature && dep.is_optional()) { shell.warn(format!( - "feature `{}` does not exist in package `{}`.", - feature, dep_id + "invalid feature `{}` in required-features of target `{}`: \ + feature `{}` does not exist in package `{}`", + fv, target_name, dep_feature, dep_id ))?; } } None => { shell.warn(format!( - "dependency `{}` specified in required-features as `{}/{}` \ - does not exist.", - krate, krate, feature + "invalid feature `{}` in required-features of target `{}`: \ + dependency `{}` does not exist", + fv, target_name, dep_name ))?; } } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index abdce2267e1..de583392b62 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -274,7 +274,7 @@ fn transmit( .map(|(feat, values)| { ( feat.to_string(), - values.iter().map(|fv| fv.to_string(summary)).collect(), + values.iter().map(|fv| fv.to_string()).collect(), ) }) .collect::>>(); diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index 363199a218b..ac1821269ce 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -340,7 +340,11 @@ fn add_pkg( if dep.is_optional() { // If the new feature resolver does not enable this // optional dep, then don't use it. - if !node_features.contains(&dep.name_in_toml()) { + if !resolved_features.is_dep_activated( + package_id, + features_for, + dep.name_in_toml(), + ) { return false; } } @@ -542,10 +546,10 @@ fn add_feature_rec( }; for fv in fvs { match fv { - FeatureValue::Feature(fv_name) | FeatureValue::Crate(fv_name) => { + FeatureValue::Feature(dep_name) => { let feat_index = add_feature( graph, - *fv_name, + *dep_name, Some(from), package_index, EdgeKind::Feature, @@ -553,13 +557,18 @@ fn add_feature_rec( add_feature_rec( graph, resolve, - *fv_name, + *dep_name, package_id, feat_index, package_index, ); } - FeatureValue::CrateFeature(dep_name, fv_name) => { + FeatureValue::Crate { .. } => {} + FeatureValue::CrateFeature { + dep_name, + dep_feature, + explicit, + } => { let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) { Some(indexes) => indexes.clone(), None => { @@ -569,14 +578,14 @@ fn add_feature_rec( feature_name, package_id, dep_name, - fv_name + dep_feature ); continue; } }; for (dep_index, is_optional) in dep_indexes { let dep_pkg_id = graph.package_id_for_index(dep_index); - if is_optional { + if is_optional && !explicit { // Activate the optional dep on self. add_feature( graph, @@ -586,9 +595,21 @@ fn add_feature_rec( EdgeKind::Feature, ); } - let feat_index = - add_feature(graph, *fv_name, Some(from), dep_index, EdgeKind::Feature); - add_feature_rec(graph, resolve, *fv_name, dep_pkg_id, feat_index, dep_index); + let feat_index = add_feature( + graph, + *dep_feature, + Some(from), + dep_index, + EdgeKind::Feature, + ); + add_feature_rec( + graph, + resolve, + *dep_feature, + dep_pkg_id, + feat_index, + dep_index, + ); } } } diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index d9a32f0230b..5fe8d0352d7 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -270,6 +270,7 @@ impl<'cfg> RegistryIndex<'cfg> { 'a: 'b, { let source_id = self.source_id; + let namespaced_features = self.config.cli_unstable().namespaced_features; // First up actually parse what summaries we have available. If Cargo // has run previously this will parse a Cargo-specific cache file rather @@ -294,7 +295,8 @@ impl<'cfg> RegistryIndex<'cfg> { info!("failed to parse `{}` registry package: {}", name, e); None } - })) + }) + .filter(move |is| is.summary.unstable_gate(namespaced_features).is_ok())) } fn load_summaries( @@ -723,8 +725,7 @@ impl IndexSummary { .into_iter() .map(|dep| dep.into_dep(source_id)) .collect::>>()?; - let namespaced_features = false; - let mut summary = Summary::new(pkgid, deps, &features, links, namespaced_features)?; + let mut summary = Summary::new(pkgid, deps, &features, links)?; summary.set_checksum(cksum); Ok(IndexSummary { summary, diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 72430e7ce93..0b99dc80a23 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -262,7 +262,7 @@ pub struct TomlManifest { build_dependencies: Option>, #[serde(rename = "build_dependencies")] build_dependencies2: Option>, - features: Option>>, + features: Option>>, target: Option>, replace: Option>, patch: Option>>, @@ -800,8 +800,6 @@ pub struct TomlProject { autoexamples: Option, autotests: Option, autobenches: Option, - #[serde(rename = "namespaced-features")] - namespaced_features: Option, #[serde(rename = "default-run")] default_run: Option, @@ -1190,26 +1188,15 @@ impl TomlManifest { let exclude = project.exclude.clone().unwrap_or_default(); let include = project.include.clone().unwrap_or_default(); - if project.namespaced_features.is_some() { - features.require(Feature::namespaced_features())?; - } + let empty_features = BTreeMap::new(); - let summary_features = me - .features - .as_ref() - .map(|x| { - x.iter() - .map(|(k, v)| (k.as_str(), v.iter().collect())) - .collect() - }) - .unwrap_or_else(BTreeMap::new); let summary = Summary::new( pkgid, deps, - &summary_features, + me.features.as_ref().unwrap_or(&empty_features), project.links.as_deref(), - project.namespaced_features.unwrap_or(false), )?; + summary.unstable_gate(config.cli_unstable().namespaced_features)?; let metadata = ManifestMetadata { description: project.description.clone(), @@ -1526,6 +1513,10 @@ impl TomlManifest { pub fn has_profiles(&self) -> bool { self.profile.is_some() } + + pub fn features(&self) -> Option<&BTreeMap>> { + self.features.as_ref() + } } /// Returns the name of the README file for a `TomlProject`. diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index db1b6d0dc62..6c576487e43 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -191,29 +191,62 @@ lto = true * Original issue: [#1286](https://github.com/rust-lang/cargo/issues/1286) * Tracking Issue: [#5565](https://github.com/rust-lang/cargo/issues/5565) -Currently, it is not possible to have a feature and a dependency with the same -name in the manifest. If you set `namespaced-features` to `true`, the namespaces -for features and dependencies are separated. The effect of this is that, in the -feature requirements, dependencies have to be prefixed with `crate:`. Like this: +The `namespaced-features` option makes two changes to how features can be +specified: + +* Features may now be defined with the same name as a dependency. +* Optional dependencies can be explicitly enabled in the `[features]` table + with the `crate:` prefix, which enables the dependency without enabling a + feature of the same name. + +By default, an optional dependency `foo` will define a feature `foo = +["crate:foo"]` *unless* `crate:foo` is mentioned in any other feature, or the +`foo` feature is already defined. This helps prevent unnecessary boilerplate +of listing every optional dependency, but still allows you to override the +implicit feature. + +This allows two use cases that were previously not possible: + +* You can "hide" an optional dependency, so that external users cannot + explicitly enable that optional dependency. +* There is no longer a need to create "funky" feature names to work around the + restriction that features cannot shadow dependency names. + +To enable namespaced-features, use the `-Z namespaced-features` command-line +flag. + +An example of hiding an optional dependency: ```toml -[package] -namespaced-features = true +[dependencies] +regex = { version = "1.4.1", optional = true } +lazy_static = { version = "1.4.0", optional = true } [features] -bar = ["crate:baz", "foo"] -foo = [] +regex = ["crate:regex", "crate:lazy_static"] +``` + +In this example, the "regex" feature enables both `regex` and `lazy_static`. +The `lazy_static` feature does not exist, and a user cannot explicitly enable +it. This helps hide internal details of how your package is implemented. +An example of avoiding "funky" names: + +```toml [dependencies] -baz = { version = "0.1", optional = true } -``` +bigdecimal = "0.1" +chrono = "0.4" +num-bigint = "0.2" +serde = {version = "1.0", optional = true } -To prevent unnecessary boilerplate from having to explicitly declare features -for each optional dependency, implicit features get created for any optional -dependencies where a feature of the same name is not defined. However, if -a feature of the same name as a dependency is defined, that feature must -include the dependency as a requirement, as `foo = ["crate:foo"]`. +[features] +serde = ["crate:serde", "bigdecimal/serde", "chrono/serde", "num-bigint/serde"] +``` +In this case, `serde` is a natural name to use for a feature, because it is +relevant to your exported API. However, previously you would need to use a +name like `serde1` to work around the naming limitation if you wanted to also +enable other features. ### Build-plan * Tracking Issue: [#5579](https://github.com/rust-lang/cargo/issues/5579) diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index dc346e7fed7..0097a6e0c8d 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -29,7 +29,7 @@ fn invalid1() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` includes `baz` which is neither a dependency nor another feature + feature `bar` includes `baz` which is neither a dependency nor another feature ", ) .run(); @@ -48,12 +48,15 @@ fn invalid2() { [features] bar = ["baz"] + baz = [] [dependencies.bar] - path = "foo" + path = "bar" "#, ) .file("src/main.rs", "") + .file("bar/Cargo.toml", &basic_manifest("bar", "1.0.0")) + .file("bar/src/lib.rs", "") .build(); p.cargo("build") @@ -63,7 +66,7 @@ fn invalid2() { [ERROR] failed to parse manifest at `[..]` Caused by: - Features and dependencies cannot have the same name: `bar` + features and dependencies cannot have the same name: `bar` ", ) .run(); @@ -97,8 +100,8 @@ fn invalid3() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` depends on `baz` which is not an optional dependency. - Consider adding `optional = true` to the dependency + feature `bar` includes `baz`, but `baz` is not an optional dependency + A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition. ", ) .run(); @@ -144,7 +147,7 @@ failed to select a version for `bar` which could resolve this conflict", p.cargo("build --features test") .with_status(101) - .with_stderr("error: Package `foo v0.0.1 ([..])` does not have these features: `test`") + .with_stderr("error: Package `foo v0.0.1 ([..])` does not have the feature `test`") .run(); } @@ -174,7 +177,7 @@ fn invalid5() { [ERROR] failed to parse manifest at `[..]` Caused by: - Dev-dependencies are not allowed to be optional: `bar` + dev-dependencies are not allowed to be optional: `bar` ", ) .run(); @@ -205,7 +208,7 @@ fn invalid6() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `foo` requires a feature of `bar` which is not a dependency + feature `foo` includes `bar/baz`, but `bar` is not a dependency ", ) .run(); @@ -237,7 +240,7 @@ fn invalid7() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `foo` requires a feature of `bar` which is not a dependency + feature `foo` includes `bar/baz`, but `bar` is not a dependency ", ) .run(); @@ -1183,7 +1186,7 @@ fn dep_feature_in_cmd_line() { // Trying to enable features of transitive dependencies is an error p.cargo("build --features bar/some-feat") .with_status(101) - .with_stderr("error: Package `foo v0.0.1 ([..])` does not have these features: `bar`") + .with_stderr("error: package `foo v0.0.1 ([..])` does not have a dependency named `bar`") .run(); // Hierarchical feature specification should still be disallowed @@ -1959,9 +1962,14 @@ fn nonexistent_required_features() { p.cargo("build --examples") .with_stderr_contains( -"[WARNING] feature `not_present` is not present in [features] section. -[WARNING] feature `not_existing` does not exist in package `required_dependency v0.1.0`. -[WARNING] dependency `not_specified_dependency` specified in required-features as `not_specified_dependency/some_feature` does not exist. + "\ +[WARNING] invalid feature `not_present` in required-features of target `ololo`: \ + `not_present` is not present in [features] section +[WARNING] invalid feature `required_dependency/not_existing` in required-features \ + of target `ololo`: feature `not_existing` does not exist in package \ + `required_dependency v0.1.0` +[WARNING] invalid feature `not_specified_dependency/some_feature` in required-features \ + of target `ololo`: dependency `not_specified_dependency` does not exist ", ) .run(); diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index afd72d964f2..081ac1c7a3f 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -96,6 +96,7 @@ fn inactive_target_optional() { [features] foo1 = ["dep1/f2"] + foo2 = ["dep2"] "#, ) .file( @@ -103,6 +104,7 @@ fn inactive_target_optional() { r#" fn main() { if cfg!(feature="foo1") { println!("foo1"); } + if cfg!(feature="foo2") { println!("foo2"); } if cfg!(feature="dep1") { println!("dep1"); } if cfg!(feature="dep2") { println!("dep2"); } if cfg!(feature="common") { println!("common"); } @@ -149,7 +151,7 @@ fn inactive_target_optional() { .build(); p.cargo("run --all-features") - .with_stdout("foo1\ndep1\ndep2\ncommon\nf1\nf2\nf3\nf4\n") + .with_stdout("foo1\nfoo2\ndep1\ndep2\ncommon\nf1\nf2\nf3\nf4\n") .run(); p.cargo("run --features dep1") .with_stdout("dep1\nf1\n") @@ -166,7 +168,7 @@ fn inactive_target_optional() { p.cargo("run -Zfeatures=itarget --all-features") .masquerade_as_nightly_cargo() - .with_stdout("foo1\n") + .with_stdout("foo1\nfoo2\ndep1\ndep2\ncommon") .run(); p.cargo("run -Zfeatures=itarget --features dep1") .masquerade_as_nightly_cargo() diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index 5cee06d01b5..6f5960d2167 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -1,20 +1,152 @@ //! Tests for namespaced features. use cargo_test_support::project; +use cargo_test_support::registry::{Dependency, Package}; #[cargo_test] -fn namespaced_invalid_feature() { +fn gated() { + // Need namespaced-features to use `crate:` syntax. + Package::new("bar", "1.0.0").publish(); let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional = true } + + [features] + foo = ["crate:bar"] + "#, + ) + .file("src/lib.rs", "") + .build(); - [project] + p.cargo("check") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]/foo/Cargo.toml` + +Caused by: + namespaced features with the `crate:` prefix are only allowed on the nightly channel \ + and requires the `-Z namespaced-features` flag on the command-line +", + ) + .run(); +} + +#[cargo_test] +fn dependency_gate_ignored() { + // Dependencies with `crate:` features are ignored in the registry if not on nightly. + Package::new("baz", "1.0.0").publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat", &["crate:baz"]) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] no matching package named `bar` found +location searched: registry `https://github.com/rust-lang/crates.io-index` +required by package `foo v0.1.0 ([..]/foo)` +", + ) + .run(); + + // Publish a version without namespaced features, it should ignore 1.0.0 + // an use this instead. + Package::new("bar", "1.0.1") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat", &["baz"]) + .publish(); + p.cargo("check") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar [..] +[CHECKING] bar v1.0.1 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn dependency_with_crate_syntax() { + // Registry dependency uses crate: syntax. + Package::new("baz", "1.0.0").publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat", &["crate:baz"]) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = {version="1.0", features=["feat"]} + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] [..] +[DOWNLOADED] [..] +[CHECKING] baz v1.0.0 +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn namespaced_invalid_feature() { + // Specifies a feature that doesn't exist. + let p = project() + .file( + "Cargo.toml", + r#" + [package] name = "foo" version = "0.0.1" authors = [] - namespaced-features = true [features] bar = ["baz"] @@ -23,7 +155,7 @@ fn namespaced_invalid_feature() { .file("src/main.rs", "") .build(); - p.cargo("build") + p.cargo("build -Z namespaced-features") .masquerade_as_nightly_cargo() .with_status(101) .with_stderr( @@ -31,7 +163,7 @@ fn namespaced_invalid_feature() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` includes `baz` which is not defined as a feature + feature `bar` includes `baz` which is neither a dependency nor another feature ", ) .run(); @@ -39,17 +171,14 @@ Caused by: #[cargo_test] fn namespaced_invalid_dependency() { + // Specifies a crate:name that doesn't exist. let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] bar = ["crate:baz"] @@ -58,7 +187,7 @@ fn namespaced_invalid_dependency() { .file("src/main.rs", "") .build(); - p.cargo("build") + p.cargo("build -Z namespaced-features") .masquerade_as_nightly_cargo() .with_status(101) .with_stderr( @@ -66,7 +195,7 @@ fn namespaced_invalid_dependency() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` includes `crate:baz` which is not a known dependency + feature `bar` includes `crate:baz`, but `baz` is not listed as a dependency ", ) .run(); @@ -74,17 +203,14 @@ Caused by: #[cargo_test] fn namespaced_non_optional_dependency() { + // Specifies a crate:name for a dependency that is not optional. let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] bar = ["crate:baz"] @@ -96,7 +222,7 @@ fn namespaced_non_optional_dependency() { .file("src/main.rs", "") .build(); - p.cargo("build") + p.cargo("build -Z namespaced-features") .masquerade_as_nightly_cargo() .with_status(101) .with_stderr( @@ -104,8 +230,8 @@ fn namespaced_non_optional_dependency() { [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` includes `crate:baz` which is not an optional dependency. - Consider adding `optional = true` to the dependency + feature `bar` includes `crate:baz`, but `baz` is not an optional dependency + A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition. ", ) .run(); @@ -113,17 +239,16 @@ Caused by: #[cargo_test] fn namespaced_implicit_feature() { + // Backwards-compatible with old syntax. + Package::new("baz", "0.1.0").publish(); + let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] bar = ["baz"] @@ -135,22 +260,41 @@ fn namespaced_implicit_feature() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("build").masquerade_as_nightly_cargo().run(); + p.cargo("check -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[CHECKING] foo v0.0.1 [..] +[FINISHED] [..] +", + ) + .run(); + p.cargo("check -Z namespaced-features --features baz") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.1.0 [..] +[CHECKING] baz v0.1.0 +[CHECKING] foo v0.0.1 [..] +[FINISHED] [..] +", + ) + .run(); } #[cargo_test] fn namespaced_shadowed_dep() { + // An optional dependency is not listed in the features table, and its + // implicit feature is overridden. let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] baz = [] @@ -162,31 +306,32 @@ fn namespaced_shadowed_dep() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( - "\ + p.cargo("build -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `baz` includes the optional dependency of the same name, but this is left implicit in the features included by this feature. - Consider adding `crate:baz` to this feature's requirements. + optional dependency `baz` is not included in any feature + Make sure that `crate:baz` is included in one of features in the [features] table. ", - ) + ) .run(); } #[cargo_test] fn namespaced_shadowed_non_optional() { + // Able to specify a feature with the same name as a required dependency. + Package::new("baz", "0.1.0").publish(); let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] baz = [] @@ -195,35 +340,24 @@ fn namespaced_shadowed_non_optional() { baz = "0.1" "#, ) - .file("src/main.rs", "fn main() {}") + .file("src/lib.rs", "") .build(); - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( - "\ -[ERROR] failed to parse manifest at `[..]` - -Caused by: - Feature `baz` includes the dependency of the same name, but this is left implicit in the features included by this feature. - Additionally, the dependency must be marked as optional to be included in the feature definition. - Consider adding `crate:baz` to this feature's requirements and marking the dependency as `optional = true` -", - ) + p.cargo("check -Z namespaced-features") + .masquerade_as_nightly_cargo() .run(); } #[cargo_test] fn namespaced_implicit_non_optional() { + // Includes a non-optional dependency in [features] table. let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] bar = ["baz"] @@ -235,30 +369,29 @@ fn namespaced_implicit_non_optional() { .file("src/main.rs", "fn main() {}") .build(); - p.cargo("build").masquerade_as_nightly_cargo().with_status(101).with_stderr( + p.cargo("build -Z namespaced-features").masquerade_as_nightly_cargo().with_status(101).with_stderr( "\ [ERROR] failed to parse manifest at `[..]` Caused by: - Feature `bar` includes `baz` which is not defined as a feature. - A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition + feature `bar` includes `baz`, but `baz` is not an optional dependency + A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition. ", ).run(); } #[cargo_test] fn namespaced_same_name() { + // Explicitly listing an optional dependency in the [features] table. + Package::new("baz", "0.1.0").publish(); + let p = project() .file( "Cargo.toml", r#" - cargo-features = ["namespaced-features"] - - [project] + [package] name = "foo" version = "0.0.1" - authors = [] - namespaced-features = true [features] baz = ["crate:baz"] @@ -267,8 +400,603 @@ fn namespaced_same_name() { baz = { version = "0.1", optional = true } "#, ) + .file( + "src/main.rs", + r#" + fn main() { + if cfg!(feature="baz") { println!("baz"); } + } + "#, + ) + .build(); + + p.cargo("run -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[COMPILING] foo v0.0.1 [..] +[FINISHED] [..] +[RUNNING] [..] +", + ) + .with_stdout("") + .run(); + + p.cargo("run -Z namespaced-features --features baz") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] baz v0.1.0 [..] +[COMPILING] baz v0.1.0 +[COMPILING] foo v0.0.1 [..] +[FINISHED] [..] +[RUNNING] [..] +", + ) + .with_stdout("baz") + .run(); +} + +#[cargo_test] +fn no_implicit_feature() { + // Using `crate:` will not create an implicit feature. + Package::new("regex", "1.0.0").publish(); + Package::new("lazy_static", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + regex = { version = "1.0", optional = true } + lazy_static = { version = "1.0", optional = true } + + [features] + regex = ["crate:regex", "crate:lazy_static"] + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + if cfg!(feature = "regex") { println!("regex"); } + if cfg!(feature = "lazy_static") { println!("lazy_static"); } + } + "#, + ) + .build(); + + p.cargo("run -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[COMPILING] foo v0.1.0 [..] +[FINISHED] [..] +[RUNNING] `target/debug/foo[EXE]` +", + ) + .with_stdout("") + .run(); + + p.cargo("run -Z namespaced-features --features regex") + .masquerade_as_nightly_cargo() + .with_stderr_unordered( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] regex v1.0.0 [..] +[DOWNLOADED] lazy_static v1.0.0 [..] +[COMPILING] regex v1.0.0 +[COMPILING] lazy_static v1.0.0 +[COMPILING] foo v0.1.0 [..] +[FINISHED] [..] +[RUNNING] `target/debug/foo[EXE]` +", + ) + .with_stdout("regex") + .run(); + + p.cargo("run -Z namespaced-features --features lazy_static") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[ERROR] Package `foo v0.1.0 [..]` does not have feature `lazy_static`. \ +It has an optional dependency with that name, but that dependency uses the \"crate:\" \ +syntax in the features table, so it does not have an implicit feature with that name. +", + ) + .with_status(101) + .run(); +} + +#[cargo_test] +fn crate_feature_explicit() { + // crate:name/feature syntax shouldn't set implicit feature. + Package::new("bar", "1.0.0") + .file( + "src/lib.rs", + r#" + #[cfg(not(feature="feat"))] + compile_error!{"feat missing"} + "#, + ) + .feature("feat", &[]) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = {version = "1.0", optional=true} + + [features] + f1 = ["crate:bar/feat"] + "#, + ) + .file( + "src/lib.rs", + r#" + #[cfg(not(feature="f1"))] + compile_error!{"f1 missing"} + + #[cfg(feature="bar")] + compile_error!{"bar should not be set"} + "#, + ) + .build(); + + p.cargo("check -Z namespaced-features --features f1") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn crate_syntax_bad_name() { + // "crate:bar" = [] + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version="1.0", optional=true } + + [features] + "crate:bar" = [] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Z namespaced-features --features crate:bar") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at [..]/foo/Cargo.toml` + +Caused by: + feature named `crate:bar` is not allowed to start with `crate:` +", + ) + .run(); +} + +#[cargo_test] +fn crate_syntax_in_dep() { + // features = ["crate:baz"] + Package::new("baz", "1.0.0").publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", features = ["crate:baz"] } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] feature value `crate:baz` is not allowed to use explicit `crate:` syntax +", + ) + .run(); +} + +#[cargo_test] +fn crate_syntax_cli() { + // --features crate:bar + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional=true } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check -Z namespaced-features --features crate:bar") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] feature value `crate:bar` is not allowed to use explicit `crate:` syntax +", + ) + .run(); +} + +#[cargo_test] +fn crate_required_features() { + // required-features = ["crate:bar"] + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional=true } + + [[bin]] + name = "foo" + required-features = ["crate:bar"] + "#, + ) .file("src/main.rs", "fn main() {}") .build(); - p.cargo("build").masquerade_as_nightly_cargo().run(); + p.cargo("check -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] invalid feature `crate:bar` in required-features of target `foo`: \ +explicit `crate:` feature values are not allowed in required-features +", + ) + .run(); +} + +#[cargo_test] +fn json_not_exposed() { + // Checks that crate: is not exposed in JSON. + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional=true } + "#, + ) + .file("src/lib.rs", "") + .build(); + + // Check that "features" is empty (doesn't include the implicit features). + let json = r#" + { + "packages": [ + { + "name": "foo", + "version": "0.1.0", + "id": "foo 0.1.0 [..]", + "license": null, + "license_file": null, + "description": null, + "homepage": null, + "documentation": null, + "source": null, + "dependencies": "{...}", + "targets": "{...}", + "features": {}, + "manifest_path": "[..]foo/Cargo.toml", + "metadata": null, + "publish": null, + "authors": [], + "categories": [], + "keywords": [], + "readme": null, + "repository": null, + "edition": "2015", + "links": null + } + ], + "workspace_members": "{...}", + "resolve": null, + "target_directory": "[..]foo/target", + "version": 1, + "workspace_root": "[..]foo", + "metadata": null + } + "#; + + p.cargo("metadata --no-deps").with_json(json).run(); + + // And namespaced-features shouldn't be any different. + // NOTE: I don't actually know if this is ideal behavior. + p.cargo("metadata -Z namespaced-features --no-deps") + .masquerade_as_nightly_cargo() + .with_json(json) + .run(); +} + +#[cargo_test] +fn crate_feature_with_explicit() { + // crate_name/feat_name syntax where crate_name already has a feature defined. + // NOTE: I don't know if this is actually ideal behavior. + Package::new("bar", "1.0.0") + .feature("bar_feat", &[]) + .file( + "src/lib.rs", + r#" + #[cfg(not(feature="bar_feat"))] + compile_error!("bar_feat is not enabled"); + "#, + ) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version="1.0", optional = true } + + [features] + f1 = ["bar/bar_feat"] + bar = ["crate:bar", "f2"] + f2 = [] + "#, + ) + .file( + "src/lib.rs", + r#" + #[cfg(not(feature="bar"))] + compile_error!("bar should be enabled"); + + #[cfg(not(feature="f2"))] + compile_error!("f2 should be enabled"); + "#, + ) + .build(); + + p.cargo("check -Z namespaced-features --features f1") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] crates ... +[DOWNLOADED] bar v1.0.0 [..] +[CHECKING] bar v1.0.0 +[CHECKING] foo v0.1.0 [..] +[FINISHED] [..] +", + ) + .run(); +} + +#[cargo_test] +fn optional_explicit_without_crate() { + // "feat" syntax when there is no implicit "feat" feature because it is + // explicitly listed elsewhere. + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", optional = true } + + [features] + feat1 = ["crate:bar"] + feat2 = ["bar"] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at [..] + +Caused by: + feature `feat2` includes `bar`, but `bar` is an optional dependency without an implicit feature + Use `crate:bar` to enable the dependency. +", + ) + .run(); +} + +#[cargo_test] +fn tree() { + Package::new("baz", "1.0.0").publish(); + Package::new("bar", "1.0.0") + .add_dep(Dependency::new("baz", "1.0").optional(true)) + .feature("feat1", &["crate:baz"]) + .feature("feat2", &[]) + .publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { version = "1.0", features = ["feat1"], optional=true } + + [features] + a = ["bar/feat2"] + b = ["crate:bar/feat2"] + bar = ["crate:bar"] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("tree -e features -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout("foo v0.1.0 ([ROOT]/foo)") + .run(); + + p.cargo("tree -e features --features a -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +foo v0.1.0 ([ROOT]/foo) +├── bar feature \"default\" +│ └── bar v1.0.0 +│ └── baz feature \"default\" +│ └── baz v1.0.0 +└── bar feature \"feat1\" + └── bar v1.0.0 (*) +", + ) + .run(); + + p.cargo("tree -e features --features a -i bar -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +bar v1.0.0 +├── bar feature \"default\" +│ └── foo v0.1.0 ([ROOT]/foo) +│ ├── foo feature \"a\" (command-line) +│ ├── foo feature \"bar\" +│ │ └── foo feature \"a\" (command-line) +│ └── foo feature \"default\" (command-line) +├── bar feature \"feat1\" +│ └── foo v0.1.0 ([ROOT]/foo) (*) +└── bar feature \"feat2\" + └── foo feature \"a\" (command-line) +", + ) + .run(); + + p.cargo("tree -e features --features b -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +foo v0.1.0 ([ROOT]/foo) +├── bar feature \"default\" +│ └── bar v1.0.0 +│ └── baz feature \"default\" +│ └── baz v1.0.0 +└── bar feature \"feat1\" + └── bar v1.0.0 (*) +", + ) + .run(); + + p.cargo("tree -e features --features b -i bar -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +bar v1.0.0 +├── bar feature \"default\" +│ └── foo v0.1.0 ([ROOT]/foo) +│ ├── foo feature \"b\" (command-line) +│ └── foo feature \"default\" (command-line) +├── bar feature \"feat1\" +│ └── foo v0.1.0 ([ROOT]/foo) (*) +└── bar feature \"feat2\" + └── foo feature \"b\" (command-line) +", + ) + .run(); + + p.cargo("tree -e features --features bar -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +foo v0.1.0 ([ROOT]/foo) +├── bar feature \"default\" +│ └── bar v1.0.0 +│ └── baz feature \"default\" +│ └── baz v1.0.0 +└── bar feature \"feat1\" + └── bar v1.0.0 (*) +", + ) + .run(); + + p.cargo("tree -e features --features bar -i bar -Z namespaced-features") + .masquerade_as_nightly_cargo() + .with_stdout( + "\ +bar v1.0.0 +├── bar feature \"default\" +│ └── foo v0.1.0 ([ROOT]/foo) +│ ├── foo feature \"bar\" (command-line) +│ └── foo feature \"default\" (command-line) +└── bar feature \"feat1\" + └── foo v0.1.0 ([ROOT]/foo) (*) +", + ) + .run(); } diff --git a/tests/testsuite/help.rs b/tests/testsuite/help.rs index b83b1cd8d09..5b2a87d0461 100644 --- a/tests/testsuite/help.rs +++ b/tests/testsuite/help.rs @@ -48,7 +48,7 @@ fn z_flags_help() { // Test that the output of `cargo -Z help` shows a different help screen with // all the `-Z` flags. cargo_process("-Z help") - .with_stdout_contains(" -Z unstable-options -- Allow the usage of unstable options") + .with_stdout_contains(" -Z unstable-options -- Allow the usage of unstable options") .run(); } diff --git a/tests/testsuite/package_features.rs b/tests/testsuite/package_features.rs index 35a6621eafe..3fdb4ac293d 100644 --- a/tests/testsuite/package_features.rs +++ b/tests/testsuite/package_features.rs @@ -290,7 +290,7 @@ fn other_member_from_current() { p.cargo("run -p bar --features f1,f2") .with_status(101) - .with_stderr("[ERROR] Package `foo[..]` does not have these features: `f2`") + .with_stderr("[ERROR] Package `foo[..]` does not have the feature `f2`") .run(); p.cargo("run -p bar --features f1,f2 -Zpackage-features") diff --git a/tests/testsuite/rename_deps.rs b/tests/testsuite/rename_deps.rs index c85bb9a2c22..16d15994b2e 100644 --- a/tests/testsuite/rename_deps.rs +++ b/tests/testsuite/rename_deps.rs @@ -361,7 +361,7 @@ fn features_not_working() { error: failed to parse manifest at `[..]` Caused by: - Feature `default` includes `p1` which is neither a dependency nor another feature + feature `default` includes `p1` which is neither a dependency nor another feature ", ) .run(); From b2cceaf76e754fdad82c77005fc79248058c253a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 22 Oct 2020 09:48:45 -0700 Subject: [PATCH 5/9] Expose implicit features in `cargo metadata`. --- src/bin/cargo/commands/read_manifest.rs | 2 +- src/cargo/core/package.rs | 171 ++++++++++++------------ src/cargo/ops/cargo_output_metadata.rs | 10 +- tests/testsuite/features_namespaced.rs | 87 ++++++------ 4 files changed, 135 insertions(+), 135 deletions(-) diff --git a/src/bin/cargo/commands/read_manifest.rs b/src/bin/cargo/commands/read_manifest.rs index 96cba1e082a..4f66cedfb44 100644 --- a/src/bin/cargo/commands/read_manifest.rs +++ b/src/bin/cargo/commands/read_manifest.rs @@ -15,6 +15,6 @@ Deprecated, use `cargo metadata --no-deps` instead.\ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { let ws = args.workspace(config)?; - config.shell().print_json(&ws.current()?); + config.shell().print_json(&ws.current()?.serialized(config)); Ok(()) } diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 90722692b34..ad240b94102 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -15,7 +15,6 @@ use curl::multi::{EasyHandle, Multi}; use lazycell::LazyCell; use log::{debug, warn}; use semver::Version; -use serde::ser; use serde::Serialize; use crate::core::compiler::{CompileKind, RustcTargetData}; @@ -77,94 +76,31 @@ impl PartialOrd for Package { /// A Package in a form where `Serialize` can be derived. #[derive(Serialize)] -struct SerializedPackage<'a> { - name: &'a str, - version: &'a Version, +pub struct SerializedPackage { + name: InternedString, + version: Version, id: PackageId, - license: Option<&'a str>, - license_file: Option<&'a str>, - description: Option<&'a str>, + license: Option, + license_file: Option, + description: Option, source: SourceId, - dependencies: &'a [Dependency], - targets: Vec<&'a Target>, - features: &'a BTreeMap>, - manifest_path: &'a Path, - metadata: Option<&'a toml::Value>, - publish: Option<&'a Vec>, - authors: &'a [String], - categories: &'a [String], - keywords: &'a [String], - readme: Option<&'a str>, - repository: Option<&'a str>, - homepage: Option<&'a str>, - documentation: Option<&'a str>, - edition: &'a str, - links: Option<&'a str>, + dependencies: Vec, + targets: Vec, + features: BTreeMap>, + manifest_path: PathBuf, + metadata: Option, + publish: Option>, + authors: Vec, + categories: Vec, + keywords: Vec, + readme: Option, + repository: Option, + homepage: Option, + documentation: Option, + edition: String, + links: Option, #[serde(skip_serializing_if = "Option::is_none")] - metabuild: Option<&'a Vec>, -} - -impl ser::Serialize for Package { - fn serialize(&self, s: S) -> Result - where - S: ser::Serializer, - { - let summary = self.manifest().summary(); - let package_id = summary.package_id(); - let manmeta = self.manifest().metadata(); - let license = manmeta.license.as_deref(); - let license_file = manmeta.license_file.as_deref(); - let description = manmeta.description.as_deref(); - let authors = manmeta.authors.as_ref(); - let categories = manmeta.categories.as_ref(); - let keywords = manmeta.keywords.as_ref(); - let readme = manmeta.readme.as_deref(); - let repository = manmeta.repository.as_deref(); - let homepage = manmeta.homepage.as_ref().map(String::as_ref); - let documentation = manmeta.documentation.as_ref().map(String::as_ref); - // Filter out metabuild targets. They are an internal implementation - // detail that is probably not relevant externally. There's also not a - // real path to show in `src_path`, and this avoids changing the format. - let targets: Vec<&Target> = self - .manifest() - .targets() - .iter() - .filter(|t| t.src_path().is_path()) - .collect(); - let empty_feats = BTreeMap::new(); - let features = self - .manifest() - .original() - .features() - .unwrap_or(&empty_feats); - - SerializedPackage { - name: &*package_id.name(), - version: package_id.version(), - id: package_id, - license, - license_file, - description, - source: summary.source_id(), - dependencies: summary.dependencies(), - targets, - features, - manifest_path: self.manifest_path(), - metadata: self.manifest().custom_metadata(), - authors, - categories, - keywords, - readme, - repository, - homepage, - documentation, - edition: &self.manifest().edition().to_string(), - links: self.manifest().links(), - metabuild: self.manifest().metabuild(), - publish: self.publish().as_ref(), - } - .serialize(s) - } + metabuild: Option>, } impl Package { @@ -261,6 +197,69 @@ impl Package { pub fn include_lockfile(&self) -> bool { self.targets().iter().any(|t| t.is_example() || t.is_bin()) } + + pub fn serialized(&self, config: &Config) -> SerializedPackage { + let summary = self.manifest().summary(); + let package_id = summary.package_id(); + let manmeta = self.manifest().metadata(); + // Filter out metabuild targets. They are an internal implementation + // detail that is probably not relevant externally. There's also not a + // real path to show in `src_path`, and this avoids changing the format. + let targets: Vec = self + .manifest() + .targets() + .iter() + .filter(|t| t.src_path().is_path()) + .cloned() + .collect(); + let features = if config.cli_unstable().namespaced_features { + // Convert Vec to Vec + summary + .features() + .iter() + .map(|(k, v)| { + ( + *k, + v.iter() + .map(|fv| InternedString::new(&fv.to_string())) + .collect(), + ) + }) + .collect() + } else { + self.manifest() + .original() + .features() + .cloned() + .unwrap_or_default() + }; + + SerializedPackage { + name: package_id.name(), + version: package_id.version().clone(), + id: package_id, + license: manmeta.license.clone(), + license_file: manmeta.license_file.clone(), + description: manmeta.description.clone(), + source: summary.source_id(), + dependencies: summary.dependencies().to_vec(), + targets, + features, + manifest_path: self.manifest_path().to_path_buf(), + metadata: self.manifest().custom_metadata().cloned(), + authors: manmeta.authors.clone(), + categories: manmeta.categories.clone(), + keywords: manmeta.keywords.clone(), + readme: manmeta.readme.clone(), + repository: manmeta.repository.clone(), + homepage: manmeta.homepage.clone(), + documentation: manmeta.documentation.clone(), + edition: self.manifest().edition().to_string(), + links: self.manifest().links().map(|s| s.to_owned()), + metabuild: self.manifest().metabuild().cloned(), + publish: self.publish().as_ref().cloned(), + } + } } impl fmt::Display for Package { diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index a3aaf2e7a07..70709efb056 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -1,5 +1,6 @@ use crate::core::compiler::{CompileKind, RustcTargetData}; use crate::core::dependency::DepKind; +use crate::core::package::SerializedPackage; use crate::core::resolver::{HasDevUnits, Resolve, ResolveOpts}; use crate::core::{Dependency, Package, PackageId, Workspace}; use crate::ops::{self, Packages}; @@ -32,8 +33,9 @@ pub fn output_metadata(ws: &Workspace<'_>, opt: &OutputMetadataOptions) -> Cargo VERSION ); } + let config = ws.config(); let (packages, resolve) = if opt.no_deps { - let packages = ws.members().cloned().collect(); + let packages = ws.members().map(|pkg| pkg.serialized(config)).collect(); (packages, None) } else { let (packages, resolve) = build_resolve_graph(ws, opt)?; @@ -56,7 +58,7 @@ pub fn output_metadata(ws: &Workspace<'_>, opt: &OutputMetadataOptions) -> Cargo /// See cargo-metadata.adoc for detailed documentation of the format. #[derive(Serialize)] pub struct ExportInfo { - packages: Vec, + packages: Vec, workspace_members: Vec, resolve: Option, target_directory: PathBuf, @@ -105,7 +107,7 @@ impl From<&Dependency> for DepKindInfo { fn build_resolve_graph( ws: &Workspace<'_>, metadata_opts: &OutputMetadataOptions, -) -> CargoResult<(Vec, MetadataResolve)> { +) -> CargoResult<(Vec, MetadataResolve)> { // TODO: Without --filter-platform, features are being resolved for `host` only. // How should this work? let requested_kinds = @@ -153,9 +155,11 @@ fn build_resolve_graph( ); } // Get a Vec of Packages. + let config = ws.config(); let actual_packages = package_map .into_iter() .filter_map(|(pkg_id, pkg)| node_map.get(&pkg_id).map(|_| pkg)) + .map(|pkg| pkg.serialized(config)) .collect(); let mr = MetadataResolve { diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index 6f5960d2167..f20c822994b 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -708,8 +708,8 @@ explicit `crate:` feature values are not allowed in required-features } #[cargo_test] -fn json_not_exposed() { - // Checks that crate: is not exposed in JSON. +fn json_exposed() { + // Checks that the implicit crate: values are exposed in JSON. Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -726,51 +726,48 @@ fn json_not_exposed() { .file("src/lib.rs", "") .build(); - // Check that "features" is empty (doesn't include the implicit features). - let json = r#" - { - "packages": [ - { - "name": "foo", - "version": "0.1.0", - "id": "foo 0.1.0 [..]", - "license": null, - "license_file": null, - "description": null, - "homepage": null, - "documentation": null, - "source": null, - "dependencies": "{...}", - "targets": "{...}", - "features": {}, - "manifest_path": "[..]foo/Cargo.toml", - "metadata": null, - "publish": null, - "authors": [], - "categories": [], - "keywords": [], - "readme": null, - "repository": null, - "edition": "2015", - "links": null - } - ], - "workspace_members": "{...}", - "resolve": null, - "target_directory": "[..]foo/target", - "version": 1, - "workspace_root": "[..]foo", - "metadata": null - } - "#; - - p.cargo("metadata --no-deps").with_json(json).run(); - - // And namespaced-features shouldn't be any different. - // NOTE: I don't actually know if this is ideal behavior. p.cargo("metadata -Z namespaced-features --no-deps") .masquerade_as_nightly_cargo() - .with_json(json) + .with_json( + r#" + { + "packages": [ + { + "name": "foo", + "version": "0.1.0", + "id": "foo 0.1.0 [..]", + "license": null, + "license_file": null, + "description": null, + "homepage": null, + "documentation": null, + "source": null, + "dependencies": "{...}", + "targets": "{...}", + "features": { + "bar": ["crate:bar"] + }, + "manifest_path": "[..]foo/Cargo.toml", + "metadata": null, + "publish": null, + "authors": [], + "categories": [], + "keywords": [], + "readme": null, + "repository": null, + "edition": "2015", + "links": null + } + ], + "workspace_members": "{...}", + "resolve": null, + "target_directory": "[..]foo/target", + "version": 1, + "workspace_root": "[..]foo", + "metadata": null + } + "#, + ) .run(); } From 1149f68026c3e292eb8d0cacf12466c7c5682c2d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 22 Oct 2020 09:54:48 -0700 Subject: [PATCH 6/9] Some minor cleanup. --- src/cargo/core/resolver/errors.rs | 4 ++-- src/cargo/ops/tree/graph.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index 8f5dc7cd0b4..c0deeb5abe3 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -124,9 +124,9 @@ pub(super) fn activation_error( msg.push_str("\n\nthe package `"); msg.push_str(&*dep.package_name()); msg.push_str("` links to the native library `"); - msg.push_str(&link); + msg.push_str(link); msg.push_str("`, but it conflicts with a previous package which links to `"); - msg.push_str(&link); + msg.push_str(link); msg.push_str("` as well:\n"); msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); } diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index ac1821269ce..b0c0c0fd639 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -305,7 +305,7 @@ fn add_pkg( }; let node = Node::Package { package_id, - features: node_features.clone(), + features: node_features, kind: node_kind, }; if let Some(idx) = graph.index.get(&node) { From c8a3db8e8f25f8b9626b760c172dba7abee46a7a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 23 Oct 2020 16:12:10 -0700 Subject: [PATCH 7/9] Minor cleanup. --- src/cargo/core/resolver/features.rs | 1 - src/cargo/core/summary.rs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 113d18461cd..8db68ceaedd 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -475,7 +475,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // Activate the crate on self. let fv = FeatureValue::Crate { dep_name: *dep_name, - // explicit: *explicit, }; self.activate_fv(pkg_id, &fv, for_host)?; if !explicit { diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 0ca8e2cbc64..a18fa4be4ba 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -350,8 +350,8 @@ impl FeatureValue { Some(pos) => { let (dep, dep_feat) = feature.split_at(pos); let dep_feat = &dep_feat[1..]; - let (dep, explicit) = if dep.starts_with("crate:") { - (&dep[6..], true) + let (dep, explicit) = if let Some(dep) = dep.strip_prefix("crate:") { + (dep, true) } else { (dep, false) }; From 8ff130bb289d8dfc2fb384ed81df6d0abe638b09 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 23 Oct 2020 16:32:15 -0700 Subject: [PATCH 8/9] Rename "explicit" to "crate_prefix". --- src/cargo/core/resolver/dep_cache.rs | 10 +++++----- src/cargo/core/resolver/features.rs | 4 ++-- src/cargo/core/summary.rs | 18 +++++++++--------- src/cargo/ops/cargo_compile.rs | 9 ++++++--- src/cargo/ops/tree/graph.rs | 4 ++-- tests/testsuite/features_namespaced.rs | 2 +- 6 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 8a598d79b22..2bf6098874e 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -374,7 +374,7 @@ fn build_requirements<'a, 'b: 'a>( } else { for &f in opts.features.features.iter() { let fv = FeatureValue::new(f); - if fv.is_explicit_crate() { + if fv.has_crate_prefix() { return Err(ActivateError::Fatal(anyhow::format_err!( "feature value `{}` is not allowed to use explicit `crate:` syntax", fv @@ -442,12 +442,12 @@ impl Requirements<'_> { &mut self, package: InternedString, feat: InternedString, - explicit: bool, + crate_prefix: bool, ) -> Result<(), RequirementError> { // If `package` is indeed an optional dependency then we activate the // feature named `package`, but otherwise if `package` is a required // dependency then there's no feature associated with it. - if !explicit + if !crate_prefix && self .summary .dependencies() @@ -493,8 +493,8 @@ impl Requirements<'_> { FeatureValue::CrateFeature { dep_name, dep_feature, - explicit, - } => self.require_crate_feature(*dep_name, *dep_feature, *explicit)?, + crate_prefix, + } => self.require_crate_feature(*dep_name, *dep_feature, *crate_prefix)?, }; Ok(()) } diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 8db68ceaedd..5d3c8f86773 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -463,7 +463,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { FeatureValue::CrateFeature { dep_name, dep_feature, - explicit, + crate_prefix, } => { // Activate a feature within a dependency. for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) { @@ -477,7 +477,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { dep_name: *dep_name, }; self.activate_fv(pkg_id, &fv, for_host)?; - if !explicit { + if !crate_prefix { // To retain compatibility with old behavior, // this also enables a feature of the same // name. diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index a18fa4be4ba..0d2a605de63 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -183,7 +183,7 @@ fn build_feature_map( (*feature, fvs) }) .collect(); - let has_namespaced_features = map.values().flatten().any(|fv| fv.is_explicit_crate()); + let has_namespaced_features = map.values().flatten().any(|fv| fv.has_crate_prefix()); // Add implicit features for optional dependencies if they weren't // explicitly listed anywhere. @@ -194,7 +194,7 @@ fn build_feature_map( Crate { dep_name } | CrateFeature { dep_name, - explicit: true, + crate_prefix: true, .. } => Some(*dep_name), _ => None, @@ -340,7 +340,7 @@ pub enum FeatureValue { dep_feature: InternedString, /// If this is true, then the feature used the `crate:` prefix, which /// prevents enabling the feature named `dep_name`. - explicit: bool, + crate_prefix: bool, }, } @@ -350,7 +350,7 @@ impl FeatureValue { Some(pos) => { let (dep, dep_feat) = feature.split_at(pos); let dep_feat = &dep_feat[1..]; - let (dep, explicit) = if let Some(dep) = dep.strip_prefix("crate:") { + let (dep, crate_prefix) = if let Some(dep) = dep.strip_prefix("crate:") { (dep, true) } else { (dep, false) @@ -358,7 +358,7 @@ impl FeatureValue { FeatureValue::CrateFeature { dep_name: InternedString::new(dep), dep_feature: InternedString::new(dep_feat), - explicit, + crate_prefix, } } None if feature.starts_with("crate:") => FeatureValue::Crate { @@ -369,8 +369,8 @@ impl FeatureValue { } /// Returns `true` if this feature explicitly used `crate:` syntax. - pub fn is_explicit_crate(&self) -> bool { - matches!(self, FeatureValue::Crate{..} | FeatureValue::CrateFeature{explicit:true, ..}) + pub fn has_crate_prefix(&self) -> bool { + matches!(self, FeatureValue::Crate{..} | FeatureValue::CrateFeature{crate_prefix:true, ..}) } } @@ -383,12 +383,12 @@ impl fmt::Display for FeatureValue { CrateFeature { dep_name, dep_feature, - explicit: true, + crate_prefix: true, } => write!(f, "crate:{}/{}", dep_name, dep_feature), CrateFeature { dep_name, dep_feature, - explicit: false, + crate_prefix: false, } => write!(f, "{}/{}", dep_name, dep_feature), } } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 95d4e51f930..ca89590db9e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -1071,10 +1071,13 @@ fn validate_required_features( ))?; } } - FeatureValue::Crate { .. } | FeatureValue::CrateFeature { explicit: true, .. } => { + FeatureValue::Crate { .. } + | FeatureValue::CrateFeature { + crate_prefix: true, .. + } => { anyhow::bail!( "invalid feature `{}` in required-features of target `{}`: \ - explicit `crate:` feature values are not allowed in required-features", + `crate:` prefixed feature values are not allowed in required-features", fv, target_name ); @@ -1083,7 +1086,7 @@ fn validate_required_features( FeatureValue::CrateFeature { dep_name, dep_feature, - explicit: false, + crate_prefix: false, } => { match resolve .deps(summary.package_id()) diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index b0c0c0fd639..b19583ac246 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -567,7 +567,7 @@ fn add_feature_rec( FeatureValue::CrateFeature { dep_name, dep_feature, - explicit, + crate_prefix, } => { let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) { Some(indexes) => indexes.clone(), @@ -585,7 +585,7 @@ fn add_feature_rec( }; for (dep_index, is_optional) in dep_indexes { let dep_pkg_id = graph.package_id_for_index(dep_index); - if is_optional && !explicit { + if is_optional && !crate_prefix { // Activate the optional dep on self. add_feature( graph, diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index f20c822994b..be46238bd74 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -701,7 +701,7 @@ fn crate_required_features() { "\ [UPDATING] [..] [ERROR] invalid feature `crate:bar` in required-features of target `foo`: \ -explicit `crate:` feature values are not allowed in required-features +`crate:` prefixed feature values are not allowed in required-features ", ) .run(); From f4ac82a0f5b03eed60b7363bfe15ff1a41ec22e6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 25 Oct 2020 12:51:27 -0700 Subject: [PATCH 9/9] Rename crate: to dep: --- src/bin/cargo/cli.rs | 2 +- src/cargo/core/resolver/dep_cache.rs | 24 ++++---- src/cargo/core/resolver/errors.rs | 2 +- src/cargo/core/resolver/features.rs | 14 ++--- src/cargo/core/resolver/types.rs | 2 +- src/cargo/core/summary.rs | 77 ++++++++++++----------- src/cargo/ops/cargo_compile.rs | 12 ++-- src/cargo/ops/tree/graph.rs | 8 +-- src/cargo/sources/registry/remote.rs | 20 +++--- src/doc/src/reference/unstable.md | 8 +-- tests/testsuite/features_namespaced.rs | 84 +++++++++++++------------- 11 files changed, 129 insertions(+), 124 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 4c18f3e68a6..70bc511608e 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -42,7 +42,7 @@ Available unstable (nightly-only) flags: -Z timings -- Display concurrency information -Z doctest-xcompile -- Compile and run doctests for non-host target using runner config -Z terminal-width -- Provide a terminal width to rustc for error truncation - -Z namespaced-features -- Allow features with `crate:` prefix + -Z namespaced-features -- Allow features with `dep:` prefix Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" ); diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 2bf6098874e..5cc57781fa3 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -342,7 +342,7 @@ pub fn resolve_features<'b>( } // This is a special case for command-line `--features - // crate_name/feat_name` where `crate_name` does not exist. All other + // dep_name/feat_name` where `dep_name` does not exist. All other // validation is done either in `build_requirements` or // `build_feature_map`. for dep_name in reqs.deps.keys() { @@ -374,9 +374,9 @@ fn build_requirements<'a, 'b: 'a>( } else { for &f in opts.features.features.iter() { let fv = FeatureValue::new(f); - if fv.has_crate_prefix() { + if fv.has_dep_prefix() { return Err(ActivateError::Fatal(anyhow::format_err!( - "feature value `{}` is not allowed to use explicit `crate:` syntax", + "feature value `{}` is not allowed to use explicit `dep:` syntax", fv ))); } @@ -438,16 +438,16 @@ impl Requirements<'_> { self.features } - fn require_crate_feature( + fn require_dep_feature( &mut self, package: InternedString, feat: InternedString, - crate_prefix: bool, + dep_prefix: bool, ) -> Result<(), RequirementError> { // If `package` is indeed an optional dependency then we activate the // feature named `package`, but otherwise if `package` is a required // dependency then there's no feature associated with it. - if !crate_prefix + if !dep_prefix && self .summary .dependencies() @@ -489,12 +489,12 @@ impl Requirements<'_> { fn require_value(&mut self, fv: &FeatureValue) -> Result<(), RequirementError> { match fv { FeatureValue::Feature(feat) => self.require_feature(*feat)?, - FeatureValue::Crate { dep_name } => self.require_dependency(*dep_name), - FeatureValue::CrateFeature { + FeatureValue::Dep { dep_name } => self.require_dependency(*dep_name), + FeatureValue::DepFeature { dep_name, dep_feature, - crate_prefix, - } => self.require_crate_feature(*dep_name, *dep_feature, *crate_prefix)?, + dep_prefix, + } => self.require_dep_feature(*dep_name, *dep_feature, *dep_prefix)?, }; Ok(()) } @@ -526,7 +526,7 @@ impl RequirementError { match parent { None => ActivateError::Fatal(anyhow::format_err!( "Package `{}` does not have feature `{}`. It has an optional dependency \ - with that name, but that dependency uses the \"crate:\" \ + with that name, but that dependency uses the \"dep:\" \ syntax in the features table, so it does not have an implicit feature with that name.", summary.package_id(), feat @@ -559,7 +559,7 @@ impl RequirementError { dep_name )), // This code path currently isn't used, since `foo/bar` - // and `crate:` syntax is not allowed in a dependency. + // and `dep:` syntax is not allowed in a dependency. Some(p) => ActivateError::Conflict( p, ConflictReason::MissingFeatures(dep_name.to_string()), diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index c0deeb5abe3..8e9968db4e8 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -170,7 +170,7 @@ pub(super) fn activation_error( msg.push_str("` does not have these features.\n"); msg.push_str( " It has an optional dependency with that name, \ - but but that dependency uses the \"crate:\" \ + but but that dependency uses the \"dep:\" \ syntax in the features table, so it does not have an \ implicit feature with that name.\n", ); diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index 5d3c8f86773..3cc696c6d59 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -412,7 +412,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { // For example, consider we've already processed our dependencies, // and another package comes along and enables one of our optional // dependencies, it will do so immediately in the - // `FeatureValue::CrateFeature` branch, and then immediately + // `FeatureValue::DepFeature` branch, and then immediately // recurse into that optional dependency. This also holds true for // features that enable other features. return Ok(()); @@ -443,7 +443,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { FeatureValue::Feature(f) => { self.activate_rec(pkg_id, *f, for_host)?; } - FeatureValue::Crate { dep_name } => { + FeatureValue::Dep { dep_name } => { // Mark this dependency as activated. self.activated_dependencies .entry((pkg_id, self.opts.decouple_host_deps && for_host)) @@ -460,10 +460,10 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { } } } - FeatureValue::CrateFeature { + FeatureValue::DepFeature { dep_name, dep_feature, - crate_prefix, + dep_prefix, } => { // Activate a feature within a dependency. for (dep_pkg_id, deps) in self.deps(pkg_id, for_host) { @@ -472,12 +472,12 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { continue; } if dep.is_optional() { - // Activate the crate on self. - let fv = FeatureValue::Crate { + // Activate the dependency on self. + let fv = FeatureValue::Dep { dep_name: *dep_name, }; self.activate_fv(pkg_id, &fv, for_host)?; - if !crate_prefix { + if !dep_prefix { // To retain compatibility with old behavior, // this also enables a feature of the same // name. diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index fd53e856cb7..554b9dd0eae 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -297,7 +297,7 @@ pub enum ConflictReason { RequiredDependencyAsFeature(InternedString), /// A dependency listed a feature for an optional dependency, but that - /// optional dependency is "hidden" using namespaced `crate:` syntax. + /// optional dependency is "hidden" using namespaced `dep:` syntax. NonImplicitDependencyAsFeature(InternedString), // TODO: needs more info for `activation_error` diff --git a/src/cargo/core/summary.rs b/src/cargo/core/summary.rs index 0d2a605de63..c6d453f7c42 100644 --- a/src/cargo/core/summary.rs +++ b/src/cargo/core/summary.rs @@ -88,7 +88,7 @@ impl Summary { if !namespaced_features { if self.inner.has_namespaced_features { bail!( - "namespaced features with the `crate:` prefix are only allowed on \ + "namespaced features with the `dep:` prefix are only allowed on \ the nightly channel and requires the `-Z namespaced-features` flag on the command-line" ); } @@ -158,7 +158,7 @@ impl Hash for Summary { /// and creates FeatureValues for each feature. /// /// The returned `bool` indicates whether or not the `[features]` table -/// included a `crate:` prefixed namespaced feature (used for gating on +/// included a `dep:` prefixed namespaced feature (used for gating on /// nightly). fn build_feature_map( features: &BTreeMap>, @@ -183,7 +183,7 @@ fn build_feature_map( (*feature, fvs) }) .collect(); - let has_namespaced_features = map.values().flatten().any(|fv| fv.has_crate_prefix()); + let has_namespaced_features = map.values().flatten().any(|fv| fv.has_dep_prefix()); // Add implicit features for optional dependencies if they weren't // explicitly listed anywhere. @@ -191,10 +191,10 @@ fn build_feature_map( .values() .flatten() .filter_map(|fv| match fv { - Crate { dep_name } - | CrateFeature { + Dep { dep_name } + | DepFeature { dep_name, - crate_prefix: true, + dep_prefix: true, .. } => Some(*dep_name), _ => None, @@ -209,7 +209,7 @@ fn build_feature_map( { continue; } - let fv = Crate { + let fv = Dep { dep_name: dep_name_in_toml, }; map.insert(dep_name_in_toml, vec![fv]); @@ -217,9 +217,9 @@ fn build_feature_map( // Validate features are listed properly. for (feature, fvs) in &map { - if feature.starts_with("crate:") { + if feature.starts_with("dep:") { bail!( - "feature named `{}` is not allowed to start with `crate:`", + "feature named `{}` is not allowed to start with `dep:`", feature ); } @@ -227,7 +227,7 @@ fn build_feature_map( // Find data for the referenced dependency... let dep_data = { match fv { - Feature(dep_name) | Crate { dep_name, .. } | CrateFeature { dep_name, .. } => { + Feature(dep_name) | Dep { dep_name, .. } | DepFeature { dep_name, .. } => { dep_map.get(dep_name) } } @@ -253,7 +253,7 @@ fn build_feature_map( bail!( "feature `{}` includes `{}`, but `{}` is an \ optional dependency without an implicit feature\n\ - Use `crate:{}` to enable the dependency.", + Use `dep:{}` to enable the dependency.", feature, fv, f, @@ -268,7 +268,7 @@ fn build_feature_map( } } } - Crate { dep_name } => { + Dep { dep_name } => { if !is_any_dep { bail!( "feature `{}` includes `{}`, but `{}` is not listed as a dependency", @@ -288,7 +288,7 @@ fn build_feature_map( ); } } - CrateFeature { dep_name, .. } => { + DepFeature { dep_name, .. } => { // Validation of the feature name will be performed in the resolver. if !is_any_dep { bail!( @@ -308,7 +308,7 @@ fn build_feature_map( .values() .flatten() .filter_map(|fv| match fv { - Crate { dep_name } | CrateFeature { dep_name, .. } => Some(dep_name), + Dep { dep_name } | DepFeature { dep_name, .. } => Some(dep_name), _ => None, }) .collect(); @@ -318,7 +318,7 @@ fn build_feature_map( { bail!( "optional dependency `{}` is not included in any feature\n\ - Make sure that `crate:{}` is included in one of features in the [features] table.", + Make sure that `dep:{}` is included in one of features in the [features] table.", dep.name_in_toml(), dep.name_in_toml(), ); @@ -332,15 +332,15 @@ fn build_feature_map( pub enum FeatureValue { /// A feature enabling another feature. Feature(InternedString), - /// A feature enabling a dependency with `crate:dep_name` syntax. - Crate { dep_name: InternedString }, + /// A feature enabling a dependency with `dep:dep_name` syntax. + Dep { dep_name: InternedString }, /// A feature enabling a feature on a dependency with `crate_name/feat_name` syntax. - CrateFeature { + DepFeature { dep_name: InternedString, dep_feature: InternedString, - /// If this is true, then the feature used the `crate:` prefix, which + /// If this is true, then the feature used the `dep:` prefix, which /// prevents enabling the feature named `dep_name`. - crate_prefix: bool, + dep_prefix: bool, }, } @@ -350,27 +350,32 @@ impl FeatureValue { Some(pos) => { let (dep, dep_feat) = feature.split_at(pos); let dep_feat = &dep_feat[1..]; - let (dep, crate_prefix) = if let Some(dep) = dep.strip_prefix("crate:") { + let (dep, dep_prefix) = if let Some(dep) = dep.strip_prefix("dep:") { (dep, true) } else { (dep, false) }; - FeatureValue::CrateFeature { + FeatureValue::DepFeature { dep_name: InternedString::new(dep), dep_feature: InternedString::new(dep_feat), - crate_prefix, + dep_prefix, + } + } + None => { + if let Some(dep_name) = feature.strip_prefix("dep:") { + FeatureValue::Dep { + dep_name: InternedString::new(dep_name), + } + } else { + FeatureValue::Feature(feature) } } - None if feature.starts_with("crate:") => FeatureValue::Crate { - dep_name: InternedString::new(&feature[6..]), - }, - None => FeatureValue::Feature(feature), } } - /// Returns `true` if this feature explicitly used `crate:` syntax. - pub fn has_crate_prefix(&self) -> bool { - matches!(self, FeatureValue::Crate{..} | FeatureValue::CrateFeature{crate_prefix:true, ..}) + /// Returns `true` if this feature explicitly used `dep:` syntax. + pub fn has_dep_prefix(&self) -> bool { + matches!(self, FeatureValue::Dep{..} | FeatureValue::DepFeature{dep_prefix:true, ..}) } } @@ -379,16 +384,16 @@ impl fmt::Display for FeatureValue { use self::FeatureValue::*; match self { Feature(feat) => write!(f, "{}", feat), - Crate { dep_name } => write!(f, "crate:{}", dep_name), - CrateFeature { + Dep { dep_name } => write!(f, "dep:{}", dep_name), + DepFeature { dep_name, dep_feature, - crate_prefix: true, - } => write!(f, "crate:{}/{}", dep_name, dep_feature), - CrateFeature { + dep_prefix: true, + } => write!(f, "dep:{}/{}", dep_name, dep_feature), + DepFeature { dep_name, dep_feature, - crate_prefix: false, + dep_prefix: false, } => write!(f, "{}/{}", dep_name, dep_feature), } } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index ca89590db9e..58e4fbc6a6d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -1071,22 +1071,22 @@ fn validate_required_features( ))?; } } - FeatureValue::Crate { .. } - | FeatureValue::CrateFeature { - crate_prefix: true, .. + FeatureValue::Dep { .. } + | FeatureValue::DepFeature { + dep_prefix: true, .. } => { anyhow::bail!( "invalid feature `{}` in required-features of target `{}`: \ - `crate:` prefixed feature values are not allowed in required-features", + `dep:` prefixed feature values are not allowed in required-features", fv, target_name ); } // Handling of dependent_crate/dependent_crate_feature syntax - FeatureValue::CrateFeature { + FeatureValue::DepFeature { dep_name, dep_feature, - crate_prefix: false, + dep_prefix: false, } => { match resolve .deps(summary.package_id()) diff --git a/src/cargo/ops/tree/graph.rs b/src/cargo/ops/tree/graph.rs index b19583ac246..f69f924f655 100644 --- a/src/cargo/ops/tree/graph.rs +++ b/src/cargo/ops/tree/graph.rs @@ -563,11 +563,11 @@ fn add_feature_rec( package_index, ); } - FeatureValue::Crate { .. } => {} - FeatureValue::CrateFeature { + FeatureValue::Dep { .. } => {} + FeatureValue::DepFeature { dep_name, dep_feature, - crate_prefix, + dep_prefix, } => { let dep_indexes = match graph.dep_name_map[&package_index].get(dep_name) { Some(indexes) => indexes.clone(), @@ -585,7 +585,7 @@ fn add_feature_rec( }; for (dep_index, is_optional) in dep_indexes { let dep_pkg_id = graph.package_id_for_index(dep_index); - if is_optional && !crate_prefix { + if is_optional && !dep_prefix { // Activate the optional dep on self. add_feature( graph, diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs index 2e9ffeabc0b..2e44d9ae3ea 100644 --- a/src/cargo/sources/registry/remote.rs +++ b/src/cargo/sources/registry/remote.rs @@ -20,7 +20,7 @@ use std::mem; use std::path::Path; use std::str; -fn make_crate_prefix(name: &str) -> String { +fn make_dep_prefix(name: &str) -> String { match name.len() { 1 => String::from("1"), 2 => String::from("2"), @@ -274,7 +274,7 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> { { write!(url, "/{}/{}/download", CRATE_TEMPLATE, VERSION_TEMPLATE).unwrap(); } - let prefix = make_crate_prefix(&*pkg.name()); + let prefix = make_dep_prefix(&*pkg.name()); let url = url .replace(CRATE_TEMPLATE, &*pkg.name()) .replace(VERSION_TEMPLATE, &pkg.version().to_string()) @@ -341,15 +341,15 @@ impl<'cfg> Drop for RemoteRegistry<'cfg> { #[cfg(test)] mod tests { - use super::make_crate_prefix; + use super::make_dep_prefix; #[test] - fn crate_prefix() { - assert_eq!(make_crate_prefix("a"), "1"); - assert_eq!(make_crate_prefix("ab"), "2"); - assert_eq!(make_crate_prefix("abc"), "3/a"); - assert_eq!(make_crate_prefix("Abc"), "3/A"); - assert_eq!(make_crate_prefix("AbCd"), "Ab/Cd"); - assert_eq!(make_crate_prefix("aBcDe"), "aB/cD"); + fn dep_prefix() { + assert_eq!(make_dep_prefix("a"), "1"); + assert_eq!(make_dep_prefix("ab"), "2"); + assert_eq!(make_dep_prefix("abc"), "3/a"); + assert_eq!(make_dep_prefix("Abc"), "3/A"); + assert_eq!(make_dep_prefix("AbCd"), "Ab/Cd"); + assert_eq!(make_dep_prefix("aBcDe"), "aB/cD"); } } diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 6c576487e43..4664a1f96f0 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -196,11 +196,11 @@ specified: * Features may now be defined with the same name as a dependency. * Optional dependencies can be explicitly enabled in the `[features]` table - with the `crate:` prefix, which enables the dependency without enabling a + with the `dep:` prefix, which enables the dependency without enabling a feature of the same name. By default, an optional dependency `foo` will define a feature `foo = -["crate:foo"]` *unless* `crate:foo` is mentioned in any other feature, or the +["dep:foo"]` *unless* `dep:foo` is mentioned in any other feature, or the `foo` feature is already defined. This helps prevent unnecessary boilerplate of listing every optional dependency, but still allows you to override the implicit feature. @@ -223,7 +223,7 @@ regex = { version = "1.4.1", optional = true } lazy_static = { version = "1.4.0", optional = true } [features] -regex = ["crate:regex", "crate:lazy_static"] +regex = ["dep:regex", "dep:lazy_static"] ``` In this example, the "regex" feature enables both `regex` and `lazy_static`. @@ -240,7 +240,7 @@ num-bigint = "0.2" serde = {version = "1.0", optional = true } [features] -serde = ["crate:serde", "bigdecimal/serde", "chrono/serde", "num-bigint/serde"] +serde = ["dep:serde", "bigdecimal/serde", "chrono/serde", "num-bigint/serde"] ``` In this case, `serde` is a natural name to use for a feature, because it is diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index be46238bd74..c5206865ec2 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -5,7 +5,7 @@ use cargo_test_support::registry::{Dependency, Package}; #[cargo_test] fn gated() { - // Need namespaced-features to use `crate:` syntax. + // Need namespaced-features to use `dep:` syntax. Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -19,7 +19,7 @@ fn gated() { bar = { version = "1.0", optional = true } [features] - foo = ["crate:bar"] + foo = ["dep:bar"] "#, ) .file("src/lib.rs", "") @@ -32,7 +32,7 @@ fn gated() { [ERROR] failed to parse manifest at `[..]/foo/Cargo.toml` Caused by: - namespaced features with the `crate:` prefix are only allowed on the nightly channel \ + namespaced features with the `dep:` prefix are only allowed on the nightly channel \ and requires the `-Z namespaced-features` flag on the command-line ", ) @@ -41,11 +41,11 @@ Caused by: #[cargo_test] fn dependency_gate_ignored() { - // Dependencies with `crate:` features are ignored in the registry if not on nightly. + // Dependencies with `dep:` features are ignored in the registry if not on nightly. Package::new("baz", "1.0.0").publish(); Package::new("bar", "1.0.0") .add_dep(Dependency::new("baz", "1.0").optional(true)) - .feature("feat", &["crate:baz"]) + .feature("feat", &["dep:baz"]) .publish(); let p = project() .file( @@ -98,11 +98,11 @@ required by package `foo v0.1.0 ([..]/foo)` #[cargo_test] fn dependency_with_crate_syntax() { - // Registry dependency uses crate: syntax. + // Registry dependency uses dep: syntax. Package::new("baz", "1.0.0").publish(); Package::new("bar", "1.0.0") .add_dep(Dependency::new("baz", "1.0").optional(true)) - .feature("feat", &["crate:baz"]) + .feature("feat", &["dep:baz"]) .publish(); let p = project() .file( @@ -171,7 +171,7 @@ Caused by: #[cargo_test] fn namespaced_invalid_dependency() { - // Specifies a crate:name that doesn't exist. + // Specifies a dep:name that doesn't exist. let p = project() .file( "Cargo.toml", @@ -181,7 +181,7 @@ fn namespaced_invalid_dependency() { version = "0.0.1" [features] - bar = ["crate:baz"] + bar = ["dep:baz"] "#, ) .file("src/main.rs", "") @@ -195,7 +195,7 @@ fn namespaced_invalid_dependency() { [ERROR] failed to parse manifest at `[..]` Caused by: - feature `bar` includes `crate:baz`, but `baz` is not listed as a dependency + feature `bar` includes `dep:baz`, but `baz` is not listed as a dependency ", ) .run(); @@ -203,7 +203,7 @@ Caused by: #[cargo_test] fn namespaced_non_optional_dependency() { - // Specifies a crate:name for a dependency that is not optional. + // Specifies a dep:name for a dependency that is not optional. let p = project() .file( "Cargo.toml", @@ -213,7 +213,7 @@ fn namespaced_non_optional_dependency() { version = "0.0.1" [features] - bar = ["crate:baz"] + bar = ["dep:baz"] [dependencies] baz = "0.1" @@ -230,7 +230,7 @@ fn namespaced_non_optional_dependency() { [ERROR] failed to parse manifest at `[..]` Caused by: - feature `bar` includes `crate:baz`, but `baz` is not an optional dependency + feature `bar` includes `dep:baz`, but `baz` is not an optional dependency A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition. ", ) @@ -315,7 +315,7 @@ fn namespaced_shadowed_dep() { Caused by: optional dependency `baz` is not included in any feature - Make sure that `crate:baz` is included in one of features in the [features] table. + Make sure that `dep:baz` is included in one of features in the [features] table. ", ) .run(); @@ -394,7 +394,7 @@ fn namespaced_same_name() { version = "0.0.1" [features] - baz = ["crate:baz"] + baz = ["dep:baz"] [dependencies] baz = { version = "0.1", optional = true } @@ -441,7 +441,7 @@ fn namespaced_same_name() { #[cargo_test] fn no_implicit_feature() { - // Using `crate:` will not create an implicit feature. + // Using `dep:` will not create an implicit feature. Package::new("regex", "1.0.0").publish(); Package::new("lazy_static", "1.0.0").publish(); @@ -458,7 +458,7 @@ fn no_implicit_feature() { lazy_static = { version = "1.0", optional = true } [features] - regex = ["crate:regex", "crate:lazy_static"] + regex = ["dep:regex", "dep:lazy_static"] "#, ) .file( @@ -507,7 +507,7 @@ fn no_implicit_feature() { .with_stderr( "\ [ERROR] Package `foo v0.1.0 [..]` does not have feature `lazy_static`. \ -It has an optional dependency with that name, but that dependency uses the \"crate:\" \ +It has an optional dependency with that name, but that dependency uses the \"dep:\" \ syntax in the features table, so it does not have an implicit feature with that name. ", ) @@ -517,7 +517,7 @@ syntax in the features table, so it does not have an implicit feature with that #[cargo_test] fn crate_feature_explicit() { - // crate:name/feature syntax shouldn't set implicit feature. + // dep:name/feature syntax shouldn't set implicit feature. Package::new("bar", "1.0.0") .file( "src/lib.rs", @@ -540,7 +540,7 @@ fn crate_feature_explicit() { bar = {version = "1.0", optional=true} [features] - f1 = ["crate:bar/feat"] + f1 = ["dep:bar/feat"] "#, ) .file( @@ -572,7 +572,7 @@ fn crate_feature_explicit() { #[cargo_test] fn crate_syntax_bad_name() { - // "crate:bar" = [] + // "dep:bar" = [] Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -586,13 +586,13 @@ fn crate_syntax_bad_name() { bar = { version="1.0", optional=true } [features] - "crate:bar" = [] + "dep:bar" = [] "#, ) .file("src/lib.rs", "") .build(); - p.cargo("check -Z namespaced-features --features crate:bar") + p.cargo("check -Z namespaced-features --features dep:bar") .masquerade_as_nightly_cargo() .with_status(101) .with_stderr( @@ -600,7 +600,7 @@ fn crate_syntax_bad_name() { [ERROR] failed to parse manifest at [..]/foo/Cargo.toml` Caused by: - feature named `crate:bar` is not allowed to start with `crate:` + feature named `dep:bar` is not allowed to start with `dep:` ", ) .run(); @@ -608,7 +608,7 @@ Caused by: #[cargo_test] fn crate_syntax_in_dep() { - // features = ["crate:baz"] + // features = ["dep:baz"] Package::new("baz", "1.0.0").publish(); Package::new("bar", "1.0.0") .add_dep(Dependency::new("baz", "1.0").optional(true)) @@ -622,7 +622,7 @@ fn crate_syntax_in_dep() { version = "0.1.0" [dependencies] - bar = { version = "1.0", features = ["crate:baz"] } + bar = { version = "1.0", features = ["dep:baz"] } "#, ) .file("src/lib.rs", "") @@ -634,7 +634,7 @@ fn crate_syntax_in_dep() { .with_stderr( "\ [UPDATING] [..] -[ERROR] feature value `crate:baz` is not allowed to use explicit `crate:` syntax +[ERROR] feature value `dep:baz` is not allowed to use explicit `dep:` syntax ", ) .run(); @@ -642,7 +642,7 @@ fn crate_syntax_in_dep() { #[cargo_test] fn crate_syntax_cli() { - // --features crate:bar + // --features dep:bar Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -659,13 +659,13 @@ fn crate_syntax_cli() { .file("src/lib.rs", "") .build(); - p.cargo("check -Z namespaced-features --features crate:bar") + p.cargo("check -Z namespaced-features --features dep:bar") .masquerade_as_nightly_cargo() .with_status(101) .with_stderr( "\ [UPDATING] [..] -[ERROR] feature value `crate:bar` is not allowed to use explicit `crate:` syntax +[ERROR] feature value `dep:bar` is not allowed to use explicit `dep:` syntax ", ) .run(); @@ -673,7 +673,7 @@ fn crate_syntax_cli() { #[cargo_test] fn crate_required_features() { - // required-features = ["crate:bar"] + // required-features = ["dep:bar"] Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -688,7 +688,7 @@ fn crate_required_features() { [[bin]] name = "foo" - required-features = ["crate:bar"] + required-features = ["dep:bar"] "#, ) .file("src/main.rs", "fn main() {}") @@ -700,8 +700,8 @@ fn crate_required_features() { .with_stderr( "\ [UPDATING] [..] -[ERROR] invalid feature `crate:bar` in required-features of target `foo`: \ -`crate:` prefixed feature values are not allowed in required-features +[ERROR] invalid feature `dep:bar` in required-features of target `foo`: \ +`dep:` prefixed feature values are not allowed in required-features ", ) .run(); @@ -709,7 +709,7 @@ fn crate_required_features() { #[cargo_test] fn json_exposed() { - // Checks that the implicit crate: values are exposed in JSON. + // Checks that the implicit dep: values are exposed in JSON. Package::new("bar", "1.0.0").publish(); let p = project() .file( @@ -745,7 +745,7 @@ fn json_exposed() { "dependencies": "{...}", "targets": "{...}", "features": { - "bar": ["crate:bar"] + "bar": ["dep:bar"] }, "manifest_path": "[..]foo/Cargo.toml", "metadata": null, @@ -798,7 +798,7 @@ fn crate_feature_with_explicit() { [features] f1 = ["bar/bar_feat"] - bar = ["crate:bar", "f2"] + bar = ["dep:bar", "f2"] f2 = [] "#, ) @@ -846,7 +846,7 @@ fn optional_explicit_without_crate() { bar = { version = "1.0", optional = true } [features] - feat1 = ["crate:bar"] + feat1 = ["dep:bar"] feat2 = ["bar"] "#, ) @@ -862,7 +862,7 @@ fn optional_explicit_without_crate() { Caused by: feature `feat2` includes `bar`, but `bar` is an optional dependency without an implicit feature - Use `crate:bar` to enable the dependency. + Use `dep:bar` to enable the dependency. ", ) .run(); @@ -873,7 +873,7 @@ fn tree() { Package::new("baz", "1.0.0").publish(); Package::new("bar", "1.0.0") .add_dep(Dependency::new("baz", "1.0").optional(true)) - .feature("feat1", &["crate:baz"]) + .feature("feat1", &["dep:baz"]) .feature("feat2", &[]) .publish(); let p = project() @@ -889,8 +889,8 @@ fn tree() { [features] a = ["bar/feat2"] - b = ["crate:bar/feat2"] - bar = ["crate:bar"] + b = ["dep:bar/feat2"] + bar = ["dep:bar"] "#, ) .file("src/lib.rs", "")