clippy_lints/src/loops/mod.rs

   1 mod empty_loop;
   2 mod explicit_counter_loop;
   3 mod explicit_into_iter_loop;
   4 mod explicit_iter_loop;
   5 mod for_kv_map;
   6 mod for_loops_over_fallibles;
   7 mod iter_next_loop;
   8 mod manual_flatten;
   9 mod manual_memcpy;
  10 mod mut_range_bound;
  11 mod needless_collect;
  12 mod needless_range_loop;
  13 mod never_loop;
  14 mod same_item_push;
  15 mod single_element_loop;
  16 mod utils;
  17 mod while_immutable_condition;
  18 mod while_let_loop;
  19 mod while_let_on_iterator;
  20
  21 use clippy_utils::higher;
  22 use rustc_hir::{Expr, ExprKind, LoopSource, Pat};
  23 use rustc_lint::{LateContext, LateLintPass};
  24 use rustc_session::{declare_lint_pass, declare_tool_lint};
  25 use rustc_span::source_map::Span;
  26 use utils::{get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor};
  27
  28 declare_clippy_lint! {
  29     /// ### What it does
  30     /// Checks for for-loops that manually copy items between
  31     /// slices that could be optimized by having a memcpy.
  32     ///
  33     /// ### Why is this bad?
  34     /// It is not as fast as a memcpy.
  35     ///
  36     /// ### Example
  37     /// ```rust
  38     /// # let src = vec![1];
  39     /// # let mut dst = vec![0; 65];
  40     /// for i in 0..src.len() {
  41     ///     dst[i + 64] = src[i];
  42     /// }
  43     /// ```
  44     /// Could be written as:
  45     /// ```rust
  46     /// # let src = vec![1];
  47     /// # let mut dst = vec![0; 65];
  48     /// dst[64..(src.len() + 64)].clone_from_slice(&src[..]);
  49     /// ```
  50     pub MANUAL_MEMCPY,
  51     perf,
  52     "manually copying items between slices"
  53 }
  54
  55 declare_clippy_lint! {
  56     /// ### What it does
  57     /// Checks for looping over the range of `0..len` of some
  58     /// collection just to get the values by index.
  59     ///
  60     /// ### Why is this bad?
  61     /// Just iterating the collection itself makes the intent
  62     /// more clear and is probably faster.
  63     ///
  64     /// ### Example
  65     /// ```rust
  66     /// let vec = vec!['a', 'b', 'c'];
  67     /// for i in 0..vec.len() {
  68     ///     println!("{}", vec[i]);
  69     /// }
  70     /// ```
  71     /// Could be written as:
  72     /// ```rust
  73     /// let vec = vec!['a', 'b', 'c'];
  74     /// for i in vec {
  75     ///     println!("{}", i);
  76     /// }
  77     /// ```
  78     pub NEEDLESS_RANGE_LOOP,
  79     style,
  80     "for-looping over a range of indices where an iterator over items would do"
  81 }
  82
  83 declare_clippy_lint! {
  84     /// ### What it does
  85     /// Checks for loops on `x.iter()` where `&x` will do, and
  86     /// suggests the latter.
  87     ///
  88     /// ### Why is this bad?
  89     /// Readability.
  90     ///
  91     /// ### Known problems
  92     /// False negatives. We currently only warn on some known
  93     /// types.
  94     ///
  95     /// ### Example
  96     /// ```rust
  97     /// // with `y` a `Vec` or slice:
  98     /// # let y = vec![1];
  99     /// for x in y.iter() {
 100     ///     // ..
 101     /// }
 102     /// ```
 103     /// can be rewritten to
 104     /// ```rust
 105     /// # let y = vec![1];
 106     /// for x in &y {
 107     ///     // ..
 108     /// }
 109     /// ```
 110     pub EXPLICIT_ITER_LOOP,
 111     pedantic,
 112     "for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do"
 113 }
 114
 115 declare_clippy_lint! {
 116     /// ### What it does
 117     /// Checks for loops on `y.into_iter()` where `y` will do, and
 118     /// suggests the latter.
 119     ///
 120     /// ### Why is this bad?
 121     /// Readability.
 122     ///
 123     /// ### Example
 124     /// ```rust
 125     /// # let y = vec![1];
 126     /// // with `y` a `Vec` or slice:
 127     /// for x in y.into_iter() {
 128     ///     // ..
 129     /// }
 130     /// ```
 131     /// can be rewritten to
 132     /// ```rust
 133     /// # let y = vec![1];
 134     /// for x in y {
 135     ///     // ..
 136     /// }
 137     /// ```
 138     pub EXPLICIT_INTO_ITER_LOOP,
 139     pedantic,
 140     "for-looping over `_.into_iter()` when `_` would do"
 141 }
 142
 143 declare_clippy_lint! {
 144     /// ### What it does
 145     /// Checks for loops on `x.next()`.
 146     ///
 147     /// ### Why is this bad?
 148     /// `next()` returns either `Some(value)` if there was a
 149     /// value, or `None` otherwise. The insidious thing is that `Option<_>`
 150     /// implements `IntoIterator`, so that possibly one value will be iterated,
 151     /// leading to some hard to find bugs. No one will want to write such code
 152     /// [except to win an Underhanded Rust
 153     /// Contest](https://www.reddit.com/r/rust/comments/3hb0wm/underhanded_rust_contest/cu5yuhr).
 154     ///
 155     /// ### Example
 156     /// ```ignore
 157     /// for x in y.next() {
 158     ///     ..
 159     /// }
 160     /// ```
 161     pub ITER_NEXT_LOOP,
 162     correctness,
 163     "for-looping over `_.next()` which is probably not intended"
 164 }
 165
 166 declare_clippy_lint! {
 167     /// ### What it does
 168     /// Checks for `for` loops over `Option` or `Result` values.
 169     ///
 170     /// ### Why is this bad?
 171     /// Readability. This is more clearly expressed as an `if
 172     /// let`.
 173     ///
 174     /// ### Example
 175     /// ```rust
 176     /// # let opt = Some(1);
 177     ///
 178     /// // Bad
 179     /// for x in opt {
 180     ///     // ..
 181     /// }
 182     ///
 183     /// // Good
 184     /// if let Some(x) = opt {
 185     ///     // ..
 186     /// }
 187     /// ```
 188     ///
 189     /// // or
 190     ///
 191     /// ```rust
 192     /// # let res: Result<i32, std::io::Error> = Ok(1);
 193     ///
 194     /// // Bad
 195     /// for x in &res {
 196     ///     // ..
 197     /// }
 198     ///
 199     /// // Good
 200     /// if let Ok(x) = res {
 201     ///     // ..
 202     /// }
 203     /// ```
 204     pub FOR_LOOPS_OVER_FALLIBLES,
 205     suspicious,
 206     "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`"
 207 }
 208
 209 declare_clippy_lint! {
 210     /// ### What it does
 211     /// Detects `loop + match` combinations that are easier
 212     /// written as a `while let` loop.
 213     ///
 214     /// ### Why is this bad?
 215     /// The `while let` loop is usually shorter and more
 216     /// readable.
 217     ///
 218     /// ### Known problems
 219     /// Sometimes the wrong binding is displayed ([#383](https://github.com/rust-lang/rust-clippy/issues/383)).
 220     ///
 221     /// ### Example
 222     /// ```rust,no_run
 223     /// # let y = Some(1);
 224     /// loop {
 225     ///     let x = match y {
 226     ///         Some(x) => x,
 227     ///         None => break,
 228     ///     };
 229     ///     // .. do something with x
 230     /// }
 231     /// // is easier written as
 232     /// while let Some(x) = y {
 233     ///     // .. do something with x
 234     /// };
 235     /// ```
 236     pub WHILE_LET_LOOP,
 237     complexity,
 238     "`loop { if let { ... } else break }`, which can be written as a `while let` loop"
 239 }
 240
 241 declare_clippy_lint! {
 242     /// ### What it does
 243     /// Checks for functions collecting an iterator when collect
 244     /// is not needed.
 245     ///
 246     /// ### Why is this bad?
 247     /// `collect` causes the allocation of a new data structure,
 248     /// when this allocation may not be needed.
 249     ///
 250     /// ### Example
 251     /// ```rust
 252     /// # let iterator = vec![1].into_iter();
 253     /// let len = iterator.clone().collect::<Vec<_>>().len();
 254     /// // should be
 255     /// let len = iterator.count();
 256     /// ```
 257     pub NEEDLESS_COLLECT,
 258     perf,
 259     "collecting an iterator when collect is not needed"
 260 }
 261
 262 declare_clippy_lint! {
 263     /// ### What it does
 264     /// Checks `for` loops over slices with an explicit counter
 265     /// and suggests the use of `.enumerate()`.
 266     ///
 267     /// ### Why is this bad?
 268     /// Using `.enumerate()` makes the intent more clear,
 269     /// declutters the code and may be faster in some instances.
 270     ///
 271     /// ### Example
 272     /// ```rust
 273     /// # let v = vec![1];
 274     /// # fn bar(bar: usize, baz: usize) {}
 275     /// let mut i = 0;
 276     /// for item in &v {
 277     ///     bar(i, *item);
 278     ///     i += 1;
 279     /// }
 280     /// ```
 281     /// Could be written as
 282     /// ```rust
 283     /// # let v = vec![1];
 284     /// # fn bar(bar: usize, baz: usize) {}
 285     /// for (i, item) in v.iter().enumerate() { bar(i, *item); }
 286     /// ```
 287     pub EXPLICIT_COUNTER_LOOP,
 288     complexity,
 289     "for-looping with an explicit counter when `_.enumerate()` would do"
 290 }
 291
 292 declare_clippy_lint! {
 293     /// ### What it does
 294     /// Checks for empty `loop` expressions.
 295     ///
 296     /// ### Why is this bad?
 297     /// These busy loops burn CPU cycles without doing
 298     /// anything. It is _almost always_ a better idea to `panic!` than to have
 299     /// a busy loop.
 300     ///
 301     /// If panicking isn't possible, think of the environment and either:
 302     ///   - block on something
 303     ///   - sleep the thread for some microseconds
 304     ///   - yield or pause the thread
 305     ///
 306     /// For `std` targets, this can be done with
 307     /// [`std::thread::sleep`](https://doc.rust-lang.org/std/thread/fn.sleep.html)
 308     /// or [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html).
 309     ///
 310     /// For `no_std` targets, doing this is more complicated, especially because
 311     /// `#[panic_handler]`s can't panic. To stop/pause the thread, you will
 312     /// probably need to invoke some target-specific intrinsic. Examples include:
 313     ///   - [`x86_64::instructions::hlt`](https://docs.rs/x86_64/0.12.2/x86_64/instructions/fn.hlt.html)
 314     ///   - [`cortex_m::asm::wfi`](https://docs.rs/cortex-m/0.6.3/cortex_m/asm/fn.wfi.html)
 315     ///
 316     /// ### Example
 317     /// ```no_run
 318     /// loop {}
 319     /// ```
 320     pub EMPTY_LOOP,
 321     suspicious,
 322     "empty `loop {}`, which should block or sleep"
 323 }
 324
 325 declare_clippy_lint! {
 326     /// ### What it does
 327     /// Checks for `while let` expressions on iterators.
 328     ///
 329     /// ### Why is this bad?
 330     /// Readability. A simple `for` loop is shorter and conveys
 331     /// the intent better.
 332     ///
 333     /// ### Example
 334     /// ```ignore
 335     /// while let Some(val) = iter() {
 336     ///     ..
 337     /// }
 338     /// ```
 339     pub WHILE_LET_ON_ITERATOR,
 340     style,
 341     "using a `while let` loop instead of a for loop on an iterator"
 342 }
 343
 344 declare_clippy_lint! {
 345     /// ### What it does
 346     /// Checks for iterating a map (`HashMap` or `BTreeMap`) and
 347     /// ignoring either the keys or values.
 348     ///
 349     /// ### Why is this bad?
 350     /// Readability. There are `keys` and `values` methods that
 351     /// can be used to express that don't need the values or keys.
 352     ///
 353     /// ### Example
 354     /// ```ignore
 355     /// for (k, _) in &map {
 356     ///     ..
 357     /// }
 358     /// ```
 359     ///
 360     /// could be replaced by
 361     ///
 362     /// ```ignore
 363     /// for k in map.keys() {
 364     ///     ..
 365     /// }
 366     /// ```
 367     pub FOR_KV_MAP,
 368     style,
 369     "looping on a map using `iter` when `keys` or `values` would do"
 370 }
 371
 372 declare_clippy_lint! {
 373     /// ### What it does
 374     /// Checks for loops that will always `break`, `return` or
 375     /// `continue` an outer loop.
 376     ///
 377     /// ### Why is this bad?
 378     /// This loop never loops, all it does is obfuscating the
 379     /// code.
 380     ///
 381     /// ### Example
 382     /// ```rust
 383     /// loop {
 384     ///     ..;
 385     ///     break;
 386     /// }
 387     /// ```
 388     pub NEVER_LOOP,
 389     correctness,
 390     "any loop that will always `break` or `return`"
 391 }
 392
 393 declare_clippy_lint! {
 394     /// ### What it does
 395     /// Checks for loops which have a range bound that is a mutable variable
 396     ///
 397     /// ### Why is this bad?
 398     /// One might think that modifying the mutable variable changes the loop bounds
 399     ///
 400     /// ### Example
 401     /// ```rust
 402     /// let mut foo = 42;
 403     /// for i in 0..foo {
 404     ///     foo -= 1;
 405     ///     println!("{}", i); // prints numbers from 0 to 42, not 0 to 21
 406     /// }
 407     /// ```
 408     pub MUT_RANGE_BOUND,
 409     suspicious,
 410     "for loop over a range where one of the bounds is a mutable variable"
 411 }
 412
 413 declare_clippy_lint! {
 414     /// ### What it does
 415     /// Checks whether variables used within while loop condition
 416     /// can be (and are) mutated in the body.
 417     ///
 418     /// ### Why is this bad?
 419     /// If the condition is unchanged, entering the body of the loop
 420     /// will lead to an infinite loop.
 421     ///
 422     /// ### Known problems
 423     /// If the `while`-loop is in a closure, the check for mutation of the
 424     /// condition variables in the body can cause false negatives. For example when only `Upvar` `a` is
 425     /// in the condition and only `Upvar` `b` gets mutated in the body, the lint will not trigger.
 426     ///
 427     /// ### Example
 428     /// ```rust
 429     /// let i = 0;
 430     /// while i > 10 {
 431     ///     println!("let me loop forever!");
 432     /// }
 433     /// ```
 434     pub WHILE_IMMUTABLE_CONDITION,
 435     correctness,
 436     "variables used within while expression are not mutated in the body"
 437 }
 438
 439 declare_clippy_lint! {
 440     /// ### What it does
 441     /// Checks whether a for loop is being used to push a constant
 442     /// value into a Vec.
 443     ///
 444     /// ### Why is this bad?
 445     /// This kind of operation can be expressed more succinctly with
 446     /// `vec![item;SIZE]` or `vec.resize(NEW_SIZE, item)` and using these alternatives may also
 447     /// have better performance.
 448     ///
 449     /// ### Example
 450     /// ```rust
 451     /// let item1 = 2;
 452     /// let item2 = 3;
 453     /// let mut vec: Vec<u8> = Vec::new();
 454     /// for _ in 0..20 {
 455     ///    vec.push(item1);
 456     /// }
 457     /// for _ in 0..30 {
 458     ///     vec.push(item2);
 459     /// }
 460     /// ```
 461     /// could be written as
 462     /// ```rust
 463     /// let item1 = 2;
 464     /// let item2 = 3;
 465     /// let mut vec: Vec<u8> = vec![item1; 20];
 466     /// vec.resize(20 + 30, item2);
 467     /// ```
 468     pub SAME_ITEM_PUSH,
 469     style,
 470     "the same item is pushed inside of a for loop"
 471 }
 472
 473 declare_clippy_lint! {
 474     /// ### What it does
 475     /// Checks whether a for loop has a single element.
 476     ///
 477     /// ### Why is this bad?
 478     /// There is no reason to have a loop of a
 479     /// single element.
 480     ///
 481     /// ### Example
 482     /// ```rust
 483     /// let item1 = 2;
 484     /// for item in &[item1] {
 485     ///     println!("{}", item);
 486     /// }
 487     /// ```
 488     /// could be written as
 489     /// ```rust
 490     /// let item1 = 2;
 491     /// let item = &item1;
 492     /// println!("{}", item);
 493     /// ```
 494     pub SINGLE_ELEMENT_LOOP,
 495     complexity,
 496     "there is no reason to have a single element loop"
 497 }
 498
 499 declare_clippy_lint! {
 500     /// ### What it does
 501     /// Check for unnecessary `if let` usage in a for loop
 502     /// where only the `Some` or `Ok` variant of the iterator element is used.
 503     ///
 504     /// ### Why is this bad?
 505     /// It is verbose and can be simplified
 506     /// by first calling the `flatten` method on the `Iterator`.
 507     ///
 508     /// ### Example
 509     ///
 510     /// ```rust
 511     /// let x = vec![Some(1), Some(2), Some(3)];
 512     /// for n in x {
 513     ///     if let Some(n) = n {
 514     ///         println!("{}", n);
 515     ///     }
 516     /// }
 517     /// ```
 518     /// Use instead:
 519     /// ```rust
 520     /// let x = vec![Some(1), Some(2), Some(3)];
 521     /// for n in x.into_iter().flatten() {
 522     ///     println!("{}", n);
 523     /// }
 524     /// ```
 525     pub MANUAL_FLATTEN,
 526     complexity,
 527     "for loops over `Option`s or `Result`s with a single expression can be simplified"
 528 }
 529
 530 declare_lint_pass!(Loops => [
 531     MANUAL_MEMCPY,
 532     MANUAL_FLATTEN,
 533     NEEDLESS_RANGE_LOOP,
 534     EXPLICIT_ITER_LOOP,
 535     EXPLICIT_INTO_ITER_LOOP,
 536     ITER_NEXT_LOOP,
 537     FOR_LOOPS_OVER_FALLIBLES,
 538     WHILE_LET_LOOP,
 539     NEEDLESS_COLLECT,
 540     EXPLICIT_COUNTER_LOOP,
 541     EMPTY_LOOP,
 542     WHILE_LET_ON_ITERATOR,
 543     FOR_KV_MAP,
 544     NEVER_LOOP,
 545     MUT_RANGE_BOUND,
 546     WHILE_IMMUTABLE_CONDITION,
 547     SAME_ITEM_PUSH,
 548     SINGLE_ELEMENT_LOOP,
 549 ]);
 550
 551 impl<'tcx> LateLintPass<'tcx> for Loops {
 552     #[allow(clippy::too_many_lines)]
 553     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
 554         if let Some((pat, arg, body, span)) = higher::for_loop(expr) {
 555             // we don't want to check expanded macros
 556             // this check is not at the top of the function
 557             // since higher::for_loop expressions are marked as expansions
 558             if body.span.from_expansion() {
 559                 return;
 560             }
 561             check_for_loop(cx, pat, arg, body, expr, span);
 562         }
 563
 564         // we don't want to check expanded macros
 565         if expr.span.from_expansion() {
 566             return;
 567         }
 568
 569         // check for never_loop
 570         never_loop::check(cx, expr);
 571
 572         // check for `loop { if let {} else break }` that could be `while let`
 573         // (also matches an explicit "match" instead of "if let")
 574         // (even if the "match" or "if let" is used for declaration)
 575         if let ExprKind::Loop(block, _, LoopSource::Loop, _) = expr.kind {
 576             // also check for empty `loop {}` statements, skipping those in #[panic_handler]
 577             empty_loop::check(cx, expr, block);
 578             while_let_loop::check(cx, expr, block);
 579         }
 580
 581         while_let_on_iterator::check(cx, expr);
 582
 583         if let Some((cond, body)) = higher::while_loop(expr) {
 584             while_immutable_condition::check(cx, cond, body);
 585         }
 586
 587         needless_collect::check(expr, cx);
 588     }
 589 }
 590
 591 fn check_for_loop<'tcx>(
 592     cx: &LateContext<'tcx>,
 593     pat: &'tcx Pat<'_>,
 594     arg: &'tcx Expr<'_>,
 595     body: &'tcx Expr<'_>,
 596     expr: &'tcx Expr<'_>,
 597     span: Span,
 598 ) {
 599     let is_manual_memcpy_triggered = manual_memcpy::check(cx, pat, arg, body, expr);
 600     if !is_manual_memcpy_triggered {
 601         needless_range_loop::check(cx, pat, arg, body, expr);
 602         explicit_counter_loop::check(cx, pat, arg, body, expr);
 603     }
 604     check_for_loop_arg(cx, pat, arg, expr);
 605     for_kv_map::check(cx, pat, arg, body, expr);
 606     mut_range_bound::check(cx, arg, body);
 607     single_element_loop::check(cx, pat, arg, body, expr);
 608     same_item_push::check(cx, pat, arg, body, expr);
 609     manual_flatten::check(cx, pat, arg, body, span);
 610 }
 611
 612 fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr: &Expr<'_>) {
 613     let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used
 614
 615     if let ExprKind::MethodCall(method, _, [self_arg], _) = arg.kind {
 616         let method_name = &*method.ident.as_str();
 617         // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
 618         match method_name {
 619             "iter" | "iter_mut" => explicit_iter_loop::check(cx, self_arg, arg, method_name),
 620             "into_iter" => {
 621                 explicit_iter_loop::check(cx, self_arg, arg, method_name);
 622                 explicit_into_iter_loop::check(cx, self_arg, arg);
 623             },
 624             "next" => {
 625                 next_loop_linted = iter_next_loop::check(cx, arg, expr);
 626             },
 627             _ => {},
 628         }
 629     }
 630
 631     if !next_loop_linted {
 632         for_loops_over_fallibles::check(cx, pat, arg);
 633     }
 634 }