Merge pull request #2116 from niklasf/range-plus-minus-one

Lint range_plus_one and range_minus_one (closes #329)

clippy_lints/src/lib.rs

@@ -263,7 +263,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box loops::Pass);
     reg.register_late_lint_pass(box lifetimes::LifetimePass);
     reg.register_late_lint_pass(box entry::HashMapLint);
-    reg.register_late_lint_pass(box ranges::StepByZero);
+    reg.register_late_lint_pass(box ranges::Pass);
     reg.register_late_lint_pass(box types::CastPass);
     reg.register_late_lint_pass(box types::TypeComplexityPass::new(conf.type_complexity_threshold));
     reg.register_late_lint_pass(box matches::MatchPass);

clippy_lints/src/ranges.rs

@@ -1,7 +1,10 @@
 use rustc::lint::*;
 use rustc::hir::*;
-use utils::{is_integer_literal, paths, snippet, span_lint};
+use syntax::ast::RangeLimits;
+use syntax::codemap::Spanned;
+use utils::{is_integer_literal, paths, snippet, span_lint, span_lint_and_then};
 use utils::{get_trait_def_id, higher, implements_trait};
+use utils::sugg::Sugg;
 
 /// **What it does:** Checks for calling `.step_by(0)` on iterators,
 /// which never terminates.
@@ -38,16 +41,57 @@ declare_lint! {
     "zipping iterator with a range when `enumerate()` would do"
 }
 
-#[derive(Copy, Clone)]
-pub struct StepByZero;
+/// **What it does:** Checks for exclusive ranges where 1 is added to the
+/// upper bound, e.g. `x..(y+1)`.
+///
+/// **Why is this bad?** The code is more readable with an inclusive range
+/// like `x..=y`.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// for x..(y+1) { .. }
+/// ```
+declare_lint! {
+    pub RANGE_PLUS_ONE,
+    Allow,
+    "`x..(y+1)` reads better as `x..=y`"
+}
 
-impl LintPass for StepByZero {
+/// **What it does:** Checks for inclusive ranges where 1 is subtracted from
+/// the upper bound, e.g. `x..=(y-1)`.
+///
+/// **Why is this bad?** The code is more readable with an exclusive range
+/// like `x..y`.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// for x..=(y-1) { .. }
+/// ```
+declare_lint! {
+    pub RANGE_MINUS_ONE,
+    Warn,
+    "`x..=(y-1)` reads better as `x..y`"
+}
+
+#[derive(Copy, Clone)]
+pub struct Pass;
+
+impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(ITERATOR_STEP_BY_ZERO, RANGE_ZIP_WITH_LEN)
+        lint_array!(
+            ITERATOR_STEP_BY_ZERO,
+            RANGE_ZIP_WITH_LEN,
+            RANGE_PLUS_ONE,
+            RANGE_MINUS_ONE
+        )
     }
 }
 
-impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StepByZero {
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
         if let ExprMethodCall(ref path, _, ref args) = expr.node {
             let name = path.name.as_str();
@@ -92,6 +136,46 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StepByZero {
                 }}
             }
         }
+
+        // exclusive range plus one: x..(y+1)
+        if_let_chain! {[
+            let Some(higher::Range { start, end: Some(end), limits: RangeLimits::HalfOpen }) = higher::range(expr),
+            let Some(y) = y_plus_one(end),
+        ], {
+            span_lint_and_then(
+                cx,
+                RANGE_PLUS_ONE,
+                expr.span,
+                "an inclusive range would be more readable",
+                |db| {
+                    let start = start.map_or("".to_owned(), |x| Sugg::hir(cx, x, "x").to_string());
+                    let end = Sugg::hir(cx, y, "y");
+                    db.span_suggestion(expr.span,
+                                       "use",
+                                       format!("{}..={}", start, end));
+                },
+            );
+        }}
+
+        // inclusive range minus one: x..=(y-1)
+        if_let_chain! {[
+            let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(expr),
+            let Some(y) = y_minus_one(end),
+        ], {
+            span_lint_and_then(
+                cx,
+                RANGE_MINUS_ONE,
+                expr.span,
+                "an exclusive range would be more readable",
+                |db| {
+                    let start = start.map_or("".to_owned(), |x| Sugg::hir(cx, x, "x").to_string());
+                    let end = Sugg::hir(cx, y, "y");
+                    db.span_suggestion(expr.span,
+                                       "use",
+                                       format!("{}..{}", start, end));
+                },
+            );
+        }}
     }
 }
 
