Skip to content
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

Warn about unused crate deps #72342

Merged
merged 1 commit into from
May 27, 2020
Merged

Warn about unused crate deps #72342

merged 1 commit into from
May 27, 2020

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented May 19, 2020

Implements #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 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_dependencies)] or with -Wunused-crate-dependencies.

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2020
@rust-highfive

This comment has been minimized.

@jsgf jsgf force-pushed the warn-unused-deps branch 2 times, most recently from e3c3331 to 33c3c08 Compare May 19, 2020 07:22
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@jsgf
Copy link
Contributor Author

jsgf commented May 19, 2020

I'll split the deps cleanup into a separate PR.

@jsgf
Copy link
Contributor Author

jsgf commented May 19, 2020

This seems to work well in practice on a real codebase, except that the emitted diagnostics don't give any clue as to which crate they relate to when they're all jumbled up in a parallel builds.

@estebank @petrochenkov Since we can't (yet - I have some thoughts) report the actual source location of the dependency, can it/should it report the top-level module source or some other way to identify the crate? Something like foo/src/lib.rs (or even foo/src/lib.rs:1:1 at a pinch).

@ehuss
Copy link
Contributor

ehuss commented May 19, 2020

On option is to do something similar to what other global errors do, like main not found. Explicitly add the name of the crate to the message, perhaps with a suggestion on how to fix it. This is similar to what #69203 did.

warning: external crate `glob` is unused in crate `foo`
  |
  = help: consider removing `glob` from `Cargo.toml` or adding `extern crate glob as _;` to `src/lib.rs`

I think it would be nice to suggest editing Cargo.toml only if the "CARGO" environment variable is set. I've been thinking about doing this for some other errors that could be improved by suggesting edits to Cargo.toml. I'm not sure what the non-Cargo message should say, maybe "remove the --extern flag?", seems kinda weird.

@rust-highfive

This comment has been minimized.

@jsgf jsgf force-pushed the warn-unused-deps branch 2 times, most recently from 745f732 to 99cd1ae Compare May 19, 2020 23:17
@jsgf
Copy link
Contributor Author

jsgf commented May 19, 2020

@ehuss Yes, just the crate name is better than the full span of the top-level module. As far as I can see the the early buffered lint stuff doesn't support additional help/advice diagnostics, so I can skip that for now.

In a later PR I'd like to investigate an extension to allow Cargo/Buck/etc give rustc something it can put in its diagnostics for each --extern so that it can report something sensible in the context of the build environment.

@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor

As far as I can see the the early buffered lint stuff doesn't support additional help/advice diagnostics

It's not available in all contexts, but buffer_lint_with_diagnostic exists which takes an additional enum that can carry extra information for the lints that can then be customized in any way that DiagnosticBuilder supports. It is a bit of a hack, but it works for now and should work for you.

@jsgf
Copy link
Contributor Author

jsgf commented May 20, 2020

@estebank I think the diagnostics are OK now for a first pass. I'd like to revisit this with some way of directly referencing the source of the stray dependency which can use that mechanism.

@rust-highfive

This comment has been minimized.

@est31
Copy link
Member

est31 commented May 20, 2020

rustc only sees a limited part of the world, and any rustc-based lint will always be unsuited for rustc-with-Cargo use cases. Any good lint has to reside in Cargo, as outlined in the issue. Yes, one can make the --extern params for dev dependencies special and let the lint auto ignore them, but a good lint would also find unused dev dependencies, which only works if all examples/tests are compiled and usage info from all of them combined. Rustc only sees one example/doctest/etc at a time.

That being said, temporarily, until Cargo gets a native lint, it could pass -Wunused-crate-deps by default for build.rs, where the lint would work perfectly.

Another place where the lint works perfectly are rustc-with-buck use cases. This use case creates permanent legitimization of the feature inside rustc.

In the ideal end situation, the lint would be implemented twice: once in rustc, for tools like buck, and then in tools like Cargo that have a more complicated situation of extern crates.

@est31
Copy link
Member

est31 commented May 20, 2020

Actually disregard that, I think it's better if the lint will be enabled eventually, just some deps are suppressed. For cargo that would help find unused proper deps and unused build deps. The dev-deps can be checked by cargo's implementation, but it would only have to cover the dev-deps, nothing more.

@jsgf
Copy link
Contributor Author

jsgf commented May 20, 2020

@est31 Implementing the lint in Cargo seems pretty awkward, since Cargo can't directly tell what crates the source is accessing. Using the deps output seems pretty brittle. I definitely think that its a rustc-level task, which is helped by extra information from the build system.

The Cargo limitation is that it has a pretty coarse model of dependencies - broadly only at the package scope, build- and dev-dependencies notwithstanding. It would be nice if we can refine the dependency model to something more fine grained (eg bin targets get separate deps from libraries in the same package). The interaction with this lint is just another fallout of that.

@est31
Copy link
Member

est31 commented May 20, 2020

Using the deps output seems pretty brittle.

Note that the file format was made for compilers to tell build tools which files it uses. So we'd be using it for the purpose it was made for. There are a few gotchas that make it a bit brittle, indeed (especially in cargo-udeps), but if it ever turns out to be too brittle, one could think about creating json files or printing stuff to stdout (cargo gets json stuff from rustc already now).

It would be nice if we can refine the dependency model to something more fine grained (eg bin targets get separate deps from libraries in the same package).

While there is a trend for more refinement, and I do support introduction of separate binary dependencies, I doubt that the refinement will reach levels where each separate test in the tests/ directory or each separate example in the examples/ directory has its own set of dependencies. If almost all of your tests except one use a specific crate, what should you do? List it in all tests?

bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#72270 (add a lint against references to packed fields)
 - rust-lang#72294 (JS cleanup)
 - rust-lang#72342 (Warn about unused crate deps)
 - rust-lang#72401 (Use correct function for detecting `const fn` in unsafety checking)
 - rust-lang#72581 (Allow unlabeled breaks from desugared `?` in labeled blocks)
 - rust-lang#72592 (Update books)

Failed merges:

r? @ghost
@bors bors merged commit e38fdda into rust-lang:master May 27, 2020
matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request May 27, 2020
bors added a commit to rust-lang/rust-clippy that referenced this pull request May 27, 2020
rustup rust-lang/rust#72342, allow unused_crate_dependencies

changelog: none
@retep998
Copy link
Member

This broke winapi CI
https://github.com/retep998/winapi-rs/runs/717012925

retep998 added a commit to retep998/winapi-rs that referenced this pull request May 28, 2020
@petrochenkov
Copy link
Contributor

@retep998
Fixed in #72702.

SimonSapin added a commit to servo/servo that referenced this pull request May 28, 2020
This is based on compiling with `RUSTFLAGS="-W unused_crate_dependencies"` (CC rust-lang/rust#72342) in a recent Nightly (more so than used in the tree as of this writing, CC #26661 for work-arounds).

Only one crate is actually removed from the dependency graph, others are still dependended from other places.
SimonSapin added a commit to servo/servo that referenced this pull request May 28, 2020
This is based on compiling with `RUSTFLAGS="-W unused_crate_dependencies"` (CC rust-lang/rust#72342) in a recent Nightly (more so than used in the tree as of this writing, CC #26661 for work-arounds).

Only one crate is actually removed from the dependency graph, others are still dependended from other places.
bors-servo added a commit to servo/servo that referenced this pull request May 29, 2020
Remove some unused dependency declarations

This is based on compiling with `RUSTFLAGS="-W unused_crate_dependencies"` (CC rust-lang/rust#72342) in a recent Nightly (more so than used in the tree as of this writing, CC #26661 for work-arounds).

Only one crate is actually removed from the dependency graph, others are still dependended from other places.
bors-servo added a commit to servo/servo that referenced this pull request May 29, 2020
Remove some unused dependency declarations

This is based on compiling with `RUSTFLAGS="-W unused_crate_dependencies"` (CC rust-lang/rust#72342) in a recent Nightly (more so than used in the tree as of this writing, CC #26661 for work-arounds).

Only one crate is actually removed from the dependency graph, others are still dependended from other places.
skrzyp1 pushed a commit to skrzyp1/servo that referenced this pull request Jun 2, 2020
This is based on compiling with `RUSTFLAGS="-W unused_crate_dependencies"` (CC rust-lang/rust#72342) in a recent Nightly (more so than used in the tree as of this writing, CC servo#26661 for work-arounds).

Only one crate is actually removed from the dependency graph, others are still dependended from other places.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2021
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`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2021
…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`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2021
…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``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2021
…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```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants