Fixes #8128

changelog: Fix error suggestion of skip(..).next() for immutable variable.

6 files changed, 138 insertions, 10 deletions

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<T>(mut s: T)
+    where
+        T: Iterator<Item = String>,
+    {
+        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<T>(mut s: T)
+    where
+        T: Iterator<Item = String>,
+    {
+        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<T>(s: T)
+    where
+        T: Iterator<Item = String>,
+    {
+        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<T>(s: T)
+   |                 ^
+
+error: aborting due to 3 previous errors
+