Add FILTER_NEXT lint

Author: devonhollowood

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 86 lints included in this crate:
+There are 87 lints included in this crate:
 
 name                                                                                                     | default | meaning
 ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -28,6 +28,7 @@ name
 [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op)                                           | warn    | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`)
 [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop)           | warn    | for-looping with an explicit counter when `_.enumerate()` would do
 [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop)                 | warn    | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do
+[filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next)                               | warn    | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`
 [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp)                                   | warn    | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds)
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                               | warn    | using identity operations, e.g. `x + 0` or `y / 1`
 [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask)             | warn    | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2`
@@ -55,8 +56,8 @@ name
 [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal)                   | allow   | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
 [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options)     | warn    | nonsensical combination of options for opening a file
 [ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect)                                   | warn    | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result
-[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or)             | warn    | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`)
-[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else)   | warn    | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`)
+[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or)             | warn    | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`
+[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else)   | warn    | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`
 [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used)                 | allow   | using `Option.unwrap()`, which should at least get a better message using `expect()`
 [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing)         | deny    | out of bound constant indexing
 [panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params)                             | warn    | missing parameters in `panic!`

src/lib.rs

@@ -176,6 +176,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         matches::MATCH_BOOL,
         matches::MATCH_REF_PATS,
         matches::SINGLE_MATCH,
+        methods::FILTER_NEXT,
         methods::OK_EXPECT,
         methods::OPTION_MAP_UNWRAP_OR,
         methods::OPTION_MAP_UNWRAP_OR_ELSE,

src/methods.rs

@@ -6,7 +6,7 @@ use std::iter;
 use std::borrow::Cow;
 
 use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args,
-            walk_ptrs_ty_depth, walk_ptrs_ty};
+            match_trait_method, walk_ptrs_ty_depth, walk_ptrs_ty};
 use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
 use utils::MethodArgs;
 
@@ -135,7 +135,7 @@ declare_lint!(pub OK_EXPECT, Warn,
 /// **Example:** `x.map(|a| a + 1).unwrap_or(0)`
 declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn,
               "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as \
-               `map_or(a, f)`)");
+               `map_or(a, f)`");
 
 /// **What it does:** This lint `Warn`s on `_.map(_).unwrap_or_else(_)`.
 ///
@@ -146,7 +146,17 @@ declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn,
 /// **Example:** `x.map(|a| a + 1).unwrap_or_else(some_function)`
 declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn,
               "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \
-               `map_or_else(g, f)`)");
+               `map_or_else(g, f)`");
+
+/// **What it does:** This lint `Warn`s on `_.filter(_).next()`.
+///
+/// **Why is this bad?** Readability, this can be written more concisely as `_.find(_)`.
+///
+/// **Known problems:** None.
+///
+/// **Example:** `iter.filter(|x| x == 0).next()`
+declare_lint!(pub FILTER_NEXT, Warn,
+              "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`");
 
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
@@ -174,6 +184,9 @@ impl LateLintPass for MethodsPass {
             else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or_else"]) {
                 lint_map_unwrap_or_else(cx, expr, arglists[0], arglists[1]);
             }
+            else if let Some(arglists) = method_chain_args(expr, &["filter", "next"]) {
+                lint_filter_next(cx, expr, arglists[0]);
+            }
         }
     }
 
@@ -331,6 +344,24 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodAr
     }
 }
 
+#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec
+/// lint use of `filter().next() for Iterators`
+fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) {
+    // lint if caller of `.filter().next()` is an Iterator
+    if match_trait_method(cx, expr, &["core", "iter", "Iterator"]) {
+        let msg = "called `filter(p).next()` on an Iterator. This is more succinctly expressed by \
+                   calling `.find(p)` instead.";
+        let filter_snippet = snippet(cx, filter_args[1].span, "..");
+        if filter_snippet.lines().count() <= 1 { // add note if not multi-line
+            span_note_and_lint(cx, FILTER_NEXT, expr.span, msg, expr.span,
+                               &format!("replace this with `find({})`)", filter_snippet));
+        }
+        else {
+            span_lint(cx, FILTER_NEXT, expr.span, msg);
+        }
+    }
+}
+
 // Given a `Result<T, E>` type, return its error type (`E`)
 fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
     if !match_type(cx, ty, &RESULT_PATH) {

tests/compile-fail/methods.rs

@@ -83,6 +83,39 @@ fn option_methods() {
 
 }
 
+/// Struct to generate false positive for FILTER_NEXT lint
+struct FilterNextTest {
+    _foo: u32,
+}
+
+impl FilterNextTest {
+    fn filter(self) -> FilterNextTest {
+        self
+    }
+    fn next(self) -> FilterNextTest {
+        self
+    }
+}
+
+/// Checks implementation of FILTER_NEXT lint
+fn filter_next() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+
+    // check single-line case
+    let _ = v.iter().filter(|&x| *x < 0).next(); //~ERROR called `filter(p).next()` on an Iterator.
+                                                 //~| NOTE replace this
+
+    // check multi-line case
+    let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an Iterator.
+                                *x < 0
+                            }
+                   ).next();
+
+    // check that we don't lint if the caller is not an Iterator
+    let foo = FilterNextTest { _foo: 0 };
+    let _ = foo.filter().next();
+}
+
 fn main() {
     use std::io;