Make unused-extern-crate warn-by-default

[ishitatsuyuki]

Apart from enabling the lint, this pull request also removes existing unused crates in the codebase, and fix some amount of false positives on crates with special purposes.

Now that all false positive issues are closed, it should be possible to make it available to wider users.

Quote:

Now that macro modularization is implemented, this is true today! #30849 (comment)

Concerns: can break some #[deny(warnings)].

Close#42591

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[retep998]

#[deny(warnings)] is not a huge concern because cargo caps lints for dependencies.

[ishitatsuyuki]

Blocked by #42591

[oli-obk]

I'm on mobile right now, so I can't really check, but looking at the code it seems to me that when you import a macro that does an unused extern crate import, this lint will trigger, but you can't do anything about it except complain to the author.

The macro might unconditionally depend on a crate because it might be hard to detect from the macro args, whether the extern crate is needed.

[nikomatsakis]

@oli-obk is this ever intended to become a hard error?

it seems like, in that scenario, you could also set #[allow]?

[nikomatsakis]

I don't really understand why this was allow before, can somebody give me some background? @ishitatsuyuki sorry for the delay btw.

[oli-obk]

Right. I got so hung up in the lint triggering inside the macro, I forgot you can attach attributes to macro invocations.

[ishitatsuyuki]

@nikomatsakis#30849 (comment)

[ishitatsuyuki]

I've got a draft level fix for the blocker by using a hand crafted whitelist. I have no idea if this would work or not.

Is there anyone who can give me some hints on this? @jseyfried submitted multiple PRs about the lint, but I'm afraid he's busy on something else since his activity graph is empty recently.

[nikomatsakis]

@ishitatsuyuki sorry, what are you fixing with this whitelist?

[nikomatsakis]

(I guess some errors we see in practice?)

[nikomatsakis]

I'm not really sure what's the best path forward, but I don't particularly love the whitelist. =) Maybe we aren't yet ready to make this lint default to warn.

[ishitatsuyuki]

@nikomatsakis#42591

[nikomatsakis]

@ishitatsuyuki I see. Whitelisting the builtins crate is maybe right, but allocator crate doesn't quite seem right -- there could be other allocator crates -- maybe we should instead check whether it is an allocator crate.

[nikomatsakis]

We can do that via the is_allocator(CrateNum) function on metadata -- but I'm not sure where's the best place to call it. Are you familiar with the source of the lint, or should I go digging?

[ishitatsuyuki]

Sorry, I have absolutely no understanding of rustc internals. Can you take a look for me?

[alexcrichton]

ping @nikomatsakis, do you have thoughts on the last comment?

---

As an aside, I personally feel this lint should never be turned on by default. I've very commonly used extern crate to get linkage to work correctly, and there's basically no way for rustc to detect that.

[nikomatsakis]

@alexcrichton

I've very commonly used extern crate to get linkage to work correctly, and there's basically no way for rustc to detect that.

Can you say a bit more?

[alexcrichton]

Sure yes. The compiler interprets extern crate directives in order to order arguments passed to the linker. Essentially the compiler does a topological sort of the crate graph and passes arguments in that order. This is important on Unix where linker argument order matters (it'll fail if it's in the wrong order).

Many *-sys crates on crates.io then link statically to their contents. In other words, the rlib is a proxy for the actual native library itself. Let's say then that you're working with a library like libgit2. The C library libgit2 depends on the C library libz. As a result we have two sys crates, one libgit2-sys and one libz-sys, and then namely libgit2-sys depends on libz-sys.

To get everything to work, libgit2-sys contains extern crate libz_sys but it doesn't actually use anything from the crate. If I add #![deny(unused_extern_crates)] then a compile error declares that it's not used.

I understand this is useful for pruning dependencies that are accidentally left in, but I don't think we're at a point yet where we should turn it on by default. As a result many sys crates are likely to just start picking up a bunch of #[allow] directives, which I take as a proxy for a lint that shouldn't be on by default.

[petrochenkov]

As a result many sys crates are likely to just start picking up a bunch of #[allow] directives, which I take as a proxy for a lint that shouldn't be on by default.

bad_style lints are in a very similar position - they are generally useful, but usually allow-ed in some specific subset of crates - FFI crates, which follow foreign naming conventions. Still, they are warn-by-default and making them allow-by-default would be a worse choice. So, "bunch of #[allow] directives" doesn't always mean "shouldn't be on by default".

