Skip to content

Commit ce8d263

Browse files
samueltardieusamueltardieu
authored
Fixes manual_flatten removes the useless if let (#14861)
Closes rust-lang/rust-clippy#14692 The suggestion of `manual_flatten` does not includes the replacement of `if let` so far despite of `.flatten()` suggestion. This PR eliminates a redundant `if let`. changelog: [`manual_flatten`] the suggestion removes `if let`
2 parents 6d7c16a + 358b80c commit ce8d263

File tree

4 files changed

+307
-57
lines changed

4 files changed

+307
-57
lines changed

‎clippy_lints/src/loops/manual_flatten.rs

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@ use super::MANUAL_FLATTEN;
22
use super::utils::make_iterator_snippet;
33
use clippy_utils::diagnostics::span_lint_and_then;
44
use clippy_utils::msrvs::{self, Msrv};
5+
use clippy_utils::source::{HasSession, indent_of, reindent_multiline, snippet_with_applicability};
56
use clippy_utils::visitors::is_local_used;
6-
use clippy_utils::{higher, is_refutable, path_to_local_id, peel_blocks_with_stmt};
7+
use clippy_utils::{higher, is_refutable, path_to_local_id, peel_blocks_with_stmt, span_contains_comment};
78
use rustc_errors::Applicability;
89
use rustc_hir::def::{DefKind, Res};
910
use rustc_hir::{Expr, Pat, PatKind};
@@ -39,13 +40,20 @@ pub(super) fn check<'tcx>(
3940
&& msrv.meets(cx, msrvs::ITER_FLATTEN)
4041
&& !is_refutable(cx, inner_pat)
4142
{
43+
if arg.span.from_expansion() || if_then.span.from_expansion() {
44+
return;
45+
}
4246
let if_let_type = if some_ctor { "Some" } else { "Ok" };
4347
// Prepare the error message
4448
let msg =
4549
format!("unnecessary `if let` since only the `{if_let_type}` variant of the iterator element is used");
4650

4751
// Prepare the help message
48-
let mut applicability = Applicability::MaybeIncorrect;
52+
let mut applicability = if span_contains_comment(cx.sess().source_map(), body.span) {
53+
Applicability::MaybeIncorrect
54+
} else {
55+
Applicability::MachineApplicable
56+
};
4957
let arg_snippet = make_iterator_snippet(cx, arg, &mut applicability);
5058
let copied = match cx.typeck_results().expr_ty(let_expr).kind() {
5159
ty::Ref(_, inner, _) => match inner.kind() {
@@ -55,20 +63,26 @@ pub(super) fn check<'tcx>(
5563
_ => "",
5664
};
5765

58-
let sugg = format!("{arg_snippet}{copied}.flatten()");
66+
let help_msg = "try `.flatten()` and remove the `if let` statement in the for loop";
5967

60-
// If suggestion is not a one-liner, it won't be shown inline within the error message. In that
61-
// case, it will be shown in the extra `help` message at the end, which is why the first
62-
// `help_msg` needs to refer to the correct relative position of the suggestion.
63-
let help_msg = if sugg.contains('\n') {
64-
"remove the `if let` statement in the for loop and then..."
65-
} else {
66-
"...and remove the `if let` statement in the for loop"
67-
};
68+
let pat_snippet =
69+
snippet_with_applicability(cx, inner_pat.span.source_callsite(), "_", &mut applicability).to_string();
70+
let body_snippet =
71+
snippet_with_applicability(cx, if_then.span.source_callsite(), "[body]", &mut applicability).to_string();
72+
let suggestions = vec![
73+
// flatten the iterator
74+
(arg.span, format!("{arg_snippet}{copied}.flatten()")),
75+
(pat.span, pat_snippet),
76+
// remove the `if let` statement
77+
(
78+
body.span,
79+
reindent_multiline(&body_snippet, true, indent_of(cx, body.span)),
80+
),
81+
];
6882

6983
span_lint_and_then(cx, MANUAL_FLATTEN, span, msg, |diag| {
70-
diag.span_suggestion(arg.span, "try", sugg, applicability);
7184
diag.span_help(inner_expr.span, help_msg);
85+
diag.multipart_suggestion("try", suggestions, applicability);
7286
});
7387
}
7488
}

‎tests/ui/manual_flatten.fixed

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
#![warn(clippy::manual_flatten)]
2+
#![allow(clippy::useless_vec, clippy::uninlined_format_args)]
3+
4+
fn main() {
5+
// Test for loop over implicitly adjusted `Iterator` with `if let` expression
6+
let x = vec![Some(1), Some(2), Some(3)];
7+
for y in x.into_iter().flatten() {
8+
println!("{}", y);
9+
}
10+
11+
// Test for loop over implicitly adjusted `Iterator` with `if let` statement
12+
let y: Vec<Result<i32, i32>> = vec![];
13+
for n in y.clone().into_iter().flatten() {
14+
println!("{}", n);
15+
}
16+
17+
// Test for loop over by reference
18+
for n in y.iter().flatten() {
19+
println!("{}", n);
20+
}
21+
22+
// Test for loop over an implicit reference
23+
let z = &y;
24+
for n in z.iter().flatten() {
25+
println!("{}", n);
26+
}
27+
28+
// Test for loop over `Iterator` with `if let` expression
29+
let z = vec![Some(1), Some(2), Some(3)];
30+
let z = z.iter();
31+
for m in z.flatten() {
32+
println!("{}", m);
33+
}
34+
35+
// Using the `None` variant should not trigger the lint
36+
// Note: for an autofixable suggestion, the binding in the for loop has to take the
37+
// name of the binding in the `if let`
38+
let z = vec![Some(1), Some(2), Some(3)];
39+
for n in z {
40+
if n.is_none() {
41+
println!("Nada.");
42+
}
43+
}
44+
45+
// Using the `Err` variant should not trigger the lint
46+
for n in y.clone() {
47+
if let Err(e) = n {
48+
println!("Oops: {}!", e);
49+
}
50+
}
51+
52+
// Having an else clause should not trigger the lint
53+
for n in y.clone() {
54+
if let Ok(n) = n {
55+
println!("{}", n);
56+
} else {
57+
println!("Oops!");
58+
}
59+
}
60+
61+
let vec_of_ref = vec![&Some(1)];
62+
for n in vec_of_ref.iter().copied().flatten() {
63+
println!("{:?}", n);
64+
}
65+
66+
let vec_of_ref = &vec_of_ref;
67+
for n in vec_of_ref.iter().copied().flatten() {
68+
println!("{:?}", n);
69+
}
70+
71+
let slice_of_ref = &[&Some(1)];
72+
for n in slice_of_ref.iter().copied().flatten() {
73+
println!("{:?}", n);
74+
}
75+
76+
struct Test {
77+
a: usize,
78+
}
79+
80+
let mut vec_of_struct = [Some(Test { a: 1 }), None];
81+
82+
// Usage of `if let` expression should not trigger lint
83+
for n in vec_of_struct.iter_mut() {
84+
if let Some(z) = n {
85+
*n = None;
86+
}
87+
}
88+
89+
// Using manual flatten should not trigger the lint
90+
for n in vec![Some(1), Some(2), Some(3)].iter().flatten() {
91+
println!("{}", n);
92+
}
93+
94+
// Using nested `Some` pattern should not trigger the lint
95+
for n in vec![Some((1, Some(2)))] {
96+
if let Some((_, Some(n))) = n {
97+
println!("{}", n);
98+
}
99+
}
100+
101+
macro_rules! inner {
102+
($id:ident / $new:pat => $action:expr) => {
103+
if let Some($new) = $id {
104+
$action;
105+
}
106+
};
107+
}
108+
109+
// Usage of `if let` expression with macro should not trigger lint
110+
for ab in [Some((1, 2)), Some((3, 4))] {
111+
inner!(ab / (c, d) => println!("{c}-{d}"));
112+
}
113+
114+
macro_rules! args {
115+
($($arg:expr),*) => {
116+
vec![$(Some($arg)),*]
117+
};
118+
}
119+
120+
// Usage of `if let` expression with macro should not trigger lint
121+
for n in args!(1, 2, 3) {
122+
if let Some(n) = n {
123+
println!("{:?}", n);
124+
}
125+
}
126+
127+
// This should trigger the lint, but the applicability is `MaybeIncorrect`
128+
let z = vec![Some(1), Some(2), Some(3)];
129+
for n in z.into_iter().flatten() {
130+
println!("{:?}", n);
131+
}
132+
133+
run_unformatted_tests();
134+
}
135+
136+
#[rustfmt::skip]
137+
fn run_unformatted_tests() {
138+
// Skip rustfmt here on purpose so the suggestion does not fit in one line
139+
for n in vec![
140+
//~^ manual_flatten
141+
142+
Some(1),
143+
Some(2),
144+
Some(3)
145+
].iter().flatten() {
146+
println!("{:?}", n);
147+
}
148+
}

‎tests/ui/manual_flatten.rs

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![warn(clippy::manual_flatten)]
22
#![allow(clippy::useless_vec, clippy::uninlined_format_args)]
3-
//@no-rustfix
3+
44
fn main() {
55
// Test for loop over implicitly adjusted `Iterator` with `if let` expression
66
let x = vec![Some(1), Some(2), Some(3)];
@@ -130,6 +130,43 @@ fn main() {
130130
}
131131
}
132132

133+
macro_rules! inner {
134+
($id:ident / $new:pat => $action:expr) => {
135+
if let Some($new) = $id {
136+
$action;
137+
}
138+
};
139+
}
140+
141+
// Usage of `if let` expression with macro should not trigger lint
142+
for ab in [Some((1, 2)), Some((3, 4))] {
143+
inner!(ab / (c, d) => println!("{c}-{d}"));
144+
}
145+
146+
macro_rules! args {
147+
($($arg:expr),*) => {
148+
vec![$(Some($arg)),*]
149+
};
150+
}
151+
152+
// Usage of `if let` expression with macro should not trigger lint
153+
for n in args!(1, 2, 3) {
154+
if let Some(n) = n {
155+
println!("{:?}", n);
156+
}
157+
}
158+
159+
// This should trigger the lint, but the applicability is `MaybeIncorrect`
160+
let z = vec![Some(1), Some(2), Some(3)];
161+
for n in z {
162+
//~^ manual_flatten
163+
164+
if let Some(n) = n {
165+
println!("{:?}", n);
166+
}
167+
// foo
168+
}
169+
133170
run_unformatted_tests();
134171
}
135172

0 commit comments

Comments
 (0)