From e097fca4df2ff70e0213d747a408d109db16c5d2 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 18 Dec 2019 18:59:43 +0200 Subject: [PATCH 1/5] Update iterator_step_by_zero Move `iterator_step_by_zero` into `methods` since it applies to all iterators and not just ranges. Simplify the code while doing so. --- clippy_lints/src/lib.rs | 6 ++-- clippy_lints/src/methods/mod.rs | 36 ++++++++++++++++++++ clippy_lints/src/ranges.rs | 47 ++------------------------- tests/ui/iterator_step_by_zero.rs | 28 ++++++++++++++++ tests/ui/iterator_step_by_zero.stderr | 46 ++++++++++++++++++++++++++ tests/ui/range.rs | 24 +------------- tests/ui/range.stderr | 36 ++------------------ 7 files changed, 119 insertions(+), 104 deletions(-) create mode 100644 tests/ui/iterator_step_by_zero.rs create mode 100644 tests/ui/iterator_step_by_zero.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bae4eebf8..7fb499ebf 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -606,6 +606,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &methods::GET_UNWRAP, &methods::INEFFICIENT_TO_STRING, &methods::INTO_ITER_ON_REF, + &methods::ITERATOR_STEP_BY_ZERO, &methods::ITER_CLONED_COLLECT, &methods::ITER_NTH, &methods::ITER_SKIP_NEXT, @@ -699,7 +700,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &ptr::PTR_ARG, &ptr_offset_with_cast::PTR_OFFSET_WITH_CAST, &question_mark::QUESTION_MARK, - &ranges::ITERATOR_STEP_BY_ZERO, &ranges::RANGE_MINUS_ONE, &ranges::RANGE_PLUS_ONE, &ranges::RANGE_ZIP_WITH_LEN, @@ -1179,6 +1179,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&methods::FLAT_MAP_IDENTITY), LintId::of(&methods::INEFFICIENT_TO_STRING), LintId::of(&methods::INTO_ITER_ON_REF), + LintId::of(&methods::ITERATOR_STEP_BY_ZERO), LintId::of(&methods::ITER_CLONED_COLLECT), LintId::of(&methods::ITER_NTH), LintId::of(&methods::ITER_SKIP_NEXT), @@ -1244,7 +1245,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&ptr::PTR_ARG), LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST), LintId::of(&question_mark::QUESTION_MARK), - LintId::of(&ranges::ITERATOR_STEP_BY_ZERO), LintId::of(&ranges::RANGE_MINUS_ONE), LintId::of(&ranges::RANGE_PLUS_ONE), LintId::of(&ranges::RANGE_ZIP_WITH_LEN), @@ -1521,6 +1521,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM), LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT), LintId::of(&methods::CLONE_DOUBLE_REF), + LintId::of(&methods::ITERATOR_STEP_BY_ZERO), LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR), LintId::of(&methods::UNINIT_ASSUMED_INIT), LintId::of(&methods::ZST_OFFSET), @@ -1533,7 +1534,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&ptr::MUT_FROM_REF), - LintId::of(&ranges::ITERATOR_STEP_BY_ZERO), LintId::of(®ex::INVALID_REGEX), LintId::of(&serde_api::SERDE_API_MISUSE), LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ca62e7ea9..8e94cc0f0 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -737,6 +737,26 @@ declare_clippy_lint! { "getting the inner pointer of a temporary `CString`" } +declare_clippy_lint! { + /// **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 + /// `loop { .. }` there is an obvious better way to endlessly loop. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```ignore + /// for x in (5..5).step_by(0) { + /// .. + /// } + /// ``` + pub ITERATOR_STEP_BY_ZERO, + correctness, + "using `Iterator::step_by(0)`, which produces an infinite iterator" +} + declare_clippy_lint! { /// **What it does:** Checks for use of `.iter().nth()` (and the related /// `.iter_mut().nth()`) on standard library types with O(1) element access. @@ -1115,6 +1135,7 @@ declare_lint_pass!(Methods => [ FLAT_MAP_IDENTITY, FIND_MAP, MAP_FLATTEN, + ITERATOR_STEP_BY_ZERO, ITER_NTH, ITER_SKIP_NEXT, GET_UNWRAP, @@ -1173,6 +1194,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { }, ["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false), ["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true), + ["step_by", ..] => lint_step_by(cx, expr, arg_lists[0]), ["next", "skip"] => lint_iter_skip_next(cx, expr), ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]), ["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]), @@ -1950,6 +1972,20 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: } } +fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, args: &'tcx [hir::Expr]) { + if match_trait_method(cx, expr, &paths::ITERATOR) { + use crate::consts::{constant, Constant}; + if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) { + span_lint( + cx, + ITERATOR_STEP_BY_ZERO, + expr.span, + "Iterator::step_by(0) will panic at runtime", + ); + } + } +} + fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) { let mut_str = if is_mut { "_mut" } else { "" }; let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() { diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index b2db5f936..6a16adf71 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -8,28 +8,8 @@ use syntax::ast::RangeLimits; use syntax::source_map::Spanned; use crate::utils::sugg::Sugg; -use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq}; -use crate::utils::{is_integer_const, paths, snippet, snippet_opt, span_lint, span_lint_and_then}; - -declare_clippy_lint! { - /// **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 - /// `loop { .. }` there is an obvious better way to endlessly loop. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```ignore - /// for x in (5..5).step_by(0) { - /// .. - /// } - /// ``` - pub ITERATOR_STEP_BY_ZERO, - correctness, - "using `Iterator::step_by(0)`, which produces an infinite iterator" -} +use crate::utils::{higher, SpanlessEq}; +use crate::utils::{is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then}; declare_clippy_lint! { /// **What it does:** Checks for zipping a collection with the range of @@ -102,7 +82,6 @@ declare_clippy_lint! { } declare_lint_pass!(Ranges => [ - ITERATOR_STEP_BY_ZERO, RANGE_ZIP_WITH_LEN, RANGE_PLUS_ONE, RANGE_MINUS_ONE @@ -112,19 +91,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind { let name = path.ident.as_str(); - - // Range with step_by(0). - if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) { - use crate::consts::{constant, Constant}; - if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) { - span_lint( - cx, - ITERATOR_STEP_BY_ZERO, - expr.span, - "Iterator::step_by(0) will panic at runtime", - ); - } - } else if name == "zip" && args.len() == 2 { + if name == "zip" && args.len() == 2 { let iter = &args[0].kind; let zip_arg = &args[1]; if_chain! { @@ -232,14 +199,6 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) { } } -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_adjusted(expr); - - get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[])) -} - fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> { match expr.kind { ExprKind::Binary( diff --git a/tests/ui/iterator_step_by_zero.rs b/tests/ui/iterator_step_by_zero.rs new file mode 100644 index 000000000..13d1cfd42 --- /dev/null +++ b/tests/ui/iterator_step_by_zero.rs @@ -0,0 +1,28 @@ +#[warn(clippy::iterator_step_by_zero)] +fn main() { + let _ = vec!["A", "B", "B"].iter().step_by(0); + let _ = "XXX".chars().step_by(0); + let _ = (0..1).step_by(0); + + // No error, not an iterator. + let y = NotIterator; + y.step_by(0); + + // No warning for non-zero step + let _ = (0..1).step_by(1); + + let _ = (1..).step_by(0); + let _ = (1..=2).step_by(0); + + let x = 0..1; + let _ = x.step_by(0); + + // check const eval + let v1 = vec![1, 2, 3]; + let _ = v1.iter().step_by(2 / 3); +} + +struct NotIterator; +impl NotIterator { + fn step_by(&self, _: u32) {} +} diff --git a/tests/ui/iterator_step_by_zero.stderr b/tests/ui/iterator_step_by_zero.stderr new file mode 100644 index 000000000..c2c6803b3 --- /dev/null +++ b/tests/ui/iterator_step_by_zero.stderr @@ -0,0 +1,46 @@ +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:3:13 + | +LL | let _ = vec!["A", "B", "B"].iter().step_by(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::iterator-step-by-zero` implied by `-D warnings` + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:4:13 + | +LL | let _ = "XXX".chars().step_by(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:5:13 + | +LL | let _ = (0..1).step_by(0); + | ^^^^^^^^^^^^^^^^^ + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:14:13 + | +LL | let _ = (1..).step_by(0); + | ^^^^^^^^^^^^^^^^ + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:15:13 + | +LL | let _ = (1..=2).step_by(0); + | ^^^^^^^^^^^^^^^^^^ + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:18:13 + | +LL | let _ = x.step_by(0); + | ^^^^^^^^^^^^ + +error: Iterator::step_by(0) will panic at runtime + --> $DIR/iterator_step_by_zero.rs:22:13 + | +LL | let _ = v1.iter().step_by(2 / 3); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors + diff --git a/tests/ui/range.rs b/tests/ui/range.rs index d0c5cc93b..628282509 100644 --- a/tests/ui/range.rs +++ b/tests/ui/range.rs @@ -1,31 +1,9 @@ -struct NotARange; -impl NotARange { - fn step_by(&self, _: u32) {} -} - -#[warn(clippy::iterator_step_by_zero, clippy::range_zip_with_len)] +#[warn(clippy::range_zip_with_len)] fn main() { - let _ = (0..1).step_by(0); - // No warning for non-zero step - let _ = (0..1).step_by(1); - - let _ = (1..).step_by(0); - let _ = (1..=2).step_by(0); - - let x = 0..1; - let _ = x.step_by(0); - - // No error, not a range. - let y = NotARange; - y.step_by(0); - let v1 = vec![1, 2, 3]; 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); } #[allow(unused)] diff --git a/tests/ui/range.stderr b/tests/ui/range.stderr index 387d1f674..c8d4e557d 100644 --- a/tests/ui/range.stderr +++ b/tests/ui/range.stderr @@ -1,42 +1,10 @@ -error: Iterator::step_by(0) will panic at runtime - --> $DIR/range.rs:8:13 - | -LL | let _ = (0..1).step_by(0); - | ^^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::iterator-step-by-zero` implied by `-D warnings` - -error: Iterator::step_by(0) will panic at runtime - --> $DIR/range.rs:12:13 - | -LL | let _ = (1..).step_by(0); - | ^^^^^^^^^^^^^^^^ - -error: Iterator::step_by(0) will panic at runtime - --> $DIR/range.rs:13:13 - | -LL | let _ = (1..=2).step_by(0); - | ^^^^^^^^^^^^^^^^^^ - -error: Iterator::step_by(0) will panic at runtime - --> $DIR/range.rs:16:13 - | -LL | let _ = x.step_by(0); - | ^^^^^^^^^^^^ - error: It is more idiomatic to use v1.iter().enumerate() - --> $DIR/range.rs:24:14 + --> $DIR/range.rs:5:14 | LL | let _x = v1.iter().zip(0..v1.len()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::range-zip-with-len` implied by `-D warnings` -error: Iterator::step_by(0) will panic at runtime - --> $DIR/range.rs:28:13 - | -LL | let _ = v1.iter().step_by(2 / 3); - | ^^^^^^^^^^^^^^^^^^^^^^^^ - -error: aborting due to 6 previous errors +error: aborting due to previous error From 38d0b2199a5a945c9ac31cf5d9a96b28869edcb1 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 18 Dec 2019 18:59:59 +0200 Subject: [PATCH 2/5] Correct `iterator_step_by_zero` documentation --- clippy_lints/src/methods/mod.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8e94cc0f0..9b375d595 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -738,18 +738,17 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for calling `.step_by(0)` on iterators, - /// which never terminates. + /// **What it does:** Checks for calling `.step_by(0)` on iterators which panics. /// - /// **Why is this bad?** This very much looks like an oversight, since with - /// `loop { .. }` there is an obvious better way to endlessly loop. + /// **Why is this bad?** This very much looks like an oversight. Use `panic!()` instead if you + /// actually intend to panic. /// /// **Known problems:** None. /// /// **Example:** - /// ```ignore - /// for x in (5..5).step_by(0) { - /// .. + /// ```should_panic + /// for x in (0..100).step_by(0) { + /// //.. /// } /// ``` pub ITERATOR_STEP_BY_ZERO, From ecbfa386d4070708c512ddcc15ef7fec24da0b57 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 18 Dec 2019 19:19:26 +0200 Subject: [PATCH 3/5] Fix `iterator_step_by_zero` definition --- src/lintlist/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f4ebf6cbd..201a80d81 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -887,7 +887,7 @@ pub const ALL_LINTS: [Lint; 340] = [ group: "correctness", desc: "using `Iterator::step_by(0)`, which produces an infinite iterator", deprecation: None, - module: "ranges", + module: "methods", }, Lint { name: "just_underscores_and_digits", From 710e06dd298f13ce3d68c03b91ecb2c26086d134 Mon Sep 17 00:00:00 2001 From: mikerite <33983332+mikerite@users.noreply.github.com> Date: Thu, 19 Dec 2019 06:51:26 +0200 Subject: [PATCH 4/5] Fix `iterator_step_by_zero` description in declaration Co-Authored-By: Phil Hansch --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 9b375d595..5dfa0e328 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -753,7 +753,7 @@ declare_clippy_lint! { /// ``` pub ITERATOR_STEP_BY_ZERO, correctness, - "using `Iterator::step_by(0)`, which produces an infinite iterator" + "using `Iterator::step_by(0)`, which will panic at runtime" } declare_clippy_lint! { From 3a81e60a29c53d6638319dafd5123fcbe59bb0b2 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 20 Dec 2019 08:22:43 +0200 Subject: [PATCH 5/5] Update lints for `iterator_step_by_zero` changes --- src/lintlist/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 201a80d81..c91be11f4 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -885,7 +885,7 @@ pub const ALL_LINTS: [Lint; 340] = [ Lint { name: "iterator_step_by_zero", group: "correctness", - desc: "using `Iterator::step_by(0)`, which produces an infinite iterator", + desc: "using `Iterator::step_by(0)`, which will panic at runtime", deprecation: None, module: "methods", },