Skip to content

Commit a86463c

Browse files
borsbors
committed
Auto merge of #4913 - mikerite:step-by-zero-20191218, r=phansch
Move `iterator_step_by_zero` and correct the documentation Move `iterator_step_by_zero` and correct the documentation. changelog: Corrected `iterator_step_by_zero` documentation
2 parents ab105ee + 3a81e60 commit a86463c

File tree

8 files changed

+120
-106
lines changed

8 files changed

+120
-106
lines changed

‎clippy_lints/src/lib.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -606,6 +606,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
606606
&methods::GET_UNWRAP,
607607
&methods::INEFFICIENT_TO_STRING,
608608
&methods::INTO_ITER_ON_REF,
609+
&methods::ITERATOR_STEP_BY_ZERO,
609610
&methods::ITER_CLONED_COLLECT,
610611
&methods::ITER_NTH,
611612
&methods::ITER_SKIP_NEXT,
@@ -699,7 +700,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
699700
&ptr::PTR_ARG,
700701
&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
701702
&question_mark::QUESTION_MARK,
702-
&ranges::ITERATOR_STEP_BY_ZERO,
703703
&ranges::RANGE_MINUS_ONE,
704704
&ranges::RANGE_PLUS_ONE,
705705
&ranges::RANGE_ZIP_WITH_LEN,
@@ -1179,6 +1179,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
11791179
LintId::of(&methods::FLAT_MAP_IDENTITY),
11801180
LintId::of(&methods::INEFFICIENT_TO_STRING),
11811181
LintId::of(&methods::INTO_ITER_ON_REF),
1182+
LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
11821183
LintId::of(&methods::ITER_CLONED_COLLECT),
11831184
LintId::of(&methods::ITER_NTH),
11841185
LintId::of(&methods::ITER_SKIP_NEXT),
@@ -1244,7 +1245,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
12441245
LintId::of(&ptr::PTR_ARG),
12451246
LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST),
12461247
LintId::of(&question_mark::QUESTION_MARK),
1247-
LintId::of(&ranges::ITERATOR_STEP_BY_ZERO),
12481248
LintId::of(&ranges::RANGE_MINUS_ONE),
12491249
LintId::of(&ranges::RANGE_PLUS_ONE),
12501250
LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
@@ -1521,6 +1521,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
15211521
LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM),
15221522
LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT),
15231523
LintId::of(&methods::CLONE_DOUBLE_REF),
1524+
LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
15241525
LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
15251526
LintId::of(&methods::UNINIT_ASSUMED_INIT),
15261527
LintId::of(&methods::ZST_OFFSET),
@@ -1533,7 +1534,6 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
15331534
LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
15341535
LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
15351536
LintId::of(&ptr::MUT_FROM_REF),
1536-
LintId::of(&ranges::ITERATOR_STEP_BY_ZERO),
15371537
LintId::of(&regex::INVALID_REGEX),
15381538
LintId::of(&serde_api::SERDE_API_MISUSE),
15391539
LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),

‎clippy_lints/src/methods/mod.rs

+35
Original file line numberDiff line numberDiff line change
@@ -737,6 +737,25 @@ declare_clippy_lint! {
737737
"getting the inner pointer of a temporary `CString`"
738738
}
739739

