Auto merge of #5067 - JohnTitor:lint-skip-while-next, r=flip1995

Add `skip_while_next` lint

Fixes #4036

changelog: Add `skip_while_next` lint

CHANGELOG.md

@@ -1285,6 +1285,7 @@ Released 2018-09-13
 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
 [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
 [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
+[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
 [`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
 [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
 [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 347 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 348 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -645,6 +645,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::SEARCH_IS_SOME,
         &methods::SHOULD_IMPLEMENT_TRAIT,
         &methods::SINGLE_CHAR_PATTERN,
+        &methods::SKIP_WHILE_NEXT,
         &methods::STRING_EXTEND_CHARS,
         &methods::SUSPICIOUS_MAP,
         &methods::TEMPORARY_CSTRING_AS_PTR,
@@ -1223,6 +1224,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::SEARCH_IS_SOME),
         LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
         LintId::of(&methods::SINGLE_CHAR_PATTERN),
+        LintId::of(&methods::SKIP_WHILE_NEXT),
         LintId::of(&methods::STRING_EXTEND_CHARS),
         LintId::of(&methods::SUSPICIOUS_MAP),
         LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
@@ -1475,6 +1477,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::FLAT_MAP_IDENTITY),
         LintId::of(&methods::OPTION_AND_THEN_SOME),
         LintId::of(&methods::SEARCH_IS_SOME),
+        LintId::of(&methods::SKIP_WHILE_NEXT),
         LintId::of(&methods::SUSPICIOUS_MAP),
         LintId::of(&methods::UNNECESSARY_FILTER_MAP),
         LintId::of(&methods::USELESS_ASREF),

clippy_lints/src/methods/mod.rs

@@ -375,6 +375,29 @@ declare_clippy_lint! {
     "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.skip_while(condition).next()`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as
+    /// `_.find(!condition)`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # let vec = vec![1];
+    /// vec.iter().skip_while(|x| **x == 0).next();
+    /// ```
+    /// Could be written as
+    /// ```rust
+    /// # let vec = vec![1];
+    /// vec.iter().find(|x| **x != 0);
+    /// ```
+    pub SKIP_WHILE_NEXT,
+    complexity,
+    "using `skip_while(p).next()`, which is more succinctly expressed as `.find(!p)`"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of `_.map(_).flatten(_)`,
     ///
@@ -1193,6 +1216,7 @@ declare_lint_pass!(Methods => [
     SEARCH_IS_SOME,
     TEMPORARY_CSTRING_AS_PTR,
     FILTER_NEXT,
+    SKIP_WHILE_NEXT,
     FILTER_MAP,
     FILTER_MAP_NEXT,
     FLAT_MAP_IDENTITY,
@@ -1238,6 +1262,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             ["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
             ["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]),
             ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
+            ["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
             ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]),
@@ -2531,6 +2556,24 @@ fn lint_filter_next<'a, 'tcx>(
     }
 }
 
+/// lint use of `skip_while().next()` for `Iterators`
+fn lint_skip_while_next<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx hir::Expr<'_>,
+    _skip_while_args: &'tcx [hir::Expr<'_>],
+) {
+    // lint if caller of `.skip_while().next()` is an Iterator
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        span_help_and_lint(
+            cx,
+            SKIP_WHILE_NEXT,
+            expr.span,
+            "called `skip_while(p).next()` on an `Iterator`",
+            "this is more succinctly expressed by calling `.find(!p)` instead",
+        );
+    }
+}
+
 /// lint use of `filter().map()` for `Iterators`
 fn lint_filter_map<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,

doc/adding_lints.md

@@ -55,7 +55,7 @@ we want to check. The output of Clippy is compared against a `.stderr` file.
 Note that you don't have to create this file yourself, we'll get to
 generating the `.stderr` files further down.
 
-We start by opening the test file created at `tests/ui/foo_functions.rs`. 
+We start by opening the test file created at `tests/ui/foo_functions.rs`.
 
 Update the file with some examples to get started:
 
@@ -102,7 +102,7 @@ Once we are satisfied with the output, we need to run
 `tests/ui/update-all-references.sh` to update the `.stderr` file for our lint.
 Please note that, we should run `TESTNAME=foo_functions cargo uitest`
 every time before running `tests/ui/update-all-references.sh`.
-Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit 
+Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit
 our lint, we need to commit the generated `.stderr` files, too.
 
 ### Rustfix tests
@@ -133,7 +133,7 @@ With tests in place, let's have a look at implementing our lint now.
 
 ### Lint declaration
 
-Let's start by opening the new file created in the `clippy_lints` crate 
+Let's start by opening the new file created in the `clippy_lints` crate
 at `clippy_lints/src/foo_functions.rs`. That's the crate where all the
 lint code is. This file has already imported some initial things we will need:
 
@@ -178,7 +178,7 @@ state the thing that is being checked for and read well when used with
 * The last part should be a text that explains what exactly is wrong with the
   code
 
-The rest of this file contains an empty implementation for our lint pass, 
+The rest of this file contains an empty implementation for our lint pass,
 which in this case is `EarlyLintPass` and should look like this:
 
 ```rust
@@ -194,7 +194,7 @@ impl EarlyLintPass for FooFunctions {}
 Don't worry about the `name` method here. As long as it includes the name of the
 lint pass it should be fine.
 
-The new lint automation runs `update_lints`, which automates some things, but it 
+The new lint automation runs `update_lints`, which automates some things, but it
 doesn't automate everything. We will have to register our lint pass manually in
 the `register_plugins` function in `clippy_lints/src/lib.rs`:
 

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 347] = [
+pub const ALL_LINTS: [Lint; 348] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1862,6 +1862,13 @@ pub const ALL_LINTS: [Lint; 347] = [
         deprecation: None,
         module: "matches",
     },
+    Lint {
+        name: "skip_while_next",
+        group: "complexity",
+        desc: "using `skip_while(p).next()`, which is more succinctly expressed as `.find(!p)`",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "slow_vector_initialization",
         group: "perf",

tests/ui/auxiliary/option_helpers.rs

@@ -44,4 +44,8 @@ impl IteratorFalsePositives {
     pub fn skip(self, _: usize) -> IteratorFalsePositives {
         self
     }
+
+    pub fn skip_while(self) -> IteratorFalsePositives {
+        self
+    }
 }

tests/ui/skip_while_next.rs

@@ -0,0 +1,29 @@
+// aux-build:option_helpers.rs
+
+#![warn(clippy::skip_while_next)]
+#![allow(clippy::blacklisted_name)]
+
+extern crate option_helpers;
+use option_helpers::IteratorFalsePositives;
+
+#[rustfmt::skip]
+fn skip_while_next() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+
+    // Single-line case.
+    let _ = v.iter().skip_while(|&x| *x < 0).next();
+
+    // Multi-line case.
+    let _ = v.iter().skip_while(|&x| {
+                                *x < 0
+                            }
+                   ).next();
+
+    // Check that hat we don't lint if the caller is not an `Iterator`.
+    let foo = IteratorFalsePositives { foo: 0 };
+    let _ = foo.skip_while().next();
+}
+
+fn main() {
+    skip_while_next();
+}

tests/ui/skip_while_next.stderr

@@ -0,0 +1,23 @@
+error: called `skip_while(p).next()` on an `Iterator`
+  --> $DIR/skip_while_next.rs:14:13
+   |
+LL |     let _ = v.iter().skip_while(|&x| *x < 0).next();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::skip-while-next` implied by `-D warnings`
+   = help: this is more succinctly expressed by calling `.find(!p)` instead
+
+error: called `skip_while(p).next()` on an `Iterator`
+  --> $DIR/skip_while_next.rs:17:13
+   |
+LL |       let _ = v.iter().skip_while(|&x| {
+   |  _____________^
+LL | |                                 *x < 0
+LL | |                             }
+LL | |                    ).next();
+   | |___________________________^
+   |
+   = help: this is more succinctly expressed by calling `.find(!p)` instead
+
+error: aborting due to 2 previous errors
+