-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement unused crate lint for 2018 edition #69203
Implement unused crate lint for 2018 edition #69203
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
This ended up catching several unused crates within the compiler itself, which I fixed. Several crates needed suppressions due to rustc-specific quirks (forcing things to end up in the sysroot, or |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This seems significantly more complicated than it can be. All crate resolutions go through a single point ( The list is finalized after the |
I'm a little concerned about turning this on by default. Because Cargo cannot specify dependencies for individual targets, this might give noisy warnings that are annoying to resolve. For example, if you have multiple bin targets, and only one of them uses a dependency, you have to squelch it in all the other targets (lib, other bins, examples, integration tests, benchmarks). Same with dev-dependencies, where it is only used in one example or test, then all other targets will get warnings. For larger projects, this could be very frustrating. I'm not sure what the right solution is. Perhaps this could be "allow" by default and make it opt-in? Maybe when Cargo has per-target controls, it will be easier to silence the warnings and it could be switched to "warn" by default? |
I see - that's quite unfortunate.
I'm definitely biased, but I think this lint loses a lot of its value if it's opt-in. Removing unused dependencies improves build times for both your own crates as well as downstream crates - it would be really nice if we could warn about this by default.
Is there a tracking issue or RFC for that kind of per-target control? |
I agree this would be useful. I just want to be cautious to not make things worse. It might be useful to gather more data about how common legitimate unused crates are vs the false positives. rust-lang/cargo#1982 is the main issue tracking per-target dependencies. Someone was working on an RFC, but it looks like they didn't finish. |
We could potentially have Cargo disable the lint (split the new part into a lint called |
An alternative would be to add the ability to tell rust to ignore crates for the purposes of this lint. This could be applied to panic runtime crates, as well as dev-dependencies. |
This comment has been minimized.
This comment has been minimized.
6537794
to
c669c2b
Compare
This is blocked on reaching some kind of decision about how to enable this (on-by-default, off-by-default, or some hybrid). |
FWIW, it would be nice to land the part of this PR that removes unused crates from Otherwise, this could be implemented as a new allow-by-default lint |
Even if cargo passes all the dependencies correctly, it would still make sense to configure this lint separately for cases like playground, which adds 100 or so most popular crates to the command line regardless of whether they are used or not. |
@petrochenkov: Could the playground disable this via |
@Aaron1011 |
This comment has been minimized.
This comment has been minimized.
c669c2b
to
c38da53
Compare
This comment has been minimized.
This comment has been minimized.
This lint fires for any unused crates in the Rust 2018 extern prelude (e.g. passed in via `--extern`) For now, it is allow-by-default to prevent false positives in multi-target crate. See rust-lang#69203 (comment) for more details, and rust-lang/cargo#1982 for the tracking issue.
c38da53
to
3db14a8
Compare
This comment has been minimized.
This comment has been minimized.
This lint fires for any unused crates in the Rust 2018 extern prelude (e.g. passed in via `--extern`) For now, it is allow-by-default to prevent false positives in multi-target crate. See rust-lang#69203 (comment) for more details, and rust-lang/cargo#1982 for the tracking issue.
This fixes an ICE for items without a `DefId`
This is necessary for Rustdoc to work, since we never end up calling `Resolver.into_outputs`
894499b
to
bb0dac2
Compare
This comment has been minimized.
This comment has been minimized.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This is firing on a large number of tests. I think users are going to run into this problem as well - should we be suppressing these warnings for tests? |
Why does it happen? |
@@ -1067,6 +1067,10 @@ impl<'a> Builder<'a> { | |||
// some code doesn't go through this `rustc` wrapper. | |||
rustflags.arg("-Wrust_2018_idioms"); | |||
rustflags.arg("-Wunused_lifetimes"); | |||
// Remove this after the next cfg(bootstrap) bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Remove this after the next cfg(bootstrap) bump | |
// Remove this condition after the next cfg(bootstrap) bump |
declare_lint! { | ||
pub UNUSED_EXTERN_OPTIONS, | ||
Allow, | ||
"extern path crates that are never used" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extern path crates that are never used" | |
"`--extern` command line options that are never used" |
@@ -136,6 +136,9 @@ pub struct ResolverOutputs { | |||
/// Extern prelude entries. The value is `true` if the entry was introduced | |||
/// via `extern crate` item and not `--extern` option or compiler built-in. | |||
pub extern_prelude: FxHashMap<Name, bool>, | |||
/// All crates that ended up getting loaded. | |||
/// Used to determine which `--extern` entries are unused | |||
pub used_crates: Option<FxHashSet<Symbol>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting this list into ResolverOutputs
should be unnecessary, the lint can be reported before ResolverOutputs
are constructed.
The whole lint logic should be able to live entirely inside rustc_metadata
, I suggested the point to report it in #69203 (comment) (CrateLoader::postprocess
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrochenkov: Things are not actually done when postprocess
is called - while we won't resolve any more crates, we may register additional names for crates that are already loaded. I initially finalized things in postprocess
, but that ended up causing false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may register additional names for crates that are already loaded.
How?
(Ignoring rustdoc, which shouldn't report this lint.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we find a previous crate when loading a crate:
rust/src/librustc_metadata/creader.rs
Line 522 in 1572c43
result = LoadResult::Previous(cnum); |
then we may map a different name in the extern prelude to an already loaded crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CrateLoader::load
can only happen before postprocess
.
(Or rather it can happen later during AST lowering when resolving std
paths, but if lowering actually loads new crates, it will break more than just this lint.)
Could you give some examples of the false positives?
☔ The latest upstream changes (presumably #69076) made this pull request unmergeable. Please resolve the merge conflicts. |
From an end user perspective, the location that would make sense is the declaration in Cargo.toml. However, that would require cargo to tell the span to rustc and would bloat the rustc invocation even more than now. Note that there's an upper limit onto how much you can pass to a process. Maybe cargo could intercept the warning somehow and add a span to it? Note that Cargo currently doesn't yield any spans whatsoever. |
Closing this due to inactivity. |
Fixes #57274
This PR fixes the
unused_extern_crate
lint to work with the 2018 edition. We now separately track whether or not crates in the extern prelude ended up getting used, and lint those that do not.Notes:
extern crate
statement. That is,exern crate mycrate as renamed
will not trigger a lint formycrate
(but the existingextern crate
-based lints will still run). This avoids false-positives when#[macro_use]
is involved, and allows this lint to be suppressed using eitheruse mycrate as _
orextern mycrate
.panic_abort
andpanic_unwind
, to avoid lints inlibstd
. Ideally, we could just douse panic_abort as _
anduse panic_unwind as _
- however, this currently triggers an error due to two different panic runtimes being linked in.extern crate
-based lint (e.g. the already existing lint) performs several checks on the metadata of unused crates (e.g.is_compiler_builtins
) to see if it should skip linting an otherwise unused crate. Unfortuantely, the nature of the extern prelude means that if a crate ends up unused, we will never have loaded the metadata for it. Unless we want to deliberately load unused crates during the new lint (which seems like a horrible hack), we can only use command line flags to determine if we should skip linting a crate. This prevents us from detectingpanic_abort
andpanic_unwind
via crate metadata instead of crate names.