Skip to content

Make sure -Dunused-crate-dependencies --json unused-externs makes rustc exit with error status #96085

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

Merged
merged 5 commits into from
Apr 28, 2022
Merged
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
7 changes: 6 additions & 1 deletion compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,12 @@ pub trait Emitter {
fn emit_future_breakage_report(&mut self, _diags: Vec<Diagnostic>) {}

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

/// Checks if should show explanations about "rustc --explain"
fn should_show_explain(&self) -> bool {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_errors/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ impl Emitter for JsonEmitter {
}
}

fn emit_unused_externs(&mut self, lint_level: &str, unused_externs: &[&str]) {
fn emit_unused_externs(&mut self, lint_level: rustc_lint_defs::Level, unused_externs: &[&str]) {
let lint_level = lint_level.as_str();
let data = UnusedExterns { lint_level, unused_extern_names: unused_externs };
let result = if self.pretty {
writeln!(&mut self.dst, "{}", as_pretty_json(&data))
Expand Down
17 changes: 14 additions & 3 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -969,8 +969,19 @@ 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 emit_unused_externs(
&self,
lint_level: rustc_lint_defs::Level,
loud: bool,
unused_externs: &[&str],
) {
let mut inner = self.inner.borrow_mut();

if loud && lint_level.is_error() {
inner.bump_err_count();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? bumping the error count should happen automatically if the lint is an error, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's needed because if we're going via the json event path, the normal diagnostics are not emitted, so error count isn't bumped. This is the core of the change to make get errors to be reported in the exit status.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, makes sense 👍 .

}

inner.emit_unused_externs(lint_level, unused_externs)
}

pub fn update_unstable_expectation_id(
Expand Down Expand Up @@ -1141,7 +1152,7 @@ impl HandlerInner {
self.emitter.emit_artifact_notification(path, artifact_type);
}

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

Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,13 @@ impl Level {
_ => None,
}
}

pub fn is_error(self) -> bool {
match self {
Level::Allow | Level::Expect(_) | Level::Warn | Level::ForceWarn => false,
Level::Deny | Level::Forbid => true,
}
}
}

/// Specification of a single lint.
Expand Down
15 changes: 9 additions & 6 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,12 @@ impl CStore {
}

pub fn report_unused_deps(&self, tcx: TyCtxt<'_>) {
let json_unused_externs = tcx.sess.opts.json_unused_externs;

// 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 {
if !json_unused_externs.is_enabled() {
return;
}
let level = tcx
Expand All @@ -208,10 +210,11 @@ impl CStore {
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);
tcx.sess.parse_sess.span_diagnostic.emit_unused_externs(
level,
json_unused_externs.is_loud(),
&unused_externs,
);
}
}
}
Expand Down Expand Up @@ -917,7 +920,7 @@ impl<'a> CrateLoader<'a> {
}

// Got a real unused --extern
if self.sess.opts.json_unused_externs {
if self.sess.opts.json_unused_externs.is_enabled() {
self.cstore.unused_externs.push(name_interned);
continue;
}
Expand Down
38 changes: 33 additions & 5 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ impl Default for Options {
real_rust_source_base_dir: None,
edition: DEFAULT_EDITION,
json_artifact_notifications: false,
json_unused_externs: false,
json_unused_externs: JsonUnusedExterns::No,
json_future_incompat: false,
pretty: None,
working_dir: RealFileName::LocalPath(std::env::current_dir().unwrap()),
Expand Down Expand Up @@ -1493,10 +1493,37 @@ pub fn parse_color(matches: &getopts::Matches) -> ColorConfig {
pub struct JsonConfig {
pub json_rendered: HumanReadableErrorType,
pub json_artifact_notifications: bool,
pub json_unused_externs: bool,
pub json_unused_externs: JsonUnusedExterns,
pub json_future_incompat: bool,
}

/// Report unused externs in event stream
#[derive(Copy, Clone)]
pub enum JsonUnusedExterns {
/// Do not
No,
/// Report, but do not exit with failure status for deny/forbid
Silent,
/// Report, and also exit with failure status for deny/forbid
Loud,
}

impl JsonUnusedExterns {
pub fn is_enabled(&self) -> bool {
match self {
JsonUnusedExterns::No => false,
JsonUnusedExterns::Loud | JsonUnusedExterns::Silent => true,
}
}

pub fn is_loud(&self) -> bool {
match self {
JsonUnusedExterns::No | JsonUnusedExterns::Silent => false,
JsonUnusedExterns::Loud => true,
}
}
}

/// Parse the `--json` flag.
///
/// The first value returned is how to render JSON diagnostics, and the second
Expand All @@ -1506,7 +1533,7 @@ pub fn parse_json(matches: &getopts::Matches) -> JsonConfig {
HumanReadableErrorType::Default;
let mut json_color = ColorConfig::Never;
let mut json_artifact_notifications = false;
let mut json_unused_externs = false;
let mut json_unused_externs = JsonUnusedExterns::No;
let mut json_future_incompat = false;
for option in matches.opt_strs("json") {
// For now conservatively forbid `--color` with `--json` since `--json`
Expand All @@ -1524,7 +1551,8 @@ pub fn parse_json(matches: &getopts::Matches) -> JsonConfig {
"diagnostic-short" => json_rendered = HumanReadableErrorType::Short,
"diagnostic-rendered-ansi" => json_color = ColorConfig::Always,
"artifacts" => json_artifact_notifications = true,
"unused-externs" => json_unused_externs = true,
"unused-externs" => json_unused_externs = JsonUnusedExterns::Loud,
"unused-externs-silent" => json_unused_externs = JsonUnusedExterns::Silent,
"future-incompat" => json_future_incompat = true,
s => early_error(
ErrorOutputType::default(),
Expand Down Expand Up @@ -2224,7 +2252,7 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options {

check_debug_option_stability(&debugging_opts, error_format, json_rendered);

if !debugging_opts.unstable_options && json_unused_externs {
if !debugging_opts.unstable_options && json_unused_externs.is_enabled() {
early_error(
error_format,
"the `-Z unstable-options` flag must also be passed to enable \
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ top_level_options!(
json_artifact_notifications: bool [TRACKED],

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

/// `true` if we're emitting a JSON job containing a future-incompat report for lints
json_future_incompat: bool [TRACKED],
Expand Down
6 changes: 4 additions & 2 deletions src/librustdoc/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use rustc_session::config::{
self, parse_crate_types_from_list, parse_externs, parse_target_triple, CrateType,
};
use rustc_session::config::{get_cmd_lint_options, nightly_options};
use rustc_session::config::{CodegenOptions, DebuggingOptions, ErrorOutputType, Externs};
use rustc_session::config::{
CodegenOptions, DebuggingOptions, ErrorOutputType, Externs, JsonUnusedExterns,
};
use rustc_session::getopts;
use rustc_session::lint::Level;
use rustc_session::search_paths::SearchPath;
Expand Down Expand Up @@ -147,7 +149,7 @@ crate struct Options {
/// documentation.
crate run_check: bool,
/// Whether doctests should emit unused externs
crate json_unused_externs: bool,
crate json_unused_externs: JsonUnusedExterns,
/// Whether to skip capturing stdout and stderr of tests.
crate nocapture: bool,

Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/doctest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ crate fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> {

// Collect and warn about unused externs, but only if we've gotten
// reports for each doctest
if json_unused_externs {
if json_unused_externs.is_enabled() {
let unused_extern_reports: Vec<_> =
std::mem::take(&mut unused_extern_reports.lock().unwrap());
if unused_extern_reports.len() == compiling_test_count {
Expand Down Expand Up @@ -337,7 +337,7 @@ fn run_test(
if lang_string.test_harness {
compiler.arg("--test");
}
if rustdoc_options.json_unused_externs && !lang_string.compile_fail {
if rustdoc_options.json_unused_externs.is_enabled() && !lang_string.compile_fail {
compiler.arg("--error-format=json");
compiler.arg("--json").arg("unused-externs");
compiler.arg("-Z").arg("unstable-options");
Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/unused-crate-deps/deny-attr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Check for unused crate dep, no path

// edition:2018
// aux-crate:bar=bar.rs

#![deny(unused_crate_dependencies)]
//~^ ERROR external crate `bar` unused in

fn main() {}
14 changes: 14 additions & 0 deletions src/test/ui/unused-crate-deps/deny-attr.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: external crate `bar` unused in `deny_attr`: remove the dependency or add `use bar as _;`
--> $DIR/deny-attr.rs:6:1
|
LL | #![deny(unused_crate_dependencies)]
| ^
|
note: the lint level is defined here
--> $DIR/deny-attr.rs:6:9
|
LL | #![deny(unused_crate_dependencies)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

8 changes: 8 additions & 0 deletions src/test/ui/unused-crate-deps/deny-cmdline-json-silent.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Check for unused crate dep, json event, deny but we're not reporting that in exit status

// edition:2018
// check-pass
// compile-flags: -Dunused-crate-dependencies -Zunstable-options --json unused-externs-silent --error-format=json
// aux-crate:bar=bar.rs

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"lint_level":"deny","unused_extern_names":["bar"]}
7 changes: 7 additions & 0 deletions src/test/ui/unused-crate-deps/deny-cmdline-json.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Check for unused crate dep, json event, deny, expect compile failure

// edition:2018
// compile-flags: -Dunused-crate-dependencies -Zunstable-options --json unused-externs --error-format=json
// aux-crate:bar=bar.rs

fn main() {}
1 change: 1 addition & 0 deletions src/test/ui/unused-crate-deps/deny-cmdline-json.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"lint_level":"deny","unused_extern_names":["bar"]}
8 changes: 8 additions & 0 deletions src/test/ui/unused-crate-deps/deny-cmdline.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Check for unused crate dep, deny, expect failure

// edition:2018
// compile-flags: -Dunused-crate-dependencies
// aux-crate:bar=bar.rs

fn main() {}
//~^ ERROR external crate `bar` unused in
10 changes: 10 additions & 0 deletions src/test/ui/unused-crate-deps/deny-cmdline.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: external crate `bar` unused in `deny_cmdline`: remove the dependency or add `use bar as _;`
--> $DIR/deny-cmdline.rs:7:1
|
LL | fn main() {}
| ^
|
= note: requested on the command line with `-D unused-crate-dependencies`

error: aborting due to previous error

8 changes: 8 additions & 0 deletions src/test/ui/unused-crate-deps/warn-cmdline-json.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Check for unused crate dep, warn, json event, expect pass

// edition:2018
// check-pass
// compile-flags: -Wunused-crate-dependencies -Zunstable-options --json unused-externs --error-format=json
// aux-crate:bar=bar.rs

fn main() {}
1 change: 1 addition & 0 deletions src/test/ui/unused-crate-deps/warn-cmdline-json.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"lint_level":"warn","unused_extern_names":["bar"]}
11 changes: 11 additions & 0 deletions src/tools/compiletest/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ struct ArtifactNotification {
artifact: PathBuf,
}

#[derive(Deserialize)]
struct UnusedExternNotification {
#[allow(dead_code)]
lint_level: String,
#[allow(dead_code)]
unused_extern_names: Vec<String>,
}

#[derive(Deserialize, Clone)]
struct DiagnosticSpan {
file_name: String,
Expand Down Expand Up @@ -113,6 +121,9 @@ pub fn extract_rendered(output: &str) -> String {
} else if serde_json::from_str::<ArtifactNotification>(line).is_ok() {
// Ignore the notification.
None
} else if serde_json::from_str::<UnusedExternNotification>(line).is_ok() {
// Ignore the notification.
None
} else {
print!(
"failed to decode compiler output as json: line: {}\noutput: {}",
Expand Down