Stabilize --json=unused-externs(-silent)

[jsgf]

Proposal

This would stabilize two related options:

• --json=unused-externs
• --json=unused-externs-silent

--json=unused-externs was introduced about 3 years ago in rust-lang/rust#73945.

When using json diagnostics, this flag causes rustc to emit extra diagnostics about unused crates. Specifically, it lists all the crates specified by an --extern cratename option which never had any symbols referenced when compiling the current crate.

This is distinct from a normal diagnostic message because there's no actual problem with the Rust code per-se - it simply means that the build system provided extra dependencies which were not used. In particular, there's no source file or line number a diagnostic can reference, because that's the whole point - we're reporting an absence.

Typically a user will want to remove those dependencies because they're either the result of cut-and-paste, no longer needed after code changes, and so on. The intended use of this message is that the build system itself will consume it, and turn it into a form which makes sense to the user with respect to the build system. For example, Cargo could consume it, make sure that none the crates in a given Cargo package require the unused crate(s), and then reference specific lines in Cargo.toml which should be removed or altered. Alternatively one could auto-fix such dependencies.

unused-externs and unused-externs-silent are identical except for how they interact with lint levels. Firstly neither does anything unless the unused-crate-dependencies lint is enabled. If it's set to deny or forbid level, then unused-externs will cause the build to fail as expected. unused-externs-silent suppresses this, and leaves it to the build system to present a build failure. This is because Cargo needs to determine whether a given dependency is unused over multiple crates, so its only an error if it is unused in all crates.

This feature has been used extensively with a non-Cargo build system (buck2) with great success - it is used as part of tooling to automatically remove unused dependencies across a very large codebase. The only change required in functionality since the original introduction was making unused-externs honor lint levels and introducting unused-externs-silent in order to resolve issue rust-lang/rust#96068.

Mentors or Reviewers

@est31 @ehuss

Process

The main points of the Major Change Process are as follows:

• File an issue describing the proposal.
• A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

[jsgf]

Implementation rust-lang/rust#115717

[davidtwco]

@rustbot second

[jsgf]

I guess another question is whether to stabilize --extern nounused:....

[jsgf]

What's outstanding here? Is there anything I need to do?

[est31]

Not sure about the way the final comment period is handled for MCPs, but it usually takes longer. Often the MCPs get accepted in a row. I think it's after special compiler team meetings?

[workingjubilee]

Process concern: MCPs have not historically been used for stabilizing things?

[compiler-errors]

Yeah, stabilization needs an FCP. In the interest of getting it kicked off sooner than later, let's do that now.

@rfcbot fcp merge

[rfcbot]

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Aaron1011
• @cjgillot
• @compiler-errors
• @davidtwco
• @eddyb
• @estebank
• @jackh726
• @lcnr
• @matthewjasper
• @michaelwoerister
• @nagisa
• @oli-obk
• @petrochenkov
• @pnkfelix
• @wesleywiser

Concerns:

• lint-interactions resolved by Stabilize --json=unused-externs(-silent) #674 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[jsgf]

So what process should I have used instead?

[RalfJung]

The FCP process has been started, you don't need to do anything. :)

[saethlin]

The MCP process (sort of) is documented on this page: https://forge.rust-lang.org/compiler/mcp.html

[jsgf]

@RalfJung My question was specifically about

Process concern: MCPs have not historically been used for stabilizing things?

Which made me think I should have used a process other than MCP. But I think the conclusion is that MCP w/ FCP is actually the right process?

[compiler-errors]

This FCP could've been done on a stabilization PR instead, but it doesn't really matter. Point is that public changes and stabilization of compiler flags always need an FCP.

[workingjubilee]

@RalfJung My question was specifically about

Process concern: MCPs have not historically been used for stabilizing things?

Which made me think I should have used a process other than MCP. But I think the conclusion is that MCP w/ FCP is actually the right process?

The stabilization process is documented by the rustc dev guide.

[petrochenkov]

