Add build and test subcommands (binary-emitting Clippy)

[blyxyas]

Being that currently, if projects want to include Clippy in their testing, it has to be separate from testing and building (which wastes so much CI time, as Clippy and build/test don't share caches), this project adds a cargo clippy build and cargo clippy test subcommands

Also see #clippy > Clippy but binary-emitting

changelog:Add cargo clippy build and cargo clippy test subcommands

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[epage]

Is the expectation that this is being insta-stabilized?

[weihanglo]

The idea of reducing the build time is nice!

My experience is that clippy always gets better in each version and enforces more good patterns. As a result, it may fail existing code when bumping the Rust toolchain version.

However I am a bit unsure about whether coupling linting with artifact producing / test executions is a best practice. At work I maintain Rust toolchain infrastructure for hundreds of Rust projects. We recommend Rust developers at work not to set -Dwarnings as a required job in CI, because that may fail whenever we upgrade our internal toolchain as clippy becomes smarter. And if there is an urgent hotfix needed, lint errors would block their builds and releases. Instead we recommend -Dwarnings in a separate non- blocking CI job, and post a comment at PR review time. Of course some perf, correctness or safety lints can be enforced but the developers should be aware and responsible for those choices. You may say hey don't -Dwarnings but at a company with thousands of Rust devs not everyone is a Rust toolchain expert.

I am not sure if these new commands are gonna push more people to tie linting with test executions and artifact producing. I think we should have the message clear about practices we want to recommend.

[weihanglo]

To make my above concern less a concern, we might want to have lint levels/configs for type-check and binary-emit commands separately. However it is not possible today through Cargo's [lints] table so we are gonna enforce the same set of lint rules. If there is a way to configure maybe that should go hand in hand with these new commands. Given this, I kinda have the same question of insta-stabilization.

[joshtriplett]

So, first of all, I'm excited about the idea of avoiding duplication between clippy and compilation. This is an interesting prototype of that.

Assuming I'm understanding the architecture correctly here, I would be hesitant to have clippy drive build and test, rather than having build and test drive clippy. I had the impression the goal was to integrate clippy into rustc, rather than having the separate clippy driver start doing builds. The former would be less disruptive to existing compilation mechanisms.

[blyxyas]

The expectance is that this will be rolled out slowly and be completely opt-in, we can add an unstable notice for a few versions before committing to a specific style of running the lints.

About the lint levels, I assume that we could define a cfg value when running cargo clippy test instead of cargo clippy, and people could match for that with [target.’cfg(clippy-emit-bin)’.lints], or just add a [lints.clippy-emit-bin] table (name pending). Even a [lints.clippy.build-test] subtable to the current clippy table.

[blyxyas]

I had the impression the goal was to integrate clippy into rustc, rather than having the separate clippy driver start doing builds.

clippy-driver doesn’t really change, so the building is still handled by cargo, just that instead of registering the Clippy lints to a LintStore in a check run, we register them to a build one.

Btw, I’d say that priority is against the test subcommand, so if it’s needed, there’d be little problem with dropping build support.

[epage]

we can add an unstable notice for a few versions before committing to a specific style of running the lints.

At least For T-cargo, and I assume T-compiler, new CLIs generally start as unstable, requiring a nightly toolchain and a -Z to use. Stabilizing it requires a team FCP.

clippy-driver doesn’t really change, so the building is still handled by cargo, just that instead of registering the Clippy lints to a LintStore in a check run, we register them to a build one.

I'd recommend exercising caution considering 3 different cargo team members, including the one you are quoting as endorsing this work, are skeptical of exposing top-level cargo commands inside of cargo clippy.

For myself, I'd want to step back and look at the problem more holistically in terms of expected and common workflows, where we want to steer users, and how to best impact those workflows for improving them with work like this.