From 214d4991038d0fef99efbb0fc8899bc7f003d7c1 Mon Sep 17 00:00:00 2001 From: Jeremy Stucki Date: Wed, 14 Aug 2019 19:34:50 +0200 Subject: [PATCH 1/4] Add multiline test --- tests/ui/unnecessary_fold.fixed | 8 ++++++++ tests/ui/unnecessary_fold.rs | 8 ++++++++ tests/ui/unnecessary_fold.stderr | 8 +++++++- 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/ui/unnecessary_fold.fixed b/tests/ui/unnecessary_fold.fixed index 5f12d72a76ae..52300a3b6406 100644 --- a/tests/ui/unnecessary_fold.fixed +++ b/tests/ui/unnecessary_fold.fixed @@ -41,4 +41,12 @@ fn unnecessary_fold_should_ignore() { let _ = [(0..2), (0..3)].iter().fold(1, |a, b| a * b.len()); } +/// Should lint only the line containing the fold +fn unnecessary_fold_over_multiple_lines() { + let _ = (0..3) + .map(|x| x + 1) + .filter(|x| x % 2 == 0) + .any(|x| x > 2); +} + fn main() {} diff --git a/tests/ui/unnecessary_fold.rs b/tests/ui/unnecessary_fold.rs index ae667d1ac063..4028d80c0a3c 100644 --- a/tests/ui/unnecessary_fold.rs +++ b/tests/ui/unnecessary_fold.rs @@ -41,4 +41,12 @@ fn unnecessary_fold_should_ignore() { let _ = [(0..2), (0..3)].iter().fold(1, |a, b| a * b.len()); } +/// Should lint only the line containing the fold +fn unnecessary_fold_over_multiple_lines() { + let _ = (0..3) + .map(|x| x + 1) + .filter(|x| x % 2 == 0) + .fold(false, |acc, x| acc || x > 2); +} + fn main() {} diff --git a/tests/ui/unnecessary_fold.stderr b/tests/ui/unnecessary_fold.stderr index f9911d4a3dcb..3282b3d9a708 100644 --- a/tests/ui/unnecessary_fold.stderr +++ b/tests/ui/unnecessary_fold.stderr @@ -30,5 +30,11 @@ error: this `.fold` can be written more succinctly using another method LL | let _: bool = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` -error: aborting due to 5 previous errors +error: this `.fold` can be written more succinctly using another method + --> $DIR/unnecessary_fold.rs:49:10 + | +LL | .fold(false, |acc, x| acc || x > 2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `any(|x| x > 2)` + +error: aborting due to 6 previous errors From 08f658bc72ed2f7d19e6a40ae17e80c289acfa20 Mon Sep 17 00:00:00 2001 From: Jeremy Stucki Date: Wed, 14 Aug 2019 19:35:06 +0200 Subject: [PATCH 2/4] Use different span --- clippy_lints/src/methods/mod.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9481cf4c8614..d73f74e0866a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1663,6 +1663,7 @@ fn lint_iter_cloned_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Ex fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: &[hir::Expr]) { fn check_fold_with_op( cx: &LateContext<'_, '_>, + expr: &hir::Expr, fold_args: &[hir::Expr], op: hir::BinOpKind, replacement_method_name: &str, @@ -1685,22 +1686,20 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: if match_var(&*left_expr, first_arg_ident); if replacement_has_args || match_var(&*right_expr, second_arg_ident); - then { - // Span containing `.fold(...)` - let next_point = cx.sess().source_map().next_point(fold_args[0].span); - let fold_span = next_point.with_hi(fold_args[2].span.hi() + BytePos(1)); + if let hir::ExprKind::MethodCall(_, span, _) = &expr.node; + then { let mut applicability = Applicability::MachineApplicable; let sugg = if replacement_has_args { format!( - ".{replacement}(|{s}| {r})", + "{replacement}(|{s}| {r})", replacement = replacement_method_name, s = second_arg_ident, r = snippet_with_applicability(cx, right_expr.span, "EXPR", &mut applicability), ) } else { format!( - ".{replacement}()", + "{replacement}()", replacement = replacement_method_name, ) }; @@ -1708,7 +1707,7 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: span_lint_and_sugg( cx, UNNECESSARY_FOLD, - fold_span, + span.with_hi(expr.span.hi()), // 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", @@ -1732,10 +1731,10 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: // Check if the first argument to .fold is a suitable literal if let hir::ExprKind::Lit(ref lit) = fold_args[1].node { match lit.node { - ast::LitKind::Bool(false) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Or, "any", true), - ast::LitKind::Bool(true) => check_fold_with_op(cx, fold_args, hir::BinOpKind::And, "all", true), - ast::LitKind::Int(0, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Add, "sum", false), - ast::LitKind::Int(1, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Mul, "product", false), + ast::LitKind::Bool(false) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Or, "any", true), + ast::LitKind::Bool(true) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::And, "all", true), + ast::LitKind::Int(0, _) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Add, "sum", false), + ast::LitKind::Int(1, _) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Mul, "product", false), _ => (), } } From 4366137d2e73d52724069b2e6335857b953d475c Mon Sep 17 00:00:00 2001 From: Jeremy Stucki Date: Wed, 14 Aug 2019 19:35:17 +0200 Subject: [PATCH 3/4] Update tests --- tests/ui/unnecessary_fold.stderr | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/ui/unnecessary_fold.stderr b/tests/ui/unnecessary_fold.stderr index 3282b3d9a708..22c44588ab7a 100644 --- a/tests/ui/unnecessary_fold.stderr +++ b/tests/ui/unnecessary_fold.stderr @@ -1,34 +1,34 @@ error: this `.fold` can be written more succinctly using another method - --> $DIR/unnecessary_fold.rs:8:19 + --> $DIR/unnecessary_fold.rs:8:20 | LL | let _ = (0..3).fold(false, |acc, x| acc || x > 2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `any(|x| x > 2)` | = note: `-D clippy::unnecessary-fold` implied by `-D warnings` error: this `.fold` can be written more succinctly using another method - --> $DIR/unnecessary_fold.rs:10:19 + --> $DIR/unnecessary_fold.rs:10:20 | LL | let _ = (0..3).fold(true, |acc, x| acc && x > 2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.all(|x| x > 2)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `all(|x| x > 2)` error: this `.fold` can be written more succinctly using another method - --> $DIR/unnecessary_fold.rs:12:24 + --> $DIR/unnecessary_fold.rs:12:25 | LL | let _: i32 = (0..3).fold(0, |acc, x| acc + x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.sum()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `sum()` error: this `.fold` can be written more succinctly using another method - --> $DIR/unnecessary_fold.rs:14:24 + --> $DIR/unnecessary_fold.rs:14:25 | LL | let _: i32 = (0..3).fold(1, |acc, x| acc * x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.product()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `product()` error: this `.fold` can be written more succinctly using another method - --> $DIR/unnecessary_fold.rs:19:40 + --> $DIR/unnecessary_fold.rs:19:41 | LL | let _: bool = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `any(|x| x > 2)` error: this `.fold` can be written more succinctly using another method --> $DIR/unnecessary_fold.rs:49:10 From fdf82eb1a8d3d3ba53744e0d1a24aba26f53c18c Mon Sep 17 00:00:00 2001 From: Jeremy Stucki Date: Wed, 14 Aug 2019 20:24:05 +0200 Subject: [PATCH 4/4] Remove unused import --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d73f74e0866a..0020dbd233d9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -15,7 +15,7 @@ use rustc::ty::{self, Predicate, Ty}; use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_errors::Applicability; use syntax::ast; -use syntax::source_map::{BytePos, Span}; +use syntax::source_map::Span; use syntax::symbol::LocalInternedString; use crate::utils::paths;