diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4af88d5fed..0f27a74ac8e 100644 --- a/clippy_lints/src/lib.rs +++ b/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); diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 61b61c9fb6f..aff0c4b08ab 100644 --- a/clippy_lints/src/ranges.rs +++ b/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, + } +} diff --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs new file mode 100644 index 00000000000..dce81634876 --- /dev/null +++ b/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(..); +} diff --git a/tests/ui/range_plus_minus_one.stderr b/tests/ui/range_plus_minus_one.stderr new file mode 100644 index 00000000000..a2a3ae6077f --- /dev/null +++ b/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 +