From c0e3e5ae44669089ec903f6bb6785b191172f74c Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 10 Aug 2020 02:19:09 +0200 Subject: [PATCH] Support specifying different lint levels Specifying via the command line is not possible for now because cargo currently has to pass -W via the command line, but once the lint is set to warn by default, this won't be needed :). --- src/cargo/core/compiler/job_queue.rs | 6 +- src/cargo/core/compiler/mod.rs | 16 +- .../core/compiler/unused_dependencies.rs | 186 +++++++++++------- src/cargo/ops/cargo_test.rs | 11 +- tests/testsuite/unused_dependencies.rs | 157 ++++++++++++++- 5 files changed, 282 insertions(+), 94 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 7bae8ff0af9..ab49e64d77b 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -70,7 +70,7 @@ use super::job::{ Job, }; use super::timings::Timings; -use super::unused_dependencies::UnusedDepState; +use super::unused_dependencies::{UnusedDepState, UnusedExterns}; use super::{BuildContext, BuildPlan, CompileMode, Context, Unit}; use crate::core::compiler::future_incompat::{ FutureBreakageItem, OnDiskReport, FUTURE_INCOMPAT_FILE, @@ -244,7 +244,7 @@ enum Message { Token(io::Result), Finish(JobId, Artifact, CargoResult<()>), FutureIncompatReport(JobId, Vec), - UnusedExterns(JobId, Vec), + UnusedExterns(JobId, UnusedExterns), // This client should get release_raw called on it with one of our tokens NeedsToken(JobId), @@ -308,7 +308,7 @@ impl<'a> JobState<'a> { /// /// This is useful for checking unused dependencies. /// Should only be called once, as the compiler only emits it once per compilation. - pub fn unused_externs(&self, unused_externs: Vec) { + pub fn unused_externs(&self, unused_externs: UnusedExterns) { self.messages .push(Message::UnusedExterns(self.id, unused_externs)); } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 697757aa29d..715fd0aed41 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -50,6 +50,7 @@ pub(crate) use self::layout::Layout; pub use self::lto::Lto; use self::output_depinfo::output_depinfo; use self::unit_graph::UnitDep; +use self::unused_dependencies::UnusedExterns; use crate::core::compiler::future_incompat::FutureIncompatReport; pub use crate::core::compiler::unit::{Unit, UnitInterner}; use crate::core::manifest::TargetSourcePath; @@ -216,6 +217,10 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> Car add_cap_lints(cx.bcx, unit, &mut rustc); + if cx.bcx.config.cli_unstable().warn_unused_deps && unit.show_warnings(cx.bcx.config) { + rustc.arg("-W").arg("unused_crate_dependencies"); + } + let outputs = cx.outputs(unit)?; let root = cx.files().out_dir(unit); @@ -1338,16 +1343,9 @@ fn on_stderr_line_inner( } } - #[derive(serde::Deserialize)] - struct UnusedExterns { - unused_extern_names: Vec, - } if let Ok(uext) = serde_json::from_str::(compiler_message.get()) { - log::trace!( - "obtained unused externs list from rustc: `{:?}`", - uext.unused_extern_names - ); - state.unused_externs(uext.unused_extern_names); + log::trace!("obtained unused externs message from rustc: `{:?}`", uext); + state.unused_externs(uext); return Ok(true); } diff --git a/src/cargo/core/compiler/unused_dependencies.rs b/src/cargo/core/compiler/unused_dependencies.rs index 4494dfc4078..2e53a6be057 100644 --- a/src/cargo/core/compiler/unused_dependencies.rs +++ b/src/cargo/core/compiler/unused_dependencies.rs @@ -14,9 +14,28 @@ use std::collections::{HashMap, HashSet}; pub type AllowedKinds = HashSet; +#[derive(serde::Deserialize, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[serde(rename_all = "lowercase")] +/// Lint levels +/// +/// Note that order is important here +pub enum LintLevel { + // Allow isn't mentioned as the unused dependencies message + // isn't emitted if the lint is set to allow. + Warn, + Deny, + Forbid, +} + +#[derive(serde::Deserialize, Debug)] +pub struct UnusedExterns { + lint_level: LintLevel, + unused_extern_names: Vec, +} + #[derive(Default, Clone)] struct State { - /// All externs of a root unit. + /// All externs passed to units externs: HashMap>, /// The used externs so far. /// The DepKind is included so that we can tell when @@ -28,6 +47,8 @@ struct State { #[derive(Clone)] pub struct UnusedDepState { states: HashMap<(PackageId, Option), State>, + /// The worst encountered lint level so far + worst_lint_level: LintLevel, /// Tracking for which units we have received reports from. /// /// When we didn't receive reports, e.g. because of an error, @@ -152,6 +173,7 @@ impl UnusedDepState { Self { states, + worst_lint_level: LintLevel::Warn, reports_obtained: HashSet::new(), } } @@ -161,15 +183,18 @@ impl UnusedDepState { &mut self, unit_deps: &[UnitDep], unit: &Unit, - unused_externs: Vec, + unused_externs: UnusedExterns, ) { self.reports_obtained.insert(unit.clone()); + self.worst_lint_level = self.worst_lint_level.max(unused_externs.lint_level); + let usable_deps_iter = unit_deps .iter() // compare with similar check in extern_args .filter(|dep| dep.unit.target.is_linkable() && !dep.unit.mode.is_doc()); let unused_externs_set = unused_externs + .unused_extern_names .iter() .map(|ex| InternedString::new(ex)) .collect::>(); @@ -220,89 +245,108 @@ impl UnusedDepState { allowed_kinds_or_late ); - // Sort the states to have a consistent output - let mut states_sorted = self.states.iter().collect::>(); - states_sorted.sort_by_key(|(k, _v)| k.clone()); - for ((pkg_id, dep_kind), state) in states_sorted.iter() { - let outstanding_reports = state - .reports_needed_by - .iter() - .filter(|report| !self.reports_obtained.contains(report)) - .collect::>(); - if !outstanding_reports.is_empty() { - trace!("Supressing unused deps warning of pkg {} v{} mode '{}dep' due to outstanding reports {:?}", pkg_id.name(), pkg_id.version(), dep_kind_desc(*dep_kind), + let mut error_count = 0; + { + let mut emit_lint: Box CargoResult<()>> = + if self.worst_lint_level == LintLevel::Warn { + Box::new(|msg| config.shell().warn(msg)) + } else { + Box::new(|msg| { + error_count += 1; + config.shell().error(msg) + }) + }; + + // Sort the states to have a consistent output + let mut states_sorted = self.states.iter().collect::>(); + states_sorted.sort_by_key(|(k, _v)| k.clone()); + for ((pkg_id, dep_kind), state) in states_sorted.iter() { + let outstanding_reports = state + .reports_needed_by + .iter() + .filter(|report| !self.reports_obtained.contains(report)) + .collect::>(); + if !outstanding_reports.is_empty() { + trace!("Supressing unused deps warning of pkg {} v{} mode '{}dep' due to outstanding reports {:?}", pkg_id.name(), pkg_id.version(), dep_kind_desc(*dep_kind), outstanding_reports.iter().map(|unit| unit_desc(unit)).collect::>()); - // Some compilations errored without printing the unused externs. - // Don't print the warning in order to reduce false positive - // spam during errors. - continue; - } - // Sort the externs to have a consistent output - let mut externs_sorted = state.externs.iter().collect::>(); - externs_sorted.sort_by_key(|(k, _v)| k.clone()); - for (ext, dependency) in externs_sorted.iter() { - let dep_kind = if let Some(dep_kind) = dep_kind { - dep_kind - } else { - // Internal dep_kind isn't interesting to us - continue; - }; - if state.used_externs.contains(&(**ext, *dep_kind)) { - // The dependency is used + // Some compilations errored without printing the unused externs. + // Don't print the warning in order to reduce false positive + // spam during errors. continue; } - // Implicitly added dependencies (in the same crate) aren't interesting - let dependency = if let Some(dependency) = dependency { - dependency - } else { - continue; - }; - if let Some(allowed_kinds) = allowed_kinds_or_late { - if !allowed_kinds.contains(dep_kind) { - // We can't warn for dependencies of this target kind - // as we aren't compiling all the units - // that use the dependency kind - trace!("Supressing unused deps warning of {} in pkg {} v{} as mode '{}dep' not allowed", dependency.name_in_toml(), pkg_id.name(), pkg_id.version(), dep_kind_desc(Some(*dep_kind))); + // Sort the externs to have a consistent output + let mut externs_sorted = state.externs.iter().collect::>(); + externs_sorted.sort_by_key(|(k, _v)| k.clone()); + for (ext, dependency) in externs_sorted.iter() { + let dep_kind = if let Some(dep_kind) = dep_kind { + dep_kind + } else { + // Internal dep_kind isn't interesting to us + continue; + }; + if state.used_externs.contains(&(**ext, *dep_kind)) { + // The dependency is used continue; } - } else { - } - if dependency.name_in_toml().starts_with("_") { - // Dependencies starting with an underscore - // are marked as ignored - trace!( - "Supressing unused deps warning of {} in pkg {} v{} due to name", - dependency.name_in_toml(), - pkg_id.name(), - pkg_id.version() - ); - continue; - } - if dep_kind == &DepKind::Normal - && state.used_externs.contains(&(**ext, DepKind::Development)) - { - // The dependency is used but only by dev targets, - // which means it should be a dev-dependency instead - config.shell().warn(format!( - "dependency {} in package {} v{} is only used by dev targets", + // Implicitly added dependencies (in the same crate) aren't interesting + let dependency = if let Some(dependency) = dependency { + dependency + } else { + continue; + }; + if let Some(allowed_kinds) = allowed_kinds_or_late { + if !allowed_kinds.contains(dep_kind) { + // We can't warn for dependencies of this target kind + // as we aren't compiling all the units + // that use the dependency kind + trace!("Supressing unused deps warning of {} in pkg {} v{} as mode '{}dep' not allowed", dependency.name_in_toml(), pkg_id.name(), pkg_id.version(), dep_kind_desc(Some(*dep_kind))); + continue; + } + } else { + } + if dependency.name_in_toml().starts_with("_") { + // Dependencies starting with an underscore + // are marked as ignored + trace!( + "Supressing unused deps warning of {} in pkg {} v{} due to name", + dependency.name_in_toml(), + pkg_id.name(), + pkg_id.version() + ); + continue; + } + if dep_kind == &DepKind::Normal + && state.used_externs.contains(&(**ext, DepKind::Development)) + { + // The dependency is used but only by dev targets, + // which means it should be a dev-dependency instead + emit_lint(format!( + "dependency {} in package {} v{} is only used by dev targets", + dependency.name_in_toml(), + pkg_id.name(), + pkg_id.version() + ))?; + continue; + } + + emit_lint(format!( + "unused {}dependency {} in package {} v{}", + dep_kind_desc(Some(*dep_kind)), dependency.name_in_toml(), pkg_id.name(), pkg_id.version() ))?; - continue; } - - config.shell().warn(format!( - "unused {}dependency {} in package {} v{}", - dep_kind_desc(Some(*dep_kind)), - dependency.name_in_toml(), - pkg_id.name(), - pkg_id.version() - ))?; } } + if error_count > 0 { + anyhow::bail!( + "exiting because of {} unused dependencies error(s)", + error_count + ); + } Ok(()) } } diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index 340f261534c..584d24621c3 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/src/cargo/ops/cargo_test.rs @@ -1,3 +1,4 @@ +use crate::core::compiler::unused_dependencies::UnusedExterns; use crate::core::compiler::{Compilation, CompileKind, Doctest, UnitOutput}; use crate::core::shell::Verbosity; use crate::core::{TargetKind, Workspace}; @@ -260,16 +261,8 @@ fn run_doc_tests( Ok(()) }, &mut |line| { - #[derive(serde::Deserialize)] - struct UnusedExterns { - unused_extern_names: Vec, - } if let Ok(uext) = serde_json::from_str::(line) { - unused_dep_state.record_unused_externs_for_unit( - &unit_deps, - unit, - uext.unused_extern_names, - ); + unused_dep_state.record_unused_externs_for_unit(&unit_deps, unit, uext); // Supress output of the json formatted unused extern message return Ok(()); } diff --git a/tests/testsuite/unused_dependencies.rs b/tests/testsuite/unused_dependencies.rs index 94370ca6d89..2d35c21ae71 100644 --- a/tests/testsuite/unused_dependencies.rs +++ b/tests/testsuite/unused_dependencies.rs @@ -11,6 +11,7 @@ use cargo_test_support::project; use cargo_test_support::registry::Package; // TODO more commands for the tests to test the allowed kinds logic +// TODO ensure that build-dependencies are warned about when there's no build.rs at all // TODO document the tests #[cargo_test(unused_dependencies)] @@ -377,6 +378,7 @@ fn unused_dep_lib_bin() { .run(); } +#[rustfmt::skip] #[cargo_test(unused_dependencies)] fn should_be_dev() { // Test the warning that a dependency should be a dev dep. @@ -434,7 +436,6 @@ fn should_be_dev() { ) .build(); - #[rustfmt::skip] /* // Currently disabled because of a bug: test --no-run witholds unused dep warnings // for doctests that never happen @@ -531,7 +532,6 @@ fn should_be_dev() { #[cargo_test(unused_dependencies)] fn dev_deps() { // Test for unused dev dependencies - // In this instance, Package::new("bar", "0.1.0").publish(); Package::new("baz", "0.1.0").publish(); Package::new("baz2", "0.1.0").publish(); @@ -666,6 +666,159 @@ fn dev_deps() { .run(); } +#[rustfmt::skip] +#[cargo_test(unused_dependencies)] +fn lint_control() { + // Test for user control of lint severity + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + Package::new("baz2", "0.1.0").publish(); + Package::new("qux", "0.1.0").publish(); + Package::new("quux", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [build-dependencies] + baz = "0.1.0" + + [dependencies] + qux = "0.1.0" + + [dev-dependencies] + bar = "0.1.0" + baz2 = "0.1.0" + quux = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + /// ``` + /// use baz2 as _; extern crate baz2; + /// ``` + pub fn foo() {} + "#, + ) + .file( + "tests/hello.rs", + r#" + #![deny(unused_crate_dependencies)] + use bar as _; + "#, + ) + .build(); + + // cargo test --no-run doesn't test doctests and benches + // and thus doesn't create unused dev dep warnings + p.cargo("test --no-run -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] qux v0.1.0 [..] +[DOWNLOADED] quux v0.1.0 [..] +[DOWNLOADED] baz2 v0.1.0 [..] +[DOWNLOADED] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // cargo test --no-run --all-targets + // doesn't test doctests, still no unused dev dep warnings + p.cargo("test --no-run --all-targets -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[ERROR] unused dependency qux in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // cargo test tests + // everything including doctests, but not + // the benches + p.cargo("test -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[FINISHED] test [unoptimized + debuginfo] target(s) in [..] +[RUNNING] [..] +[RUNNING] [..] +[..] +[ERROR] unused dependency qux in package foo v0.1.0 +[ERROR] unused dev-dependency quux in package foo v0.1.0\ + ", + ) + .run(); + + // cargo build doesn't build the tests + // so it can't find the error setting and thus emits a warning + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[WARNING] unused dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // TODO remove the /* */ when rustc sets the + // unused dependency lint to warn by default +/* + // Passing -D for the lint turns it into an error + p.cargo("rustc -Zwarn-unused-deps -- -D unused-crate-dependencies") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[ERROR] unused dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // Passing -F for the lint turns it into an error + p.cargo("rustc -Zwarn-unused-deps -- -F unused_crate_dependencies") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[ERROR] unused dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // Passing -A for the lint removes it + p.cargo("rustc -Zwarn-unused-deps -- -Aunused_crate_dependencies") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +*/ +} + #[cargo_test(unused_dependencies)] fn cfg_test_used() { // Ensure that a dependency only used from #[cfg(test)] code