-Dunused-crate-dependency --json unused-externs doesn't cause rustc to exit with error status

[jsgf]

Given the following code:
t.rs:

fn main() {}

unused.rs:

// empty

rustc --crate-type lib unused.rs
rustc --crate-type bin t.rs --extern unused=libunused.rlib -Dunused-crate-dependencies -Zunstable-options --error-format=json --json unused-externs

The current output is:

{"lint_level":"deny","unused_extern_names":["unused"]}

It reports a "deny" level event as expected, but it does not cause rustc to exit with a non-zero status.

I think it should since this is a fatal compiler diagnostic, I think it should exit with an error code. Of course a process monitoring the json stream could treat the "deny" level as a fatal error, but it seems odd to have a special case just for this.

[est31]

I've made it this way on purpose so that if you use it with cargo, you can put #![deny(unused_externs)] into one of the units and not have compilation of that unit fail. The extern might be used in another compilation unit, so it's up to cargo (or some other non json tool) to figure that out. Basically turning on the json mode silences the lint and turns it into a configuration variable.

[jsgf]

@est31 Hm, I think I see, but it seems like a strange way of doing it.

Right now, if you put -Dunused-crate-dependencies/#![deny(unused_crate_dependencies)] and don't use --json unused-externs, then you'll get a diagnostic + rustc exit 1 like any other lint. But as soon as you add --json unused-externs then you'll end up losing the error unless the build tool also has special-case logic to act on the lint-level of the event. (I assume Cargo has this?)

This seems surprising to me.

In effect, --json unused-externs has the effect of a (hypothetical) --cap-lints warn=unused-crate-dependencies. Maybe we should add that instead?

Also, perhaps related, I put up PR #96067 to implement --extern nounused: to suppress unused messages for specific externs. Does that help Cargo in this case?

[est31]

But as soon as you add --json unused-externs then you'll end up losing the error unless the build tool also has special-case logic to act on the lint-level of the event. (I assume Cargo has this?)

Yes, that's what my (non-upstreamed) implementation of the cargo side does. It moves the reporting entirely outside of rustc and into cargo. The dependencies are not specified in rust source code, but they are specified in toml files read by cargo. That's why the logic should be there and not in rustc.

This seems surprising to me.

In effect, --json unused-externs has the effect of a (hypothetical) --cap-lints warn=unused-crate-dependencies. Maybe we should add that instead?

If anything then the lint is allowed, because the warning is entirely suppressed. I agree that it is not an optimal solution, and I would like cleaner alternatives. I'm not sure a cap-lints flag as suggested by you is good because it would bloat the rustc command line even further. That's why I went with this "surprising" approach, to save on command line bloat. (Edited to add: also I'm not sure about how lint levels are handled internally by rustc, that is if the reported lint level in the json would be able to become deny if it's capped to warn, one needs to figure out if the "raw" uncapped lint level can be reported...).

The reason this entire thing is done is so that #![deny(unused_externs)] and other level specifications "work" in the sense that cargo can interpret them.

Also, perhaps related, I put up PR #96067 to implement --extern nounused: to suppress unused messages for specific externs. Does that help Cargo in this case?

Not much, as the number of non unused units is not known.

[jsgf]

If anything then the lint is allowed, because the warning is entirely suppressed

Yep.

I'm not sure a cap-lints flag as suggested by you is good because it would bloat the rustc command line even further. That's why I went with this "surprising" approach, to save on command line bloat.

That doesn't seem like a great tradeoff. While I agree generally about command-line bloat, we're taking about a single option with a fixed size - it's not like an option per dependency or something. (In general I wouldn't worry about command-line length at all if it weren't for Windows and its ridiculous limits, but we do have @file as an escape hatch.)

By contrast this behaviour is surprising because it has this non-orthogonal side effect. It makes the mechanism essentially Cargo-specific, as a workaround for Cargo's imprecise dependency specifications.

Edited to add: also I'm not sure about how lint levels are handled internally by rustc, that is if the reported lint level in the json would be able to become deny if it's capped to warn, one needs to figure out if the "raw" uncapped lint level can be reported...

I'm not quite sure how to parse this, but the lint level in the json does behave as expected - it is capped by --cap-lints and it's also elevated from warn->deny by #![deny(warnings)] (and presumably forbid, though I didn't specifically test that).

Actually I just noticed there's a discrepancy between the json and regular diagnostics. The json uses strings from

rust/compiler/rustc_lint_defs/src/lib.rs

