Skip to content

Commit

Permalink
Rollup merge of #73945 - est31:unused_externs, r=Mark-Simulacrum
Browse files Browse the repository at this point in the history
Add an unstable --json=unused-externs flag to print unused externs

This adds an unstable flag to print a list of the extern names not used by cargo.

This PR will enable cargo to collect unused dependencies from all units and provide warnings.
The companion PR to cargo is: rust-lang/cargo#8437

The goal is eventual stabilization of this flag in rustc as well as in cargo.

Discussion of this feature is mostly contained inside these threads: #57274 #72342 #72603

The feature builds upon the internal datastructures added by #72342

Externs are uniquely identified by name and the information is sufficient for cargo.
If the mode is enabled, rustc will print json messages like:

```
{"unused_extern_names":["byteorder","openssl","webpki"]}
```

For a crate that got passed byteorder, openssl and webpki dependencies but needed none of them.

### Q: Why not pass -Wunused-crate-dependencies?
A: See [ehuss's comment here](#57274 (comment))
   TLDR: it's cleaner. Rust's warning system wasn't built to be filtered or edited by cargo.
   Even a basic implementation of the feature would have to change the "n warnings emitted" line that rustc prints at the end.
   Cargo ideally wants to synthesize its own warnings anyways. For example, it would be hard for rustc to emit warnings like
   "dependency foo is only used by dev targets", suggesting to make it a dev-dependency instead.

### Q: Make rustc emit used or unused externs?
A: Emitting used externs has the advantage that it simplifies cargo's collection job.
   However, emitting unused externs creates less data to be communicated between rustc and cargo.
   Often you want to paste a cargo command obtained from `cargo build -vv` for doing something
   completely unrelated. The message is emitted always, even if no warning or error is emitted.
   At that point, even this tiny difference in "noise" matters. That's why I went with emitting unused externs.

### Q: One json msg per extern or a collective json msg?
A: Same as above, the data format should be concise. Having 30 lines for the 30 crates a crate uses would be disturbing to readers.
   Also it helps the cargo implementation to know that there aren't more unused deps coming.

### Q: Why use names of externs instead of e.g. paths?
A: Names are both sufficient as well as neccessary to uniquely identify a passed `--extern` arg.
   Names are sufficient because you *must* pass a name when passing an `--extern` arg.
   Passing a path is optional on the other hand so rustc might also figure out a crate's location from the file system.
   You can also put multiple paths for the same extern name, via e.g. `--extern hello=/usr/lib/hello.rmeta --extern hello=/usr/local/lib/hello.rmeta`,
   but rustc will only ever use one of those paths.
   Also, paths don't identify a dependency uniquely as it is possible to have multiple different extern names point to the same path.
   So paths are ill-suited for identification.

### Q: What about 2015 edition crates?
A: They are fully supported.
   Even on the 2015 edition, an explicit `--extern` flag is is required to enable `extern crate foo;` to work (outside of sysroot crates, which this flag doesn't warn about anyways).
   So the lint would still fire on 2015 edition crates if you haven't included a dependency specified in Cargo.toml using `extern crate foo;` or similar.
   The lint won't fire if your sole use in the crate is through a `extern crate foo;`   statement, but that's not its job.
   For detecting unused `extern crate foo` statements, there is the `unused_extern_crates` lint
   which can be enabled by `#![warn(unused_extern_crates)]` or similar.

cc ```@jsgf``` ```@ehuss``` ```@petrochenkov``` ```@estebank```
  • Loading branch information
Dylan-DPC authored Apr 4, 2021
2 parents b1ea261 + d018ef1 commit a1c3449
Show file tree
Hide file tree
Showing 9 changed files with 218 additions and 13 deletions.
3 changes: 3 additions & 0 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ pub trait Emitter {

fn emit_future_breakage_report(&mut self, _diags: Vec<(FutureBreakage, Diagnostic)>) {}

/// Emit list of unused externs
fn emit_unused_externs(&mut self, _lint_level: &str, _unused_externs: &[&str]) {}

/// Checks if should show explanations about "rustc --explain"
fn should_show_explain(&self) -> bool {
true
Expand Down
25 changes: 25 additions & 0 deletions compiler/rustc_errors/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,19 @@ impl Emitter for JsonEmitter {
}
}

fn emit_unused_externs(&mut self, lint_level: &str, unused_externs: &[&str]) {
let data = UnusedExterns { lint_level, unused_extern_names: unused_externs };
let result = if self.pretty {
writeln!(&mut self.dst, "{}", as_pretty_json(&data))
} else {
writeln!(&mut self.dst, "{}", as_json(&data))
}
.and_then(|_| self.dst.flush());
if let Err(e) = result {
panic!("failed to print unused externs: {:?}", e);
}
}

fn source_map(&self) -> Option<&Lrc<SourceMap>> {
Some(&self.sm)
}
Expand Down Expand Up @@ -322,6 +335,18 @@ struct FutureIncompatReport {
future_incompat_report: Vec<FutureBreakageItem>,
}

// NOTE: Keep this in sync with the equivalent structs in rustdoc's
// doctest component (as well as cargo).
// We could unify this struct the one in rustdoc but they have different
// ownership semantics, so doing so would create wasteful allocations.
#[derive(Encodable)]
struct UnusedExterns<'a, 'b, 'c> {
/// The severity level of the unused dependencies lint
lint_level: &'a str,
/// List of unused externs by their names.
unused_extern_names: &'b [&'c str],
}

impl Diagnostic {
fn from_errors_diagnostic(diag: &crate::Diagnostic, je: &JsonEmitter) -> Diagnostic {
let sugg = diag.suggestions.iter().map(|sugg| Diagnostic {
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,10 @@ impl Handler {
self.inner.borrow_mut().emitter.emit_future_breakage_report(diags)
}

pub fn emit_unused_externs(&self, lint_level: &str, unused_externs: &[&str]) {
self.inner.borrow_mut().emit_unused_externs(lint_level, unused_externs)
}

pub fn delay_as_bug(&self, diagnostic: Diagnostic) {
self.inner.borrow_mut().delay_as_bug(diagnostic)
}
Expand Down Expand Up @@ -839,6 +843,10 @@ impl HandlerInner {
self.emitter.emit_artifact_notification(path, artifact_type);
}

fn emit_unused_externs(&mut self, lint_level: &str, unused_externs: &[&str]) {
self.emitter.emit_unused_externs(lint_level, unused_externs);
}

fn treat_err_as_bug(&self) -> bool {
self.flags.treat_err_as_bug.map_or(false, |c| self.err_count() >= c.get())
}
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
use rustc_hir::definitions::Definitions;
use rustc_hir::Crate;
use rustc_lint::LintStore;
use rustc_metadata::creader::CStore;
use rustc_middle::arena::Arena;
use rustc_middle::dep_graph::DepGraph;
use rustc_middle::middle;
Expand Down Expand Up @@ -831,6 +832,12 @@ fn analysis(tcx: TyCtxt<'_>, cnum: CrateNum) -> Result<()> {
});

sess.time("looking_for_derive_registrar", || proc_macro_decls::find(tcx));

let cstore = tcx
.cstore_as_any()
.downcast_ref::<CStore>()
.expect("`tcx.cstore` is not a `CStore`");
cstore.report_unused_deps(tcx);
},
{
par_iter(&tcx.hir().krate().modules).for_each(|(&module, _)| {
Expand Down
37 changes: 34 additions & 3 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ pub struct CStore {
/// This map is used to verify we get no hash conflicts between
/// `StableCrateId` values.
stable_crate_ids: FxHashMap<StableCrateId, CrateNum>,

/// Unused externs of the crate
unused_externs: Vec<Symbol>,
}

pub struct CrateLoader<'a> {
Expand Down Expand Up @@ -190,6 +193,27 @@ impl CStore {
crate fn has_global_allocator(&self) -> bool {
self.has_global_allocator
}

pub fn report_unused_deps(&self, tcx: TyCtxt<'_>) {
// We put the check for the option before the lint_level_at_node call
// because the call mutates internal state and introducing it
// leads to some ui tests failing.
if !tcx.sess.opts.json_unused_externs {
return;
}
let level = tcx
.lint_level_at_node(lint::builtin::UNUSED_CRATE_DEPENDENCIES, rustc_hir::CRATE_HIR_ID)
.0;
if level != lint::Level::Allow {
let unused_externs =
self.unused_externs.iter().map(|ident| ident.to_ident_string()).collect::<Vec<_>>();
let unused_externs = unused_externs.iter().map(String::as_str).collect::<Vec<&str>>();
tcx.sess
.parse_sess
.span_diagnostic
.emit_unused_externs(level.as_str(), &unused_externs);
}
}
}

impl<'a> CrateLoader<'a> {
Expand Down Expand Up @@ -217,6 +241,7 @@ impl<'a> CrateLoader<'a> {
allocator_kind: None,
has_global_allocator: false,
stable_crate_ids,
unused_externs: Vec::new(),
},
used_extern_options: Default::default(),
}
Expand Down Expand Up @@ -904,11 +929,17 @@ impl<'a> CrateLoader<'a> {
// Don't worry about pathless `--extern foo` sysroot references
continue;
}
if self.used_extern_options.contains(&Symbol::intern(name)) {
let name_interned = Symbol::intern(name);
if self.used_extern_options.contains(&name_interned) {
continue;
}

// Got a real unused --extern
if self.sess.opts.json_unused_externs {
self.cstore.unused_externs.push(name_interned);
continue;
}

let diag = match self.sess.opts.extern_dep_specs.get(name) {
Some(loc) => BuiltinLintDiagnostics::ExternDepSpec(name.clone(), loc.into()),
None => {
Expand Down Expand Up @@ -941,9 +972,9 @@ impl<'a> CrateLoader<'a> {
self.inject_allocator_crate(krate);
self.inject_panic_runtime(krate);

info!("{:?}", CrateDump(&self.cstore));

self.report_unused_deps(krate);

info!("{:?}", CrateDump(&self.cstore));
}

pub fn process_extern_crate(
Expand Down
35 changes: 32 additions & 3 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,10 @@ impl Externs {
pub fn iter(&self) -> BTreeMapIter<'_, String, ExternEntry> {
self.0.iter()
}

pub fn len(&self) -> usize {
self.0.len()
}
}

impl ExternEntry {
Expand Down Expand Up @@ -698,6 +702,7 @@ impl Default for Options {
remap_path_prefix: Vec::new(),
edition: DEFAULT_EDITION,
json_artifact_notifications: false,
json_unused_externs: false,
pretty: None,
}
}
Expand Down Expand Up @@ -1196,15 +1201,23 @@ pub fn parse_color(matches: &getopts::Matches) -> ColorConfig {
}
}

/// Possible json config files
pub struct JsonConfig {
pub json_rendered: HumanReadableErrorType,
pub json_artifact_notifications: bool,
pub json_unused_externs: bool,
}

/// Parse the `--json` flag.
///
/// The first value returned is how to render JSON diagnostics, and the second
/// is whether or not artifact notifications are enabled.
pub fn parse_json(matches: &getopts::Matches) -> (HumanReadableErrorType, bool) {
pub fn parse_json(matches: &getopts::Matches) -> JsonConfig {
let mut json_rendered: fn(ColorConfig) -> HumanReadableErrorType =
HumanReadableErrorType::Default;
let mut json_color = ColorConfig::Never;
let mut json_artifact_notifications = false;
let mut json_unused_externs = false;
for option in matches.opt_strs("json") {
// For now conservatively forbid `--color` with `--json` since `--json`
// won't actually be emitting any colors and anything colorized is
Expand All @@ -1221,14 +1234,20 @@ pub fn parse_json(matches: &getopts::Matches) -> (HumanReadableErrorType, bool)
"diagnostic-short" => json_rendered = HumanReadableErrorType::Short,
"diagnostic-rendered-ansi" => json_color = ColorConfig::Always,
"artifacts" => json_artifact_notifications = true,
"unused-externs" => json_unused_externs = true,
s => early_error(
ErrorOutputType::default(),
&format!("unknown `--json` option `{}`", s),
),
}
}
}
(json_rendered(json_color), json_artifact_notifications)

JsonConfig {
json_rendered: json_rendered(json_color),
json_artifact_notifications,
json_unused_externs,
}
}

/// Parses the `--error-format` flag.
Expand Down Expand Up @@ -1806,7 +1825,8 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {

let edition = parse_crate_edition(matches);

let (json_rendered, json_artifact_notifications) = parse_json(matches);
let JsonConfig { json_rendered, json_artifact_notifications, json_unused_externs } =
parse_json(matches);

let error_format = parse_error_format(matches, color, json_rendered);

Expand All @@ -1819,6 +1839,14 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
let mut debugging_opts = build_debugging_options(matches, error_format);
check_debug_option_stability(&debugging_opts, error_format, json_rendered);

if !debugging_opts.unstable_options && json_unused_externs {
early_error(
error_format,
"the `-Z unstable-options` flag must also be passed to enable \
the flag `--json=unused-externs`",
);
}

let output_types = parse_output_types(&debugging_opts, matches, error_format);

let mut cg = build_codegen_options(matches, error_format);
Expand Down Expand Up @@ -1979,6 +2007,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
remap_path_prefix,
edition,
json_artifact_notifications,
json_unused_externs,
pretty,
}
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ top_level_options!(
// by the compiler.
json_artifact_notifications: bool [TRACKED],

// `true` if we're emitting a JSON blob containing the unused externs
json_unused_externs: bool [UNTRACKED],

pretty: Option<PpMode> [UNTRACKED],
}
);
Expand Down
6 changes: 5 additions & 1 deletion src/librustdoc/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ crate struct Options {
/// If this option is set to `true`, rustdoc will only run checks and not generate
/// documentation.
crate run_check: bool,
/// Whether doctests should emit unused externs
crate json_unused_externs: bool,
}

impl fmt::Debug for Options {
Expand Down Expand Up @@ -352,7 +354,8 @@ impl Options {
}

let color = config::parse_color(&matches);
let (json_rendered, _artifacts) = config::parse_json(&matches);
let config::JsonConfig { json_rendered, json_unused_externs, .. } =
config::parse_json(&matches);
let error_format = config::parse_error_format(&matches, color, json_rendered);

let codegen_options = build_codegen_options(matches, error_format);
Expand Down Expand Up @@ -687,6 +690,7 @@ impl Options {
},
crate_name,
output_format,
json_unused_externs,
})
}

Expand Down
Loading

0 comments on commit a1c3449

Please sign in to comment.