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

jsgf · 2022-04-15T18:04:27Z

This PR:

fixes compiletest to understand unused extern notifications
adds tests for --json unused-externs
makes sure that deny-level unused externs notifications are treated as compile errors
- refactors the emit_unused_externs callstack to plumb through the level as an enum as a string, and adds Level::is_error

Update: adds --json unused-externs-silent with the original behaviour since Cargo needs it. Should address @est31's concerns.

Fixes: #96068

rust-highfive · 2022-04-15T18:04:30Z

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

est31 · 2022-04-15T18:35:40Z

I don't like this PR, see my comment here: #96068 (comment)

compiler-errors · 2022-04-15T19:39:22Z

Just took a look at this. These changes look good to me, but I don't have context around why the behavior this PR adjusts is the way it currently is (except for reading #96068 (comment)).

More concretely, is this blocked on #96067 landing and cargo being adjusted to use --extern nounused:, so we don't introduce new failures in cargo projects with deny(unused_crate_dependencies)? if so, not sure what the best coordination strategy of this PR would be w/ the corresponding cargo changes.

est31 · 2022-04-17T00:43:49Z

src/test/ui/unused-crate-deps/deny-cmdline-json-silent.rs

+// aux-crate:bar=bar.rs
+
+fn main() {}
+//~^ ERROR external crate `bar` unused in


This line can be removed now, right? stderr should not contain it.

est31 · 2022-04-17T00:58:10Z

compiler/rustc_errors/src/lib.rs

+        let mut inner = self.inner.borrow_mut();
+
+        if loud && lint_level.is_error() {
+            inner.bump_err_count();


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

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.

I see, makes sense 👍 .

est31

With the --json unused-externs-silent flag my general concerns are resolved. I still wonder about two of the code changes, see inline comments.

jsgf · 2022-04-22T16:54:06Z

ping?

compiler-errors · 2022-04-22T18:40:55Z

@jsgf, you never answered my question 😛

More concretely, is this blocked on #96067 landing and cargo being adjusted to use --extern nounused:, so we don't introduce new failures in cargo projects with deny(unused_crate_dependencies)?

est31 · 2022-04-22T22:52:05Z

@compiler-errors the cargo implementation was never merged. #96067. It won't break any users, but I would still prefer for the feature to be compatible with the design my PR proposed.

jsgf · 2022-04-24T05:35:38Z

@compiler-errors Yeah, I think nounused is really orthogonal to this.

compiler-errors · 2022-04-26T15:56:22Z

@bors r+

bors · 2022-04-26T15:56:23Z

📌 Commit bb91830f7a1545ec38a2736819a1931c4b259c98 has been approved by compiler-errors

bors · 2022-04-26T18:49:14Z

⌛ Testing commit bb91830f7a1545ec38a2736819a1931c4b259c98 with merge 299f80b2776088e087a626daf2c3e455d5044c1b...

bors · 2022-04-26T19:07:14Z

💔 Test failed - checks-actions

As generated by --json unused-externs.

There were none at all. These test for original functionality, but this also adds a test that `-Dunused-crate-dependencies` causes a compilation failure, which currently fails (rust-lang#96068). This is fixed in subsequent changes.

…error status Closes: rust-lang#96068

Since Cargo wants to do its own fatal error handling for unused dependencies, add the option `--json unused-externs-silent` which has the original behaviour of not indicating non-zero exit status for `deny`/`forbid`-level unused dependencies.

jsgf · 2022-04-27T17:04:57Z

Update with reblessed tests.

compiler-errors · 2022-04-27T17:05:42Z

@bors r+

bors · 2022-04-27T17:05:43Z

📌 Commit c6bafa7 has been approved by compiler-errors

…rrors Make sure `-Dunused-crate-dependencies --json unused-externs` makes rustc exit with error status This PR: - fixes compiletest to understand unused extern notifications - adds tests for `--json unused-externs` - makes sure that deny-level unused externs notifications are treated as compile errors - refactors the `emit_unused_externs` callstack to plumb through the level as an enum as a string, and adds `Level::is_error` Update: adds `--json unused-externs-silent` with the original behaviour since Cargo needs it. Should address `@est31's` concerns. Fixes: rust-lang#96068

bors · 2022-04-28T04:17:55Z

⌛ Testing commit c6bafa7 with merge 0e7915d...

bors · 2022-04-28T06:47:31Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0e7915d to master...

rust-timer · 2022-04-28T08:04:33Z

Finished benchmarking commit (0e7915d): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 15, 2022

rust-highfive assigned compiler-errors Apr 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2022

jsgf force-pushed the deny-unused-deps branch from 2d9ab89 to a5d3f41 Compare April 15, 2022 18:05

jsgf force-pushed the deny-unused-deps branch from abd0c56 to b570d5c Compare April 17, 2022 00:38

est31 reviewed Apr 17, 2022

View reviewed changes

jsgf force-pushed the deny-unused-deps branch 2 times, most recently from 03a2965 to bb91830 Compare April 17, 2022 01:24

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2022

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 26, 2022

jsgf and others added 3 commits April 27, 2022 09:37

compiletest: add support for unused extern notifications

0981b98

As generated by --json unused-externs.

Add tests for --json unused-externs

2373599

There were none at all. These test for original functionality, but this also adds a test that `-Dunused-crate-dependencies` causes a compilation failure, which currently fails (rust-lang#96068). This is fixed in subsequent changes.

Make --json unused-extern deny/forbid level messages cause exit with …

39f2f18

…error status Closes: rust-lang#96068

jsgf added 2 commits April 27, 2022 10:04

Plumb through rustc_lint_defs::Level as enum rather than string.

0529a13

jsgf force-pushed the deny-unused-deps branch from bb91830 to c6bafa7 Compare April 27, 2022 17:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2022

Dylan-DPC mentioned this pull request Apr 27, 2022

Rollup of 5 pull requests #96484

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2022

bors merged commit 0e7915d into rust-lang:master Apr 28, 2022

rustbot added this to the 1.62.0 milestone Apr 28, 2022

jsgf deleted the deny-unused-deps branch April 28, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jsgf commented Apr 15, 2022 •

edited

Loading

rust-highfive commented Apr 15, 2022

est31 commented Apr 15, 2022

compiler-errors commented Apr 15, 2022

est31 Apr 17, 2022

est31 Apr 17, 2022

jsgf Apr 17, 2022

est31 Apr 17, 2022

est31 left a comment

jsgf commented Apr 22, 2022

compiler-errors commented Apr 22, 2022

est31 commented Apr 22, 2022

jsgf commented Apr 24, 2022

compiler-errors commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

This comment has been minimized.

bors commented Apr 26, 2022

jsgf commented Apr 27, 2022

compiler-errors commented Apr 27, 2022

bors commented Apr 27, 2022

bors commented Apr 28, 2022

bors commented Apr 28, 2022

rust-timer commented Apr 28, 2022

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

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

Conversation

jsgf commented Apr 15, 2022 • edited Loading

rust-highfive commented Apr 15, 2022

est31 commented Apr 15, 2022

compiler-errors commented Apr 15, 2022

est31 Apr 17, 2022

Choose a reason for hiding this comment

est31 Apr 17, 2022

Choose a reason for hiding this comment

jsgf Apr 17, 2022

Choose a reason for hiding this comment

est31 Apr 17, 2022

Choose a reason for hiding this comment

est31 left a comment

Choose a reason for hiding this comment

jsgf commented Apr 22, 2022

compiler-errors commented Apr 22, 2022

est31 commented Apr 22, 2022

jsgf commented Apr 24, 2022

compiler-errors commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

This comment has been minimized.

bors commented Apr 26, 2022

jsgf commented Apr 27, 2022

compiler-errors commented Apr 27, 2022

bors commented Apr 27, 2022

bors commented Apr 28, 2022

bors commented Apr 28, 2022

rust-timer commented Apr 28, 2022

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

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

jsgf commented Apr 15, 2022 •

edited

Loading