Rename clippy-preview to clippy and stabilize it

[yaahc]

related to rust-lang/rust-clippy#3837

[rust-highfive]

r? @alexcrichton

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

[yaahc]

r? @ehuss

[Manishearth]

should get final review from cargo team, but good to go from our side!

[yaahc]

I imagine we will need to add the docs changes that @ehuss mentioned in rust-lang/rust-clippy#3837 (comment) before this is actually ready for merge.

[alexcrichton]

Looks good to me! Want to go ahead and merge the stabilization of cargo fix --clippy here as well? With docs I think this should be good to go

[ehuss]

Thanks @yaahc! This looks good.

For the documentation:

I would just pick a command like cargo-vendor and just duplicate what it does. The .adoc files are the source, and from there it uses asciidoc to generate the man pages and HTML pages. There is a Makefile in src/doc/Makefile, with some brief instructions on how to run it. There's also some instructions in the README.md with how to run mdbook to verify how it looks (we use mdbook 0.3). Some additional notes:

• There needs to be a markdown wrapper in src/doc/src/commands to load the HTML.
• Don't forget src/doc/src/SUMMARY.md. The categories are really rough, but I guess it would go in the "build" section, since that is where check lives.
• If the makefile touches a bunch of files just to update the date, just revert those files. The Makefile is not very smart, since it is mtime-based.
• I would maybe add a see-also link at the bottom of the cargo-check.adoc page, since it is kinda similar functionality (and conversely from cargo-clippy to cargo-check).
• EDIT: To verify how the man page looks, on most unix platforms you should be able to run man src/etc/man/cargo-clippy.1 to view the file directly.

[ehuss]

merge the stabilization of cargo fix --clippy here as well?

There was some prior discussion where the clippy team may want to delay the fix command because the suggestions aren't quite ready (the machine-applicable ones have a tendency to fail). So I think we'll want to delay that part until they're comfortable with it, and it has had more testing.

[alexcrichton]

Oops sorry, I thought the approval here from the clippy folks also covered cargo fix --clippy, but if that's not the case then yeah let's keep them separate

[ehuss]

Proposal to merge this PR to make cargo clippy a built-in command.

@rfcbot merge

What is stabilized

This adds a new built-in command named cargo clippy. This replaces the cargo-clippy plugin that ships with the clippy rustup component.

Why?

This makes it easier to provide a better experience using the cargo clippy command by taking advantage of some internal aspects of cargo and making it behave in a more similar fashion to other Cargo commands.

Some specific differences:

• Forces clippy to run, avoiding confusing behavior where if you run cargo check and then cargo clippy, Cargo fails to do anything because Cargo believes it is "fresh".
• Avoids running the clippy driver on unrelated dependencies.
• Makes it easier to integrate with the -Zcache-messages feature.
• Command-line help and documentation more closely match the underlying command (cargo check).

Impact

In my belief this will have negligible maintenance burden.

Notes

• Not yet implemented: In the future the clippy flags may be promoted to be parsed by Cargo. The -W, -A, -D, and -F flags can control lint levels. Currently they must be passed after a double dash (such as cargo clippy -- -Wclippy::style). There may be a future enhancement that would allow skipping the double dash by having Cargo parse them.
• Basically unrelated to this (though made much easier to implement due to the work here), cargo fix --clippy is now available as an unstable feature, and will be stabilized independently. This is currently waiting for the clippy team to be comfortable with the reliability of the "machine-applicable" lints.

References

• Initial implementation: Cargo clippy #6759
• Tracking issue: Move cargo-clippy into cargo rust-clippy#3837

[rfcbot]

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @alexcrichton
• @ehuss
• @joshtriplett
• @nrc
• @withoutboats

Concerns:

• overall-ux-of-cargo (Rename clippy-preview to clippy and stabilize it #7382 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[withoutboats]

@rfcbot concern overall-ux-of-cargo

I've skimmed through the issues tracking the work on cargo/clippy integration this year and I can see that a lot has been done. I think that's great, and I'd like the lints in clippy to be accessible through cargo on stable Rust.

But here's my comment from about a year ago in rust-lang/rust-clippy#1358, all of which I still think today:

I think clippy is a great name for this project and wouldn't suggest changing it, but I'm somewhat concerned about the overall Rust + cargo interface implications of what's happened here.

I know this is implemented as a custom subcommand, but if we start shipping this component by default, we've functionally added a new (official) subcommand to cargo. Is that the intention for devtools in general? - any new devtool with a CLI will just be accessed by a cargo <project-name> interface? Where is the step where we think holistically about the interface our tools present to the user?

If we are going to eventually ship clippy by default, I'd be more comfortable with some sort of unified cargo lint interface, possibly with some way to configure sources of lints, of which clippy would be a default source. Or possibly there shouldn't be a subcommand at all once its shipped by default, and you should instead just say you want these lint passes run in addition as part of cargo build.

I don't care about the interface to the clippy-preview optional subcomponent, but once we start shipping components by default I think we need to revisit how they fit into the overall Rust toolchain experience: given that they will be on equal footing with Rust and cargo at that point, there is both a need for further revision and less constraints on what kind of interface you can have (i.e. at that point you can make PRs to cargo and rustc that are necessary to get the interface you want).

Unfortunately, this post didn't get a lot of engagement from people involved in clippy a year ago. I don't know if there are other threads that are relevant that I should read that would contain more information on this subject that would convince me these concerns should not be blocking on stabilizing the cargo clippy command.

I don't think we need to have a fully configurable linting infrastructure today. But I do think we need to design the interface to be forward compatible with that. I think cargo test provides a good model - though the test harness has not been configurable, the interface is intended to support use with a different test harness someday in the future.

Concretely, I think my concern boils down to two things:

• The use of clippy in the interface name. I think this command should be named cargo lint (and the flag to cargo fix should be similarly --lint or something along those lines). I don't think the clippy project should be renamed, but I expect cargo subcommands to be neutral and descriptive, not to contain branding. The meaning of this command would be very opaque to a user who doesn't have pre-existing knowledge of Rust community history.
• The pass through of clippy arguments concerns me a bit, because I am concerned it could create a barrier to using other lint sources in the future. I'm not saying I think this is bad per se, I'm saying I'd like to see written up an explanation of how this pass through is forward compatible with other lint providers in the future.

I know that I've been a very absentee member of the cargo team this past year, and I apologize if this has been discussed at length in cargo team meetings I've not been present for. I'll gladly make resolving this concern a priority for me and I hope clippy's lints can be accessible by default to all users of stable Rust very soon.

[Manishearth]

@withoutboats to be clear, Clippy is already shipped by rustup on stable as cargo clippy, just as an external cargo-clippy subcommand (and has been for a year). This PR changes very little from the immediately-noticeable interface, the CLI is a nicer (and we've opened the door for an even nicer CLI) and a bunch of impossible-to-otherwise-fix bugs are now fixed. cargo fix --clippy is new and being stabilized in #7383, likely after this lands (?), but everything else here existed already, with some bugs. This is largely an implementation detail being "stabilized".

The subcommand already exists with this name, we're replacing it with a cleaner implementation, and cargo clippy-preview was basically a way for us to test the cleaner implementation without stomping on the preexisting cargo clippy binary that has been shipping for a year. This subcommand still needs the clippy rustup component installed to work.

With that in mind, and with stability guarantees, at a bare minimum this PR must add cargo clippy, with a compatible interface (You can also not land this, but this fixes a lot of bugs that we couldn't earlier that people have been waiting on for ages). We can discuss adding a separate lint command (and going through some deprecation), but I don't see how that's a blocking concern for this issue.

---

So onto that topic: I would rather add such a command when it is easy to hook more lints into the compiler. The way clippy does it is not scalable when it comes to tooling (replacing the rustc means that all other tools get confused: a bunch of the bugs this PR fixes arises from that, and this is also why RLS needs to know about clippy).

Furthermore, I think not calling it lint has a lot of benefits: right now you both have all the builtin lints, as well as the clippy lints, and I feel like calling the subcommand lint would be counterproductive and make this distinction more confusing. This is "branding", but it's branding for the set of lints that get loaded, not for the tool itself.

Given that we're trying to deprecate plugins, we're moving in the direction of making custom lints harder to do, not easier. Clippy has discussed supporting a reduced set of custom lints based on a Coccinelle-like syntax. This arises from a bunch of refactorings we're planning internally to make lints easier to write (rust-lang/rust-clippy#3875), though there's a long way to go before any of this can be pluggable. Once we're closer to that it might be worth imagining what a custom lint interface for cargo lint would look like.

Ultimately I don't consider the name to be a significant barrier to learning. I don't think the name lint will have a bunch of people randomly trying cargo lint and being pleasantly surprised it works -- there's no consistent cross-language tool name for this kind of thing (Go uses vet, Python and JS have multiple named linters). People are going to discover this tool through learning materials or cargo's help and I don't think the name matters in either case. I've found "use clippy" to be a popular refrain when folks are talking to newcomers, "use the linter" is less direct.

To some extent, "cargo lint" may actually give off the wrong impression, since in many ecosystems "linter" is really their equivalent of a non-autofixing rustfmt with a couple extra stuff. C++ has a lot of clippy-like tools, and the community usually calls them "static code analysis tools". The main reason clippy uses the word "lint" at all is because that's what they're called in the rustc source.

[Manishearth]

The meaning of this command would be very opaque to a user who doesn't have pre-existing knowledge of Rust community history

Also, note that the lints are also namespaced under clippy, and are supposed to be namespaced under something. If we were to resolve the opacity you're worried about here, we'd have to rename those as well (to lint::?). I'd argue that if we want support for alternate custom lint sets in the future, giving clippy a neutral name right now would be counterproductive since it would be confusing when we have more than one lint set.

We basically need to keep some name around for the lint set, and if that name exists then we'd need cargo lint --lintsetname, and given that we have precisely one non-builtin lint set right now, with no short-term plans for more, cargo lintsetname seems like a good shortcut to have. And as I explained that a neutral name for the set of lints is counterproductive here, keeping clippy doesn't seem so bad with these constraints.

If we're going to be adding support for custom lint sets in the future, giving the interface a more neutral name now while keeping decent UX seems like a bad idea to me.

It seems like if we were to figure out what custom lint sets would look like, then it might be easier to design this neutral interface, but without that I'm wary of predesigning an interface. There has been very little interest in supporting easy-to-implement custom lint sets from the rustc side, and while clippy may have a viable path for this we're not sure and our primary interest in that direction for now is making clippy's own lints easier to maintain. This is quite a ways away from happening.

[yaahc]

The subcommand already exists with this name, we're replacing it with a cleaner implementation, and cargo clippy-preview was basically a way for us to test the cleaner implementation without stomping on the preexisting cargo clippy binary that has been shipping for a year. This subcommand still needs the clippy rustup component installed to work.

It seems like we could keep shipping the cargo-clippy external subcommand if this were shipped as cargo lint and that would not be a breaking change. I do worry about having an apparently slightly buggy UI to clippy live forever, am I correct in remembering that there have been bugs that basically got closed as "wont fix" because they're already fixed by the cargo clippy-preview interface? These bugs would all have to be reopened if we shipped this as cargo lint I expect.

Also, note that the lints are also namespaced under clippy, and are supposed to be namespaced under something. If we were to resolve the opacity you're worried about here, we'd have to rename those as well (to lint::?). I'd argue that if we want support for alternate custom lint sets in the future, giving clippy a neutral name right now would be counterproductive since it would be confusing when we have more than one lint set.

My guess is that the lints would stay name spaced under clippy:: as the lint provider scope, and this would also solve the forward compat issue boats has by having cargo separate the args for providers based on their scope, at least for the -A -D -W args being passed through.

there is both a need for further revision and less constraints on what kind of interface you can have (i.e. at that point you can make PRs to cargo and rustc that are necessary to get the interface you want).

Not sure I'm understanding the point here correctly, but I don't think its a particularly big deal for clippy changes to require PRs to cargo or rustc. Clippy depends on rustc internals so I think thats already a constraint on clippy. And as for cargo's PR process my experience with it has always been easy and positive.

---

I do see the value to a generic lint interface and I would love to see one, and more linter sources. But I do think I lean in Manish's direction on this one. I think its fine to stabilize this as clippy and then we could immediately open an issue for cargo lint and copy the source of the cargo clippy subcommand and start adjusting it to have the features we would need for a generic lint interface.

[Manishearth]

I do worry about having an apparently slightly buggy UI to clippy live forever, am I correct in remembering that there have been bugs that basically got closed as "wont fix" because they're already fixed by the cargo clippy-preview interface?

Yes, and it's not just "won't fix", it's also often "can't fix".

My guess is that the lints would stay name spaced under clippy:: as the lint provider scope

Right, which means that people need to learn the name "clippy" anyway.

[withoutboats]

Sorry, my previous comment was unproductive. Spoke a bit to @Manishearth just now, and clarified some things.

The division line that I see us crossing here (and that I've been concerned about, including in the comment last year) is when we ship a cargo subcommand pre-installed to everyone by default. If the user has to take special action to get cargo clippy installed as a valid command on their machine (even if that action involves adding rustup components and not cargo install), I'm comfortable with it having whatever interface makes local sense for clippy. But if the interface is a part of the pre-installed cargo interface, I think the interface needs to be examined and supported from the holistic view of the cargo tool.

From that perspective, I have these concerns with the current cargo clippy proposal:

• I think using the name clippy in the interface is very inconsistent with other cargo subcommands, which are overwhelmingly short, generic verbs. I think a preferable interface would be to choose a verb for the "run lints" command, and for clippy to be the default provider of lints, just as the current test harness is the default test harness for cargo test. I don't care that it is "lint" specifically, just that it is a short, generic verb like the other cargo subcommands.
• I think if we have a command shipped to every machine, that command should work on everyones' machines. I'm concerned that cargo clippy will error unless users install clippy. I know that there are real infrastructural challenges to shipping clippy by default, but I see being able to do that as a prerequisite for making a command to run it available on every single stable user's machine.

I prefer what I think is already the status quo (but I admit I've had difficulty piecing it together from these threads): an external cargo-clippy tool that people can install will call the cargo clippy-preview subcommand, passing whatever bootstrap flags are needed to run unstable features on stable cargo. I think that's a fine compromise in the same sense that we allow std to use nightly features on stable today.

[Manishearth]

That's fair.

Anyway, I'll wait for y'all to discuss this and come to consensus within yourselves, and we'll see how to move forward from there.

[oli-obk]

I think we can have our 🍰 and eat it, too. If I understood the suggestions and positions correctly, we have one uncontroversial way forward:

1. Rename cargo clippy-preview to cargo lint
2. Make cargo lint unstable so it won't be callable from stable (or call it lint-preview?)
3. Give cargo lint a --command flag
   • this will do whatever cargo clippy-preview does right now, but use the argument to --command instead of hardcoded clippy-driver
4. keep cargo clippy as a subcommand installed by the clippy component, but make it just a shim that calls cargo lint --command clippy-driver
   • add some hacks so clippy can call the unstable cargo lint even on stable

This will keep the custom name and special cased clippy API entirely in the hands of the clippy project without expanding the stable cargo API

[Manishearth]

@oli-obk I mean, we don't need to yet introduce cargo lint for this, we can rename cargo clippy-preview to cargo clippy-internal and get the same benefits. This has already been proposed 😄 . I'm okay with it, though I'd prefer if we got the benefits of a full cargo command (a place in help and the docs)

This way we don't have to tie the design of the lint command to this, though I'm definitely interested in kicking off that discussion as well (if a bit skeptical that we can come up with a good design before custom lint engines start existing)

[oli-obk]

Oh yea, we can definitely start out by just doing that and slowly iterating the unstable design.

This way we don't have to tie the design of the lint command to this, though I'm definitely interested in kicking off that discussion as well

Oh I definitely didn't mean to suggest this to be the final design. I just wanted something that we can reach quickly.

[yaahc]

Keeping the command unstable seems to do a good job of not tying down the design and it existing would in theory introduce people to the concept of a future lint subcommand, though i doubt it would be extremely visible, so I imagine most discovery of it would be by word of mouth.

[spunit262]

Should not the name of a future lint command just be check? I always just think of clippy as just check but with more lints.

[joshtriplett]

On October 5, 2019 8:47:09 AM GMT+02:00, spunit262 ***@***.***> wrote: Should not the name of a future `lint` command just be `check`? I always just think of `clippy` as just `check` but with more lints.

Agreed. We should fully integrate the two so that check runs both and provides all the output by line/column, rather than all the compiler output followed by all the clippy output. I don't think we should have a separate `cargo lint` at all. I do think we should have a separate `cargo clippy`that just runs clippy, but it can be an alias for an option to check, too.

[Manishearth]

Agreed. We should fully integrate the two so that check runs both and provides all the output by line/column, rather than all the compiler output followed by all the clippy output.

I think that's already the case? The compiler and clippy lints get printed together. No other compiler output is mixed in because of the way the compiler pipeline works.

[Manishearth]

Should not the name of a future lint command just be check? I always just think of clippy as just check but with more lints.

slight difference: you want to rerun lints each time you run the command

[nrc]

Sorry I missed this for so long, I have been absolutely drowning in GitHub notifications lately :-(

First off, I think there is a lot of very valid discussion here and given the permanent nature of a change due to back-compat, I think we should not rush this decision.

There are too many points to quote and reply so here is a random selection of thoughts in random order:

• lint does have some pedigree - it is used in Java and JS and other places
• I think this PR makes perfect sense from the perspective of Clippy's evolution, but I think that @withoutboats is correct that we must consider this from the angle of Cargo's interface.
• I think that it is important that Cargo support tools like Clippy and Rustfmt. IMO Cargo is not just a package manager or build system, it is a platform for operating on Rust projects - rustc is one of the tools it uses, but it can also publish to crates.io (and other regitries), which is totally unrelated to compilation, and so forth. Being able to run cargo fmt and cargo fix is a real ergonomic win for Rust, and I would like Cargo to do more of this. Having said that, I don't want Cargo to be a dumping ground, and I think that the line between 'plugin' and 'built-in' is the right place to police that.
• I think having built-in commands which are not installed by default is a new category of thing which we should consider a bit more deeply than just adding another Cargo command.
• I don't worry too much about back-compat with the existing commands. clippy-preview can be removed - that is the whole point of the -preview suffix. cargo clippy can be supported, but deprecated and removed at the next edition (if we can make things more ergonomic by adding some temporary hacks to Cargo, then that also seems fine, lets be practical about it).
• I think the check/clippy issue is in fact wider than that in that cargo build and cargo clippy don't cooperate in any way (cargo check and cargo build should also be better integrated, but that seems like it should be out of scope here).
• Even if having other lint providers seems impractical now, I feel that embedding that constraint in the UI is a bad move for the future.

I think that my preferred solution (mentioned by @withoutboats and @spunit262) is that clippy linting should be an argument to check and build, rather than a command of it's own. That has the benefit of integrating nicely with both commands and doesn't require the user to use Cargo twice.

I'm not sure of the exact syntax, but I think something like --lints clippy would be good since it leave the door open to other suppliers of lints some day. We might in fact want something more general like --tool clippy, I'm not sure. Such a flag would not be passed to rustc, it would be handled by Cargo and Cargo would have to know about Clippy and if not installed could direct the user to install it (it is possible to have Cargo but not rustup, so the clippy shim might not always exist). Such a syntax (c.f., --clippy or something like --more-lints) would be extensible in the future but is still not too much typing (we could alias cargo clippy to cargo check --lints clippy either temporarily with a deprecation message or permanently).

[Manishearth]

I think the check/clippy issue is in fact wider than that in that cargo build and cargo clippy don't cooperate in any way (cargo check and cargo build should also be better integrated, but that seems like it should be out of scope here).

Not sure what you mean by that -- cargo clippy generates rmeta files just like cargo check and i think pipelined builds mean that they help cargo build). Basically, buildandclippyinterop just as much asbuildandcheckdo, because clippy is a modified version ofcheck`

lint does have some pedigree - it is used in Java and JS and other places

This pedigree is somewhat tainted: in some communities it's more on the side of rustfmt things than clippy.

---

My pushback against starting off with cargo whatever --lints clippy is that we don't know what the design of custom lints in the future will be like, and I'm wary of closing any doors when this thing is very far in the future.

I also want it to be easy to run clippy, CLI flags are not easy. This is a major criticism of Git, for example -- common tasks are often hidden behind CLI flags that you need to memorize (and Git, at least, ships with bash completion!). I think this is a far worse problem than "our verbs are inconsistent", actually. I can't think of any common task that is currently behind a flag in Cargo, it's all "cargo dothething", and that's a very good thing. It's not that I don't use flags, it's just that I typically only use flags in exceptional situations (cross compilation, features, etc).

This is not a problem if we also ship the alias (which can be done through rustup as it's done right now), but we shouldn't deprecate it.

Furthermore, one of the reasons behind this PR is to make cargo clippy work slightly differently from cargo check (which the current implementation is thinly built on): it adds support for passing in -W flags, as well as making it run even when you have a completed build. You'd have to be willing to make these changes to check if you want to roll it into check.

---

To reclarify my current positions

• I would like us to ship a cargo clippy command by default in cargo (but have it complain early if clippy isn't installed)
• Failing that, I'm still okay with shipping cargo clippy via rustup as we do today as a shim around some internal hidden cargo clippy-internal command that is enabled with a secret env var or something
• I'm also okay with doing the same thing as a shim around cargo whatever --lints clippy, but I don't want to deprecate or remove the one-word way of invoking clippy.
• I'm not okay with the recommended way of using clippy being with a CLI flag
• I'm wary of designing for a world with multiple lint engines this early given how far off that is.

[nrc]

I can't think of any common task that is currently behind a flag in Cargo, it's all "cargo dothething", and that's a very good thing

I agree with this statement, but come to the opposite conclusion. Although we're used to thinking of Clippy as an action (i.e., a thing to do), I think that is because of history. As a user, I don't actually want to run Clippy, I want to check my code for errors or attempt to build my code, but I want to get Clippy's input as well as the compilers (i.e., the thing I want to do is to check or build my code, and using Clippy is about customising how that thing is done).

Not sure what you mean by that

I mean that I want to try build my program and see Clippy warnings at the same time as the compilers, i.e., I want a single command that does cargo clippy && cargo build (but integrating the warnings together).

[bors]

☔ The latest upstream changes (presumably #7450) made this pull request unmergeable. Please resolve the merge conflicts.

[Nemo157]

As a user I strongly agree with the sentiment that Clippy lints should just integrate into the normal build process. My preference would even be to note in the Cargo.toml that Clippy should be active for this crate, and have the lints run during all kinds of builds (this is the issue I assumed @nrc meant about integrating cargo clippy and cargo build better, they might share output files but they don't share output warnings). This would allow me as a project maintainer to not have to tell contributors anything about running Clippy, they would just run cargo test as normal and either see Clippy warnings output, or a warning like this project has registered the `clippy` lint provider, but you do not have it installed, you can get it by running `rustup component add clippy` .

I believe having lint providers configured in the Cargo.toml would also help integrating with other ecosystem tools, e.g. RLS currently requires you to explicitly add #![warn(clippy::all)] in order for it to detect that it should be using Clippy when producing warnings.

[Manishearth]

There has historically been a lot of pushback from other teams to running Clippy as part of the standard build process. That option isn't being put on the table here.

…

On Wed, Oct 16, 2019, 2:27 AM Nemo157 ***@***.***> wrote: As a user I strongly agree with the sentiment that Clippy lints should just integrate into the normal build process. My preference would even be to note in the Cargo.toml that Clippy should be active for this crate, and have the lints run during all kinds of builds (these is the issue I assumed @nrc <https://github.com/nrc> meant about integrating cargo clippy and cargo build better, they might share output *files* but they don't share output *warnings*). This would allow me as a project maintainer to not have to tell contributors anything about running Clippy, they would just run cargo test as normal and either see Clippy warnings output, or a warning like this project has registered the `clippy` lint provider, but you do not have it installed, you can get it by running `rustup component add clippy` . I believe having lint providers configured in the Cargo.toml would also help integrating with other ecosystem tools, e.g. RLS currently requires you to explicitly add #![warn(clippy::all)] in order for it to detect that it should be using Clippy when producing warnings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7382?email_source=notifications&email_token=AAMK6SHWSURAIQHX5EORUXLQO3NBTA5CNFSM4IYDTLEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBL2AAA#issuecomment-542613504>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMK6SEOVZE3KIK2WIXNHB3QO3NBTANCNFSM4IYDTLEA> .

[ehuss]

The Cargo team discussed the issues brought up here, and we'd like to propose the following alternative:

Given the primary motivator here is to fix the fingerprint/caching issue, we'd like to extend Cargo in such a way that the cargo-clippy plugin can work properly, and remove the proposed clippy-preview command.

Background: Currently cargo-clippy uses the RUSTC_WRAPPER environment variable to replace rustc with clippy-driver, which uses rustc_driver to extend rustc. It then calls cargo check to perform linting. Currently, RUSTC_WRAPPER is not included in the fingerprint, so Cargo thinks the result of cargo check is identical to cargo clippy and does not rebuild.

The proposal is to add a RUSTC_WORKSPACE_WRAPPER environment variable. This variable would be used for workspace members only, and take precedence over RUSTC_WRAPPER. This variable will be added to the fingerprint, so that clippy results will be independent from cargo check results.

@yaahc indicated they'd be willing to implement this, which will initially be only used behind the -Zunstable-options flag. This will give use the opportunity to test it out with the cargo-clippy plugin. We hope to stabilize it relatively soon afterwards.

Some of the other issues with cargo-clippy can possibly be addressed by other means, and should be done independently as desired. For example, one of the concerns is that the --help page is incomplete. It may be possible for cargo-clippy to run cargo check --help and re-display that output (possibly with some modifications), and then append its own additions. Or it could bake-in a near-copy, edited as desired. This isn't as clean, but is a problem with any cargo plugin, that perhaps some future work can make smoother.

This maintains the status quo of the current solution while providing a fix for the immediate concern. If there is a desire in the future to have a more extensible or integrated linting experience (however you want to define that), and someone willing to drive it, then that can be a separate project for the future, but I don't foresee that happening soon.

How does this plan sound to everyone? Were there any other motivators that would cause issues with this arrangement?

(Note: I have a separate comment on #7383 for integrating with cargo fix.)

[yaahc]

This plan gets a big +1 from me.

Also I'd like to point out for everyone else that the internal variable in cargo that is being set via RUSTC_WORKSPACE_WRAPPER already exists and has been merged. It and was added to make it possible to implement both cargo clippy-preview and cargo fix --clippy. The new plan is to just expose this so that all the necessary changes to make cargo clippy and cargo clippy --fix work the way we want can be done outside of cargo.

[Manishearth]

@ehuss i'm concerned about including it in the fingerprint: won't this mean that cargo clippy will delete existing metadata from a previous check run? i'd rather clippy not cause problems with other workflows, it should be able to read and write the same metadata

Otherwise this sounds great.

I'm not a huge fan of the wrapper model -- i feel like PRIMARY_RUSTC would be better than PRIMARY_RUSTC_WRAPPER -- but I'd defer to y'all for which plugin interface you want to encourage.

[ehuss]

Oh, I should have been clearer on that. It should include it in the metadata hash, not the fingerprint (it doesn't really matter if it is in the fingerprint, it's probably not necessary). That way, the clippy artifacts will have a separate filename, and won't collide with check.

As for RUSTC vs RUSTC_WRAPPER, I think clippy is using the wrapper variant intentionally (rust-lang/rust-clippy#2087). I'd be reluctant to switch that, but if there is some way to make it work, that's an option.

I just realized in my note on fix --clippy that I mentioned using RUSTC, but clippy expects RUSTC_WRAPPER. That will need to be considered, so maybe I should have said RUSTC_WRAPPER there, and the code will need to inject the extra arg as needed.

[Manishearth]

oh, yeah build scripts do make this an issue. Let's stick to the wrapper then.

[Manishearth]

That way, the clippy artifacts will have a separate filename, and won't collide with check.

Hmm, ideally they should be able to share artefacts since clippy doesn't do anything to change the generated artefact, but i guess this is fine

[ehuss]

Hmm, ideally they should be able to share artefacts since clippy doesn't do anything to change the generated artefact, but i guess this is fine

Yea, there's not much that can be done about that for now. The only consequence is a little wasted disk space. The dependencies will all be shared, just the workspace members might have two copies. I personally would also be fine with just putting it in the fingerprint if the extra disk space usage is a concern, I don't really care either way.

[yaahc]

Confused by the discussion around WRAPPER. My understanding is that clippy currently sets primary_unit_rustc which does not interact with the wrapper itself. The compile step will pick between invoking rustc_process and primary_unit_rustc_process depending on if its a workspace member or not. My understanding is that this should be independent from the wrapper, in that both will be executed with the same wrapper.

I'm not really sure if this has any impact on the discussion around fingerprints and metadata hashes but I want to make sure I'm not misunderstanding how people want this implemented. My plan right now does not include messing with any form of wrapper.

[ehuss]

Confused by the discussion around WRAPPER. My understanding is that clippy currently sets primary_unit_rustc which does not interact with the wrapper itself.

clippy-preview does that, but the proposal is to remove it. The current cargo-clippy plugin would need to be changed to set a new environment variable (RUSTC_WORKSPACE_WRAPPER). Cargo would need to read that, somewhere. I haven't thought about how best to structure that. It could go in Compilation::new, or Compilation::rustc_process, or in the Rustc struct itself. Compilation::rustc_process would be the place where it makes the decision on which process to use. Then either compute_metadata or the fingerprint will need to hash the value of that environment variable.