Merge pull request #524 from devonhollowood/iterator-suggestions

Iterator suggestions

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 88 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](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_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!`
@@ -68,6 +69,7 @@ name
 [redundant_pattern](https://github.com/Manishearth/rust-clippy/wiki#redundant_pattern)                   | warn    | using `name @ _` in a pattern
 [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used)                 | allow   | using `Result.unwrap()`, which might be better handled
 [reverse_range_loop](https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop)                 | warn    | Iterating over an empty range, such as `10..0` or `5..5`
+[search_is_some](https://github.com/Manishearth/rust-clippy/wiki#search_is_some)                         | warn    | using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()`
 [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse)                             | allow   | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1`
 [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same)                               | allow   | rebinding a name to itself, e.g. `let mut x = &mut x`
 [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated)                     | allow   | The name is re-bound without even using the original value

src/lib.rs

@@ -176,9 +176,11 @@ 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,
+        methods::SEARCH_IS_SOME,
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::STR_TO_STRING,
         methods::STRING_TO_STRING,

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,29 @@ 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)`");
+
+/// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or
+/// `rposition()`) followed by a call to `is_some()`.
+///
+/// **Why is this bad?** Readability, this can be written more concisely as `_.any(_)`.
+///
+/// **Known problems:** None.
+///
+/// **Example:** `iter.find(|x| x == 0).is_some()`
+declare_lint!(pub SEARCH_IS_SOME, Warn,
+              "using an iterator search followed by `is_some()`, which is more succinctly \
+               expressed as a call to `any()`");
 
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
@@ -174,6 +196,18 @@ 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]);
+            }
+            else if let Some(arglists) = method_chain_args(expr, &["find", "is_some"]) {
+                lint_search_is_some(cx, expr, "find", arglists[0], arglists[1]);
+            }
+            else if let Some(arglists) = method_chain_args(expr, &["position", "is_some"]) {
+                lint_search_is_some(cx, expr, "position", arglists[0], arglists[1]);
+            }
+            else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) {
+                lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]);
+            }
         }
     }
 
