From f6e56d255905abd2b7728ffbf24c0c5b09f5d1ed Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 08:27:53 +0000 Subject: [PATCH 01/14] First pass at linting for .any expressed as a .fold --- clippy_lints/src/methods.rs | 88 ++++++++++++++++++++++++++++++++++++- tests/ui/methods.rs | 5 +++ tests/ui/methods.stderr | 12 ++++- 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 64335f81a6347..4277b4b15d3f8 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -12,7 +12,7 @@ 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, 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 @@ declare_lint! { "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 @@ impl LintPass for Pass { GET_UNWRAP, STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, - USELESS_ASREF + USELESS_ASREF, + FOLD_ANY ) } } @@ -717,6 +735,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_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 { + 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 c80f6acd06b9c..2dcc085add322 100644 --- 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 65d8b82da1400..768fbd1df5456 100644 --- 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` From 1feb9fd5502d50f50b2e6e3bf8101e1d860e3dba Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 09:30:08 +0000 Subject: [PATCH 02/14] Tidy using if_chain and snippet function. Actually check that the initial fold value is false. Remove some unwraps --- clippy_lints/src/methods.rs | 73 ++++++++++++++++--------------------- tests/ui/methods.rs | 5 +++ tests/ui/methods.stderr | 4 +- 3 files changed, 38 insertions(+), 44 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 4277b4b15d3f8..36e6fb4b1a29a 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1139,53 +1139,42 @@ 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 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_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); - 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); + // 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); - let first_arg = &closure_body.arguments[0]; - let arg_ident = get_arg_name(&first_arg.pat).unwrap(); + // 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_first_arg_ident) = get_arg_name(&closure_body.arguments[1].pat); - let second_arg = &closure_body.arguments[1]; - let second_arg_ident = get_arg_name(&second_arg.pat).unwrap(); + // 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; - 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?"); - } + then { + let right_source = snippet(cx, right_expr.span, "EXPR"); + + span_lint( + cx, + FOLD_ANY, + expr.span, + // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f) + &format!( + ".fold(false, |{f}, {s}| {f} || {r})) is more succinctly expressed as .any(|{s}| {r})", + f = first_arg_ident, + s = second_first_arg_ident, + r = right_source + ), + ); } - } else { - return; } } diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 2dcc085add322..0dd4ff47fa909 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -390,6 +390,11 @@ fn fold_any() { let _ = (0..3).fold(false, |acc, x| acc || x > 2); } +/// Checks implementation of the `FOLD_ANY` lint +fn fold_any_ignore_initial_value_of_true() { + let _ = (0..3).fold(true, |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 768fbd1df5456..ef24e4a8e22de 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -502,9 +502,9 @@ error: .fold(false, |acc, x| acc || x > 2)) is more succinctly expressed as .any = 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:396:13 + --> $DIR/methods.rs:401:13 | -396 | let _ = opt.unwrap(); +401 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From 528be23c07f1578161713a3a217876a60117cd81 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 10:05:01 +0000 Subject: [PATCH 03/14] Move get_arg_name into utils --- clippy_lints/src/map_clone.rs | 12 ++---------- clippy_lints/src/methods.rs | 11 +---------- clippy_lints/src/utils/mod.rs | 8 ++++++++ 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index e126d5c07d700..3bcaccf345a97 100644 --- 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. /// @@ -121,14 +121,6 @@ fn get_type_name(cx: &LateContext, expr: &Expr, arg: &Expr) -> Option<&'static s } } -fn get_arg_name(pat: &Pat) -> Option { - 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 index 36e6fb4b1a29a..67bbed487419e 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -10,7 +10,7 @@ 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 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, 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}; @@ -1125,15 +1125,6 @@ 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 { - 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, diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 0c1b10f05c89e..94d2caf9c50f6 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1034,3 +1034,11 @@ pub fn type_size<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> Option bool { cx.tcx.lint_level_at_node(lint, id).0 == Level::Allow } + +pub fn get_arg_name(pat: &Pat) -> Option { + match pat.node { + PatKind::Binding(_, _, name, None) => Some(name.node), + PatKind::Ref(ref subpat, _) => get_arg_name(subpat), + _ => None, + } +} \ No newline at end of file From 7e833ea5ce306566e273e078974d922bc2b45fb1 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 10:07:41 +0000 Subject: [PATCH 04/14] Add description --- clippy_lints/src/methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 67bbed487419e..bdfd729a32749 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -637,7 +637,7 @@ declare_lint! { declare_lint! { pub FOLD_ANY, Warn, - "TODO" + "using `fold` to emulate the behaviour of `any`" } impl LintPass for Pass { From 360f2359d5c4d8a332182143c4aa350b0f2734df Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 15:30:06 +0000 Subject: [PATCH 05/14] Fix name --- clippy_lints/src/methods.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index bdfd729a32749..138dbed31358d 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1142,7 +1142,7 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { // 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_first_arg_ident) = get_arg_name(&closure_body.arguments[1].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; @@ -1161,7 +1161,7 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { &format!( ".fold(false, |{f}, {s}| {f} || {r})) is more succinctly expressed as .any(|{s}| {r})", f = first_arg_ident, - s = second_first_arg_ident, + s = second_arg_ident, r = right_source ), ); From 70a5535ffa2040829e6a7a8f673b1fb13505b636 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 18:18:09 +0000 Subject: [PATCH 06/14] Address some review comments --- clippy_lints/src/methods.rs | 11 ++++++----- clippy_lints/src/utils/mod.rs | 14 ++++++++++++++ tests/ui/methods.rs | 7 ++++++- tests/ui/methods.stderr | 8 ++++---- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 138dbed31358d..9661a6011bd86 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1153,17 +1153,18 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { then { let right_source = snippet(cx, right_expr.span, "EXPR"); - span_lint( + span_lint_and_sugg( cx, FOLD_ANY, expr.span, // TODO: don't suggest .any(|x| f(x)) if we can suggest .any(f) - &format!( - ".fold(false, |{f}, {s}| {f} || {r})) is more succinctly expressed as .any(|{s}| {r})", - f = first_arg_ident, + "this `.fold` can more succintly be expressed as `.any`", + "try", + format!( + ".any(|{s}| {r})", s = second_arg_ident, r = right_source - ), + ) ); } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 94d2caf9c50f6..44ebc5aa60018 100644 --- 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, diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 0dd4ff47fa909..8cffbf76924fa 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -391,10 +391,15 @@ fn fold_any() { } /// Checks implementation of the `FOLD_ANY` lint -fn fold_any_ignore_initial_value_of_true() { +fn fold_any_ignores_initial_value_of_true() { let _ = (0..3).fold(true, |acc, x| acc || x > 2); } +/// Checks implementation of the `FOLD_ANY` lint +fn fold_any_ignores_non_boolean_accumalator() { + let _ = (0..3).fold(0, |acc, x| acc + if x > 2 { 1 } else { 0 }); +} + #[allow(similar_names)] fn main() { let opt = Some(0); diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index ef24e4a8e22de..f1746354380ff 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -493,18 +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) +error: this `.fold` can more succintly be 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` 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:401:13 + --> $DIR/methods.rs:406:13 | -401 | let _ = opt.unwrap(); +406 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From ad164939ed1cdd7164683814f8411d20d49e090b Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 14 Jan 2018 20:04:34 +0000 Subject: [PATCH 07/14] Check that we're calling Iterator::fold --- clippy_lints/src/methods.rs | 6 +++++- tests/ui/methods.rs | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 9661a6011bd86..98dfb3ad98031 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1126,7 +1126,11 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir } fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { - // DONOTMERGE: What if this is just some other method called fold? + // 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"); diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 8cffbf76924fa..ae347269430cc 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -385,17 +385,17 @@ fn iter_skip_next() { let _ = foo.filter().skip(42).next(); } -/// Checks implementation of the `FOLD_ANY` lint +/// Should trigger the `FOLD_ANY` lint fn fold_any() { let _ = (0..3).fold(false, |acc, x| acc || x > 2); } -/// Checks implementation of the `FOLD_ANY` lint +/// 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); } -/// Checks implementation of the `FOLD_ANY` lint +/// 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 }); } From a64d19cc0e8c1c47dd253b5bcb4a1a619c4ae7d3 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Tue, 16 Jan 2018 21:20:55 +0000 Subject: [PATCH 08/14] Fix error span to play nicely with rustfix --- clippy_lints/src/methods.rs | 7 +++++-- clippy_lints/src/utils/mod.rs | 4 ++-- tests/ui/methods.rs | 5 +++++ tests/ui/methods.stderr | 14 ++++++++++---- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 98dfb3ad98031..5ff48a25b1c3f 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -9,7 +9,7 @@ use std::borrow::Cow; use std::fmt; use std::iter; use syntax::ast; -use syntax::codemap::Span; +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, remove_blocks, same_tys, single_segment_path, snippet, span_lint, @@ -1157,10 +1157,13 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { then { let right_source = snippet(cx, right_expr.span, "EXPR"); + // 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, - expr.span, + 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", diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 44ebc5aa60018..d4f7539cea9eb 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -606,7 +606,7 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( /// --> $DIR/methods.rs:390:13 /// | /// 390 | let _ = (0..3).fold(false, |acc, x| acc || x > 2); -/// | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` +/// | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` /// | /// = note: `-D fold-any` implied by `-D warnings` /// @@ -1055,4 +1055,4 @@ pub fn get_arg_name(pat: &Pat) -> Option { PatKind::Ref(ref subpat, _) => get_arg_name(subpat), _ => None, } -} \ No newline at end of file +} diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index ae347269430cc..d50f8e35fa4b2 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -400,6 +400,11 @@ 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); +} + #[allow(similar_names)] fn main() { let opt = Some(0); diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index f1746354380ff..2c03e077d5725 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -494,17 +494,23 @@ error: called `skip(x).next()` on an iterator. This is more succinctly expressed | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `.fold` can more succintly be expressed as `.any` - --> $DIR/methods.rs:390:13 + --> $DIR/methods.rs:390:19 | 390 | let _ = (0..3).fold(false, |acc, x| acc || x > 2); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| 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 + | +405 | 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:406:13 + --> $DIR/methods.rs:411:13 | -406 | let _ = opt.unwrap(); +411 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From 1cac693bc767e6c5648baeab1164feb4aea6ae30 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 19:12:44 +0000 Subject: [PATCH 09/14] Lint on folds implementing .all, .sum and .product --- clippy_lints/src/methods.rs | 93 ++++++++++++++++++++++++------------- tests/ui/methods.rs | 37 ++++++++++----- tests/ui/methods.stderr | 34 ++++++++++---- 3 files changed, 111 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 5ff48a25b1c3f..6eb48a1bc2927 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; + + then { + let right_source = snippet(cx, right_expr.span, "EXPR"); - // Span containing `.fold(...)` - let fold_span = fold_args[0].span.next_point().with_hi(fold_args[2].span.hi() + BytePos(1)); + // 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 .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_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 d50f8e35fa4b2..3ca77f744fe09 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); -/// 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); -} + // 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); -/// 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 }); + // Can be replaced by .product + let _ = (0..3).fold(1, |acc, x| acc * x); + let _ = (0..3).fold(1, |acc, x| x * acc); } -/// 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 2c03e077d5725..1c8569e8d6ba3 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` From 29a2dd4cb8f8f386656f6207ff007f068d170887 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 20:11:40 +0000 Subject: [PATCH 10/14] Fix bug. Don't expect lint when acc is on rhs --- tests/ui/methods.rs | 17 ++++++++++------- tests/ui/methods.stderr | 24 ++++++++++++------------ 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 3ca77f744fe09..f7a4b39c7a6f2 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -389,19 +389,12 @@ fn iter_skip_next() { 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 trigger the `UNNECESSARY_FOLD` lint, with an error span including exactly `.fold(...)` @@ -416,6 +409,16 @@ fn unnecessary_fold_should_ignore() { 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)] diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 1c8569e8d6ba3..57f073f00b040 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -502,33 +502,33 @@ error: this `.fold` can be written more succinctly using another method = note: `-D fold-any` implied by `-D warnings` error: this `.fold` can be written more succinctly using another method - --> $DIR/methods.rs:395:19 + --> $DIR/methods.rs:393:19 | -395 | let _ = (0..3).fold(true, |acc, x| acc && x > 2); +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:399:19 + --> $DIR/methods.rs:395:19 | -399 | let _ = (0..3).fold(0, |acc, x| acc + x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.sum(|x| x)` +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:403:19 + --> $DIR/methods.rs:397:19 | -403 | let _ = (0..3).fold(1, |acc, x| acc * x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.product(|x| x)` +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:409:34 + --> $DIR/methods.rs:402:34 | -409 | let _ = (0..3).map(|x| 2 * x).fold(false, |acc, x| acc || x > 2); +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:424:13 + --> $DIR/methods.rs:427:13 | -424 | let _ = opt.unwrap(); +427 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From 9806b31d530eabaaccaef17f2755000979a4c2a7 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 20:21:29 +0000 Subject: [PATCH 11/14] Rename lint, improve documentation --- clippy_lints/src/methods.rs | 56 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 6eb48a1bc2927..822bae14ff917 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -624,20 +624,26 @@ declare_lint! { } -/// **What it does:** Checks for using `fold` to implement `any`. +/// **What it does:** Checks for using `fold` when a more succint 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:** Changes semantics - the suggested replacement is short-circuiting. +/// **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 FOLD_ANY, + pub UNNECESSARY_FOLD, Warn, - "using `fold` to emulate the behaviour of `any`" + "using `fold` when a more succint alternative exists" } impl LintPass for Pass { @@ -671,7 +677,7 @@ impl LintPass for Pass { STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, USELESS_ASREF, - FOLD_ANY + UNNECESSARY_FOLD ) } } @@ -736,7 +742,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } 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_unnecessary_fold(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, &method_call.name.as_str(), args); @@ -1125,7 +1131,7 @@ fn lint_iter_cloned_collect(cx: &LateContext, expr: &hir::Expr, iter_args: &[hir } } -fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { +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; @@ -1138,7 +1144,8 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { cx: &LateContext, fold_args: &[hir::Expr], op: hir::BinOp_, - replacement_method_name: &str) { + replacement_method_name: &str, + replacement_has_args: bool) { if_chain! { // Extract the body of the closure passed to fold @@ -1158,24 +1165,31 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { if path.segments.len() == 1 && &path.segments[0].name == &first_arg_ident; then { - let right_source = snippet(cx, right_expr.span, "EXPR"); - // 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, - FOLD_ANY, + UNNECESSARY_FOLD, 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 - ) + sugg ); } } @@ -1186,16 +1200,16 @@ fn lint_fold_any(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::Expr]) { hir::ExprLit(ref lit) => { match lit.node { ast::LitKind::Bool(false) => check_fold_with_op( - cx, fold_args, hir::BinOp_::BiOr, "any" + cx, fold_args, hir::BinOp_::BiOr, "any", true ), ast::LitKind::Bool(true) => check_fold_with_op( - cx, fold_args, hir::BinOp_::BiAnd, "all" + cx, fold_args, hir::BinOp_::BiAnd, "all", true ), ast::LitKind::Int(0, _) => check_fold_with_op( - cx, fold_args, hir::BinOp_::BiAdd, "sum" + cx, fold_args, hir::BinOp_::BiAdd, "sum", false ), ast::LitKind::Int(1, _) => check_fold_with_op( - cx, fold_args, hir::BinOp_::BiMul, "product" + cx, fold_args, hir::BinOp_::BiMul, "product", false ), _ => return } From b73efad6001ac140aa233b63cc093ea71049e2bb Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 21:06:16 +0000 Subject: [PATCH 12/14] Add some reviewer comments --- clippy_lints/src/methods.rs | 6 +++--- clippy_lints/src/utils/mod.rs | 4 ++-- tests/ui/methods.stderr | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 822bae14ff917..556c6988d668a 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1173,7 +1173,7 @@ fn lint_unnecessary_fold(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::E ".{replacement}(|{s}| {r})", replacement = replacement_method_name, s = second_arg_ident, - r = snippet(cx, right_expr.span, "EXPR") + r = snippet(cx, right_expr.span, "EXPR"), ) } else { format!( @@ -1186,10 +1186,10 @@ fn lint_unnecessary_fold(cx: &LateContext, expr: &hir::Expr, fold_args: &[hir::E cx, UNNECESSARY_FOLD, fold_span, - // TODO: don't suggest e.g. .any(|x| f(x)) if we can suggest .any(f) + // 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 + sugg, ); } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index d4f7539cea9eb..4019321d71160 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -601,7 +601,7 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( /// 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
 ///     |
@@ -609,7 +609,7 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>(
 ///     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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, diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 57f073f00b040..254c7bf189501 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -499,7 +499,7 @@ error: this `.fold` can be written more succinctly using another method 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` + = note: `-D unnecessary-fold` implied by `-D warnings` error: this `.fold` can be written more succinctly using another method --> $DIR/methods.rs:393:19 From a324a2bc38738f288b1fd0bd39c1d203aef88bd4 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 17 Jan 2018 21:54:09 +0000 Subject: [PATCH 13/14] Fix typos --- clippy_lints/src/methods.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 556c6988d668a..ea5fba8adb204 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -624,7 +624,7 @@ declare_lint! { } -/// **What it does:** Checks for using `fold` when a more succint alternative exists. +/// **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`. /// @@ -643,7 +643,7 @@ declare_lint! { declare_lint! { pub UNNECESSARY_FOLD, Warn, - "using `fold` when a more succint alternative exists" + "using `fold` when a more succinct alternative exists" } impl LintPass for Pass { From 71abd81d221235bb61fc6683e81b346234802879 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 19 Jan 2018 13:18:44 +0100 Subject: [PATCH 14/14] Update error count --- tests/ui/methods.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 149fa4c179663..5d3015f5e606f 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -533,5 +533,5 @@ error: used unwrap() on an Option value. If you don't want to handle the None ca | = note: `-D option-unwrap-used` implied by `-D warnings` -error: aborting due to 66 previous errors +error: aborting due to 71 previous errors