Skip to content

Commit 837a8aa

Browse files
profetiaprofetia
committed
Enhance manual_filter to cover and_then 
1 parent dc57e57 commit 837a8aa

15 files changed

+174
-48
lines changed

‎clippy_lints/src/matches/manual_filter.rs‎

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
use clippy_utils::as_some_expr;
2-
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
33
use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
4+
use clippy_utils::source::snippet_with_context;
5+
use clippy_utils::sugg::Sugg;
6+
use clippy_utils::ty::is_copy;
47
use clippy_utils::visitors::contains_unsafe_block;
5-
8+
use rustc_errors::Applicability;
69
use rustc_hir::LangItem::OptionNone;
7-
use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind};
10+
use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind, PathSegment};
811
use rustc_lint::LateContext;
9-
use rustc_span::{SyntaxContext, sym};
12+
use rustc_span::{Span, SyntaxContext, sym};
1013

1114
use super::MANUAL_FILTER;
1215
use super::manual_utils::{SomeExpr, check_with};
@@ -41,12 +44,11 @@ fn get_cond_expr<'tcx>(
4144
fn peels_blocks_incl_unsafe_opt<'a>(expr: &'a Expr<'a>) -> Option<&'a Expr<'a>> {
4245
// we don't want to use `peel_blocks` here because we don't care if the block is unsafe, it's
4346
// checked by `contains_unsafe_block`
44-
if let ExprKind::Block(block, None) = expr.kind
45-
&& block.stmts.is_empty()
46-
{
47-
return block.expr;
47+
if let ExprKind::Block(block, None) = expr.kind {
48+
return if block.stmts.is_empty() { block.expr } else { None };
4849
}
49-
None
50+
51+
Some(expr)
5052
}
5153

5254
fn peels_blocks_incl_unsafe<'a>(expr: &'a Expr<'a>) -> &'a Expr<'a> {
@@ -89,6 +91,65 @@ fn add_ampersand_if_copy(body_str: String, has_copy_trait: bool) -> String {
8991
}
9092
}
9193

94+
pub(super) fn check_method_call<'tcx>(
95+
cx: &LateContext<'tcx>,
96+
path: &PathSegment<'_>,
97+
scrutinee: &'tcx Expr<'_>,
98+
args: &'tcx [Expr<'_>],
99+
call_span: Span,
100+
expr: &'tcx Expr<'_>,
101+
) {
102+
let ty = cx.typeck_results().expr_ty(scrutinee).peel_refs();
103+
if ty.is_diag_item(cx, sym::Option)
104+
&& path.ident.name == sym::and_then
105+
&& let [first] = args
106+
&& let ExprKind::Closure(closure) = first.kind
107+
&& let body = cx.tcx.hir_body(closure.body)
108+
&& let Some(fn_arg_span) = closure.fn_arg_span
109+
&& let [param] = body.params
110+
&& let expr_span_ctxt = expr.span.ctxt()
111+
&& let Some(some_expr) = get_cond_expr(cx, param.pat, body.value, expr_span_ctxt)
112+
{
113+
span_lint_and_then(
114+
cx,
115+
MANUAL_FILTER,
116+
call_span,
117+
"manual implementation of `Option::filter`",
118+
|diag| {
119+
let mut applicability = Applicability::MachineApplicable;
120+
121+
let mut cond_snip =
122+
Sugg::hir_with_context(cx, some_expr.expr, expr_span_ctxt, "..", &mut applicability);
123+
if some_expr.needs_unsafe_block {
124+
cond_snip = cond_snip.unsafeify();
125+
}
126+
if some_expr.needs_negated {
127+
cond_snip = !cond_snip;
128+
}
129+
130+
let (prefix_snip, _) = snippet_with_context(
131+
cx,
132+
closure.fn_decl_span.shrink_to_lo().until(fn_arg_span),
133+
expr_span_ctxt,
134+
"..",
135+
&mut applicability,
136+
);
137+
let (param_snip, _) =
138+
snippet_with_context(cx, param.pat.span, expr_span_ctxt, "..", &mut applicability);
139+
diag.span_suggestion(
140+
call_span,
141+
"try",
142+
format!(
143+
"filter({prefix_snip}|{}{param_snip}| {cond_snip})",
144+
if is_copy(cx, ty) { "&" } else { "" }
145+
),
146+
applicability,
147+
);
148+
},
149+
);
150+
}
151+
}
152+
92153
pub(super) fn check_match<'tcx>(
93154
cx: &LateContext<'tcx>,
94155
scrutinee: &'tcx Expr<'_>,

‎clippy_lints/src/matches/mod.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1202,6 +1202,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
12021202
}
12031203
if !from_expansion {
12041204
redundant_pattern_match::check(cx, expr);
1205+
1206+
if let ExprKind::MethodCall(path, recv, args, call_span) = expr.kind {
1207+
manual_filter::check_method_call(cx, path, recv, args, call_span, expr);
1208+
}
12051209
}
12061210
}
12071211
}

‎clippy_utils/src/sugg.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,11 @@ impl<'a> Sugg<'a> {
335335
Sugg::NonParen(Cow::Owned(format!("{{ {self} }}")))
336336
}
337337

338+
/// Convenience method to wrap the expression in an `unsafe` block.
339+
pub fn unsafeify(self) -> Sugg<'static> {
340+
Sugg::NonParen(Cow::Owned(format!("unsafe {{ {self} }}")))
341+
}
342+
338343
/// Convenience method to prefix the expression with the `async` keyword.
339344
/// Can be used after `blockify` to create an async block.
340345
pub fn asyncify(self) -> Sugg<'static> {

‎tests/ui/bind_instead_of_map.fixed‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#![deny(clippy::bind_instead_of_map)]
2+
#![allow(clippy::manual_filter)]
23

34
// need a main anyway, use it get rid of unused warnings too
45
pub fn main() {

‎tests/ui/bind_instead_of_map.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#![deny(clippy::bind_instead_of_map)]
2+
#![allow(clippy::manual_filter)]
23

34
// need a main anyway, use it get rid of unused warnings too
45
pub fn main() {

‎tests/ui/bind_instead_of_map.stderr‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: using `Option.and_then(Some)`, which is a no-op
2-
--> tests/ui/bind_instead_of_map.rs:7:13
2+
--> tests/ui/bind_instead_of_map.rs:8:13
33
|
44
LL | let _ = x.and_then(Some);
55
| ^^^^^^^^^^^^^^^^ help: use the expression directly: `x`
@@ -11,13 +11,13 @@ LL | #![deny(clippy::bind_instead_of_map)]
1111
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
1212

1313
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
14-
--> tests/ui/bind_instead_of_map.rs:9:13
14+
--> tests/ui/bind_instead_of_map.rs:10:13
1515
|
1616
LL | let _ = x.and_then(|o| Some(o + 1));
1717
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.map(|o| o + 1)`
1818

1919
error: using `Result.and_then(Ok)`, which is a no-op
20-
--> tests/ui/bind_instead_of_map.rs:16:13
20+
--> tests/ui/bind_instead_of_map.rs:17:13
2121
|
2222
LL | let _ = x.and_then(Ok);
2323
| ^^^^^^^^^^^^^^ help: use the expression directly: `x`

‎tests/ui/if_then_some_else_none.fixed‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#![warn(clippy::if_then_some_else_none)]
2-
#![allow(clippy::redundant_pattern_matching, clippy::unnecessary_lazy_evaluations)]
2+
#![allow(
3+
clippy::redundant_pattern_matching,
4+
clippy::unnecessary_lazy_evaluations,
5+
clippy::manual_filter
6+
)]
37

48
fn main() {
59
// Should issue an error.

‎tests/ui/if_then_some_else_none.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#![warn(clippy::if_then_some_else_none)]
2-
#![allow(clippy::redundant_pattern_matching, clippy::unnecessary_lazy_evaluations)]
2+
#![allow(
3+
clippy::redundant_pattern_matching,
4+
clippy::unnecessary_lazy_evaluations,
5+
clippy::manual_filter
6+
)]
37

48
fn main() {
59
// Should issue an error.

‎tests/ui/if_then_some_else_none.stderr‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this could be simplified with `bool::then`
2-
--> tests/ui/if_then_some_else_none.rs:6:13
2+
--> tests/ui/if_then_some_else_none.rs:10:13
33
|
44
LL | let _ = if foo() {
55
| _____________^
@@ -24,7 +24,7 @@ LL ~ });
2424
|
2525

2626
error: this could be simplified with `bool::then`
27-
--> tests/ui/if_then_some_else_none.rs:16:13
27+
--> tests/ui/if_then_some_else_none.rs:20:13
2828
|
2929
LL | let _ = if matches!(true, true) {
3030
| _____________^
@@ -47,19 +47,19 @@ LL ~ });
4747
|
4848

4949
error: this could be simplified with `bool::then_some`
50-
--> tests/ui/if_then_some_else_none.rs:27:28
50+
--> tests/ui/if_then_some_else_none.rs:31:28
5151
|
5252
LL | let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });
5353
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(o < 32).then_some(o)`
5454

5555
error: this could be simplified with `bool::then_some`
56-
--> tests/ui/if_then_some_else_none.rs:32:13
56+
--> tests/ui/if_then_some_else_none.rs:36:13
5757
|
5858
LL | let _ = if !x { Some(0) } else { None };
5959
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(!x).then_some(0)`
6060

6161
error: this could be simplified with `bool::then`
62-
--> tests/ui/if_then_some_else_none.rs:88:13
62+
--> tests/ui/if_then_some_else_none.rs:92:13
6363
|
6464
LL | let _ = if foo() {
6565
| _____________^
@@ -82,13 +82,13 @@ LL ~ });
8282
|
8383

8484
error: this could be simplified with `bool::then`
85-
--> tests/ui/if_then_some_else_none.rs:138:5
85+
--> tests/ui/if_then_some_else_none.rs:142:5
8686
|
8787
LL | if s == "1" { Some(true) } else { None }
8888
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(s == "1").then(|| true)`
8989

9090
error: this could be simplified with `bool::then`
91-
--> tests/ui/if_then_some_else_none.rs:154:9
91+
--> tests/ui/if_then_some_else_none.rs:158:9
9292
|
9393
LL | / if rs.len() == 1 && rs[0].start == rs[0].end {
9494
LL | |
@@ -99,7 +99,7 @@ LL | | }
9999
| |_________^ help: try: `(rs.len() == 1 && rs[0].start == rs[0].end).then(|| vec![rs[0].start])`
100100

101101
error: this could be simplified with `bool::then_some`
102-
--> tests/ui/if_then_some_else_none.rs:164:9
102+
--> tests/ui/if_then_some_else_none.rs:168:9
103103
|
104104
LL | / if modulo == 0 {
105105
LL | |
@@ -110,7 +110,7 @@ LL | | }
110110
| |_________^ help: try: `(modulo == 0).then_some(i)`
111111

112112
error: this could be simplified with `bool::then_some`
113-
--> tests/ui/if_then_some_else_none.rs:181:22
113+
--> tests/ui/if_then_some_else_none.rs:185:22
114114
|
115115
LL | do_something(if i % 2 == 0 {
116116
| ______________________^
@@ -122,7 +122,7 @@ LL | | });
122122
| |_________^ help: try: `(i % 2 == 0).then_some(item_fn)`
123123

124124
error: this could be simplified with `bool::then_some`
125-
--> tests/ui/if_then_some_else_none.rs:198:22
125+
--> tests/ui/if_then_some_else_none.rs:202:22
126126
|
127127
LL | do_something(if i % 2 == 0 {
128128
| ______________________^
@@ -134,7 +134,7 @@ LL | | });
134134
| |_________^ help: try: `(i % 2 == 0).then_some(item_fn)`
135135

136136
error: this could be simplified with `bool::then_some`
137-
--> tests/ui/if_then_some_else_none.rs:206:22
137+
--> tests/ui/if_then_some_else_none.rs:210:22
138138
|
139139
LL | do_something(if i % 2 == 0 {
140140
| ______________________^
@@ -146,7 +146,7 @@ LL | | });
146146
| |_________^ help: try: `(i % 2 == 0).then_some(closure_fn)`
147147

148148
error: this could be simplified with `bool::then`
149-
--> tests/ui/if_then_some_else_none.rs:231:13
149+
--> tests/ui/if_then_some_else_none.rs:235:13
150150
|
151151
LL | / if self.count < 5 {
152152
LL | | self.count += 1;
@@ -165,7 +165,7 @@ LL + })
165165
|
166166

167167
error: this could be simplified with `bool::then`
168-
--> tests/ui/if_then_some_else_none.rs:249:13
168+
--> tests/ui/if_then_some_else_none.rs:253:13
169169
|
170170
LL | let _ = if true {
171171
| _____________^
@@ -185,7 +185,7 @@ LL ~ });
185185
|
186186

187187
error: this could be simplified with `bool::then`
188-
--> tests/ui/if_then_some_else_none.rs:292:5
188+
--> tests/ui/if_then_some_else_none.rs:296:5
189189
|
190190
LL | / if 1 <= 3 {
191191
LL | | let a = UnsafeCell::new(1);

‎tests/ui/manual_filter.fixed‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![warn(clippy::manual_filter)]
2-
#![allow(unused_variables, dead_code, clippy::useless_vec)]
2+
#![allow(unused_variables, dead_code, clippy::useless_vec, clippy::nonminimal_bool)]
33

44
fn main() {
55
Some(0).filter(|&x| x <= 0);
@@ -156,3 +156,16 @@ fn main() {
156156
fn maybe_some() -> Option<u32> {
157157
Some(0)
158158
}
159+
160+
fn issue14440(opt: Option<i32>) {
161+
opt.filter(|&x| x != 0);
162+
//~^ manual_filter
163+
164+
let y = 1i32;
165+
opt.filter(move |&x| x == y);
166+
//~^ manual_filter
167+
168+
let opt1 = Some("123".to_string());
169+
opt1.filter(|s| s.len() > 2);
170+
//~^ manual_filter
171+
}

0 commit comments

Comments
 (0)