[WIP] Add flag to warn about unused dependencies

[est31]

This commit adds an experimental flag to cargo, -Zwarn-unused-deps.

If the flag is enabled, cargo will warn about unused dependencies. rustc issue for the feature: rust-lang/rust#57274

The goal is to stabilize it eventually, enabling it by default.

The lint builds upon the --json=unused-externs flag of rustc
that is being proposed for this purpose in the companion PR rust-lang/rust#73945 .
This PR makes cargo pass the flag if -Zwarn-unused-deps is enabled to compilations of units it prints warnings for.

During compilation, cargo collects the unused dependencies
from all units, converts them into used dependencies,
continuously extending the record of used dependencies for
any type of DepKind: [dependencies], [dev-dependencies] and
[build-dependencies].

Once the compilation has ended, this record is used to
obtain those dependencies in a class that weren't used by any unit,
and warn about them.

The goal is to stabilize the flag and enable it by default
once the lint has become robust and the robustness is proven.

Then, cargo shall opportunistically warn about unused dependencies
when it has compiled (or loaded the unused externs from cache of)
all units that could possibly use dependencies of the kind.
Roughly, it's like this:

• cargo always compiles build.rs
• cargo check compiles all units that would use [dependencies]
• cargo test --no-run --all-targets compiles all units that can use
  [dev-dependencies]

TODO

The PR is still WIP but I've opened it to get initial feedback. TODO list:

• Making cargo figure out the allowed dependency kinds to warn about. Found a solution that should work
• Support #[cfg(test)] use extern_crate as _;
• Doctest support. There is no separation of build vs run steps for them (cc Cargo check does not check doctests #6424) so we'd have to block after they have finished. And we'll have to add json unused deps reporting to them in the first place. Currently we just skip them leading to false positives if a dev dependency is only used in doc tests.
• Finish the tests

cc @ehuss @jsgf

[rust-highfive]

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

[est31]

This is a table of some commands and what they actually compile/check:

	lib.rs	main.rs	lib.rs cfg test	tests/	benches/	doctests
check	yes	yes	no	no	no	no
check --all-targets	yes	yes	no	yes	yes	no
test --no-run	yes	yes	yes	yes	no	no
test	yes	yes	yes	yes	no	yes
test --all-targets	yes	yes	yes	yes	yes	no

So practically in almost every instance cargo can't warn about dev-dependencies without making false positives possible.

[jsgf]

If cargo can't support dependency warnings as a side effect of normal builds, does dependency checking need to be its own explicit operation? Eg cargo check --dependencies which does a check build on all possible targets and complain about completely unused targets (and make fix proposals to remove unused deps or move deps to dev/build deps).

[est31]

@jsgf in fact I think a command that compiles every part of a crate would be useful even outside of the unused dependencies scenario, to ensure that no code is breaking the build. Currently there is no such command. But I think that should better be discussed separately from this PR, as the PR implements checks for normal and build dependencies as well, and those checks are both more helpful (people depending on your crate don't compile your dev deps, only the normal and build deps), as well as more easily triggered (cargo build/check/test without any params all trigger the warnings). A rejection of such a mode, or any technical difficulties with implementing it shouldn't slow down the introduction of warnings for normal and build deps.

[alexcrichton]

This is a pretty slick way to detect unused dependencies, thanks for prototyping this!

One thing I figured I'd comment on is that warnings is one thing Cargo has historically not handled well. Generally with warnings you want the ability to do something like "turn them all to errors" or "allow them for this project or temporarily". This is handled really well in the compiler with #[allow], #[deny], -D, etc. Cargo has no similar system, however, and it's not entirely clear how we'd hook up such a system.

More concretely, I think that new warnings could fall short in a few situations:

• On CI folks probably want to run this but with a "turn warnings into errors" mode.
• For some crates they may wish to permanently opt out of "don't warn about that crate" for one reason or another.
• Sometimes during local development you may want to turn warnings off but then have them back on once you push to CI.

Since this is being developed in tandem with rustc I think it would be great to basically just figure out how to get rustc to emit these warnings one way or another. Either that or let rustc make all the decisions about whether Cargo should be allowing or denying warnings. Or... something along those lines, I don't know of a great solution myself, but I wanted to at least write this up since I think it will be important to get right.

[est31]

@alexcrichton

Generally with warnings you want the ability to do something like "turn them all to errors" or "allow them for this project or temporarily". This is handled really well in the compiler with #[allow], #[deny], -D, etc. Cargo has no similar system, however, and it's not entirely clear how we'd hook up such a system.

Yeah, before the feature is stabilized and enabled by default there should be a way to turn off the warnings at least. I'm not certain myself how it should look like so I thought it should be done after the PR but we can already discuss it now.

For some crates they may wish to permanently opt out of "don't warn about that crate" for one reason or another.

There's already support for this in the PR! It works just like let _v = 42; works. You prefix the name with a _:

_bar = { package = "bar", version = "0.1.0" }

But there is also a way to do it from Rust code via use cratename as _;.

I think it would be great to basically just figure out how to get rustc to emit these warnings one way or another.

In fact this has already been implemented for buck use cases in rust-lang/rust#72342 , but for cargo it is not suiting, as cargo doesn't have a 1:1 correspondence between dependencies and compilation units. In order to detect that an entry in the [dependencies] of Cargo.toml is really unused, you'll need output from multiple compilation units if you have multiple [[bin]] sections, or a lib and a bin section. So you'd get the unused dependency warning multiple times and it might still be wrong as it's used by one of the other units. Detecting unused dev-dependencies is even harder. There are proposals to make people list dependencies of every unit in Cargo.toml, and I support the idea especially to separate binary dependencies from library dependencies (e.g. crates.io users of cargo-the-library currently compile some dependencies needed for the binary even though it's not required), but to specify special dependencies for each of the dev units would be extremely unergonomic, so I doubt there will be lots of support to deprecate dev-dependencies entirely.

Either that or let rustc make all the decisions about whether Cargo should be allowing or denying warnings.

rustc would have to tell this info to cargo in some way so that cargo could unify the output from multiple units and tell the result. I could add info on whether to allow, deny, or warn to the json output communicated by rustc.

[bors]

☔ The latest upstream changes (presumably #8427) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

One idea we had for warning control in yesterday's Cargo meeting is that rustc could emit warnings/errors for unused crates (e.g. unused --extern flags) and then Cargo would just not emit them by default. Cargo already buffers all diagnostics so it could just decide to not actually emit some of the diagnostics.

This strategy would require Cargo to be able to identify lints precisely for unused crates (it's either already in the JSON structure or presumably we could add it). It would also require the addition of a "non fatal error" in rustc where if the lint is #[deny]'d then it wouldn't actually halt compilation or fail compilation, instead the final status code would be left up to Cargo.

Unifying this across many crates would look something like:

• If any unit that could use crate foo didn't emit a diagnostic, then all diagnostics relating to foo being unused (warnings and errors included) are squelched. This either means that a unit wasn't compiled or something had #[allow].
• If all units that could use a crate foo emitted a diagnostic, then the highest severity level is emitted. For example if everything warned then a warning is emitted (a single warning), and if anything emitted an error an error is emitted. We'd probably do some sorting or something like that to ensure the same diagnostic deterministically comes out.

That will require more changes in rustc for sure, but it seems quite appealing in terms of being able to control lint levels in Cargo.

[est31]

@alexcrichton if I have this code:

fn hello() {
}
mod foo {
}
mod bar {
}

Then why should a #[allow(unused)] on mod bar silence the unused warning for hello? Adding #[allow(unused_dependencies)] to lib.rs would be the same concept. The units are the modules, conceptually.

Also see the PR description of rust-lang/rust#73945 for further issues with that approach under the "Why not pass -Wunused-crate-dependencies?" section.

For controlling lint levels, I think that cargo should think about ways of allowing/denying lints for all units in a crate or even workspace, as that's usually what people want when they control lints in their crate lib.rs root (#5034). Control over the unused dependencies lint could build on that. Depending on the name, if it starts with cargo::, it could interpret the lint as a cargo lint, or if not, pass it to rustc via a flag. The system already exists to distinguish clippy from rustc lints. Clippy lints start with clippy::. Cargo already emits other warnings. Maybe it would be useful to convert some of them into "cargo lints".

[alexcrichton]

We explicitly are trying to avoid a system of managing lints in Cargo. It's not perfect to use rustc for this, but IMO it's way better than inventing something entirely new for Cargo.

[est31]

We explicitly are trying to avoid a system of managing lints in Cargo.

Who is "we" in that sentence? You? The entire cargo team? Most of the complexity of a "lints for cargo" system would already be covered by #5034 , so not really be a cost introduced by the unused dependencies lint.

It would also require the addition of a "non fatal error" in rustc where if the lint is #[deny]'d then it wouldn't actually halt compilation or fail compilation, instead the final status code would be left up to Cargo.

It would make the unused dependencies lint different from all other lints. Furthermore, cargo wouldn't just always re-emit that warning/error unchanged if it wants to support "this should rather be a dev-dependency" or (in the future) "this should rather be a bin-dependency" like messages (wich this PR already does). So the entire distinction between warning/error/etc would have to be supported by cargo.

Another issue would be that it would make doctests more complicated. Rustdoc has to collect the reports of all doc snippets and combine them. The more simple the format is, the easier this combination. It also offloads a lot of the ugly hacks to rustc so not sure whether the rustc team will support that (or the rustdoc team). THAT being said, if all three teams are aware of what the proposal entails and okay with that, I'm ok with it as well.

[alexcrichton]

The "we" is the Cargo team. If you'd like to block this on #5034 that's your choice, but AFAIK there are no plans to implement that.

What I mentioned was a suggestion, if you don't want to pursue it, that's ok. The Cargo team's requirements have been laid out here and we're offering one way to do it, but if you'd rather do something else that checks the boxes that should work too.

[est31]

Sounds fair! I'll think a bit about other approaches that satisfy the demands of the cargo team.

Note though that I won't have that much time to work on it in the next few months. The next thing I'll focus on will probably be json output for rustdoc errors. The mode will be required for implementation methods.

[bors]

☔ The latest upstream changes (presumably #8701) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[bors]

☔ The latest upstream changes (presumably #8777) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[camelid]

The companion PR has been merged, so I believe this is no longer blocked!

[est31]

@camelid I'll rebase the PR soon!

[bors]

☔ The latest upstream changes (presumably #9369) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

I'm gonna close this due to inactivity. Feel free to reopen or create a new PR when you've got time to work on this again. Thanks!

[est31]

The main blocker at this point is having a line with only "yes" inside in this table: #8437 (comment)