Auto merge of #5597 - esamudera:slice_iter_next, r=flip1995

New lint: iter_next_slice

Hello, this is a work-in-progress PR for issue: #5572

I have implemented lint to replace `iter().next()` for `slice[index..]` and `array` with `get(index)` and `get(0)` respectively. However since I made a lot of changes, I would like to request some feedback before continuing so that I could fix mistakes.

Thank you!

---

changelog: implement `iter_next_slice` lint and test, and modify `needless_continues`, `for_loop_over_options_result` UI tests since they have `iter().next()`

Author: bors

CHANGELOG.md

@@ -1401,6 +1401,7 @@ Released 2018-09-13
 [`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements
 [`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect
 [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop
+[`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice
 [`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth
 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next

clippy_lints/src/lib.rs

@@ -666,6 +666,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::INTO_ITER_ON_REF,
         &methods::ITERATOR_STEP_BY_ZERO,
         &methods::ITER_CLONED_COLLECT,
+        &methods::ITER_NEXT_SLICE,
         &methods::ITER_NTH,
         &methods::ITER_NTH_ZERO,
         &methods::ITER_SKIP_NEXT,
@@ -1310,6 +1311,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::INTO_ITER_ON_REF),
         LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
         LintId::of(&methods::ITER_CLONED_COLLECT),
+        LintId::of(&methods::ITER_NEXT_SLICE),
         LintId::of(&methods::ITER_NTH),
         LintId::of(&methods::ITER_NTH_ZERO),
         LintId::of(&methods::ITER_SKIP_NEXT),
@@ -1491,6 +1493,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::CHARS_NEXT_CMP),
         LintId::of(&methods::INTO_ITER_ON_REF),
         LintId::of(&methods::ITER_CLONED_COLLECT),
+        LintId::of(&methods::ITER_NEXT_SLICE),
         LintId::of(&methods::ITER_NTH_ZERO),
         LintId::of(&methods::ITER_SKIP_NEXT),
         LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),

clippy_lints/src/loops.rs

@@ -27,7 +27,7 @@ use rustc_middle::middle::region;
 use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
-use rustc_span::BytePos;
+use rustc_span::symbol::Symbol;
 use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, Place, PlaceBase};
 use std::iter::{once, Iterator};
 use std::mem;
