diff --git a/CHANGELOG.md b/CHANGELOG.md index e7884818596c..7264dda310f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3270,6 +3270,7 @@ Released 2018-09-13 [`map_err_ignore`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten [`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity +[`map_then_identity_transformer`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_then_identity_transformer [`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or [`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref [`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index 7277e4080c5c..3d587def3171 100644 --- a/clippy_lints/src/derive.rs +++ b/clippy_lints/src/derive.rs @@ -362,8 +362,10 @@ fn check_unsafe_derive_deserialize<'tcx>( if !is_lint_allowed(cx, UNSAFE_DERIVE_DESERIALIZE, adt_hir_id); if cx.tcx.inherent_impls(def.did) .iter() - .map(|imp_did| cx.tcx.hir().expect_item(imp_did.expect_local())) - .any(|imp| has_unsafe(cx, imp)); + .any(|imp_did| { + let imp = cx.tcx.hir().expect_item(imp_did.expect_local()); + has_unsafe(cx, imp) + }); then { span_lint_and_help( cx, diff --git a/clippy_lints/src/disallowed_script_idents.rs b/clippy_lints/src/disallowed_script_idents.rs index 0c27c3f9255f..84daf1ccb292 100644 --- a/clippy_lints/src/disallowed_script_idents.rs +++ b/clippy_lints/src/disallowed_script_idents.rs @@ -53,8 +53,7 @@ impl DisallowedScriptIdents { pub fn new(whitelist: &[String]) -> Self { let whitelist = whitelist .iter() - .map(String::as_str) - .filter_map(Script::from_full_name) + .filter_map(|s| Script::from_full_name(String::as_str(s))) .collect(); Self { whitelist } } diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 8bfa1dfe5853..ad1e4147be9a 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -309,6 +309,7 @@ store.register_lints(&[ methods::MAP_COLLECT_RESULT_UNIT, methods::MAP_FLATTEN, methods::MAP_IDENTITY, + methods::MAP_THEN_IDENTITY_TRANSFORMER, methods::MAP_UNWRAP_OR, methods::NEEDLESS_SPLITN, methods::NEW_RET_NO_SELF, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 00d305131810..a0ecc07b43df 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -62,6 +62,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(methods::FROM_ITER_INSTEAD_OF_COLLECT), LintId::of(methods::IMPLICIT_CLONE), LintId::of(methods::INEFFICIENT_TO_STRING), + LintId::of(methods::MAP_THEN_IDENTITY_TRANSFORMER), LintId::of(methods::MAP_UNWRAP_OR), LintId::of(misc::FLOAT_CMP), LintId::of(misc::USED_UNDERSCORE_BINDING), diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index a0b2302662e6..7d60749901f1 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -97,8 +97,9 @@ fn never_loop_block(block: &Block<'_>, main_loop_id: HirId) -> NeverLoopResult { } fn never_loop_expr_seq<'a, T: Iterator>>(es: &mut T, main_loop_id: HirId) -> NeverLoopResult { - es.map(|e| never_loop_expr(e, main_loop_id)) - .fold(NeverLoopResult::Otherwise, combine_seq) + es.fold(NeverLoopResult::Otherwise, |seq, e| { + combine_seq(seq, never_loop_expr(e, main_loop_id)) + }) } fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<&'tcx Expr<'tcx>> { @@ -189,13 +190,15 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult { } fn never_loop_expr_all<'a, T: Iterator>>(es: &mut T, main_loop_id: HirId) -> NeverLoopResult { - es.map(|e| never_loop_expr(e, main_loop_id)) - .fold(NeverLoopResult::Otherwise, combine_both) + es.fold(NeverLoopResult::Otherwise, |seq, e| { + combine_both(seq, never_loop_expr(e, main_loop_id)) + }) } fn never_loop_expr_branch<'a, T: Iterator>>(e: &mut T, main_loop_id: HirId) -> NeverLoopResult { - e.map(|e| never_loop_expr(e, main_loop_id)) - .fold(NeverLoopResult::AlwaysBreak, combine_branches) + e.fold(NeverLoopResult::AlwaysBreak, |br, e| { + combine_branches(br, never_loop_expr(e, main_loop_id)) + }) } fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>) -> String { diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs index 777ec9b75bc2..3393809c62ef 100644 --- a/clippy_lints/src/matches/redundant_pattern_match.rs +++ b/clippy_lints/src/matches/redundant_pattern_match.rs @@ -58,8 +58,7 @@ fn type_needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, see ty::Array(ty, _) => type_needs_ordered_drop_inner(cx, *ty, seen), ty::Adt(adt, subs) => adt .all_fields() - .map(|f| f.ty(cx.tcx, subs)) - .any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)), + .any(|f| type_needs_ordered_drop_inner(cx, f.ty(cx.tcx, subs), seen)), _ => true, } } diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs index d1b5e945dfda..88a4a9821732 100644 --- a/clippy_lints/src/methods/filter_map_identity.rs +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -7,7 +7,12 @@ use rustc_span::{source_map::Span, sym}; use super::FILTER_MAP_IDENTITY; -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: &hir::Expr<'_>, filter_map_span: Span) { +pub(super) fn check( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + filter_map_arg: &hir::Expr<'_>, + filter_map_span: Span, +) -> bool { if is_trait_method(cx, expr, sym::Iterator) && is_expr_identity_function(cx, filter_map_arg) { span_lint_and_sugg( cx, @@ -18,5 +23,9 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_map_arg: "flatten()".to_string(), Applicability::MachineApplicable, ); + // Returns a boolean indicating whether this lint has been triggered or not + true + } else { + false } } diff --git a/clippy_lints/src/methods/flat_map_identity.rs b/clippy_lints/src/methods/flat_map_identity.rs index 6f911d79d0bc..e92608a6d08b 100644 --- a/clippy_lints/src/methods/flat_map_identity.rs +++ b/clippy_lints/src/methods/flat_map_identity.rs @@ -13,7 +13,7 @@ pub(super) fn check<'tcx>( expr: &'tcx hir::Expr<'_>, flat_map_arg: &'tcx hir::Expr<'_>, flat_map_span: Span, -) { +) -> bool { if is_trait_method(cx, expr, sym::Iterator) && is_expr_identity_function(cx, flat_map_arg) { span_lint_and_sugg( cx, @@ -24,5 +24,9 @@ pub(super) fn check<'tcx>( "flatten()".to_string(), Applicability::MachineApplicable, ); + // Returns a boolean indicating whether this lint has been triggered or not + true + } else { + false } } diff --git a/clippy_lints/src/methods/flat_map_option.rs b/clippy_lints/src/methods/flat_map_option.rs index 615bde941334..6b233c7b1683 100644 --- a/clippy_lints/src/methods/flat_map_option.rs +++ b/clippy_lints/src/methods/flat_map_option.rs @@ -9,18 +9,23 @@ use rustc_span::{source_map::Span, sym}; use super::FLAT_MAP_OPTION; use clippy_utils::ty::is_type_diagnostic_item; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, span: Span) { +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + arg: &'tcx hir::Expr<'_>, + span: Span, +) -> bool { if !is_trait_method(cx, expr, sym::Iterator) { - return; + return false; } let arg_ty = cx.typeck_results().expr_ty_adjusted(arg); let sig = match arg_ty.kind() { ty::Closure(_, substs) => substs.as_closure().sig(), _ if arg_ty.is_fn() => arg_ty.fn_sig(cx.tcx), - _ => return, + _ => return false, }; if !is_type_diagnostic_item(cx, sig.output().skip_binder(), sym::Option) { - return; + return false; } span_lint_and_sugg( cx, @@ -31,4 +36,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, arg "filter_map".into(), Applicability::MachineApplicable, ); + // Returns a boolean indicating whether this lint has been triggered or not + true } diff --git a/clippy_lints/src/methods/map_then_identity_transformer.rs b/clippy_lints/src/methods/map_then_identity_transformer.rs new file mode 100644 index 000000000000..6deef68a84dc --- /dev/null +++ b/clippy_lints/src/methods/map_then_identity_transformer.rs @@ -0,0 +1,136 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::path_to_local_id; +use clippy_utils::visitors::expr_visitor; +use rustc_hir::intravisit::Visitor; +use rustc_hir::{BodyId, Expr, ExprKind, PatKind}; +use rustc_lint::{LateContext, LintContext}; +use rustc_span::{MultiSpan, Span}; + +use super::MAP_THEN_IDENTITY_TRANSFORMER; + +pub(super) fn check<'tcx>( + cx: &LateContext<'_>, + map_span: Span, + map_name: &str, + map_param: &'tcx Expr<'_>, + transformer_name: &str, + // Use the last parameter of the transformer because the transfomer may be `fold(_, _)` + transformer_last_param: &'tcx Expr<'_>, +) { + match &transformer_last_param.kind { + ExprKind::Closure(_, _, transformer_clos_body_id, _, _) => { + match &map_param.kind { + // map(Closure).(Closure) + ExprKind::Closure(_, _, map_clos_body_id, _, _) => { + let map_clos_val = &cx.tcx.hir().body(*map_clos_body_id).value; + if_chain!( + // checks if the body of the closure of the `map` is an one-line expression + if is_one_line(cx, map_clos_val.span); + // checks if the parameter of the closure of the transformer appears once in its body + if let Some(refd_param_span) = refd_param_span(cx, *transformer_clos_body_id); + then { + span_lint_and_then( + cx, + MAP_THEN_IDENTITY_TRANSFORMER, + MultiSpan::from_span(map_span), + &format!("this `{map_name}` can be collapsed into the `{transformer_name}`"), + |diag| { + let mut help_span = MultiSpan::from_spans(vec![map_clos_val.span, refd_param_span]); + help_span.push_span_label(refd_param_span, "replace this variable".into()); + help_span.push_span_label(map_clos_val.span, "with this expression".into()); + diag.span_help(help_span, &format!("these `{map_name}` and `{transformer_name}` can be merged into a single `{transformer_name}`")); + }, + ); + } + + ); + }, + // map(Path).(Closure) + ExprKind::Path(_) => { + if_chain!( + // checks if the parameter of the `map` fits within one line + if is_one_line(cx, map_param.span); + // checks if the parameter of the closure of the transformer appears once in its body + if let Some(refd_param_span) = refd_param_span(cx, *transformer_clos_body_id); + then { + span_lint_and_then( + cx, + MAP_THEN_IDENTITY_TRANSFORMER, + MultiSpan::from_span(map_span), + &format!("this `{map_name}` can be collapsed into the `{transformer_name}`"), + |diag| { + let mut help_span = MultiSpan::from_spans(vec![map_param.span, refd_param_span]); + help_span.push_span_label(map_param.span, "apply this".into()); + help_span.push_span_label(refd_param_span, "to this variable".into()); + diag.span_help(help_span, &format!("these `{map_name}` and `{transformer_name}` can be merged into a single `{transformer_name}`")); + }, + ); + } + + ); + }, + _ => (), + } + }, + // map(Path).(Path) or map(Closure).(Path) + ExprKind::Path(_) => { + // checks if the parameter of the `map` fits within one line + if is_one_line(cx, map_param.span) { + span_lint_and_then( + cx, + MAP_THEN_IDENTITY_TRANSFORMER, + MultiSpan::from_span(map_span), + &format!("this `{map_name}` can be collapsed into the `{transformer_name}`"), + |diag| { + let mut help_span = MultiSpan::from_spans(vec![map_param.span, transformer_last_param.span]); + help_span.push_span_label(map_param.span, format!("and use this in the `{transformer_name}`")); + help_span.push_span_label(transformer_last_param.span, "change this to a closure".into()); + diag.span_help(help_span, &format!("these `{map_name}` and `{transformer_name}` can be merged into a single `{transformer_name}`")); + }, + ); + } + }, + _ => (), + } +} + +// Given a closure `|.., x| y`, checks if `x` is referenced just exactly once in `y` and returns +// the span of `x` in `y` +fn refd_param_span(cx: &LateContext<'_>, clos_body_id: BodyId) -> Option { + let clos_body = cx.tcx.hir().body(clos_body_id); + if_chain! { + if let [.., clos_param] = clos_body.params; + if let PatKind::Binding(_, clos_param_id, _, _) = clos_param.pat.kind; + then { + let clos_val = &clos_body.value; + let mut count = 0; + let mut span = None; + expr_visitor(cx, |expr| { + if path_to_local_id(expr, clos_param_id) { + count += 1; + span = Some(expr.span); + } + count <= 1 + }) + .visit_expr(clos_val); + + if count == 1 { + span + } else { + None + } + } else { + None + } + } +} + +fn is_one_line(cx: &LateContext<'_>, span: Span) -> bool { + let src = cx.sess().source_map(); + if let Ok(lo) = src.lookup_line(span.lo()) { + if let Ok(hi) = src.lookup_line(span.hi()) { + return hi.line == lo.line; + } + } + false +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5edd22cd14c7..7862ed24c3fe 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -39,6 +39,7 @@ mod manual_str_repeat; mod map_collect_result_unit; mod map_flatten; mod map_identity; +mod map_then_identity_transformer; mod map_unwrap_or; mod ok_expect; mod option_as_ref_deref; @@ -2012,6 +2013,31 @@ declare_clippy_lint! { "unnecessary calls to `to_owned`-like functions" } +declare_clippy_lint! { + /// ### What it does + /// Finds `_.map(_).(_)` where the method calls may be collapsed + /// together and `transformer` is any of `all`, `any`, `find_map`, `flat_map`, + /// `filter_map`, `fold`, `map`, and `position`. + /// + /// ### Why is this bad? + /// It is unnecessarily verbose and complex. + /// + /// ### Example + /// ```rust + /// let iter = vec![1, 2, 3].into_iter(); + /// let _ = iter.map(|x| x * 2).map(|x| x + 1); + /// ``` + /// Use instead: + /// ```rust + /// let iter = vec![1, 2, 3].into_iter(); + /// let _ = iter.map(|x| x * 2 + 1); + /// ``` + #[clippy::version = "1.60.0"] + pub MAP_THEN_IDENTITY_TRANSFORMER, + pedantic, + "using transformers which can be collapsed together." +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -2096,6 +2122,7 @@ impl_lint_pass!(Methods => [ MANUAL_SPLIT_ONCE, NEEDLESS_SPLITN, UNNECESSARY_TO_OWNED, + MAP_THEN_IDENTITY_TRANSFORMER, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2321,6 +2348,11 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("add" | "offset" | "sub" | "wrapping_offset" | "wrapping_add" | "wrapping_sub", [_arg]) => { zst_offset::check(cx, expr, recv); }, + (name @ ("all" | "any" | "position"), [arg]) => { + if let Some((name2 @ "map", [_, arg2], span2)) = method_call(recv) { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, arg); + } + }, ("and_then", [arg]) => { let biom_option_linted = bind_instead_of_map::OptionAndThenSome::check(cx, expr, recv, arg); let biom_result_linted = bind_instead_of_map::ResultAndThenOk::check(cx, expr, recv, arg); @@ -2365,23 +2397,45 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); }, - ("filter_map", [arg]) => { - unnecessary_filter_map::check(cx, expr, arg, name); - filter_map_identity::check(cx, expr, arg, span); + (name @ "filter_map", [arg]) => { + let triggered = unnecessary_filter_map::check(cx, expr, arg, name); + let triggered2 = filter_map_identity::check(cx, expr, arg, span); + if !triggered && !triggered2 { + if let Some((name2 @ "map", [_, arg2], span2)) = method_call(recv) { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, arg); + } + } }, - ("find_map", [arg]) => { - unnecessary_filter_map::check(cx, expr, arg, name); + (name @ "find_map", [arg]) => { + let triggered = unnecessary_filter_map::check(cx, expr, arg, name); + if !triggered { + if let Some((name2 @ "map", [_, arg2], span2)) = method_call(recv) { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, arg); + } + } }, - ("flat_map", [arg]) => { - flat_map_identity::check(cx, expr, arg, span); - flat_map_option::check(cx, expr, arg, span); + (name @ "flat_map", [arg]) => { + let triggered = flat_map_identity::check(cx, expr, arg, span); + let triggered2 = flat_map_option::check(cx, expr, arg, span); + if !triggered && !triggered2 { + if let Some((name2 @ "map", [_, arg2], span2)) = method_call(recv) { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, arg); + } + } }, (name @ "flatten", args @ []) => match method_call(recv) { Some(("map", [recv, map_arg], _)) => map_flatten::check(cx, expr, recv, map_arg), Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args), _ => {}, }, - ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span), + (name @ "fold", [init, acc]) => { + let triggered = unnecessary_fold::check(cx, expr, init, acc, span); + if !triggered { + if let Some((name2 @ "map", [_, arg2], span2)) = method_call(recv) { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, acc); + } + } + }, ("for_each", [_]) => { if let Some(("inspect", [_, _], span2)) = method_call(recv) { inspect_for_each::check(cx, expr, span2); @@ -2398,15 +2452,18 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio } } }, - ("map", [m_arg]) => { - if let Some((name, [recv2, args @ ..], span2)) = method_call(recv) { - match (name, args) { + (name @ "map", [m_arg]) => { + if let Some((name2, [recv2, args @ ..], span2)) = method_call(recv) { + match (name2, args) { ("as_mut", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, true, msrv), ("as_ref", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, false, msrv), ("filter", [f_arg]) => { filter_map::check(cx, expr, recv2, f_arg, span2, recv, m_arg, span, false); }, ("find", [f_arg]) => filter_map::check(cx, expr, recv2, f_arg, span2, recv, m_arg, span, true), + (name2 @ "map", [arg2]) => { + map_then_identity_transformer::check(cx, span2, name2, arg2, name, m_arg); + }, _ => {}, } } diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index 9e48cd13b4cd..cb00ce2ba53a 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -13,9 +13,9 @@ use rustc_span::sym; use super::UNNECESSARY_FILTER_MAP; use super::UNNECESSARY_FIND_MAP; -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<'_>, name: &str) { +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<'_>, name: &str) -> bool { if !is_trait_method(cx, expr, sym::Iterator) { - return; + return false; } if let hir::ExprKind::Closure(_, _, body_id, ..) = arg.kind { @@ -43,10 +43,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< { if name == "filter_map" { "filter" } else { "find" } }, - _ => return, + _ => return false, } } else { - return; + return false; }; span_lint( cx, @@ -58,6 +58,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< expr.span, &format!("this `.{}` can be written more simply using `.{}`", name, sugg), ); + // Returns a boolean indicating whether this lint has been triggered or not + true + } else { + false } } diff --git a/clippy_lints/src/methods/unnecessary_fold.rs b/clippy_lints/src/methods/unnecessary_fold.rs index 47a811996085..8692901434f7 100644 --- a/clippy_lints/src/methods/unnecessary_fold.rs +++ b/clippy_lints/src/methods/unnecessary_fold.rs @@ -17,7 +17,7 @@ pub(super) fn check( init: &hir::Expr<'_>, acc: &hir::Expr<'_>, fold_span: Span, -) { +) -> bool { fn check_fold_with_op( cx: &LateContext<'_>, expr: &hir::Expr<'_>, @@ -26,7 +26,7 @@ pub(super) fn check( op: hir::BinOpKind, replacement_method_name: &str, replacement_has_args: bool, - ) { + ) -> bool { if_chain! { // Extract the body of the closure passed to fold if let hir::ExprKind::Closure(_, _, body_id, _, _) = acc.kind; @@ -71,13 +71,17 @@ pub(super) fn check( sugg, applicability, ); + // Returns a boolean indicating whether this lint has been triggered or not + true + } else { + false } } } // Check that this is a call to Iterator::fold rather than just some function called fold if !is_trait_method(cx, expr, sym::Iterator) { - return; + return false; } // Check if the first argument to .fold is a suitable literal @@ -87,9 +91,11 @@ pub(super) fn check( ast::LitKind::Bool(true) => check_fold_with_op(cx, expr, acc, fold_span, hir::BinOpKind::And, "all", true), ast::LitKind::Int(0, _) => check_fold_with_op(cx, expr, acc, fold_span, hir::BinOpKind::Add, "sum", false), ast::LitKind::Int(1, _) => { - check_fold_with_op(cx, expr, acc, fold_span, hir::BinOpKind::Mul, "product", false); + check_fold_with_op(cx, expr, acc, fold_span, hir::BinOpKind::Mul, "product", false) }, - _ => (), + _ => false, } + } else { + false } } diff --git a/tests/lint_message_convention.rs b/tests/lint_message_convention.rs index b4d94dc983fe..613580fee9c8 100644 --- a/tests/lint_message_convention.rs +++ b/tests/lint_message_convention.rs @@ -69,13 +69,10 @@ fn lint_message_convention() { // * don't have puncuation at the end of the last sentence // these directories have interesting tests - let test_dirs = ["ui", "ui-cargo", "ui-internal", "ui-toml"] - .iter() - .map(PathBuf::from) - .map(|p| { - let base = PathBuf::from("tests"); - base.join(p) - }); + let test_dirs = ["ui", "ui-cargo", "ui-internal", "ui-toml"].iter().map(|p| { + let base = PathBuf::from("tests"); + base.join(PathBuf::from(p)) + }); // gather all .stderr files let tests = test_dirs diff --git a/tests/ui/map_then_identity_transformer.rs b/tests/ui/map_then_identity_transformer.rs new file mode 100644 index 000000000000..154e593c19a5 --- /dev/null +++ b/tests/ui/map_then_identity_transformer.rs @@ -0,0 +1,67 @@ +#![warn(clippy::map_then_identity_transformer)] +#![allow(clippy::redundant_closure)] + +fn main() { + let a = [1, 2, 3]; + + // _.map(Path).transformerp(Closure) + // should lint + let _ = a.into_iter().map(func1).all(|y| y > 0); + let _ = a.into_iter().map(func1).any(|y| y > 0); + let _ = a.into_iter().map(func1).find_map(|y| y.checked_add(1)); + let _ = a.into_iter().map(func1).flat_map(|y| func2(y)); + let _ = a.into_iter().map(func1).filter_map(|y| y.checked_add(1)); + let _ = a.into_iter().map(func1).fold(1, |pd, x| pd * x + 1); + let _ = a.into_iter().map(func1).map(|y| func1(y)); + let _ = a.into_iter().map(func1).position(|y| y > 0); + + // _.map(Path).transformer(Closure) + // should lint + let _ = a.into_iter().map(func1).all(func3); + let _ = a.into_iter().map(func1).any(func3); + + // _.map(Path).transformer(Closure) + // should lint + let _ = a.into_iter().map(|x| func1(x) + 1).all(|y| y > 0); + let _ = a.into_iter().map(|x| func1(x) * func1(x)).any(|y| y > 0); + let _ = a.into_iter().map(|x| func1(x) * func1(x)).fold(1, |pd, x| pd * x + 1); + + // _.map(Closure).transformer(Path) + // should lint + let _ = a.into_iter().map(|x| func1(x) + 1).all(func3); + let _ = a.into_iter().map(|x| func1(x) + 1).any(func3); + let _ = a.into_iter().map(|x| func1(x) + 1).fold(1, func4); + + // should not when the transformer is `find` + let _ = a.into_iter().map(func1).find(|&y| y > 0); + + // should not lint this because the last param of the closure occurs more than once + let _ = a.into_iter().map(func1).all(|y| func1(y) * func1(y) > 10); + let _ = a.into_iter().map(|x| func1(x) + 1).any(|y| func1(y) * func1(y) > 10); + let _ = a.into_iter().map(func1).fold(1, |pd, x| pd * x * x); + + // should not lint this because the param of the `map` is not within one line + let _ = a + .into_iter() + .map(|x| { + // This comment has no special meaning:) + x * x + }) + .any(func3); +} + +fn func1(a: i32) -> i32 { + unimplemented!(); +} + +fn func2(a: i32) -> Vec { + unimplemented!(); +} + +fn func3(a: i32) -> bool { + unimplemented!(); +} + +fn func4(a: i32, b: i32) -> i32 { + unimplemented!(); +} diff --git a/tests/ui/map_then_identity_transformer.stderr b/tests/ui/map_then_identity_transformer.stderr new file mode 100644 index 000000000000..b8ff5dcd7afa --- /dev/null +++ b/tests/ui/map_then_identity_transformer.stderr @@ -0,0 +1,213 @@ +error: this `map` can be collapsed into the `all` + --> $DIR/map_then_identity_transformer.rs:9:27 + | +LL | let _ = a.into_iter().map(func1).all(|y| y > 0); + | ^^^ + | + = note: `-D clippy::map-then-identity-transformer` implied by `-D warnings` +help: these `map` and `all` can be merged into a single `all` + --> $DIR/map_then_identity_transformer.rs:9:31 + | +LL | let _ = a.into_iter().map(func1).all(|y| y > 0); + | ^^^^^ ^ to this variable + | | + | apply this + +error: this `map` can be collapsed into the `any` + --> $DIR/map_then_identity_transformer.rs:10:27 + | +LL | let _ = a.into_iter().map(func1).any(|y| y > 0); + | ^^^ + | +help: these `map` and `any` can be merged into a single `any` + --> $DIR/map_then_identity_transformer.rs:10:31 + | +LL | let _ = a.into_iter().map(func1).any(|y| y > 0); + | ^^^^^ ^ to this variable + | | + | apply this + +error: this `map` can be collapsed into the `find_map` + --> $DIR/map_then_identity_transformer.rs:11:27 + | +LL | let _ = a.into_iter().map(func1).find_map(|y| y.checked_add(1)); + | ^^^ + | +help: these `map` and `find_map` can be merged into a single `find_map` + --> $DIR/map_then_identity_transformer.rs:11:31 + | +LL | let _ = a.into_iter().map(func1).find_map(|y| y.checked_add(1)); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `flat_map` + --> $DIR/map_then_identity_transformer.rs:12:27 + | +LL | let _ = a.into_iter().map(func1).flat_map(|y| func2(y)); + | ^^^ + | +help: these `map` and `flat_map` can be merged into a single `flat_map` + --> $DIR/map_then_identity_transformer.rs:12:31 + | +LL | let _ = a.into_iter().map(func1).flat_map(|y| func2(y)); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `filter_map` + --> $DIR/map_then_identity_transformer.rs:13:27 + | +LL | let _ = a.into_iter().map(func1).filter_map(|y| y.checked_add(1)); + | ^^^ + | +help: these `map` and `filter_map` can be merged into a single `filter_map` + --> $DIR/map_then_identity_transformer.rs:13:31 + | +LL | let _ = a.into_iter().map(func1).filter_map(|y| y.checked_add(1)); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `fold` + --> $DIR/map_then_identity_transformer.rs:14:27 + | +LL | let _ = a.into_iter().map(func1).fold(1, |pd, x| pd * x + 1); + | ^^^ + | +help: these `map` and `fold` can be merged into a single `fold` + --> $DIR/map_then_identity_transformer.rs:14:31 + | +LL | let _ = a.into_iter().map(func1).fold(1, |pd, x| pd * x + 1); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `map` + --> $DIR/map_then_identity_transformer.rs:15:27 + | +LL | let _ = a.into_iter().map(func1).map(|y| func1(y)); + | ^^^ + | +help: these `map` and `map` can be merged into a single `map` + --> $DIR/map_then_identity_transformer.rs:15:31 + | +LL | let _ = a.into_iter().map(func1).map(|y| func1(y)); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `position` + --> $DIR/map_then_identity_transformer.rs:16:27 + | +LL | let _ = a.into_iter().map(func1).position(|y| y > 0); + | ^^^ + | +help: these `map` and `position` can be merged into a single `position` + --> $DIR/map_then_identity_transformer.rs:16:31 + | +LL | let _ = a.into_iter().map(func1).position(|y| y > 0); + | ^^^^^ apply this ^ to this variable + +error: this `map` can be collapsed into the `all` + --> $DIR/map_then_identity_transformer.rs:20:27 + | +LL | let _ = a.into_iter().map(func1).all(func3); + | ^^^ + | +help: these `map` and `all` can be merged into a single `all` + --> $DIR/map_then_identity_transformer.rs:20:31 + | +LL | let _ = a.into_iter().map(func1).all(func3); + | ^^^^^ ^^^^^ change this to a closure + | | + | and use this in the `all` + +error: this `map` can be collapsed into the `any` + --> $DIR/map_then_identity_transformer.rs:21:27 + | +LL | let _ = a.into_iter().map(func1).any(func3); + | ^^^ + | +help: these `map` and `any` can be merged into a single `any` + --> $DIR/map_then_identity_transformer.rs:21:31 + | +LL | let _ = a.into_iter().map(func1).any(func3); + | ^^^^^ ^^^^^ change this to a closure + | | + | and use this in the `any` + +error: this `map` can be collapsed into the `all` + --> $DIR/map_then_identity_transformer.rs:25:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).all(|y| y > 0); + | ^^^ + | +help: these `map` and `all` can be merged into a single `all` + --> $DIR/map_then_identity_transformer.rs:25:35 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).all(|y| y > 0); + | ^^^^^^^^^^^^ ^ replace this variable + | | + | with this expression + +error: this `map` can be collapsed into the `any` + --> $DIR/map_then_identity_transformer.rs:26:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) * func1(x)).any(|y| y > 0); + | ^^^ + | +help: these `map` and `any` can be merged into a single `any` + --> $DIR/map_then_identity_transformer.rs:26:35 + | +LL | let _ = a.into_iter().map(|x| func1(x) * func1(x)).any(|y| y > 0); + | ^^^^^^^^^^^^^^^^^^^ ^ replace this variable + | | + | with this expression + +error: this `map` can be collapsed into the `fold` + --> $DIR/map_then_identity_transformer.rs:27:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) * func1(x)).fold(1, |pd, x| pd * x + 1); + | ^^^ + | +help: these `map` and `fold` can be merged into a single `fold` + --> $DIR/map_then_identity_transformer.rs:27:35 + | +LL | let _ = a.into_iter().map(|x| func1(x) * func1(x)).fold(1, |pd, x| pd * x + 1); + | ^^^^^^^^^^^^^^^^^^^ with this expression ^ replace this variable + +error: this `map` can be collapsed into the `all` + --> $DIR/map_then_identity_transformer.rs:31:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).all(func3); + | ^^^ + | +help: these `map` and `all` can be merged into a single `all` + --> $DIR/map_then_identity_transformer.rs:31:31 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).all(func3); + | ^^^^^^^^^^^^^^^^ ^^^^^ change this to a closure + | | + | and use this in the `all` + +error: this `map` can be collapsed into the `any` + --> $DIR/map_then_identity_transformer.rs:32:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).any(func3); + | ^^^ + | +help: these `map` and `any` can be merged into a single `any` + --> $DIR/map_then_identity_transformer.rs:32:31 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).any(func3); + | ^^^^^^^^^^^^^^^^ ^^^^^ change this to a closure + | | + | and use this in the `any` + +error: this `map` can be collapsed into the `fold` + --> $DIR/map_then_identity_transformer.rs:33:27 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).fold(1, func4); + | ^^^ + | +help: these `map` and `fold` can be merged into a single `fold` + --> $DIR/map_then_identity_transformer.rs:33:31 + | +LL | let _ = a.into_iter().map(|x| func1(x) + 1).fold(1, func4); + | ^^^^^^^^^^^^^^^^ ^^^^^ change this to a closure + | | + | and use this in the `fold` + +error: aborting due to 16 previous errors +