idiom lints for removing extern crate
#50260
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
5 tasks
|
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 |
Manishearth
force-pushed
the
no-extern-crate
branch
from
0e54010
to
10bc64d
Compare
|
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 |
Manishearth
force-pushed
the
no-extern-crate
branch
from
10bc64d
to
c904d98
Compare
11 tasks
|
I don't see why the second lint is problematic to implement. Could you elaborate? |
|
The path could be part of a larger path like Also, we don't have a way of getting the span information of "this path up to this segment", we have to add and subtract spans for this (which is super fragile) |
|
To be clear, the lint itself is easy to do (it's implemented already), it's the suggestion that's tricky |
|
☔ The latest upstream changes (presumably #49789) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ahh, I see. Well, maybe a vague lint for now would be the best approach? |
|
@alexreg edition stuff must have suggestions. But we're actually ditching that part of the lint anyway, so this isn't a problem anymore. |
|
So @aturon and I sketched out a migration plan in this paper document here. I'm trying to figure out how this PR relates to that. |
nikomatsakis
reviewed
May 2, 2018
src/librustc_lint/builtin.rs
Outdated
| } | ||
| let mut err = cx.struct_span_lint(UNNECESSARY_EXTERN_CRATE, | ||
| it.span, "`extern crate` is unnecessary in the new edition"); | ||
| if it.vis == hir::Visibility::Public { |
There was a problem hiding this comment.
We should probably watch out for #[macro_use] here.
There are also a number of corner cases that I suppose we ought to document (eg., in a migration guide). It might be nice if we used a diagnostic code or maybe just pointed users at the document once it becomes available. (e.g., "what do I do if extern crate is used to ensure linking but not otherwise used?" etc)
src/librustc_lint/builtin.rs
Outdated
| err.span_suggestion(it.span, "use `pub use`", | ||
| format!("pub use {}", it.name)); | ||
| } | ||
| err.emit(); |
There was a problem hiding this comment.
one thing that certainly differs from the doc that @aturon and I prepared is that we hoped to suggest removing extern crate entries entirely when we could.
Manishearth
force-pushed
the
no-extern-crate
branch
from
c904d98
to
8d9f795
Compare
|
I'm going to defer making this lint bail on cases where the crate contains a module that shadows a crate since that's not so important for now. @alexreg woudl you be interested in working on this? I'll file a bug once I'm done. |
Manishearth
force-pushed
the
no-extern-crate
branch
from
8d9f795
to
687a1f0
Compare
Manishearth
force-pushed
the
no-extern-crate
branch
from
687a1f0
to
c3d4704
Compare
|
Deferring that minor bit of the lint to later |
|
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 |
|
@Manishearth Sure, I can have a stab at that. |
Manishearth
force-pushed
the
no-extern-crate
branch
from
c3d4704
to
dee8dc0
Compare
Manishearth
changed the title
extern crateextern crate
|
Actually I realized it may not be necessary (See comment discussion) |
|
@Manishearth Didn't quite understand your reasoning. I still see the possibility of a clash with a module name. |
|
Can you give a concrete example? |
|
Note that we already have a fallback mechanism for inline paths. If you do |
|
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 |
Manishearth
force-pushed
the
no-extern-crate
branch
from
dee8dc0
to
01791de
Compare
|
@Manishearth Hmm, maybe you're right. If there is a name clash after this lint is applied, then there was still a name clash before. |
nikomatsakis
approved these changes
May 7, 2018
|
@bors r+ |
|
📌 Commit 01791de has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
May 7, 2018
idiom lints for removing `extern crate` Based off of #49789 This contains two lints: - One that suggests replacing pub extern crates with pub use, and removing non-pub extern crates entirely - One that suggests rewriting `use modulename::...::cratename::foo` as `cratename::foo` The latter is a bit tricky to emit suggestions for; for one this involves splicing spans (never a good idea), and it also won't be able to correctly handle `use module::{cratename, foo}` and use-trees. I'm not sure how to proceed here. Currently it doesn't suggest anything at all. Perhaps we can go the other way and suggest removal of all extern crates _except_ those used through modules (stash node ids somewhere) and suggest replacing those with `<visibility> use`? r? @nikomatsakis fixes #48719
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
|
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 |
|
Either an ephemeral error or the "apple-alt" build server is down? |
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
May 8, 2018
idiom lints for removing `extern crate` Based off of #49789 This contains two lints: - One that suggests replacing pub extern crates with pub use, and removing non-pub extern crates entirely - One that suggests rewriting `use modulename::...::cratename::foo` as `cratename::foo` The latter is a bit tricky to emit suggestions for; for one this involves splicing spans (never a good idea), and it also won't be able to correctly handle `use module::{cratename, foo}` and use-trees. I'm not sure how to proceed here. Currently it doesn't suggest anything at all. Perhaps we can go the other way and suggest removal of all extern crates _except_ those used through modules (stash node ids somewhere) and suggest replacing those with `<visibility> use`? r? @nikomatsakis fixes #48719
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based off of #49789
This contains two lints:
use modulename::...::cratename::fooascratename::fooThe latter is a bit tricky to emit suggestions for; for one this involves splicing spans (never a good idea), and it also won't be able to correctly
handle
use module::{cratename, foo}and use-trees. I'm not sure how to proceed here. Currently it doesn't suggest anything at all.Perhaps we can go the other way and suggest removal of all extern crates except those used through modules (stash node ids somewhere) and suggest replacing those with
<visibility> use?r? @nikomatsakis
fixes #48719