Skip to content

Commit

Permalink
Auto merge of #10949 - y21:issue8010, r=Alexendoo
Browse files Browse the repository at this point in the history 
[`manual_filter_map`]: lint on `matches` and pattern matching

Fixes #8010

Previously this lint only worked specifically for a very limited set of methods on the filter call (`.filter(|opt| opt.is_some())` and `.filter(|res| res.is_ok())`). This PR extends it to also recognize `matches!` in the `filter` and pattern matching with `if let` or `match` in the `map`.

Example:
```rs
enum Enum {
  A(i32),
  B,
}

let _ = [Enum::A(123), Enum::B].into_iter()
  .filter(|x| matches!(x, Enum::A(_)))
  .map(|x| if let Enum::A(s) = x { s } else { unreachable!() });
```
Now suggests:
```diff
-  .filter(|x| matches!(x, Enum::A(_))).map(if let Enum::A(s) = x { s } else { unreachable!() })
+  .filter_map(|x| match x { Enum::A(s) => Some(s), _ => None })
```

Adding this required a somewhat large change in code because it originally seemed to be specifically written with only method calls in the filter in mind, and `matches!` has different behavior in the map, so this new setup should make it possible to support more "generic" cases that need different handling for the filter and map calls.

changelog: [`manual_filter_map`]: lint on `matches` and pattern matching (and some internal refactoring)
  • Loading branch information
@bors
bors committed Jul 19, 2023
2 parents 7a34143 + 648d1ae commit 0b63e95
Show file tree
Hide file tree
Showing 6 changed files with 440 additions and 59 deletions.
320 changes: 262 additions & 58 deletions clippy_lints/src/methods/filter_map.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::macros::{is_panic, root_macro_call};
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, peel_blocks, SpanlessEq};
use clippy_utils::{higher, is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
use hir::{Body, HirId, MatchSource, Pat};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
Expand All @@ -10,7 +12,7 @@ use rustc_hir::{Closure, 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 rustc_span::symbol::{sym, Ident, Symbol};
use std::borrow::Cow;

use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP};
Expand Down Expand Up @@ -48,6 +50,214 @@ fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_ar
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
}

#[derive(Debug, Copy, Clone)]
enum OffendingFilterExpr<'tcx> {
/// `.filter(|opt| opt.is_some())`
IsSome {
/// The receiver expression
receiver: &'tcx Expr<'tcx>,
/// If `Some`, then this contains the span of an expression that possibly contains side
/// effects: `.filter(|opt| side_effect(opt).is_some())`
/// ^^^^^^^^^^^^^^^^
///
/// We will use this later for warning the user that the suggested fix may change
/// the behavior.
side_effect_expr_span: Option<Span>,
},
/// `.filter(|res| res.is_ok())`
IsOk {
/// The receiver expression
receiver: &'tcx Expr<'tcx>,
/// See `IsSome`
side_effect_expr_span: Option<Span>,
},
/// `.filter(|enum| matches!(enum, Enum::A(_)))`
Matches {
/// The DefId of the variant being matched
variant_def_id: hir::def_id::DefId,
},
}

#[derive(Debug)]
enum CalledMethod {
OptionIsSome,
ResultIsOk,
}

/// The result of checking a `map` call, returned by `OffendingFilterExpr::check_map_call`
#[derive(Debug)]
enum CheckResult<'tcx> {
Method {
map_arg: &'tcx Expr<'tcx>,
/// The method that was called inside of `filter`
method: CalledMethod,
/// See `OffendingFilterExpr::IsSome`
side_effect_expr_span: Option<Span>,
},
PatternMatching {
/// The span of the variant being matched
/// if let Some(s) = enum
/// ^^^^^^^
variant_span: Span,
/// if let Some(s) = enum
/// ^
variant_ident: Ident,
},
}

impl<'tcx> OffendingFilterExpr<'tcx> {
pub fn check_map_call(
&mut self,
cx: &LateContext<'tcx>,
map_body: &'tcx Body<'tcx>,
map_param_id: HirId,
filter_param_id: HirId,
is_filter_param_ref: bool,
) -> Option<CheckResult<'tcx>> {
match *self {
OffendingFilterExpr::IsSome {
receiver,
side_effect_expr_span,
}
| OffendingFilterExpr::IsOk {
receiver,
side_effect_expr_span,
} => {
// check if closure ends with expect() or unwrap()
if let ExprKind::MethodCall(seg, map_arg, ..) = map_body.value.kind
&& matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or)
// .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,
}
// .map(|y| y[.acceptable_method()].unwrap())
&& let simple_equal = (path_to_local_id(receiver, 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! {
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
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)
})
&& (simple_equal
|| SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(receiver, map_arg_peeled))
{
Some(CheckResult::Method {
map_arg,
side_effect_expr_span,
method: match self {
OffendingFilterExpr::IsSome { .. } => CalledMethod::OptionIsSome,
OffendingFilterExpr::IsOk { .. } => CalledMethod::ResultIsOk,
OffendingFilterExpr::Matches { .. } => unreachable!("only IsSome and IsOk can get here"),
}
})
} else {
None
}
},
OffendingFilterExpr::Matches { variant_def_id } => {
let expr_uses_local = |pat: &Pat<'_>, expr: &Expr<'_>| {
if let PatKind::TupleStruct(QPath::Resolved(_, path), [subpat], _) = pat.kind
&& let PatKind::Binding(_, local_id, ident, _) = subpat.kind
&& path_to_local_id(expr.peel_blocks(), local_id)
&& let Some(local_variant_def_id) = path.res.opt_def_id()
&& local_variant_def_id == variant_def_id
{
Some((ident, pat.span))
} else {
None
}
};

// look for:
// `if let Variant (v) = enum { v } else { unreachable!() }`
// ^^^^^^^ ^ ^^^^ ^^^^^^^^^^^^^^^^^^
// variant_span variant_ident scrutinee else_ (blocks peeled later)
// OR
// `match enum { Variant (v) => v, _ => unreachable!() }`
// ^^^^ ^^^^^^^ ^ ^^^^^^^^^^^^^^
// scrutinee variant_span variant_ident else_
let (scrutinee, else_, variant_ident, variant_span) =
match higher::IfLetOrMatch::parse(cx, map_body.value) {
// For `if let` we want to check that the variant matching arm references the local created by its pattern
Some(higher::IfLetOrMatch::IfLet(sc, pat, then, Some(else_)))
if let Some((ident, span)) = expr_uses_local(pat, then) =>
{
(sc, else_, ident, span)
},
// For `match` we want to check that the "else" arm is the wildcard (`_`) pattern
// and that the variant matching arm references the local created by its pattern
Some(higher::IfLetOrMatch::Match(sc, [arm, wild_arm], MatchSource::Normal))
if let PatKind::Wild = wild_arm.pat.kind
&& let Some((ident, span)) = expr_uses_local(arm.pat, arm.body.peel_blocks()) =>
{
(sc, wild_arm.body, ident, span)
},
_ => return None,
};

if path_to_local_id(scrutinee, map_param_id)
// else branch should be a `panic!` or `unreachable!` macro call
&& let Some(mac) = root_macro_call(else_.peel_blocks().span)
&& (is_panic(cx, mac.def_id) || cx.tcx.opt_item_name(mac.def_id) == Some(sym::unreachable))
{
Some(CheckResult::PatternMatching { variant_span, variant_ident })
} else {
None
}
},
}
}

fn hir(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, filter_param_id: HirId) -> Option<Self> {
if let ExprKind::MethodCall(path, receiver, [], _) = expr.kind
&& let Some(recv_ty) = cx.typeck_results().expr_ty(receiver).peel_refs().ty_adt_def()
{
// we still want to lint if the expression possibly contains side effects,
// *but* it can't be machine-applicable then, because that can change the behavior of the program:
// .filter(|x| effect(x).is_some()).map(|x| effect(x).unwrap())
// vs.
// .filter_map(|x| effect(x))
//
// the latter only calls `effect` once
let side_effect_expr_span = receiver.can_have_side_effects().then_some(receiver.span);

if cx.tcx.is_diagnostic_item(sym::Option, recv_ty.did())
&& path.ident.name == sym!(is_some)
{
Some(Self::IsSome { receiver, side_effect_expr_span })
} else if cx.tcx.is_diagnostic_item(sym::Result, recv_ty.did())
&& path.ident.name == sym!(is_ok)
{
Some(Self::IsOk { receiver, side_effect_expr_span })
} else {
None
}
} else if let Some(macro_call) = root_macro_call(expr.span)
&& cx.tcx.get_diagnostic_name(macro_call.def_id) == Some(sym::matches_macro)
// we know for a fact that the wildcard pattern is the second arm
&& let ExprKind::Match(scrutinee, [arm, _], _) = expr.kind
&& path_to_local_id(scrutinee, filter_param_id)
&& let PatKind::TupleStruct(QPath::Resolved(_, path), ..) = arm.pat.kind
&& let Some(variant_def_id) = path.res.opt_def_id()
{
Some(OffendingFilterExpr::Matches { variant_def_id })
} else {
None
}
}
}

/// is `filter(|x| x.is_some()).map(|x| x.unwrap())`
fn is_filter_some_map_unwrap(
cx: &LateContext<'_>,
Expand Down Expand Up @@ -102,55 +312,18 @@ pub(super) fn check(
} 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).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()) {
Some(true)
} else {
None
};
if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" };
if let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id);

// ...map(|x| ...unwrap())
if let ExprKind::Closure(&Closure { body: 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);

// .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);
if let Some(check_result) =
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref);

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
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 {
Expand All @@ -159,22 +332,53 @@ pub(super) fn check(
("filter", MANUAL_FILTER_MAP)
};
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, note_and_span, applicability) = match check_result {
CheckResult::Method { map_arg, method, side_effect_expr_span } => {
let (to_opt, deref) = match method {
CalledMethod::ResultIsOk => (".ok()", String::new()),
CalledMethod::OptionIsSome => {
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, ".."),
);
let (note_and_span, applicability) = if let Some(span) = side_effect_expr_span {
let note = "the suggestion might change the behavior of the program when merging `filter` and `map`, \
because this expression potentially contains side effects and will only execute once";

(Some((note, span)), Applicability::MaybeIncorrect)
} else {
(None, Applicability::MachineApplicable)
};

(sugg, note_and_span, applicability)
}
CheckResult::PatternMatching { variant_span, variant_ident } => {
let pat = snippet(cx, variant_span, "<pattern>");

(format!("{filter_name}_map(|{map_param_ident}| match {map_param_ident} {{ \
{pat} => Some({variant_ident}), \
_ => None \
}})"), None, Applicability::MachineApplicable)
}
};
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);
span_lint_and_then(cx, lint, span, &msg, |diag| {
diag.span_suggestion(span, "try", sugg, applicability);

if let Some((note, span)) = note_and_span {
diag.span_note(span, note);
}
});
}
}
}
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/higher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ impl<'hir> IfLet<'hir> {
}

/// An `if let` or `match` expression. Useful for lints that trigger on one or the other.
#[derive(Debug)]
pub enum IfLetOrMatch<'hir> {
/// Any `match` expression
Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/manual_filter_map.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,27 @@ fn issue_8920() {
.iter()
.filter_map(|f| f.result_field.to_owned().ok());
}

fn issue8010() {
#[derive(Clone)]
enum Enum {
A(i32),
B,
}

let iter = [Enum::A(123), Enum::B].into_iter();

let _x = iter.clone().filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
let _x = iter.clone().filter(|x| matches!(x, Enum::B)).map(|x| match x {
Enum::A(s) => s,
_ => unreachable!(),
});
let _x = iter
.clone()
.filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
#[allow(clippy::unused_unit)]
let _x = iter
.clone()
.filter(|x| matches!(x, Enum::B))
.map(|x| if let Enum::B = x { () } else { unreachable!() });
}
Loading

0 comments on commit 0b63e95

Please sign in to comment.