-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Cargo clippy #6759
Cargo clippy #6759
Conversation
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. |
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. |
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 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 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 |
@alexcrichton the transition plan is that:
So yeah, making it nightly only makes sense. Naming it "preview" lets us experiment without causing any issues to real users of cargo clippy.
As far as I can tell this is the cargo-fix-style approach,
|
I'll go ahead and add the -Z unstable-options requirement right now. |
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. |
I agree that it feels like there is too much special code. With this, there are now 4 wrappers ( |
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. |
We curently already have We can probably fix this duplication and scattering easily with:
|
Rustc has a |
I don't recall the purpose of the mutex offhand but I would presume that adding Arc would be fine |
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 |
|
Also heh I didn't realize it was already there! In that case we should just rename |
src/cargo/util/rustc.rs
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, additionally, multiple wrappers wouldn't work anyway, since we can't pass more than one wrapper down as the first argument (yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert wont work thanks to the test fix::does_not_crash_with_rustc_wrapper which is expected to replace an already defined wrapper.
also this is totally broken atm. all the I have to take a break from working on this until tonight or tomorrow though. |
I think this is looking pretty good though! If you need some help with tests and such just let us know! |
☔ 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.
Ok I pushed a follow up commit with some further refactorings, and I think this is good to go. Thanks @yaahallo! @bors: r+ |
📌 Commit 842da62 has been approved by |
Cargo clippy resolves rust-lang/rust-clippy#3837
My pleasure :D @alexcrichton |
☀️ Test successful - checks-travis, status-appveyor |
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.
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.
resolves rust-lang/rust-clippy#3837