Cargo clippy #6759

yaahc · 2019-03-17T20:27:50Z

rust-highfive · 2019-03-17T20:27:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/bin/cargo/commands/clippy.rs

src/cargo/util/config.rs

src/cargo/util/paths.rs

Manishearth · 2019-03-17T20:38:50Z

For some background, this is the first major step for implementing cargo-clippy in Cargo, which is why the command is called clippy-preview. Doing it step by step so we can ensure the clippy repo can still CI with this.

alexcrichton · 2019-03-18T20:06:06Z

Thanks for the PR @yaahallo! Scanning over rust-lang/rust-clippy#3837 and the implementation here, it's not entirely clear to me what the transition plan is here, although it looks like cargo clippy-preview is being added here to eventually replace cargo clippy itself, but the thinking is that clippy-preview can be used in the meantime?

If so I agree with @Manishearth that we'll want this command to be nightly-only for now, which basically means we should probably just require that -Z unstable-options is passed as well to enable the command.

Would it be possible to take a cargo-fix-style approach though to avoid adding too much special code for clippy in a few places? Ideally all the clippy-specific logic would be in one or two central spots

Manishearth · 2019-03-18T20:39:31Z

@alexcrichton the transition plan is that:

clippy-preview is implemented (this PR). nobody aside clippy devs use it, while experimenting with upgrading clippy CI for this
clippy CI is upgraded to run all tests with cargo clippy-preview
cargo clippy-preview is renamed to cargo clippy, and made stable. Clippy still ships a cargo-clippy binary so that people using fallback cargos are not affected during the transition.
Clippy's cargo-clippy binary is removed after a release cycle.
Further improvements to cargo clippy may be made

So yeah, making it nightly only makes sense. Naming it "preview" lets us experiment without causing any issues to real users of cargo clippy.

Would it be possible to take a cargo-fix-style approach though to avoid adding too much special code for clippy in a few places?

As far as I can tell this is the cargo-fix-style approach, cargo-clippy is an extremely thin wrapper around cargo check and will be implemented as such here. The main departures from cargo check will be:

Runs clippy-driver on toplevel crates instead of rustc (implemented here)
- In the future may be improved to do this for all local workspace crates or something. Cargo fix needs this too. (not implemented here)
Different help text (implemented here)
May directly accept -W clippy::foo-style arguments. (not implemented yet)
Eventually will integrate with cargo fix (not implemented yet, but easier to implement once cargo clippy is part of cargo)

yaahc · 2019-03-18T20:41:26Z

I'll go ahead and add the -Z unstable-options requirement right now.

Manishearth · 2019-03-18T20:44:35Z

A future simplification that can be made is to stop using the rustc-wrapper functionality and instead directly call it as a drop-in for rustc. Note that doing this would require tweaking cargo to know which crates to run clippy on (currently clippy-driver makes this decision). This makes it easier to support things like running clippy on all local workspace crates.

src/bin/cargo/commands/clippy.rs

src/cargo/util/config.rs

ehuss · 2019-03-18T22:38:27Z

avoid adding too much special code for clippy in a few places

I agree that it feels like there is too much special code. With this, there are now 4 wrappers (BuildConfig::cargo_as_rustc_wrapper, Config::clippy_override, RUSTC_WRAPPER, and RUSTC). It would be nice if it was simplified and more generalized (and be easy to use clippy with cargo-fix), but that may not be easy.

Manishearth · 2019-03-18T22:42:28Z

How complex would it be to merge the wrappers? If it's pretty complex I'd prefer to land this so we can make further progress on the clippy side, and then in tandem improve the wrapper situation.

We plan to make cargo clippy work with cargo fix as part of these changes so I suspect the wrappers will get merged eventually anyway.

src/cargo/util/config.rs

alexcrichton · 2019-03-19T19:49:45Z

We curently already have cargo::util::rustc::Rustc as an abstraction for RUSTC and RUSTC_WRAPPER. I think Config::clippy_override is probably not what we want because it's such a global decision for a local property (aka only during cargo clippy's own build are we using clippy as a wrapper).

We can probably fix this duplication and scattering easily with:

Rename Config::rustc to something like Config::load_global_rustc
Add #[derive(Clone)] to Rustc
Add Rustc field to BuildConfig
Remove cargo-fix specific logic in compiler/compilation.rs
Update Rustc to have the various functionality necessary, adding hooks for cargo clippy as well as cargo fix)

