search

rust-lang / compiler-team

A home for compiler team planning documents, meeting minutes, and other such things.
https://rust-lang.github.io/compiler-team/
Apache License 2.0
387 stars 69 forks source link

Stabilize `--json=unused-externs(-silent)` #674

Closed jsgf closed 9 months ago

jsgf commented 1 year ago

Proposal

This would stabilize two related options:

--json=unused-externs was introduced about 3 years ago in https://github.com/rust-lang/rust/pull/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 https://github.com/rust-lang/rust/issues/96068.

Mentors or Reviewers

@est31 @ehuss

Process

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

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 commented 1 year ago

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 commented 1 year ago

Implementation https://github.com/rust-lang/rust/pull/115717

davidtwco commented 1 year ago

@rustbot second

jsgf commented 1 year ago

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

jsgf commented 1 year ago

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

est31 commented 1 year ago

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 commented 1 year ago

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

compiler-errors commented 1 year ago

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 commented 1 year ago

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

Concerns:

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 commented 1 year ago

So what process should I have used instead?

RalfJung commented 1 year ago

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

saethlin commented 1 year ago

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

jsgf commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@rfcbot concern lint-interactions

est31 commented 1 year ago

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 https://github.com/rust-lang/cargo/issues/12115 . It would probably better fit into https://github.com/rust-lang/cargo/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@petrochenkov Are there still outstanding questions about lint interactions?

petrochenkov commented 1 year ago

@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

All the cases need to be documented.

jsgf commented 1 year ago

@petrochenkov I added documentation to https://github.com/rust-lang/rust/pull/115717.

jsgf commented 11 months ago

Ping?

jsgf commented 10 months ago

Hi all, is there anything outstanding here?

riking commented 10 months ago

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

rfcbot commented 10 months ago

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

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

rfcbot commented 9 months ago

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 commented 9 months ago

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