From 4ffd66074a462b30d15da8d40cc0f356dd43bb1c Mon Sep 17 00:00:00 2001 From: surechen Date: Fri, 17 Dec 2021 10:03:59 +0800 Subject: [PATCH] Fixes #8128 changelog: Fix error suggestion of skip(..).next() for immutable variable. --- clippy_lints/src/methods/iter_skip_next.rs | 32 +++++++++++++++--- tests/ui/iter_skip_next.fixed | 15 +++++++++ tests/ui/iter_skip_next.rs | 15 +++++++++ tests/ui/iter_skip_next.stderr | 28 +++++++++++++--- tests/ui/iter_skip_next_unfixable.rs | 19 +++++++++++ tests/ui/iter_skip_next_unfixable.stderr | 39 ++++++++++++++++++++++ 6 files changed, 138 insertions(+), 10 deletions(-) create mode 100644 tests/ui/iter_skip_next_unfixable.rs create mode 100644 tests/ui/iter_skip_next_unfixable.stderr diff --git a/clippy_lints/src/methods/iter_skip_next.rs b/clippy_lints/src/methods/iter_skip_next.rs index e32594757d0..f5410c7fd7f 100644 --- a/clippy_lints/src/methods/iter_skip_next.rs +++ b/clippy_lints/src/methods/iter_skip_next.rs @@ -1,8 +1,10 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::is_trait_method; +use clippy_utils::path_to_local; use clippy_utils::source::snippet; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::{BindingAnnotation, Node, PatKind}; use rustc_lint::LateContext; use rustc_span::sym; @@ -11,14 +13,34 @@ use super::ITER_SKIP_NEXT; pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) { // lint if caller of skip is an Iterator if is_trait_method(cx, expr, sym::Iterator) { - span_lint_and_sugg( + let mut application = Applicability::MachineApplicable; + span_lint_and_then( cx, ITER_SKIP_NEXT, expr.span.trim_start(recv.span).unwrap(), "called `skip(..).next()` on an iterator", - "use `nth` instead", - format!(".nth({})", snippet(cx, arg.span, "..")), - Applicability::MachineApplicable, + |diag| { + if_chain! { + if let Some(id) = path_to_local(recv); + if let Node::Binding(pat) = cx.tcx.hir().get(id); + if let PatKind::Binding(ann, _, _, _) = pat.kind; + if ann != BindingAnnotation::Mutable; + then { + application = Applicability::Unspecified; + diag.span_help( + pat.span, + &format!("for this change `{}` has to be mutable", snippet(cx, pat.span, "..")), + ); + } + } + + diag.span_suggestion( + expr.span.trim_start(recv.span).unwrap(), + "use `nth` instead", + format!(".nth({})", snippet(cx, arg.span, "..")), + application, + ); + }, ); } } diff --git a/tests/ui/iter_skip_next.fixed b/tests/ui/iter_skip_next.fixed index 928b6acb951..2db4c2bee7f 100644 --- a/tests/ui/iter_skip_next.fixed +++ b/tests/ui/iter_skip_next.fixed @@ -4,6 +4,7 @@ #![warn(clippy::iter_skip_next)] #![allow(clippy::blacklisted_name)] #![allow(clippy::iter_nth)] +#![allow(unused_mut, dead_code)] extern crate option_helpers; @@ -19,4 +20,18 @@ fn main() { let foo = IteratorFalsePositives { foo: 0 }; let _ = foo.skip(42).next(); let _ = foo.filter().skip(42).next(); + + // fix #8128 + let test_string = "1|1 2"; + let mut sp = test_string.split('|').map(|s| s.trim()); + let _: Vec<&str> = sp.nth(1).unwrap().split(' ').collect(); + if let Some(mut s) = Some(test_string.split('|').map(|s| s.trim())) { + let _: Vec<&str> = s.nth(1).unwrap().split(' ').collect(); + }; + fn check(mut s: T) + where + T: Iterator, + { + let _: Vec<&str> = s.nth(1).unwrap().split(' ').collect(); + } } diff --git a/tests/ui/iter_skip_next.rs b/tests/ui/iter_skip_next.rs index 7075e2598eb..692edb9aed9 100644 --- a/tests/ui/iter_skip_next.rs +++ b/tests/ui/iter_skip_next.rs @@ -4,6 +4,7 @@ #![warn(clippy::iter_skip_next)] #![allow(clippy::blacklisted_name)] #![allow(clippy::iter_nth)] +#![allow(unused_mut, dead_code)] extern crate option_helpers; @@ -19,4 +20,18 @@ fn main() { let foo = IteratorFalsePositives { foo: 0 }; let _ = foo.skip(42).next(); let _ = foo.filter().skip(42).next(); + + // fix #8128 + let test_string = "1|1 2"; + let mut sp = test_string.split('|').map(|s| s.trim()); + let _: Vec<&str> = sp.skip(1).next().unwrap().split(' ').collect(); + if let Some(mut s) = Some(test_string.split('|').map(|s| s.trim())) { + let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + }; + fn check(mut s: T) + where + T: Iterator, + { + let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + } } diff --git a/tests/ui/iter_skip_next.stderr b/tests/ui/iter_skip_next.stderr index 486de718bb5..ca6970b27f1 100644 --- a/tests/ui/iter_skip_next.stderr +++ b/tests/ui/iter_skip_next.stderr @@ -1,5 +1,5 @@ error: called `skip(..).next()` on an iterator - --> $DIR/iter_skip_next.rs:15:28 + --> $DIR/iter_skip_next.rs:16:28 | LL | let _ = some_vec.iter().skip(42).next(); | ^^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(42)` @@ -7,22 +7,40 @@ LL | let _ = some_vec.iter().skip(42).next(); = note: `-D clippy::iter-skip-next` implied by `-D warnings` error: called `skip(..).next()` on an iterator - --> $DIR/iter_skip_next.rs:16:36 + --> $DIR/iter_skip_next.rs:17:36 | LL | let _ = some_vec.iter().cycle().skip(42).next(); | ^^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(42)` error: called `skip(..).next()` on an iterator - --> $DIR/iter_skip_next.rs:17:20 + --> $DIR/iter_skip_next.rs:18:20 | LL | let _ = (1..10).skip(10).next(); | ^^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(10)` error: called `skip(..).next()` on an iterator - --> $DIR/iter_skip_next.rs:18:33 + --> $DIR/iter_skip_next.rs:19:33 | LL | let _ = &some_vec[..].iter().skip(3).next(); | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(3)` -error: aborting due to 4 previous errors +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next.rs:27:26 + | +LL | let _: Vec<&str> = sp.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next.rs:29:29 + | +LL | let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next.rs:35:29 + | +LL | let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + +error: aborting due to 7 previous errors diff --git a/tests/ui/iter_skip_next_unfixable.rs b/tests/ui/iter_skip_next_unfixable.rs new file mode 100644 index 00000000000..3607330cfa0 --- /dev/null +++ b/tests/ui/iter_skip_next_unfixable.rs @@ -0,0 +1,19 @@ +#![warn(clippy::iter_skip_next)] +#![allow(dead_code)] + +/// Checks implementation of `ITER_SKIP_NEXT` lint +fn main() { + // fix #8128 + let test_string = "1|1 2"; + let sp = test_string.split('|').map(|s| s.trim()); + let _: Vec<&str> = sp.skip(1).next().unwrap().split(' ').collect(); + if let Some(s) = Some(test_string.split('|').map(|s| s.trim())) { + let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + }; + fn check(s: T) + where + T: Iterator, + { + let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + } +} diff --git a/tests/ui/iter_skip_next_unfixable.stderr b/tests/ui/iter_skip_next_unfixable.stderr new file mode 100644 index 00000000000..74c327c7483 --- /dev/null +++ b/tests/ui/iter_skip_next_unfixable.stderr @@ -0,0 +1,39 @@ +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next_unfixable.rs:9:26 + | +LL | let _: Vec<&str> = sp.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + | + = note: `-D clippy::iter-skip-next` implied by `-D warnings` +help: for this change `sp` has to be mutable + --> $DIR/iter_skip_next_unfixable.rs:8:9 + | +LL | let sp = test_string.split('|').map(|s| s.trim()); + | ^^ + +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next_unfixable.rs:11:29 + | +LL | let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + | +help: for this change `s` has to be mutable + --> $DIR/iter_skip_next_unfixable.rs:10:17 + | +LL | if let Some(s) = Some(test_string.split('|').map(|s| s.trim())) { + | ^ + +error: called `skip(..).next()` on an iterator + --> $DIR/iter_skip_next_unfixable.rs:17:29 + | +LL | let _: Vec<&str> = s.skip(1).next().unwrap().split(' ').collect(); + | ^^^^^^^^^^^^^^^ help: use `nth` instead: `.nth(1)` + | +help: for this change `s` has to be mutable + --> $DIR/iter_skip_next_unfixable.rs:13:17 + | +LL | fn check(s: T) + | ^ + +error: aborting due to 3 previous errors +