Add an unstable --json=unused-externs flag to print unused externs

[est31]

This adds an unstable flag to print a list of the extern names not used by cargo.

This PR will enable cargo to collect unused dependencies from all units and provide warnings.
The companion PR to cargo is: rust-lang/cargo#8437

The goal is eventual stabilization of this flag in rustc as well as in cargo.

Discussion of this feature is mostly contained inside these threads: #57274 #72342 #72603

The feature builds upon the internal datastructures added by #72342

Externs are uniquely identified by name and the information is sufficient for cargo.
If the mode is enabled, rustc will print json messages like:

{"unused_extern_names":["byteorder","openssl","webpki"]}

For a crate that got passed byteorder, openssl and webpki dependencies but needed none of them.

Q: Why not pass -Wunused-crate-dependencies?

A: See ehuss's comment here
TLDR: it's cleaner. Rust's warning system wasn't built to be filtered or edited by cargo.
Even a basic implementation of the feature would have to change the "n warnings emitted" line that rustc prints at the end.
Cargo ideally wants to synthesize its own warnings anyways. For example, it would be hard for rustc to emit warnings like
"dependency foo is only used by dev targets", suggesting to make it a dev-dependency instead.

Q: Make rustc emit used or unused externs?

A: Emitting used externs has the advantage that it simplifies cargo's collection job.
However, emitting unused externs creates less data to be communicated between rustc and cargo.
Often you want to paste a cargo command obtained from cargo build -vv for doing something
completely unrelated. The message is emitted always, even if no warning or error is emitted.
At that point, even this tiny difference in "noise" matters. That's why I went with emitting unused externs.

Q: One json msg per extern or a collective json msg?

A: Same as above, the data format should be concise. Having 30 lines for the 30 crates a crate uses would be disturbing to readers.
Also it helps the cargo implementation to know that there aren't more unused deps coming.

Q: Why use names of externs instead of e.g. paths?

A: Names are both sufficient as well as neccessary to uniquely identify a passed --extern arg.
Names are sufficient because you must pass a name when passing an --extern arg.
Passing a path is optional on the other hand so rustc might also figure out a crate's location from the file system.
You can also put multiple paths for the same extern name, via e.g. --extern hello=/usr/lib/hello.rmeta --extern hello=/usr/local/lib/hello.rmeta,
but rustc will only ever use one of those paths.
Also, paths don't identify a dependency uniquely as it is possible to have multiple different extern names point to the same path.
So paths are ill-suited for identification.

Q: What about 2015 edition crates?

A: They are fully supported.
Even on the 2015 edition, an explicit --extern flag is is required to enable extern crate foo; to work (outside of sysroot crates, which this flag doesn't warn about anyways).
So the lint would still fire on 2015 edition crates if you haven't included a dependency specified in Cargo.toml using extern crate foo; or similar.
The lint won't fire if your sole use in the crate is through a extern crate foo; statement, but that's not its job.
For detecting unused extern crate foo statements, there is the unused_extern_crates lint
which can be enabled by #![warn(unused_extern_crates)] or similar.

cc @jsgf @ehuss @petrochenkov @estebank

[rust-highfive]

r? @davidtwco

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

[jsgf]

This is an interesting approach. I can see a possible path to making Buck support this, but it would need some philosophical discussion about how concerns should be separated (ie, is it really up to Buck to diagnose unused dependencies?).

[davidtwco]

Implementation looks good to me.

[davidtwco]

r? @estebank

[ehuss]

I haven't had a chance to test and review this in depth, but one minor thing I think would be good to discuss is adding some kind of tag to the JSON messages so they can be easily distinguished. For example, Cargo uses the "reason" field to distinguish them, and I was wondering if rustc should adopt something similar.

but it would need some philosophical discussion about how concerns should be separated (ie, is it really up to Buck to diagnose unused dependencies?).

Can you say more? I was assuming #72342 would be sufficient for your use case, and Buck wouldn't need to use this extra flag.

[est31]

Yeah currently cargo tries to deserialize each message by its type one after another https://github.com/rust-lang/cargo/blob/0506d8d5c9dd229c94641dd39a37bd947dc0a770/src/cargo/core/compiler/mod.rs#L1192

One could put everything into an enum that's internally tagged. This would create a tag value for each of the messages. Tools that invoke rustc can then do with it whatever they want. Ignore it, or create an enum of their own. https://serde.rs/enum-representations.html#internally-tagged

If there's support, I can make a PR to add an enum to rustc.

[jsgf]

@ehuss Well, it would be ideal if we can make one mechanism work well for everything. As I see it the pros and cons of the two approaches are:

-Wunused-crate-dependencies:

• fits in with the general Rust lint handing flow, including deny/allow/warn/forbid
• straightforward to integrate with external workflows (just another warning)
• easy to integrate with a build system, so long as dependencies are precise

• doesn't work well with Cargo's model
• requires --extern-location to be able to provide good diagnostics/tooling

--json=unused-externs:

• leaves decisions to build system, so can apply higher-level logic
• allows build system to provide diagnostics

• not treated as a rustc lint (Cargo doesn't currently have a lint concept)
• requires close coupling between rustc and build system

Strictly speaking, --json=unused-externs isn't necessary if you have -Wunused-crate-dependencies, since in principle Cargo could parse the diagnostics output and look for the appropriate messages and consume/process/filter them. But it does leave awkwardness like the number of warnings being wrong unless Cargo also corrects those which is messy.

[est31]

Huh, tests still failing.

[Mark-Simulacrum]

@bors r+

Sorry for the delay here. This looks good to me.

[bors]

📌 Commit d018ef1 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit d018ef1 with merge 1efaea80dd096579f012c5fc418efd2f3e0554a3...

[bors]

💔 Test failed - checks-actions

[JohnTitor]

@bors retry

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Command failed. Attempt 4/5:
Sending build context to Docker daemon  494.1kB

Step 1/20 : FROM ubuntu:20.04
Head https://registry-1.docker.io/v2/library/ubuntu/manifests/20.04: Get https://auth.docker.io/token?account=githubactions&scope=repository%3Alibrary%2Fubuntu%3Apull&service=registry.docker.io: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
Sending build context to Docker daemon  494.1kB

Step 1/20 : FROM ubuntu:20.04
Step 1/20 : FROM ubuntu:20.04
Get https://registry-1.docker.io/v2/: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
##[error]Process completed with exit code 1.
Post job cleanup.

[jsgf]

BTW I've been using this in some experiments and it works out really well in our new build system.