-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #6342 - bbqbaron:issue-6061, r=flip1995
Lint: filter(Option::is_some).map(Option::unwrap) Fixes #6061 *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: * add new lint for filter(Option::is_some).map(Option::unwrap) First Rust PR, so I'm sure I've violated some idioms. Happy to change anything. I'm getting one test failure locally -- a stderr diff for `compile_test`. I'm having a hard time seeing how I could be causing it, so I'm tentatively opening this in the hopes that it's an artifact of my local setup against `rustc`. Hoping it can at least still be reviewed in the meantime. I'm gathering that since this is a method lint, and `.filter(...).map(...)` is already checked, the means of implementation needs to be a little different, so I didn't exactly follow the setup boilerplate. My way of checking for method calls seems a little too direct (ie, "is the second element of the expression literally the path for `Option::is_some`?"), but it seems like that's how some other lints work, so I went with it. I'm assuming we're not concerned about, eg, closures that just end up equivalent to `Option::is_some` by eta reduction.
- Loading branch information
Showing
7 changed files
with
279 additions
and
69 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,87 +1,166 @@ | ||
| use clippy_utils::diagnostics::span_lint_and_sugg; | ||
| use clippy_utils::source::snippet; | ||
| use clippy_utils::{is_trait_method, path_to_local_id, SpanlessEq}; | ||
| use clippy_utils::source::{indent_of, reindent_multiline, snippet}; | ||
| use clippy_utils::ty::is_type_diagnostic_item; | ||
| use clippy_utils::{is_trait_method, path_to_local_id, remove_blocks, SpanlessEq}; | ||
| use if_chain::if_chain; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir as hir; | ||
| use rustc_hir::{Expr, ExprKind, PatKind, UnOp}; | ||
| use rustc_hir::def::Res; | ||
| use rustc_hir::{Expr, ExprKind, PatKind, QPath, UnOp}; | ||
| use rustc_lint::LateContext; | ||
| use rustc_middle::ty::TyS; | ||
| use rustc_span::symbol::sym; | ||
| use rustc_span::source_map::Span; | ||
| use rustc_span::symbol::{sym, Symbol}; | ||
| use std::borrow::Cow; | ||
|
|
||
| use super::MANUAL_FILTER_MAP; | ||
| use super::MANUAL_FIND_MAP; | ||
| use super::OPTION_FILTER_MAP; | ||
|
|
||
| fn is_method<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { | ||
| match &expr.kind { | ||
| hir::ExprKind::Path(QPath::TypeRelative(_, ref mname)) => mname.ident.name == method_name, | ||
| hir::ExprKind::Path(QPath::Resolved(_, segments)) => { | ||
| segments.segments.last().unwrap().ident.name == method_name | ||
| }, | ||
| hir::ExprKind::Closure(_, _, c, _, _) => { | ||
| let body = cx.tcx.hir().body(*c); | ||
| let closure_expr = remove_blocks(&body.value); | ||
| let arg_id = body.params[0].pat.hir_id; | ||
| match closure_expr.kind { | ||
| hir::ExprKind::MethodCall(hir::PathSegment { ident, .. }, _, ref args, _) => { | ||
| if_chain! { | ||
| if ident.name == method_name; | ||
| if let hir::ExprKind::Path(path) = &args[0].kind; | ||
| if let Res::Local(ref local) = cx.qpath_res(path, args[0].hir_id); | ||
| then { | ||
| return arg_id == *local | ||
| } | ||
| } | ||
| false | ||
| }, | ||
| _ => false, | ||
| } | ||
| }, | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| fn is_option_filter_map<'tcx>( | ||
| cx: &LateContext<'tcx>, | ||
| filter_arg: &'tcx hir::Expr<'_>, | ||
| map_arg: &'tcx hir::Expr<'_>, | ||
| ) -> bool { | ||
| 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<'tcx>( | ||
| cx: &LateContext<'tcx>, | ||
| expr: &'tcx hir::Expr<'_>, | ||
| filter_recv: &'tcx hir::Expr<'_>, | ||
| filter_arg: &'tcx hir::Expr<'_>, | ||
| map_arg: &'tcx hir::Expr<'_>, | ||
| target_span: Span, | ||
| methods_span: Span, | ||
| ) { | ||
| 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_type); | ||
| 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, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// lint use of `filter().map()` or `find().map()` for `Iterators` | ||
| pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_find: bool) { | ||
| pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_find: bool, target_span: Span) { | ||
| if_chain! { | ||
| if let ExprKind::MethodCall(_, _, [map_recv, map_arg], map_span) = expr.kind; | ||
| if let ExprKind::MethodCall(_, _, [_, filter_arg], filter_span) = map_recv.kind; | ||
| if is_trait_method(cx, map_recv, sym::Iterator); | ||
| if let ExprKind::MethodCall(_, _, [map_recv, map_arg], map_span) = expr.kind; | ||
| if let ExprKind::MethodCall(_, _, [filter_recv, filter_arg], filter_span) = map_recv.kind; | ||
| then { | ||
| lint_filter_some_map_unwrap(cx, expr, filter_recv, filter_arg, | ||
| map_arg, target_span, filter_span.to(map_span)); | ||
| if_chain! { | ||
| if is_trait_method(cx, map_recv, sym::Iterator); | ||
|
|
||
| // filter(|x| ...is_some())... | ||
| if let ExprKind::Closure(_, _, filter_body_id, ..) = filter_arg.kind; | ||
| let filter_body = cx.tcx.hir().body(filter_body_id); | ||
| if let [filter_param] = filter_body.params; | ||
| // optional ref pattern: `filter(|&x| ..)` | ||
| let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind { | ||
| (ref_pat, true) | ||
| } else { | ||
| (filter_param.pat, false) | ||
| }; | ||
| // 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(is_result) = if cx.tcx.is_diagnostic_item(sym::option_type, opt_ty.did) { | ||
| Some(false) | ||
| } else if cx.tcx.is_diagnostic_item(sym::result_type, opt_ty.did) { | ||
| Some(true) | ||
| } else { | ||
| None | ||
| }; | ||
| if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" }; | ||
| // filter(|x| ...is_some())... | ||
| if let ExprKind::Closure(_, _, filter_body_id, ..) = filter_arg.kind; | ||
| let filter_body = cx.tcx.hir().body(filter_body_id); | ||
| if let [filter_param] = filter_body.params; | ||
| // optional ref pattern: `filter(|&x| ..)` | ||
| let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind { | ||
| (ref_pat, true) | ||
| } else { | ||
| (filter_param.pat, false) | ||
| }; | ||
| // 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(is_result) = if cx.tcx.is_diagnostic_item(sym::option_type, opt_ty.did) { | ||
| Some(false) | ||
| } else if cx.tcx.is_diagnostic_item(sym::result_type, opt_ty.did) { | ||
| Some(true) | ||
| } else { | ||
| None | ||
| }; | ||
| if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" }; | ||
|
|
||
| // ...map(|x| ...unwrap()) | ||
| if let ExprKind::Closure(_, _, map_body_id, ..) = map_arg.kind; | ||
| let map_body = cx.tcx.hir().body(map_body_id); | ||
| if let [map_param] = map_body.params; | ||
| if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind; | ||
| // closure ends with expect() or unwrap() | ||
| if let ExprKind::MethodCall(seg, _, [map_arg, ..], _) = map_body.value.kind; | ||
| if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or); | ||
| // ...map(|x| ...unwrap()) | ||
| if let ExprKind::Closure(_, _, map_body_id, ..) = map_arg.kind; | ||
| let map_body = cx.tcx.hir().body(map_body_id); | ||
| if let [map_param] = map_body.params; | ||
| if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind; | ||
| // closure ends with expect() or unwrap() | ||
| if let ExprKind::MethodCall(seg, _, [map_arg, ..], _) = map_body.value.kind; | ||
| if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or); | ||
|
|
||
| let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| { | ||
| // in `filter(|x| ..)`, replace `*x` with `x` | ||
| let a_path = if_chain! { | ||
| if !is_filter_param_ref; | ||
| if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind; | ||
| 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 TyS::same_type(cx.typeck_results().expr_ty_adjusted(a), cx.typeck_results().expr_ty_adjusted(b)); | ||
| then { | ||
| return true; | ||
| let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| { | ||
| // in `filter(|x| ..)`, replace `*x` with `x` | ||
| let a_path = if_chain! { | ||
| if !is_filter_param_ref; | ||
| if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind; | ||
| 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 TyS::same_type(cx.typeck_results().expr_ty_adjusted(a), cx.typeck_results().expr_ty_adjusted(b)); | ||
| then { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| false | ||
| }; | ||
| if SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg); | ||
| then { | ||
| let span = filter_span.to(map_span); | ||
| let (filter_name, lint) = if is_find { | ||
| ("find", MANUAL_FIND_MAP) | ||
| } else { | ||
| ("filter", MANUAL_FILTER_MAP) | ||
| false | ||
| }; | ||
| 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); | ||
| span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable); | ||
| if SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg); | ||
| then { | ||
| let span = filter_span.to(map_span); | ||
| 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); | ||
| span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| #![warn(clippy::option_filter_map)] | ||
| // run-rustfix | ||
| fn odds_out(x: i32) -> Option<i32> { | ||
| if x % 2 == 0 { Some(x) } else { None } | ||
| } | ||
|
|
||
| fn main() { | ||
| let _ = Some(Some(1)).flatten(); | ||
| let _ = Some(Some(1)).flatten(); | ||
| let _ = Some(1).map(odds_out).flatten(); | ||
| let _ = Some(1).map(odds_out).flatten(); | ||
|
|
||
| let _ = vec![Some(1)].into_iter().flatten(); | ||
| let _ = vec![Some(1)].into_iter().flatten(); | ||
| let _ = vec![1] | ||
| .into_iter() | ||
| .map(odds_out) | ||
| .flatten(); | ||
| let _ = vec![1] | ||
| .into_iter() | ||
| .map(odds_out) | ||
| .flatten(); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #![warn(clippy::option_filter_map)] | ||
| // run-rustfix | ||
| fn odds_out(x: i32) -> Option<i32> { | ||
| if x % 2 == 0 { Some(x) } else { None } | ||
| } | ||
|
|
||
| fn main() { | ||
| let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap); | ||
| let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap()); | ||
| let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap); | ||
| let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap()); | ||
|
|
||
| let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap); | ||
| let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap()); | ||
| let _ = vec![1] | ||
| .into_iter() | ||
| .map(odds_out) | ||
| .filter(Option::is_some) | ||
| .map(Option::unwrap); | ||
| let _ = vec![1] | ||
| .into_iter() | ||
| .map(odds_out) | ||
| .filter(|o| o.is_some()) | ||
| .map(|o| o.unwrap()); | ||
| } |
Oops, something went wrong.