Merge pull request #1837 from Manishearth/step_by

Replace `Range::step_by` checking with `Iterator::step_by`

Author: mcarton

CHANGELOG.md

@@ -3,6 +3,8 @@ All notable changes to this project will be documented in this file.
 
 ## 0.0.141
 * Rewrite of the `doc_markdown` lint.
+* Deprecated [`range_step_by_zero`]
+* New lint: [`iterator_step_by_zero`]
 
 ## 0.0.140 - 2017-06-16
 * Update to *rustc 1.19.0-nightly (258ae6dd9 2017-06-15)*
@@ -439,6 +441,7 @@ All notable changes to this project will be documented in this file.
 [`iter_next_loop`]: https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop
 [`iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#iter_nth
 [`iter_skip_next`]: https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next
+[`iterator_step_by_zero`]: https://github.com/Manishearth/rust-clippy/wiki#iterator_step_by_zero
 [`large_enum_variant`]: https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant
 [`len_without_is_empty`]: https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty
 [`len_zero`]: https://github.com/Manishearth/rust-clippy/wiki#len_zero

README.md

@@ -260,6 +260,7 @@ name
 [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop)                                       | warn    | for-looping over `_.next()` which is probably not intended
 [iter_nth](https://github.com/Manishearth/rust-clippy/wiki#iter_nth)                                                   | warn    | using `.iter().nth()` on a standard library type with O(1) element access
 [iter_skip_next](https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next)                                       | warn    | using `.skip(x).next()` on an iterator
+[iterator_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#iterator_step_by_zero)                         | warn    | using `Iterator::step_by(0)`, which produces an infinite iterator
 [large_enum_variant](https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant)                               | warn    | large size difference between variants on an enum
 [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty)                           | warn    | traits or impls with a public `len` method but no corresponding `is_empty` method
 [len_zero](https://github.com/Manishearth/rust-clippy/wiki#len_zero)                                                   | warn    | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead
@@ -321,7 +322,6 @@ name
 [print_with_newline](https://github.com/Manishearth/rust-clippy/wiki#print_with_newline)                               | warn    | using `print!()` with a format string that ends in a newline
 [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg)                                                     | warn    | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively
 [pub_enum_variant_names](https://github.com/Manishearth/rust-clippy/wiki#pub_enum_variant_names)                       | allow   | enums where all variants share a prefix/postfix
-[range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero)                               | warn    | using `Range::step_by(0)`, which produces an infinite iterator
 [range_zip_with_len](https://github.com/Manishearth/rust-clippy/wiki#range_zip_with_len)                               | warn    | zipping iterator with a range when `enumerate()` would do
 [redundant_closure](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure)                                 | warn    | redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)
 [redundant_closure_call](https://github.com/Manishearth/rust-clippy/wiki#redundant_closure_call)                       | warn    | throwaway closures called in the expression they are defined

clippy_lints/src/deprecated_lints.rs

@@ -14,6 +14,16 @@ declare_deprecated_lint! {
     "`.extend_from_slice(_)` is a faster way to extend a Vec by a slice"
 }
 
+/// **What it does:** Nothing. This lint has been deprecated.
+///
+/// **Deprecation reason:** `Range::step_by(0)` used to be linted since it's
+/// an infinite iterator, which is better expressed by `iter::repeat`,
+/// but the method has been removed for `Iterator::step_by` which panics
+/// if given a zero
+declare_deprecated_lint! {
+    pub RANGE_STEP_BY_ZERO,
+    "`iterator.step_by(0)` panics nowadays"
+}
 
 /// **What it does:** Nothing. This lint has been deprecated.
 ///

clippy_lints/src/lib.rs

@@ -194,6 +194,10 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         "extend_from_slice",
         "`.extend_from_slice(_)` is a faster way to extend a Vec by a slice",
     );
+    store.register_removed(
+        "range_step_by_zero",
+        "`iterator.step_by(0)` panics nowadays",
+    );
     store.register_removed(
         "unstable_as_slice",
         "`Vec::as_slice` has been stabilized in 1.7",
@@ -490,7 +494,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         ptr::CMP_NULL,
         ptr::MUT_FROM_REF,
         ptr::PTR_ARG,
-        ranges::RANGE_STEP_BY_ZERO,
+        ranges::ITERATOR_STEP_BY_ZERO,
         ranges::RANGE_ZIP_WITH_LEN,
         reference::DEREF_ADDROF,
         regex::INVALID_REGEX,

clippy_lints/src/ranges.rs

@@ -1,10 +1,10 @@
 use rustc::lint::*;
 use rustc::hir::*;
 use syntax::codemap::Spanned;
-use utils::{is_integer_literal, match_type, paths, snippet, span_lint};
-use utils::higher;
+use utils::{is_integer_literal, paths, snippet, span_lint};
+use utils::{higher, implements_trait, get_trait_def_id};
 
-/// **What it does:** Checks for iterating over ranges with a `.step_by(0)`,
+/// **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
@@ -17,10 +17,11 @@ use utils::higher;
 /// for x in (5..5).step_by(0) { .. }
 /// ```
 declare_lint! {
-    pub RANGE_STEP_BY_ZERO,
+    pub ITERATOR_STEP_BY_ZERO,
     Warn,
-    "using `Range::step_by(0)`, which produces an infinite iterator"
+    "using `Iterator::step_by(0)`, which produces an infinite iterator"
 }
+
 /// **What it does:** Checks for zipping a collection with the range of `0.._.len()`.
 ///
 /// **Why is this bad?** The code is better expressed with `.enumerate()`.
@@ -42,7 +43,7 @@ pub struct StepByZero;
 
 impl LintPass for StepByZero {
     fn get_lints(&self) -> LintArray {
-        lint_array!(RANGE_STEP_BY_ZERO, RANGE_ZIP_WITH_LEN)
+        lint_array!(ITERATOR_STEP_BY_ZERO, RANGE_ZIP_WITH_LEN)
     }
 }
 
@@ -52,12 +53,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StepByZero {
             let name = name.as_str();
 
             // Range with step_by(0).
-            if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) && is_integer_literal(&args[1], 0) {
-                span_lint(cx,
-                          RANGE_STEP_BY_ZERO,
-                          expr.span,
-                          "Range::step_by(0) produces an infinite iterator. Consider using `std::iter::repeat()` \
-                           instead");
+            if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) {
+                use consts::{Constant, constant};
+                use rustc_const_math::ConstInt::Usize;
+                if let Some((Constant::Int(Usize(us)), _)) = constant(cx, &args[1]) {
+                    if us.as_u64(cx.sess().target.uint_type) == 0 {
+                        span_lint(
+                            cx,
+                            ITERATOR_STEP_BY_ZERO,
+                            expr.span,
+                            "Iterator::step_by(0) will panic at runtime",
+                        );
+                    }
+                }
             } else if name == "zip" && args.len() == 2 {
                 let iter = &args[0].node;
                 let zip_arg = &args[1];
@@ -90,9 +98,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StepByZero {
 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(expr);
+    let ty = cx.tables.expr_ty_adjusted(expr);
 
-    // Note: `RangeTo`, `RangeToInclusive` and `RangeFull` don't have step_by
-    match_type(cx, ty, &paths::RANGE) || match_type(cx, ty, &paths::RANGE_FROM) ||
-    match_type(cx, ty, &paths::RANGE_INCLUSIVE)
+    get_trait_def_id(cx, &paths::ITERATOR)
+        .map_or(
+            false,
+            |iterator_trait| implements_trait(cx, ty, iterator_trait, &[])
+        )
 }

clippy_tests/examples/range.rs

@@ -1,3 +1,4 @@
+#![feature(iterator_step_by)]
 #![feature(step_by)]
 #![feature(inclusive_range_syntax)]
 #![feature(plugin)]
@@ -8,7 +9,7 @@ impl NotARange {
     fn step_by(&self, _: u32) {}
 }
 
-#[warn(range_step_by_zero, range_zip_with_len)]
+#[warn(iterator_step_by_zero, range_zip_with_len)]
 fn main() {
     (0..1).step_by(0);
     // No warning for non-zero step
@@ -28,4 +29,7 @@ fn main() {
     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);
 }

clippy_tests/examples/range.stderr

@@ -1,51 +1,59 @@
 error: use of deprecated item: replaced by `Iterator::step_by`
-  --> range.rs:13:12
+  --> range.rs:14:12
    |
-13 |     (0..1).step_by(0);
+14 |     (0..1).step_by(0);
    |            ^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
 
 error: use of deprecated item: replaced by `Iterator::step_by`
-  --> range.rs:15:12
+  --> range.rs:16:12
    |
-15 |     (0..1).step_by(1);
+16 |     (0..1).step_by(1);
    |            ^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
 
 error: use of deprecated item: replaced by `Iterator::step_by`
-  --> range.rs:17:11
+  --> range.rs:18:11
    |
-17 |     (1..).step_by(0);
+18 |     (1..).step_by(0);
    |           ^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
 
 error: use of deprecated item: replaced by `Iterator::step_by`
-  --> range.rs:18:13
+  --> range.rs:19:13
    |
-18 |     (1...2).step_by(0);
+19 |     (1...2).step_by(0);
    |             ^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
 
 error: use of deprecated item: replaced by `Iterator::step_by`
-  --> range.rs:21:7
+  --> range.rs:22:7
    |
-21 |     x.step_by(0);
+22 |     x.step_by(0);
    |       ^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
 
 error: It is more idiomatic to use v1.iter().enumerate()
-  --> range.rs:29:14
+  --> range.rs:30:14
    |
-29 |     let _x = v1.iter().zip(0..v1.len());
+30 |     let _x = v1.iter().zip(0..v1.len());
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D range-zip-with-len` implied by `-D warnings`
 
+error: Iterator::step_by(0) will panic at runtime
+  --> range.rs:34:13
+   |
+34 |     let _ = v1.iter().step_by(2/3);
+   |             ^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D iterator-step-by-zero` implied by `-D warnings`
+
 error: aborting due to previous error(s)
 
 error: Could not compile `clippy_tests`.