@@ -102,3 +186,25 @@ fn has_step_by(cx: &LateContext, expr: &Expr) -> bool {
 
     get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[]))
 }
+
+fn y_plus_one(expr: &Expr) -> Option<&Expr> {
+    match expr.node {
+        ExprBinary(Spanned { node: BiAdd, .. }, ref lhs, ref rhs) => {
+            if is_integer_literal(lhs, 1) {
+                Some(rhs)
+            } else if is_integer_literal(rhs, 1) {
+                Some(lhs)
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}
+
+fn y_minus_one(expr: &Expr) -> Option<&Expr> {
+    match expr.node {
+        ExprBinary(Spanned { node: BiSub, .. }, ref lhs, ref rhs) if is_integer_literal(rhs, 1) => Some(lhs),
+        _ => None,
+    }
+}

tests/ui/range_plus_minus_one.rs

@@ -0,0 +1,34 @@
+#![feature(inclusive_range_syntax)]
+
+fn f() -> usize {
+    42
+}
+
+#[warn(range_plus_one)]
+fn main() {
+    for _ in 0..2 { }
+    for _ in 0..=2 { }
+
+    for _ in 0..3+1 { }
+    for _ in 0..=3+1 { }
+
+    for _ in 0..1+5 { }
+    for _ in 0..=1+5 { }
+
+    for _ in 1..1+1 { }
+    for _ in 1..=1+1 { }
+
+    for _ in 0..13+13 { }
+    for _ in 0..=13-7 { }
+
+    for _ in 0..(1+f()) { }
+    for _ in 0..=(1+f()) { }
+
+    let _ = ..11-1;
+    let _ = ..=11-1;
+    let _ = ..=(11-1);
+    let _ = (f()+1)..(f()+1);
+
+    let mut vec: Vec<()> = std::vec::Vec::new();
+    vec.drain(..);
+}

tests/ui/range_plus_minus_one.stderr

@@ -0,0 +1,67 @@
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:12:14
+   |
+12 |     for _ in 0..3+1 { }
+   |              ------
+   |              |
+   |              help: use: `0..=3`
+   |              in this macro invocation
+   |
+   = note: `-D range-plus-one` implied by `-D warnings`
+
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:15:14
+   |
+15 |     for _ in 0..1+5 { }
+   |              ------
+   |              |
+   |              help: use: `0..=5`
+   |              in this macro invocation
+
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:18:14
+   |
+18 |     for _ in 1..1+1 { }
+   |              ------
+   |              |
+   |              help: use: `1..=1`
+   |              in this macro invocation
+
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:24:14
+   |
+24 |     for _ in 0..(1+f()) { }
+   |              ----------
+   |              |
+   |              help: use: `0..=f()`
+   |              in this macro invocation
+
+error: an exclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:28:13
+   |
+28 |     let _ = ..=11-1;
+   |             -------
+   |             |
+   |             help: use: `..11`
+   |             in this macro invocation
+   |
+   = note: `-D range-minus-one` implied by `-D warnings`
+
+error: an exclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:29:13
+   |
+29 |     let _ = ..=(11-1);
+   |             ---------
+   |             |
+   |             help: use: `..11`
+   |             in this macro invocation
+
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:30:13
+   |
+30 |     let _ = (f()+1)..(f()+1);
+   |             ----------------
+   |             |
+   |             help: use: `(f()+1)..=f()`
+   |             in this macro invocation
+