Auto merge of #4913 - mikerite:step-by-zero-20191218, r=phansch

Move `iterator_step_by_zero` and correct the documentation

Move `iterator_step_by_zero` and correct the documentation.

changelog: Corrected `iterator_step_by_zero` documentation

Author: bors

clippy_lints/src/lib.rs

@@ -606,6 +606,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         &methods::GET_UNWRAP,
         &methods::INEFFICIENT_TO_STRING,
         &methods::INTO_ITER_ON_REF,
+        &methods::ITERATOR_STEP_BY_ZERO,
         &methods::ITER_CLONED_COLLECT,
         &methods::ITER_NTH,
         &methods::ITER_SKIP_NEXT,
@@ -699,7 +700,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         &ptr::PTR_ARG,
         &ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
         &question_mark::QUESTION_MARK,
-        &ranges::ITERATOR_STEP_BY_ZERO,
         &ranges::RANGE_MINUS_ONE,
         &ranges::RANGE_PLUS_ONE,
         &ranges::RANGE_ZIP_WITH_LEN,
@@ -1179,6 +1179,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&methods::FLAT_MAP_IDENTITY),
         LintId::of(&methods::INEFFICIENT_TO_STRING),
         LintId::of(&methods::INTO_ITER_ON_REF),
+        LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
         LintId::of(&methods::ITER_CLONED_COLLECT),
         LintId::of(&methods::ITER_NTH),
         LintId::of(&methods::ITER_SKIP_NEXT),
@@ -1244,7 +1245,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&ptr::PTR_ARG),
         LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST),
         LintId::of(&question_mark::QUESTION_MARK),
-        LintId::of(&ranges::ITERATOR_STEP_BY_ZERO),
         LintId::of(&ranges::RANGE_MINUS_ONE),
         LintId::of(&ranges::RANGE_PLUS_ONE),
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
@@ -1521,6 +1521,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM),
         LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT),
         LintId::of(&methods::CLONE_DOUBLE_REF),
+        LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
         LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
         LintId::of(&methods::UNINIT_ASSUMED_INIT),
         LintId::of(&methods::ZST_OFFSET),
@@ -1533,7 +1534,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
         LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
         LintId::of(&ptr::MUT_FROM_REF),
-        LintId::of(&ranges::ITERATOR_STEP_BY_ZERO),
         LintId::of(&regex::INVALID_REGEX),
         LintId::of(&serde_api::SERDE_API_MISUSE),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),

clippy_lints/src/methods/mod.rs

