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

fix: Make auto-fix note work with clippy #11399

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Nov 21, 2022

Someone pointed out that the suggestion to run cargo --fix did not work properly for cargo clippy. This makes sure the correct command to run is output when running under cargo clippy.

This is done by checking if the RUSTC_WORKSPACE_WRAPPER environment variable is set, since clippy sets this every run. Since other things can use RUSTC_WORKSPACE_WRAPPER, we look for a wrapper named clippy-driver.

Since clippy might not be available everywhere cargo is tested, this is tested using a rustc wrapper.

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2022

r? @ehuss

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2022
// check if `RUSTC_WORKSPACE_WRAPPER` is set. `clippy` sets
// this when being ran so it can partially be identified
// when this is set.
let command = if rustc_workspace_wrapper.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately clippy isn't the only tool which sets RUSTC_WORKSPACE_WRAPPER. I think some other approach will be needed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about that, I was hoping it would be fine since it was used before. The only thing I can think of is using CLIPPY_ARGS, which appears to pass the test I wrote. I will update it to that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion, it was decided that RUSTC_WORKSPACE_WRAPPER would be better as it is already agreed upon by clippy and cargo. Since other things can use RUSTC_WORKSPACE_WRAPPER, we look for a wrapper named clippy-driver.

@Muscraft Muscraft force-pushed the autofix-for-clippy branch 2 times, most recently from e8e9b8b to 12f0c8a Compare November 21, 2022 21:36
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks goog to me. Thanks!

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just some tiny not important stuff.

Comment on lines 1255 to 1297
let command = match rustc_workspace_wrapper {
#[cfg(windows)]
Some(wrapper) if wrapper.ends_with("clippy-driver.exe") => {
"cargo clippy --fix"
}
#[cfg(not(windows))]
Some(wrapper) if wrapper.ends_with("clippy-driver") => "cargo clippy --fix",
_ => "cargo fix",
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Suggested change
let command = match rustc_workspace_wrapper {
#[cfg(windows)]
Some(wrapper) if wrapper.ends_with("clippy-driver.exe") => {
"cargo clippy --fix"
}
#[cfg(not(windows))]
Some(wrapper) if wrapper.ends_with("clippy-driver") => "cargo clippy --fix",
_ => "cargo fix",
};
let clippy = std::ffi::OsStr::new("clippy-driver");
let command = match rustc_workspace_wrapper.as_ref().and_then(|x| x.file_stem())
{
Some(wrapper) if wrapper == clippy => "cargo clippy --fix",
_ => "cargo fix",
};

Comment on lines 1284 to 1322
" (run `{} --{}` to apply {})",
command, args, suggestions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit)

Suggested change
" (run `{} --{}` to apply {})",
command, args, suggestions
" (run `{command} --{args}` to apply {suggestions})"

@weihanglo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 15, 2022

📌 Commit 33c3208 has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 Dec 15, 2022
@bors
Copy link
Contributor

bors commented Dec 15, 2022

⌛ Testing commit 33c3208 with merge 8607003...

@bors
Copy link
Contributor

bors commented Dec 15, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 8607003 to master...

@bors bors merged commit 8607003 into rust-lang:master Dec 15, 2022
@Muscraft Muscraft deleted the autofix-for-clippy branch December 15, 2022 18:20
weihanglo added a commit to weihanglo/rust that referenced this pull request Dec 18, 2022
11 commits in cc0a320879c17207bbfb96b5d778e28a2c62030d..c994a4a638370bc7e0ffcbb0e2865afdfa7d4415
2022-12-14 14:46:57 +0000 to 2022-12-18 21:50:58 +0000
- Fix examples of proc-macro crates being scraped for examples (rust-lang/cargo#11497)
- Enable triagebot's relabel functionality (rust-lang/cargo#11498)
- Revert "temporarily disable test `lto::test_profile`" (rust-lang/cargo#11495)
- Bump to 0.69.0, update changelog (rust-lang/cargo#11493)
- Fix typo (rust-lang/cargo#11491)
- Display CPU info in CI (rust-lang/cargo#11488)
- Fix collision_doc_profile test error (rust-lang/cargo#11489)
- fix: Make auto-fix note work with `clippy` (rust-lang/cargo#11399)
- fix(add): use the possessive in error message (rust-lang/cargo#11483)
- Document home crate in contrib docs (rust-lang/cargo#11481)
- Split up registry documentation into multiple sections (rust-lang/cargo#11480)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2022
Update cargo

11 commits in cc0a320879c17207bbfb96b5d778e28a2c62030d..c994a4a638370bc7e0ffcbb0e2865afdfa7d4415 2022-12-14 14:46:57 +0000 to 2022-12-18 21:50:58 +0000
- Fix examples of proc-macro crates being scraped for examples (rust-lang/cargo#11497)
- Enable triagebot's relabel functionality (rust-lang/cargo#11498)
- Revert "temporarily disable test `lto::test_profile`" (rust-lang/cargo#11495)
- Bump to 0.69.0, update changelog (rust-lang/cargo#11493)
- Fix typo (rust-lang/cargo#11491)
- Display CPU info in CI (rust-lang/cargo#11488)
- Fix collision_doc_profile test error (rust-lang/cargo#11489)
- fix: Make auto-fix note work with `clippy` (rust-lang/cargo#11399)
- fix(add): use the possessive in error message (rust-lang/cargo#11483)
- Document home crate in contrib docs (rust-lang/cargo#11481)
- Split up registry documentation into multiple sections (rust-lang/cargo#11480)

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Dec 30, 2022
bors added a commit that referenced this pull request Jan 22, 2023
feat: stabilize auto fix note

A note that some warnings could be fixed by running a `cargo fix` command was added in #10989 and made to work with `clippy` in #11399. It has only been turned on for `nightly` builds so far; this PR would make it show on `stable`.

The original motivation for making this note `nightly` only, was to [allow for iteration](#10976 (comment)) on the message output. There has yet to be any feedback on the message format in the time that it has been on `nightly`. This was brought up in a recent cargo team meeting and it was thought that we should move forward with showing this on `stable`.

close #10976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants