Merge pull request #2350 from theotherphil/fold_any

oli-obk · web-flow · commit e6428873cb0e · 2018-01-19T13:31:00.000+01:00
Lint for using hand-writing a fold with the same behaviour as any
diff --git a/clippy_lints/src/map_clone.rs b/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.
 ///
@@ -123,14 +123,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),
diff --git a/clippy_lints/src/methods.rs b/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;
@@ -622,6 +622,29 @@ declare_lint! {
     "using `as_ref` where the types before and after the call are the same"
 }
 
+
+/// **What it does:** Checks for using `fold` when a more succinct alternative exists.
+/// Specifically, this checks for `fold`s which could be replaced by `any`, `all`,
+/// `sum` or `product`.
+///
+/// **Why is this bad?** Readability.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+/// ```
+/// This could be written as:
+/// ```rust
+/// let _ = (0..3).any(|x| x > 2);
+/// ```
+declare_lint! {
+    pub UNNECESSARY_FOLD,
+    Warn,
+    "using `fold` when a more succinct alternative exists"
+}
+
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
         lint_array!(
@@ -652,7 +675,8 @@ impl LintPass for Pass {
             GET_UNWRAP,
             STRING_EXTEND_CHARS,
             ITER_CLONED_COLLECT,
-            USELESS_ASREF
+            USELESS_ASREF,
+            UNNECESSARY_FOLD
         )
     }
 }
@@ -716,6 +740,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_unnecessary_fold(cx, expr, arglists[0]);
                 }
 
                 lint_or_fun_call(cx, expr, &method_call.name.as_str(), args);
@@ -1106,6 +1132,93 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir
     }
 }
 
+fn lint_unnecessary_fold(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");
+
+    fn check_fold_with_op(
+        cx: &LateContext,
+        fold_args: &[hir::Expr],
+        op: hir::BinOp_,
+        replacement_method_name: &str,
+        replacement_has_args: bool) {
+
+        if_chain! {
+            // 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);
+
+            // Check if the closure body is of the form `acc <op> some_expr(x)`
+            if let hir::ExprBinary(ref bin_op, ref left_expr, ref right_expr) = closure_expr.node;
+            if bin_op.node == op;
+
+            // 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);
+
+            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 {
+                // Span containing `.fold(...)`
+                let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1));
+
+                let sugg = if replacement_has_args {
+                    format!(
+                        ".{replacement}(|{s}| {r})",
+                        replacement = replacement_method_name,
+                        s = second_arg_ident,
+                        r = snippet(cx, right_expr.span, "EXPR"),
+                    )
+                } else {
+                    format!(
+                        ".{replacement}()",
+                        replacement = replacement_method_name,
+                    )
+                };
+
+                span_lint_and_sugg(
+                    cx,
+                    UNNECESSARY_FOLD,
+                    fold_span,
+                    // TODO #2371 don't suggest e.g. .any(|x| f(x)) if we can suggest .any(f)
+                    "this `.fold` can be written more succinctly using another method",
+                    "try",
+                    sugg,
+                );
+            }
+        }
+    }
+
+    // Check if the first argument to .fold is a suitable literal
+    match fold_args[1].node {
+        hir::ExprLit(ref lit) => {
+            match lit.node {
+                ast::LitKind::Bool(false) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiOr, "any", true
+                ),
+                ast::LitKind::Bool(true) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiAnd, "all", true
+                ),
+                ast::LitKind::Int(0, _) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiAdd, "sum", false
+                ),
+                ast::LitKind::Int(1, _) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiMul, "product", false
+                ),
+                _ => return
+            }
+        }
+        _ => 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/clippy_lints/src/utils/mod.rs b/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)"`.
+///
+/// ```
+/// 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`
+/// ```
 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,
+    }
+}
diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
@@ -385,6 +385,42 @@ fn iter_skip_next() {
     let _ = foo.filter().skip(42).next();
 }
 
+/// Calls which should trigger the `UNNECESSARY_FOLD` lint
+fn unnecessary_fold() {
+    // Can be replaced by .any
+    let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+    // Can be replaced by .all
+    let _ = (0..3).fold(true, |acc, x| acc && x > 2);
+    // Can be replaced by .sum
+    let _ = (0..3).fold(0, |acc, x| acc + x);
+    // Can be replaced by .product
+    let _ = (0..3).fold(1, |acc, x| acc * x);
+}
+
+/// Should trigger the `UNNECESSARY_FOLD` lint, with an error span including exactly `.fold(...)`
+fn unnecessary_fold_span_for_multi_element_chain() {
+    let _ = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2);
+}
+
+/// Calls which should not trigger the `UNNECESSARY_FOLD` lint
+fn unnecessary_fold_should_ignore() {
+    let _ = (0..3).fold(true, |acc, x| acc || x > 2);
+    let _ = (0..3).fold(false, |acc, x| acc && x > 2);
+    let _ = (0..3).fold(1, |acc, x| acc + x);
+    let _ = (0..3).fold(0, |acc, x| acc * x);
+    let _ = (0..3).fold(0, |acc, x| 1 + acc + x);
+
+    // We only match against an accumulator on the left
+    // hand side. We could lint for .sum and .product when
+    // it's on the right, but don't for now (and this wouldn't
+    // be valid if we extended the lint to cover arbitrary numeric
+    // types).
+    let _ = (0..3).fold(false, |acc, x| x > 2 || acc);
+    let _ = (0..3).fold(true, |acc, x| x > 2 && acc);
+    let _ = (0..3).fold(0, |acc, x| x + acc);
+    let _ = (0..3).fold(1, |acc, x| x * acc);
+}
+
 #[allow(similar_names)]
 fn main() {
     let opt = Some(0);
diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr
@@ -493,13 +493,45 @@ 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 be written more succinctly using another method
+   --> $DIR/methods.rs:391:19
+    |
+391 |     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)`
+    |
+    = note: `-D unnecessary-fold` implied by `-D warnings`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:393:19
+    |
+393 |     let _ = (0..3).fold(true, |acc, x| acc && x > 2);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.all(|x| x > 2)`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:395:19
+    |
+395 |     let _ = (0..3).fold(0, |acc, x| acc + x);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.sum()`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:397:19
+    |
+397 |     let _ = (0..3).fold(1, |acc, x| acc * x);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.product()`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:402:34
+    |
+402 |     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:427:13
     |
-391 |     let _ = opt.unwrap();
+427 |     let _ = opt.unwrap();
     |             ^^^^^^^^^^^^
     |
     = note: `-D option-unwrap-used` implied by `-D warnings`
 
-error: aborting due to 66 previous errors
+error: aborting due to 71 previous errors
 