yaahc · 2019-03-19T20:01:53Z

Rustc has a cache: Mutex<Cache>, which prevents us from deriving Clone, should I wrap this in Rc/Arc?

alexcrichton · 2019-03-19T20:55:13Z

I don't recall the purpose of the mutex offhand but I would presume that adding Arc would be fine

yaahc · 2019-03-20T00:30:55Z

I'm a bit confused on what the relationship between BuildConfig and Config::load_global_rustc is supposed to be. Did you intend to say to add the Rustc field to Config rather than BuildConfig?

Also BuildContext has a Rustc member

alexcrichton · 2019-03-20T14:24:21Z

BuildConfig is sort of a one-shot configuration just for one build, while Config is intended for long-term usage across possibly multiple commands. I'm thinking that the Rustc definition, which is currently loaded from Config, would instead be loaded from BuildConfig. That way commands like fix/clippy would configure BuildConfig appropriately, and the backend would load the compiler from that field instead of Config which doesn't have the fix/clippy-specific modifications

alexcrichton · 2019-03-20T14:25:16Z

Also heh I didn't realize it was already there! In that case we should just rename Config::rustc and reroute existing users to build_config.rustc

yaahc · 2019-03-21T23:08:48Z

src/cargo/util/rustc.rs

@@ -141,8 +141,8 @@ impl Rustc {
        self.cache.lock().unwrap().cached_success(cmd)
    }