@@ -737,6 +737,25 @@ declare_clippy_lint! {
     "getting the inner pointer of a temporary `CString`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for calling `.step_by(0)` on iterators which panics.
+    ///
+    /// **Why is this bad?** This very much looks like an oversight. Use `panic!()` instead if you
+    /// actually intend to panic.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```should_panic
+    /// for x in (0..100).step_by(0) {
+    ///     //..
+    /// }
+    /// ```
+    pub ITERATOR_STEP_BY_ZERO,
+    correctness,
+    "using `Iterator::step_by(0)`, which will panic at runtime"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for use of `.iter().nth()` (and the related
     /// `.iter_mut().nth()`) on standard library types with O(1) element access.
@@ -1115,6 +1134,7 @@ declare_lint_pass!(Methods => [
     FLAT_MAP_IDENTITY,
     FIND_MAP,
     MAP_FLATTEN,
+    ITERATOR_STEP_BY_ZERO,
     ITER_NTH,
     ITER_SKIP_NEXT,
     GET_UNWRAP,
@@ -1173,6 +1193,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             },
             ["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false),
             ["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true),
+            ["step_by", ..] => lint_step_by(cx, expr, arg_lists[0]),
             ["next", "skip"] => lint_iter_skip_next(cx, expr),
             ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]),
             ["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]),
@@ -1950,6 +1971,20 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args:
     }
 }
 
+fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, args: &'tcx [hir::Expr]) {
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        use crate::consts::{constant, Constant};
+        if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) {
+            span_lint(
+                cx,
+                ITERATOR_STEP_BY_ZERO,
+                expr.span,
+                "Iterator::step_by(0) will panic at runtime",
+            );
+        }
+    }
+}
+
 fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) {
     let mut_str = if is_mut { "_mut" } else { "" };
     let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() {

clippy_lints/src/ranges.rs

@@ -8,28 +8,8 @@ use syntax::ast::RangeLimits;
 use syntax::source_map::Spanned;
 
 use crate::utils::sugg::Sugg;
-use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq};
-use crate::utils::{is_integer_const, paths, snippet, snippet_opt, span_lint, span_lint_and_then};
-
-declare_clippy_lint! {
-    /// **What it does:** Checks for calling `.step_by(0)` on iterators,
-    /// which never terminates.
-    ///
-    /// **Why is this bad?** This very much looks like an oversight, since with
-    /// `loop { .. }` there is an obvious better way to endlessly loop.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    /// ```ignore
-    /// for x in (5..5).step_by(0) {
-    ///     ..
-    /// }
-    /// ```
-    pub ITERATOR_STEP_BY_ZERO,
-    correctness,
-    "using `Iterator::step_by(0)`, which produces an infinite iterator"
-}
+use crate::utils::{higher, SpanlessEq};
+use crate::utils::{is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for zipping a collection with the range of
@@ -102,7 +82,6 @@ declare_clippy_lint! {
 }
 
 declare_lint_pass!(Ranges => [
-    ITERATOR_STEP_BY_ZERO,
     RANGE_ZIP_WITH_LEN,
     RANGE_PLUS_ONE,
     RANGE_MINUS_ONE
@@ -112,19 +91,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
         if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind {
             let name = path.ident.as_str();
-
-            // Range with step_by(0).
-            if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) {
-                use crate::consts::{constant, Constant};
-                if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) {
-                    span_lint(
-                        cx,
-                        ITERATOR_STEP_BY_ZERO,
-                        expr.span,
-                        "Iterator::step_by(0) will panic at runtime",
-                    );
-                }
-            } else if name == "zip" && args.len() == 2 {
+            if name == "zip" && args.len() == 2 {
                 let iter = &args[0].kind;
                 let zip_arg = &args[1];
                 if_chain! {
@@ -232,14 +199,6 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
     }
 }
 
-fn has_step_by(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
-    // No need for `walk_ptrs_ty` here because `step_by` moves `self`, so it
-    // can't be called on a borrowed range.
-    let ty = cx.tables.expr_ty_adjusted(expr);
-
-    get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[]))
-}
-
 fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> {
     match expr.kind {
         ExprKind::Binary(

src/lintlist/mod.rs

@@ -885,9 +885,9 @@ pub const ALL_LINTS: [Lint; 340] = [
     Lint {
         name: "iterator_step_by_zero",
         group: "correctness",
-        desc: "using `Iterator::step_by(0)`, which produces an infinite iterator",
+        desc: "using `Iterator::step_by(0)`, which will panic at runtime",
         deprecation: None,
-        module: "ranges",
+        module: "methods",
     },
     Lint {
         name: "just_underscores_and_digits",

tests/ui/iterator_step_by_zero.rs

@@ -0,0 +1,28 @@
+#[warn(clippy::iterator_step_by_zero)]
+fn main() {
+    let _ = vec!["A", "B", "B"].iter().step_by(0);
+    let _ = "XXX".chars().step_by(0);
+    let _ = (0..1).step_by(0);
+
+    // No error, not an iterator.
+    let y = NotIterator;
+    y.step_by(0);
+
+    // No warning for non-zero step
+    let _ = (0..1).step_by(1);
+
+    let _ = (1..).step_by(0);
+    let _ = (1..=2).step_by(0);
+
+    let x = 0..1;
+    let _ = x.step_by(0);
+
+    // check const eval
+    let v1 = vec![1, 2, 3];
+    let _ = v1.iter().step_by(2 / 3);
+}
+
+struct NotIterator;
+impl NotIterator {
+    fn step_by(&self, _: u32) {}
+}

tests/ui/iterator_step_by_zero.stderr

@@ -0,0 +1,46 @@
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:3:13
+   |
+LL |     let _ = vec!["A", "B", "B"].iter().step_by(0);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::iterator-step-by-zero` implied by `-D warnings`
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:4:13
+   |
+LL |     let _ = "XXX".chars().step_by(0);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:5:13
+   |
+LL |     let _ = (0..1).step_by(0);
+   |             ^^^^^^^^^^^^^^^^^
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:14:13
+   |
+LL |     let _ = (1..).step_by(0);
+   |             ^^^^^^^^^^^^^^^^
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:15:13
+   |
+LL |     let _ = (1..=2).step_by(0);
+   |             ^^^^^^^^^^^^^^^^^^
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:18:13
+   |
+LL |     let _ = x.step_by(0);
+   |             ^^^^^^^^^^^^
+
+error: Iterator::step_by(0) will panic at runtime
+  --> $DIR/iterator_step_by_zero.rs:22:13
+   |
+LL |     let _ = v1.iter().step_by(2 / 3);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 7 previous errors
+

tests/ui/range.rs

@@ -1,31 +1,9 @@
-struct NotARange;
-impl NotARange {
-    fn step_by(&self, _: u32) {}
-}
-
-#[warn(clippy::iterator_step_by_zero, clippy::range_zip_with_len)]
+#[warn(clippy::range_zip_with_len)]
 fn main() {
-    let _ = (0..1).step_by(0);
-    // No warning for non-zero step
-    let _ = (0..1).step_by(1);
-
-    let _ = (1..).step_by(0);
-    let _ = (1..=2).step_by(0);
-
-    let x = 0..1;
-    let _ = x.step_by(0);
-
-    // No error, not a range.
-    let y = NotARange;
-    y.step_by(0);
-
     let v1 = vec![1, 2, 3];
     let v2 = vec![4, 5];
     let _x = v1.iter().zip(0..v1.len());
     let _y = v1.iter().zip(0..v2.len()); // No error
-
-    // check const eval
-    let _ = v1.iter().step_by(2 / 3);
 }
 
 #[allow(unused)]

tests/ui/range.stderr

@@ -1,42 +1,10 @@
-error: Iterator::step_by(0) will panic at runtime
-  --> $DIR/range.rs:8:13
-   |
-LL |     let _ = (0..1).step_by(0);
-   |             ^^^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::iterator-step-by-zero` implied by `-D warnings`
-
-error: Iterator::step_by(0) will panic at runtime
-  --> $DIR/range.rs:12:13
-   |
-LL |     let _ = (1..).step_by(0);
-   |             ^^^^^^^^^^^^^^^^
-
-error: Iterator::step_by(0) will panic at runtime
-  --> $DIR/range.rs:13:13
-   |
-LL |     let _ = (1..=2).step_by(0);
-   |             ^^^^^^^^^^^^^^^^^^
-
-error: Iterator::step_by(0) will panic at runtime
-  --> $DIR/range.rs:16:13
-   |
-LL |     let _ = x.step_by(0);
-   |             ^^^^^^^^^^^^
-
 error: It is more idiomatic to use v1.iter().enumerate()
-  --> $DIR/range.rs:24:14
+  --> $DIR/range.rs:5:14
    |
 LL |     let _x = v1.iter().zip(0..v1.len());
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::range-zip-with-len` implied by `-D warnings`
 
-error: Iterator::step_by(0) will panic at runtime
-  --> $DIR/range.rs:28:13
-   |
-LL |     let _ = v1.iter().step_by(2 / 3);
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^
-
-error: aborting due to 6 previous errors
+error: aborting due to previous error