Expose implicit features in cargo metadata.

ehuss · ehuss · commit b2cceaf76e75 · 2020-10-23T16:04:41.000-07:00
diff --git 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
@@ -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<String>,
+    license_file: Option<String>,
+    description: Option<String>,
     source: SourceId,
-    dependencies: &'a [Dependency],
-    targets: Vec<&'a Target>,
-    features: &'a BTreeMap<InternedString, Vec<InternedString>>,
-    manifest_path: &'a Path,
-    metadata: Option<&'a toml::Value>,
-    publish: Option<&'a Vec<String>>,
-    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<Dependency>,
+    targets: Vec<Target>,
+    features: BTreeMap<InternedString, Vec<InternedString>>,
+    manifest_path: PathBuf,
+    metadata: Option<toml::Value>,
+    publish: Option<Vec<String>>,
+    authors: Vec<String>,
+    categories: Vec<String>,
+    keywords: Vec<String>,
+    readme: Option<String>,
+    repository: Option<String>,
+    homepage: Option<String>,
+    documentation: Option<String>,
+    edition: String,
+    links: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
-    metabuild: Option<&'a Vec<String>>,
-}
-
-impl ser::Serialize for Package {
-    fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
-    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<Vec<String>>,
 }
 
 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<Target> = self
+            .manifest()
+            .targets()
+            .iter()
+            .filter(|t| t.src_path().is_path())
+            .cloned()
+            .collect();
+        let features = if config.cli_unstable().namespaced_features {
+            // Convert Vec<FeatureValue> to Vec<InternedString>
+            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
@@ -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<Package>,
+    packages: Vec<SerializedPackage>,
     workspace_members: Vec<PackageId>,
     resolve: Option<MetadataResolve>,
     target_directory: PathBuf,
@@ -105,7 +107,7 @@ impl From<&Dependency> for DepKindInfo {
 fn build_resolve_graph(
     ws: &Workspace<'_>,
     metadata_opts: &OutputMetadataOptions,
-) -> CargoResult<(Vec<Package>, MetadataResolve)> {
+) -> CargoResult<(Vec<SerializedPackage>, 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
@@ -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();
 }
 

Original file line number	Diff line number	Diff line change
@@ -15,6 +15,6 @@ Deprecated, use `cargo metadata --no-deps` instead.\
`15`	`15`
`16`	`16`	`pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {`
`17`	`17`	`let ws = args.workspace(config)?;`
`18`		`- config.shell().print_json(&ws.current()?);`
	`18`	`+ config.shell().print_json(&ws.current()?.serialized(config));`
`19`	`19`	`Ok(())`
`20`	`20`	`}`