740+
declare_clippy_lint! {
741+
/// **What it does:** Checks for calling `.step_by(0)` on iterators which panics.
742+
///
743+
/// **Why is this bad?** This very much looks like an oversight. Use `panic!()` instead if you
744+
/// actually intend to panic.
745+
///
746+
/// **Known problems:** None.
747+
///
748+
/// **Example:**
749+
/// ```should_panic
750+
/// for x in (0..100).step_by(0) {
751+
/// //..
752+
/// }
753+
/// ```
754+
pub ITERATOR_STEP_BY_ZERO,
755+
correctness,
756+
"using `Iterator::step_by(0)`, which will panic at runtime"
757+
}
758+
740759
declare_clippy_lint! {
741760
/// **What it does:** Checks for use of `.iter().nth()` (and the related
742761
/// `.iter_mut().nth()`) on standard library types with O(1) element access.
@@ -1115,6 +1134,7 @@ declare_lint_pass!(Methods => [
11151134
FLAT_MAP_IDENTITY,
11161135
FIND_MAP,
11171136
MAP_FLATTEN,
1137+
ITERATOR_STEP_BY_ZERO,
11181138
ITER_NTH,
11191139
ITER_SKIP_NEXT,
11201140
GET_UNWRAP,
@@ -1173,6 +1193,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
11731193
},
11741194
["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false),
11751195
["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true),
1196+
["step_by", ..] => lint_step_by(cx, expr, arg_lists[0]),
11761197
["next", "skip"] => lint_iter_skip_next(cx, expr),
11771198
["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]),
11781199
["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]),
@@ -1950,6 +1971,20 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args:
19501971
}
19511972
}
19521973

1974+
fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, args: &'tcx [hir::Expr]) {
1975+
if match_trait_method(cx, expr, &paths::ITERATOR) {
1976+
use crate::consts::{constant, Constant};
1977+
if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) {
1978+
span_lint(
1979+
cx,
1980+
ITERATOR_STEP_BY_ZERO,
1981+
expr.span,
1982+
"Iterator::step_by(0) will panic at runtime",
1983+
);
1984+
}
1985+
}
1986+
}
1987+
19531988
fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) {
19541989
let mut_str = if is_mut { "_mut" } else { "" };
19551990
let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() {

‎clippy_lints/src/ranges.rs

+3-44
Original file line numberDiff line numberDiff line change
@@ -8,28 +8,8 @@ use syntax::ast::RangeLimits;
88
use syntax::source_map::Spanned;
99

1010
use crate::utils::sugg::Sugg;
11-
use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq};
12-
use crate::utils::{is_integer_const, paths, snippet, snippet_opt, span_lint, span_lint_and_then};
13-
14-
declare_clippy_lint! {
15-
/// **What it does:** Checks for calling `.step_by(0)` on iterators,
16-
/// which never terminates.
17-
///
18-
/// **Why is this bad?** This very much looks like an oversight, since with
19-
/// `loop { .. }` there is an obvious better way to endlessly loop.
20-
///
21-
/// **Known problems:** None.
22-
///
23-
/// **Example:**
24-
/// ```ignore
25-
/// for x in (5..5).step_by(0) {
26-
/// ..
27-
/// }
28-
/// ```
29-
pub ITERATOR_STEP_BY_ZERO,
30-
correctness,
31-
"using `Iterator::step_by(0)`, which produces an infinite iterator"
32-
}
11+
use crate::utils::{higher, SpanlessEq};
12+
use crate::utils::{is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
3313

3414
declare_clippy_lint! {
3515
/// **What it does:** Checks for zipping a collection with the range of
@@ -102,7 +82,6 @@ declare_clippy_lint! {
10282
}
10383

10484
declare_lint_pass!(Ranges => [
105-
ITERATOR_STEP_BY_ZERO,
10685
RANGE_ZIP_WITH_LEN,
10786
RANGE_PLUS_ONE,
10887
RANGE_MINUS_ONE
@@ -112,19 +91,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
11291
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
11392
if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind {
11493
let name = path.ident.as_str();
115-
116-
// Range with step_by(0).
117-
if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) {
118-
use crate::consts::{constant, Constant};
119-
if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) {
120-
span_lint(
121-
cx,
122-
ITERATOR_STEP_BY_ZERO,
123-
expr.span,
124-
"Iterator::step_by(0) will panic at runtime",
125-
);
126-
}
127-
} else if name == "zip" && args.len() == 2 {
94+
if name == "zip" && args.len() == 2 {
12895
let iter = &args[0].kind;
12996
let zip_arg = &args[1];
13097
if_chain! {
@@ -232,14 +199,6 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
232199
}
233200
}
234201

235-
fn has_step_by(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
236-
// No need for `walk_ptrs_ty` here because `step_by` moves `self`, so it
237-
// can't be called on a borrowed range.
238-
let ty = cx.tables.expr_ty_adjusted(expr);
239-
240-
get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[]))
241-
}
242-
243202
fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> {
244203
match expr.kind {
245204
ExprKind::Binary(

‎src/lintlist/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -885,9 +885,9 @@ pub const ALL_LINTS: [Lint; 340] = [
885885
Lint {
886886
name: "iterator_step_by_zero",
887887
group: "correctness",
888-
desc: "using `Iterator::step_by(0)`, which produces an infinite iterator",
888+
desc: "using `Iterator::step_by(0)`, which will panic at runtime",
889889
deprecation: None,
890-
module: "ranges",
890+
module: "methods",
891891
},
892892
Lint {
893893
name: "just_underscores_and_digits",

‎tests/ui/iterator_step_by_zero.rs

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
#[warn(clippy::iterator_step_by_zero)]
2+
fn main() {
3+
let _ = vec!["A", "B", "B"].iter().step_by(0);
4+
let _ = "XXX".chars().step_by(0);
5+
let _ = (0..1).step_by(0);
6+
7+
// No error, not an iterator.
8+
let y = NotIterator;
9+
y.step_by(0);
10+
11+
// No warning for non-zero step
12+
let _ = (0..1).step_by(1);
13+
14+
let _ = (1..).step_by(0);
15+
let _ = (1..=2).step_by(0);
16+
17+
let x = 0..1;
18+
let _ = x.step_by(0);
19+
20+
// check const eval
21+
let v1 = vec![1, 2, 3];
22+
let _ = v1.iter().step_by(2 / 3);
23+
}
24+
25+
struct NotIterator;
26+
impl NotIterator {
27+
fn step_by(&self, _: u32) {}
28+
}

‎tests/ui/iterator_step_by_zero.stderr

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
error: Iterator::step_by(0) will panic at runtime
2+
--> $DIR/iterator_step_by_zero.rs:3:13
3+
|
4+
LL | let _ = vec!["A", "B", "B"].iter().step_by(0);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::iterator-step-by-zero` implied by `-D warnings`
8+
9+
error: Iterator::step_by(0) will panic at runtime
10+
--> $DIR/iterator_step_by_zero.rs:4:13
11+
|
12+
LL | let _ = "XXX".chars().step_by(0);
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^
14+
15+
error: Iterator::step_by(0) will panic at runtime
16+
--> $DIR/iterator_step_by_zero.rs:5:13
17+
|
18+
LL | let _ = (0..1).step_by(0);
19+
| ^^^^^^^^^^^^^^^^^
20+
21+
error: Iterator::step_by(0) will panic at runtime
22+
--> $DIR/iterator_step_by_zero.rs:14:13
23+
|
24+
LL | let _ = (1..).step_by(0);
25+
| ^^^^^^^^^^^^^^^^
26+
27+
error: Iterator::step_by(0) will panic at runtime
28+
--> $DIR/iterator_step_by_zero.rs:15:13
29+
|
30+
LL | let _ = (1..=2).step_by(0);
31+
| ^^^^^^^^^^^^^^^^^^
32+
33+
error: Iterator::step_by(0) will panic at runtime
34+
--> $DIR/iterator_step_by_zero.rs:18:13
35+
|
36+
LL | let _ = x.step_by(0);
37+
| ^^^^^^^^^^^^
38+
39+
error: Iterator::step_by(0) will panic at runtime
40+
--> $DIR/iterator_step_by_zero.rs:22:13
41+
|
42+
LL | let _ = v1.iter().step_by(2 / 3);
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^
44+
45+
error: aborting due to 7 previous errors
46+

‎tests/ui/range.rs

+1-23
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,9 @@
1-
struct NotARange;
2-
impl NotARange {
3-
fn step_by(&self, _: u32) {}
4-
}
5-
6-
#[warn(clippy::iterator_step_by_zero, clippy::range_zip_with_len)]
1+
#[warn(clippy::range_zip_with_len)]
72
fn main() {
8-
let _ = (0..1).step_by(0);
9-
// No warning for non-zero step
10-
let _ = (0..1).step_by(1);
11-
12-
let _ = (1..).step_by(0);
13-
let _ = (1..=2).step_by(0);
14-
15-
let x = 0..1;
16-
let _ = x.step_by(0);
17-
18-
// No error, not a range.
19-
let y = NotARange;
20-
y.step_by(0);
21-
223
let v1 = vec![1, 2, 3];
234
let v2 = vec![4, 5];
245
let _x = v1.iter().zip(0..v1.len());
256
let _y = v1.iter().zip(0..v2.len()); // No error
26-
27-
// check const eval
28-
let _ = v1.iter().step_by(2 / 3);
297
}
308

319
#[allow(unused)]

‎tests/ui/range.stderr

+2-34
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,10 @@
1-
error: Iterator::step_by(0) will panic at runtime
2-
--> $DIR/range.rs:8:13
3-
|
4-
LL | let _ = (0..1).step_by(0);
5-
| ^^^^^^^^^^^^^^^^^
6-
|
7-
= note: `-D clippy::iterator-step-by-zero` implied by `-D warnings`
8-
9-
error: Iterator::step_by(0) will panic at runtime
10-
--> $DIR/range.rs:12:13
11-
|
12-
LL | let _ = (1..).step_by(0);
13-
| ^^^^^^^^^^^^^^^^
14-
15-
error: Iterator::step_by(0) will panic at runtime
16-
--> $DIR/range.rs:13:13
17-
|
18-
LL | let _ = (1..=2).step_by(0);
19-
| ^^^^^^^^^^^^^^^^^^
20-
21-
error: Iterator::step_by(0) will panic at runtime
22-
--> $DIR/range.rs:16:13
23-
|
24-
LL | let _ = x.step_by(0);
25-
| ^^^^^^^^^^^^
26-
271
error: It is more idiomatic to use v1.iter().enumerate()
28-
--> $DIR/range.rs:24:14
2+
--> $DIR/range.rs:5:14
293
|
304
LL | let _x = v1.iter().zip(0..v1.len());
315
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
326
|
337
= note: `-D clippy::range-zip-with-len` implied by `-D warnings`
348

35-
error: Iterator::step_by(0) will panic at runtime
36-
--> $DIR/range.rs:28:13
37-
|
38-
LL | let _ = v1.iter().step_by(2 / 3);
39-
| ^^^^^^^^^^^^^^^^^^^^^^^^
40-
41-
error: aborting due to 6 previous errors
9+
error: aborting due to previous error
4210

0 commit comments

Comments
 (0)