Lines 151 to 176 in 1e6fe58

	pub enum Level {
	/// The `allow` level will not issue any message.
	Allow,
	/// The `expect` level will suppress the lint message but in turn produce a message
	/// if the lint wasn't issued in the expected scope. `Expect` should not be used as
	/// an initial level for a lint.
	///
	/// Note that this still means that the lint is enabled in this position and should
	/// be emitted, this will in turn fulfill the expectation and suppress the lint.
	///
	/// See RFC 2383.
	///
	/// The `LintExpectationId` is used to later link a lint emission to the actual
	/// expectation. It can be ignored in most cases.
	Expect(LintExpectationId),
	/// The `warn` level will produce a warning if the lint was violated, however the
	/// compiler will continue with its execution.
	Warn,
	ForceWarn,
	/// The `deny` level will produce an error and stop further execution after the lint
	/// pass is complete.
	Deny,
	/// `Forbid` is equivalent to the `deny` level but can't be overwritten like the previous
	/// levels.
	Forbid,
	}

and so are "allow", "warn", "deny", etc. But regular diagnostics use

rust/compiler/rustc_errors/src/lib.rs

Lines 1435 to 1451 in 1e6fe58

	pub enum Level {
	Bug,
	DelayedBug,
	Fatal,
	Error {
	/// If this error comes from a lint, don't abort compilation even when abort_if_errors() is called.
	lint: bool,
	},
	Warning,
	Note,
	/// A note that is only emitted once.
	OnceNote,
	Help,
	FailureNote,
	Allow,
	Expect(LintExpectationId),
	}

and so are "warning", "error", "note", "help", internal errors and so on.

The dependencies are not specified in rust source code, but they are specified in toml files read by cargo. That's why the logic should be there and not in rustc.

Sure, I agree that the presentation of the diagnostic belongs at the level which has some knowledge of how the dependencies are specified. But the logic of whether the error should be considered fatal seems to me like it should be handled like any other diagnostic, with that being entirely rustc's domain.

As I understand it, the problem is:

• Cargo doesn't support precise per-built-target (ie, invocations of rustc) dependencies (only coarser dev/non-dev)
• As a result, -Wunused-crate-dependencies can't be determined on a target-by-target basis
• Instead, Cargo effectively aggregates this so that a dependency is only considered unused if no target uses it
• To implement this, Cargo must make sure that rustc never treats unused-dependency diagnostics as fatal
  • This is currently implemented as a side-effect of using --json unused-externs

As a result anything using --json unused-externs also needs to implement logic for identifying fatal errors rather than relying on rustc's exit status as in all other cases - in particular it needs to hard code knowledge of which lint levels are considered fatal. That's particularly awkward because, as noted above, the json isn't even using the same level notation as other diagnostics.

One could imagine solving this in a generic way by having something like:

• --json unused-externs doesn't exit status (ie, what this PR implements)
• Cargo basically uses --cap-lints warn/allow (to prevent rustc from ever having a non-zero exit for lints)
• Cargo implements "denied lints" internally.
  • I think this would need extra "pre-capped level" info as part of diagnostics, so it would know what's intended to be denied/forbidden

Or the other alternative this, but with a per-lint --cap-lints capability which Cargo could use to control unused-crate-dependencies specifically.

Either way, this would allow Buck or other build systems with precise dependency information to use --json unused-externs in a more natural way.

[est31]

As I understand it, the problem is:

* Cargo doesn't support precise per-built-target (ie, invocations of rustc) dependencies (only coarser dev/non-dev)

* As a result, -Wunused-crate-dependencies can't be determined on a target-by-target basis

* Instead, Cargo effectively aggregates this so that a dependency is only considered unused if _no_ target uses it

* To implement this, Cargo must make sure that rustc never treats unused-dependency diagnostics as fatal
  
  * This is currently implemented as a side-effect of using `--json unused-externs`

Your understanding is correct. Note that it's not upstreamed atm, but that's my intent.

As a result anything using --json unused-externs also needs to implement logic for identifying fatal errors rather than relying on rustc's exit status as in all other cases - in particular it needs to hard code knowledge of which lint levels are considered fatal. That's particularly awkward because, as noted above, the json isn't even using the same level notation as other diagnostics.

It uses the same notation as lints do, which as you correctly point out is a different notation from the one that warnings do. The level has to be "combined" from the output of multiple compilation units. As for build tools knowing about the concept of lint levels, it already passes arguments to cap lints at warn. Furthermore, there are discussions to allow users to put lint level information into Cargo.toml.

Either way, this would allow Buck or other build systems with precise dependency information to use --json unused-externs in a more natural way.

Actually good that you bring this up. As we've talked about cargo's use cases of the flag, what are buck's use cases, if not reporting on unused crates on its own?

I have another alternative: a --json unused-externs-silent flag which silences the unused externs warning, basically taking the current behaviour and putting it into a separate name.

[jsgf]

I have another alternative: a --json unused-externs-silent flag which silences the unused externs warning, basically taking the current behaviour and putting it into a separate name.

Yeah, I'm fine with that. I can update my PR.

Edit: Done.