Skip to content

Lint simple expressions in manual_filter_map, manual_find_map #8958

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.

Sign up for GitHub

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

Merged
bors merged 1 commit into from
Jul 7, 2022
Merged

Lint simple expressions in manual_filter_map, manual_find_map #8958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
107 changes: 54 additions & 53 deletions clippy_lints/src/methods/filter_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_hir::{Expr, ExprKind, PatKind, PathSegment, QPath, UnOp};
use rustc_lint::LateContext;
use rustc_middle::ty::adjustment::Adjust;
use rustc_span::source_map::Span;
use rustc_span::symbol::{sym, Symbol};
use std::borrow::Cow;
Expand Down Expand Up @@ -49,35 +50,18 @@ fn is_option_filter_map<'tcx>(cx: &LateContext<'tcx>, filter_arg: &hir::Expr<'_>
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
}

/// lint use of `filter().map()` for `Iterators`
fn lint_filter_some_map_unwrap(
/// is `filter(|x| x.is_some()).map(|x| x.unwrap())`
fn is_filter_some_map_unwrap(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
filter_recv: &hir::Expr<'_>,
filter_arg: &hir::Expr<'_>,
map_arg: &hir::Expr<'_>,
target_span: Span,
methods_span: Span,
) {
) -> bool {
Copy link
Contributor

@giraffate giraffate Jul 7, 2022

Choose a reason for hiding this comment

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

IMO it would be better to separate function into one that check and return bool and one that just emit the lint. I was confused that the function name lint_filter_some_map_unwrap return bool.

Copy link
Member Author

@Alexendoo Alexendoo Jul 7, 2022

Choose a reason for hiding this comment

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

Thanks, changed it to be an is_ function

let iterator = is_trait_method(cx, expr, sym::Iterator);
let option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(filter_recv), sym::Option);
if (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg) {
let msg = "`filter` for `Some` followed by `unwrap`";
let help = "consider using `flatten` instead";
let sugg = format!(
"{}",
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, target_span),)
);
span_lint_and_sugg(
cx,
OPTION_FILTER_MAP,
methods_span,
msg,
help,
sugg,
Applicability::MachineApplicable,
);
}

(iterator || option) && is_option_filter_map(cx, filter_arg, map_arg)
}

/// lint use of `filter().map()` or `find().map()` for `Iterators`
Expand All @@ -93,15 +77,20 @@ pub(super) fn check<'tcx>(
map_span: Span,
is_find: bool,
) {
lint_filter_some_map_unwrap(
cx,
expr,
filter_recv,
filter_arg,
map_arg,
map_span,
filter_span.with_hi(expr.span.hi()),
);
if is_filter_some_map_unwrap(cx, expr, filter_recv, filter_arg, map_arg) {
span_lint_and_sugg(
cx,
OPTION_FILTER_MAP,
filter_span.with_hi(expr.span.hi()),
"`filter` for `Some` followed by `unwrap`",
"consider using `flatten` instead",
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(),
Applicability::MachineApplicable,
);

return;
}

if_chain! {
if is_trait_method(cx, map_recv, sym::Iterator);

Expand All @@ -118,7 +107,7 @@ pub(super) fn check<'tcx>(
// closure ends with is_some() or is_ok()
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
if let ExprKind::MethodCall(path, [filter_arg], _) = filter_body.value.kind;
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).ty_adt_def();
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).peel_refs().ty_adt_def();
if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::Option, opt_ty.did()) {
Some(false)
} else if cx.tcx.is_diagnostic_item(sym::Result, opt_ty.did()) {
Expand All @@ -137,6 +126,19 @@ pub(super) fn check<'tcx>(
if let ExprKind::MethodCall(seg, [map_arg, ..], _) = map_body.value.kind;
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);

// .filter(..).map(|y| f(y).copied().unwrap())
// ~~~~
let map_arg_peeled = match map_arg.kind {
ExprKind::MethodCall(method, [original_arg], _) if acceptable_methods(method) => {
original_arg
},
_ => map_arg,
};

// .filter(|x| x.is_some()).map(|y| y[.acceptable_method()].unwrap())
let simple_equal = path_to_local_id(filter_arg, filter_param_id)
&& path_to_local_id(map_arg_peeled, map_param_id);

let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
// in `filter(|x| ..)`, replace `*x` with `x`
let a_path = if_chain! {
Expand All @@ -145,36 +147,35 @@ pub(super) fn check<'tcx>(
then { expr_path } else { a }
};
// let the filter closure arg and the map closure arg be equal
if_chain! {
if path_to_local_id(a_path, filter_param_id);
if path_to_local_id(b, map_param_id);
if cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b);
then {
return true;
}
}
false
};

if match map_arg.kind {
ExprKind::MethodCall(method, [original_arg], _) => {
acceptable_methods(method)
&& SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, original_arg)
},
_ => SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg)
path_to_local_id(a_path, filter_param_id)
&& path_to_local_id(b, map_param_id)
&& cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
};

