Improve unused extern crate and unused #[macro_use] warnings by jseyfried · Pull Request #39060 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation31 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR
- adds
unused_importswarnings for unused#[macro_use] extern cratemacro imports, - improves
unused_extern_crateswarnings (avoids false negatives), and - removes unused
#[macro_use]imports and unusedextern crates.
r? @nrc
| extern crate libc; |
|---|
| extern crate std_unicode; |
| extern crate serialize as rustc_serialize; // used by deriving |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And all these unused crates will still live undetected in Cargo.tomls and introduce build dependencies :(rustc needs to somehow communicate this information to cargo so it could warn too.
cc @alexcrichton
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true yeah that it needs to be propagated outwards, but this may not need rustc -> cargo support. The compiler could simply warn about unused --extern flags, and we could perhaps tailor it so the warning makes sense if you're just using Cargo.
In any case that seems like future work to me.
makes
unused_extern_crateswarn by default instead of allow by default.
I've been wary of doing this in the past because linking a crate can have side effects other than wanting to pull in items/macros. For example you might link an allocator or just link a crate which contains a native library. I'd personally still prefer that the lint were allow by default.
@alexcrichton
Do you think it would be feasible to track crates that link an allocator, contain a native library, or have other "side effects" from linking to avoid emitting unused warnings for these crates?
@jseyfried I haven't though too closely about how we might solve that problem, although it would likely involve taking a look at the crates that just that one crate pulls in (e.g. not shared dependencies from other crates) and then seeing what items it pulls in (e.g. any native libs, allocators, etc.)
@alexcrichton Ok, seems feasible but not easy -- I might try later in another PR.
@nrc I removed the commit makes unused_extern_crates warn-by-default instead of allow-by-default.
@alexcrichton, @jseyfried
Why can't we make unused_extern_crates to be warn-by-default and make a developer write #[allow(unused_extern_crates)] where it is applicable?
@alexcrichton
There's some existing practice regarding unused lints, for example
is reported by the unused_variables lint despite m being used for its destructor.
The user has to actively suppress it, e.g. by prefixing the variable name with _.
Using crates for linking only is relatively exotic, detecting unused usual crates seems by default seems more useful.
@jseyfried what was the ratio "true positive" / "false positive" (for rustc bootstrap) for unused_extern_crates before it was made allow-by-default?
I'm merely stating my opinion that I would like to not have this lint turned on by default. I personally feel very strongly that if a lint has a false positive it should be allow-by-default, but that's mostly just me.
@petrochenkov There were 31 "true positives" and 1 "false positive" in rustc bootstrap.
📌 Commit 2b1d677 has been approved by nrc
bors added a commit that referenced this pull request
Rollup of 28 pull requests
- Successful merges: #38603, #38761, #38842, #38847, #38955, #38966, #39062, #39068, #39077, #39111, #39112, #39114, #39118, #39120, #39132, #39135, #39138, #39142, #39143, #39146, #39157, #39166, #39167, #39168, #39179, #39184, #39195, #39197
- Failed merges: #39060, #39145
☔ The latest upstream changes (presumably #39199) made this pull request unmergeable. Please resolve the merge conflicts.
📌 Commit 7753f19 has been approved by nrc
⌛ Testing commit 7753f19 with merge f117e15...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw if you want to annotate crates in tree to deny this lint by default I'd be down for that
| extern crate getopts; |
| extern crate term; |
| extern crate libc; |
| extern crate panic_unwind; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, while unused, is intended to be linked for the side effects.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed -- thanks again :)
📌 Commit 9e9262b has been approved by nrc
⌛ Testing commit 9e9262b with merge 7a8c8af...
📌 Commit 191abc4 has been approved by nrc
bors added a commit that referenced this pull request
Improve unused extern crate and unused #[macro_use] warnings
This PR
- adds
unused_importswarnings for unused#[macro_use] extern cratemacro imports, - improves
unused_extern_crateswarnings (avoids false negatives), and - removes unused
#[macro_use]imports and unusedextern crates.
r? @nrc
This was referenced
Jan 22, 2017