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

Improve unused extern crate and unused #[macro_use] warnings #39060

Merged
merged 3 commits into from
Jan 22, 2017

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Jan 14, 2017

This PR

  • adds unused_imports warnings for unused #[macro_use] extern crate macro imports,
  • improves unused_extern_crates warnings (avoids false negatives), and
  • removes unused #[macro_use] imports and unused extern crates.

r? @nrc

@jseyfried jseyfried force-pushed the improve_unused branch 2 times, most recently from 47c8ba5 to 4a90ee4 Compare January 14, 2017 10:51
extern crate libc;
extern crate std_unicode;
extern crate serialize as rustc_serialize; // used by deriving
Copy link
Contributor

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

Copy link
Member

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.

@alexcrichton
Copy link
Member

makes unused_extern_crates warn 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.

@jseyfried jseyfried force-pushed the improve_unused branch 2 times, most recently from 2ae2155 to 3186708 Compare January 15, 2017 01:50
@jseyfried
Copy link
Contributor Author

@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 jseyfried force-pushed the improve_unused branch 3 times, most recently from 4785cf4 to ec51ba3 Compare January 15, 2017 06:19
@alexcrichton
Copy link
Member

@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.)

@jseyfried jseyfried force-pushed the improve_unused branch 3 times, most recently from 67dc901 to 2b1d677 Compare January 16, 2017 03:57
@jseyfried
Copy link
Contributor Author

@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.

@KalitaAlexey
Copy link
Contributor

KalitaAlexey commented Jan 16, 2017

@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?

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 16, 2017

@alexcrichton
There's some existing practice regarding unused lints, for example

let m = MyMutex;

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?

@alexcrichton
Copy link
Member

@KalitaAlexey @petrochenkov

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.

@KalitaAlexey
Copy link
Contributor

@petrochenkov,
But what if a false positive is very rare?

@jseyfried
Copy link
Contributor Author

@petrochenkov There were 31 "true positives" and 1 "false positive" in rustc bootstrap.

@nrc
Copy link
Member

nrc commented Jan 18, 2017

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit 2b1d677 has been approved by nrc

bors added a commit that referenced this pull request Jan 21, 2017
@bors
Copy link
Contributor

bors commented Jan 21, 2017

☔ The latest upstream changes (presumably #39199) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried
Copy link
Contributor Author

@alexcrichton Thanks!
@bors r=nrc

@bors
Copy link
Contributor

bors commented Jan 21, 2017

📌 Commit 7753f19 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jan 21, 2017

⌛ Testing commit 7753f19 with merge f117e15...

@bors
Copy link
Contributor

bors commented Jan 21, 2017

💔 Test failed - status-travis

Copy link
Member

@alexcrichton alexcrichton left a comment

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;
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed -- thanks again :)

@jseyfried jseyfried force-pushed the improve_unused branch 2 times, most recently from 60240aa to 9e9262b Compare January 21, 2017 23:33
@bors
Copy link
Contributor

bors commented Jan 21, 2017

📌 Commit 9e9262b has been approved by nrc

@bors
Copy link
Contributor

bors commented Jan 22, 2017

⌛ Testing commit 9e9262b with merge 7a8c8af...

@bors
Copy link
Contributor

bors commented Jan 22, 2017

💔 Test failed - status-travis

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Jan 22, 2017

📌 Commit 191abc4 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jan 22, 2017

⌛ Testing commit 191abc4 with merge e5b0829...

bors added a commit that referenced this pull request Jan 22, 2017
Improve unused `extern crate` and unused `#[macro_use]` warnings

This PR
 - adds `unused_imports` warnings for unused `#[macro_use] extern crate` macro imports,
 - improves `unused_extern_crates` warnings (avoids false negatives), and
 - removes unused `#[macro_use]` imports and unused `extern crate`s.

r? @nrc
@bors
Copy link
Contributor

bors commented Jan 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing e5b0829 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants