diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 48ab42d587c8..b647dbdc8468 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -134,9 +134,8 @@ mod unnecessary_lazy_eval; mod unnecessary_literal_unwrap; mod unnecessary_map_or; +mod unnecessary_map_or_else; mod unnecessary_min_or_max; -mod unnecessary_option_map_or_else; -mod unnecessary_result_map_or_else; mod unnecessary_sort_by; mod unnecessary_to_owned; mod unwrap_expect_used; @@ -4347,6 +4346,7 @@ } declare_clippy_lint! { + /// ### What it does /// Checks for usage of `.map_or_else()` "map closure" for `Option` type. /// /// ### Why is this bad? @@ -5449,8 +5449,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }, (sym::map_or_else, [def, map]) => { result_map_or_else_none::check(cx, expr, recv, def, map); - unnecessary_option_map_or_else::check(cx, expr, recv, def, map); - unnecessary_result_map_or_else::check(cx, expr, recv, def, map); + unnecessary_map_or_else::check(cx, expr, recv, def, map, call_span); }, (sym::next, []) => { if let Some((name2, recv2, args2, _, _)) = method_call(recv) { diff --git a/clippy_lints/src/methods/unnecessary_map_or_else.rs b/clippy_lints/src/methods/unnecessary_map_or_else.rs new file mode 100644 index 000000000000..7c1c31d2bd38 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_map_or_else.rs @@ -0,0 +1,39 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::is_expr_identity_function; +use clippy_utils::res::MaybeDef; +use clippy_utils::source::snippet_with_applicability; +use rustc_errors::Applicability; +use rustc_hir::Expr; +use rustc_lint::LateContext; +use rustc_span::Span; +use rustc_span::symbol::sym; + +use super::{UNNECESSARY_OPTION_MAP_OR_ELSE, UNNECESSARY_RESULT_MAP_OR_ELSE}; + +/// lint use of `_.map_or_else(|err| err, |n| n)` for `Result`s and `Option`s. +pub(super) fn check( + cx: &LateContext<'_>, + expr: &Expr<'_>, + recv: &Expr<'_>, + def_arg: &Expr<'_>, + map_arg: &Expr<'_>, + call_span: Span, +) { + let (symbol, lint) = match cx.typeck_results().expr_ty(recv).opt_diag_name(cx) { + Some(x @ sym::Result) => (x, UNNECESSARY_RESULT_MAP_OR_ELSE), + Some(x @ sym::Option) => (x, UNNECESSARY_OPTION_MAP_OR_ELSE), + _ => return, + }; + + if is_expr_identity_function(cx, map_arg) { + let msg = format!("unused \"map closure\" when calling `{symbol}::map_or_else` value"); + + span_lint_and_then(cx, lint, expr.span, msg, |diag| { + let mut applicability = Applicability::MachineApplicable; + let err_snippet = snippet_with_applicability(cx, def_arg.span, "..", &mut applicability); + let sugg = format!("unwrap_or_else({err_snippet})"); + + diag.span_suggestion_verbose(call_span, "consider using `unwrap_or_else`", sugg, applicability); + }); + } +} diff --git a/clippy_lints/src/methods/unnecessary_option_map_or_else.rs b/clippy_lints/src/methods/unnecessary_option_map_or_else.rs deleted file mode 100644 index 265619e26376..000000000000 --- a/clippy_lints/src/methods/unnecessary_option_map_or_else.rs +++ /dev/null @@ -1,111 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::res::MaybeDef; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{expr_or_init, find_binding_init, peel_blocks}; -use rustc_errors::Applicability; -use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Body, BodyId, Closure, Expr, ExprKind, HirId, QPath}; -use rustc_lint::LateContext; -use rustc_span::symbol::sym; - -use super::UNNECESSARY_OPTION_MAP_OR_ELSE; -use super::utils::get_last_chain_binding_hir_id; - -fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) { - let msg = "unused \"map closure\" when calling `Option::map_or_else` value"; - let mut applicability = Applicability::MachineApplicable; - let self_snippet = snippet_with_applicability(cx, recv.span, "_", &mut applicability); - let err_snippet = snippet_with_applicability(cx, def_arg.span, "..", &mut applicability); - span_lint_and_sugg( - cx, - UNNECESSARY_OPTION_MAP_OR_ELSE, - expr.span, - msg, - "consider using `unwrap_or_else`", - format!("{self_snippet}.unwrap_or_else({err_snippet})"), - Applicability::MachineApplicable, - ); -} - -fn handle_qpath( - cx: &LateContext<'_>, - expr: &Expr<'_>, - recv: &Expr<'_>, - def_arg: &Expr<'_>, - expected_hir_id: HirId, - qpath: QPath<'_>, -) { - if let QPath::Resolved(_, path) = qpath - && let Res::Local(hir_id) = path.res - && expected_hir_id == hir_id - { - emit_lint(cx, expr, recv, def_arg); - } -} - -fn handle_closure(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body_id: BodyId) { - let body = cx.tcx.hir_body(body_id); - handle_fn_body(cx, expr, recv, def_arg, body); -} - -fn handle_fn_body(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, body: &Body<'_>) { - if let Some(first_param) = body.params.first() { - let body_expr = peel_blocks(body.value); - match body_expr.kind { - ExprKind::Path(qpath) => { - handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath); - }, - // If this is a block (that wasn't peeled off), then it means there are statements. - ExprKind::Block(block, _) => { - if let Some(block_expr) = block.expr - // First we ensure that this is a "binding chain" (each statement is a binding - // of the previous one) and that it is a binding of the closure argument. - && let Some(last_chain_binding_id) = - get_last_chain_binding_hir_id(first_param.pat.hir_id, block.stmts) - && let ExprKind::Path(qpath) = block_expr.kind - { - handle_qpath(cx, expr, recv, def_arg, last_chain_binding_id, qpath); - } - }, - _ => {}, - } - } -} - -/// lint use of `_.map_or_else(|err| err, |n| n)` for `Option`s. -pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>, map_arg: &Expr<'_>) { - // lint if the caller of `map_or_else()` is an `Option` - if !cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Option) { - return; - } - match map_arg.kind { - // If the second argument is a closure, we can check its body. - ExprKind::Closure(&Closure { body, .. }) => { - handle_closure(cx, expr, recv, def_arg, body); - }, - ExprKind::Path(qpath) => { - let res = cx.qpath_res(&qpath, map_arg.hir_id); - match res { - // Case 1: Local variable (could be a closure) - Res::Local(hir_id) => { - if let Some(init_expr) = find_binding_init(cx, hir_id) { - let origin = expr_or_init(cx, init_expr); - if let ExprKind::Closure(&Closure { body, .. }) = origin.kind { - handle_closure(cx, expr, recv, def_arg, body); - } - } - }, - // Case 2: Function definition - Res::Def(DefKind::Fn, def_id) => { - if let Some(local_def_id) = def_id.as_local() - && let Some(body) = cx.tcx.hir_maybe_body_owned_by(local_def_id) - { - handle_fn_body(cx, expr, recv, def_arg, body); - } - }, - _ => (), - } - }, - _ => (), - } -} diff --git a/clippy_lints/src/methods/unnecessary_result_map_or_else.rs b/clippy_lints/src/methods/unnecessary_result_map_or_else.rs deleted file mode 100644 index 1f6bb60414ae..000000000000 --- a/clippy_lints/src/methods/unnecessary_result_map_or_else.rs +++ /dev/null @@ -1,80 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::peel_blocks; -use clippy_utils::res::MaybeDef; -use clippy_utils::source::snippet; -use rustc_errors::Applicability; -use rustc_hir as hir; -use rustc_hir::{Closure, Expr, ExprKind, HirId, QPath}; -use rustc_lint::LateContext; -use rustc_span::symbol::sym; - -use super::UNNECESSARY_RESULT_MAP_OR_ELSE; -use super::utils::get_last_chain_binding_hir_id; - -fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, def_arg: &Expr<'_>) { - let msg = "unused \"map closure\" when calling `Result::map_or_else` value"; - let self_snippet = snippet(cx, recv.span, ".."); - let err_snippet = snippet(cx, def_arg.span, ".."); - span_lint_and_sugg( - cx, - UNNECESSARY_RESULT_MAP_OR_ELSE, - expr.span, - msg, - "consider using `unwrap_or_else`", - format!("{self_snippet}.unwrap_or_else({err_snippet})"), - Applicability::MachineApplicable, - ); -} - -fn handle_qpath( - cx: &LateContext<'_>, - expr: &Expr<'_>, - recv: &Expr<'_>, - def_arg: &Expr<'_>, - expected_hir_id: HirId, - qpath: QPath<'_>, -) { - if let QPath::Resolved(_, path) = qpath - && let hir::def::Res::Local(hir_id) = path.res - && expected_hir_id == hir_id - { - emit_lint(cx, expr, recv, def_arg); - } -} - -/// lint use of `_.map_or_else(|err| err, |n| n)` for `Result`s. -pub(super) fn check<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - recv: &'tcx Expr<'_>, - def_arg: &'tcx Expr<'_>, - map_arg: &'tcx Expr<'_>, -) { - // lint if the caller of `map_or_else()` is a `Result` - if cx.typeck_results().expr_ty(recv).is_diag_item(cx, sym::Result) - && let ExprKind::Closure(&Closure { body, .. }) = map_arg.kind - && let body = cx.tcx.hir_body(body) - && let Some(first_param) = body.params.first() - { - let body_expr = peel_blocks(body.value); - - match body_expr.kind { - ExprKind::Path(qpath) => { - handle_qpath(cx, expr, recv, def_arg, first_param.pat.hir_id, qpath); - }, - // If this is a block (that wasn't peeled off), then it means there are statements. - ExprKind::Block(block, _) => { - if let Some(block_expr) = block.expr - // First we ensure that this is a "binding chain" (each statement is a binding - // of the previous one) and that it is a binding of the closure argument. - && let Some(last_chain_binding_id) = - get_last_chain_binding_hir_id(first_param.pat.hir_id, block.stmts) - && let ExprKind::Path(qpath) = block_expr.kind - { - handle_qpath(cx, expr, recv, def_arg, last_chain_binding_id, qpath); - } - }, - _ => {}, - } - } -} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index f502dfd6d388..310317d2c0d2 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1840,13 +1840,38 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { /// * `|[x, y]| [x, y]` /// * `|Foo(bar, baz)| Foo(bar, baz)` /// * `|Foo { bar, baz }| Foo { bar, baz }` +/// * `|x| { let y = x; ...; let z = y; z }` +/// * `|x| { let y = x; ...; let z = y; return z }` /// /// Consider calling [`is_expr_untyped_identity_function`] or [`is_expr_identity_function`] instead. -fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool { +fn is_body_identity_function<'hir>(cx: &LateContext<'_>, func: &Body<'hir>) -> bool { let [param] = func.params else { return false; }; + let mut param_pat = param.pat; + + // Given a sequence of `Stmt`s of the form `let p = e` where `e` is an expr identical to the + // current `param_pat`, advance the current `param_pat` to `p`. + // + // Note: This is similar to `clippy_utils::get_last_chain_binding_hir_id`, but it works + // directly over a `Pattern` rather than a `HirId`. And it checks for compatibility via + // `is_expr_identity_of_pat` rather than `HirId` equality + let mut advance_param_pat_over_stmts = |stmts: &[Stmt<'hir>]| { + for stmt in stmts { + if let StmtKind::Let(local) = stmt.kind + && let Some(init) = local.init + && is_expr_identity_of_pat(cx, param_pat, init, true) + { + param_pat = local.pat; + } else { + return false; + } + } + + true + }; + let mut expr = func.value; loop { match expr.kind { @@ -1875,7 +1900,30 @@ fn is_body_identity_function(cx: &LateContext<'_>, func: &Body<'_>) -> bool { return false; } }, - _ => return is_expr_identity_of_pat(cx, param.pat, expr, true), + ExprKind::Block( + &Block { + stmts, expr: Some(e), .. + }, + _, + ) => { + if !advance_param_pat_over_stmts(stmts) { + return false; + } + + expr = e; + }, + ExprKind::Block(&Block { stmts, expr: None, .. }, _) => { + if let Some((last_stmt, stmts)) = stmts.split_last() + && advance_param_pat_over_stmts(stmts) + && let StmtKind::Semi(e) | StmtKind::Expr(e) = last_stmt.kind + && let ExprKind::Ret(Some(ret_val)) = e.kind + { + expr = ret_val; + } else { + return false; + } + }, + _ => return is_expr_identity_of_pat(cx, param_pat, expr, true), } } } diff --git a/tests/ui/eta.fixed b/tests/ui/eta.fixed index 41e5bbe3cbed..36300aa56eb3 100644 --- a/tests/ui/eta.fixed +++ b/tests/ui/eta.fixed @@ -13,7 +13,8 @@ clippy::uninlined_format_args, clippy::useless_vec, clippy::unnecessary_map_on_constructor, - clippy::needless_lifetimes + clippy::needless_lifetimes, + clippy::unnecessary_option_map_or_else )] use std::path::{Path, PathBuf}; diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index de0d3fb63c51..63b441851ec1 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -13,7 +13,8 @@ clippy::uninlined_format_args, clippy::useless_vec, clippy::unnecessary_map_on_constructor, - clippy::needless_lifetimes + clippy::needless_lifetimes, + clippy::unnecessary_option_map_or_else )] use std::path::{Path, PathBuf}; diff --git a/tests/ui/eta.stderr b/tests/ui/eta.stderr index c19947cddafa..b57038963b7e 100644 --- a/tests/ui/eta.stderr +++ b/tests/ui/eta.stderr @@ -1,5 +1,5 @@ error: redundant closure - --> tests/ui/eta.rs:34:27 + --> tests/ui/eta.rs:35:27 | LL | let a = Some(1u8).map(|a| foo(a)); | ^^^^^^^^^^ help: replace the closure with the function itself: `foo` @@ -8,31 +8,31 @@ LL | let a = Some(1u8).map(|a| foo(a)); = help: to override `-D warnings` add `#[allow(clippy::redundant_closure)]` error: redundant closure - --> tests/ui/eta.rs:39:40 + --> tests/ui/eta.rs:40:40 | LL | let _: Option> = true.then(|| vec![]); // special case vec! | ^^^^^^^^^ help: replace the closure with `Vec::new`: `std::vec::Vec::new` error: redundant closure - --> tests/ui/eta.rs:42:35 + --> tests/ui/eta.rs:43:35 | LL | let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted? | ^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo2` error: redundant closure - --> tests/ui/eta.rs:45:26 + --> tests/ui/eta.rs:46:26 | LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted | ^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `below` error: redundant closure - --> tests/ui/eta.rs:54:27 + --> tests/ui/eta.rs:55:27 | LL | let e = Some(1u8).map(|a| generic(a)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `generic` error: redundant closure - --> tests/ui/eta.rs:107:51 + --> tests/ui/eta.rs:108:51 | LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo()); | ^^^^^^^^^^^ help: replace the closure with the method itself: `TestStruct::foo` @@ -41,229 +41,229 @@ LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo()); = help: to override `-D warnings` add `#[allow(clippy::redundant_closure_for_method_calls)]` error: redundant closure - --> tests/ui/eta.rs:109:51 + --> tests/ui/eta.rs:110:51 | LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo()); | ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `TestTrait::trait_foo` error: redundant closure - --> tests/ui/eta.rs:112:42 + --> tests/ui/eta.rs:113:42 | LL | let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear()); | ^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::vec::Vec::clear` error: redundant closure - --> tests/ui/eta.rs:117:29 + --> tests/ui/eta.rs:118:29 | LL | let e = Some("str").map(|s| s.to_string()); | ^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::string::ToString::to_string` error: redundant closure - --> tests/ui/eta.rs:119:27 + --> tests/ui/eta.rs:120:27 | LL | let e = Some('a').map(|s| s.to_uppercase()); | ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_uppercase` error: redundant closure - --> tests/ui/eta.rs:122:65 + --> tests/ui/eta.rs:123:65 | LL | let e: std::vec::Vec = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `char::to_ascii_uppercase` error: redundant closure - --> tests/ui/eta.rs:139:23 + --> tests/ui/eta.rs:140:23 | LL | let _ = x.map(|x| x.parse::()); | ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `str::parse::` error: redundant closure - --> tests/ui/eta.rs:192:22 + --> tests/ui/eta.rs:193:22 | LL | requires_fn_once(|| x()); | ^^^^^^ help: replace the closure with the function itself: `x` error: redundant closure - --> tests/ui/eta.rs:200:27 + --> tests/ui/eta.rs:201:27 | LL | let a = Some(1u8).map(|a| foo_ptr(a)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `foo_ptr` error: redundant closure - --> tests/ui/eta.rs:206:27 + --> tests/ui/eta.rs:207:27 | LL | let a = Some(1u8).map(|a| closure(a)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `closure` error: redundant closure - --> tests/ui/eta.rs:239:28 + --> tests/ui/eta.rs:240:28 | LL | x.into_iter().for_each(|x| add_to_res(x)); | ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut add_to_res` error: redundant closure - --> tests/ui/eta.rs:241:28 + --> tests/ui/eta.rs:242:28 | LL | y.into_iter().for_each(|x| add_to_res(x)); | ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut add_to_res` error: redundant closure - --> tests/ui/eta.rs:243:28 + --> tests/ui/eta.rs:244:28 | LL | z.into_iter().for_each(|x| add_to_res(x)); | ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `add_to_res` error: redundant closure - --> tests/ui/eta.rs:251:21 + --> tests/ui/eta.rs:252:21 | LL | Some(1).map(|n| closure(n)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut closure` error: redundant closure - --> tests/ui/eta.rs:256:21 + --> tests/ui/eta.rs:257:21 | LL | Some(1).map(|n| in_loop(n)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `in_loop` error: redundant closure - --> tests/ui/eta.rs:350:18 + --> tests/ui/eta.rs:351:18 | LL | takes_fn_mut(|| f()); | ^^^^^^ help: replace the closure with the function itself: `&mut f` error: redundant closure - --> tests/ui/eta.rs:354:19 + --> tests/ui/eta.rs:355:19 | LL | takes_fn_once(|| f()); | ^^^^^^ help: replace the closure with the function itself: `&mut f` error: redundant closure - --> tests/ui/eta.rs:359:26 + --> tests/ui/eta.rs:360:26 | LL | move || takes_fn_mut(|| f_used_once()) | ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut f_used_once` error: redundant closure - --> tests/ui/eta.rs:372:19 + --> tests/ui/eta.rs:373:19 | LL | array_opt.map(|a| a.as_slice()); | ^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `<[u8; 3]>::as_slice` error: redundant closure - --> tests/ui/eta.rs:376:19 + --> tests/ui/eta.rs:377:19 | LL | slice_opt.map(|s| s.len()); | ^^^^^^^^^^^ help: replace the closure with the method itself: `<[u8]>::len` error: redundant closure - --> tests/ui/eta.rs:380:17 + --> tests/ui/eta.rs:381:17 | LL | ptr_opt.map(|p| p.is_null()); | ^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `<*const usize>::is_null` error: redundant closure - --> tests/ui/eta.rs:385:17 + --> tests/ui/eta.rs:386:17 | LL | dyn_opt.map(|d| d.method_on_dyn()); | ^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `::method_on_dyn` error: redundant closure - --> tests/ui/eta.rs:446:19 + --> tests/ui/eta.rs:447:19 | LL | let _ = f(&0, |x, y| f2(x, y)); | ^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `f2` error: redundant closure - --> tests/ui/eta.rs:475:22 + --> tests/ui/eta.rs:476:22 | LL | test.map(|t| t.method()) | ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `Test::method` error: redundant closure - --> tests/ui/eta.rs:480:22 + --> tests/ui/eta.rs:481:22 | LL | test.map(|t| t.method()) | ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `super::Outer::method` error: redundant closure - --> tests/ui/eta.rs:494:18 + --> tests/ui/eta.rs:495:18 | LL | test.map(|t| t.method()) | ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `test_mod::Test::method` error: redundant closure - --> tests/ui/eta.rs:502:30 + --> tests/ui/eta.rs:503:30 | LL | test.map(|t| t.method()) | ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `crate::issue_10854::d::Test::method` error: redundant closure - --> tests/ui/eta.rs:522:38 + --> tests/ui/eta.rs:523:38 | LL | let x = Box::new(|| None.map(|x| f(x))); | ^^^^^^^^ help: replace the closure with the function itself: `&f` error: redundant closure - --> tests/ui/eta.rs:527:38 + --> tests/ui/eta.rs:528:38 | LL | let x = Box::new(|| None.map(|x| f(x))); | ^^^^^^^^ help: replace the closure with the function itself: `f` error: redundant closure - --> tests/ui/eta.rs:545:35 + --> tests/ui/eta.rs:546:35 | LL | let _field = bind.or_else(|| get_default()).unwrap(); | ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `get_default` error: redundant closure - --> tests/ui/eta.rs:592:16 + --> tests/ui/eta.rs:593:16 | LL | accepts_fn(|| f()); | ^^^^^^ help: replace the closure with the function itself: `**f` error: redundant closure - --> tests/ui/eta.rs:596:16 + --> tests/ui/eta.rs:597:16 | LL | accepts_fn(|| g()); | ^^^^^^ help: replace the closure with the function itself: `g` error: redundant closure - --> tests/ui/eta.rs:609:16 + --> tests/ui/eta.rs:610:16 | LL | accepts_fn(|| b()); | ^^^^^^ help: replace the closure with the function itself: `***b` error: redundant closure - --> tests/ui/eta.rs:633:14 + --> tests/ui/eta.rs:634:14 | LL | .map(|n| MyError::A(n)) | ^^^^^^^^^^^^^^^^^ help: replace the closure with the tuple variant itself: `MyError::A` error: redundant closure - --> tests/ui/eta.rs:630:14 + --> tests/ui/eta.rs:631:14 | LL | .map(|n| S(n)) | ^^^^^^^^ help: replace the closure with the tuple struct itself: `S` error: redundant closure - --> tests/ui/eta.rs:627:14 + --> tests/ui/eta.rs:628:14 | LL | .map(|n| g(n)) | ^^^^^^^^ help: replace the closure with the function itself: `g` error: redundant closure - --> tests/ui/eta.rs:624:14 + --> tests/ui/eta.rs:625:14 | LL | .map(|n| f(n)) | ^^^^^^^^ help: replace the closure with the function itself: `f` error: redundant closure - --> tests/ui/eta.rs:651:31 + --> tests/ui/eta.rs:652:31 | LL | array.iter().for_each(|item| item.method()); | ^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `Self::method` error: redundant closure - --> tests/ui/eta.rs:661:38 + --> tests/ui/eta.rs:662:38 | LL | (0..10).flat_map(|x| (0..10).map(|y| closure(y))).count(); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&*closure` diff --git a/tests/ui/unnecessary_option_map_or_else.fixed b/tests/ui/unnecessary_option_map_or_else.fixed index 9974ee2d08eb..8d25efbeea00 100644 --- a/tests/ui/unnecessary_option_map_or_else.fixed +++ b/tests/ui/unnecessary_option_map_or_else.fixed @@ -3,13 +3,10 @@ clippy::let_and_return, clippy::let_unit_value, clippy::unnecessary_lazy_evaluations, - clippy::unnecessary_literal_unwrap + clippy::unnecessary_literal_unwrap, + clippy::needless_return )] -const fn identity(x: T) -> T { - x -} - const fn double_it(x: i32) -> i32 { x * 2 } @@ -18,31 +15,42 @@ fn main() { // Expected errors // Basic scenario let option = Some(()); - option.unwrap_or_else(|| ()); //~ ERROR: unused "map closure" when calling + option.unwrap_or_else(|| ()); //~ unnecessary_option_map_or_else // Type ascription let option = Some(()); - option.unwrap_or_else(|| ()); //~ ERROR: unused "map closure" when calling + option.unwrap_or_else(|| ()); //~ unnecessary_option_map_or_else // Auto-deref let string = String::new(); let option = Some(&string); - let _: &str = option.unwrap_or_else(|| &string); //~ ERROR: unused "map closure" when calling + let _: &str = option.map_or_else(|| &string, |x| x); + // This should in theory lint with a smarter check // Temporary variable let option = Some(()); option.unwrap_or_else(|| ()); - // Identity + // Temporary variable with pattern + let option = Some(((), ())); + option.unwrap_or_else(|| ((), ())); + + // std::convert::identity let string = String::new(); let option = Some(&string); - let _: &str = option.unwrap_or_else(|| &string); //~ ERROR: unused "map closure" when calling + let _: &str = option.unwrap_or_else(|| &string); //~ unnecessary_option_map_or_else - // Closure bound to a variable - let do_nothing = |x: String| x; - let string = String::new(); - let option = Some(string.clone()); - let _: String = option.unwrap_or_else(|| string); //~ ERROR: unused "map closure" when calling + let x: Option<((), ())> = Some(((), ())); + x.unwrap_or_else(|| ((), ())); + //~^ unnecessary_option_map_or_else + + // Returned temporary variable. + let x: Option<()> = Some(()); + x.unwrap_or_else(|| ()); + + // Returned temporary variable with pattern + let x: Option<((), ())> = Some(((), ())); + x.unwrap_or_else(|| ((), ())); // Correct usages let option = Some(()); diff --git a/tests/ui/unnecessary_option_map_or_else.rs b/tests/ui/unnecessary_option_map_or_else.rs index 9b53f3fcd521..42d4ad6ac1f8 100644 --- a/tests/ui/unnecessary_option_map_or_else.rs +++ b/tests/ui/unnecessary_option_map_or_else.rs @@ -3,13 +3,10 @@ clippy::let_and_return, clippy::let_unit_value, clippy::unnecessary_lazy_evaluations, - clippy::unnecessary_literal_unwrap + clippy::unnecessary_literal_unwrap, + clippy::needless_return )] -const fn identity(x: T) -> T { - x -} - const fn double_it(x: i32) -> i32 { x * 2 } @@ -18,21 +15,22 @@ fn main() { // Expected errors // Basic scenario let option = Some(()); - option.map_or_else(|| (), |x| x); //~ ERROR: unused "map closure" when calling + option.map_or_else(|| (), |x| x); //~ unnecessary_option_map_or_else // Type ascription let option = Some(()); - option.map_or_else(|| (), |x: ()| x); //~ ERROR: unused "map closure" when calling + option.map_or_else(|| (), |x: ()| x); //~ unnecessary_option_map_or_else // Auto-deref let string = String::new(); let option = Some(&string); - let _: &str = option.map_or_else(|| &string, |x| x); //~ ERROR: unused "map closure" when calling + let _: &str = option.map_or_else(|| &string, |x| x); + // This should in theory lint with a smarter check // Temporary variable let option = Some(()); option.map_or_else( - //~^ ERROR: unused "map closure" when calling + //~^ unnecessary_option_map_or_else || (), |x| { let tmp = x; @@ -40,16 +38,50 @@ fn main() { }, ); - // Identity + // Temporary variable with pattern + let option = Some(((), ())); + option.map_or_else( + //~^ unnecessary_option_map_or_else + || ((), ()), + |x| { + let tmp = x; + let (a, b) = tmp; + (a, b) + }, + ); + + // std::convert::identity let string = String::new(); let option = Some(&string); - let _: &str = option.map_or_else(|| &string, identity); //~ ERROR: unused "map closure" when calling + let _: &str = option.map_or_else(|| &string, std::convert::identity); //~ unnecessary_option_map_or_else - // Closure bound to a variable - let do_nothing = |x: String| x; - let string = String::new(); - let option = Some(string.clone()); - let _: String = option.map_or_else(|| string, do_nothing); //~ ERROR: unused "map closure" when calling + let x: Option<((), ())> = Some(((), ())); + x.map_or_else(|| ((), ()), |(a, b)| (a, b)); + //~^ unnecessary_option_map_or_else + + // Returned temporary variable. + let x: Option<()> = Some(()); + x.map_or_else( + //~^ unnecessary_option_map_or_else + || (), + |n| { + let tmp = n; + let tmp2 = tmp; + return tmp2; + }, + ); + + // Returned temporary variable with pattern + let x: Option<((), ())> = Some(((), ())); + x.map_or_else( + //~^ unnecessary_option_map_or_else + || ((), ()), + |n| { + let tmp = n; + let (a, b) = tmp; + return (a, b); + }, + ); // Correct usages let option = Some(()); diff --git a/tests/ui/unnecessary_option_map_or_else.stderr b/tests/ui/unnecessary_option_map_or_else.stderr index d90875e4efc7..3b579ebdb289 100644 --- a/tests/ui/unnecessary_option_map_or_else.stderr +++ b/tests/ui/unnecessary_option_map_or_else.stderr @@ -1,26 +1,31 @@ error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:21:5 + --> tests/ui/unnecessary_option_map_or_else.rs:18:5 | LL | option.map_or_else(|| (), |x| x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unnecessary-option-map-or-else` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unnecessary_option_map_or_else)]` +help: consider using `unwrap_or_else` + | +LL - option.map_or_else(|| (), |x| x); +LL + option.unwrap_or_else(|| ()); + | error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:25:5 + --> tests/ui/unnecessary_option_map_or_else.rs:22:5 | LL | option.map_or_else(|| (), |x: ()| x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())` - -error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:30:19 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `unwrap_or_else` + | +LL - option.map_or_else(|| (), |x: ()| x); +LL + option.unwrap_or_else(|| ()); | -LL | let _: &str = option.map_or_else(|| &string, |x| x); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| &string)` error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:34:5 + --> tests/ui/unnecessary_option_map_or_else.rs:32:5 | LL | / option.map_or_else( LL | | @@ -29,19 +34,122 @@ LL | | |x| { ... | LL | | }, LL | | ); - | |_____^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| ())` + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - option.map_or_else( +LL - +LL - || (), +LL - |x| { +LL - let tmp = x; +LL - tmp +LL - }, +LL - ); +LL + option.unwrap_or_else(|| ()); + | error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:46:19 + --> tests/ui/unnecessary_option_map_or_else.rs:43:5 + | +LL | / option.map_or_else( +LL | | +LL | | || ((), ()), +LL | | |x| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - option.map_or_else( +LL - +LL - || ((), ()), +LL - |x| { +LL - let tmp = x; +LL - let (a, b) = tmp; +LL - (a, b) +LL - }, +LL - ); +LL + option.unwrap_or_else(|| ((), ())); | -LL | let _: &str = option.map_or_else(|| &string, identity); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| &string)` error: unused "map closure" when calling `Option::map_or_else` value - --> tests/ui/unnecessary_option_map_or_else.rs:52:21 + --> tests/ui/unnecessary_option_map_or_else.rs:56:19 + | +LL | let _: &str = option.map_or_else(|| &string, std::convert::identity); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `unwrap_or_else` + | +LL - let _: &str = option.map_or_else(|| &string, std::convert::identity); +LL + let _: &str = option.unwrap_or_else(|| &string); | -LL | let _: String = option.map_or_else(|| string, do_nothing); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `option.unwrap_or_else(|| string)` -error: aborting due to 6 previous errors +error: unused "map closure" when calling `Option::map_or_else` value + --> tests/ui/unnecessary_option_map_or_else.rs:59:5 + | +LL | x.map_or_else(|| ((), ()), |(a, b)| (a, b)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else(|| ((), ()), |(a, b)| (a, b)); +LL + x.unwrap_or_else(|| ((), ())); + | + +error: unused "map closure" when calling `Option::map_or_else` value + --> tests/ui/unnecessary_option_map_or_else.rs:64:5 + | +LL | / x.map_or_else( +LL | | +LL | | || (), +LL | | |n| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - || (), +LL - |n| { +LL - let tmp = n; +LL - let tmp2 = tmp; +LL - return tmp2; +LL - }, +LL - ); +LL + x.unwrap_or_else(|| ()); + | + +error: unused "map closure" when calling `Option::map_or_else` value + --> tests/ui/unnecessary_option_map_or_else.rs:76:5 + | +LL | / x.map_or_else( +LL | | +LL | | || ((), ()), +LL | | |n| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - || ((), ()), +LL - |n| { +LL - let tmp = n; +LL - let (a, b) = tmp; +LL - return (a, b); +LL - }, +LL - ); +LL + x.unwrap_or_else(|| ((), ())); + | + +error: aborting due to 8 previous errors diff --git a/tests/ui/unnecessary_result_map_or_else.fixed b/tests/ui/unnecessary_result_map_or_else.fixed index 5d7e3fa355fd..55542519bdb7 100644 --- a/tests/ui/unnecessary_result_map_or_else.fixed +++ b/tests/ui/unnecessary_result_map_or_else.fixed @@ -1,5 +1,10 @@ #![warn(clippy::unnecessary_result_map_or_else)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::let_and_return, clippy::let_unit_value)] +#![allow( + clippy::unnecessary_literal_unwrap, + clippy::let_and_return, + clippy::let_unit_value, + clippy::needless_return +)] fn main() { let x: Result<(), ()> = Ok(()); @@ -14,13 +19,17 @@ fn main() { // Auto-deref. let y = String::new(); let x: Result<&String, &String> = Ok(&y); - let y: &str = x.unwrap_or_else(|err| err); - //~^ unnecessary_result_map_or_else + let y: &str = x.map_or_else(|err| err, |n| n); + // This should lint with a smarter check // Temporary variable. let x: Result<(), ()> = Ok(()); x.unwrap_or_else(|err| err); + // Temporary variable with pattern + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.unwrap_or_else(|err| err); + // Should not warn. let x: Result = Ok(0); x.map_or_else(|err| err, |n| n + 1); @@ -61,4 +70,16 @@ fn main() { y }, ); + + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.unwrap_or_else(|err| err); + //~^ unnecessary_result_map_or_else + + // Returned temporary variable. + let x: Result<(), ()> = Ok(()); + x.unwrap_or_else(|err| err); + + // Returned temporary variable with pattern + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.unwrap_or_else(|err| err); } diff --git a/tests/ui/unnecessary_result_map_or_else.rs b/tests/ui/unnecessary_result_map_or_else.rs index d2bab0f9d9c1..21a4826911e5 100644 --- a/tests/ui/unnecessary_result_map_or_else.rs +++ b/tests/ui/unnecessary_result_map_or_else.rs @@ -1,5 +1,10 @@ #![warn(clippy::unnecessary_result_map_or_else)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::let_and_return, clippy::let_unit_value)] +#![allow( + clippy::unnecessary_literal_unwrap, + clippy::let_and_return, + clippy::let_unit_value, + clippy::needless_return +)] fn main() { let x: Result<(), ()> = Ok(()); @@ -15,7 +20,7 @@ fn main() { let y = String::new(); let x: Result<&String, &String> = Ok(&y); let y: &str = x.map_or_else(|err| err, |n| n); - //~^ unnecessary_result_map_or_else + // This should lint with a smarter check // Temporary variable. let x: Result<(), ()> = Ok(()); @@ -29,6 +34,18 @@ fn main() { }, ); + // Temporary variable with pattern + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.map_or_else( + //~^ unnecessary_result_map_or_else + |err| err, + |n| { + let tmp = n; + let (a, b) = tmp; + (a, b) + }, + ); + // Should not warn. let x: Result = Ok(0); x.map_or_else(|err| err, |n| n + 1); @@ -69,4 +86,32 @@ fn main() { y }, ); + + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.map_or_else(|err| err, |(a, b)| (a, b)); + //~^ unnecessary_result_map_or_else + + // Returned temporary variable. + let x: Result<(), ()> = Ok(()); + x.map_or_else( + //~^ unnecessary_result_map_or_else + |err| err, + |n| { + let tmp = n; + let tmp2 = tmp; + return tmp2; + }, + ); + + // Returned temporary variable with pattern + let x: Result<((), ()), ((), ())> = Ok(((), ())); + x.map_or_else( + //~^ unnecessary_result_map_or_else + |err| err, + |n| { + let tmp = n; + let (a, b) = tmp; + return (a, b); + }, + ); } diff --git a/tests/ui/unnecessary_result_map_or_else.stderr b/tests/ui/unnecessary_result_map_or_else.stderr index e6afa50217cc..f76999127c64 100644 --- a/tests/ui/unnecessary_result_map_or_else.stderr +++ b/tests/ui/unnecessary_result_map_or_else.stderr @@ -1,26 +1,31 @@ -error: unused "map closure" when calling `Result::map_or_else` value - --> tests/ui/unnecessary_result_map_or_else.rs:6:5 - | -LL | x.map_or_else(|err| err, |n| n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)` - | - = note: `-D clippy::unnecessary-result-map-or-else` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::unnecessary_result_map_or_else)]` - error: unused "map closure" when calling `Result::map_or_else` value --> tests/ui/unnecessary_result_map_or_else.rs:11:5 | -LL | x.map_or_else(|err: ()| err, |n: ()| n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err: ()| err)` - -error: unused "map closure" when calling `Result::map_or_else` value - --> tests/ui/unnecessary_result_map_or_else.rs:17:19 +LL | x.map_or_else(|err| err, |n| n); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unnecessary-result-map-or-else` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_result_map_or_else)]` +help: consider using `unwrap_or_else` + | +LL - x.map_or_else(|err| err, |n| n); +LL + x.unwrap_or_else(|err| err); | -LL | let y: &str = x.map_or_else(|err| err, |n| n); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)` error: unused "map closure" when calling `Result::map_or_else` value - --> tests/ui/unnecessary_result_map_or_else.rs:22:5 + --> tests/ui/unnecessary_result_map_or_else.rs:16:5 + | +LL | x.map_or_else(|err: ()| err, |n: ()| n); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else(|err: ()| err, |n: ()| n); +LL + x.unwrap_or_else(|err: ()| err); + | + +error: unused "map closure" when calling `Result::map_or_else` value + --> tests/ui/unnecessary_result_map_or_else.rs:27:5 | LL | / x.map_or_else( LL | | @@ -29,7 +34,111 @@ LL | | |n| { ... | LL | | }, LL | | ); - | |_____^ help: consider using `unwrap_or_else`: `x.unwrap_or_else(|err| err)` + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - |err| err, +LL - |n| { +LL - let tmp = n; +LL - let tmp2 = tmp; +LL - tmp2 +LL - }, +LL - ); +LL + x.unwrap_or_else(|err| err); + | -error: aborting due to 4 previous errors +error: unused "map closure" when calling `Result::map_or_else` value + --> tests/ui/unnecessary_result_map_or_else.rs:39:5 + | +LL | / x.map_or_else( +LL | | +LL | | |err| err, +LL | | |n| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - |err| err, +LL - |n| { +LL - let tmp = n; +LL - let (a, b) = tmp; +LL - (a, b) +LL - }, +LL - ); +LL + x.unwrap_or_else(|err| err); + | + +error: unused "map closure" when calling `Result::map_or_else` value + --> tests/ui/unnecessary_result_map_or_else.rs:91:5 + | +LL | x.map_or_else(|err| err, |(a, b)| (a, b)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else(|err| err, |(a, b)| (a, b)); +LL + x.unwrap_or_else(|err| err); + | + +error: unused "map closure" when calling `Result::map_or_else` value + --> tests/ui/unnecessary_result_map_or_else.rs:96:5 + | +LL | / x.map_or_else( +LL | | +LL | | |err| err, +LL | | |n| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - |err| err, +LL - |n| { +LL - let tmp = n; +LL - let tmp2 = tmp; +LL - return tmp2; +LL - }, +LL - ); +LL + x.unwrap_or_else(|err| err); + | + +error: unused "map closure" when calling `Result::map_or_else` value + --> tests/ui/unnecessary_result_map_or_else.rs:108:5 + | +LL | / x.map_or_else( +LL | | +LL | | |err| err, +LL | | |n| { +... | +LL | | }, +LL | | ); + | |_____^ + | +help: consider using `unwrap_or_else` + | +LL - x.map_or_else( +LL - +LL - |err| err, +LL - |n| { +LL - let tmp = n; +LL - let (a, b) = tmp; +LL - return (a, b); +LL - }, +LL - ); +LL + x.unwrap_or_else(|err| err); + | + +error: aborting due to 7 previous errors