Lint for using hand-writing a fold with the same behaviour as any

clippy_lints/src/map_clone.rs

@@ -2,8 +2,8 @@ use rustc::lint::*;
 use rustc::hir::*;
 use rustc::ty;
 use syntax::ast;
-use utils::{is_adjusted, iter_input_pats, match_qpath, match_trait_method, match_type, paths, remove_blocks, snippet,
-            span_help_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth};
+use utils::{get_arg_name, is_adjusted, iter_input_pats, match_qpath, match_trait_method, match_type,
+            paths, remove_blocks, snippet, span_help_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth};
 
 /// **What it does:** Checks for mapping `clone()` over an iterator.
 ///
@@ -121,14 +121,6 @@ fn get_type_name(cx: &LateContext, expr: &Expr, arg: &Expr) -> Option<&'static s
     }
 }
 
-fn get_arg_name(pat: &Pat) -> Option<ast::Name> {
-    match pat.node {
-        PatKind::Binding(_, _, name, None) => Some(name.node),
-        PatKind::Ref(ref subpat, _) => get_arg_name(subpat),
-        _ => None,
-    }
-}
-
 fn only_derefs(cx: &LateContext, expr: &Expr, id: ast::Name) -> bool {
     match expr.node {
         ExprUnary(UnDeref, ref subexpr) if !is_adjusted(cx, subexpr) => only_derefs(cx, subexpr, id),

clippy_lints/src/methods.rs

@@ -9,10 +9,10 @@ use std::borrow::Cow;
 use std::fmt;
 use std::iter;
 use syntax::ast;
-use syntax::codemap::Span;
-use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, is_self, is_self_ty,
+use syntax::codemap::{Span, BytePos};
+use utils::{get_arg_name, get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, is_self, is_self_ty,
             iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method,
-            match_type, method_chain_args, return_ty, same_tys, single_segment_path, snippet, span_lint,
+            match_type, method_chain_args, return_ty, remove_blocks, same_tys, single_segment_path, snippet, span_lint,
             span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth};
 use utils::paths;
 use utils::sugg;
@@ -623,6 +623,23 @@ declare_lint! {
     "using `as_ref` where the types before and after the call are the same"
 }
 
+
+/// **What it does:** Checks for using `fold` to implement `any`.
+///
+/// **Why is this bad?** Readability.
+///
+/// **Known problems:** Changes semantics - the suggested replacement is short-circuiting.
+///
+/// **Example:**
+/// ```rust
+/// let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+/// ```
+declare_lint! {
+    pub FOLD_ANY,
+    Warn,
+    "using `fold` to emulate the behaviour of `any`"
+}
+
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
         lint_array!(
@@ -653,7 +670,8 @@ impl LintPass for Pass {
             GET_UNWRAP,
             STRING_EXTEND_CHARS,
             ITER_CLONED_COLLECT,
-            USELESS_ASREF
+            USELESS_ASREF,
+            FOLD_ANY
         )
     }
 }
@@ -717,6 +735,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                     lint_asref(cx, expr, "as_ref", arglists[0]);
                 } else if let Some(arglists) = method_chain_args(expr, &["as_mut"]) {
                     lint_asref(cx, expr, "as_mut", arglists[0]);
+                } else if let Some(arglists) = method_chain_args(expr, &["fold"]) {
+                    lint_fold_any(cx, expr, arglists[0]);
                 }
 
                 lint_or_fun_call(cx, expr, &method_call.name.as_str(), args);
@@ -1105,6 +1125,58 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir
     }
 }
 
+fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) {
+    // Check that this is a call to Iterator::fold rather than just some function called fold
+    if !match_trait_method(cx, expr, &paths::ITERATOR) {
+        return;
+    }
+
+    assert!(fold_args.len() == 3,
+        "Expected fold_args to have three entries - the receiver, the initial value and the closure");
+
+    if_chain! {
+        // Check if the initial value for the fold is the literal `false`
+        if let hir::ExprLit(ref lit) = fold_args[1].node;
+        if lit.node == ast::LitKind::Bool(false);
+
+        // Extract the body of the closure passed to fold
+        if let hir::ExprClosure(_, _, body_id, _, _) = fold_args[2].node;
+        let closure_body = cx.tcx.hir.body(body_id);
+        let closure_expr = remove_blocks(&closure_body.value);
+
+        // Extract the names of the two arguments to the closure
+        if let Some(first_arg_ident) = get_arg_name(&closure_body.arguments[0].pat);
+        if let Some(second_arg_ident) = get_arg_name(&closure_body.arguments[1].pat);
+
+        // Check if the closure body is of the form `acc || some_expr(x)`
+        if let hir::ExprBinary(ref bin_op, ref left_expr, ref right_expr) = closure_expr.node;
+        if bin_op.node == hir::BinOp_::BiOr;
+        if let hir::ExprPath(hir::QPath::Resolved(None, ref path)) = left_expr.node;
+        if path.segments.len() == 1 && &path.segments[0].name == &first_arg_ident;
+
+        then {
+            let right_source = snippet(cx, right_expr.span, "EXPR");
+
+            // Span containing `.fold(...)`
+            let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1));
+
+            span_lint_and_sugg(
+                cx,
+                FOLD_ANY,
+                fold_span,
+                // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f)
+                "this `.fold` can more succintly be expressed as `.any`",
+                "try",
+                format!(
+                    ".any(|{s}| {r})",
+                    s = second_arg_ident,
+                    r = right_source
+                )
+            );
+        }
+    }
+}
+
 fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir::Expr], is_mut: bool) {
     let mut_str = if is_mut { "_mut" } else { "" };
     let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() {

clippy_lints/src/utils/mod.rs

@@ -596,6 +596,20 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>(
     db.docs_link(lint);
 }
 
+/// Add a span lint with a suggestion on how to fix it.
+///
+/// These suggestions can be parsed by rustfix to allow it to automatically fix your code.
+/// In the example below, `help` is `"try"` and `sugg` is the suggested replacement `".any(|x| x > 2)"`.
+///
+/// <pre>
+/// error: This `.fold` can be more succinctly expressed as `.any`
+/// --> $DIR/methods.rs:390:13
+///     |
+/// 390 |     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+///     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)`
+///     |
+///     = note: `-D fold-any` implied by `-D warnings`
+/// </pre>
 pub fn span_lint_and_sugg<'a, 'tcx: 'a, T: LintContext<'tcx>>(
     cx: &'a T,
     lint: &'static Lint,
@@ -1034,3 +1048,11 @@ pub fn type_size<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> Option<u
 pub fn is_allowed(cx: &LateContext, lint: &'static Lint, id: NodeId) -> bool {
     cx.tcx.lint_level_at_node(lint, id).0 == Level::Allow
 }
+
+pub fn get_arg_name(pat: &Pat) -> Option<ast::Name> {
+    match pat.node {
+        PatKind::Binding(_, _, name, None) => Some(name.node),
+        PatKind::Ref(ref subpat, _) => get_arg_name(subpat),
+        _ => None,
+    }
+}

tests/ui/methods.rs

@@ -385,6 +385,26 @@ fn iter_skip_next() {
     let _ = foo.filter().skip(42).next();
 }
 
+/// Should trigger the `FOLD_ANY` lint
+fn fold_any() {
+    let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+}
+
+/// Should not trigger the `FOLD_ANY` lint as the initial value is not the literal `false`
+fn fold_any_ignores_initial_value_of_true() {
+    let _ = (0..3).fold(true, |acc, x| acc || x > 2);
+}
+
+/// Should not trigger the `FOLD_ANY` lint as the accumulator is not integer valued
+fn fold_any_ignores_non_boolean_accumalator() {
+    let _ = (0..3).fold(0, |acc, x| acc + if x > 2 { 1 } else { 0 });
+}
+
+/// Should trigger the `FOLD_ANY` lint, with the error span including exactly `.fold(...)`
+fn fold_any_span_for_multi_element_chain() {
+    let _ = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2);
+}
+
 #[allow(similar_names)]
 fn main() {
     let opt = Some(0);

tests/ui/methods.stderr

@@ -493,10 +493,24 @@ error: called `skip(x).next()` on an iterator. This is more succinctly expressed
 382 |     let _ = &some_vec[..].iter().skip(3).next();
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error: this `.fold` can more succintly be expressed as `.any`
+   --> $DIR/methods.rs:390:19
+    |
+390 |     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)`
+    |
+    = note: `-D fold-any` implied by `-D warnings`
+
+error: this `.fold` can more succintly be expressed as `.any`
+   --> $DIR/methods.rs:405:34
+    |
+405 |     let _ = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2);
+    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)`
+
 error: used unwrap() on an Option value. If you don't want to handle the None case gracefully, consider using expect() to provide a better panic message
-   --> $DIR/methods.rs:391:13
+   --> $DIR/methods.rs:411:13
     |
-391 |     let _ = opt.unwrap();
+411 |     let _ = opt.unwrap();
     |             ^^^^^^^^^^^^
     |
     = note: `-D option-unwrap-used` implied by `-D warnings`