@@ -275,8 +309,8 @@ fn lint_ok_expect(cx: &LateContext, expr: &Expr, ok_args: &MethodArgs) {
 
 #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec
 /// lint use of `map().unwrap_or()` for `Option`s
-fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs,
+fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, map_args: &MethodArgs,
-                      map_args: &MethodArgs) {
+                      unwrap_args: &MethodArgs) {
     // lint if the caller of `map()` is an `Option`
     if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) {
         // lint message
@@ -293,7 +327,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs,
         if same_span && !multiline {
             span_note_and_lint(
                 cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span,
-                &format!("replace this with map_or({1}, {0})", map_snippet, unwrap_snippet)
+                &format!("replace `map({0}).unwrap_or({1})` with `map_or({1}, {0})`", map_snippet,
+                         unwrap_snippet)
             );
         }
         else if same_span && multiline {
@@ -304,8 +339,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs,
 
 #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec
 /// lint use of `map().unwrap_or_else()` for `Option`s
-fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs,
+fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, map_args: &MethodArgs,
-                           map_args: &MethodArgs) {
+                           unwrap_args: &MethodArgs) {
     // lint if the caller of `map()` is an `Option`
     if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) {
         // lint message
@@ -322,7 +357,8 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodAr
         if same_span && !multiline {
             span_note_and_lint(
                 cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span,
-                &format!("replace this with map_or_else({1}, {0})", map_snippet, unwrap_snippet)
+                &format!("replace `map({0}).unwrap_or_else({1})` with `with map_or_else({1}, {0})`",
+                          map_snippet, unwrap_snippet)
             );
         }
         else if same_span && multiline {
@@ -331,6 +367,44 @@ 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 `filter({0}).next()` with `find({0})`", filter_snippet));
+        }
+        else {
+            span_lint(cx, FILTER_NEXT, expr.span, msg);
+        }
+    }
+}
+
+#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec
+/// lint searching an Iterator followed by `is_some()`
+fn lint_search_is_some(cx: &LateContext, expr: &Expr, search_method: &str, search_args: &MethodArgs,
+                       is_some_args: &MethodArgs) {
+    // lint if caller of search is an Iterator
+    if match_trait_method(cx, &*is_some_args[0], &["core", "iter", "Iterator"]) {
+        let msg = format!("called `is_some()` after searching an iterator with {}. This is more \
+                           succinctly expressed by calling `any()`.", search_method);
+        let search_snippet = snippet(cx, search_args[1].span, "..");
+        if search_snippet.lines().count() <= 1 { // add note if not multi-line
+            span_note_and_lint(cx, SEARCH_IS_SOME, expr.span, &msg, expr.span,
+                &format!("replace `{0}({1}).is_some()` with `any({1})`", search_method,
+                         search_snippet));
+        }
+        else {
+            span_lint(cx, SEARCH_IS_SOME, 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

@@ -50,7 +50,7 @@ fn option_methods() {
     // Check OPTION_MAP_UNWRAP_OR
     // single line case
     let _ = opt.map(|x| x + 1) //~  ERROR called `map(f).unwrap_or(a)`
-                               //~| NOTE replace this
+                               //~| NOTE replace `map(|x| x + 1).unwrap_or(0)`
                .unwrap_or(0); // should lint even though this call is on a separate line
     // multi line cases
     let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or(a)`
@@ -67,7 +67,7 @@ fn option_methods() {
     // Check OPTION_MAP_UNWRAP_OR_ELSE
     // single line case
     let _ = opt.map(|x| x + 1) //~  ERROR called `map(f).unwrap_or_else(g)`
-                               //~| NOTE replace this
+                               //~| NOTE replace `map(|x| x + 1).unwrap_or_else(|| 0)`
                .unwrap_or_else(|| 0); // should lint even though this call is on a separate line
     // multi line cases
     let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or_else(g)`
@@ -83,6 +83,98 @@ fn option_methods() {
 
 }
 
+/// Struct to generate false positive for Iterator-based lints
+#[derive(Copy, Clone)]
+struct IteratorFalsePositives {
+    foo: u32,
+}
+
+impl IteratorFalsePositives {
+    fn filter(self) -> IteratorFalsePositives {
+        self
+    }
+
+    fn next(self) -> IteratorFalsePositives {
+        self
+    }
+
+    fn find(self) -> Option<u32> {
+        Some(self.foo)
+    }
+
+    fn position(self) -> Option<u32> {
+        Some(self.foo)
+    }
+
+    fn rposition(self) -> Option<u32> {
+        Some(self.foo)
+    }
+}
+
+/// 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 `filter(|&x| *x < 0).next()`
+
+    // 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 = IteratorFalsePositives { foo: 0 };
+    let _ = foo.filter().next();
+}
+
+/// Checks implementation of SEARCH_IS_SOME lint
+fn search_is_some() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+
+    // check `find().is_some()`, single-line
+    let _ = v.iter().find(|&x| *x < 0).is_some();
+    //~^ ERROR called `is_some()` after searching
+    //~| NOTE replace `find(|&x| *x < 0).is_some()`
+
+    // check `find().is_some()`, multi-line
+    let _ = v.iter().find(|&x| { //~ERROR called `is_some()` after searching
+                              *x < 0
+                          }
+                   ).is_some();
+
+    // check `position().is_some()`, single-line
+    let _ = v.iter().position(|&x| x < 0).is_some();
+    //~^ ERROR called `is_some()` after searching
+    //~| NOTE replace `position(|&x| x < 0).is_some()`
+
+    // check `position().is_some()`, multi-line
+    let _ = v.iter().position(|&x| { //~ERROR called `is_some()` after searching
+                                  x < 0
+                              }
+                   ).is_some();
+
+    // check `rposition().is_some()`, single-line
+    let _ = v.iter().rposition(|&x| x < 0).is_some();
+    //~^ ERROR called `is_some()` after searching
+    //~| NOTE replace `rposition(|&x| x < 0).is_some()`
+
+    // check `rposition().is_some()`, multi-line
+    let _ = v.iter().rposition(|&x| { //~ERROR called `is_some()` after searching
+                                   x < 0
+                               }
+                   ).is_some();
+
+    // check that we don't lint if the caller is not an Iterator
+    let foo = IteratorFalsePositives { foo: 0 };
+    let _ = foo.find().is_some();
+    let _ = foo.position().is_some();
+    let _ = foo.rposition().is_some();
+}
+
 fn main() {
     use std::io;