First pass at linting for .any expressed as a .fold

diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs
index 64335f81a6347110eeb9c6901a6f037eb99b28c9..4277b4b15d3f853c707e989c03fe745f4dedb13c 100644 (file)
--- a/clippy_lints/src/methods.rs
+++ b/clippy_lints/src/methods.rs
@@ -12,7 +12,7 @@
 use syntax::codemap::Span;
 use utils::{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 @@
     "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,
+    "TODO"
+}
+
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
         lint_array!(
@@ -653,7 +670,8 @@ fn get_lints(&self) -> LintArray {
             GET_UNWRAP,
             STRING_EXTEND_CHARS,
             ITER_CLONED_COLLECT,
-            USELESS_ASREF
+            USELESS_ASREF,
+            FOLD_ANY
         )
     }
 }
@@ -717,6 +735,8 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) {
                     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,70 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir
     }
 }
 
+// DONOTMERGE: copy-pasted from map_clone
+fn get_arg_name(pat: &hir::Pat) -> Option<ast::Name> {
+    match pat.node {
+        hir::PatKind::Binding(_, _, name, None) => Some(name.node),
+        hir::PatKind::Ref(ref subpat, _) => get_arg_name(subpat),
+        _ => None,
+    }
+}
+
+fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) {
+    // DONOTMERGE: What if this is just some other method called fold?
+    assert!(fold_args.len() == 3,
+        "Expected fold_args to have three entries - the receiver, the initial value and the closure");
+
+    if let hir::ExprLit(ref lit) = fold_args[1].node {
+        if let ast::LitKind::Bool(ref b) = lit.node {
+            let initial_value = b.to_string();
+
+            if let hir::ExprClosure(_, ref decl, body_id, _, _) = fold_args[2].node {
+                let closure_body = cx.tcx.hir.body(body_id);
+                let closure_expr = remove_blocks(&closure_body.value);
+
+                let first_arg = &closure_body.arguments[0];
+                let arg_ident = get_arg_name(&first_arg.pat).unwrap();
+
+                let second_arg = &closure_body.arguments[1];
+                let second_arg_ident = get_arg_name(&second_arg.pat).unwrap();
+
+                if let hir::ExprBinary(ref bin_op, ref left_expr, ref right_expr) = closure_expr.node {
+                    if bin_op.node != hir::BinOp_::BiOr {
+                        return;
+                    }
+                    if let hir::ExprPath(hir::QPath::Resolved(None, ref path)) = left_expr.node {
+                        if path.segments.len() == 1 {
+                            let left_name =  &path.segments[0].name;
+                            let right_source = cx.sess().codemap().span_to_snippet(right_expr.span).unwrap();
+
+                            if left_name == &arg_ident {
+                                span_lint(
+                                    cx,
+                                    FOLD_ANY,
+                                    expr.span,
+                                    // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f)
+                                    // TODO: these have difference semantics - original code might be deliberately avoiding short-circuiting
+                                    &format!(
+                                        ".fold(false, |{f}, {s}| {f} || {r})) is more succinctly expressed as .any(|{s}| {r})",
+                                        f = arg_ident,
+                                        s = second_arg_ident,
+                                        r = right_source
+                                    ),
+                                );
+                            }
+                        }
+                    }
+                }
+            } else{
+                panic!("DONOTMERGE: can this happen?");
+            }
+        }
+    } else {
+        return;
+    }
+}
+
 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() {

diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
index c80f6acd06b9cedd6440bf4a7380068b1045e827..2dcc085add322bb9702bd0651c6a840375a91769 100644 (file)
--- a/tests/ui/methods.rs
+++ b/tests/ui/methods.rs
@@ -385,6 +385,11 @@ fn iter_skip_next() {
     let _ = foo.filter().skip(42).next();
 }
 
+/// Checks implementation of the `FOLD_ANY` lint
+fn fold_any() {
+    let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+}
+
 #[allow(similar_names)]
 fn main() {
     let opt = Some(0);

diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr
index 65d8b82da1400b87ce2500494de5029f08fe9d64..768fbd1df54569180fc25a576f220894855db1c9 100644 (file)
--- a/tests/ui/methods.stderr
+++ b/tests/ui/methods.stderr
@@ -493,10 +493,18 @@ error: called `skip(x).next()` on an iterator. This is more succinctly expressed
 382 |     let _ = &some_vec[..].iter().skip(3).next();
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+error: .fold(false, |acc, x| acc || x > 2)) is more succinctly expressed as .any(|x| x > 2)
+   --> $DIR/methods.rs:390:13
+    |
+390 |     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    |
+    = note: `-D fold-any` implied by `-D warnings`
+
 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:396:13
     |
-391 |     let _ = opt.unwrap();
+396 |     let _ = opt.unwrap();
     |             ^^^^^^^^^^^^
     |
     = note: `-D option-unwrap-used` implied by `-D warnings`