Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move cargo-clippy into cargo #3837

Open
Manishearth opened this issue Mar 3, 2019 · 19 comments · Fixed by rust-lang/cargo#6759

Comments

@Manishearth
Copy link
Member

commented Mar 3, 2019

See rust-lang/cargo#6664 (comment) for some context.

Currently cargo clippy is a thin wrapper script around Cargo that invokes cargo check with a RUSTC_WRAPPER: https://github.com/rust-lang/rust-clippy/blob/8dfabdf11c6cdaffd7c6e6552a6ead8d52c49b10/src/main.rs

It does some additional hacks to support the clippy dogfood test and passing down arguments.

We should move this into cargo proper, like rustc: Ideally it's just a copy of cargo check that sets a different rustc executable (temporarily can be done by overriding RUSTC_WRAPPER, but the better solution probably involves modifying config.rustc() https://github.com/rust-lang/cargo/blob/716b02cb4c7b75ce435eb06defa25bc2d725909c/src/cargo/util/config.rs#L194-L215

Steps:

  • Add cargo clippy-preview to cargo rust-lang/cargo#6759
  • Make it work with dogfood tests #4050
  • Improve cargo integration, especially with cargo fix rust-lang/cargo#7006
  • add better help #4173
  • Make cargo-clippy shell out to clippy-preview?
  • Perhaps make -Wlintname work as a shortcut for -Wclippy::lintname
  • rename and stabilize cargo clippy-preview
@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

I think as a first step we should write a very basic cargo clippy-preview (nightly only?) subcommand for cargo that just is a copy of cargo check with rustc_wrapper set. We can also clean it up to make this part of the config.

We can then do the back and forth of testing it with clippy to ensure it works, and eventually rename and ship it. We should probably continue to ship cargo-clippy for a couple releases since in some cases you use a cargo from an older release.

Once we have this there are a bunch of nice changes we can make: e.g. we can modify the argument parsing so that you can just do cargo clippy -Wclippy::foo instead of needing the pesky --.

This also helps integration with cargo fix, since that can now be wholly solved on the cargo side.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

One implementation issue is that rustup currently does not export a clippy-driver binary, we should change it to do this.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

This could also be handled by using the current exe path I guess.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

Filed rust-lang/rustup.rs#1678

I do have concerns as to how this will work for non-rustup users.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@yaahallo is looking into implementing this

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Shouldn't have been closed

@kentfredric

This comment has been minimized.

Copy link

commented Jun 3, 2019

Where should I file bugs about clippy-preview? rust-clippy or cargo?

Context:

cargo clippy-preview --help
To allow or deny a lint from the command line you can use `cargo clippy --`
with:

    -W --warn OPT       Set lint warnings
    -A --allow OPT      Set lint allowed
    -D --deny OPT       Set lint denied
    -F --forbid OPT     Set lint forbidden

cargo clippy-preview --all-targets -Z unstable-options -- -W clippy::all
error: Found argument '-W' which wasn't expected, or isn't valid in this context
cargo clippy-preview --all-targets -Z unstable-options -W clippy::all
error: Found argument '-W' which wasn't expected, or isn't valid in this context

The actual value passed to -W seems irrelevant.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

It's not supposed to be used yet, that behavior is expected.

@kentfredric

This comment has been minimized.

Copy link

commented Jun 4, 2019

It's not supposed to be used yet, that behavior is expected.

I'm aware, I don't use it actively, I've just thrown it into my stack in such a way I can preemptively test it, so I can identify anything that might need to be addressed before it progresses towards some kind of usable thing.

Just trying to be helpful, not annoying :)

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Yeah, so that stuff is on the list of things to make work, but it's not yet fixed.

I should file some subissues for people to work on, let me know if you're interested in helping!

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Filed #4172 and #4173

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I think this may have been inadvertently closed (GitHub seems to be a little too aggressive scanning commit messages when pushed). Although, given that, I'm a little uncertain which steps are left to transition to clippy-preview. Should we discuss flipping the switch in 1.39? Exactly which steps should we take from here?

@phansch phansch reopened this Aug 19, 2019

@yaahc

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

@ehuss I've been waiting on a review for the change below before proceeding with the last few features for the new clippy.

rust-lang/cargo#7232

@yaahc

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

@ehuss

As for finishing up the other tasks in the checklist in the first post of this issue.

I think the better cargo integration is done other than finishing up the above issue I posted.

For shelling out to clippy-preview I assume we'll only want to do that on nightly. So I expect some conditional compilation or whatever the preferred mechanism for this will need to be added to cargo-clippy temporarily so that it can start shelling out to clippy-preview without clippy-preview erroring out because of lack of -Zunstable-options or usage of unstable options on stable. I'll defer to your judgment on the proper way to accomplish this.

Allowing args for disabling / enabling lints to be passed through to clippy without a leading clippy:: is gonna be pretty straight forward I expect, the only controversy I foresee here is whether or not we should allow non scoped clippy lint names as args at all.

And yea I'm going to also rely on you and @Manishearth for instructions on how manage the transition from cargo-clippy to cargo clippy-preview. I'm assuming it will just involve renaming clippy-preview and then making sure that cargo-clippy doesn't take over if it exists at the same time as the new cargo clippy(-preview) but I also bet there's a high chance I'm wrong on how this will work...

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

For shelling out to clippy-preview I assume we'll only want to do that on nightly.

I'm not really following this (and I don't know what Make cargo-clippy shell out to clippy-preview? means). Is this referring to clippy's own test suite? My understanding of the stabilization process would be:

  1. Rename clippy-preview subcommand in cargo to just clippy, and remove the unstable check.
    • We can also probably stabilize cargo fix --clippy at the same time, it seems good to me.
      • EDIT: Maybe not, the clippy team has indicated they may want more time, see #3630.
    • Update cargo's tests to call the clippy subcommand instead of clippy-preview.
    • Add clippy to the docs (add to src/doc/man and src/doc/src/commands).
  2. Later, stop building cargo-clippy in the clippy project (that is, remove src/main.rs). This doesn't need to be done immediately, as cargo's internal commands take precedence over external ones. But it may be a good idea to avoid confusion.
    • Would also need to update the calls to "cargo-clippy" in clippy's test suite to execute "cargo clippy" instead. This last step would need to wait for the cargo update to hit nightly. Alternatively, this step can be done earlier and execute "cargo clippy-preview" instead, but would need to be changed as soon as clippy-preview is stabilized.
      • EDIT: Hm, or something along these lines, I haven't looked at clippy's tests too closely. It may need something different to run the correct driver.

I don't really have an opinion on skipping the clippy:: prefix for lint names. That seems like it could be done at any time.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Oh, and also for step 2 of removing cargo-clippy, there are some updates needed in rustbuild to remove it from the distribution (roughly around here), but I can help with that if needed when the time comes.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

But it may be a good idea to avoid confusion.

I would wait a bit because iirc there are situations where you have an older cargo and a newer toolchain, so we should wait a couple cycles for this.

@U007D

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

I'm curious what the plan is for cargo test and cargo run, etc. being invoked with clippy?

For context, I prefer to use clippy like a -Wall/-Weverything (gcc/clang) strict compilation setting (ie. clippy runs on every cargo build/run/test/etc.), as opposed to an occasional "check to see what the linter thinks about all this new code I wrote".

To achieve this, currently I use the RUSTC wrapper technique. With clippy integrated into cargo, will clippy be able to be configured to be invoked on any compilation initiated by cargo?

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

No plan currently, but it should be easy to add once the main stuff is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.