From 23a38c4170887401b3c75f9ad4443ab1f6beae11 Mon Sep 17 00:00:00 2001
From: Nathan Weston <nweston@fastmail.com>
Date: Sat, 15 Aug 2015 12:55:25 -0400
Subject: [PATCH] New lint: Range::step_by(0) (fixes #95)

Uses type information so it can detect non-literal ranges as well
(Range or RangeFrom -- the other range types don't have step_by).
---
 README.md                   |  1 +
 src/lib.rs                  |  3 +++
 src/ranges.rs               | 52 +++++++++++++++++++++++++++++++++++++
 tests/compile-fail/range.rs | 24 +++++++++++++++++
 4 files changed, 80 insertions(+)
 create mode 100644 src/ranges.rs
 create mode 100644 tests/compile-fail/range.rs

diff --git a/README.md b/README.md
index d9dea8163..bc82b2ee5 100644
--- a/README.md
+++ b/README.md
@@ -35,6 +35,7 @@ non_ascii_literal    | allow   | using any literal non-ASCII chars in a string l
 option_unwrap_used   | allow   | using `Option.unwrap()`, which should at least get a better message using `expect()`
 precedence           | warn    | expressions where precedence may trip up the unwary reader of the source; suggests adding parentheses, e.g. `x << 2 + y` will be parsed as `x << (2 + y)`
 ptr_arg              | allow   | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively
+range_step_by_zero   | warn    | using Range::step_by(0), which produces an infinite iterator
 redundant_closure    | warn    | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)
 result_unwrap_used   | allow   | using `Result.unwrap()`, which might be better handled
 single_match         | warn    | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead
diff --git a/src/lib.rs b/src/lib.rs
index 01a2d6560..91859005b 100755
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -35,6 +35,7 @@ pub mod methods;
 pub mod returns;
 pub mod lifetimes;
 pub mod loops;
+pub mod ranges;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
@@ -64,6 +65,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box types::LetPass as LintPassObject);
     reg.register_lint_pass(box loops::LoopsPass as LintPassObject);
     reg.register_lint_pass(box lifetimes::LifetimePass as LintPassObject);
+    reg.register_lint_pass(box ranges::StepByZero as LintPassObject);
 
     reg.register_lint_group("clippy", vec![
         approx_const::APPROX_CONSTANT,
@@ -93,6 +95,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         mut_mut::MUT_MUT,
         needless_bool::NEEDLESS_BOOL,
         ptr_arg::PTR_ARG,
+        ranges::RANGE_STEP_BY_ZERO,
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
         strings::STRING_ADD,
diff --git a/src/ranges.rs b/src/ranges.rs
new file mode 100644
index 000000000..2981574e5
--- /dev/null
+++ b/src/ranges.rs
@@ -0,0 +1,52 @@
+use rustc::lint::{Context, LintArray, LintPass};
+use rustc::middle::ty::TypeVariants::TyStruct;
+use syntax::ast::*;
+use syntax::codemap::Spanned;
+use utils::{match_def_path, walk_ptrs_ty};
+
+declare_lint! {
+    pub RANGE_STEP_BY_ZERO, Warn,
+    "using Range::step_by(0), which produces an infinite iterator"
+}
+
+#[derive(Copy,Clone)]
+pub struct StepByZero;
+
+impl LintPass for StepByZero {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(RANGE_STEP_BY_ZERO)
+    }
+
+    fn check_expr(&mut self, cx: &Context, expr: &Expr) {
+        if let ExprMethodCall(Spanned { node: ref ident, .. }, _,
+                              ref args) = expr.node {
+            // Only warn on literal ranges.
+            if ident.name.as_str() == "step_by" && args.len() == 2 &&
+                is_range(cx, &args[0]) && is_lit_zero(&args[1]) {
+                cx.span_lint(RANGE_STEP_BY_ZERO, expr.span,
+                             "Range::step_by(0) produces an infinite iterator. \
+                              Consider using `std::iter::repeat()` instead")
+            }
+        }
+    }
+}
+
+fn is_range(cx: &Context, 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.
+    if let TyStruct(did, _) = cx.tcx.expr_ty(expr).sty {
+        // Note: RangeTo and RangeFull don't have step_by
+        match_def_path(cx, did.did, &["core", "ops", "Range"]) ||
+        match_def_path(cx, did.did, &["core", "ops", "RangeFrom"])
+    } else { false }
+}
+
+fn is_lit_zero(expr: &Expr) -> bool {
+    // FIXME: use constant folding
+    if let ExprLit(ref spanned) = expr.node {
+        if let LitInt(0, _) = spanned.node {
+            return true;
+        }
+    }
+    false
+}
diff --git a/tests/compile-fail/range.rs b/tests/compile-fail/range.rs
new file mode 100644
index 000000000..324f129fa
--- /dev/null
+++ b/tests/compile-fail/range.rs
@@ -0,0 +1,24 @@
+#![feature(step_by)]
+#![feature(plugin)]
+#![plugin(clippy)]
+
+struct NotARange;
+impl NotARange {
+    fn step_by(&self, _: u32) {}
+}
+
+#[deny(range_step_by_zero)]
+fn main() {
+    (0..1).step_by(0); //~ERROR Range::step_by(0) produces an infinite iterator
+    // No warning for non-zero step
+    (0..1).step_by(1);
+
+    (1..).step_by(0); //~ERROR Range::step_by(0) produces an infinite iterator
+
+    let x = 0..1;
+    x.step_by(0); //~ERROR Range::step_by(0) produces an infinite iterator
+
+    // No error, not a range.
+    let y = NotARange;
+    y.step_by(0);
+}