diff options
| -rw-r--r-- | clippy_lints/src/loops/manual_flatten.rs | 38 | ||||
| -rw-r--r-- | tests/ui/manual_flatten.fixed | 148 | ||||
| -rw-r--r-- | tests/ui/manual_flatten.rs | 39 | ||||
| -rw-r--r-- | tests/ui/manual_flatten.stderr | 139 |
4 files changed, 307 insertions, 57 deletions
diff --git a/clippy_lints/src/loops/manual_flatten.rs b/clippy_lints/src/loops/manual_flatten.rs index 81f14b7b2b0..ddb8bb536c0 100644 --- a/clippy_lints/src/loops/manual_flatten.rs +++ b/clippy_lints/src/loops/manual_flatten.rs @@ -2,8 +2,9 @@ use super::MANUAL_FLATTEN; use super::utils::make_iterator_snippet; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::source::{HasSession, indent_of, reindent_multiline, snippet_with_applicability}; use clippy_utils::visitors::is_local_used; -use clippy_utils::{higher, is_refutable, path_to_local_id, peel_blocks_with_stmt}; +use clippy_utils::{higher, is_refutable, path_to_local_id, peel_blocks_with_stmt, span_contains_comment}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{Expr, Pat, PatKind}; @@ -39,13 +40,20 @@ pub(super) fn check<'tcx>( && msrv.meets(cx, msrvs::ITER_FLATTEN) && !is_refutable(cx, inner_pat) { + if arg.span.from_expansion() || if_then.span.from_expansion() { + return; + } let if_let_type = if some_ctor { "Some" } else { "Ok" }; // Prepare the error message let msg = format!("unnecessary `if let` since only the `{if_let_type}` variant of the iterator element is used"); // Prepare the help message - let mut applicability = Applicability::MaybeIncorrect; + let mut applicability = if span_contains_comment(cx.sess().source_map(), body.span) { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }; let arg_snippet = make_iterator_snippet(cx, arg, &mut applicability); let copied = match cx.typeck_results().expr_ty(let_expr).kind() { ty::Ref(_, inner, _) => match inner.kind() { @@ -55,20 +63,26 @@ pub(super) fn check<'tcx>( _ => "", }; - let sugg = format!("{arg_snippet}{copied}.flatten()"); + let help_msg = "try `.flatten()` and remove the `if let` statement in the for loop"; - // If suggestion is not a one-liner, it won't be shown inline within the error message. In that - // case, it will be shown in the extra `help` message at the end, which is why the first - // `help_msg` needs to refer to the correct relative position of the suggestion. - let help_msg = if sugg.contains('\n') { - "remove the `if let` statement in the for loop and then..." - } else { - "...and remove the `if let` statement in the for loop" - }; + let pat_snippet = + snippet_with_applicability(cx, inner_pat.span.source_callsite(), "_", &mut applicability).to_string(); + let body_snippet = + snippet_with_applicability(cx, if_then.span.source_callsite(), "[body]", &mut applicability).to_string(); + let suggestions = vec![ + // flatten the iterator + (arg.span, format!("{arg_snippet}{copied}.flatten()")), + (pat.span, pat_snippet), + // remove the `if let` statement + ( + body.span, + reindent_multiline(&body_snippet, true, indent_of(cx, body.span)), + ), + ]; span_lint_and_then(cx, MANUAL_FLATTEN, span, msg, |diag| { - diag.span_suggestion(arg.span, "try", sugg, applicability); diag.span_help(inner_expr.span, help_msg); + diag.multipart_suggestion("try", suggestions, applicability); }); } } diff --git a/tests/ui/manual_flatten.fixed b/tests/ui/manual_flatten.fixed new file mode 100644 index 00000000000..cc1fbd25765 --- /dev/null +++ b/tests/ui/manual_flatten.fixed @@ -0,0 +1,148 @@ +#![warn(clippy::manual_flatten)] +#![allow(clippy::useless_vec, clippy::uninlined_format_args)] + +fn main() { + // Test for loop over implicitly adjusted `Iterator` with `if let` expression + let x = vec![Some(1), Some(2), Some(3)]; + for y in x.into_iter().flatten() { + println!("{}", y); + } + + // Test for loop over implicitly adjusted `Iterator` with `if let` statement + let y: Vec<Result<i32, i32>> = vec![]; + for n in y.clone().into_iter().flatten() { + println!("{}", n); + } + + // Test for loop over by reference + for n in y.iter().flatten() { + println!("{}", n); + } + + // Test for loop over an implicit reference + let z = &y; + for n in z.iter().flatten() { + println!("{}", n); + } + + // Test for loop over `Iterator` with `if let` expression + let z = vec![Some(1), Some(2), Some(3)]; + let z = z.iter(); + for m in z.flatten() { + println!("{}", m); + } + + // Using the `None` variant should not trigger the lint + // Note: for an autofixable suggestion, the binding in the for loop has to take the + // name of the binding in the `if let` + let z = vec![Some(1), Some(2), Some(3)]; + for n in z { + if n.is_none() { + println!("Nada."); + } + } + + // Using the `Err` variant should not trigger the lint + for n in y.clone() { + if let Err(e) = n { + println!("Oops: {}!", e); + } + } + + // Having an else clause should not trigger the lint + for n in y.clone() { + if let Ok(n) = n { + println!("{}", n); + } else { + println!("Oops!"); + } + } + + let vec_of_ref = vec![&Some(1)]; + for n in vec_of_ref.iter().copied().flatten() { + println!("{:?}", n); + } + + let vec_of_ref = &vec_of_ref; + for n in vec_of_ref.iter().copied().flatten() { + println!("{:?}", n); + } + + let slice_of_ref = &[&Some(1)]; + for n in slice_of_ref.iter().copied().flatten() { + println!("{:?}", n); + } + + struct Test { + a: usize, + } + + let mut vec_of_struct = [Some(Test { a: 1 }), None]; + + // Usage of `if let` expression should not trigger lint + for n in vec_of_struct.iter_mut() { + if let Some(z) = n { + *n = None; + } + } + + // Using manual flatten should not trigger the lint + for n in vec![Some(1), Some(2), Some(3)].iter().flatten() { + println!("{}", n); + } + + // Using nested `Some` pattern should not trigger the lint + for n in vec![Some((1, Some(2)))] { + if let Some((_, Some(n))) = n { + println!("{}", n); + } + } + + macro_rules! inner { + ($id:ident / $new:pat => $action:expr) => { + if let Some($new) = $id { + $action; + } + }; + } + + // Usage of `if let` expression with macro should not trigger lint + for ab in [Some((1, 2)), Some((3, 4))] { + inner!(ab / (c, d) => println!("{c}-{d}")); + } + + macro_rules! args { + ($($arg:expr),*) => { + vec![$(Some($arg)),*] + }; + } + + // Usage of `if let` expression with macro should not trigger lint + for n in args!(1, 2, 3) { + if let Some(n) = n { + println!("{:?}", n); + } + } + + // This should trigger the lint, but the applicability is `MaybeIncorrect` + let z = vec![Some(1), Some(2), Some(3)]; + for n in z.into_iter().flatten() { + println!("{:?}", n); + } + + run_unformatted_tests(); +} + +#[rustfmt::skip] +fn run_unformatted_tests() { + // Skip rustfmt here on purpose so the suggestion does not fit in one line + for n in vec![ + //~^ manual_flatten + + Some(1), + Some(2), + Some(3) + ].iter().flatten() { + println!("{:?}", n); + } +} diff --git a/tests/ui/manual_flatten.rs b/tests/ui/manual_flatten.rs index f1a0053ef38..53b4ac7d3b6 100644 --- a/tests/ui/manual_flatten.rs +++ b/tests/ui/manual_flatten.rs @@ -1,6 +1,6 @@ #![warn(clippy::manual_flatten)] #![allow(clippy::useless_vec, clippy::uninlined_format_args)] -//@no-rustfix + fn main() { // Test for loop over implicitly adjusted `Iterator` with `if let` expression let x = vec![Some(1), Some(2), Some(3)]; @@ -130,6 +130,43 @@ fn main() { } } + macro_rules! inner { + ($id:ident / $new:pat => $action:expr) => { + if let Some($new) = $id { + $action; + } + }; + } + + // Usage of `if let` expression with macro should not trigger lint + for ab in [Some((1, 2)), Some((3, 4))] { + inner!(ab / (c, d) => println!("{c}-{d}")); + } + + macro_rules! args { + ($($arg:expr),*) => { + vec![$(Some($arg)),*] + }; + } + + // Usage of `if let` expression with macro should not trigger lint + for n in args!(1, 2, 3) { + if let Some(n) = n { + println!("{:?}", n); + } + } + + // This should trigger the lint, but the applicability is `MaybeIncorrect` + let z = vec![Some(1), Some(2), Some(3)]; + for n in z { + //~^ manual_flatten + + if let Some(n) = n { + println!("{:?}", n); + } + // foo + } + run_unformatted_tests(); } diff --git a/tests/ui/manual_flatten.stderr b/tests/ui/manual_flatten.stderr index 9a846fe17f3..eb39ee42071 100644 --- a/tests/ui/manual_flatten.stderr +++ b/tests/ui/manual_flatten.stderr @@ -1,10 +1,7 @@ error: unnecessary `if let` since only the `Some` variant of the iterator element is used --> tests/ui/manual_flatten.rs:7:5 | -LL | for n in x { - | ^ - help: try: `x.into_iter().flatten()` - | _____| - | | +LL | / for n in x { LL | | LL | | LL | | if let Some(y) = n { @@ -12,7 +9,7 @@ LL | | if let Some(y) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:10:9 | LL | / if let Some(y) = n { @@ -21,14 +18,17 @@ LL | | } | |_________^ = note: `-D clippy::manual-flatten` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_flatten)]` +help: try + | +LL ~ for y in x.into_iter().flatten() { +LL + println!("{}", y); +LL + } + | error: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> tests/ui/manual_flatten.rs:17:5 | -LL | for n in y.clone() { - | ^ --------- help: try: `y.clone().into_iter().flatten()` - | _____| - | | +LL | / for n in y.clone() { LL | | LL | | LL | | if let Ok(n) = n { @@ -37,21 +37,24 @@ LL | | }; LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:20:9 | LL | / if let Ok(n) = n { LL | | println!("{}", n); LL | | }; | |_________^ +help: try + | +LL ~ for n in y.clone().into_iter().flatten() { +LL + println!("{}", n); +LL + } + | error: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> tests/ui/manual_flatten.rs:26:5 | -LL | for n in &y { - | ^ -- help: try: `y.iter().flatten()` - | _____| - | | +LL | / for n in &y { LL | | LL | | LL | | if let Ok(n) = n { @@ -59,21 +62,24 @@ LL | | if let Ok(n) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:29:9 | LL | / if let Ok(n) = n { LL | | println!("{}", n); LL | | } | |_________^ +help: try + | +LL ~ for n in y.iter().flatten() { +LL + println!("{}", n); +LL + } + | error: unnecessary `if let` since only the `Ok` variant of the iterator element is used --> tests/ui/manual_flatten.rs:36:5 | -LL | for n in z { - | ^ - help: try: `z.iter().flatten()` - | _____| - | | +LL | / for n in z { LL | | LL | | LL | | if let Ok(n) = n { @@ -81,21 +87,24 @@ LL | | if let Ok(n) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:39:9 | LL | / if let Ok(n) = n { LL | | println!("{}", n); LL | | } | |_________^ +help: try + | +LL ~ for n in z.iter().flatten() { +LL + println!("{}", n); +LL + } + | error: unnecessary `if let` since only the `Some` variant of the iterator element is used --> tests/ui/manual_flatten.rs:47:5 | -LL | for n in z { - | ^ - help: try: `z.flatten()` - | _____| - | | +LL | / for n in z { LL | | LL | | LL | | if let Some(m) = n { @@ -103,21 +112,24 @@ LL | | if let Some(m) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:50:9 | LL | / if let Some(m) = n { LL | | println!("{}", m); LL | | } | |_________^ +help: try + | +LL ~ for m in z.flatten() { +LL + println!("{}", m); +LL + } + | error: unnecessary `if let` since only the `Some` variant of the iterator element is used --> tests/ui/manual_flatten.rs:82:5 | -LL | for n in &vec_of_ref { - | ^ ----------- help: try: `vec_of_ref.iter().copied().flatten()` - | _____| - | | +LL | / for n in &vec_of_ref { LL | | LL | | LL | | if let Some(n) = n { @@ -125,21 +137,24 @@ LL | | if let Some(n) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:85:9 | LL | / if let Some(n) = n { LL | | println!("{:?}", n); LL | | } | |_________^ +help: try + | +LL ~ for n in vec_of_ref.iter().copied().flatten() { +LL + println!("{:?}", n); +LL + } + | error: unnecessary `if let` since only the `Some` variant of the iterator element is used --> tests/ui/manual_flatten.rs:91:5 | -LL | for n in vec_of_ref { - | ^ ---------- help: try: `vec_of_ref.iter().copied().flatten()` - | _____| - | | +LL | / for n in vec_of_ref { LL | | LL | | LL | | if let Some(n) = n { @@ -147,21 +162,24 @@ LL | | if let Some(n) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:94:9 | LL | / if let Some(n) = n { LL | | println!("{:?}", n); LL | | } | |_________^ +help: try + | +LL ~ for n in vec_of_ref.iter().copied().flatten() { +LL + println!("{:?}", n); +LL + } + | error: unnecessary `if let` since only the `Some` variant of the iterator element is used --> tests/ui/manual_flatten.rs:100:5 | -LL | for n in slice_of_ref { - | ^ ------------ help: try: `slice_of_ref.iter().copied().flatten()` - | _____| - | | +LL | / for n in slice_of_ref { LL | | LL | | LL | | if let Some(n) = n { @@ -169,16 +187,47 @@ LL | | if let Some(n) = n { LL | | } | |_____^ | -help: ...and remove the `if let` statement in the for loop +help: try `.flatten()` and remove the `if let` statement in the for loop --> tests/ui/manual_flatten.rs:103:9 | LL | / if let Some(n) = n { LL | | println!("{:?}", n); LL | | } | |_________^ +help: try + | +LL ~ for n in slice_of_ref.iter().copied().flatten() { +LL + println!("{:?}", n); +LL + } + | + +error: unnecessary `if let` since only the `Some` variant of the iterator element is used + --> tests/ui/manual_flatten.rs:161:5 + | +LL | / for n in z { +LL | | +LL | | +LL | | if let Some(n) = n { +... | +LL | | } + | |_____^ + | +help: try `.flatten()` and remove the `if let` statement in the for loop + --> tests/ui/manual_flatten.rs:164:9 + | +LL | / if let Some(n) = n { +LL | | println!("{:?}", n); +LL | | } + | |_________^ +help: try + | +LL ~ for n in z.into_iter().flatten() { +LL + println!("{:?}", n); +LL + } + | error: unnecessary `if let` since only the `Some` variant of the iterator element is used - --> tests/ui/manual_flatten.rs:139:5 + --> tests/ui/manual_flatten.rs:176:5 | LL | / for n in vec![ LL | | @@ -188,8 +237,8 @@ LL | | Some(1), LL | | } | |_____^ | -help: remove the `if let` statement in the for loop and then... - --> tests/ui/manual_flatten.rs:146:9 +help: try `.flatten()` and remove the `if let` statement in the for loop + --> tests/ui/manual_flatten.rs:183:9 | LL | / if let Some(n) = n { LL | | println!("{:?}", n); @@ -201,7 +250,9 @@ LL | for n in vec![ ... LL | Some(3) LL ~ ].iter().flatten() { +LL + println!("{:?}", n); +LL + } | -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors |