From 688a4fa6b36239e40246eba0842809e60bb078cf Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 14 Jul 2018 09:20:15 -0700 Subject: [PATCH] Warn about unused virtual manifest keys. Fixes #4243 --- src/cargo/core/manifest.rs | 60 ++++++++++++++++++++++++---------- src/cargo/core/workspace.rs | 22 +++++++++++++ src/cargo/ops/cargo_compile.rs | 15 +-------- src/cargo/util/toml/mod.rs | 20 +++++++----- tests/testsuite/bad_config.rs | 33 +++++++++++++++++++ 5 files changed, 111 insertions(+), 39 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 2b2b86ec2cc..36077e8a029 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -28,7 +28,7 @@ pub struct Manifest { summary: Summary, targets: Vec, links: Option, - warnings: Vec, + warnings: Warnings, exclude: Vec, include: Vec, metadata: ManifestMetadata, @@ -54,12 +54,16 @@ pub struct DelayedWarning { pub is_critical: bool, } +#[derive(Clone, Debug)] +pub struct Warnings(Vec); + #[derive(Clone, Debug)] pub struct VirtualManifest { replace: Vec<(PackageIdSpec, Dependency)>, patch: HashMap>, 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 + } +} diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 4209f8a5b52..7439bf3195f 100644 --- a/src/cargo/core/workspace.rs +++ b/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> { diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index dc16c9dbf04..5197becc34e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -177,20 +177,7 @@ pub fn compile_with_exec<'a>( options: &CompileOptions<'a>, exec: Arc, ) -> CargoResult> { - 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) } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 4a5bf690002..5a7521bca9e 100644 --- a/src/cargo/util/toml/mod.rs +++ b/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()?; diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index b1da66d749e..21226f81ab9 100644 --- a/tests/testsuite/bad_config.rs +++ b/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")