Lint on folds implementing .all, .sum and .product

clippy_lints/src/methods.rs

@@ -1134,47 +1134,74 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) {
     assert!(fold_args.len() == 3,
         "Expected fold_args to have three entries - the receiver, the initial value and the closure");
 
-    if_chain! {
+    fn check_fold_with_op(
-        // Check if the initial value for the fold is the literal `false`
+        cx: &LateContext,
-        if let hir::ExprLit(ref lit) = fold_args[1].node;
+        fold_args: &[hir::Expr],
-        if lit.node == ast::LitKind::Bool(false);
+        op: hir::BinOp_,
+        replacement_method_name: &str) {
 
-        // Extract the body of the closure passed to fold
+        if_chain! {
-        if let hir::ExprClosure(_, _, body_id, _, _) = fold_args[2].node;
+            // Extract the body of the closure passed to fold
-        let closure_body = cx.tcx.hir.body(body_id);
+            if let hir::ExprClosure(_, _, body_id, _, _) = fold_args[2].node;
-        let closure_expr = remove_blocks(&closure_body.value);
+            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
+            // Check if the closure body is of the form `acc <op> some_expr(x)`
-        if let Some(first_arg_ident) = get_arg_name(&closure_body.arguments[0].pat);
+            if let hir::ExprBinary(ref bin_op, ref left_expr, ref right_expr) = closure_expr.node;
-        if let Some(second_arg_ident) = get_arg_name(&closure_body.arguments[1].pat);
+            if bin_op.node == op;
 
-        // Check if the closure body is of the form `acc || some_expr(x)`
+            // Extract the names of the two arguments to the closure
-        if let hir::ExprBinary(ref bin_op, ref left_expr, ref right_expr) = closure_expr.node;
+            if let Some(first_arg_ident) = get_arg_name(&closure_body.arguments[0].pat);
-        if bin_op.node == hir::BinOp_::BiOr;
+            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 {
+            if let hir::ExprPath(hir::QPath::Resolved(None, ref path)) = left_expr.node;
-            let right_source = snippet(cx, right_expr.span, "EXPR");
+            if path.segments.len() == 1 && &path.segments[0].name == &first_arg_ident;
 
-            // Span containing `.fold(...)`
+            then {
-            let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1));
+                let right_source = snippet(cx, right_expr.span, "EXPR");
 
-            span_lint_and_sugg(
+                // Span containing `.fold(...)`
-                cx,
+                let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1));
-                FOLD_ANY,
+
-                fold_span,
+                span_lint_and_sugg(
-                // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f)
+                    cx,
-                "this `.fold` can more succintly be expressed as `.any`",
+                    FOLD_ANY,
-                "try",
+                    fold_span,
-                format!(
+                    // TODO: don't suggest e.g. .any(|x| f(x)) if we can suggest .any(f)
-                    ".any(|{s}| {r})",
+                    "this `.fold` can be written more succinctly using another method",
-                    s = second_arg_ident,
+                    "try",
-                    r = right_source
+                    format!(
-                )
+                        ".{replacement}(|{s}| {r})",
-            );
+                        replacement = replacement_method_name,
+                        s = second_arg_ident,
+                        r = right_source
+                    )
+                );
+            }
         }
     }
+
+    // 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"
+                ),
+                ast::LitKind::Bool(true) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiAnd, "all"
+                ),
+                ast::LitKind::Int(0, _) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiAdd, "sum"
+                ),
+                ast::LitKind::Int(1, _) => check_fold_with_op(
+                    cx, fold_args, hir::BinOp_::BiMul, "product"
+                ),
+                _ => return
+            }
+        }
+        _ => return
+    };
 }
 
 fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir::Expr], is_mut: bool) {

tests/ui/methods.rs

@@ -385,26 +385,39 @@ fn iter_skip_next() {
     let _ = foo.filter().skip(42).next();
 }
 
-/// Should trigger the `FOLD_ANY` lint
+/// Calls which should trigger the `UNNECESSARY_FOLD` lint
-fn fold_any() {
+fn unnecessary_fold() {
+    // Can be replaced by .any
     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+    let _ = (0..3).fold(false, |acc, x| x > 2 || acc);
+
+    // Can be replaced by .all
+    let _ = (0..3).fold(true, |acc, x| acc && x > 2);
+    let _ = (0..3).fold(true, |acc, x| x > 2 && acc);
+
+    // Can be replaced by .sum
+    let _ = (0..3).fold(0, |acc, x| acc + x);
+    let _ = (0..3).fold(0, |acc, x| x + acc);
+
+    // Can be replaced by .product
+    let _ = (0..3).fold(1, |acc, x| acc * x);
+    let _ = (0..3).fold(1, |acc, x| x * acc);
 }
 
-/// Should not trigger the `FOLD_ANY` lint as the initial value is not the literal `false`
+/// Should trigger the `UNNECESSARY_FOLD` lint, with an error span including exactly `.fold(...)`
-fn fold_any_ignores_initial_value_of_true() {
+fn unnecessary_fold_span_for_multi_element_chain() {
-    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);
 }
 
+/// 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);
+}
+
 #[allow(similar_names)]
 fn main() {
     let opt = Some(0);

tests/ui/methods.stderr

@@ -493,24 +493,42 @@ 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`
+error: this `.fold` can be written more succinctly using another method
-   --> $DIR/methods.rs:390:19
+   --> $DIR/methods.rs:391:19
     |
-390 |     let _ = (0..3).fold(false, |acc, x| acc || x > 2);
+391 |     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`
+error: this `.fold` can be written more succinctly using another method
-   --> $DIR/methods.rs:405:34
+   --> $DIR/methods.rs:395:19
     |
-405 |     let _ = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2);
+395 |     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:399:19
+    |
+399 |     let _ = (0..3).fold(0, |acc, x| acc + x);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.sum(|x| x)`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:403:19
+    |
+403 |     let _ = (0..3).fold(1, |acc, x| acc * x);
+    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.product(|x| x)`
+
+error: this `.fold` can be written more succinctly using another method
+   --> $DIR/methods.rs:409:34
+    |
+409 |     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:411:13
+   --> $DIR/methods.rs:424:13
     |
-411 |     let _ = opt.unwrap();
+424 |     let _ = opt.unwrap();
     |             ^^^^^^^^^^^^
     |
     = note: `-D option-unwrap-used` implied by `-D warnings`