From 30a6d21c6f639e265d075e02a6e1be56284dd8a3 Mon Sep 17 00:00:00 2001 From: est31 Date: Tue, 4 Aug 2020 14:44:10 +0200 Subject: [PATCH 1/4] Return (Result<(),E>,) in drain_the_queue This refactors the drain_the_queue function to return a tuple containing Result. That way, it's still not possible to use ? or try! to handle errors, but for readers of the function declaration it's clearer now that the error actually indicates one. Bonus: it makes the calling code of drain_the_queue simpler. --- src/cargo/core/compiler/job_queue.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 4491b1fd649..209e96811f4 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -457,10 +457,8 @@ impl<'cfg> JobQueue<'cfg> { .map(move |srv| srv.start(move |msg| messages.push(Message::FixDiagnostic(msg)))); crossbeam_utils::thread::scope(move |scope| { - match state.drain_the_queue(cx, plan, scope, &helper) { - Some(err) => Err(err), - None => Ok(()), - } + let (result,) = state.drain_the_queue(cx, plan, scope, &helper); + result }) .expect("child threads shouldn't panic") } @@ -691,15 +689,16 @@ impl<'cfg> DrainState<'cfg> { /// This is the "main" loop, where Cargo does all work to run the /// compiler. /// - /// This returns an Option to prevent the use of `?` on `Result` types - /// because it is important for the loop to carefully handle errors. + /// This returns a tuple of `Result` to prevent the use of `?` on + /// `Result` types because it is important for the loop to + /// carefully handle errors. fn drain_the_queue( mut self, cx: &mut Context<'_, '_>, plan: &mut BuildPlan, scope: &Scope<'_>, jobserver_helper: &HelperThread, - ) -> Option { + ) -> (Result<(), anyhow::Error>,) { trace!("queue: {:#?}", self.queue); // Iteratively execute the entire dependency graph. Each turn of the @@ -769,7 +768,7 @@ impl<'cfg> DrainState<'cfg> { if error.is_some() { crate::display_error(&e, &mut cx.bcx.config.shell()); } else { - return Some(e); + return (Err(e),); } } if cx.bcx.build_config.emit_json() { @@ -782,13 +781,13 @@ impl<'cfg> DrainState<'cfg> { if error.is_some() { crate::display_error(&e.into(), &mut shell); } else { - return Some(e.into()); + return (Err(e.into()),); } } } if let Some(e) = error { - Some(e) + (Err(e),) } else if self.queue.is_empty() && self.pending_queue.is_empty() { let message = format!( "{} [{}] target(s) in {}", @@ -800,10 +799,10 @@ impl<'cfg> DrainState<'cfg> { self.emit_future_incompat(cx); } - None + (Ok(()),) } else { debug!("queue: {:#?}", self.queue); - Some(internal("finished with jobs still left in the queue")) + (Err(internal("finished with jobs still left in the queue")),) } } From 6a1697c7f92c9b30435c698e605e15578d1200c9 Mon Sep 17 00:00:00 2001 From: est31 Date: Wed, 1 Jul 2020 00:08:30 +0200 Subject: [PATCH 2/4] Add flag to warn about unused dependencies This commit adds an experimental flag to cargo, -Zwarn-unused-deps, with the goal of getting it eventually stabilized and enabled by default. The lint builds upon the --json=unused-externs flag of rustc that is being proposed for this purpose. This commit makes cargo pass the flag if -Zwarn-unused-deps is enabled to compilations of units it prints warnings for. During compilation, code collects the unused dependencies from all units, converts them into used dependencies, continuously extending the record of used dependencies for any type of DepKind: [dependencies], [dev-dependencies] and [build-dependencies]. Once the compilation has ended, this record is used to obtain those dependencies in a class that weren't used by any unit, and warn about them. The goal is to stabilize the flag and enable it by default once the lint has become robust and the robustness is proven. Then, cargo shall opportunistically warn about unused dependencies when it has compiled (or loaded the unused externs from cache of) all units that could possibly use dependencies of the kind. Roughly, it's like this: * cargo always compiles build.rs * cargo check compiles all units that would use [dependencies] * cargo test --no-run --all-targets compiles all units that can use [dev-dependencies]... except for the benches * cargo check --all-targets compiles all units that can use [dev-dependencies]... except for the doctests --- crates/cargo-test-macro/src/lib.rs | 11 +- src/cargo/core/compiler/build_context/mod.rs | 4 + src/cargo/core/compiler/job_queue.rs | 22 + src/cargo/core/compiler/mod.rs | 35 +- src/cargo/core/compiler/unit_dependencies.rs | 48 +- src/cargo/core/compiler/unit_graph.rs | 14 +- .../core/compiler/unused_dependencies.rs | 293 +++++++ src/cargo/core/features.rs | 2 + src/cargo/ops/cargo_compile.rs | 30 +- tests/testsuite/main.rs | 1 + tests/testsuite/unused_dependencies.rs | 757 ++++++++++++++++++ 11 files changed, 1201 insertions(+), 16 deletions(-) create mode 100644 src/cargo/core/compiler/unused_dependencies.rs create mode 100644 tests/testsuite/unused_dependencies.rs diff --git a/crates/cargo-test-macro/src/lib.rs b/crates/cargo-test-macro/src/lib.rs index dae129ade66..e455cff38f2 100644 --- a/crates/cargo-test-macro/src/lib.rs +++ b/crates/cargo-test-macro/src/lib.rs @@ -14,6 +14,7 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { )))); let build_std = contains_ident(&attr, "build_std"); + let unused_dependencies = contains_ident(&attr, "unused_dependencies"); for token in item { let group = match token { @@ -34,12 +35,14 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream { let mut new_body = to_token_stream("let _test_guard = cargo_test_support::paths::init_root();"); - // If this is a `build_std` test (aka `tests/build-std/*.rs`) then they - // only run on nightly and they only run when specifically instructed to - // on CI. - if build_std { + // If this is a test that only runs on nightly (`build_std` and `unused_dependencies`) + if build_std || unused_dependencies { let ts = to_token_stream("if !cargo_test_support::is_nightly() { return }"); new_body.extend(ts); + } + // `build_std` tests (aka `tests/build-std/*.rs`) only run + // when specifically instructed to on CI. + if build_std { let ts = to_token_stream( "if std::env::var(\"CARGO_RUN_BUILD_STD_TESTS\").is_err() { return }", ); diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index cd42c3a4ec1..b2aa40c669f 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/src/cargo/core/compiler/build_context/mod.rs @@ -1,4 +1,5 @@ use crate::core::compiler::unit_graph::UnitGraph; +use crate::core::compiler::unused_dependencies::AllowedKinds; use crate::core::compiler::{BuildConfig, CompileKind, Unit}; use crate::core::profiles::Profiles; use crate::core::PackageSet; @@ -30,6 +31,7 @@ pub struct BuildContext<'a, 'cfg> { pub profiles: Profiles, pub build_config: &'a BuildConfig, + pub allowed_kinds: AllowedKinds, /// Extra compiler args for either `rustc` or `rustdoc`. pub extra_compiler_args: HashMap>, @@ -56,6 +58,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { ws: &'a Workspace<'cfg>, packages: PackageSet<'cfg>, build_config: &'a BuildConfig, + allowed_kinds: AllowedKinds, profiles: Profiles, extra_compiler_args: HashMap>, target_data: RustcTargetData, @@ -74,6 +77,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { config: ws.config(), packages, build_config, + allowed_kinds, profiles, extra_compiler_args, target_data, diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index 209e96811f4..ffb506910c2 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -70,6 +70,7 @@ use super::job::{ Job, }; use super::timings::Timings; +use super::unused_dependencies::UnusedDepState; use super::{BuildContext, BuildPlan, CompileMode, Context, Unit}; use crate::core::compiler::future_incompat::{ FutureBreakageItem, OnDiskReport, FUTURE_INCOMPAT_FILE, @@ -133,6 +134,7 @@ struct DrainState<'cfg> { progress: Progress<'cfg>, next_id: u32, timings: Timings<'cfg>, + unused_dep_state: UnusedDepState, /// Tokens that are currently owned by this Cargo, and may be "associated" /// with a rustc process. They may also be unused, though if so will be @@ -242,6 +244,7 @@ enum Message { Token(io::Result), Finish(JobId, Artifact, CargoResult<()>), FutureIncompatReport(JobId, Vec), + UnusedExterns(JobId, Vec), // This client should get release_raw called on it with one of our tokens NeedsToken(JobId), @@ -301,6 +304,15 @@ impl<'a> JobState<'a> { .push(Message::FutureIncompatReport(self.id, report)); } + /// The rustc emitted the list of unused `--extern` args. + /// + /// 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) { + self.messages + .push(Message::UnusedExterns(self.id, unused_externs)); + } + /// The rustc underlying this Job is about to acquire a jobserver token (i.e., block) /// on the passed client. /// @@ -423,6 +435,7 @@ impl<'cfg> JobQueue<'cfg> { progress, next_id: 0, timings: self.timings, + unused_dep_state: UnusedDepState::new_with_graph(cx), // TODO tokens: Vec::new(), rustc_tokens: HashMap::new(), to_send_clients: BTreeMap::new(), @@ -614,6 +627,11 @@ impl<'cfg> DrainState<'cfg> { self.per_crate_future_incompat_reports .push(FutureIncompatReportCrate { package_id, report }); } + Message::UnusedExterns(id, unused_externs) => { + let unit = &self.active[&id]; + self.unused_dep_state + .record_unused_externs_for_unit(cx, unit, unused_externs); + } Message::Token(acquired_token) => { let token = acquired_token.chain_err(|| "failed to acquire jobserver token")?; self.tokens.push(token); @@ -786,6 +804,10 @@ impl<'cfg> DrainState<'cfg> { } } + if !cx.bcx.build_config.build_plan && cx.bcx.config.cli_unstable().warn_unused_deps { + drop(self.unused_dep_state.emit_unused_warnings(cx)); + } + if let Some(e) = error { (Err(e),) } else if self.queue.is_empty() && self.pending_queue.is_empty() { diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index a1dddf1ad82..697757aa29d 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -20,6 +20,7 @@ mod timings; mod unit; pub mod unit_dependencies; pub mod unit_graph; +pub mod unused_dependencies; use std::env; use std::ffi::{OsStr, OsString}; @@ -614,7 +615,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { rustdoc.arg("--cfg").arg(&format!("feature=\"{}\"", feat)); } - add_error_format_and_color(cx, &mut rustdoc, false); + add_error_format_and_color(cx, unit, &mut rustdoc, false); add_allow_features(cx, &mut rustdoc); if let Some(args) = cx.bcx.extra_args_for(unit) { @@ -715,7 +716,12 @@ fn add_allow_features(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder) { /// intercepting messages like rmeta artifacts, etc. rustc includes a /// "rendered" field in the JSON message with the message properly formatted, /// which Cargo will extract and display to the user. -fn add_error_format_and_color(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, pipelined: bool) { +fn add_error_format_and_color( + cx: &Context<'_, '_>, + unit: &Unit, + cmd: &mut ProcessBuilder, + pipelined: bool, +) { cmd.arg("--error-format=json"); let mut json = String::from("--json=diagnostic-rendered-ansi"); if pipelined { @@ -724,6 +730,12 @@ fn add_error_format_and_color(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, pi json.push_str(",artifacts"); } + // Emit unused externs but only if the flag is enabled + // and only for units we are interested in. + if cx.bcx.config.cli_unstable().warn_unused_deps && unit.show_warnings(cx.bcx.config) { + json.push_str(",unused-externs"); + } + match cx.bcx.build_config.message_format { MessageFormat::Short | MessageFormat::Json { short: true, .. } => { json.push_str(",diagnostic-short"); @@ -782,7 +794,7 @@ fn build_base_args( edition.cmd_edition_arg(cmd); add_path_args(bcx.ws, unit, cmd); - add_error_format_and_color(cx, cmd, cx.rmeta_required(unit)); + add_error_format_and_color(cx, unit, cmd, cx.rmeta_required(unit)); add_allow_features(cx, cmd); if !test { @@ -1038,6 +1050,10 @@ fn build_deps_args( cmd.arg(arg); } + if cx.bcx.config.cli_unstable().warn_unused_deps { + unstable_opts = true; + } + // This will only be set if we're already using a feature // requiring nightly rust if unstable_opts { @@ -1322,6 +1338,19 @@ 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); + return Ok(true); + } + #[derive(serde::Deserialize)] struct JobserverNotification { jobserver_event: Event, diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index cfcda149b37..a5874a4cedf 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -15,7 +15,7 @@ //! (for example, with and without tests), so we actually build a dependency //! graph of `Unit`s, which capture these properties. -use crate::core::compiler::unit_graph::{UnitDep, UnitGraph}; +use crate::core::compiler::unit_graph::{UnitDep, UnitDependency, UnitGraph}; use crate::core::compiler::UnitInterner; use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; use crate::core::dependency::DepKind; @@ -146,6 +146,9 @@ fn attach_std_deps( if !unit.kind.is_host() && !unit.mode.is_run_custom_build() { deps.extend(std_roots[&unit.kind].iter().map(|unit| UnitDep { unit: unit.clone(), + // There is no dependency in the manifest giving rise to this + // as the dependency is implicit + dependency: UnitDependency(None), unit_for: UnitFor::new_normal(), extern_crate_name: unit.pkg.name(), // TODO: Does this `public` make sense? @@ -269,13 +272,36 @@ fn compute_deps( let mode = check_or_build_mode(unit.mode, lib); let dep_unit_for = unit_for.with_dependency(unit, lib); + // TODO this does not take into account cases where a dependency + // comes from multiple sources like appears both as dev-dependency + // and proper dependency, same dependency through multiple cfg's, etc. + let dep = deps.iter().next().cloned(); + let start = ret.len(); if state.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host() { - let unit_dep = new_unit_dep(state, unit, pkg, lib, dep_unit_for, unit.kind, mode)?; + let dep_cloned = dep.clone(); + let unit_dep = new_unit_dep( + state, + unit, + pkg, + lib, + dep_cloned, + dep_unit_for, + unit.kind, + mode, + )?; ret.push(unit_dep); - let unit_dep = - new_unit_dep(state, unit, pkg, lib, dep_unit_for, CompileKind::Host, mode)?; + let unit_dep = new_unit_dep( + state, + unit, + pkg, + lib, + dep, + dep_unit_for, + CompileKind::Host, + mode, + )?; ret.push(unit_dep); } else { let unit_dep = new_unit_dep( @@ -283,6 +309,7 @@ fn compute_deps( unit, pkg, lib, + dep, dep_unit_for, unit.kind.for_target(lib), mode, @@ -351,6 +378,7 @@ fn compute_deps( unit, &unit.pkg, t, + None, UnitFor::new_normal(), unit.kind.for_target(t), CompileMode::Build, @@ -400,6 +428,7 @@ fn compute_deps_custom_build( unit, &unit.pkg, &unit.target, + None, script_unit_for, // Build scripts always compiled for the host. CompileKind::Host, @@ -435,6 +464,7 @@ fn compute_deps_doc( unit, dep, lib, + None, dep_unit_for, unit.kind.for_target(lib), mode, @@ -447,6 +477,7 @@ fn compute_deps_doc( unit, dep, lib, + None, dep_unit_for, unit.kind.for_target(lib), unit.mode, @@ -482,6 +513,7 @@ fn maybe_lib( unit, &unit.pkg, t, + None, dep_unit_for, unit.kind.for_target(t), mode, @@ -541,6 +573,7 @@ fn dep_build_script( unit, &unit.pkg, t, + None, script_unit_for, unit.kind, CompileMode::RunCustomBuild, @@ -574,6 +607,7 @@ fn new_unit_dep( parent: &Unit, pkg: &Package, target: &Target, + dependency: Option, unit_for: UnitFor, kind: CompileKind, mode: CompileMode, @@ -587,7 +621,9 @@ fn new_unit_dep( mode, kind, ); - new_unit_dep_with_profile(state, parent, pkg, target, unit_for, kind, mode, profile) + new_unit_dep_with_profile( + state, parent, pkg, target, dependency, unit_for, kind, mode, profile, + ) } fn new_unit_dep_with_profile( @@ -595,6 +631,7 @@ fn new_unit_dep_with_profile( parent: &Unit, pkg: &Package, target: &Target, + dependency: Option, unit_for: UnitFor, kind: CompileKind, mode: CompileMode, @@ -616,6 +653,7 @@ fn new_unit_dep_with_profile( .intern(pkg, target, profile, kind, mode, features, state.is_std, 0); Ok(UnitDep { unit, + dependency: UnitDependency(dependency), unit_for, extern_crate_name, public, diff --git a/src/cargo/core/compiler/unit_graph.rs b/src/cargo/core/compiler/unit_graph.rs index 1357afb93f8..d8d5c50ea25 100644 --- a/src/cargo/core/compiler/unit_graph.rs +++ b/src/cargo/core/compiler/unit_graph.rs @@ -1,11 +1,12 @@ use crate::core::compiler::Unit; use crate::core::compiler::{CompileKind, CompileMode}; use crate::core::profiles::{Profile, UnitFor}; -use crate::core::{PackageId, Target}; +use crate::core::{Dependency, PackageId, Target}; use crate::util::interning::InternedString; use crate::util::CargoResult; use crate::Config; use std::collections::HashMap; +use std::hash::{Hash, Hasher}; use std::io::Write; /// The dependency graph of Units. @@ -16,6 +17,8 @@ pub type UnitGraph = HashMap>; pub struct UnitDep { /// The dependency unit. pub unit: Unit, + /// The manifest dependency that gave rise to this dependency + pub dependency: UnitDependency, /// The purpose of this dependency (a dependency for a test, or a build /// script, etc.). Do not use this after the unit graph has been built. pub unit_for: UnitFor, @@ -27,6 +30,15 @@ pub struct UnitDep { pub noprelude: bool, } +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)] +pub struct UnitDependency(pub Option); + +impl Hash for UnitDependency { + fn hash(&self, _: &mut H) { + // ... + } +} + const VERSION: u32 = 1; #[derive(serde::Serialize)] diff --git a/src/cargo/core/compiler/unused_dependencies.rs b/src/cargo/core/compiler/unused_dependencies.rs new file mode 100644 index 00000000000..115118acb13 --- /dev/null +++ b/src/cargo/core/compiler/unused_dependencies.rs @@ -0,0 +1,293 @@ +use super::unit::Unit; +use super::Context; +use crate::core::compiler::build_config::CompileMode; +use crate::core::dependency::DepKind; +use crate::core::manifest::TargetKind; +use crate::core::Dependency; +use crate::core::PackageId; +use crate::util::errors::CargoResult; +use crate::util::interning::InternedString; +use log::trace; + +use std::collections::{HashMap, HashSet}; + +pub type AllowedKinds = HashSet; + +#[derive(Default)] +struct State { + /// All externs of a root unit. + externs: HashMap>, + /// The used externs so far. + /// The DepKind is included so that we can tell when + /// a proper dependency should actually be a dev-dependency + used_externs: HashSet<(InternedString, DepKind)>, + reports_needed_by: HashSet, +} + +pub struct UnusedDepState { + states: HashMap<(PackageId, Option), State>, + /// Tracking for which units we have received reports from. + /// + /// When we didn't receive reports, e.g. because of an error, + /// or because the compiler segfaulted, etc., we don't emit + /// any warnings for missing dependencies for the specific + /// class. + reports_obtained: HashSet, +} + +fn dep_kind_desc(kind: Option) -> &'static str { + match kind { + Some(kind) => match kind { + DepKind::Normal => "", + DepKind::Development => "dev-", + DepKind::Build => "build-", + }, + None => "internal-", + } +} + +fn dep_kind_of(unit: &Unit) -> DepKind { + match unit.target.kind() { + TargetKind::Lib(_) => match unit.mode { + // To support lib.rs with #[cfg(test)] use foo_crate as _; + CompileMode::Test => DepKind::Development, + _ => DepKind::Normal, + }, + TargetKind::Bin => DepKind::Normal, + TargetKind::Test => DepKind::Development, + TargetKind::Bench => DepKind::Development, + TargetKind::ExampleLib(_) => DepKind::Development, + TargetKind::ExampleBin => DepKind::Development, + TargetKind::CustomBuild => DepKind::Build, + } +} + +fn unit_desc(unit: &Unit) -> String { + format!( + "{}/{}+{:?}", + unit.target.name(), + unit.target.kind().description(), + unit.mode, + ) +} + +impl UnusedDepState { + pub fn new_with_graph(cx: &mut Context<'_, '_>) -> Self { + let mut states = HashMap::<_, State>::new(); + + let roots_without_build = &cx.bcx.roots; + + // Compute the build scripts of the roots so that we can + // lint for unused [build-dependencies]. + // First iterate on the root's dependencies, + // searching for the build-script-run units. + // Obtain the build-script-build units from those by + // another iteration, as only they depend on the + // [build-dependencies] of a package. + let mut build_root_runs = HashSet::new(); + for root in roots_without_build.iter() { + for dep in cx.unit_deps(root).iter() { + if dep.unit.pkg.package_id() != root.pkg.package_id() { + continue; + } + if !dep.unit.target.is_custom_build() { + continue; + } + build_root_runs.insert(dep.unit.clone()); + } + } + let mut build_roots = HashSet::new(); + for root in build_root_runs.iter() { + for dep in cx.unit_deps(root).iter() { + if dep.unit.pkg.package_id() != root.pkg.package_id() { + continue; + } + if !dep.unit.target.is_custom_build() { + continue; + } + if dep.unit.mode != CompileMode::Build { + continue; + } + build_roots.insert(dep.unit.clone()); + } + } + + // Now build the datastructures + for root in roots_without_build.iter().chain(build_roots.iter()) { + let pkg_id = root.pkg.package_id(); + trace!( + "Udeps root package {} tgt {}", + root.pkg.name(), + unit_desc(root), + ); + // We aren't getting output from doctests, so skip them (for now) + if root.mode == CompileMode::Doctest { + trace!(" -> skipping doctest"); + continue; + } + for dep in cx.unit_deps(root).iter() { + trace!( + " => {} {}", + dep.unit.pkg.name(), + dep.dependency.0.is_some() + ); + let dependency = if let Some(dependency) = &dep.dependency.0 { + Some(dependency.clone()) + } else if dep.unit.pkg.package_id() == root.pkg.package_id() { + None + } else { + continue; + }; + let kind = dependency.as_ref().map(|dependency| dependency.kind()); + let state = states + .entry((pkg_id, kind)) + .or_insert_with(Default::default); + state.externs.insert(dep.extern_crate_name, dependency); + state.reports_needed_by.insert(root.clone()); + } + } + + Self { + states, + reports_obtained: HashSet::new(), + } + } + /// Records the unused externs coming from the compiler by first inverting them to the used externs + /// and then updating the global list of used externs + pub fn record_unused_externs_for_unit( + &mut self, + cx: &mut Context<'_, '_>, + unit: &Unit, + unused_externs: Vec, + ) { + self.reports_obtained.insert(unit.clone()); + let usable_deps_iter = cx + .unit_deps(unit) + .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 + .iter() + .map(|ex| InternedString::new(ex)) + .collect::>(); + let used_deps_iter = + usable_deps_iter.filter(|dep| !unused_externs_set.contains(&dep.extern_crate_name)); + let pkg_id = unit.pkg.package_id(); + for used_dep in used_deps_iter { + trace!( + "Used extern {} for pkg {} v{} tgt {}", + used_dep.extern_crate_name, + pkg_id.name(), + pkg_id.version(), + unit_desc(unit), + ); + let kind = if let Some(dependency) = &used_dep.dependency.0 { + Some(dependency.kind()) + } else if used_dep.unit.pkg.package_id() == unit.pkg.package_id() { + // Deps within the same crate have no dependency entry + None + } else { + continue; + }; + if let Some(state) = self.states.get_mut(&(pkg_id, kind)) { + let record_kind = dep_kind_of(unit); + trace!( + " => updating state of {}dep", + dep_kind_desc(Some(record_kind)) + ); + state + .used_externs + .insert((used_dep.extern_crate_name, record_kind)); + } + } + } + pub fn emit_unused_warnings(&self, cx: &mut Context<'_, '_>) -> CargoResult<()> { + trace!( + "Allowed dependency kinds for the unused deps check: {:?}", + cx.bcx.allowed_kinds + ); + + // 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 + continue; + } + // Implicitly added dependencies (in the same crate) aren't interesting + let dependency = if let Some(dependency) = dependency { + dependency + } else { + continue; + }; + if !cx.bcx.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; + } + 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 + cx.bcx.config.shell().warn(format!( + "dependency {} in package {} v{} is only used by dev targets", + dependency.name_in_toml(), + pkg_id.name(), + pkg_id.version() + ))?; + continue; + } + + cx.bcx.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() + ))?; + } + } + Ok(()) + } +} diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 88b4e47e787..b4d6274c9d3 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -582,6 +582,7 @@ pub struct CliUnstable { pub timings: Option>, pub unstable_options: bool, pub weak_dep_features: bool, + pub warn_unused_deps: bool, } const STABILIZED_COMPILE_PROGRESS: &str = "The progress bar is now always \ @@ -799,6 +800,7 @@ impl CliUnstable { "crate-versions" => stabilized_warn(k, "1.47", STABILIZED_CRATE_VERSIONS), "package-features" => stabilized_warn(k, "1.51", STABILIZED_PACKAGE_FEATURES), "future-incompat-report" => self.enable_future_incompat_feature = parse_empty(k, v)?, + "warn-unused-deps" => self.warn_unused_deps = parse_empty(k, v)?, _ => bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 44945d91986..f03512c17de 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -28,10 +28,12 @@ use std::sync::Arc; use crate::core::compiler::unit_dependencies::build_unit_dependencies; use crate::core::compiler::unit_graph::{self, UnitDep, UnitGraph}; +use crate::core::compiler::unused_dependencies::AllowedKinds; use crate::core::compiler::{standard_lib, TargetInfo}; use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context}; use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit}; use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner}; +use crate::core::dependency::DepKind; use crate::core::profiles::{Profiles, UnitFor}; use crate::core::resolver::features::{self, CliFeatures, FeaturesFor}; use crate::core::resolver::{HasDevUnits, Resolve}; @@ -467,7 +469,7 @@ pub fn create_bcx<'a, 'cfg>( }) .collect(); - let mut units = generate_targets( + let (mut units, allowed_kinds) = generate_targets( ws, &to_builds, filter, @@ -610,6 +612,7 @@ pub fn create_bcx<'a, 'cfg>( ws, pkg_set, build_config, + allowed_kinds, profiles, extra_compiler_args, target_data, @@ -849,7 +852,7 @@ fn generate_targets( package_set: &PackageSet<'_>, profiles: &Profiles, interner: &UnitInterner, -) -> CargoResult> { +) -> CargoResult<(Vec, AllowedKinds)> { let config = ws.config(); // Helper for creating a list of `Unit` structures let new_unit = @@ -941,6 +944,9 @@ fn generate_targets( // Create a list of proposed targets. let mut proposals: Vec> = Vec::new(); + let mut allowed_kinds = HashSet::::new(); + allowed_kinds.insert(DepKind::Build); + match *filter { CompileFilter::Default { required_features_filterable, @@ -968,6 +974,8 @@ fn generate_targets( } } } + allowed_kinds.insert(DepKind::Normal); + allowed_kinds.insert(DepKind::Development); } CompileFilter::Only { all_targets, @@ -1027,6 +1035,21 @@ fn generate_targets( _ => mode, }; + if *lib != LibRule::False { + match (bins, examples, tests, benches) { + (FilterRule::All, FilterRule::All, FilterRule::All, FilterRule::All) => { + allowed_kinds.insert(DepKind::Development); + } + _ => (), + } + match (bins, examples, tests, benches) { + (FilterRule::All, ..) => { + allowed_kinds.insert(DepKind::Normal); + } + _ => (), + } + } + proposals.extend(list_rule_targets( packages, bins, @@ -1109,7 +1132,8 @@ fn generate_targets( } // else, silently skip target. } - Ok(units.into_iter().collect()) + + Ok((units.into_iter().collect(), allowed_kinds)) } /// Warns if a target's required-features references a feature that doesn't exist. diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 6f5556966e5..d091c079fba 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -123,6 +123,7 @@ mod tool_paths; mod tree; mod tree_graph_features; mod unit_graph; +mod unused_dependencies; mod update; mod vendor; mod verify_project; diff --git a/tests/testsuite/unused_dependencies.rs b/tests/testsuite/unused_dependencies.rs new file mode 100644 index 00000000000..8e7ab4a7487 --- /dev/null +++ b/tests/testsuite/unused_dependencies.rs @@ -0,0 +1,757 @@ +//! A test suite for `-Zwarn-unused-dependencies` +//! +//! All tests here should use `#[cargo_test(unused_dependencies)]` to indicate that +//! boilerplate should be generated to require the nightly toolchain. +//! Otherwise the tests are skipped. + +use cargo_test_support::project; +use cargo_test_support::registry::Package; + +// TODO more dev deps tests +// TODO more commands for the tests to test the allowed kinds logic +// TODO document the tests + +#[cargo_test(unused_dependencies)] +fn unused_proper_dep() { + // The most basic case where there is an unused dependency + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + bar = "0.1.0" + "#, + ) + .file( + "src/main.rs", + r#" + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] bar v0.1.0 +[COMPILING] foo [..] +[WARNING] unused dependency bar in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_build_dep() { + // A build dependency is unused + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [build-dependencies] + bar = "0.1.0" + "#, + ) + .file( + "src/main.rs", + r#" + fn main() {} + "#, + ) + .file( + "build.rs", + r#" + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] bar v0.1.0 +[COMPILING] foo [..] +[WARNING] unused build-dependency bar in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_deps_multiple() { + // Multiple dependencies are unused, + // also test that re-using dependencies + // between proper and build deps doesn't + // confuse the lint + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "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" + + [dependencies] + bar = "0.1.0" + baz = "0.1.0" + qux = "0.1.0" + quux = "0.1.0" + + [build-dependencies] + bar = "0.1.0" + baz = "0.1.0" + qux = "0.1.0" + "#, + ) + .file( + "src/main.rs", + r#" + use qux as _; + fn main() {} + "#, + ) + .file( + "build.rs", + r#" + use baz as _; + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] qux v0.1.0 [..] +[DOWNLOADED] quux v0.1.0 [..] +[DOWNLOADED] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] unused dependency bar in package foo v0.1.0 +[WARNING] unused dependency baz in package foo v0.1.0 +[WARNING] unused dependency quux in package foo v0.1.0 +[WARNING] unused build-dependency bar in package foo v0.1.0 +[WARNING] unused build-dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_build_dep_used_proper() { + // Check sharingof a dependency + // between build and proper deps + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + bar = "0.1.0" + + [build-dependencies] + bar = "0.1.0" + "#, + ) + .file( + "src/main.rs", + r#" + use bar as _; + fn main() {} + "#, + ) + .file( + "build.rs", + r#" + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] bar v0.1.0 +[COMPILING] foo [..] +[WARNING] unused build-dependency bar in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_dep_renamed() { + // Make sure that package renaming works + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.2.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + baz = { package = "bar", version = "0.1.0" } + bar = { package = "baz", version = "0.2.0" } + "#, + ) + .file( + "src/main.rs", + r#" + use bar as _; + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] baz v0.2.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] unused dependency baz in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_proper_dep_allowed() { + // There is an unused dependency but it's marked as + // allowed due to the leading underscore + Package::new("bar", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + _bar = { package = "bar", version = "0.1.0" } + "#, + ) + .file( + "src/main.rs", + r#" + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] bar v0.1.0 +[COMPILING] foo [..] +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn unused_dep_lib_bin() { + // Make sure that dependency uses by both binaries and libraries + // are being registered as uses. + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + Package::new("qux", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + bar = "0.1.0" + baz = "0.1.0" + qux = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + use baz as _; + "#, + ) + .file( + "src/main.rs", + r#" + use bar as _; + fn main() {} + "#, + ) + .build(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] qux v0.1.0 [..] +[DOWNLOADED] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] unused dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn should_be_dev() { + // Test the warning that a dependency should be a dev dep. + // Sometimes, a cargo command doesn't compile the dev unit + // that would use the dependency and thus will claim that + // the dependency is unused while it actually is used. + // However, this behaviour is common in unused lints: + // e.g. when you cfg-gate a public function foo that uses + // a function bar, the bar function will be marked as + // unused even though there is a mode that uses bar. + // + // So the "should be dev" lint should be seen as a + // best-effort improvement over the "unused dep" lint. + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "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" + + [dependencies] + bar = "0.1.0" + baz = "0.1.0" + qux = "0.1.0" + quux = "0.1.0" + "#, + ) + .file("src/lib.rs", "") + .file( + "tests/hello.rs", + r#" + use bar as _; + "#, + ) + .file( + "examples/hello.rs", + r#" + use baz as _; + fn main() {} + "#, + ) + .file( + "benches/hello.rs", + r#" + use qux as _; + fn main() {} + "#, + ) + .build(); + + 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] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] dependency bar in package foo v0.1.0 is only used by dev targets +[WARNING] dependency baz in package foo v0.1.0 is only used by dev targets +[WARNING] unused dependency quux in package foo v0.1.0 +[WARNING] unused dependency qux in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + p.cargo("test --no-run --all-targets -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[WARNING] dependency bar in package foo v0.1.0 is only used by dev targets +[WARNING] dependency baz in package foo v0.1.0 is only used by dev targets +[WARNING] unused dependency quux in package foo v0.1.0 +[WARNING] dependency qux in package foo v0.1.0 is only used by dev targets +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[WARNING] unused dependency bar in package foo v0.1.0 +[WARNING] unused dependency baz in package foo v0.1.0 +[WARNING] unused dependency quux in package foo v0.1.0 +[WARNING] unused dependency qux in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + p.cargo("check --all-targets -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] foo [..] +[WARNING] dependency bar in package foo v0.1.0 is only used by dev targets +[WARNING] dependency baz in package foo v0.1.0 is only used by dev targets +[WARNING] unused dependency quux in package foo v0.1.0 +[WARNING] dependency qux in package foo v0.1.0 is only used by dev targets +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[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("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" + + [dev-dependencies] + bar = "0.1.0" + baz = "0.1.0" + qux = "0.1.0" + quux = "0.1.0" + "#, + ) + .file("src/lib.rs", "") + .file( + "tests/hello.rs", + r#" + use bar as _; + "#, + ) + .file( + "examples/hello.rs", + r#" + use baz as _; + fn main() {} + "#, + ) + .file( + "benches/hello.rs", + r#" + use qux as _; + fn main() {} + "#, + ) + .build(); + + // cargo test --no-run doesn't test the benches + // and thus gives false positives + 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] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] unused dev-dependency quux in package foo v0.1.0 +[WARNING] unused dev-dependency qux in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // The cargo test --no-run --all-targets tests + // everything that can use dev-deps except for doctests + p.cargo("test --no-run --all-targets -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] foo [..] +[WARNING] unused dev-dependency quux in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // Check that cargo build doesn't check for unused dev-deps + p.cargo("build -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + // cargo check --all-targets checks for unused dev-deps (for now) + p.cargo("check --all-targets -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] foo [..] +[WARNING] unused dev-dependency quux in package foo v0.1.0 +[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 + // is still registered as used. + Package::new("bar", "0.1.0").publish(); + Package::new("baz", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dev-dependencies] + bar = "0.1.0" + baz = "0.1.0" + "#, + ) + .file( + "src/lib.rs", + r#" + #[cfg(test)] + mod tests { + use bar as _; + } + "#, + ) + .build(); + + p.cargo("test --no-run -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] [..] +[DOWNLOADED] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] +[WARNING] unused dev-dependency baz in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} + +#[cargo_test(unused_dependencies)] +fn cfg_test_workspace() { + // Make sure that workspaces are supported, + // --all params, -p params, etc. + Package::new("baz", "0.1.0").publish(); + Package::new("qux", "0.1.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + bar = { path = "bar" } + baz = "0.1.0" + + [workspace] + members = ["bar"] + "#, + ) + .file( + "src/lib.rs", + r#" + use bar as _; + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + version = "0.1.0" + authors = [] + edition = "2018" + + [dependencies] + baz = "0.1.0" + qux = "0.1.0" + "#, + ) + .file( + "bar/src/lib.rs", + r#" + use baz as _; + "#, + ) + .build(); + + p.cargo("check --all -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[UPDATING] [..] +[DOWNLOADING] [..] +[DOWNLOADED] [..] +[DOWNLOADED] [..] +[CHECKING] [..] +[CHECKING] [..] +[CHECKING] bar [..] +[CHECKING] foo [..] +[WARNING] unused dependency qux in package bar v0.1.0 +[WARNING] unused dependency baz in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + p.cargo("check -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[WARNING] unused dependency baz in package foo v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); + + p.cargo("check -p bar -Zwarn-unused-deps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[WARNING] unused dependency qux in package bar v0.1.0 +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +} From 7bccfc316e4126b600e6c7883439362809a05c29 Mon Sep 17 00:00:00 2001 From: est31 Date: Wed, 5 Aug 2020 07:02:56 +0200 Subject: [PATCH 3/4] Support doctests --- src/bin/cargo/cli.rs | 1 + src/cargo/core/compiler/compilation.rs | 8 +- src/cargo/core/compiler/context/mod.rs | 5 +- src/cargo/core/compiler/job_queue.rs | 22 +++-- .../core/compiler/unused_dependencies.rs | 53 +++++++---- src/cargo/ops/cargo_compile.rs | 7 -- src/cargo/ops/cargo_test.rs | 39 ++++++++- tests/testsuite/unused_dependencies.rs | 87 +++++++++++++++---- 8 files changed, 169 insertions(+), 53 deletions(-) diff --git a/src/bin/cargo/cli.rs b/src/bin/cargo/cli.rs index 55d7bd9bfdc..2b1507b220f 100644 --- a/src/bin/cargo/cli.rs +++ b/src/bin/cargo/cli.rs @@ -46,6 +46,7 @@ Available unstable (nightly-only) flags: -Z terminal-width -- Provide a terminal width to rustc for error truncation -Z namespaced-features -- Allow features with `dep:` prefix -Z weak-dep-features -- Allow `dep_name?/feature` feature syntax + -Z warn-unused-deps -- Emit warnings about unused dependencies -Z patch-in-config -- Allow `[patch]` sections in .cargo/config.toml files Run with 'cargo -Z [FLAG] [SUBCOMMAND]'" diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index c1f5abce34c..ebb2e7ec2d7 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -7,7 +7,8 @@ use cargo_platform::CfgExpr; use cargo_util::{paths, ProcessBuilder}; use semver::Version; -use super::BuildContext; +use super::unused_dependencies::UnusedDepState; +use super::{BuildContext, UnitDep}; use crate::core::compiler::{CompileKind, Metadata, Unit}; use crate::core::Package; use crate::util::{config, CargoResult, Config}; @@ -16,6 +17,8 @@ use crate::util::{config, CargoResult, Config}; pub struct Doctest { /// What's being doctested pub unit: Unit, + /// Dependencies of the unit + pub unit_deps: Vec, /// Arguments needed to pass to rustdoc to run this test. pub args: Vec, /// Whether or not -Zunstable-options is needed. @@ -86,6 +89,8 @@ pub struct Compilation<'cfg> { /// The target host triple. pub host: String, + pub(crate) unused_dep_state: Option, + config: &'cfg Config, /// Rustc process to be used by default @@ -141,6 +146,7 @@ impl<'cfg> Compilation<'cfg> { to_doc_test: Vec::new(), config: bcx.config, host: bcx.host_triple().to_string(), + unused_dep_state: None, rustc_process: rustc, rustc_workspace_wrapper_process, primary_rustc_process, diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index cb1dbb6240b..89428d82275 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/src/cargo/core/compiler/context/mod.rs @@ -168,7 +168,9 @@ impl<'a, 'cfg> Context<'a, 'cfg> { } // Now that we've figured out everything that we're going to do, do it! - queue.execute(&mut self, &mut plan)?; + let unused_dep_state = queue.execute(&mut self, &mut plan)?; + + self.compilation.unused_dep_state = Some(unused_dep_state); if build_plan { plan.set_inputs(self.build_plan_inputs()?); @@ -255,6 +257,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { self.compilation.to_doc_test.push(compilation::Doctest { unit: unit.clone(), + unit_deps: self.unit_deps(&unit).to_vec(), args, unstable_opts, linker: self.bcx.linker(unit.kind), diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs index ffb506910c2..7bae8ff0af9 100644 --- a/src/cargo/core/compiler/job_queue.rs +++ b/src/cargo/core/compiler/job_queue.rs @@ -415,7 +415,11 @@ impl<'cfg> JobQueue<'cfg> { /// This function will spawn off `config.jobs()` workers to build all of the /// necessary dependencies, in order. Freshness is propagated as far as /// possible along each dependency chain. - pub fn execute(mut self, cx: &mut Context<'_, '_>, plan: &mut BuildPlan) -> CargoResult<()> { + pub fn execute( + mut self, + cx: &mut Context<'_, '_>, + plan: &mut BuildPlan, + ) -> CargoResult { let _p = profile::start("executing the job graph"); self.queue.queue_finished(); @@ -435,7 +439,7 @@ impl<'cfg> JobQueue<'cfg> { progress, next_id: 0, timings: self.timings, - unused_dep_state: UnusedDepState::new_with_graph(cx), // TODO + unused_dep_state: UnusedDepState::new_with_graph(cx), tokens: Vec::new(), rustc_tokens: HashMap::new(), to_send_clients: BTreeMap::new(), @@ -629,8 +633,12 @@ impl<'cfg> DrainState<'cfg> { } Message::UnusedExterns(id, unused_externs) => { let unit = &self.active[&id]; - self.unused_dep_state - .record_unused_externs_for_unit(cx, unit, unused_externs); + let unit_deps = cx.unit_deps(&unit); + self.unused_dep_state.record_unused_externs_for_unit( + unit_deps, + unit, + unused_externs, + ); } Message::Token(acquired_token) => { let token = acquired_token.chain_err(|| "failed to acquire jobserver token")?; @@ -716,7 +724,7 @@ impl<'cfg> DrainState<'cfg> { plan: &mut BuildPlan, scope: &Scope<'_>, jobserver_helper: &HelperThread, - ) -> (Result<(), anyhow::Error>,) { + ) -> (Result,) { trace!("queue: {:#?}", self.queue); // Iteratively execute the entire dependency graph. Each turn of the @@ -805,7 +813,7 @@ impl<'cfg> DrainState<'cfg> { } if !cx.bcx.build_config.build_plan && cx.bcx.config.cli_unstable().warn_unused_deps { - drop(self.unused_dep_state.emit_unused_warnings(cx)); + drop(self.unused_dep_state.emit_unused_early_warnings(cx)); } if let Some(e) = error { @@ -821,7 +829,7 @@ impl<'cfg> DrainState<'cfg> { self.emit_future_incompat(cx); } - (Ok(()),) + (Ok(self.unused_dep_state),) } else { debug!("queue: {:#?}", self.queue); (Err(internal("finished with jobs still left in the queue")),) diff --git a/src/cargo/core/compiler/unused_dependencies.rs b/src/cargo/core/compiler/unused_dependencies.rs index 115118acb13..4494dfc4078 100644 --- a/src/cargo/core/compiler/unused_dependencies.rs +++ b/src/cargo/core/compiler/unused_dependencies.rs @@ -1,5 +1,5 @@ use super::unit::Unit; -use super::Context; +use super::{Context, UnitDep}; use crate::core::compiler::build_config::CompileMode; use crate::core::dependency::DepKind; use crate::core::manifest::TargetKind; @@ -7,13 +7,14 @@ use crate::core::Dependency; use crate::core::PackageId; use crate::util::errors::CargoResult; use crate::util::interning::InternedString; +use crate::Config; use log::trace; use std::collections::{HashMap, HashSet}; pub type AllowedKinds = HashSet; -#[derive(Default)] +#[derive(Default, Clone)] struct State { /// All externs of a root unit. externs: HashMap>, @@ -24,6 +25,7 @@ struct State { reports_needed_by: HashSet, } +#[derive(Clone)] pub struct UnusedDepState { states: HashMap<(PackageId, Option), State>, /// Tracking for which units we have received reports from. @@ -51,6 +53,8 @@ fn dep_kind_of(unit: &Unit) -> DepKind { TargetKind::Lib(_) => match unit.mode { // To support lib.rs with #[cfg(test)] use foo_crate as _; CompileMode::Test => DepKind::Development, + // To correctly register dev-dependencies + CompileMode::Doctest => DepKind::Development, _ => DepKind::Normal, }, TargetKind::Bin => DepKind::Normal, @@ -120,10 +124,9 @@ impl UnusedDepState { root.pkg.name(), unit_desc(root), ); - // We aren't getting output from doctests, so skip them (for now) if root.mode == CompileMode::Doctest { - trace!(" -> skipping doctest"); - continue; + //trace!(" -> skipping doctest"); + //continue; } for dep in cx.unit_deps(root).iter() { trace!( @@ -156,13 +159,12 @@ impl UnusedDepState { /// and then updating the global list of used externs pub fn record_unused_externs_for_unit( &mut self, - cx: &mut Context<'_, '_>, + unit_deps: &[UnitDep], unit: &Unit, unused_externs: Vec, ) { self.reports_obtained.insert(unit.clone()); - let usable_deps_iter = cx - .unit_deps(unit) + 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()); @@ -194,7 +196,7 @@ impl UnusedDepState { let record_kind = dep_kind_of(unit); trace!( " => updating state of {}dep", - dep_kind_desc(Some(record_kind)) + dep_kind_desc(Some(record_kind)), ); state .used_externs @@ -202,10 +204,20 @@ impl UnusedDepState { } } } - pub fn emit_unused_warnings(&self, cx: &mut Context<'_, '_>) -> CargoResult<()> { + pub fn emit_unused_early_warnings(&self, cx: &mut Context<'_, '_>) -> CargoResult<()> { + self.emit_unused_warnings_inner(cx.bcx.config, Some(&cx.bcx.allowed_kinds)) + } + pub fn emit_unused_late_warnings(&self, config: &Config) -> CargoResult<()> { + self.emit_unused_warnings_inner(config, None) + } + fn emit_unused_warnings_inner( + &self, + config: &Config, + allowed_kinds_or_late: Option<&AllowedKinds>, + ) -> CargoResult<()> { trace!( "Allowed dependency kinds for the unused deps check: {:?}", - cx.bcx.allowed_kinds + allowed_kinds_or_late ); // Sort the states to have a consistent output @@ -247,12 +259,15 @@ impl UnusedDepState { } else { continue; }; - if !cx.bcx.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; + 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 @@ -270,7 +285,7 @@ impl UnusedDepState { { // The dependency is used but only by dev targets, // which means it should be a dev-dependency instead - cx.bcx.config.shell().warn(format!( + config.shell().warn(format!( "dependency {} in package {} v{} is only used by dev targets", dependency.name_in_toml(), pkg_id.name(), @@ -279,7 +294,7 @@ impl UnusedDepState { continue; } - cx.bcx.config.shell().warn(format!( + config.shell().warn(format!( "unused {}dependency {} in package {} v{}", dep_kind_desc(Some(*dep_kind)), dependency.name_in_toml(), diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index f03512c17de..6e74800a40e 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -975,7 +975,6 @@ fn generate_targets( } } allowed_kinds.insert(DepKind::Normal); - allowed_kinds.insert(DepKind::Development); } CompileFilter::Only { all_targets, @@ -1036,12 +1035,6 @@ fn generate_targets( }; if *lib != LibRule::False { - match (bins, examples, tests, benches) { - (FilterRule::All, FilterRule::All, FilterRule::All, FilterRule::All) => { - allowed_kinds.insert(DepKind::Development); - } - _ => (), - } match (bins, examples, tests, benches) { (FilterRule::All, ..) => { allowed_kinds.insert(DepKind::Normal); diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs index 9fcb94f1336..340f261534c 100644 --- a/src/cargo/ops/cargo_test.rs +++ b/src/cargo/ops/cargo_test.rs @@ -168,6 +168,7 @@ fn run_doc_tests( args, unstable_opts, unit, + unit_deps, linker, script_meta, } = doctest_info; @@ -228,6 +229,12 @@ fn run_doc_tests( p.arg("-L").arg(arg); } + if config.cli_unstable().warn_unused_deps { + p.arg("-Z").arg("unstable-options"); + p.arg("--error-format=json"); + p.arg("--json=unused-externs"); + } + for native_dep in compilation.native_dirs.iter() { p.arg("-L").arg(native_dep); } @@ -245,13 +252,43 @@ fn run_doc_tests( config .shell() .verbose(|shell| shell.status("Running", p.to_string()))?; - if let Err(e) = p.exec() { + + let mut unused_dep_state = compilation.unused_dep_state.clone().unwrap(); + if let Err(e) = p.exec_with_streaming( + &mut |line| { + writeln!(config.shell().out(), "{}", line)?; + 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, + ); + // Supress output of the json formatted unused extern message + return Ok(()); + } + writeln!(config.shell().err(), "{}", line)?; + Ok(()) + }, + false, + ) { let e = e.downcast::()?; errors.push(e); if !options.no_fail_fast { return Ok((Test::Doc, errors)); } } + if config.cli_unstable().warn_unused_deps { + // Emit unused dependencies report which has been held back + // until now, as doctests could use things + unused_dep_state.emit_unused_late_warnings(config)?; + } } Ok((Test::Doc, errors)) } diff --git a/tests/testsuite/unused_dependencies.rs b/tests/testsuite/unused_dependencies.rs index 8e7ab4a7487..94370ca6d89 100644 --- a/tests/testsuite/unused_dependencies.rs +++ b/tests/testsuite/unused_dependencies.rs @@ -3,11 +3,13 @@ //! All tests here should use `#[cargo_test(unused_dependencies)]` to indicate that //! boilerplate should be generated to require the nightly toolchain. //! Otherwise the tests are skipped. +//! +//! In order to debug a test, you can add an env var like: +//! .env("CARGO_LOG", "cargo::core::compiler::unused_dependencies=trace") use cargo_test_support::project; use cargo_test_support::registry::Package; -// TODO more dev deps tests // TODO more commands for the tests to test the allowed kinds logic // TODO document the tests @@ -406,7 +408,7 @@ fn should_be_dev() { bar = "0.1.0" baz = "0.1.0" qux = "0.1.0" - quux = "0.1.0" + quux = "0.1.0" # only genuinely unused dep "#, ) .file("src/lib.rs", "") @@ -432,6 +434,10 @@ 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 p.cargo("test --no-run -Zwarn-unused-deps") .masquerade_as_nightly_cargo() .with_stderr( @@ -451,6 +457,26 @@ fn should_be_dev() { [WARNING] dependency baz in package foo v0.1.0 is only used by dev targets [WARNING] unused dependency quux in package foo v0.1.0 [WARNING] unused dependency qux in package foo v0.1.0 +[FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ + ", + ) + .run(); +*/ + 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] baz v0.1.0 [..] +[DOWNLOADED] bar v0.1.0 [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] [..] +[COMPILING] foo [..] [FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ ", ) @@ -508,6 +534,7 @@ fn dev_deps() { // In this instance, 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() @@ -523,11 +550,20 @@ fn dev_deps() { [dev-dependencies] bar = "0.1.0" baz = "0.1.0" + baz2 = "0.1.0" qux = "0.1.0" - quux = "0.1.0" + quux = "0.1.0" # only genuinely unused dep + "#, + ) + .file( + "src/lib.rs", + r#" + /// ``` + /// use baz2 as _; extern crate baz2; + /// ``` + pub fn foo() {} "#, ) - .file("src/lib.rs", "") .file( "tests/hello.rs", r#" @@ -550,8 +586,8 @@ fn dev_deps() { ) .build(); - // cargo test --no-run doesn't test the benches - // and thus gives false positives + // 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( @@ -560,33 +596,49 @@ fn dev_deps() { [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] [..] [COMPILING] foo [..] -[WARNING] unused dev-dependency quux in package foo v0.1.0 -[WARNING] unused dev-dependency qux in package foo v0.1.0 [FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ ", ) .run(); - // The cargo test --no-run --all-targets tests - // everything that can use dev-deps except for doctests + // 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( "\ [COMPILING] foo [..] -[WARNING] unused dev-dependency quux 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] [..] +[..] +[WARNING] unused dev-dependency quux in package foo v0.1.0 +[WARNING] unused dev-dependency qux in package foo v0.1.0\ + ", + ) + .run(); + // Check that cargo build doesn't check for unused dev-deps p.cargo("build -Zwarn-unused-deps") .masquerade_as_nightly_cargo() @@ -597,7 +649,7 @@ fn dev_deps() { ) .run(); - // cargo check --all-targets checks for unused dev-deps (for now) + // cargo check --all-targets doesn't check for unused dev-deps (no doctests ran) p.cargo("check --all-targets -Zwarn-unused-deps") .masquerade_as_nightly_cargo() .with_stderr( @@ -606,8 +658,8 @@ fn dev_deps() { [CHECKING] [..] [CHECKING] [..] [CHECKING] [..] +[CHECKING] [..] [CHECKING] foo [..] -[WARNING] unused dev-dependency quux in package foo v0.1.0 [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]\ ", ) @@ -618,6 +670,10 @@ fn dev_deps() { fn cfg_test_used() { // Ensure that a dependency only used from #[cfg(test)] code // is still registered as used. + + // TODO: this test currently doesn't actually test that bar is used + // because the warning is witheld, waiting for doctests that never happen. + // It's due to a bug in cargo test --no-run Package::new("bar", "0.1.0").publish(); Package::new("baz", "0.1.0").publish(); let p = project() @@ -632,7 +688,7 @@ fn cfg_test_used() { [dev-dependencies] bar = "0.1.0" - baz = "0.1.0" + #baz = "0.1.0" "#, ) .file( @@ -653,11 +709,8 @@ fn cfg_test_used() { [UPDATING] [..] [DOWNLOADING] [..] [DOWNLOADED] [..] -[DOWNLOADED] [..] -[COMPILING] [..] [COMPILING] [..] [COMPILING] foo [..] -[WARNING] unused dev-dependency baz in package foo v0.1.0 [FINISHED] test [unoptimized + debuginfo] target(s) in [..]\ ", ) From c0e3e5ae44669089ec903f6bb6785b191172f74c Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 10 Aug 2020 02:19:09 +0200 Subject: [PATCH 4/4] 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