unused-externs and unused-externs-silent are identical except for how they interact with lint levels. Firstly neither does anything unless the unused-crate-dependencies lint is enabled. If it's set to deny or forbid level, then unused-externs will cause the build to fail as expected. unused-externs-silent suppresses this, and leaves it to the build system to present a build failure.

I don't understand why --json=unused-externs has any interactions with the unused_crate_dependencies lint.
It's very unusual.

I would expect --json=unused-externs to always print unused externs, regardless of lint levels.
The unused_crate_dependencies lint doesn't work in isolation because rustc doesn't have enough information (that's why --json=unused-externs exists), so it may make sense to remove this lint entirely.

[petrochenkov]

@rfcbot concern lint-interactions

[est31]

I would expect --json=unused-externs to always print unused externs, regardless of lint levels.

The idea was that the user can allow the lint in the rust source file, and then it propagates to the higher level logic, implemented by the param not printing unused externs. This was introduced before lint level control abilities in Cargo, which we now have with rust-lang/cargo#12115 . It would probably better fit into rust-lang/cargo#12235 . IDK about buck though. @jsgf is there a way to make the lint be disableable on a buck build file level? That might make more sense than having allow statements in random pieces of code.

[est31]

so it may make sense to remove this lint entirely.

The lint was added for the purposes of the buck build system which lets users precisely specify the list of allowed build units, but I'm not sure if it's still used. @jsgf can tell more if it's still needed or useful for buck.

[jsgf]

The unused_crate_dependencies lint doesn't work in isolation because rustc doesn't have enough information

Rustc has enough information to accurately flag the lint condition (crate was unused), but it doesn't have enough information to fully report it to the user in an actionable way (remove this unused dependency).

The reason there's a lint is so that things like #[deny(unused_crate_dependencies)] or -Dunused_crate_dependencies make sense.

For the command-line option, I guess in principle we could have some completely separate command line options to control the level of this lint (up to and including making it the job of whatever is consuming these events), but it doesn't make much sense to me to have a completely different mechanism for this. I see it as rustc emits an event with a given level (policy) and the environment consuming it decides how to use that (enforcement). For Buck it's pretty direct because dependencies are fully defined for each individual crate, but Cargo would have to do something more complex across multiple crates.

But I don't know how in-source allow/deny directives would work without an associated lint. We do want to be able to control this at the source file level, but don't want to introduce a new bespoke mechanism.

So overall I do think it makes sense to see this primarily as a lint, which has a special reporting path because of its specific circumstances.

[workingjubilee]

It is worth considering that rustc is not neutral on policy, but is also its own enforcer. Yes, you can allow a lint if rustc would deny it, but such a lint may be issued with forbid.

[jsgf]

@workingjubilee Sure, but in this instance, while rustc has enough information to know there's a symptom (unused crate) it may not have enough information to know if it's a build-blocking problem (when used with Cargo where the results of multiple rustc invocations need to be analyzed). Therefore in that case the deny/forbid logic needs to be enforced by Cargo (hence the need for unused-externs-silent to suppress "failure" exits).

For the cases where the dependency information is known to be specifically for this crate (with Buck) then rustc can do everything (which is the behaviour of unused-externs).

[jsgf]

@petrochenkov Are there still outstanding questions about lint interactions?

[petrochenkov]

@rfcbot resolve lint-interactions

I'd like to see the behavior documented though, the issue description text above is clearly not enough.

Different combinations of --json=unused-externs(-silent) and -A/W/D unused_externs have multiple consequences

• whether the json is outputted
• whether the lint diagnostics are printed by rustc
• what exit code is reported by rustc
• something else?

All the cases need to be documented.

[jsgf]

@petrochenkov I added documentation to rust-lang/rust#115717.

[jsgf]

Ping?

[jsgf]

Hi all, is there anything outstanding here?

[riking]

Yes, you can ping @estebank @pnkfelix @wesleywiser to check their boxes. (Stabilization PR is rust-lang/rust#115717 .)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @compiler-errors, I wasn't able to add the final-comment-period label, please do so.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

psst @compiler-errors, I wasn't able to add the finished-final-comment-period label, please do so.

[apiraino]

@rustbot label -final-comment-period +major-change-accepted