From 1cac693bc767e6c5648baeab1164feb4aea6ae30 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 19:12:44 +0000 Subject: [PATCH] Lint on folds implementing .all, .sum and .product --- clippy_lints/src/methods.rs | 93 ++++++++++++++++++++++++------------- tests/ui/methods.rs | 41 ++++++++++------ tests/ui/methods.stderr | 34 ++++++++++---- 3 files changed, 113 insertions(+), 55 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 5ff48a25b..6eb48a1bc 100644 --- a/clippy_lints/src/methods.rs +++ b/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! { - // 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); + fn check_fold_with_op( + cx: &LateContext, + fold_args: &[hir::Expr], + op: hir::BinOp_, + replacement_method_name: &str) { - // 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); + 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); - // 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 == op; - // 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; + // 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); - then { - let right_source = snippet(cx, right_expr.span, "EXPR"); + 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; - // Span containing `.fold(...)` - let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1)); + then { + let right_source = snippet(cx, right_expr.span, "EXPR"); - 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 - ) - ); + // 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 e.g. .any(|x| f(x)) if we can suggest .any(f) + "this `.fold` can be written more succinctly using another method", + "try", + 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) { diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index d50f8e35f..3ca77f744 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -385,26 +385,39 @@ fn iter_skip_next() { let _ = foo.filter().skip(42).next(); } -/// Should trigger the `FOLD_ANY` lint -fn fold_any() { +/// 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); + 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` -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() { +/// 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); +} + #[allow(similar_names)] fn main() { let opt = Some(0); diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 2c03e077d..1c8569e8d 100644 --- a/tests/ui/methods.stderr +++ b/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` - --> $DIR/methods.rs:390:19 +error: this `.fold` can be written more succinctly using another method + --> $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` - --> $DIR/methods.rs:405:34 +error: this `.fold` can be written more succinctly using another method + --> $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`