Skip to content

Commit 292e313

Browse files
committed
Auto merge of #9451 - kraktus:manual_filter2, r=dswij
Add `manual_filter` lint for `Option` Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback. I'm sure it's possible to even more refactor both and would gladly take input on that as well as any clippy idiomatic usage, since this is my first lint addition. I've added the lint to the complexity section for now, I don't know if every new lint needs to go in nursery first. The matching could be expanded to more than `Some(<value>)` to lint on arbitrary struct matching inside the `Some` but I've left it like it was for `manual_map` for now. `needless_match::pat_same_as_expr` provides a more generic match example. close #8822 changelog: Add lint [`manual_filter`] for `Option`
2 parents 2c8e473 + 830fdf2 commit 292e313

14 files changed

+1118
-256
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3988,6 +3988,7 @@ Released 2018-09-13
39883988
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
39893989
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
39903990
[`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp
3991+
[`manual_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter
39913992
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
39923993
[`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find
39933994
[`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map

clippy_lints/src/lib.register_all.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
137137
LintId::of(match_result_ok::MATCH_RESULT_OK),
138138
LintId::of(matches::COLLAPSIBLE_MATCH),
139139
LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH),
140+
LintId::of(matches::MANUAL_FILTER),
140141
LintId::of(matches::MANUAL_MAP),
141142
LintId::of(matches::MANUAL_UNWRAP_OR),
142143
LintId::of(matches::MATCH_AS_REF),

clippy_lints/src/lib.register_complexity.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
2727
LintId::of(manual_strip::MANUAL_STRIP),
2828
LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN),
2929
LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN),
30+
LintId::of(matches::MANUAL_FILTER),
3031
LintId::of(matches::MANUAL_UNWRAP_OR),
3132
LintId::of(matches::MATCH_AS_REF),
3233
LintId::of(matches::MATCH_SINGLE_BINDING),

clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ store.register_lints(&[
259259
match_result_ok::MATCH_RESULT_OK,
260260
matches::COLLAPSIBLE_MATCH,
261261
matches::INFALLIBLE_DESTRUCTURING_MATCH,
262+
matches::MANUAL_FILTER,
262263
matches::MANUAL_MAP,
263264
matches::MANUAL_UNWRAP_OR,
264265
matches::MATCH_AS_REF,
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::ty::is_type_diagnostic_item;
3+
use clippy_utils::visitors::contains_unsafe_block;
4+
use clippy_utils::{is_res_lang_ctor, path_res, path_to_local_id};
5+
6+
use rustc_hir::LangItem::OptionSome;
7+
use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind};
8+
use rustc_lint::LateContext;
9+
use rustc_span::{sym, SyntaxContext};
10+
11+
use super::manual_utils::{check_with, SomeExpr};
12+
use super::MANUAL_FILTER;
13+
14+
// Function called on the <expr> of `[&+]Some((ref | ref mut) x) => <expr>`
15+
// Need to check if it's of the form `<expr>=if <cond> {<then_expr>} else {<else_expr>}`
16+
// AND that only one `then/else_expr` resolves to `Some(x)` while the other resolves to `None`
17+
// return the `cond` expression if so.
18+
fn get_cond_expr<'tcx>(
19+
cx: &LateContext<'tcx>,
20+
pat: &Pat<'_>,
21+
expr: &'tcx Expr<'_>,
22+
ctxt: SyntaxContext,
23+
) -> Option<SomeExpr<'tcx>> {
24+
if_chain! {
25+
if let Some(block_expr) = peels_blocks_incl_unsafe_opt(expr);
26+
if let ExprKind::If(cond, then_expr, Some(else_expr)) = block_expr.kind;
27+
if let PatKind::Binding(_,target, ..) = pat.kind;
28+
if let (then_visitor, else_visitor)
29+
= (is_some_expr(cx, target, ctxt, then_expr),
30+
is_some_expr(cx, target, ctxt, else_expr));
31+
if then_visitor != else_visitor; // check that one expr resolves to `Some(x)`, the other to `None`
32+
then {
33+
return Some(SomeExpr {
34+
expr: peels_blocks_incl_unsafe(cond.peel_drop_temps()),
35+
needs_unsafe_block: contains_unsafe_block(cx, expr),
36+
needs_negated: !then_visitor // if the `then_expr` resolves to `None`, need to negate the cond
37+
})
38+
}
39+
};
40+
None
41+
}
42+
43+
fn peels_blocks_incl_unsafe_opt<'a>(expr: &'a Expr<'a>) -> Option<&'a Expr<'a>> {
44+
// we don't want to use `peel_blocks` here because we don't care if the block is unsafe, it's
45+
// checked by `contains_unsafe_block`
46+
if let ExprKind::Block(block, None) = expr.kind {
47+
if block.stmts.is_empty() {
48+
return block.expr;
49+
}
50+
};
51+
None
52+
}
53+
54+
fn peels_blocks_incl_unsafe<'a>(expr: &'a Expr<'a>) -> &'a Expr<'a> {
55+
peels_blocks_incl_unsafe_opt(expr).unwrap_or(expr)
56+
}
57+
58+
// function called for each <expr> expression:
59+
// Some(x) => if <cond> {
60+
// <expr>
61+
// } else {
62+
// <expr>
63+
// }
64+
// Returns true if <expr> resolves to `Some(x)`, `false` otherwise
65+
fn is_some_expr<'tcx>(cx: &LateContext<'_>, target: HirId, ctxt: SyntaxContext, expr: &'tcx Expr<'_>) -> bool {
66+
if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(expr) {
67+
// there can be not statements in the block as they would be removed when switching to `.filter`
68+
if let ExprKind::Call(callee, [arg]) = inner_expr.kind {
69+
return ctxt == expr.span.ctxt()
70+
&& is_res_lang_ctor(cx, path_res(cx, callee), OptionSome)
71+
&& path_to_local_id(arg, target);
72+
}
73+
};
74+
false
75+
}
76+
77+
// given the closure: `|<pattern>| <expr>`
78+
// returns `|&<pattern>| <expr>`
79+
fn add_ampersand_if_copy(body_str: String, has_copy_trait: bool) -> String {
80+
if has_copy_trait {
81+
let mut with_ampersand = body_str;
82+
with_ampersand.insert(1, '&');
83+
with_ampersand
84+
} else {
85+
body_str
86+
}
87+
}
88+
89+
pub(super) fn check_match<'tcx>(
90+
cx: &LateContext<'tcx>,
91+
scrutinee: &'tcx Expr<'_>,
92+
arms: &'tcx [Arm<'_>],
93+
expr: &'tcx Expr<'_>,
94+
) {
95+
let ty = cx.typeck_results().expr_ty(expr);
96+
if is_type_diagnostic_item(cx, ty, sym::Option)
97+
&& let [first_arm, second_arm] = arms
98+
&& first_arm.guard.is_none()
99+
&& second_arm.guard.is_none()
100+
{
101+
check(cx, expr, scrutinee, first_arm.pat, first_arm.body, Some(second_arm.pat), second_arm.body);
102+
}
103+
}
104+
105+
pub(super) fn check_if_let<'tcx>(
106+
cx: &LateContext<'tcx>,
107+
expr: &'tcx Expr<'_>,
108+
let_pat: &'tcx Pat<'_>,
109+
let_expr: &'tcx Expr<'_>,
110+
then_expr: &'tcx Expr<'_>,
111+
else_expr: &'tcx Expr<'_>,
112+
) {
113+
check(cx, expr, let_expr, let_pat, then_expr, None, else_expr);
114+
}
115+
116+
fn check<'tcx>(
117+
cx: &LateContext<'tcx>,
118+
expr: &'tcx Expr<'_>,
119+
scrutinee: &'tcx Expr<'_>,
120+
then_pat: &'tcx Pat<'_>,
121+
then_body: &'tcx Expr<'_>,
122+
else_pat: Option<&'tcx Pat<'_>>,
123+
else_body: &'tcx Expr<'_>,
124+
) {
125+
if let Some(sugg_info) = check_with(
126+
cx,
127+
expr,
128+
scrutinee,
129+
then_pat,
130+
then_body,
131+
else_pat,
132+
else_body,
133+
get_cond_expr,
134+
) {
135+
let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy);
136+
span_lint_and_sugg(
137+
cx,
138+
MANUAL_FILTER,
139+
expr.span,
140+
"manual implementation of `Option::filter`",
141+
"try this",
142+
if sugg_info.needs_brackets {
143+
format!(
144+
"{{ {}{}.filter({body_str}) }}",
145+
sugg_info.scrutinee_str, sugg_info.as_ref_str
146+
)
147+
} else {
148+
format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str)
149+
},
150+
sugg_info.app,
151+
);
152+
}
153+
}

0 commit comments

Comments
 (0)