diff --git a/clippy_lints/src/map_clone.rs b/clippy_lints/src/map_clone.rs index 8403974629d61..4eeaf675c88b9 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. /// @@ -123,14 +123,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 8a92e49340f4e..d9b61a9e5cdcc 100644 --- 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 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 index 0c1b10f05c89e..4019321d71160 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, @@ -1034,3 +1048,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, + } +} diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index c80f6acd06b9c..f7a4b39c7a6f2 100644 --- 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 index feea8e5e512fa..5d3015f5e606f 100644 --- 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