Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add flag to warn about unused dependencies #8437

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions crates/cargo-test-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 }",
);
Expand Down
1 change: 1 addition & 0 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]'"
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<Unit, Vec<String>>,

Expand All @@ -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<Unit, Vec<String>>,
target_data: RustcTargetData,
Expand All @@ -74,6 +77,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
config: ws.config(),
packages,
build_config,
allowed_kinds,
profiles,
extra_compiler_args,
target_data,
Expand Down
8 changes: 7 additions & 1 deletion src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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<UnitDep>,
/// Arguments needed to pass to rustdoc to run this test.
pub args: Vec<OsString>,
/// Whether or not -Zunstable-options is needed.
Expand Down Expand Up @@ -86,6 +89,8 @@ pub struct Compilation<'cfg> {
/// The target host triple.
pub host: String,

pub(crate) unused_dep_state: Option<UnusedDepState>,

config: &'cfg Config,

/// Rustc process to be used by default
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()?);
Expand Down Expand Up @@ -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),
Expand Down
55 changes: 42 additions & 13 deletions src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ use super::job::{
Job,
};
use super::timings::Timings;
use super::unused_dependencies::{UnusedDepState, UnusedExterns};
use super::{BuildContext, BuildPlan, CompileMode, Context, Unit};
use crate::core::compiler::future_incompat::{
FutureBreakageItem, OnDiskReport, FUTURE_INCOMPAT_FILE,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -242,6 +244,7 @@ enum Message {
Token(io::Result<Acquired>),
Finish(JobId, Artifact, CargoResult<()>),
FutureIncompatReport(JobId, Vec<FutureBreakageItem>),
UnusedExterns(JobId, UnusedExterns),

// This client should get release_raw called on it with one of our tokens
NeedsToken(JobId),
Expand Down Expand Up @@ -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: UnusedExterns) {
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.
///
Expand Down Expand Up @@ -403,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<UnusedDepState> {
let _p = profile::start("executing the job graph");
self.queue.queue_finished();

Expand All @@ -423,6 +439,7 @@ impl<'cfg> JobQueue<'cfg> {
progress,
next_id: 0,
timings: self.timings,
unused_dep_state: UnusedDepState::new_with_graph(cx),
tokens: Vec::new(),
rustc_tokens: HashMap::new(),
to_send_clients: BTreeMap::new(),
Expand Down Expand Up @@ -457,10 +474,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")
}
Expand Down Expand Up @@ -616,6 +631,15 @@ 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];
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")?;
self.tokens.push(token);
Expand Down Expand Up @@ -691,15 +715,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<anyhow::Error> {
) -> (Result<UnusedDepState, anyhow::Error>,) {
trace!("queue: {:#?}", self.queue);

// Iteratively execute the entire dependency graph. Each turn of the
Expand Down Expand Up @@ -769,7 +794,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() {
Expand All @@ -782,13 +807,17 @@ 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 !cx.bcx.build_config.build_plan && cx.bcx.config.cli_unstable().warn_unused_deps {
drop(self.unused_dep_state.emit_unused_early_warnings(cx));
}

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 {}",
Expand All @@ -800,10 +829,10 @@ impl<'cfg> DrainState<'cfg> {
self.emit_future_incompat(cx);
}

None
(Ok(self.unused_dep_state),)
} 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")),)
}
}

Expand Down
33 changes: 30 additions & 3 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -49,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;
Expand Down Expand Up @@ -215,6 +217,10 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> 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);

Expand Down Expand Up @@ -614,7 +620,7 @@ fn rustdoc(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Work> {
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) {
Expand Down Expand Up @@ -715,7 +721,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 {
Expand All @@ -724,6 +735,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");
Expand Down Expand Up @@ -782,7 +799,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 {
Expand Down Expand Up @@ -1038,6 +1055,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 {
Expand Down Expand Up @@ -1322,6 +1343,12 @@ fn on_stderr_line_inner(
}
}

if let Ok(uext) = serde_json::from_str::<UnusedExterns>(compiler_message.get()) {
log::trace!("obtained unused externs message from rustc: `{:?}`", uext);
state.unused_externs(uext);
return Ok(true);
}

#[derive(serde::Deserialize)]
struct JobserverNotification {
jobserver_event: Event,
Expand Down
Loading