From 38925a55b7bd2e2f3b9e6c8fe0fde4dc41bb8549 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Sun, 18 Jun 2017 15:14:46 +0200 Subject: [PATCH] Replace `Range::step_by` checking with `Iterator::step_by` --- CHANGELOG.md | 3 ++ README.md | 2 +- clippy_lints/src/deprecated_lints.rs | 10 +++++++ clippy_lints/src/lib.rs | 6 +++- clippy_lints/src/ranges.rs | 42 +++++++++++++++++----------- clippy_tests/examples/range.rs | 6 +++- clippy_tests/examples/range.stderr | 32 +++++++++++++-------- 7 files changed, 70 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78132722c56..5c11be44c08 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index e0f45d21b10..4b79c11aa38 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index 17139f28c2e..0c40823cd06 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/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. /// diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 410cdf6264a..ec73e9d50d9 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 5b7cb9aa86f..5f1ac61918a 100644 --- a/clippy_lints/src/ranges.rs +++ b/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, &[]) + ) } diff --git a/clippy_tests/examples/range.rs b/clippy_tests/examples/range.rs index 0f648f8f876..cb7504a9ecc 100644 --- a/clippy_tests/examples/range.rs +++ b/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); } diff --git a/clippy_tests/examples/range.stderr b/clippy_tests/examples/range.stderr index 7620908b084..53ba8d45b5d 100644 --- a/clippy_tests/examples/range.stderr +++ b/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`.