Rollup merge of #73945 - est31:unused_externs, r=Mark-Simulacrum

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: https://github.com/rust-lang/cargo/pull/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](https://github.com/rust-lang/rust/issues/57274#issuecomment-624839355)
   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```

compiler/rustc_errors/src/emitter.rs

@@ -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

compiler/rustc_errors/src/json.rs

@@ -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)
     }
@@ -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 {

compiler/rustc_errors/src/lib.rs

@@ -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)
     }
@@ -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())
     }

compiler/rustc_interface/src/passes.rs

@@ -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;
@@ -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, _)| {

compiler/rustc_metadata/src/creader.rs

@@ -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> {
@@ -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> {
@@ -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(),
         }
@@ -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 => {
@@ -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(

compiler/rustc_session/src/config.rs

@@ -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 {
@@ -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,
         }
     }
@@ -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
@@ -1221,6 +1234,7 @@ 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),
@@ -1228,7 +1242,12 @@ pub fn parse_json(matches: &getopts::Matches) -> (HumanReadableErrorType, bool)
             }
         }
     }
-    (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.
@@ -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);
 
@@ -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);
@@ -1979,6 +2007,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {
         remap_path_prefix,
         edition,
         json_artifact_notifications,
+        json_unused_externs,
         pretty,
     }
 }

compiler/rustc_session/src/options.rs

@@ -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],
     }
 );

src/librustdoc/config.rs

@@ -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 {
@@ -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);
@@ -687,6 +690,7 @@ impl Options {
             },
             crate_name,
             output_format,
+            json_unused_externs,
         })
     }
 

src/librustdoc/doctest.rs

@@ -1,5 +1,5 @@
 use rustc_ast as ast;
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::sync::Lrc;
 use rustc_errors::{ColorConfig, ErrorReported};
 use rustc_hir as hir;
@@ -23,6 +23,8 @@ use std::panic;
 use std::path::PathBuf;
 use std::process::{self, Command, Stdio};
 use std::str;
+use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::{Arc, Mutex};
 
 use crate::clean::Attributes;
 use crate::config::Options;
@@ -104,8 +106,10 @@ crate fn run(options: Options) -> Result<(), ErrorReported> {
 
     let mut test_args = options.test_args.clone();
     let display_warnings = options.display_warnings;
+    let externs = options.externs.clone();
+    let json_unused_externs = options.json_unused_externs;
 
-    let tests = interface::run_compiler(config, |compiler| {
+    let res = interface::run_compiler(config, |compiler| {
         compiler.enter(|queries| {
             let _lower_to_hir = queries.lower_to_hir()?;
 
@@ -151,12 +155,15 @@ crate fn run(options: Options) -> Result<(), ErrorReported> {
             });
             compiler.session().abort_if_errors();
 
-            let ret: Result<_, ErrorReported> = Ok(collector.tests);
+            let unused_extern_reports = collector.unused_extern_reports.clone();
+            let compiling_test_count = collector.compiling_test_count.load(Ordering::SeqCst);
+            let ret: Result<_, ErrorReported> =
+                Ok((collector.tests, unused_extern_reports, compiling_test_count));
             ret
         })
     });
-    let tests = match tests {
-        Ok(tests) => tests,
+    let (tests, unused_extern_reports, compiling_test_count) = match res {
+        Ok(res) => res,
         Err(ErrorReported) => return Err(ErrorReported),
     };
 
@@ -168,6 +175,44 @@ crate fn run(options: Options) -> Result<(), ErrorReported> {
         Some(testing::Options::new().display_output(display_warnings)),
     );
 
+    // Collect and warn about unused externs, but only if we've gotten
+    // reports for each doctest
+    if json_unused_externs {
+        let unused_extern_reports: Vec<_> =
+            std::mem::take(&mut unused_extern_reports.lock().unwrap());
+        if unused_extern_reports.len() == compiling_test_count {
+            let extern_names = externs.iter().map(|(name, _)| name).collect::<FxHashSet<&String>>();
+            let mut unused_extern_names = unused_extern_reports
+                .iter()
+                .map(|uexts| uexts.unused_extern_names.iter().collect::<FxHashSet<&String>>())
+                .fold(extern_names, |uextsa, uextsb| {
+                    uextsa.intersection(&uextsb).map(|v| *v).collect::<FxHashSet<&String>>()
+                })
+                .iter()
+                .map(|v| (*v).clone())
+                .collect::<Vec<String>>();
+            unused_extern_names.sort();
+            // Take the most severe lint level
+            let lint_level = unused_extern_reports
+                .iter()
+                .map(|uexts| uexts.lint_level.as_str())
+                .max_by_key(|v| match *v {
+                    "warn" => 1,
+                    "deny" => 2,
+                    "forbid" => 3,
+                    // The allow lint level is not expected,
+                    // as if allow is specified, no message
+                    // is to be emitted.
+                    v => unreachable!("Invalid lint level '{}'", v),
+                })
+                .unwrap_or("warn")
+                .to_string();
+            let uext = UnusedExterns { lint_level, unused_extern_names };
+            let unused_extern_json = serde_json::to_string(&uext).unwrap();
+            eprintln!("{}", unused_extern_json);
+        }
+    }
+
     Ok(())
 }
 
@@ -235,6 +280,18 @@ impl DirState {
     }
 }
 
+// NOTE: Keep this in sync with the equivalent structs in rustc
+// and cargo.
+// We could unify this struct the one in rustc but they have different
+// ownership semantics, so doing so would create wasteful allocations.
+#[derive(serde::Serialize, serde::Deserialize)]
+struct UnusedExterns {
+    /// Lint level of the unused_crate_dependencies lint
+    lint_level: String,
+    /// List of unused externs by their names.
+    unused_extern_names: Vec<String>,
+}
+
 fn run_test(
     test: &str,
     cratename: &str,
@@ -253,6 +310,7 @@ fn run_test(
     outdir: DirState,
     path: PathBuf,
     test_id: &str,
+    report_unused_externs: impl Fn(UnusedExterns),
 ) -> Result<(), TestFailure> {
     let (test, line_offset, supports_color) =
         make_test(test, Some(cratename), as_test_harness, opts, edition, Some(test_id));
@@ -278,6 +336,12 @@ fn run_test(
     if as_test_harness {
         compiler.arg("--test");
     }
+    if options.json_unused_externs && !compile_fail {
+        compiler.arg("--error-format=json");
+        compiler.arg("--json").arg("unused-externs");
+        compiler.arg("-Z").arg("unstable-options");
+        compiler.arg("-W").arg("unused_crate_dependencies");
+    }
     for lib_str in &options.lib_strs {
         compiler.arg("-L").arg(&lib_str);
     }
@@ -337,7 +401,26 @@ fn run_test(
             eprint!("{}", self.0);
         }
     }
-    let out = str::from_utf8(&output.stderr).unwrap();
+    let mut out_lines = str::from_utf8(&output.stderr)
+        .unwrap()
+        .lines()
+        .filter(|l| {
+            if let Ok(uext) = serde_json::from_str::<UnusedExterns>(l) {
+                report_unused_externs(uext);
+                false
+            } else {
+                true
+            }
+        })
+        .collect::<Vec<_>>();
+
+    // Add a \n to the end to properly terminate the last line,
+    // but only if there was output to be printed
+    if out_lines.len() > 0 {
+        out_lines.push("");
+    }
+
+    let out = out_lines.join("\n");
     let _bomb = Bomb(&out);
     match (output.status.success(), compile_fail) {
         (true, true) => {
@@ -721,6 +804,8 @@ crate struct Collector {
     source_map: Option<Lrc<SourceMap>>,
     filename: Option<PathBuf>,
     visited_tests: FxHashMap<(String, usize), usize>,
+    unused_extern_reports: Arc<Mutex<Vec<UnusedExterns>>>,
+    compiling_test_count: AtomicUsize,
 }
 
 impl Collector {
@@ -745,6 +830,8 @@ impl Collector {
             source_map,
             filename,
             visited_tests: FxHashMap::default(),
+            unused_extern_reports: Default::default(),
+            compiling_test_count: AtomicUsize::new(0),
         }
     }
 
@@ -791,6 +878,10 @@ impl Tester for Collector {
         let runtool_args = self.options.runtool_args.clone();
         let target = self.options.target.clone();
         let target_str = target.to_string();
+        let unused_externs = self.unused_extern_reports.clone();
+        if !config.compile_fail {
+            self.compiling_test_count.fetch_add(1, Ordering::SeqCst);
+        }
 
         // FIXME(#44940): if doctests ever support path remapping, then this filename
         // needs to be the result of `SourceMap::span_to_unmapped_path`.
@@ -846,6 +937,9 @@ impl Tester for Collector {
                 test_type: testing::TestType::DocTest,
             },
             testfn: testing::DynTestFn(box move || {
+                let report_unused_externs = |uext| {
+                    unused_externs.lock().unwrap().push(uext);
+                };
                 let res = run_test(
                     &test,
                     &cratename,
@@ -864,6 +958,7 @@ impl Tester for Collector {
                     outdir,
                     path,
                     &test_id,
+                    report_unused_externs,
                 );
 
                 if let Err(err) = res {