Auto merge of rust-lang#11046 - Centri3:iter_skip_zero, r=blyxyas

New lint [`iter_skip_zero`]

Idea came from my contribution to `unnecessary_cast` recently. Sorry about that 😅

Could be an issue if somebody manually implements `skip`, but I don't think anybody will (and this would be an issue with other lints too, afaik)

changelog: New lint [`iter_skip_zero`]

Author: bors

CHANGELOG.md

@@ -4939,6 +4939,7 @@ Released 2018-09-13
 [`iter_on_single_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_on_single_items
 [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
+[`iter_skip_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_zero
 [`iter_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain
 [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero
 [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits

clippy_lints/src/casts/unnecessary_cast.rs

@@ -257,7 +257,7 @@ fn is_cast_from_ty_alias<'tcx>(cx: &LateContext<'tcx>, expr: impl Visitable<'tcx
                 // Will this work for more complex types? Probably not!
                 if !snippet
                     .split("->")
-                    .skip(0)
+                    .skip(1)
                     .map(|s| {
                         s.trim() == cast_from.to_string()
                             || s.split("where").any(|ty| ty.trim() == cast_from.to_string())

clippy_lints/src/declared_lints.rs

@@ -360,6 +360,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::ITER_ON_SINGLE_ITEMS_INFO,
     crate::methods::ITER_OVEREAGER_CLONED_INFO,
     crate::methods::ITER_SKIP_NEXT_INFO,
+    crate::methods::ITER_SKIP_ZERO_INFO,
     crate::methods::ITER_WITH_DRAIN_INFO,
     crate::methods::MANUAL_FILTER_MAP_INFO,
     crate::methods::MANUAL_FIND_MAP_INFO,

clippy_lints/src/methods/iter_skip_zero.rs

@@ -0,0 +1,34 @@
+use clippy_utils::consts::{constant, Constant};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{is_from_proc_macro, is_trait_method};
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+use super::ITER_SKIP_ZERO;
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, arg_expr: &Expr<'_>) {
+    if !expr.span.from_expansion()
+        && is_trait_method(cx, expr, sym::Iterator)
+        && let Some(arg) = constant(cx, cx.typeck_results(), arg_expr).and_then(|constant| {
+            if let Constant::Int(arg) = constant {
+                Some(arg)
+            } else {
+                None
+            }
+        })
+        && arg == 0
+        && !is_from_proc_macro(cx, expr)
+    {
+        span_lint_and_then(cx, ITER_SKIP_ZERO, arg_expr.span, "usage of `.skip(0)`", |diag| {
+            diag.span_suggestion(
+                arg_expr.span,
+                "if you meant to skip the first element, use",
+                "1",
+                Applicability::MaybeIncorrect,
+            )
+            .note("this call to `skip` does nothing and is useless; remove it");
+        });
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -45,6 +45,7 @@ mod iter_nth_zero;
 mod iter_on_single_or_empty_collections;
 mod iter_overeager_cloned;
 mod iter_skip_next;
+mod iter_skip_zero;
 mod iter_with_drain;
 mod iterator_step_by_zero;
 mod manual_next_back;
@@ -3443,6 +3444,27 @@ declare_clippy_lint! {
     "`format!`ing every element in a collection, then collecting the strings into a new `String`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `.skip(0)` on iterators.
+    ///
+    /// ### Why is this bad?
+    /// This was likely intended to be `.skip(1)` to skip the first element, as `.skip(0)` does
+    /// nothing. If not, the call should be removed.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let v = vec![1, 2, 3];
+    /// let x = v.iter().skip(0).collect::<Vec<_>>();
+    /// let y = v.iter().collect::<Vec<_>>();
+    /// assert_eq!(x, y);
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub ITER_SKIP_ZERO,
+    correctness,
+    "disallows `.skip(0)`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3579,6 +3601,7 @@ impl_lint_pass!(Methods => [
     MANUAL_TRY_FOLD,
     FORMAT_COLLECT,
     STRING_LIT_CHARS_ANY,
+    ITER_SKIP_ZERO,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -3901,7 +3924,16 @@ impl Methods {
                         unnecessary_join::check(cx, expr, recv, join_arg, span);
                     }
                 },
-                ("last", []) | ("skip", [_]) => {
+                ("skip", [arg]) => {
+                    iter_skip_zero::check(cx, expr, arg);
+
+                    if let Some((name2, recv2, args2, _span2, _)) = method_call(recv) {
+                        if let ("cloned", []) = (name2, args2) {
+                            iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);
+                        }
+                    }
+                }
+                ("last", []) => {
                     if let Some((name2, recv2, args2, _span2, _)) = method_call(recv) {
                         if let ("cloned", []) = (name2, args2) {
                             iter_overeager_cloned::check(cx, expr, recv, recv2, false, false);

tests/ui/iter_skip_zero.fixed

@@ -0,0 +1,25 @@
+//@run-rustfix
+//@aux-build:proc_macros.rs:proc-macro
+#![allow(clippy::useless_vec, unused)]
+#![warn(clippy::iter_skip_zero)]
+
+#[macro_use]
+extern crate proc_macros;
+
+use std::iter::once;
+
+fn main() {
+    let _ = [1, 2, 3].iter().skip(1);
+    let _ = vec![1, 2, 3].iter().skip(1);
+    let _ = once([1, 2, 3]).skip(1);
+    let _ = vec![1, 2, 3].iter().chain([1, 2, 3].iter().skip(1)).skip(1);
+    // Don't lint
+    let _ = [1, 2, 3].iter().skip(1);
+    let _ = vec![1, 2, 3].iter().skip(1);
+    external! {
+        let _ = [1, 2, 3].iter().skip(0);
+    }
+    with_span! {
+        let _ = [1, 2, 3].iter().skip(0);
+    }
+}

tests/ui/iter_skip_zero.rs

@@ -0,0 +1,25 @@
+//@run-rustfix
+//@aux-build:proc_macros.rs:proc-macro
+#![allow(clippy::useless_vec, unused)]
+#![warn(clippy::iter_skip_zero)]
+
+#[macro_use]
+extern crate proc_macros;
+
+use std::iter::once;
+
+fn main() {
+    let _ = [1, 2, 3].iter().skip(0);
+    let _ = vec![1, 2, 3].iter().skip(0);
+    let _ = once([1, 2, 3]).skip(0);
+    let _ = vec![1, 2, 3].iter().chain([1, 2, 3].iter().skip(0)).skip(0);
+    // Don't lint
+    let _ = [1, 2, 3].iter().skip(1);
+    let _ = vec![1, 2, 3].iter().skip(1);
+    external! {
+        let _ = [1, 2, 3].iter().skip(0);
+    }
+    with_span! {
+        let _ = [1, 2, 3].iter().skip(0);
+    }
+}

tests/ui/iter_skip_zero.stderr

@@ -0,0 +1,43 @@
+error: usage of `.skip(0)`
+  --> $DIR/iter_skip_zero.rs:12:35
+   |
+LL |     let _ = [1, 2, 3].iter().skip(0);
+   |                                   ^ help: if you meant to skip the first element, use: `1`
+   |
+   = note: this call to `skip` does nothing and is useless; remove it
+   = note: `-D clippy::iter-skip-zero` implied by `-D warnings`
+
+error: usage of `.skip(0)`
+  --> $DIR/iter_skip_zero.rs:13:39
+   |
+LL |     let _ = vec![1, 2, 3].iter().skip(0);
+   |                                       ^ help: if you meant to skip the first element, use: `1`
+   |
+   = note: this call to `skip` does nothing and is useless; remove it
+
+error: usage of `.skip(0)`
+  --> $DIR/iter_skip_zero.rs:14:34
+   |
+LL |     let _ = once([1, 2, 3]).skip(0);
+   |                                  ^ help: if you meant to skip the first element, use: `1`
+   |
+   = note: this call to `skip` does nothing and is useless; remove it
+
+error: usage of `.skip(0)`
+  --> $DIR/iter_skip_zero.rs:15:71
+   |
+LL |     let _ = vec![1, 2, 3].iter().chain([1, 2, 3].iter().skip(0)).skip(0);
+   |                                                                       ^ help: if you meant to skip the first element, use: `1`
+   |
+   = note: this call to `skip` does nothing and is useless; remove it
+
+error: usage of `.skip(0)`
+  --> $DIR/iter_skip_zero.rs:15:62
+   |
+LL |     let _ = vec![1, 2, 3].iter().chain([1, 2, 3].iter().skip(0)).skip(0);
+   |                                                              ^ help: if you meant to skip the first element, use: `1`
+   |
+   = note: this call to `skip` does nothing and is useless; remove it
+
+error: aborting due to 5 previous errors
+