diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 64335f81a..4277b4b15 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 c80f6acd0..2dcc085ad 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 65d8b82da..768fbd1df 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: 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 +error: .fold(false, |acc, x| acc || x > 2)) is more succinctly expressed as .any(|x| x > 2) + --> $DIR/methods.rs:390:13 | -391 | let _ = opt.unwrap(); +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:396:13 + | +396 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings`