Skip to content

Commit 2af95a4

Browse files
ada4aada4a
committed
manual_{map,filter}: lint exprs with adjustments, but with reduced Applicability 
1 parent deff3fd commit 2af95a4

13 files changed

+398
-209
lines changed

‎clippy_lints/src/matches/manual_filter.rs‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use clippy_utils::as_some_expr;
2-
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::diagnostics::span_lint_and_then;
33
use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
44
use clippy_utils::visitors::contains_unsafe_block;
55

@@ -9,7 +9,7 @@ use rustc_lint::LateContext;
99
use rustc_span::{SyntaxContext, sym};
1010

1111
use super::MANUAL_FILTER;
12-
use super::manual_utils::{SomeExpr, check_with};
12+
use super::manual_utils::{SomeExpr, SuggInfo, check_with};
1313

1414
// Function called on the <expr> of `[&+]Some((ref | ref mut) x) => <expr>`
1515
// Need to check if it's of the form `<expr>=if <cond> {<then_expr>} else {<else_expr>}`
@@ -133,7 +133,15 @@ fn check<'tcx>(
133133
else_pat: Option<&'tcx Pat<'_>>,
134134
else_body: &'tcx Expr<'_>,
135135
) {
136-
if let Some(sugg_info) = check_with(
136+
if let Some(SuggInfo {
137+
needs_brackets,
138+
scrutinee_impl_copy,
139+
scrutinee_str,
140+
as_ref_str,
141+
body_str,
142+
app,
143+
requires_coercion,
144+
}) = check_with(
137145
cx,
138146
expr,
139147
scrutinee,
@@ -143,22 +151,27 @@ fn check<'tcx>(
143151
else_body,
144152
get_cond_expr,
145153
) {
146-
let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy);
147-
span_lint_and_sugg(
154+
let body_str = add_ampersand_if_copy(body_str, scrutinee_impl_copy);
155+
span_lint_and_then(
148156
cx,
149157
MANUAL_FILTER,
150158
expr.span,
151159
"manual implementation of `Option::filter`",
152-
"try",
153-
if sugg_info.needs_brackets {
154-
format!(
155-
"{{ {}{}.filter({body_str}) }}",
156-
sugg_info.scrutinee_str, sugg_info.as_ref_str
157-
)
158-
} else {
159-
format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str)
160+
|diag| {
161+
diag.span_suggestion(
162+
expr.span,
163+
"try",
164+
if needs_brackets {
165+
format!("{{ {scrutinee_str}{as_ref_str}.filter({body_str}) }}")
166+
} else {
167+
format!("{scrutinee_str}{as_ref_str}.filter({body_str})")
168+
},
169+
app,
170+
);
171+
if requires_coercion {
172+
diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
173+
}
160174
},
161-
sugg_info.app,
162175
);
163176
}
164177
}

‎clippy_lints/src/matches/manual_map.rs‎

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use super::MANUAL_MAP;
2-
use super::manual_utils::{SomeExpr, check_with};
3-
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use super::manual_utils::{SomeExpr, SuggInfo, check_with};
3+
use clippy_utils::diagnostics::span_lint_and_then;
44

55
use clippy_utils::res::{MaybeDef, MaybeQPath};
66
use rustc_hir::LangItem::OptionSome;
@@ -42,7 +42,15 @@ fn check<'tcx>(
4242
else_pat: Option<&'tcx Pat<'_>>,
4343
else_body: &'tcx Expr<'_>,
4444
) {
45-
if let Some(sugg_info) = check_with(
45+
if let Some(SuggInfo {
46+
needs_brackets,
47+
scrutinee_impl_copy: _,
48+
scrutinee_str,
49+
as_ref_str,
50+
body_str,
51+
app,
52+
requires_coercion,
53+
}) = check_with(
4654
cx,
4755
expr,
4856
scrutinee,
@@ -52,24 +60,26 @@ fn check<'tcx>(
5260
else_body,
5361
get_some_expr,
5462
) {
55-
span_lint_and_sugg(
63+
span_lint_and_then(
5664
cx,
5765
MANUAL_MAP,
5866
expr.span,
5967
"manual implementation of `Option::map`",
60-
"try",
61-
if sugg_info.needs_brackets {
62-
format!(
63-
"{{ {}{}.map({}) }}",
64-
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
65-
)
66-
} else {
67-
format!(
68-
"{}{}.map({})",
69-
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
70-
)
68+
|diag| {
69+
diag.span_suggestion(
70+
expr.span,
71+
"try",
72+
if needs_brackets {
73+
format!("{{ {scrutinee_str}{as_ref_str}.map({body_str}) }}")
74+
} else {
75+
format!("{scrutinee_str}{as_ref_str}.map({body_str})")
76+
},
77+
app,
78+
);
79+
if requires_coercion {
80+
diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
81+
}
7182
},
72-
sugg_info.app,
7383
);
7484
}
7585
}

‎clippy_lints/src/matches/manual_utils.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,12 @@ where
7272
return None;
7373
}
7474

75+
let mut app = Applicability::MachineApplicable;
76+
7577
// `map` won't perform any adjustments.
76-
if expr_requires_coercion(cx, expr) {
77-
return None;
78+
let requires_coercion = expr_requires_coercion(cx, expr);
79+
if requires_coercion {
80+
app = Applicability::MaybeIncorrect;
7881
}
7982

8083
// Determine which binding mode to use.
@@ -111,8 +114,6 @@ where
111114
None => return None,
112115
}
113116

114-
let mut app = Applicability::MachineApplicable;
115-
116117
// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
117118
// it's being passed by value.
118119
let scrutinee = peel_hir_expr_refs(scrutinee).0;
@@ -172,6 +173,7 @@ where
172173
as_ref_str,
173174
body_str,
174175
app,
176+
requires_coercion,
175177
})
176178
}
177179