@@ -2381,32 +2381,32 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
                 match_type(cx, ty, &paths::BTREEMAP) ||
                 is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) {
                 if method.ident.name == sym!(len) {
-                    let span = shorten_needless_collect_span(expr);
+                    let span = shorten_span(expr, sym!(collect));
                     span_lint_and_sugg(
                         cx,
                         NEEDLESS_COLLECT,
                         span,
                         NEEDLESS_COLLECT_MSG,
                         "replace with",
-                        ".count()".to_string(),
+                        "count()".to_string(),
                         Applicability::MachineApplicable,
                     );
                 }
                 if method.ident.name == sym!(is_empty) {
-                    let span = shorten_needless_collect_span(expr);
+                    let span = shorten_span(expr, sym!(iter));
                     span_lint_and_sugg(
                         cx,
                         NEEDLESS_COLLECT,
                         span,
                         NEEDLESS_COLLECT_MSG,
                         "replace with",
-                        ".next().is_none()".to_string(),
+                        "get(0).is_none()".to_string(),
                         Applicability::MachineApplicable,
                     );
                 }
                 if method.ident.name == sym!(contains) {
                     let contains_arg = snippet(cx, args[1].span, "??");
-                    let span = shorten_needless_collect_span(expr);
+                    let span = shorten_span(expr, sym!(collect));
                     span_lint_and_then(
                         cx,
                         NEEDLESS_COLLECT,
@@ -2422,7 +2422,7 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
                                 span,
                                 "replace with",
                                 format!(
-                                    ".any(|{}| x == {})",
+                                    "any(|{}| x == {})",
                                     arg, pred
                                 ),
                                 Applicability::MachineApplicable,
@@ -2435,13 +2435,13 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
     }
 }
 
-fn shorten_needless_collect_span(expr: &Expr<'_>) -> Span {
-    if_chain! {
-        if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
-        if let ExprKind::MethodCall(_, ref span, _) = args[0].kind;
-        then {
-            return expr.span.with_lo(span.lo() - BytePos(1));
+fn shorten_span(expr: &Expr<'_>, target_fn_name: Symbol) -> Span {
+    let mut current_expr = expr;
+    while let ExprKind::MethodCall(ref path, ref span, ref args) = current_expr.kind {
+        if path.ident.name == target_fn_name {
+            return expr.span.with_lo(span.lo());
         }
+        current_expr = &args[0];
     }
     unreachable!()
 }

clippy_lints/src/methods/mod.rs

@@ -26,7 +26,7 @@ use rustc_span::symbol::{sym, SymbolStr};
 use crate::consts::{constant, Constant};
 use crate::utils::usage::mutated_variables;
 use crate::utils::{
-    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy,
+    get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy,
     is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
     match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths,
     remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_applicability,
@@ -1242,6 +1242,32 @@ declare_clippy_lint! {
     "using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `iter().next()` on a Slice or an Array
+    ///
+    /// **Why is this bad?** These can be shortened into `.get()`
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # let a = [1, 2, 3];
+    /// # let b = vec![1, 2, 3];
+    /// a[2..].iter().next();
+    /// b.iter().next();
+    /// ```
+    /// should be written as:
+    /// ```rust
+    /// # let a = [1, 2, 3];
+    /// # let b = vec![1, 2, 3];
+    /// a.get(2);
+    /// b.get(0);
+    /// ```
+    pub ITER_NEXT_SLICE,
+    style,
+    "using `.iter().next()` on a sliced array, which can be shortened to just `.get()`"
+}
+
 declare_lint_pass!(Methods => [
     UNWRAP_USED,
     EXPECT_USED,
@@ -1273,6 +1299,7 @@ declare_lint_pass!(Methods => [
     FIND_MAP,
     MAP_FLATTEN,
     ITERATOR_STEP_BY_ZERO,
+    ITER_NEXT_SLICE,
     ITER_NTH,
     ITER_NTH_ZERO,
     ITER_SKIP_NEXT,
@@ -1320,6 +1347,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             },
             ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
             ["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
+            ["next", "iter"] => lint_iter_next(cx, expr, arg_lists[1]),
             ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]),
@@ -2199,6 +2227,60 @@ fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, args
     }
 }
 
+fn lint_iter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>, iter_args: &'tcx [hir::Expr<'_>]) {
+    let caller_expr = &iter_args[0];
+
+    // Skip lint if the `iter().next()` expression is a for loop argument,
+    // since it is already covered by `&loops::ITER_NEXT_LOOP`
+    let mut parent_expr_opt = get_parent_expr(cx, expr);
+    while let Some(parent_expr) = parent_expr_opt {
+        if higher::for_loop(parent_expr).is_some() {
+            return;
+        }
+        parent_expr_opt = get_parent_expr(cx, parent_expr);
+    }
+
+    if derefs_to_slice(cx, caller_expr, cx.tables.expr_ty(caller_expr)).is_some() {
+        // caller is a Slice
+        if_chain! {
+            if let hir::ExprKind::Index(ref caller_var, ref index_expr) = &caller_expr.kind;
+            if let Some(higher::Range { start: Some(start_expr), end: None, limits: ast::RangeLimits::HalfOpen })
+                = higher::range(cx, index_expr);
+            if let hir::ExprKind::Lit(ref start_lit) = &start_expr.kind;
+            if let ast::LitKind::Int(start_idx, _) = start_lit.node;
+            then {
+                let mut applicability = Applicability::MachineApplicable;
+                span_lint_and_sugg(
+                    cx,
+                    ITER_NEXT_SLICE,
+                    expr.span,
+                    "Using `.iter().next()` on a Slice without end index.",
+                    "try calling",
+                    format!("{}.get({})", snippet_with_applicability(cx, caller_var.span, "..", &mut applicability), start_idx),
+                    applicability,
+                );
+            }
+        }
+    } else if is_type_diagnostic_item(cx, cx.tables.expr_ty(caller_expr), sym!(vec_type))
+        || matches!(&walk_ptrs_ty(cx.tables.expr_ty(caller_expr)).kind, ty::Array(_, _))
+    {
+        // caller is a Vec or an Array
+        let mut applicability = Applicability::MachineApplicable;
+        span_lint_and_sugg(
+            cx,
+            ITER_NEXT_SLICE,
+            expr.span,
+            "Using `.iter().next()` on an array",
+            "try calling",
+            format!(
+                "{}.get(0)",
+                snippet_with_applicability(cx, caller_expr.span, "..", &mut applicability)
+            ),
+            applicability,
+        );
+    }
+}
+
 fn lint_iter_nth<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     expr: &hir::Expr<'_>,

clippy_lints/src/needless_continue.rs

@@ -424,7 +424,7 @@ fn erode_from_back(s: &str) -> String {
 }
 
 fn span_of_first_expr_in_block(block: &ast::Block) -> Option<Span> {
-    block.stmts.iter().next().map(|stmt| stmt.span)
+    block.stmts.get(0).map(|stmt| stmt.span)
 }
 
 #[cfg(test)]

src/lintlist/mod.rs

@@ -934,6 +934,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "loops",
     },
+    Lint {
+        name: "iter_next_slice",
+        group: "style",
+        desc: "using `.iter().next()` on a sliced array, which can be shortened to just `.get()`",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "iter_nth",
         group: "perf",

tests/ui/into_iter_on_ref.fixed

@@ -40,4 +40,6 @@ fn main() {
     let _ = (&HashSet::<i32>::new()).iter(); //~ WARN equivalent to .iter()
     let _ = std::path::Path::new("12/34").iter(); //~ WARN equivalent to .iter()
     let _ = std::path::PathBuf::from("12/34").iter(); //~ ERROR equivalent to .iter()
+
+    let _ = (&[1, 2, 3]).iter().next(); //~ WARN equivalent to .iter()
 }

tests/ui/into_iter_on_ref.rs

@@ -40,4 +40,6 @@ fn main() {
     let _ = (&HashSet::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
     let _ = std::path::Path::new("12/34").into_iter(); //~ WARN equivalent to .iter()
     let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter()
+
+    let _ = (&[1, 2, 3]).into_iter().next(); //~ WARN equivalent to .iter()
 }

tests/ui/into_iter_on_ref.stderr

@@ -156,5 +156,11 @@ error: this `.into_iter()` call is equivalent to `.iter()` and will not move the
 LL |     let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter()
    |                                               ^^^^^^^^^ help: call directly: `iter`
 
-error: aborting due to 26 previous errors
+error: this `.into_iter()` call is equivalent to `.iter()` and will not move the `array`
+  --> $DIR/into_iter_on_ref.rs:44:26
+   |
+LL |     let _ = (&[1, 2, 3]).into_iter().next(); //~ WARN equivalent to .iter()
+   |                          ^^^^^^^^^ help: call directly: `iter`
+
+error: aborting due to 27 previous errors
 

tests/ui/iter_next_slice.fixed

@@ -0,0 +1,24 @@
+// run-rustfix
+#![warn(clippy::iter_next_slice)]
+
+fn main() {
+    // test code goes here
+    let s = [1, 2, 3];
+    let v = vec![1, 2, 3];
+
+    s.get(0);
+    // Should be replaced by s.get(0)
+
+    s.get(2);
+    // Should be replaced by s.get(2)
+
+    v.get(5);
+    // Should be replaced by v.get(5)
+
+    v.get(0);
+    // Should be replaced by v.get(0)
+
+    let o = Some(5);
+    o.iter().next();
+    // Shouldn't be linted since this is not a Slice or an Array
+}