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

Makes clippy-driver check for --sysroot in arg files #12203

Merged

Conversation

daivinhtran
Copy link
Contributor

@daivinhtran daivinhtran commented Jan 26, 2024

Fixes #12201


changelog: none

cc: @UebelAndre @illicitonion

@rustbot
Copy link
Collaborator

rustbot commented Jan 26, 2024

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@daivinhtran
Copy link
Contributor Author

@vorner Let me know if you have any questions. I hope to land this change to unblock rules_rust's users.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Great catch! Please add a test case for this to the .github/driver.sh script. Don't create an actual arg file in the repo please, but just create one in the script with echo "--sysroot bar" > arg_file.txt.

src/driver.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

r? @flip1995

@rustbot rustbot assigned flip1995 and unassigned xFrednet Jan 26, 2024
@daivinhtran
Copy link
Contributor Author

echo "--sysroot bar" > arg_file.txt

@flip1995 Thanks for the review. I addressed your comment. PTAL.

@daivinhtran daivinhtran changed the title Makes clippy-driver to check for --sysroot in arg files Makes clippy-driver check for --sysroot in arg files Jan 26, 2024
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

You have a few more warnings in your code (unused imports). Also make sure to run cargo dev dogfood locally and make sure that succeeds. That command runs Clippy on Clippy itself.

.github/driver.sh Show resolved Hide resolved
src/driver.rs Outdated Show resolved Hide resolved
@daivinhtran daivinhtran force-pushed the fix-clippy-driver-to-accept-param-file branch from 117b052 to aa3490a Compare January 26, 2024 16:53
arg_file.txt Outdated Show resolved Hide resolved
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

`cargo dev dogfood` shows these errors:
error: unneeded `return` statement
   --> src/driver.rs:210:13
    |
210 |             return false;
    |             ^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
    = note: `-D clippy::needless-return` implied by `-D clippy::all`
    = help: to override `-D clippy::all` add `#[allow(clippy::needless_return)]`
help: remove `return`
    |
210 -             return false;
210 +             false
    |

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/driver.rs:194:26
    |
194 |             if arg_value(&args, "--sysroot", |_| true).is_some() {
    |                          ^^^^^ help: change this to: `args`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `-D clippy::needless-borrow` implied by `-D clippy::all`
    = help: to override `-D clippy::all` add `#[allow(clippy::needless_borrow)]`

error: single-character string constant used as pattern
   --> src/driver.rs:201:63
    |
201 |                 if let Some(arg_file_path) = arg.strip_prefix("@") {
    |                                                               ^^^ help: consider using a `char`: `'@'`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
    = note: `-D clippy::single-char-pattern` implied by `-D clippy::all`
    = help: to override `-D clippy::all` add `#[allow(clippy::single_char_pattern)]`

error: redundant closure
   --> src/driver.rs:203:80
    |
203 |                         let split_arg_file: Vec<String> = arg_file.lines().map(|s| s.to_string()).collect();
    |                                                                                ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::string::ToString::to_string`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls
    = note: `-D clippy::redundant-closure-for-method-calls` implied by `-D clippy::pedantic`
    = help: to override `-D clippy::pedantic` add `#[allow(clippy::redundant_closure_for_method_calls)]`

Please fix them and this should be ready to go.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM!

Can you please squash your commits into one? After that, this is ready to go.

@daivinhtran
Copy link
Contributor Author

@daivinhtran daivinhtran force-pushed the fix-clippy-driver-to-accept-param-file branch from 166e671 to c7722c4 Compare January 29, 2024 15:50
@daivinhtran daivinhtran force-pushed the fix-clippy-driver-to-accept-param-file branch from c7722c4 to 73706e8 Compare January 29, 2024 15:53
@daivinhtran
Copy link
Contributor Author

@flip1995 PTAL

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jan 29, 2024

📌 Commit 73706e8 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 29, 2024

⌛ Testing commit 73706e8 with merge 1156375...

@bors
Copy link
Contributor

bors commented Jan 29, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 1156375 to master...

@bors bors merged commit 1156375 into rust-lang:master Jan 29, 2024
5 checks passed
nmattia added a commit to dfinity/ic that referenced this pull request Jun 13, 2024
This removes a workaround that was necessary in the past to work around
an issue in clippy. The issue is now fixed in clippy, and our clippy
version is recent enough to include the fix.

See also:
* bazelbuild/rules_rust#2277
* rust-lang/rust-clippy#12203
github-merge-queue bot pushed a commit to dfinity/ic that referenced this pull request Jun 13, 2024
This removes a workaround that was necessary in the past to work around
an issue in clippy. The issue is now fixed in clippy, and our clippy
version is recent enough to include the fix.

See also:
* bazelbuild/rules_rust#2277
* rust-lang/rust-clippy#12203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy-driver passes --sysroot more than once
5 participants