@@ -182,6 +184,7 @@ pub struct SuggInfo<'a> {
182184
pub as_ref_str: &'a str,
183185
pub body_str: String,
184186
pub app: Applicability,
187+
pub requires_coercion: bool,
185188
}
186189

187190
// Checks whether the expression could be passed as a function, or whether a closure is needed.

‎tests/ui/manual_filter_unfixable.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
//@no-rustfix
2+
#![warn(clippy::manual_filter)]
3+
4+
// the suggestion is missing the necessary adjustments
5+
fn issue16031() -> Option<&'static i32> {
6+
match Some(&&0) {
7+
//~^ manual_filter
8+
None => None,
9+
Some(x) => {
10+
if **x > 0 {
11+
None
12+
} else {
13+
Some(x)
14+
}
15+
},
16+
}
17+
}

‎tests/ui/manual_filter_unfixable.stderr‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
error: manual implementation of `Option::filter`
2+
--> tests/ui/manual_filter_unfixable.rs:6:5
3+
|
4+
LL | / match Some(&&0) {
5+
LL | |
6+
LL | | None => None,
7+
LL | | Some(x) => {
8+
... |
9+
LL | | },
10+
LL | | }
11+
| |_____^ help: try: `Some(&&0).filter(|&x| **x <= 0)`
12+
|
13+
= note: you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type
14+
= note: `-D clippy::manual-filter` implied by `-D warnings`
15+
= help: to override `-D warnings` add `#[allow(clippy::manual_filter)]`
16+
17+
error: aborting due to 1 previous error
18+

‎tests/ui/manual_map_option.fixed‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -113,16 +113,7 @@ fn main() {
113113
}
114114

115115
// #6811
116-
match Some(0) {
117-
Some(x) => Some(vec![x]),
118-
None => None,
119-
};
120-
121-
// Don't lint, coercion
122-
let x: Option<Vec<&[u8]>> = match Some(()) {
123-
Some(_) => Some(vec![b"1234"]),
124-
None => None,
125-
};
116+
Some(0).map(|x| vec![x]);
126117

127118
option_env!("").map(String::from);
128119

@@ -154,12 +145,4 @@ fn main() {
154145
None => None,
155146
};
156147
}
157-
158-
// #7077
159-
let s = &String::new();
160-
#[allow(clippy::needless_match)]
161-
let _: Option<&str> = match Some(s) {
162-
Some(s) => Some(s),
163-
None => None,
164-
};
165148
}

‎tests/ui/manual_map_option.rs‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -183,16 +183,11 @@ fn main() {
183183

184184
// #6811
185185
match Some(0) {
186+
//~^ manual_map
186187
Some(x) => Some(vec![x]),
187188
None => None,
188189
};
189190

190-
// Don't lint, coercion
191-
let x: Option<Vec<&[u8]>> = match Some(()) {
192-
Some(_) => Some(vec![b"1234"]),
193-
None => None,
194-
};
195-
196191
match option_env!("") {
197192
//~^ manual_map
198193
Some(x) => Some(String::from(x)),
@@ -237,12 +232,4 @@ fn main() {
237232
None => None,
238233
};
239234
}
240-
241-
// #7077
242-
let s = &String::new();
243-
#[allow(clippy::needless_match)]
244-
let _: Option<&str> = match Some(s) {
245-
Some(s) => Some(s),
246-
None => None,
247-
};
248235
}

‎tests/ui/manual_map_option.stderr‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,19 @@ LL | | };
173173
| |_____^ help: try: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
174174

175175
error: manual implementation of `Option::map`
176-
--> tests/ui/manual_map_option.rs:196:5
176+
--> tests/ui/manual_map_option.rs:185:5
177+
|
178+
LL | / match Some(0) {
179+
LL | |
180+
LL | | Some(x) => Some(vec![x]),
181+
LL | | None => None,
182+
LL | | };
183+
| |_____^ help: try: `Some(0).map(|x| vec![x])`
184+
|
185+
= note: you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type
186+
187+
error: manual implementation of `Option::map`
188+
--> tests/ui/manual_map_option.rs:191:5
177189
|
178190
LL | / match option_env!("") {
179191
LL | |
@@ -183,7 +195,7 @@ LL | | };
183195
| |_____^ help: try: `option_env!("").map(String::from)`
184196

185197
error: manual implementation of `Option::map`
186-
--> tests/ui/manual_map_option.rs:217:12
198+
--> tests/ui/manual_map_option.rs:212:12
187199
|
188200
LL | } else if let Some(x) = Some(0) {
189201
| ____________^
@@ -195,7 +207,7 @@ LL | | };
195207
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
196208

197209
error: manual implementation of `Option::map`
198-
--> tests/ui/manual_map_option.rs:226:12
210+
--> tests/ui/manual_map_option.rs:221:12
199211
|
200212
LL | } else if let Some(x) = Some(0) {
201213
| ____________^
@@ -206,5 +218,5 @@ LL | | None
206218
LL | | };
207219
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
208220

209-
error: aborting due to 20 previous errors
221+
error: aborting due to 21 previous errors
210222

0 commit comments

Comments
 (0)