Skip to content

Commit 648d1ae

Browse files
y21y21
committed
lint on matches and pattern matching
1 parent a3b185b commit 648d1ae

File tree

6 files changed

+440
-59
lines changed

6 files changed

+440
-59
lines changed

‎clippy_lints/src/methods/filter_map.rs

+262-58
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2+
use clippy_utils::macros::{is_panic, root_macro_call};
23
use clippy_utils::source::{indent_of, reindent_multiline, snippet};
34
use clippy_utils::ty::is_type_diagnostic_item;
4-
use clippy_utils::{is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
5+
use clippy_utils::{higher, is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
6+
use hir::{Body, HirId, MatchSource, Pat};
57
use if_chain::if_chain;
68
use rustc_errors::Applicability;
79
use rustc_hir as hir;
@@ -10,7 +12,7 @@ use rustc_hir::{Closure, Expr, ExprKind, PatKind, PathSegment, QPath, UnOp};
1012
use rustc_lint::LateContext;
1113
use rustc_middle::ty::adjustment::Adjust;
1214
use rustc_span::source_map::Span;
13-
use rustc_span::symbol::{sym, Symbol};
15+
use rustc_span::symbol::{sym, Ident, Symbol};
1416
use std::borrow::Cow;
1517

1618
use super::MANUAL_FILTER_MAP;
@@ -50,6 +52,214 @@ fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_ar
5052
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
5153
}
5254

55+
#[derive(Debug, Copy, Clone)]
56+
enum OffendingFilterExpr<'tcx> {
57+
/// `.filter(|opt| opt.is_some())`
58+
IsSome {
59+
/// The receiver expression
60+
receiver: &'tcx Expr<'tcx>,
61+
/// If `Some`, then this contains the span of an expression that possibly contains side
62+
/// effects: `.filter(|opt| side_effect(opt).is_some())`
63+
/// ^^^^^^^^^^^^^^^^
64+
///
65+
/// We will use this later for warning the user that the suggested fix may change
66+
/// the behavior.
67+
side_effect_expr_span: Option<Span>,
68+
},
69+
/// `.filter(|res| res.is_ok())`
70+
IsOk {
71+
/// The receiver expression
72+
receiver: &'tcx Expr<'tcx>,
73+
/// See `IsSome`
74+
side_effect_expr_span: Option<Span>,
75+
},
76+
/// `.filter(|enum| matches!(enum, Enum::A(_)))`
77+
Matches {
78+
/// The DefId of the variant being matched
79+
variant_def_id: hir::def_id::DefId,
80+
},
81+
}
82+
83+
#[derive(Debug)]
84+
enum CalledMethod {
85+
OptionIsSome,
86+
ResultIsOk,
87+
}
88+
89+
/// The result of checking a `map` call, returned by `OffendingFilterExpr::check_map_call`
90+
#[derive(Debug)]
91+
enum CheckResult<'tcx> {
92+
Method {
93+
map_arg: &'tcx Expr<'tcx>,
94+
/// The method that was called inside of `filter`
95+
method: CalledMethod,
96+
/// See `OffendingFilterExpr::IsSome`
97+
side_effect_expr_span: Option<Span>,
98+
},
99+
PatternMatching {
100+
/// The span of the variant being matched
101+
/// if let Some(s) = enum
102+
/// ^^^^^^^
103+
variant_span: Span,
104+
/// if let Some(s) = enum
105+
/// ^
106+
variant_ident: Ident,
107+
},
108+
}
109+
110+
impl<'tcx> OffendingFilterExpr<'tcx> {
111+
pub fn check_map_call(
112+
&mut self,
113+
cx: &LateContext<'tcx>,
114+
map_body: &'tcx Body<'tcx>,
115+
map_param_id: HirId,
116+
filter_param_id: HirId,
117+
is_filter_param_ref: bool,
118+
) -> Option<CheckResult<'tcx>> {
119+
match *self {
120+
OffendingFilterExpr::IsSome {
121+
receiver,
122+
side_effect_expr_span,
123+
}
124+
| OffendingFilterExpr::IsOk {
125+
receiver,
126+
side_effect_expr_span,
127+
} => {
128+
// check if closure ends with expect() or unwrap()
129+
if let ExprKind::MethodCall(seg, map_arg, ..) = map_body.value.kind
130+
&& matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or)
131+
// .map(|y| f(y).copied().unwrap())
132+
// ~~~~
133+
&& let map_arg_peeled = match map_arg.kind {
134+
ExprKind::MethodCall(method, original_arg, [], _) if acceptable_methods(method) => {
135+
original_arg
136+
},
137+
_ => map_arg,
138+
}
139+
// .map(|y| y[.acceptable_method()].unwrap())
140+
&& let simple_equal = (path_to_local_id(receiver, filter_param_id)
141+
&& path_to_local_id(map_arg_peeled, map_param_id))
142+
&& let eq_fallback = (|a: &Expr<'_>, b: &Expr<'_>| {
143+
// in `filter(|x| ..)`, replace `*x` with `x`
144+
let a_path = if_chain! {
145+
if !is_filter_param_ref;
146+
if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind;
147+
then { expr_path } else { a }
148+
};
149+
// let the filter closure arg and the map closure arg be equal
150+
path_to_local_id(a_path, filter_param_id)
151+
&& path_to_local_id(b, map_param_id)
152+
&& cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
153+
})
154+
&& (simple_equal
155+
|| SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(receiver, map_arg_peeled))
156+
{
157+
Some(CheckResult::Method {
158+
map_arg,
159+
side_effect_expr_span,
160+
method: match self {
161+
OffendingFilterExpr::IsSome { .. } => CalledMethod::OptionIsSome,
162+
OffendingFilterExpr::IsOk { .. } => CalledMethod::ResultIsOk,
163+
OffendingFilterExpr::Matches { .. } => unreachable!("only IsSome and IsOk can get here"),
164+
}
165+
})
166+
} else {
167+
None
168+
}
169+
},
170+
OffendingFilterExpr::Matches { variant_def_id } => {
171+
let expr_uses_local = |pat: &Pat<'_>, expr: &Expr<'_>| {
172+
if let PatKind::TupleStruct(QPath::Resolved(_, path), [subpat], _) = pat.kind
173+
&& let PatKind::Binding(_, local_id, ident, _) = subpat.kind
174+
&& path_to_local_id(expr.peel_blocks(), local_id)
175+
&& let Some(local_variant_def_id) = path.res.opt_def_id()
176+
&& local_variant_def_id == variant_def_id
177+
{
178+
Some((ident, pat.span))
179+
} else {
180+
None
181+
}
182+
};
183+
184+
// look for:
185+
// `if let Variant (v) = enum { v } else { unreachable!() }`
186+
// ^^^^^^^ ^ ^^^^ ^^^^^^^^^^^^^^^^^^
187+
// variant_span variant_ident scrutinee else_ (blocks peeled later)
188+
// OR
189+
// `match enum { Variant (v) => v, _ => unreachable!() }`
190+
// ^^^^ ^^^^^^^ ^ ^^^^^^^^^^^^^^
191+
// scrutinee variant_span variant_ident else_
192+
let (scrutinee, else_, variant_ident, variant_span) =
193+
match higher::IfLetOrMatch::parse(cx, map_body.value) {
194+
// For `if let` we want to check that the variant matching arm references the local created by its pattern
195+
Some(higher::IfLetOrMatch::IfLet(sc, pat, then, Some(else_)))
196+
if let Some((ident, span)) = expr_uses_local(pat, then) =>
197+
{
198+
(sc, else_, ident, span)
199+
},
200+
// For `match` we want to check that the "else" arm is the wildcard (`_`) pattern
201+
// and that the variant matching arm references the local created by its pattern
202+
Some(higher::IfLetOrMatch::Match(sc, [arm, wild_arm], MatchSource::Normal))
203+
if let PatKind::Wild = wild_arm.pat.kind
204+
&& let Some((ident, span)) = expr_uses_local(arm.pat, arm.body.peel_blocks()) =>
205+
{
206+
(sc, wild_arm.body, ident, span)
207+
},
208+
_ => return None,
209+
};
210+
211+
if path_to_local_id(scrutinee, map_param_id)
212+
// else branch should be a `panic!` or `unreachable!` macro call
213+
&& let Some(mac) = root_macro_call(else_.peel_blocks().span)
214+
&& (is_panic(cx, mac.def_id) || cx.tcx.opt_item_name(mac.def_id) == Some(sym::unreachable))
215+
{
216+
Some(CheckResult::PatternMatching { variant_span, variant_ident })
217+
} else {
218+
None
219+
}
220+
},
221+
}
222+
}
223+
224+
fn hir(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, filter_param_id: HirId) -> Option<Self> {
225+
if let ExprKind::MethodCall(path, receiver, [], _) = expr.kind
226+
&& let Some(recv_ty) = cx.typeck_results().expr_ty(receiver).peel_refs().ty_adt_def()
227+
{
228+
// we still want to lint if the expression possibly contains side effects,
229+
// *but* it can't be machine-applicable then, because that can change the behavior of the program:
230+
// .filter(|x| effect(x).is_some()).map(|x| effect(x).unwrap())
231+
// vs.
232+
// .filter_map(|x| effect(x))
233+
//
234+
// the latter only calls `effect` once
235+
let side_effect_expr_span = receiver.can_have_side_effects().then_some(receiver.span);
236+
237+
if cx.tcx.is_diagnostic_item(sym::Option, recv_ty.did())
238+
&& path.ident.name == sym!(is_some)
239+
{
240+
Some(Self::IsSome { receiver, side_effect_expr_span })
241+
} else if cx.tcx.is_diagnostic_item(sym::Result, recv_ty.did())
242+
&& path.ident.name == sym!(is_ok)
243+
{
244+
Some(Self::IsOk { receiver, side_effect_expr_span })
245+
} else {
246+
None
247+
}
248+
} else if let Some(macro_call) = root_macro_call(expr.span)
249+
&& cx.tcx.get_diagnostic_name(macro_call.def_id) == Some(sym::matches_macro)
250+
// we know for a fact that the wildcard pattern is the second arm
251+
&& let ExprKind::Match(scrutinee, [arm, _], _) = expr.kind
252+
&& path_to_local_id(scrutinee, filter_param_id)
253+
&& let PatKind::TupleStruct(QPath::Resolved(_, path), ..) = arm.pat.kind
254+
&& let Some(variant_def_id) = path.res.opt_def_id()
255+
{
256+
Some(OffendingFilterExpr::Matches { variant_def_id })
257+
} else {
258+
None
259+
}
260+
}
261+
}
262+
53263
/// is `filter(|x| x.is_some()).map(|x| x.unwrap())`
54264
fn is_filter_some_map_unwrap(
55265
cx: &LateContext<'_>,
@@ -104,55 +314,18 @@ pub(super) fn check(
104314
} else {
105315
(filter_param.pat, false)
106316
};
107-
// closure ends with is_some() or is_ok()
317+
108318
if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
109-
if let ExprKind::MethodCall(path, filter_arg, [], _) = filter_body.value.kind;
110-
if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).peel_refs().ty_adt_def();
111-
if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::Option, opt_ty.did()) {
112-
Some(false)
113-
} else if cx.tcx.is_diagnostic_item(sym::Result, opt_ty.did()) {
114-
Some(true)
115-
} else {
116-
None
117-
};
118-
if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" };
319+
if let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id);
119320

120-
// ...map(|x| ...unwrap())
121321
if let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind;
122322
let map_body = cx.tcx.hir().body(map_body_id);
123323
if let [map_param] = map_body.params;
124324
if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind;
125-
// closure ends with expect() or unwrap()
126-
if let ExprKind::MethodCall(seg, map_arg, ..) = map_body.value.kind;
127-
if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
128-
129-
// .filter(..).map(|y| f(y).copied().unwrap())
130-
// ~~~~
131-
let map_arg_peeled = match map_arg.kind {
132-
ExprKind::MethodCall(method, original_arg, [], _) if acceptable_methods(method) => {
133-
original_arg
134-
},
135-
_ => map_arg,
136-
};
137325

138-
// .filter(|x| x.is_some()).map(|y| y[.acceptable_method()].unwrap())
139-
let simple_equal = path_to_local_id(filter_arg, filter_param_id)
140-
&& path_to_local_id(map_arg_peeled, map_param_id);
326+
if let Some(check_result) =
327+
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref);
141328

142-
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
143-
// in `filter(|x| ..)`, replace `*x` with `x`
144-
let a_path = if_chain! {
145-
if !is_filter_param_ref;
146-
if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind;
147-
then { expr_path } else { a }
148-
};
149-
// let the filter closure arg and the map closure arg be equal
150-
path_to_local_id(a_path, filter_param_id)
151-
&& path_to_local_id(b, map_param_id)
152-
&& cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
153-
};
154-
155-
if simple_equal || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg_peeled);
156329
then {
157330
let span = filter_span.with_hi(expr.span.hi());
158331
let (filter_name, lint) = if is_find {
@@ -161,22 +334,53 @@ pub(super) fn check(
161334
("filter", MANUAL_FILTER_MAP)
162335
};
163336
let msg = format!("`{filter_name}(..).map(..)` can be simplified as `{filter_name}_map(..)`");
164-
let (to_opt, deref) = if is_result {
165-
(".ok()", String::new())
166-
} else {
167-
let derefs = cx.typeck_results()
168-
.expr_adjustments(map_arg)
169-
.iter()
170-
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
171-
.count();
172337

173-
("", "*".repeat(derefs))
338+
let (sugg, note_and_span, applicability) = match check_result {
339+
CheckResult::Method { map_arg, method, side_effect_expr_span } => {
340+
let (to_opt, deref) = match method {
341+
CalledMethod::ResultIsOk => (".ok()", String::new()),
342+
CalledMethod::OptionIsSome => {
343+
let derefs = cx.typeck_results()
344+
.expr_adjustments(map_arg)
345+
.iter()
346+
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
347+
.count();
348+
349+
("", "*".repeat(derefs))
350+
}
351+
};
352+
353+
let sugg = format!(
354+
"{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
355+
snippet(cx, map_arg.span, ".."),
356+
);
357+
let (note_and_span, applicability) = if let Some(span) = side_effect_expr_span {
358+
let note = "the suggestion might change the behavior of the program when merging `filter` and `map`, \
359+
because this expression potentially contains side effects and will only execute once";
360+
361+
(Some((note, span)), Applicability::MaybeIncorrect)
362+
} else {
363+
(None, Applicability::MachineApplicable)
364+
};
365+
366+
(sugg, note_and_span, applicability)
367+
}
368+
CheckResult::PatternMatching { variant_span, variant_ident } => {
369+
let pat = snippet(cx, variant_span, "<pattern>");
370+
371+
(format!("{filter_name}_map(|{map_param_ident}| match {map_param_ident} {{ \
372+
{pat} => Some({variant_ident}), \
373+
_ => None \
374+
}})"), None, Applicability::MachineApplicable)
375+
}
174376
};
175-
let sugg = format!(
176-
"{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
177-
snippet(cx, map_arg.span, ".."),
178-
);
179-
span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable);
377+
span_lint_and_then(cx, lint, span, &msg, |diag| {
378+
diag.span_suggestion(span, "try", sugg, applicability);
379+
380+
if let Some((note, span)) = note_and_span {
381+
diag.span_note(span, note);
382+
}
383+
});
180384
}
181385
}
182386
}

‎clippy_utils/src/higher.rs

+1
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ impl<'hir> IfLet<'hir> {
138138
}
139139

140140
/// An `if let` or `match` expression. Useful for lints that trigger on one or the other.
141+
#[derive(Debug)]
141142
pub enum IfLetOrMatch<'hir> {
142143
/// Any `match` expression
143144
Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),

‎tests/ui/manual_filter_map.fixed

+24
Original file line numberDiff line numberDiff line change
@@ -120,3 +120,27 @@ fn issue_8920() {
120120
.iter()
121121
.filter_map(|f| f.result_field.to_owned().ok());
122122
}
123+
124+
fn issue8010() {
125+
#[derive(Clone)]
126+
enum Enum {
127+
A(i32),
128+
B,
129+
}
130+
131+
let iter = [Enum::A(123), Enum::B].into_iter();
132+
133+
let _x = iter.clone().filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
134+
let _x = iter.clone().filter(|x| matches!(x, Enum::B)).map(|x| match x {
135+
Enum::A(s) => s,
136+
_ => unreachable!(),
137+
});
138+
let _x = iter
139+
.clone()
140+
.filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
141+
#[allow(clippy::unused_unit)]
142+
let _x = iter
143+
.clone()
144+
.filter(|x| matches!(x, Enum::B))
145+
.map(|x| if let Enum::B = x { () } else { unreachable!() });
146+
}

0 commit comments

Comments
 (0)