2018: fix lint on unused dependencies

[mark-i-m]

Quoting from https://github.com/rust-lang/rust/issues/53128#issuecomment-450008852

Previously warn(unused_extern_crates) was a way to detect unused dependencies. How can I:

1. declare dependencies in the root module
2. be warned when those dependencies are not used

As it is now, cargo builds all dependencies and does not warn if they are unused.

[cramertj]

IMO it should be based on the --extern args provided to rustc, and should not ignore "for-side-effects-only" imports now that we can do use some_crate as _;

[mark-i-m]

So as far as I can tell, the set of unused crates is computed here: https://github.com/rust-lang/rust/blob/ec194646fef1a467073ad74b8b68f6f202cfce97/src/librustc_typeck/check_unused.rs#L74 and then passed around through the tcx. Is this the code that needs to be updated?

[Nemo157]

This might have issues with adding dependencies-of-dependencies to force specific versions (I'm not sure how common that pattern is, and I guess they could be treated as "for-side-effects-only" imports as @cramertj mentions above).

[dekellum]

In a project, I was just adding such (originally-)transitive dependencies to get a -Z minimal-versions build working. At least before "unused_extern_crates" is changed to default to "warn", perhaps an allow=unused flag could be supported on the Cargo.toml dependency itself, like the #[allow(some_lint)] attribute in code?

[cramertj]

@dekellum Is there a reason you want to allow it in Cargo.toml rather than use <cratename> as _; as I suggested above?

[dekellum]

I agree both that use some_crate as _; should not trigger the unused_extern_crate lint (should not be treated as unused) and with the concern for the use-case raised by @Nemo157. Needing to add use some_crate as _; for each of these originally-transitive dependencies is more cumbersome, when just wanting to guide cargo's dependency resolution, where nothing really needs to be imported as far as rustc is concerned. Note also in my case, using the [patch] section of Cargo.toml didn't work for the such resolution adjustment.

[jonhoo]

We'd also have to make sure that if you have, say, two binaries in a crate, and Cargo.toml lists a dependency that is only used by one of them, compiling the other shouldn't generate a warning.

[cramertj]

@jonhoo IMO that's a bug in cargo that it doesn't allow specifying per-build-target dependencies (or maybe it does, in which case users should do that).

[jonhoo]

@cramertj yeah, I agree, the real solution would be to have per-target dependencies, but sadly that's not something we have yet. There's some discussion in https://github.com/rust-lang/cargo/issues/1982 on how we may add support for dependencies for binary targets, which would go a long way, but that has been open for a while. Until we get support for that though, we'd have to make sure that the lint doesn't erroneously give a warning in those cases.

[cramertj]

@jonhoo It seems fine to me to silence those cases using use X as _;. They are unused external crates, and it's wrong for them to be passed in via --extern but not used in the crate. It so happens that there isn't a "correct" way to fix it right now aside from silencing the warning with use X as _;, but rustc is correct to warn in that case IMO.

What to do about this case practically is just sort of unfortunate, since rustc doesn't have enough information at any point to determine that this is just waiting on a missing cargo feature. The only clear options to me are to trigger this warning and require that users manually silence it (which is undoubtedly annoying) or not display the warning at all until the requisite feature is added to cargo (which seems similarly annoying).

A more complex but complete solution would be to have some way of telling rustc about all the targets you intend to compile in the future that should be considered in determining whether a crate is "used" without actually fully compiling each of those targets. Actually making this work well is a bit outside the reach of my imagination at the moment, and it seems of little value once we get cargo build-target-specific dependencies working.

[jonhoo]

Sounds like the best thing would be to push the effort to allow per-target depenencies in cargo forward. Sadly that's a process that's been slow going since 2015. There has been some recent effort from @pwoolcoc to write up an RFC recently though (helped by @alexreg), so maybe there's hope!

[elichai]

I think that having a lint for unnecessary dependencies in Cargo.toml is a very important and useful thing

[est31]

See also https://github.com/rust-lang/rust-clippy/issues/4341

[est31]

FYI, while I'd like to have it implemented in upstream tools as well, I have released an independent tool that does such an unused crates lint.

[jsgf]

What's the current state of this?

From the look of it, the code at https://github.com/rust-lang/rust/blob/339f574809bf8e4166b8de3cdbe7df181d37af3d/src/librustc_typeck/check_unused.rs#L68 which by its name should be responsible for this is actually making extern crate -> use suggestions, but doesn't warn about unused crates?

Is that code still the right starting point for addressing this issue?

[ehuss]

I don't think the compiler will be able to implement this without false positives. @Aaron1011 attempted in #69203, which is sort of the straightforward approach to add a lint. However, when used with Cargo, Cargo unconditionally passes every dependency to every target. This means if you have a dev-dependency that is used in tests/foo.rs, but not in src/lib.rs, then you would get an unused warning in lib.rs.

It's hard to think of an approach that could work solely in rustc. If we want the lint to work with Cargo, then a higher level tool is needed. That is exactly what cargo-udeps is, though I haven't used it much.

It may be possible that Cargo will gain per-target dependencies (https://github.com/rust-lang/rfcs/pull/2887), but that might be a long ways off, and may not completely address the problem.

We could add the lint as allow-by-default, and let non-Cargo users, or adventurous souls to turn it on manually.

It may also be possible to build this into Cargo itself. It could use binary-dep-depinfo (#63012) to glean what is used. I haven't thought about it too much. I think cargo-udeps now has experimental support for that.

Those are the approaches I can think of offhand.

[petrochenkov]

I still think https://github.com/rust-lang/rust/pull/69203 is useful even if it is allow-by-default. At least it worked on the rustc repo pretty well.

Resurrecting it is on my todo list, but I have no idea how soon I'll get to it. It would be great if someone else could do that sooner.

[jsgf]

@ehuss Thanks for the update. I'm primarily concerned with Buck builds where every target should have precisely defined dependencies, so the Cargo limitation doesn't apply.

Thanks for the pointer to #69203; I'll give it a closer look.

[jsgf]

@petrochenkov I have a draft implementation of this in https://github.com/jsgf/rust/tree/warn-unused-deps, following your advice of doing most of the logic in rustc_metadata::postprocess. The logic looks sound, but the problem I'm running into is how to report the diagnostics and handle the lint allow/deny flags and attributes without a tcx or a real Span corresponding to the lint.

Suggestions?

[est31]

As the author of cargo-udeps I fully support the mainlining of the lint. My goal is reducing the bloat on crates.io and the more native the lint is, the more people will run it and the less bloat we'll have.

I agree with @ehuss that the check needs involvement of a non rustc tool in some fashion. They rightfully point out that dev dependencies can't be checked for unused-ness in a simple cargo build, cargo check or cargo run. From what I can tell, there is no difference from rust's perspective. Cargo on the other hand has the info to filter out dev dependencies and only focus on proper dependencies.

Furthermore, when you run cargo test (with or without the --no-run param), cargo will compile everything that uses dev dependencies, so if you teach cargo to combine all the info, it can actually warn about dev dependencies as well.

So ideally, cargo check would warn about unused proper dependencies, build dependencies, and cargo test would warn about unused dev dependencies on top.

It may also be possible to build this into Cargo itself. It could use binary-dep-depinfo (#63012) to glean what is used. I haven't thought about it too much. I think cargo-udeps now has experimental support for that.

binary-dep-depinfo is weakly typed, as in everything is just a file. It's okay but my code in cargo-udeps is not very robust. I guess I can improve it with a few tricks, but now, if you include!() a file that has a special name, cargo-udeps would count it as used dependency. Cargo is privileged as it has access to the hashes and the path layout and knows where the rlibs are. See https://github.com/rust-lang/cargo/issues/8145 which would give a bit of that access to crates.io users like cargo-udeps.

[petrochenkov]

@jsgf If there's no tcx, then buffer_lint can be used, see how it's used for other early lints. The span has to be DUMMY_SP, we don't have any way to point to Cargo.toml currently.

[jsgf]

@petrochenkov Thanks for the pointers. I generically mention Cargo.toml in the help, but for my specific use-case we're not using cargo so a more specific reference wouldn't be accurate anyway. That said, I was wondering if we could extend the --extern syntax to something like:

Allow rustc to report origin of dependency (very ugly concrete syntax):

--extern depref;Cargo.toml;233:foobar=targets/debug/libfoobar.rlib

Allow cargo to tell rustc to ignore potentially redundant dependencies (test targets, etc):

--extern nounused:foobar=targets/debug/libfoobar.rlib

(cc @ehuss)

[petrochenkov]

@jsgf

I was wondering if we could extend the --extern syntax to something like:

Yes, of course. Someone needs to do a proof of concept showing how the diagnostics look in this case and making sure that we don't overflow command line length or something.

(I have this task in my TODO queue, but it has little chance to ever get to the top of it.)

[est31]

Allow rustc to report origin of dependency (very ugly concrete syntax):

--extern depref;Cargo.toml;233:foobar=targets/debug/libfoobar.rlib

Some points:

1. ; isn't great because it has to be escaped by e.g. bash. : is better.
2. Cargo uses absolute paths, and thus the path to Cargo.toml can be very long. I'd prefer a separate param that assigns IDs like --extern-decl-site 0=/path/to/Cargo.toml, and then the --extern parm would reference the ID.
3. Spans have beginning and an end, so there should be two byte offset numbers, not one.

In total, I'd propose:

--extern-decl-site 0=/path/to/Cargo.toml --extern 0:228:233:foo=targets/debug/libfoobar.rlib --extern 0:240:243:foo=targets/debug/libbar.rlib

Allow cargo to tell rustc to ignore potentially redundant dependencies (test targets, etc):

--extern nounused:foobar=targets/debug/libfoobar.rlib

There is no need to require nounused for suppression: one could only turn on the lint if a span is passed. WDYT?

[jsgf]

@est31

1. I avoided : because of the existing syntax of this option - it already supports --extern priv:name=path and noprelude:name=path, and they can be combined with , (ie --extern priv,noprelude:name=path). I also considered something like --extern deploc$path#byte-byte:name=path though that's getting pretty ugly. I also considered a separate option, but not thrilled about it. I'm not too concerned about shell quoting because that only applies when invoking rustc via a shell, which Cargo, Buck or any other build system should not do (and even if it did, it would have to already know how to properly quote arguments).
2. (& 3) I don't think this can really be treated as a Span in rustc's terms, since it isn't referring to a file that rustc has any direct interaction with. In fact it may not be a file path at all - for Buck it might be better to use a target name, which can be precisely mapped to a rule within a target definition file - that is, it may be something like //rust/common/mylib:mylib. However for Cargo I can see that it would probably be best for it to refer to a specific line in Cargo.toml. As far as the full path vs relative goes, I think we'd like to make sure that whatever this reference is, rustc would emit it verbatim in its own field in the json diagnostics, which would give Cargo a chance to manipulate it how it likes before presentation (assuming Cargo moves to a model of always consuming rustc's diagnostics as json).

---extern-decl-site 0=/path/to/Cargo.toml --extern 0:228:233:foo=targets/debug/libfoobar.rlib --extern 0:240:243:foo=targets/debug/libbar.rlib

As mentioned above, this conflicts with the existing use of : in --extern, so that doesn't fly - but something like --extern siteref$0#123-456:name=path might (again with ugly syntax). But that still assumes that a span-line reference is the best way to refer to the dependency.

I was thinking something more along the names of:

--extern-loc name=<location> --extern [options:]name[=path]

where <location> is something that makes sense in the context of the build system (eg Cargo.toml:line, or buck target). This is more verbose than your proposal in that there's approx 1:1 --extern-loc to --extern options, but it does take advantage of being able to use the name as the key to correlate them.

There is no need to require nounused for suppression: one could only turn on the lint if a span is passed. WDYT?

I understand that to mean that to get any output from this lint you'd have to have the --extern-decl-site option specified, which in turn implies that you need to modify the build system to invoke rustc with this option to get anything at all. That doesn't seem like a good idea to me - at the moment it's perfectly functional with no build system changes, even if the diagnostics could be improved.

The real reason we don't need nounused is because if Cargo knew it wasn't used, it could just not add the --extern option (and if it is really needed for the few special cases, that's probably best handled on the Rust side with a use foo as _; annotation).

[est31]

I avoided : because of the existing syntax of this option - it already supports --extern priv:name=path and noprelude:name=path, and they can be combined with , (ie --extern priv,noprelude:name=path).

Good point about priv,noprelude. I'm still no fan of ;. Your --extern-loc suggestion sounds interesting. I'm sure it can be amended somehow to allow having to only specify the file once. Of course @file exists, but ideally it shouldn't be used and people should be able to copy-paste the command printed from cargo build -vvv to debug something.

span-line reference

Note that I didn't mean line references but byte offsets. The difference is that byte offsets are more precise: from them, both line and column numbers can be reconstructed. You could theoretically have a Cargo.toml like this:

dependencies = { serde = "1.0", other-dep = "2.0", third = "0.1" }

[package]
# ...

And end offsets are important for editors so that e.g. editors, but also "just" rustc's span printing code can underline the entire declaration of the crate. That's why I suggested byte start + byte end spans.

rustc would emit it verbatim in its own field in the json diagnostics, which would give Cargo a chance to manipulate it how it likes before presentation (assuming Cargo moves to a model of always consuming rustc's diagnostics as json).

IIRC pipelined compilation is on by default so for 99% of users, cargo already uses json for the diagnostics.

For the verbatimg emission of the file. Treating these warnings specially in json would be quite equivalent to rustc emitting a list of unused extern dependencies which is very similar to rustc emitting a list of used extern dependencies, which was my proposal all along :).

If we treat the warnings specially in json, we can do away with the special arguments entirely. We can just let Cargo/Buck custom-format the error messages. Or perform analysis over multiple compilation units like what Cargo has to do.

The real reason we don't need nounused is because if Cargo knew it wasn't used, it could just not add the --extern option

Cargo doesn't know which dep is used or unused before it has invoked rustc, unless you are proposing that Cargo parses the source code itself :p . Functionality like nounused will be needed should the lint be ever enabled by default so that cargo can pass dev-dependencies as nounused (unless dev-dependencies are completely deprecated in a future edition, which I doubt).

[Nemo157]

It doesn't seem like nounused is necessary, this lint (on the Cargo side) would need to support collecting usage across multiple targets already (since dependencies is shared between the lib and all the bins), in which case it can give correct output for dev-dependencies via tracking the usage across all targets they are used in. (That also makes how to actually surface this information to the user from Cargo potentially very complicated, but hopefully common cases at least will give simple output).

[deepink-mas]

While this is still open is there a quick check that can be performed that will confirm whether a dependency is still necessary or not?

For example if I comment or remove a dep from Cargo.toml and run "cargo check --all-targets", will I always get an error if the dep is still needed, and conversely does the absence of any errors indicate that the dep is unused?

[est31]

@deepink-mas The lint is in rustc mostly to support non-cargo build systems. For cargo there needs to be cargo level comparison between multiple units, which isn't implemented in cargo right now.

Furthermore, there is no single cargo command that builds all units, see https://github.com/rust-lang/cargo/pull/8437#issuecomment-653842297