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

New Lint: Iter nth zero #4966

Merged
merged 2 commits into from
Jan 4, 2020
Merged

New Lint: Iter nth zero #4966

merged 2 commits into from
Jan 4, 2020

Conversation

bradsherman
Copy link
Contributor

Check for the use of iter.nth(0) and encourage iter.next() instead as it is more readable

changelog: add new lint when iter.nth(0) is used

Fixes #4957

@bradsherman bradsherman force-pushed the iter-nth-zero branch 2 times, most recently from 315667e to feaf179 Compare December 28, 2019 22:57
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.

@krishna-veerareddy is correct with his review, that this will lint on every method called nth.

For lints like this, we already have infrastructure. You can just add ["nth", ..] below this statements:

["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false),
["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true),

and implement a function, that checks for the lint. To verify that this function is from the Iterator trait, you can add a check like this:

// lint if caller of skip is an Iterator
if match_trait_method(cx, expr, &paths::ITERATOR) {


So steps to more easily implement the lint and addressing all review comments:

  1. add the declare_clippy_lint to `methods/mod.rs
  2. add ["nth", ..] to the above mentioned match statement
  3. add a new module for this lint in methods and implement your check function, with the above mentioned Iterator check and the check for the second (1) argument for constant 0.

@JohnTitor JohnTitor added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 29, 2019
@bradsherman bradsherman requested a review from flip1995 December 30, 2019 00:25
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 overall

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
tests/ui/iter_nth_zero.rs Show resolved Hide resolved
tests/ui/iter_nth_zero.rs Outdated Show resolved Hide resolved
@bradsherman
Copy link
Contributor Author

bradsherman commented Dec 30, 2019

I made the requested changes, however after rebasing on master sometime yesterday I could not compile clippy_lints:

error[E0107]: wrong number of lifetime arguments: expected 0, found 1
  --> clippy_lints/src/zero_div_zero.rs:29:75
   |
29 |     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
   |                                                                           ^^ unexpected lifetime argument

This happens in ~900 other places as well.

@bradsherman bradsherman requested a review from flip1995 December 30, 2019 15:15
@bradsherman
Copy link
Contributor Author

I have replicated the issue even when cloning a new repo from rust-lang:rust-clippy. Here's my rustc version:

rustc 1.42.0-nightly (0de96d37f 2019-12-19)

I'm only pointing this out because I can't build locally in order ensure all the tests pass

@JohnTitor
Copy link
Member

@bradsherman It's because there is a difference between nightly and master rustc. Clippy is built for master, not nightly. You can use rustup-toolchain-install-master.

ref. https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#fixing-build-failures-caused-by-rust

@bradsherman
Copy link
Contributor Author

@JohnTitor thanks, that fixed it. Sorry I missed that in the docs.

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.

Thanks! LGTM, just a NIT with the documentation

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs 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.

Thanks! r=me with CI green

- Encourage iter.next() rather than iter.nth(0), which is less readable
@flip1995
Copy link
Member

flip1995 commented Jan 4, 2020

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Jan 4, 2020

📌 Commit ab5ff03 has been approved by flip1995

bors added a commit that referenced this pull request Jan 4, 2020
New Lint: Iter nth zero

Check for the use of `iter.nth(0)` and encourage `iter.next()` instead as it is more readable

changelog: add new lint when `iter.nth(0)` is used

Fixes #4957
@bors
Copy link
Contributor

bors commented Jan 4, 2020

⌛ Testing commit ab5ff03 with merge d9d2013...

@bors
Copy link
Contributor

bors commented Jan 4, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing d9d2013 to master...

@bors bors merged commit ab5ff03 into rust-lang:master Jan 4, 2020
@bradsherman
Copy link
Contributor Author

thanks all for helping with my first open source contribution! 😃🎉

@bradsherman bradsherman deleted the iter-nth-zero branch January 4, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: iter_nth_zero
5 participants