If exten crate is used purely for linkage, I'd say it would benefit all if it was marked with allow and a commentary explaining that something special is happening here, because it's not very obvious. In the meantime crates not belonging to the special FFI subset can benefit from unused_extern_crates being warn-by-default.

[retep998]

I'm definitely in favor of this lint being warn by default. If you genuinely are just pulling in an external crate for the linkage, that really needs to be documented as such and having an allow there is a small price to pay for having a very useful lint enabled by default for the supermajority of cases where crates are not being pulled in for linkage.

[nikomatsakis]

I think I agree with @retep998 and @petrochenkov that this is a non-obvious use (and I don't mind #[allow] once in a while); but I do wish we had clearer guidelines here.

[ishitatsuyuki]

I agree with the points mentioned above. I would like to ask for your opinion to build a whitelist for known link-only crates or not. This should include allocator-tagged crates and compiler_builtins.

I think this isn't necessary though because if special linkages are going to be #allow with a description, it should apply to these crates too.

[ishitatsuyuki]

@bors r=petrochenkov

[bors]

📌 Commit a91bdf4 has been approved by petrochenkov

[bors]

⌛ Testing commit a91bdf4 with merge 78e95bb...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 78e95bb to master...

[ishitatsuyuki]

Finally it's done. Let's see the community's reaction to the lint.

[ghost]

While attempting to compile latest rust:

 Compiling bootstrap v0.0.0 (file:///home/xftroxgpx/build/2nonpkgs/rust.stuff/rust/rust/src/bootstrap)
error: unused extern crate
--> src/bootstrap/lib.rs:126:1
|
126 | extern crate serde;
| ^^^^^^^^^^^^^^^^^^^
|
note: lint level defined here
--> src/bootstrap/lib.rs:116:9
|
116 | #![deny(warnings)]
| ^^^^^^^^
= note: #[deny(unused_extern_crates)] implied by #[deny(warnings)]
error: aborting due to previous error
error: Could not compile `bootstrap`.

link to code as it is now in master

[ishitatsuyuki]

I think you're using local compiler for bootstrapping, but well that's indeed something worth to strip. PR coming.

[ghost]

Yes I am using config.toml rustc = "/home/xftroxgpx/.cargo/bin/rustc" which points to latest rust nightly.
Will it work if I don't set that key in config.toml?
EDIT: I didn't try but wants to download stage0 rust: downloading https://static.rust-lang.org/dist/2017-07-18/rust-std-beta-x86_64-unknown-linux-gnu.tar.gz
(it's kinda too slow xD)

Thanks!

[CryZe]

This still has some false positives in some cases unfortunately. But I just added #[allow(...)] on top of my extern crate declaration.

[petrochenkov]

@CryZe
Examples of false positives would be useful.

[CryZe]

Well, idk if it even shouldn't warn in my case. I have a C API, but I need to wrap it in a binary for emscripten (cause everything else doesn't really work well atm). So I just use extern crate my_c_api; in the binary for emscripten. This automatically exposes the whole C API to wasm / asm.js, while without the extern crate it doesn't. I'm not sure if that should be considered a proper false positive though.

So tldr: Symbols are only exported to the linker if you use extern crate. If you don't need anything else, you will still get warnings, even though the extern crate is important for the linker.

Update: This may actually be an emscripten only thing. emcc requires a list of all the no_mangle functions, so rustc prepares this list for emcc. The code that prepares this list in rustc might be the one that requires the extern crate to be there.

[petrochenkov]

@CryZe
I see, this is what was discussed in #42588 (comment) and below.
Uses like this are undetectable in general case and you have to add #[allow(unused)] (and probably a comment explaining why this extern crate is necessary).

[gilescope]

Could a crate that's likely to be 'implicitly used' have a flag indicating that it has 'side effects' so we avoid giving false warnings. I'm guessing the types of crates where we would get false positives know who they are and would be able to flag themselves as such.

I think its really important that we have this warning on by default because if each layer of crates has one or two more crates than they need build times will increase significantly. If we're to going to build truly huge stuff with Rust we need all levels of the crate ecosystem to be trying to keep their dependencies to a minimum.