if simple_equal || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg_peeled);
then {
let span = filter_span.with_hi(expr.span.hi());
let (filter_name, lint) = if is_find {
("find", MANUAL_FIND_MAP)
} else {
("filter", MANUAL_FILTER_MAP)
};
let msg = format!("`{}(..).map(..)` can be simplified as `{0}_map(..)`", filter_name);
let to_opt = if is_result { ".ok()" } else { "" };
let sugg = format!("{}_map(|{}| {}{})", filter_name, map_param_ident,
snippet(cx, map_arg.span, ".."), to_opt);
let msg = format!("`{filter_name}(..).map(..)` can be simplified as `{filter_name}_map(..)`");
let (to_opt, deref) = if is_result {
(".ok()", String::new())
} else {
let derefs = cx.typeck_results()
.expr_adjustments(map_arg)
.iter()
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
.count();

("", "*".repeat(derefs))
};
let sugg = format!(
"{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
snippet(cx, map_arg.span, ".."),
);
span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable);
}
}
Expand Down
34 changes: 34 additions & 0 deletions tests/ui/manual_filter_map.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,32 @@ fn main() {

// is_ok(), unwrap_or()
let _ = (0..).filter_map(|a| to_res(a).ok());

let _ = (1..5)
.filter_map(|y| *to_ref(to_opt(y)));
let _ = (1..5)
.filter_map(|y| *to_ref(to_opt(y)));

let _ = (1..5)
.filter_map(|y| to_ref(to_res(y)).ok());
let _ = (1..5)
.filter_map(|y| to_ref(to_res(y)).ok());
}

#[rustfmt::skip]
fn simple_equal() {
iter::<Option<&u8>>().find_map(|x| x.cloned());
iter::<&Option<&u8>>().find_map(|x| x.cloned());
iter::<&Option<String>>().find_map(|x| x.as_deref());
iter::<Option<&String>>().find_map(|y| to_ref(y).cloned());

iter::<Result<u8, ()>>().find_map(|x| x.ok());
iter::<&Result<u8, ()>>().find_map(|x| x.ok());
iter::<&&Result<u8, ()>>().find_map(|x| x.ok());
iter::<Result<&u8, ()>>().find_map(|x| x.cloned().ok());
iter::<&Result<&u8, ()>>().find_map(|x| x.cloned().ok());
iter::<&Result<String, ()>>().find_map(|x| x.as_deref().ok());
iter::<Result<&String, ()>>().find_map(|y| to_ref(y).cloned().ok());
}

fn no_lint() {
Expand All @@ -28,6 +54,10 @@ fn no_lint() {
.map(|a| to_opt(a).unwrap());
}

fn iter<T>() -> impl Iterator<Item = T> {
std::iter::empty()
}

fn to_opt<T>(_: T) -> Option<T> {
unimplemented!()
}
Expand All @@ -36,6 +66,10 @@ fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

fn to_ref<'a, T>(_: T) -> &'a T {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
Expand Down
38 changes: 38 additions & 0 deletions tests/ui/manual_filter_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,36 @@ fn main() {

// is_ok(), unwrap_or()
let _ = (0..).filter(|&n| to_res(n).is_ok()).map(|a| to_res(a).unwrap_or(1));

let _ = (1..5)
.filter(|&x| to_ref(to_opt(x)).is_some())
.map(|y| to_ref(to_opt(y)).unwrap());
let _ = (1..5)
.filter(|x| to_ref(to_opt(*x)).is_some())
.map(|y| to_ref(to_opt(y)).unwrap());

let _ = (1..5)
.filter(|&x| to_ref(to_res(x)).is_ok())
.map(|y| to_ref(to_res(y)).unwrap());
let _ = (1..5)
.filter(|x| to_ref(to_res(*x)).is_ok())
.map(|y| to_ref(to_res(y)).unwrap());
}

#[rustfmt::skip]
fn simple_equal() {
iter::<Option<&u8>>().find(|x| x.is_some()).map(|x| x.cloned().unwrap());
iter::<&Option<&u8>>().find(|x| x.is_some()).map(|x| x.cloned().unwrap());
iter::<&Option<String>>().find(|x| x.is_some()).map(|x| x.as_deref().unwrap());
iter::<Option<&String>>().find(|&x| to_ref(x).is_some()).map(|y| to_ref(y).cloned().unwrap());

iter::<Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
iter::<&Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
iter::<&&Result<u8, ()>>().find(|x| x.is_ok()).map(|x| x.unwrap());
iter::<Result<&u8, ()>>().find(|x| x.is_ok()).map(|x| x.cloned().unwrap());
iter::<&Result<&u8, ()>>().find(|x| x.is_ok()).map(|x| x.cloned().unwrap());
iter::<&Result<String, ()>>().find(|x| x.is_ok()).map(|x| x.as_deref().unwrap());
iter::<Result<&String, ()>>().find(|&x| to_ref(x).is_ok()).map(|y| to_ref(y).cloned().unwrap());
}

fn no_lint() {
Expand All @@ -28,6 +58,10 @@ fn no_lint() {
.map(|a| to_opt(a).unwrap());
}

fn iter<T>() -> impl Iterator<Item = T> {
std::iter::empty()
}

fn to_opt<T>(_: T) -> Option<T> {
unimplemented!()
}
Expand All @@ -36,6 +70,10 @@ fn to_res<T>(_: T) -> Result<T, ()> {
unimplemented!()
}

fn to_ref<'a, T>(_: T) -> &'a T {
unimplemented!()
}

struct Issue8920<'a> {
option_field: Option<String>,
result_field: Result<String, ()>,
Expand Down
Loading