Repository navigation
Lint for removing extern crate has false positive when crate is used #50672
Description
The following code
#![feature(rust_2018_preview)]
#![deny(unnecessary_extern_crate)]
#![allow(unused_imports)]
extern crate libc;
mod foo {
use crate::libc;
}
fn main() {}generates an error:
error: `extern crate` is unnecessary in the new edition
--> src/main.rs:5:1
|
5 | extern crate libc;
| ^^^^^^^^^^^^^^^^^^ help: remove it
|
note: lint level defined here
--> src/main.rs:2:9
|
2 | #![deny(unnecessary_extern_crate)]
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
error: Could not compile `playground`.
but if the extern crate libc; is removed it no longer compiles!
This situation has come up on two occasions so far:
- In attempting to fix 2018 compatibility lint isn't firing for all imports that need to be rewritten #50660 the lint started firing for
use libc;and rustfix rewrote it touse crate::libc;. This worked in the 2015 edition becauseextern crate libc;was declared at the top. Later thisunnecessary_extern_cratelint ended up showing the error above. - Discovered in rustc: Fix
cratelint for single-item paths #50665 theuse *;statement is entirely disallowed in the 2018 edition, so we instead recommend the replacementuse crate::*;. This, however, will also break if one of the identifiers used is anextern crateas the lint currently suggests removingextern crate
Activity
alexcrichton commentedon May 11, 2018
cc @Manishearth
Manishearth commentedon May 11, 2018
I and @nikomatsakis discussed this a bunch when implementing this and decided not to fix this problem. An earlier commit of mine there had an attempt which would try and ask you to directly import crates when used as
use (0+ local modules in chain)::cratename::(optional foo). It's tricky to get right though. I can dig up the commit if we want that.Manishearth commentedon May 11, 2018
The fix is in Manishearth@c904d98
It's not perfect for a bunch of reasons (also might not do precisely what I said in the commit above, but close)
alexcrichton commentedon May 11, 2018
While I can't say for sure I'd be pretty certain that this pattern shows up in the wild (especially
use foo::krate_name::bar). If we go with the status quo then it means crates with this pattern won't be able to use rustfix as currently it's an all-or-nothing wrt fixes. Given the experience I've had so far with rustfix I think we want to strive for the highest fidelity lints we can which to me means that we'll likely want to fix this.Manishearth commentedon May 11, 2018
Okay, so paging this in a bit more, while Manishearth@c904d98 is a pretty decent lint, we can't write suggestions for it. The problem is that use trees get split out in HIR, which means that the suggestions here can't work to well in the presence of these. It's hard to even detect that there's a use tree involved (in some cases)
Also, doing the span-splicing to emit the correct suggestion is tricky as well, we don't store spans for path segments.
Manishearth commentedon May 11, 2018
So basically this is doable as a rustfixable lint, but it will be buggy and/or very imperfect.
nikomatsakis commentedon May 18, 2018
I have a few questions. My memory is that, when an
extern crateappears somewhere other than the crate root, we are going to leave it in place, and let the user fix it manually.However, if the
extern crateappeared at the crate root, we would remove it, and then try not to rewrite paths likeuse foo::bar(which is technically accessingcrate::foo::bartoday, so to speak).This approach breaks if the code uses
use crate::foo::bar, but that's not a common pattern today. But where else does it break?nikomatsakis commentedon May 18, 2018
One answer to my question would be
use self::foo::baroruse super::foo::bar.nikomatsakis commentedon May 18, 2018
I am reminded that I initially wanted to do a kind of global pass to see where and how the
extern cratewas used, and only suggest removing it if it is not used from paths like those above. I'm not sure how easy/hard that would be to do but it seems like it would address this issue.nikomatsakis commentedon May 18, 2018
OK, so, it seems like the lint that @Manishearth pointed us at (Manishearth@c904d98) does the following:
use self::foo) and warns about it.As @Manishearth said, writing suggestions for this case is tricky, at least in its full generality. But we could certainly do something like the following:
That means we'd suppress the "not actionable" error but issue lints that guide you towards making the extern crate into something we can remove.
However, there is still the downside that the paths must be manually changed. I suspect we can overcome that in practice a lot of the time — I mean there are tricky cases, but we can look for the plain vanilla case and just issue the suggestion in that case (for example, by checking that the
spanof the use statement begins withuse self::foo::bar::or whatever).alexcrichton commentedon May 18, 2018
I discussed with @nikomatsakis a bit on discord and agree that perhaps the best thing to do here is to avoid linting anything at all, but rather wait till the lint is higher accuracy to turn it on by default.
nikomatsakis commentedon May 30, 2018
Pending fix in #51015
Auto merge of #51015 - nikomatsakis:issue-50672-remove-extern-crate-i…