Auto merge of #5726 - ehuss:warn-virtual-keys, r=alexcrichton

Warn about unused virtual manifest keys.

Fixes #4243

src/cargo/core/manifest.rs

@@ -28,7 +28,7 @@ pub struct Manifest {
  summary: Summary,
  targets: Vec<Target>,
  links: Option<String>,
- warnings: Vec<DelayedWarning>,
+ warnings: Warnings,
  exclude: Vec<String>,
  include: Vec<String>,
  metadata: ManifestMetadata,
@@ -54,12 +54,16 @@ pub struct DelayedWarning {
  pub is_critical: bool,
 }
 
+#[derive(Clone, Debug)]
+pub struct Warnings(Vec<DelayedWarning>);
+
 #[derive(Clone, Debug)]
 pub struct VirtualManifest {
  replace: Vec<(PackageIdSpec, Dependency)>,
  patch: HashMap<Url, Vec<Dependency>>,
  workspace: WorkspaceConfig,
  profiles: Profiles,
+ warnings: Warnings,
 }
 
 /// General metadata about a package which is just blindly uploaded to the
@@ -298,7 +302,7 @@ impl Manifest {
  Manifest {
  summary,
  targets,
- warnings: Vec::new(),
+ warnings: Warnings::new(),
  exclude,
  include,
  links,
@@ -344,7 +348,10 @@ impl Manifest {
  pub fn version(&self) -> &Version {
  self.package_id().version()
  }
- pub fn warnings(&self) -> &[DelayedWarning] {
+ pub fn warnings_mut(&mut self) -> &mut Warnings {
+ &mut self.warnings
+ }
+ pub fn warnings(&self) -> &Warnings {
  &self.warnings
  }
  pub fn profiles(&self) -> &Profiles {
@@ -377,20 +384,6 @@ impl Manifest {
  &self.features
  }
 
- pub fn add_warning(&mut self, s: String) {
- self.warnings.push(DelayedWarning {
- message: s,
- is_critical: false,
- })
- }
-
- pub fn add_critical_warning(&mut self, s: String) {
- self.warnings.push(DelayedWarning {
- message: s,
- is_critical: true,
- })
- }
-
  pub fn set_summary(&mut self, summary: Summary) {
  self.summary = summary;
  }
@@ -447,6 +440,7 @@ impl VirtualManifest {
  patch,
  workspace,
  profiles,
+ warnings: Warnings::new(),
  }
  }
 
@@ -465,6 +459,14 @@ impl VirtualManifest {
  pub fn profiles(&self) -> &Profiles {
  &self.profiles
  }
+
+ pub fn warnings_mut(&mut self) -> &mut Warnings {
+ &mut self.warnings
+ }
+
+ pub fn warnings(&self) -> &Warnings {
+ &self.warnings
+ }
 }
 
 impl Target {
@@ -743,3 +745,27 @@ impl fmt::Display for Target {
  }
  }
 }
+
+impl Warnings {
+ fn new() -> Warnings {
+ Warnings(Vec::new())
+ }
+
+ pub fn add_warning(&mut self, s: String) {
+ self.0.push(DelayedWarning {
+ message: s,
+ is_critical: false,
+ })
+ }
+
+ pub fn add_critical_warning(&mut self, s: String) {
+ self.0.push(DelayedWarning {
+ message: s,
+ is_critical: true,
+ })
+ }
+
+ pub fn warnings(&self) -> &[DelayedWarning] {
+ &self.0
+ }
+}

src/cargo/core/workspace.rs

@@ -723,6 +723,28 @@ impl<'cfg> Workspace<'cfg> {
  registry.add_preloaded(Box::new(src));
  }
  }
+
+ pub fn emit_warnings(&self) -> CargoResult<()> {
+ for (path, maybe_pkg) in &self.packages.packages {
+ let warnings = match maybe_pkg {
+ MaybePackage::Package(pkg) => pkg.manifest().warnings().warnings(),
+ MaybePackage::Virtual(vm) => vm.warnings().warnings(),
+ };
+ for warning in warnings {
+ if warning.is_critical {
+ let err = format_err!("{}", warning.message);
+ let cx = format_err!(
+ "failed to parse manifest at `{}`",
+ path.display()
+ );
+ return Err(err.context(cx).into());
+ } else {
+ self.config.shell().warn(&warning.message)?
+ }
+ }
+ }
+ Ok(())
+ }
 }
 
 impl<'cfg> Packages<'cfg> {

src/cargo/ops/cargo_compile.rs

@@ -177,20 +177,7 @@ pub fn compile_with_exec<'a>(
  options: &CompileOptions<'a>,
  exec: Arc<Executor>,
 ) -> CargoResult<Compilation<'a>> {
- for member in ws.members() {
- for warning in member.manifest().warnings().iter() {
- if warning.is_critical {
- let err = format_err!("{}", warning.message);
- let cx = format_err!(
- "failed to parse manifest at `{}`",
- member.manifest_path().display()
- );
- return Err(err.context(cx).into());
- } else {
- options.config.shell().warn(&warning.message)?
- }
- }
- }
+ ws.emit_warnings()?;
  compile_ws(ws, None, options, exec)
 }
 

src/cargo/util/toml/mod.rs

@@ -13,7 +13,7 @@ use toml;
 use url::Url;
 
 use core::dependency::{Kind, Platform};
-use core::manifest::{LibKind, ManifestMetadata};
+use core::manifest::{LibKind, ManifestMetadata, Warnings};
 use core::profiles::Profiles;
 use core::{Dependency, Manifest, PackageId, Summary, Target};
 use core::{Edition, EitherManifest, Feature, Features, VirtualManifest};
@@ -63,14 +63,17 @@ fn do_read_manifest(
  stringify(&mut key, &path);
  unused.insert(key);
  })?;
+ let add_unused = |warnings: &mut Warnings| {
+ for key in unused {
+ warnings.add_warning(format!("unused manifest key: {}", key));
+ }
+ };
 
  let manifest = Rc::new(manifest);
  return if manifest.project.is_some() || manifest.package.is_some() {
  let (mut manifest, paths) =
  TomlManifest::to_real_manifest(&manifest, source_id, package_root, config)?;
- for key in unused {
- manifest.add_warning(format!("unused manifest key: {}", key));
- }
+ add_unused(manifest.warnings_mut());
  if !manifest.targets().iter().any(|t| !t.is_custom_build()) {
  bail!(
  "no targets specified in the manifest\n \
@@ -80,8 +83,9 @@ fn do_read_manifest(
  }
  Ok((EitherManifest::Real(manifest), paths))
  } else {
- let (m, paths) =
+ let (mut m, paths) =
  TomlManifest::to_virtual_manifest(&manifest, source_id, package_root, config)?;
+ add_unused(m.warnings_mut());
  Ok((EitherManifest::Virtual(m), paths))
  };
 
@@ -969,17 +973,17 @@ impl TomlManifest {
  Rc::clone(me),
  );
  if project.license_file.is_some() && project.license.is_some() {
- manifest.add_warning(
+ manifest.warnings_mut().add_warning(
  "only one of `license` or \
  `license-file` is necessary"
  .to_string(),
  );
  }
  for warning in warnings {
- manifest.add_warning(warning);
+ manifest.warnings_mut().add_warning(warning);
  }
  for error in errors {
- manifest.add_critical_warning(error);
+ manifest.warnings_mut().add_critical_warning(error);
  }
 
  manifest.feature_gate()?;

tests/testsuite/bad_config.rs

@@ -898,6 +898,39 @@ warning: unused manifest key: lib.build
  );
 }
 
+#[test]
+fn unused_keys_in_virtual_manifest() {
+ let p = project("foo")
+ .file(
+ "Cargo.toml",
+ r#"
+ [workspace]
+ members = ["bar"]
+ bulid = "foo"
+ "#,
+ )
+ .file(
+ "bar/Cargo.toml",
+ r#"
+ [project]
+ version = "0.0.1"
+ name = "bar"
+ "#,
+ )
+ .file("bar/src/lib.rs", r"")
+ .build();
+ assert_that(
+ p.cargo("build --all"),
+ execs().with_status(0).with_stderr(
+ "\
+warning: unused manifest key: workspace.bulid
+[COMPILING] bar [..]
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
+",
+ ),
+ );
+}
+
 #[test]
 fn empty_dependencies() {
  let p = project("empty_deps")