diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c9066be51c44..2ec375021eb9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -5162,13 +5162,13 @@ impl Methods { }, (sym::filter_map, [arg]) => { unused_enumerate_index::check(cx, expr, recv, arg); - unnecessary_filter_map::check(cx, expr, arg, name); + unnecessary_filter_map::check(cx, expr, arg, call_span, unnecessary_filter_map::Kind::FilterMap); filter_map_bool_then::check(cx, expr, arg, call_span); filter_map_identity::check(cx, expr, arg, span); }, (sym::find_map, [arg]) => { unused_enumerate_index::check(cx, expr, recv, arg); - unnecessary_filter_map::check(cx, expr, arg, name); + unnecessary_filter_map::check(cx, expr, arg, call_span, unnecessary_filter_map::Kind::FindMap); }, (sym::flat_map, [arg]) => { unused_enumerate_index::check(cx, expr, recv, arg); diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index d9d642015063..7f729ac7ca94 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -2,15 +2,15 @@ use super::utils::clone_or_copy_needed; use clippy_utils::diagnostics::span_lint; use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath, MaybeTypeckRes}; use clippy_utils::sym; -use clippy_utils::ty::is_copy; +use clippy_utils::ty::{is_copy, option_arg_ty}; use clippy_utils::usage::mutated_variables; use clippy_utils::visitors::{Descend, for_each_expr_without_closures}; use core::ops::ControlFlow; use rustc_hir as hir; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_lint::LateContext; -use rustc_middle::ty; -use rustc_span::Symbol; +use rustc_span::Span; +use std::fmt::Display; use super::{UNNECESSARY_FILTER_MAP, UNNECESSARY_FIND_MAP}; @@ -18,7 +18,8 @@ pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>, arg: &'tcx hir::Expr<'tcx>, - name: Symbol, + call_span: Span, + kind: Kind, ) { if !cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator) { return; @@ -45,61 +46,88 @@ pub(super) fn check<'tcx>( let in_ty = cx.typeck_results().node_type(body.params[0].hir_id); let sugg = if !found_filtering { // Check if the closure is .filter_map(|x| Some(x)) - if name == sym::filter_map - && let hir::ExprKind::Call(expr, args) = body.value.kind + if kind.is_filter_map() + && let hir::ExprKind::Call(expr, [arg]) = body.value.kind && expr.res(cx).ctor_parent(cx).is_lang_item(cx, OptionSome) - && let hir::ExprKind::Path(_) = args[0].kind + && let hir::ExprKind::Path(_) = arg.kind { span_lint( cx, UNNECESSARY_FILTER_MAP, - expr.span, + call_span, String::from("this call to `.filter_map(..)` is unnecessary"), ); return; } - if name == sym::filter_map { - "map(..)" - } else { - "map(..).next()" + match kind { + Kind::FilterMap => "map(..)", + Kind::FindMap => "map(..).next()", } } else if !found_mapping && !mutates_arg && (!clone_or_copy_needed || is_copy(cx, in_ty)) { - match cx.typeck_results().expr_ty(body.value).kind() { - ty::Adt(adt, subst) - if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) && in_ty == subst.type_at(0) => - { - if name == sym::filter_map { - "filter(..)" - } else { - "find(..)" - } - }, - _ => return, + let ty = cx.typeck_results().expr_ty(body.value); + if option_arg_ty(cx, ty).is_some_and(|t| t == in_ty) { + match kind { + Kind::FilterMap => "filter(..)", + Kind::FindMap => "find(..)", + } + } else { + return; } } else { return; }; span_lint( cx, - if name == sym::filter_map { - UNNECESSARY_FILTER_MAP - } else { - UNNECESSARY_FIND_MAP + match kind { + Kind::FilterMap => UNNECESSARY_FILTER_MAP, + Kind::FindMap => UNNECESSARY_FIND_MAP, }, - expr.span, - format!("this `.{name}(..)` can be written more simply using `.{sugg}`"), + call_span, + format!("this `.{kind}(..)` can be written more simply using `.{sugg}`"), ); } } +#[derive(Clone, Copy)] +pub(super) enum Kind { + FilterMap, + FindMap, +} + +impl Kind { + fn is_filter_map(self) -> bool { + matches!(self, Self::FilterMap) + } +} + +impl Display for Kind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::FilterMap => f.write_str("filter_map"), + Self::FindMap => f.write_str("find_map"), + } + } +} + // returns (found_mapping, found_filtering) fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tcx hir::Expr<'_>) -> (bool, bool) { match expr.kind { + hir::ExprKind::Path(ref path) + if cx + .qpath_res(path, expr.hir_id) + .ctor_parent(cx) + .is_lang_item(cx, OptionNone) => + { + // None + (false, true) + }, hir::ExprKind::Call(func, args) => { if func.res(cx).ctor_parent(cx).is_lang_item(cx, OptionSome) { if args[0].res_local_id() == Some(arg_id) { + // Some(arg_id) return (false, false); } + // Some(not arg_id) return (true, false); } (true, true) @@ -109,8 +137,10 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc && cx.typeck_results().expr_ty(recv).is_bool() && arg.res_local_id() == Some(arg_id) { + // bool.then_some(arg_id) (false, true) } else { + // bool.then_some(not arg_id) (true, true) } }, @@ -134,14 +164,6 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc let else_check = check_expression(cx, arg_id, else_arm); (if_check.0 | else_check.0, if_check.1 | else_check.1) }, - hir::ExprKind::Path(ref path) - if cx - .qpath_res(path, expr.hir_id) - .ctor_parent(cx) - .is_lang_item(cx, OptionNone) => - { - (false, true) - }, _ => (true, true), } } diff --git a/tests/ui/unnecessary_filter_map.stderr b/tests/ui/unnecessary_filter_map.stderr index a879633e10f2..8c33c08c267d 100644 --- a/tests/ui/unnecessary_filter_map.stderr +++ b/tests/ui/unnecessary_filter_map.stderr @@ -1,17 +1,17 @@ error: this `.filter_map(..)` can be written more simply using `.filter(..)` - --> tests/ui/unnecessary_filter_map.rs:4:13 + --> tests/ui/unnecessary_filter_map.rs:4:20 | LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unnecessary-filter-map` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unnecessary_filter_map)]` error: this `.filter_map(..)` can be written more simply using `.filter(..)` - --> tests/ui/unnecessary_filter_map.rs:7:13 + --> tests/ui/unnecessary_filter_map.rs:7:20 | LL | let _ = (0..4).filter_map(|x| { - | _____________^ + | ____________________^ LL | | LL | | LL | | if x > 1 { @@ -21,10 +21,10 @@ LL | | }); | |______^ error: this `.filter_map(..)` can be written more simply using `.filter(..)` - --> tests/ui/unnecessary_filter_map.rs:15:13 + --> tests/ui/unnecessary_filter_map.rs:15:20 | LL | let _ = (0..4).filter_map(|x| match x { - | _____________^ + | ____________________^ LL | | LL | | 0 | 1 => None, LL | | _ => Some(x), @@ -32,22 +32,22 @@ LL | | }); | |______^ error: this `.filter_map(..)` can be written more simply using `.map(..)` - --> tests/ui/unnecessary_filter_map.rs:21:13 + --> tests/ui/unnecessary_filter_map.rs:21:20 | LL | let _ = (0..4).filter_map(|x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this call to `.filter_map(..)` is unnecessary - --> tests/ui/unnecessary_filter_map.rs:28:61 + --> tests/ui/unnecessary_filter_map.rs:28:46 | LL | let _ = vec![Some(10), None].into_iter().filter_map(|x| Some(x)); - | ^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^ error: this `.filter_map(..)` can be written more simply using `.filter(..)` - --> tests/ui/unnecessary_filter_map.rs:166:14 + --> tests/ui/unnecessary_filter_map.rs:166:33 | LL | let _x = std::iter::once(1).filter_map(|n| (n > 1).then_some(n)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 6 previous errors diff --git a/tests/ui/unnecessary_find_map.rs b/tests/ui/unnecessary_find_map.rs index 33ba7074d623..c5a8937488d9 100644 --- a/tests/ui/unnecessary_find_map.rs +++ b/tests/ui/unnecessary_find_map.rs @@ -1,5 +1,3 @@ -#![allow(dead_code)] - fn main() { let _ = (0..4).find_map(|x| if x > 1 { Some(x) } else { None }); //~^ unnecessary_find_map diff --git a/tests/ui/unnecessary_find_map.stderr b/tests/ui/unnecessary_find_map.stderr index 3754a3d99538..8a269119df22 100644 --- a/tests/ui/unnecessary_find_map.stderr +++ b/tests/ui/unnecessary_find_map.stderr @@ -1,17 +1,17 @@ error: this `.find_map(..)` can be written more simply using `.find(..)` - --> tests/ui/unnecessary_find_map.rs:4:13 + --> tests/ui/unnecessary_find_map.rs:2:20 | LL | let _ = (0..4).find_map(|x| if x > 1 { Some(x) } else { None }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::unnecessary-find-map` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unnecessary_find_map)]` error: this `.find_map(..)` can be written more simply using `.find(..)` - --> tests/ui/unnecessary_find_map.rs:7:13 + --> tests/ui/unnecessary_find_map.rs:5:20 | LL | let _ = (0..4).find_map(|x| { - | _____________^ + | ____________________^ LL | | LL | | LL | | if x > 1 { @@ -21,10 +21,10 @@ LL | | }); | |______^ error: this `.find_map(..)` can be written more simply using `.find(..)` - --> tests/ui/unnecessary_find_map.rs:15:13 + --> tests/ui/unnecessary_find_map.rs:13:20 | LL | let _ = (0..4).find_map(|x| match x { - | _____________^ + | ____________________^ LL | | LL | | 0 | 1 => None, LL | | _ => Some(x), @@ -32,16 +32,16 @@ LL | | }); | |______^ error: this `.find_map(..)` can be written more simply using `.map(..).next()` - --> tests/ui/unnecessary_find_map.rs:21:13 + --> tests/ui/unnecessary_find_map.rs:19:20 | LL | let _ = (0..4).find_map(|x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `.find_map(..)` can be written more simply using `.find(..)` - --> tests/ui/unnecessary_find_map.rs:33:14 + --> tests/ui/unnecessary_find_map.rs:31:33 | LL | let _x = std::iter::once(1).find_map(|n| (n > 1).then_some(n)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: aborting due to 5 previous errors