-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2018: fix lint on unused dependencies #57274
Comments
IMO it should be based on the |
So as far as I can tell, the set of unused crates is computed here: rust/src/librustc_typeck/check_unused.rs Line 74 in ec19464
tcx . Is this the code that needs to be updated?
|
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). |
In a project, I was just adding such (originally-)transitive dependencies to get a |
@dekellum Is there a reason you want to allow it in Cargo.toml rather than |
I agree both that |
We'd also have to make sure that if you have, say, two binaries in a crate, and |
@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). |
@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 rust-lang/cargo#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. |
@jonhoo It seems fine to me to silence those cases using What to do about this case practically is just sort of unfortunate, since A more complex but complete solution would be to have some way of telling |
I think that having a lint for unnecessary dependencies in |
See also rust-lang/rust-clippy#4341 |
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. |
What's the current state of this? From the look of it, the code at rust/src/librustc_typeck/check_unused.rs Line 68 in 339f574
extern crate -> use suggestions, but doesn't warn about unused crates?
Is that code still the right starting point for addressing this issue? |
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 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 (rust-lang/rfcs#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. |
I still think #69203 is useful even if it is allow-by-default. Resurrecting it is on my todo list, but I have no idea how soon I'll get to it. |
@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 Suggestions? |
This will print a diagnostic for crates which are mentioned as `--extern` arguments on the command line, but are never referenced from the source. This diagnostic is controlled by `-Wunused-crate-dependencies` or `#![warn(unused_crate_dependencies)]` and is "allow" by default. There are cases where certain crates need to be linked in but are not directly referenced - for example if they are providing symbols for C linkage. In this case the warning can be suppressed with `use needed_crate as _;`. Thanks to @petrochenkov for simplified core. Resolves issue rust-lang#57274
Warn about unused crate deps Implements rust-lang#57274 by adding -Wunused-crate-dependencies. This will warn about any `--extern` option on the command line which isn't referenced by the crate source either via `use` or `extern crate`. Crates which are added for some side effect but are otherwise unreferenced - such as for symbols they define - the warning can be suppressed with `use somecrate as _;`. If a crate has multiple aliases (eg using `foo = { package = "bar" }` in `Cargo.toml`), then it will warn about each unused alias. This does not consider crate added by some other means than `--extern`, including the standard library. It also doesn't consider any crate without `add_prelude` set (though I'm not sure about this). Unfortunately this probably [does not yet work well with Cargo](rust-lang#57274 (comment)) as it will over-specify crates, causing spurious warnings. As a result, this lint is "allow" by default and must be explicitly enabled either via `#![warn(unused_crate_deps)]` or with `-Wunused-crate-deps`.
Warn about unused crate deps Implements rust-lang#57274 by adding -Wunused-crate-dependencies. This will warn about any `--extern` option on the command line which isn't referenced by the crate source either via `use` or `extern crate`. Crates which are added for some side effect but are otherwise unreferenced - such as for symbols they define - the warning can be suppressed with `use somecrate as _;`. If a crate has multiple aliases (eg using `foo = { package = "bar" }` in `Cargo.toml`), then it will warn about each unused alias. This does not consider crate added by some other means than `--extern`, including the standard library. It also doesn't consider any crate without `add_prelude` set (though I'm not sure about this). Unfortunately this probably [does not yet work well with Cargo](rust-lang#57274 (comment)) as it will over-specify crates, causing spurious warnings. As a result, this lint is "allow" by default and must be explicitly enabled either via `#![warn(unused_crate_deps)]` or with `-Wunused-crate-deps`.
This will print a diagnostic for crates which are mentioned as `--extern` arguments on the command line, but are never referenced from the source. This diagnostic is controlled by `-Wunused-crate-dependencies` or `#![warn(unused_crate_dependencies)]` and is "allow" by default. There are cases where certain crates need to be linked in but are not directly referenced - for example if they are providing symbols for C linkage. In this case the warning can be suppressed with `use needed_crate as _;`. Thanks to @petrochenkov for simplified core. Resolves issue rust-lang#57274
Implement `--extern-location` This PR implements `--extern-location` as a followup to rust-lang#72342 as part of the implementation of rust-lang#57274. The goal of this PR is to allow rustc, in coordination with the build system, to present a useful diagnostic about how to remove an unnecessary dependency from a dependency specification file (eg Cargo.toml). EDIT: Updated to current PR state. The location is specified for each named crate - that is, for a given `--extern foo[=path]` there can also be `--extern-location foo=<location>`. It supports ~~three~~ two styles of location: ~~1. `--extern-location foo=file:<path>:<line>` - a file path and line specification 1. `--extern-location foo=span:<path>:<start>:<end>` - a span specified as a file and start and end byte offsets~~ 1. `--extern-location foo=raw:<anything>` - a raw string which is included in the output 1. `--extern-location foo=json:<anything>` - an arbitrary Json structure which is emitted via Json diagnostics in a `tool_metadata` field. ~~1 & 2 are turned into an internal `Span`, so long as the path exists and is readable, and the location is meaningful (within the file, etc). This is used as the `Span` for a fix suggestion which is reported like other fix suggestions.~~ `raw` and `json` are for the case where the location isn't best expressed as a file and location within that file. For example, it could be a rule name and the name of a dependency within that rule. `rustc` makes no attempt to parse the raw string, and simply includes it in the output diagnostic text. `json` is only included in json diagnostics. `raw` is emitted as text and also as a json string in `tool_metadata`. If no `--extern-location` option is specified then it will emit a default json structure consisting of `{"name": name, "path": path}` corresponding to the name and path in `--extern name=path`. This is a prototype/RFC to make some of the earlier conversations more concrete. It doesn't stand on its own - it's only useful if implemented by Cargo and other build systems. There's also a ton of implementation details which I'd appreciate a second eye on as well. ~~**NOTE** The first commit in this PR is rust-lang#72342 and should be ignored for the purposes of review. The first commit is a very simplistic implementation which is basically raw-only, presented as a MVP. The second implements the full thing, and subsequent commits are incremental fixes.~~ cc `@ehuss` `@est31` `@petrochenkov` `@estebank`
…crum Add an unstable --json=unused-externs flag to print unused externs This adds an unstable flag to print a list of the extern names not used by cargo. This PR will enable cargo to collect unused dependencies from all units and provide warnings. The companion PR to cargo is: rust-lang/cargo#8437 The goal is eventual stabilization of this flag in rustc as well as in cargo. Discussion of this feature is mostly contained inside these threads: rust-lang#57274 rust-lang#72342 rust-lang#72603 The feature builds upon the internal datastructures added by rust-lang#72342 Externs are uniquely identified by name and the information is sufficient for cargo. If the mode is enabled, rustc will print json messages like: ``` {"unused_extern_names":["byteorder","openssl","webpki"]} ``` For a crate that got passed byteorder, openssl and webpki dependencies but needed none of them. ### Q: Why not pass -Wunused-crate-dependencies? A: See [ehuss's comment here](rust-lang#57274 (comment)) TLDR: it's cleaner. Rust's warning system wasn't built to be filtered or edited by cargo. Even a basic implementation of the feature would have to change the "n warnings emitted" line that rustc prints at the end. Cargo ideally wants to synthesize its own warnings anyways. For example, it would be hard for rustc to emit warnings like "dependency foo is only used by dev targets", suggesting to make it a dev-dependency instead. ### Q: Make rustc emit used or unused externs? A: Emitting used externs has the advantage that it simplifies cargo's collection job. However, emitting unused externs creates less data to be communicated between rustc and cargo. Often you want to paste a cargo command obtained from `cargo build -vv` for doing something completely unrelated. The message is emitted always, even if no warning or error is emitted. At that point, even this tiny difference in "noise" matters. That's why I went with emitting unused externs. ### Q: One json msg per extern or a collective json msg? A: Same as above, the data format should be concise. Having 30 lines for the 30 crates a crate uses would be disturbing to readers. Also it helps the cargo implementation to know that there aren't more unused deps coming. ### Q: Why use names of externs instead of e.g. paths? A: Names are both sufficient as well as neccessary to uniquely identify a passed `--extern` arg. Names are sufficient because you *must* pass a name when passing an `--extern` arg. Passing a path is optional on the other hand so rustc might also figure out a crate's location from the file system. You can also put multiple paths for the same extern name, via e.g. `--extern hello=/usr/lib/hello.rmeta --extern hello=/usr/local/lib/hello.rmeta`, but rustc will only ever use one of those paths. Also, paths don't identify a dependency uniquely as it is possible to have multiple different extern names point to the same path. So paths are ill-suited for identification. ### Q: What about 2015 edition crates? A: They are fully supported. Even on the 2015 edition, an explicit `--extern` flag is is required to enable `extern crate foo;` to work (outside of sysroot crates, which this flag doesn't warn about anyways). So the lint would still fire on 2015 edition crates if you haven't included a dependency specified in Cargo.toml using `extern crate foo;` or similar. The lint won't fire if your sole use in the crate is through a `extern crate foo;` statement, but that's not its job. For detecting unused `extern crate foo` statements, there is the `unused_extern_crates` lint which can be enabled by `#![warn(unused_extern_crates)]` or similar. cc `@jsgf` `@ehuss` `@petrochenkov` `@estebank`
…crum Add an unstable --json=unused-externs flag to print unused externs This adds an unstable flag to print a list of the extern names not used by cargo. This PR will enable cargo to collect unused dependencies from all units and provide warnings. The companion PR to cargo is: rust-lang/cargo#8437 The goal is eventual stabilization of this flag in rustc as well as in cargo. Discussion of this feature is mostly contained inside these threads: rust-lang#57274 rust-lang#72342 rust-lang#72603 The feature builds upon the internal datastructures added by rust-lang#72342 Externs are uniquely identified by name and the information is sufficient for cargo. If the mode is enabled, rustc will print json messages like: ``` {"unused_extern_names":["byteorder","openssl","webpki"]} ``` For a crate that got passed byteorder, openssl and webpki dependencies but needed none of them. ### Q: Why not pass -Wunused-crate-dependencies? A: See [ehuss's comment here](rust-lang#57274 (comment)) TLDR: it's cleaner. Rust's warning system wasn't built to be filtered or edited by cargo. Even a basic implementation of the feature would have to change the "n warnings emitted" line that rustc prints at the end. Cargo ideally wants to synthesize its own warnings anyways. For example, it would be hard for rustc to emit warnings like "dependency foo is only used by dev targets", suggesting to make it a dev-dependency instead. ### Q: Make rustc emit used or unused externs? A: Emitting used externs has the advantage that it simplifies cargo's collection job. However, emitting unused externs creates less data to be communicated between rustc and cargo. Often you want to paste a cargo command obtained from `cargo build -vv` for doing something completely unrelated. The message is emitted always, even if no warning or error is emitted. At that point, even this tiny difference in "noise" matters. That's why I went with emitting unused externs. ### Q: One json msg per extern or a collective json msg? A: Same as above, the data format should be concise. Having 30 lines for the 30 crates a crate uses would be disturbing to readers. Also it helps the cargo implementation to know that there aren't more unused deps coming. ### Q: Why use names of externs instead of e.g. paths? A: Names are both sufficient as well as neccessary to uniquely identify a passed `--extern` arg. Names are sufficient because you *must* pass a name when passing an `--extern` arg. Passing a path is optional on the other hand so rustc might also figure out a crate's location from the file system. You can also put multiple paths for the same extern name, via e.g. `--extern hello=/usr/lib/hello.rmeta --extern hello=/usr/local/lib/hello.rmeta`, but rustc will only ever use one of those paths. Also, paths don't identify a dependency uniquely as it is possible to have multiple different extern names point to the same path. So paths are ill-suited for identification. ### Q: What about 2015 edition crates? A: They are fully supported. Even on the 2015 edition, an explicit `--extern` flag is is required to enable `extern crate foo;` to work (outside of sysroot crates, which this flag doesn't warn about anyways). So the lint would still fire on 2015 edition crates if you haven't included a dependency specified in Cargo.toml using `extern crate foo;` or similar. The lint won't fire if your sole use in the crate is through a `extern crate foo;` statement, but that's not its job. For detecting unused `extern crate foo` statements, there is the `unused_extern_crates` lint which can be enabled by `#![warn(unused_extern_crates)]` or similar. cc ``@jsgf`` ``@ehuss`` ``@petrochenkov`` ``@estebank``
…crum Add an unstable --json=unused-externs flag to print unused externs This adds an unstable flag to print a list of the extern names not used by cargo. This PR will enable cargo to collect unused dependencies from all units and provide warnings. The companion PR to cargo is: rust-lang/cargo#8437 The goal is eventual stabilization of this flag in rustc as well as in cargo. Discussion of this feature is mostly contained inside these threads: rust-lang#57274 rust-lang#72342 rust-lang#72603 The feature builds upon the internal datastructures added by rust-lang#72342 Externs are uniquely identified by name and the information is sufficient for cargo. If the mode is enabled, rustc will print json messages like: ``` {"unused_extern_names":["byteorder","openssl","webpki"]} ``` For a crate that got passed byteorder, openssl and webpki dependencies but needed none of them. ### Q: Why not pass -Wunused-crate-dependencies? A: See [ehuss's comment here](rust-lang#57274 (comment)) TLDR: it's cleaner. Rust's warning system wasn't built to be filtered or edited by cargo. Even a basic implementation of the feature would have to change the "n warnings emitted" line that rustc prints at the end. Cargo ideally wants to synthesize its own warnings anyways. For example, it would be hard for rustc to emit warnings like "dependency foo is only used by dev targets", suggesting to make it a dev-dependency instead. ### Q: Make rustc emit used or unused externs? A: Emitting used externs has the advantage that it simplifies cargo's collection job. However, emitting unused externs creates less data to be communicated between rustc and cargo. Often you want to paste a cargo command obtained from `cargo build -vv` for doing something completely unrelated. The message is emitted always, even if no warning or error is emitted. At that point, even this tiny difference in "noise" matters. That's why I went with emitting unused externs. ### Q: One json msg per extern or a collective json msg? A: Same as above, the data format should be concise. Having 30 lines for the 30 crates a crate uses would be disturbing to readers. Also it helps the cargo implementation to know that there aren't more unused deps coming. ### Q: Why use names of externs instead of e.g. paths? A: Names are both sufficient as well as neccessary to uniquely identify a passed `--extern` arg. Names are sufficient because you *must* pass a name when passing an `--extern` arg. Passing a path is optional on the other hand so rustc might also figure out a crate's location from the file system. You can also put multiple paths for the same extern name, via e.g. `--extern hello=/usr/lib/hello.rmeta --extern hello=/usr/local/lib/hello.rmeta`, but rustc will only ever use one of those paths. Also, paths don't identify a dependency uniquely as it is possible to have multiple different extern names point to the same path. So paths are ill-suited for identification. ### Q: What about 2015 edition crates? A: They are fully supported. Even on the 2015 edition, an explicit `--extern` flag is is required to enable `extern crate foo;` to work (outside of sysroot crates, which this flag doesn't warn about anyways). So the lint would still fire on 2015 edition crates if you haven't included a dependency specified in Cargo.toml using `extern crate foo;` or similar. The lint won't fire if your sole use in the crate is through a `extern crate foo;` statement, but that's not its job. For detecting unused `extern crate foo` statements, there is the `unused_extern_crates` lint which can be enabled by `#![warn(unused_extern_crates)]` or similar. cc ```@jsgf``` ```@ehuss``` ```@petrochenkov``` ```@estebank```
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? |
@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 rust-lang/cargo#8437 (comment) |
…itives `unused_crate_dependencies` is complaining about the `rand` dev-dependency, which is used by integ tests. This is a known problem with this lint: rust-lang/rust#57274
This lint doesn't work with multiple targets (in the case of prepare-worker, the bench-only dependencies were messing it up). See: - rust-lang/rust#95513 - rust-lang/rust#57274 (comment)
Quoting from #53128 (comment)
The text was updated successfully, but these errors were encountered: