Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unnecessary_filter_map return of Some arg #12766

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions clippy_lints/src/methods/unnecessary_filter_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ use clippy_utils::diagnostics::span_lint;
use clippy_utils::ty::is_copy;
use clippy_utils::usage::mutated_variables;
use clippy_utils::visitors::{for_each_expr, Descend};
use clippy_utils::{is_res_lang_ctor, is_trait_method, path_res, path_to_local_id};
use clippy_utils::{is_res_lang_ctor, is_trait_method, path_res, path_to_local_id, MaybePath};
use core::ops::ControlFlow;
use rustc_hir as hir;
use rustc_hir::LangItem::{OptionNone, OptionSome};
use rustc_lint::LateContext;
use rustc_middle::query::Key;
use rustc_middle::ty;
use rustc_span::sym;

Expand All @@ -17,7 +18,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>, a
if !is_trait_method(cx, expr, sym::Iterator) {
return;
}

if let hir::ExprKind::Closure(&hir::Closure { body, .. }) = arg.kind {
let body = cx.tcx.hir().body(body);
let arg_id = body.params[0].pat.hir_id;
Expand All @@ -36,9 +36,20 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>, a
ControlFlow::Continue(Descend::Yes)
}
});

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 == "filter_map"
&& let hir::ExprKind::Call(expr, args) = body.value.kind
{
if is_res_lang_ctor(cx, path_res(cx, expr), OptionSome)
&& arg_id.ty_def_id() == args[0].hir_id().ty_def_id()
{
if let hir::ExprKind::Path(_) = args[0].kind {
Comment on lines +45 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if is_res_lang_ctor(cx, path_res(cx, expr), OptionSome)
&& arg_id.ty_def_id() == args[0].hir_id().ty_def_id()
{
if let hir::ExprKind::Path(_) = args[0].kind {
if is_res_lang_ctor(cx, path_res(cx, expr), OptionSome)
&& arg_id.ty_def_id() == args[0].hir_id().ty_def_id()
&& let hir::ExprKind::Path(_) = args[0].kind {

let chains for the win!

span_lint(cx, UNNECESSARY_FILTER_MAP, arg.span, &format!("{name} is unnecessary"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it just occurs to me that... could this be span_lint_with_sugg instead 🤔 , suggesting removing the filter_map?

same thing applies here, while suggest replacing the method call, with MaybeIncorrect applicability for both ofc...

}
}
}
if name == "filter_map" { "map" } else { "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() {
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/unnecessary_filter_map.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//@no-rustfix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the whole test rustfix-incompatible? If not, could we split our a -fix test file?

#![allow(dead_code)]

fn main() {
Expand All @@ -21,6 +22,8 @@ fn main() {
//~^ ERROR: this `.filter_map` can be written more simply using `.map`

let _ = (0..4).filter_map(i32::checked_abs);

let _ = vec![Some(10), None].into_iter().filter_map(|x| Some(x));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked issue also mentions passing a path to Some, so maybe we should add this as a test too

let _ = (0..4).filter_map(Some);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more tests the merrier 😎

}

fn filter_map_none_changes_item_type() -> impl Iterator<Item = bool> {
Expand Down
33 changes: 27 additions & 6 deletions tests/ui/unnecessary_filter_map.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
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:5:13
|
LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -8,7 +8,7 @@ LL | let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
= 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:8:13
|
LL | let _ = (0..4).filter_map(|x| {
| _____________^
Expand All @@ -21,7 +21,7 @@ LL | | });
| |______^

error: this `.filter_map` can be written more simply using `.filter`
--> tests/ui/unnecessary_filter_map.rs:14:13
--> tests/ui/unnecessary_filter_map.rs:15:13
|
LL | let _ = (0..4).filter_map(|x| match x {
| _____________^
Expand All @@ -32,16 +32,37 @@ LL | | });
| |______^

error: this `.filter_map` can be written more simply using `.map`
--> tests/ui/unnecessary_filter_map.rs:20:13
--> tests/ui/unnecessary_filter_map.rs:21:13
|
LL | let _ = (0..4).filter_map(|x| Some(x + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: filter_map is unnecessary
--> tests/ui/unnecessary_filter_map.rs:26:57
|
LL | let _ = vec![Some(10), None].into_iter().filter_map(|x| Some(x));
| ^^^^^^^^^^^

error: this `.filter_map` can be written more simply using `.map`
--> tests/ui/unnecessary_filter_map.rs:26:13
|
LL | let _ = vec![Some(10), None].into_iter().filter_map(|x| Some(x));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: redundant closure
--> tests/ui/unnecessary_filter_map.rs:26:57
|
LL | let _ = vec![Some(10), None].into_iter().filter_map(|x| Some(x));
| ^^^^^^^^^^^ help: replace the closure with the function itself: `Some`
|
= note: `-D clippy::redundant-closure` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::redundant_closure)]`

error: this `.filter_map` can be written more simply using `.filter`
--> tests/ui/unnecessary_filter_map.rs:160:14
--> tests/ui/unnecessary_filter_map.rs:163:14
|
LL | let _x = std::iter::once(1).filter_map(|n| (n > 1).then_some(n));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 5 previous errors
error: aborting due to 8 previous errors

Loading