From 5a49918f36adc9fb176a9be59b6fc2afd46c3572 Mon Sep 17 00:00:00 2001 From: DevAccentor Date: Sat, 4 Jun 2022 11:03:11 +0200 Subject: [PATCH] improve for_loops_over_fallibles to detect the usage of iter, iter_mut and into_iterator --- .../src/loops/for_loops_over_fallibles.rs | 40 +++++++++++++----- clippy_lints/src/loops/mod.rs | 12 +++++- tests/ui/for_loops_over_fallibles.rs | 17 +++++++- tests/ui/for_loops_over_fallibles.stderr | 40 ++++++++++++++---- tests/ui/manual_map_option.fixed | 1 + tests/ui/manual_map_option.rs | 1 + tests/ui/manual_map_option.stderr | 42 +++++++++---------- 7 files changed, 110 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/loops/for_loops_over_fallibles.rs b/clippy_lints/src/loops/for_loops_over_fallibles.rs index 90530ebf003..77de90fd7b9 100644 --- a/clippy_lints/src/loops/for_loops_over_fallibles.rs +++ b/clippy_lints/src/loops/for_loops_over_fallibles.rs @@ -7,9 +7,22 @@ use rustc_span::symbol::sym; /// Checks for `for` loops over `Option`s and `Result`s. -pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { +pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, method_name: Option<&str>) { let ty = cx.typeck_results().expr_ty(arg); if is_type_diagnostic_item(cx, ty, sym::Option) { + let help_string = if let Some(method_name) = method_name { + format!( + "consider replacing `for {0} in {1}.{method_name}()` with `if let Some({0}) = {1}`", + snippet(cx, pat.span, "_"), + snippet(cx, arg.span, "_") + ) + } else { + format!( + "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`", + snippet(cx, pat.span, "_"), + snippet(cx, arg.span, "_") + ) + }; span_lint_and_help( cx, FOR_LOOPS_OVER_FALLIBLES, @@ -20,13 +33,22 @@ pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { snippet(cx, arg.span, "_") ), None, - &format!( - "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ), + &help_string, ); } else if is_type_diagnostic_item(cx, ty, sym::Result) { + let help_string = if let Some(method_name) = method_name { + format!( + "consider replacing `for {0} in {1}.{method_name}()` with `if let Ok({0}) = {1}`", + snippet(cx, pat.span, "_"), + snippet(cx, arg.span, "_") + ) + } else { + format!( + "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`", + snippet(cx, pat.span, "_"), + snippet(cx, arg.span, "_") + ) + }; span_lint_and_help( cx, FOR_LOOPS_OVER_FALLIBLES, @@ -37,11 +59,7 @@ pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { snippet(cx, arg.span, "_") ), None, - &format!( - "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ), + &help_string, ); } } diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index d61be78895f..e32b228e104 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -188,6 +188,10 @@ /// for x in &res { /// // .. /// } + /// + /// for x in res.iter() { + /// // .. + /// } /// ``` /// /// Use instead: @@ -695,10 +699,14 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { let method_name = method.ident.as_str(); // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x match method_name { - "iter" | "iter_mut" => explicit_iter_loop::check(cx, self_arg, arg, method_name), + "iter" | "iter_mut" => { + explicit_iter_loop::check(cx, self_arg, arg, method_name); + for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name)); + }, "into_iter" => { explicit_iter_loop::check(cx, self_arg, arg, method_name); explicit_into_iter_loop::check(cx, self_arg, arg); + for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name)); }, "next" => { next_loop_linted = iter_next_loop::check(cx, arg); @@ -708,6 +716,6 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { } if !next_loop_linted { - for_loops_over_fallibles::check(cx, pat, arg); + for_loops_over_fallibles::check(cx, pat, arg, None); } } diff --git a/tests/ui/for_loops_over_fallibles.rs b/tests/ui/for_loops_over_fallibles.rs index 1b9dde87cd5..bde78a9fd4a 100644 --- a/tests/ui/for_loops_over_fallibles.rs +++ b/tests/ui/for_loops_over_fallibles.rs @@ -2,7 +2,7 @@ fn for_loops_over_fallibles() { let option = Some(1); - let result = option.ok_or("x not found"); + let mut result = option.ok_or("x not found"); let v = vec![0, 1, 2]; // check over an `Option` @@ -10,11 +10,26 @@ fn for_loops_over_fallibles() { println!("{}", x); } + // check over an `Option` + for x in option.iter() { + println!("{}", x); + } + // check over a `Result` for x in result { println!("{}", x); } + // check over a `Result` + for x in result.into_iter() { + println!("{}", x); + } + + // check over a `Result` + for x in result.iter_mut() { + println!("{}", x); + } + for x in option.ok_or("x not found") { println!("{}", x); } diff --git a/tests/ui/for_loops_over_fallibles.stderr b/tests/ui/for_loops_over_fallibles.stderr index 52b94875aec..635e08182d5 100644 --- a/tests/ui/for_loops_over_fallibles.stderr +++ b/tests/ui/for_loops_over_fallibles.stderr @@ -7,16 +7,40 @@ LL | for x in option { = note: `-D clippy::for-loops-over-fallibles` implied by `-D warnings` = help: consider replacing `for x in option` with `if let Some(x) = option` -error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement +error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement --> $DIR/for_loops_over_fallibles.rs:14:14 | +LL | for x in option.iter() { + | ^^^^^^ + | + = help: consider replacing `for x in option.iter()` with `if let Some(x) = option` + +error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loops_over_fallibles.rs:19:14 + | LL | for x in result { | ^^^^^^ | = help: consider replacing `for x in result` with `if let Ok(x) = result` +error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loops_over_fallibles.rs:24:14 + | +LL | for x in result.into_iter() { + | ^^^^^^ + | + = help: consider replacing `for x in result.into_iter()` with `if let Ok(x) = result` + +error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loops_over_fallibles.rs:29:14 + | +LL | for x in result.iter_mut() { + | ^^^^^^ + | + = help: consider replacing `for x in result.iter_mut()` with `if let Ok(x) = result` + error: for loop over `option.ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:18:14 + --> $DIR/for_loops_over_fallibles.rs:33:14 | LL | for x in option.ok_or("x not found") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -24,7 +48,7 @@ LL | for x in option.ok_or("x not found") { = help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")` error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want - --> $DIR/for_loops_over_fallibles.rs:24:14 + --> $DIR/for_loops_over_fallibles.rs:39:14 | LL | for x in v.iter().next() { | ^^^^^^^^^^^^^^^ @@ -32,7 +56,7 @@ LL | for x in v.iter().next() { = note: `#[deny(clippy::iter_next_loop)]` on by default error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:29:14 + --> $DIR/for_loops_over_fallibles.rs:44:14 | LL | for x in v.iter().next().and(Some(0)) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -40,7 +64,7 @@ LL | for x in v.iter().next().and(Some(0)) { = help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))` error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:33:14 + --> $DIR/for_loops_over_fallibles.rs:48:14 | LL | for x in v.iter().next().ok_or("x not found") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -48,7 +72,7 @@ LL | for x in v.iter().next().ok_or("x not found") { = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")` error: this loop never actually loops - --> $DIR/for_loops_over_fallibles.rs:45:5 + --> $DIR/for_loops_over_fallibles.rs:60:5 | LL | / while let Some(x) = option { LL | | println!("{}", x); @@ -59,7 +83,7 @@ LL | | } = note: `#[deny(clippy::never_loop)]` on by default error: this loop never actually loops - --> $DIR/for_loops_over_fallibles.rs:51:5 + --> $DIR/for_loops_over_fallibles.rs:66:5 | LL | / while let Ok(x) = result { LL | | println!("{}", x); @@ -67,5 +91,5 @@ LL | | break; LL | | } | |_____^ -error: aborting due to 8 previous errors +error: aborting due to 11 previous errors diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed index fc6a7abca0e..a59da4ae10b 100644 --- a/tests/ui/manual_map_option.fixed +++ b/tests/ui/manual_map_option.fixed @@ -7,6 +7,7 @@ clippy::unit_arg, clippy::match_ref_pats, clippy::redundant_pattern_matching, + clippy::for_loops_over_fallibles, dead_code )] diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs index 16508270f64..0bdbefa51e8 100644 --- a/tests/ui/manual_map_option.rs +++ b/tests/ui/manual_map_option.rs @@ -7,6 +7,7 @@ clippy::unit_arg, clippy::match_ref_pats, clippy::redundant_pattern_matching, + clippy::for_loops_over_fallibles, dead_code )] diff --git a/tests/ui/manual_map_option.stderr b/tests/ui/manual_map_option.stderr index 0036b8151de..cdc2c0e62a9 100644 --- a/tests/ui/manual_map_option.stderr +++ b/tests/ui/manual_map_option.stderr @@ -1,5 +1,5 @@ error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:14:5 + --> $DIR/manual_map_option.rs:15:5 | LL | / match Some(0) { LL | | Some(_) => Some(2), @@ -10,7 +10,7 @@ LL | | }; = note: `-D clippy::manual-map` implied by `-D warnings` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:19:5 + --> $DIR/manual_map_option.rs:20:5 | LL | / match Some(0) { LL | | Some(x) => Some(x + 1), @@ -19,7 +19,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(|x| x + 1)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:24:5 + --> $DIR/manual_map_option.rs:25:5 | LL | / match Some("") { LL | | Some(x) => Some(x.is_empty()), @@ -28,7 +28,7 @@ LL | | }; | |_____^ help: try this: `Some("").map(|x| x.is_empty())` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:29:5 + --> $DIR/manual_map_option.rs:30:5 | LL | / if let Some(x) = Some(0) { LL | | Some(!x) @@ -38,7 +38,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(|x| !x)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:36:5 + --> $DIR/manual_map_option.rs:37:5 | LL | / match Some(0) { LL | | Some(x) => { Some(std::convert::identity(x)) } @@ -47,7 +47,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(std::convert::identity)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:41:5 + --> $DIR/manual_map_option.rs:42:5 | LL | / match Some(&String::new()) { LL | | Some(x) => Some(str::len(x)), @@ -56,7 +56,7 @@ LL | | }; | |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:51:5 + --> $DIR/manual_map_option.rs:52:5 | LL | / match &Some([0, 1]) { LL | | Some(x) => Some(x[0]), @@ -65,7 +65,7 @@ LL | | }; | |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:56:5 + --> $DIR/manual_map_option.rs:57:5 | LL | / match &Some(0) { LL | | &Some(x) => Some(x * 2), @@ -74,7 +74,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(|x| x * 2)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:61:5 + --> $DIR/manual_map_option.rs:62:5 | LL | / match Some(String::new()) { LL | | Some(ref x) => Some(x.is_empty()), @@ -83,7 +83,7 @@ LL | | }; | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:66:5 + --> $DIR/manual_map_option.rs:67:5 | LL | / match &&Some(String::new()) { LL | | Some(x) => Some(x.len()), @@ -92,7 +92,7 @@ LL | | }; | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:71:5 + --> $DIR/manual_map_option.rs:72:5 | LL | / match &&Some(0) { LL | | &&Some(x) => Some(x + x), @@ -101,7 +101,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(|x| x + x)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:84:9 + --> $DIR/manual_map_option.rs:85:9 | LL | / match &mut Some(String::new()) { LL | | Some(x) => Some(x.push_str("")), @@ -110,7 +110,7 @@ LL | | }; | |_________^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:90:5 + --> $DIR/manual_map_option.rs:91:5 | LL | / match &mut Some(String::new()) { LL | | Some(ref x) => Some(x.len()), @@ -119,7 +119,7 @@ LL | | }; | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:95:5 + --> $DIR/manual_map_option.rs:96:5 | LL | / match &mut &Some(String::new()) { LL | | Some(x) => Some(x.is_empty()), @@ -128,7 +128,7 @@ LL | | }; | |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:100:5 + --> $DIR/manual_map_option.rs:101:5 | LL | / match Some((0, 1, 2)) { LL | | Some((x, y, z)) => Some(x + y + z), @@ -137,7 +137,7 @@ LL | | }; | |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:105:5 + --> $DIR/manual_map_option.rs:106:5 | LL | / match Some([1, 2, 3]) { LL | | Some([first, ..]) => Some(first), @@ -146,7 +146,7 @@ LL | | }; | |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:110:5 + --> $DIR/manual_map_option.rs:111:5 | LL | / match &Some((String::new(), "test")) { LL | | Some((x, y)) => Some((y, x)), @@ -155,7 +155,7 @@ LL | | }; | |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:168:5 + --> $DIR/manual_map_option.rs:169:5 | LL | / match Some(0) { LL | | Some(x) => Some(vec![x]), @@ -164,7 +164,7 @@ LL | | }; | |_____^ help: try this: `Some(0).map(|x| vec![x])` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:173:5 + --> $DIR/manual_map_option.rs:174:5 | LL | / match option_env!("") { LL | | Some(x) => Some(String::from(x)), @@ -173,7 +173,7 @@ LL | | }; | |_____^ help: try this: `option_env!("").map(String::from)` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:193:12 + --> $DIR/manual_map_option.rs:194:12 | LL | } else if let Some(x) = Some(0) { | ____________^ @@ -184,7 +184,7 @@ LL | | }; | |_____^ help: try this: `{ Some(0).map(|x| x + 1) }` error: manual implementation of `Option::map` - --> $DIR/manual_map_option.rs:201:12 + --> $DIR/manual_map_option.rs:202:12 | LL | } else if let Some(x) = Some(0) { | ____________^