-    pub fn push_wrapper(&mut self, wrapper: RustcWrapper) {
-        self.wrapper = Some(wrapper);
+    pub fn push_wrapper<T: Into<Option<RustcWrapper>>>(&mut self, wrapper: T) {


You could in theory call push_wrapper(None) with this.... not sure if anybody ever will. If someone ever does though I want to be notified! it would amuse me immensely.

I wonder if instead of push_wrapper and such, we could do something like this?

pub fn set_wrapper(&mut self, wrapper: ProcessBuilder) { assert!(self.wrapper.is_none()); self.wrapper = Some(wrapper); }

That way we don't have to duplicate args/env/etc, and I think there's only currently one wrapper so it's fine to avoid the term "push" because we're not adding more than one

Yeah, additionally, multiple wrappers wouldn't work anyway, since we can't pass more than one wrapper down as the first argument (yet)

The assert wont work thanks to the test fix::does_not_crash_with_rustc_wrapper which is expected to replace an already defined wrapper.

yaahc · 2019-03-21T23:25:20Z

also this is totally broken atm. all the fix:: and build::rustc_wrapper.* tests are broken now

I have to take a break from working on this until tonight or tomorrow though.

alexcrichton · 2019-03-22T16:10:04Z

I think this is looking pretty good though! If you need some help with tests and such just let us know!

src/cargo/util/config.rs

src/cargo/core/compiler/build_context/mod.rs

src/cargo/core/compiler/compilation.rs

bors · 2019-03-24T22:15:42Z

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

Now the fields are just all represented as `rustc_wrapper` which internally contains all process configuration that's later passed down to `Rustc` as necessary.

alexcrichton · 2019-04-01T17:23:16Z

Ok I pushed a follow up commit with some further refactorings, and I think this is good to go. Thanks @yaahallo!

@bors: r+

bors · 2019-04-01T17:23:17Z

📌 Commit 842da62 has been approved by alexcrichton

bors · 2019-04-01T17:23:25Z

⌛ Testing commit 842da62 with merge 025b01e...

Cargo clippy resolves rust-lang/rust-clippy#3837

yaahc · 2019-04-01T17:33:26Z

My pleasure :D @alexcrichton

bors · 2019-04-01T17:56:07Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 025b01e to master...

@ehuss

Update cargo 20 commits in 63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c 2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000 - Fix Init for Fossil SCM project (rust-lang/cargo#6792) - Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799) - Don't include email if it is empty (rust-lang/cargo#6802) - Fix unused import warning (rust-lang/cargo#6807) - Add some help and documentation for unstable flags (rust-lang/cargo#6791) - Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803) - Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804) - Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805) - Warn on version req with metadata (rust-lang/cargo#6806) - cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801) - Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800) - Cargo clippy (rust-lang/cargo#6759) - Don't include metadata in wasm binary examples (rust-lang/cargo#6812) - Update glossary for `feature` (rust-lang/cargo#6809) - Include proc-macros in `build-override` (rust-lang/cargo#6811) - Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776) - Add some docs for `Downloads` (rust-lang/cargo#6815) - Resolve: Be less strict while offline (rust-lang/cargo#6814) - Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818) - Fix doc link (rust-lang/cargo#6820) <br> I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock rust-lang#59076. Mentioning @ehuss.

@ehuss

Update cargo 20 commits in 63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c 2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000 - Fix Init for Fossil SCM project (rust-lang/cargo#6792) - Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799) - Don't include email if it is empty (rust-lang/cargo#6802) - Fix unused import warning (rust-lang/cargo#6807) - Add some help and documentation for unstable flags (rust-lang/cargo#6791) - Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803) - Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804) - Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805) - Warn on version req with metadata (rust-lang/cargo#6806) - cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801) - Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800) - Cargo clippy (rust-lang/cargo#6759) - Don't include metadata in wasm binary examples (rust-lang/cargo#6812) - Update glossary for `feature` (rust-lang/cargo#6809) - Include proc-macros in `build-override` (rust-lang/cargo#6811) - Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776) - Add some docs for `Downloads` (rust-lang/cargo#6815) - Resolve: Be less strict while offline (rust-lang/cargo#6814) - Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818) - Fix doc link (rust-lang/cargo#6820) <br> I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock #59076. Mentioning @ehuss.

Update some fix comments. - Primary handling changed in #5824. - cargo_as_rustc_wrapper changed in #6759.

rust-highfive assigned alexcrichton Mar 17, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2019

yaahc commented Mar 17, 2019

View reviewed changes

src/bin/cargo/commands/clippy.rs Show resolved Hide resolved

src/cargo/util/config.rs Outdated Show resolved Hide resolved

src/cargo/util/config.rs Outdated Show resolved Hide resolved

src/cargo/util/paths.rs Outdated Show resolved Hide resolved

yaahc commented Mar 18, 2019

View reviewed changes

src/bin/cargo/commands/clippy.rs Show resolved Hide resolved

ehuss reviewed Mar 18, 2019

View reviewed changes

src/bin/cargo/commands/clippy.rs Outdated Show resolved Hide resolved

src/bin/cargo/commands/clippy.rs Outdated Show resolved Hide resolved

src/cargo/util/config.rs Outdated Show resolved Hide resolved

src/cargo/util/config.rs Outdated Show resolved Hide resolved

yaahc force-pushed the cargo-clippy branch from 19d1d87 to 1852403 Compare March 18, 2019 23:23

yaahc commented Mar 18, 2019

View reviewed changes

src/cargo/util/config.rs Outdated Show resolved Hide resolved

yaahc force-pushed the cargo-clippy branch from eb1d3c1 to a1896cc Compare March 21, 2019 23:07

yaahc commented Mar 21, 2019

View reviewed changes

Manishearth reviewed Mar 24, 2019

View reviewed changes

src/cargo/util/config.rs Outdated Show resolved Hide resolved

yaahc commented Mar 24, 2019

View reviewed changes

src/cargo/core/compiler/build_context/mod.rs Outdated Show resolved Hide resolved

yaahc commented Mar 24, 2019

View reviewed changes

src/cargo/core/compiler/compilation.rs Outdated Show resolved Hide resolved

yaahc force-pushed the cargo-clippy branch from 4faa31b to e93e0fd Compare March 25, 2019 16:51

yaahc added 8 commits March 29, 2019 17:39

fix: remove dbg macro calls

1f8c1c4

fix: non rustup clippy missing error

e2f2c8d

impl centralized RustcWrapper logic

0389edc

totally unnecessary template type because its fun

116c053

fix not passing tests

d2ab686

actually fix it so cargo fix kinda works with new wrapper stuff

bccc0ee

remove unnecessary rustc env for cargo fix

47c82f0

fix comments

a23f811

yaahc force-pushed the cargo-clippy branch from e93e0fd to a23f811 Compare March 30, 2019 00:40

Remove no longer needed fields in BuildConfig

842da62

Now the fields are just all represented as `rustc_wrapper` which internally contains all process configuration that's later passed down to `Rustc` as necessary.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2019

bors added a commit that referenced this pull request Apr 1, 2019

Auto merge of #6759 - yaahallo:cargo-clippy, r=alexcrichton

025b01e

Cargo clippy resolves rust-lang/rust-clippy#3837

bors merged commit 842da62 into rust-lang:master Apr 1, 2019

bors mentioned this pull request Apr 1, 2019

Implement the 'frontend' of public-private dependencies #6772

Merged

dtolnay mentioned this pull request Apr 3, 2019

Update cargo rust-lang/rust#59681

Merged

Manishearth mentioned this pull request May 1, 2019

Move cargo-clippy into cargo rust-lang/rust-clippy#3837

Closed

7 tasks

ehuss mentioned this pull request Jun 22, 2019

Update some fix comments. #7061

Merged

bors added a commit that referenced this pull request Jun 22, 2019

Auto merge of #7061 - ehuss:update-fix-comments, r=alexcrichton

5277137

Update some fix comments. - Primary handling changed in #5824. - cargo_as_rustc_wrapper changed in #6759.

ehuss mentioned this pull request Jun 22, 2019

call clippy-driver directly rather than as a rustc_wrapper #7006

Closed

7 tasks

mati865 mentioned this pull request Sep 2, 2019

check subcommand should force rebuild #6664

Closed

ehuss mentioned this pull request Sep 25, 2019

Rename clippy-preview to clippy and stabilize it #7382

Closed

ehuss added this to the 1.35.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cargo clippy #6759

Cargo clippy #6759

yaahc commented Mar 17, 2019 •

edited

Loading

rust-highfive commented Mar 17, 2019

Manishearth commented Mar 17, 2019

alexcrichton commented Mar 18, 2019

Manishearth commented Mar 18, 2019

yaahc commented Mar 18, 2019

Manishearth commented Mar 18, 2019

ehuss commented Mar 18, 2019

Manishearth commented Mar 18, 2019

alexcrichton commented Mar 19, 2019

yaahc commented Mar 19, 2019

alexcrichton commented Mar 19, 2019

yaahc commented Mar 20, 2019 •

edited

Loading

alexcrichton commented Mar 20, 2019

alexcrichton commented Mar 20, 2019

yaahc Mar 21, 2019 •

edited

Loading

alexcrichton Mar 25, 2019

Manishearth Mar 25, 2019

yaahc Mar 30, 2019

yaahc commented Mar 21, 2019 •

edited

Loading

alexcrichton commented Mar 22, 2019

bors commented Mar 24, 2019

alexcrichton commented Apr 1, 2019

bors commented Apr 1, 2019

bors commented Apr 1, 2019

yaahc commented Apr 1, 2019

bors commented Apr 1, 2019

Cargo clippy #6759

Cargo clippy #6759

Conversation

yaahc commented Mar 17, 2019 • edited Loading

rust-highfive commented Mar 17, 2019

Manishearth commented Mar 17, 2019

alexcrichton commented Mar 18, 2019

Manishearth commented Mar 18, 2019

yaahc commented Mar 18, 2019

Manishearth commented Mar 18, 2019

ehuss commented Mar 18, 2019

Manishearth commented Mar 18, 2019

alexcrichton commented Mar 19, 2019

yaahc commented Mar 19, 2019

alexcrichton commented Mar 19, 2019

yaahc commented Mar 20, 2019 • edited Loading

alexcrichton commented Mar 20, 2019

alexcrichton commented Mar 20, 2019

yaahc Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

alexcrichton Mar 25, 2019

Choose a reason for hiding this comment

Manishearth Mar 25, 2019

Choose a reason for hiding this comment

yaahc Mar 30, 2019

Choose a reason for hiding this comment

yaahc commented Mar 21, 2019 • edited Loading

alexcrichton commented Mar 22, 2019

bors commented Mar 24, 2019

alexcrichton commented Apr 1, 2019

bors commented Apr 1, 2019

bors commented Apr 1, 2019

yaahc commented Apr 1, 2019

bors commented Apr 1, 2019

yaahc commented Mar 17, 2019 •

edited

Loading

yaahc commented Mar 20, 2019 •

edited

Loading

yaahc Mar 21, 2019 •

edited

Loading

yaahc commented Mar 21, 2019 •

edited

Loading