clippy_lints/src/ranges.rs

   1 // Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
   2 // file at the top-level directory of this distribution.
   3 //
   4 // Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
   5 // http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
   6 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
   7 // option. This file may not be copied, modified, or distributed
   8 // except according to those terms.
   9
  10 use crate::rustc::hir::*;
  11 use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
  12 use crate::rustc::{declare_tool_lint, lint_array};
  13 use crate::rustc_errors::Applicability;
  14 use crate::syntax::ast::RangeLimits;
  15 use crate::syntax::source_map::Spanned;
  16 use crate::utils::sugg::Sugg;
  17 use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq};
  18 use crate::utils::{is_integer_literal, paths, snippet, snippet_opt, span_lint, span_lint_and_then};
  19 use if_chain::if_chain;
  20
  21 /// **What it does:** Checks for calling `.step_by(0)` on iterators,
  22 /// which never terminates.
  23 ///
  24 /// **Why is this bad?** This very much looks like an oversight, since with
  25 /// `loop { .. }` there is an obvious better way to endlessly loop.
  26 ///
  27 /// **Known problems:** None.
  28 ///
  29 /// **Example:**
  30 /// ```rust
  31 /// for x in (5..5).step_by(0) {
  32 ///     ..
  33 /// }
  34 /// ```
  35 declare_clippy_lint! {
  36     pub ITERATOR_STEP_BY_ZERO,
  37     correctness,
  38     "using `Iterator::step_by(0)`, which produces an infinite iterator"
  39 }
  40
  41 /// **What it does:** Checks for zipping a collection with the range of
  42 /// `0.._.len()`.
  43 ///
  44 /// **Why is this bad?** The code is better expressed with `.enumerate()`.
  45 ///
  46 /// **Known problems:** None.
  47 ///
  48 /// **Example:**
  49 /// ```rust
  50 /// x.iter().zip(0..x.len())
  51 /// ```
  52 declare_clippy_lint! {
  53     pub RANGE_ZIP_WITH_LEN,
  54     complexity,
  55     "zipping iterator with a range when `enumerate()` would do"
  56 }
  57
  58 /// **What it does:** Checks for exclusive ranges where 1 is added to the
  59 /// upper bound, e.g. `x..(y+1)`.
  60 ///
  61 /// **Why is this bad?** The code is more readable with an inclusive range
  62 /// like `x..=y`.
  63 ///
  64 /// **Known problems:** Will add unnecessary pair of parentheses when the
  65 /// expression is not wrapped in a pair but starts with a opening parenthesis
  66 /// and ends with a closing one.
  67 /// I.e: `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..=f())`.
  68 ///
  69 /// **Example:**
  70 /// ```rust
  71 /// for x..(y+1) { .. }
  72 /// ```
  73 declare_clippy_lint! {
  74     pub RANGE_PLUS_ONE,
  75     complexity,
  76     "`x..(y+1)` reads better as `x..=y`"
  77 }
  78
  79 /// **What it does:** Checks for inclusive ranges where 1 is subtracted from
  80 /// the upper bound, e.g. `x..=(y-1)`.
  81 ///
  82 /// **Why is this bad?** The code is more readable with an exclusive range
  83 /// like `x..y`.
  84 ///
  85 /// **Known problems:** None.
  86 ///
  87 /// **Example:**
  88 /// ```rust
  89 /// for x..=(y-1) { .. }
  90 /// ```
  91 declare_clippy_lint! {
  92     pub RANGE_MINUS_ONE,
  93     complexity,
  94     "`x..=(y-1)` reads better as `x..y`"
  95 }
  96
  97 #[derive(Copy, Clone)]
  98 pub struct Pass;
  99
 100 impl LintPass for Pass {
 101     fn get_lints(&self) -> LintArray {
 102         lint_array!(
 103             ITERATOR_STEP_BY_ZERO,
 104             RANGE_ZIP_WITH_LEN,
 105             RANGE_PLUS_ONE,
 106             RANGE_MINUS_ONE
 107         )
 108     }
 109 }
 110
 111 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
 112     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
 113         if let ExprKind::MethodCall(ref path, _, ref args) = expr.node {
 114             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 {
 128                 let iter = &args[0].node;
 129                 let zip_arg = &args[1];
 130                 if_chain! {
 131                     // .iter() call
 132                     if let ExprKind::MethodCall(ref iter_path, _, ref iter_args ) = *iter;
 133                     if iter_path.ident.name == "iter";
 134                     // range expression in .zip() call: 0..x.len()
 135                     if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(cx, zip_arg);
 136                     if is_integer_literal(start, 0);
 137                     // .len() call
 138                     if let ExprKind::MethodCall(ref len_path, _, ref len_args) = end.node;
 139                     if len_path.ident.name == "len" && len_args.len() == 1;
 140                     // .iter() and .len() called on same Path
 141                     if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].node;
 142                     if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].node;
 143                     if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
 144                      then {
 145                          span_lint(cx,
 146                                    RANGE_ZIP_WITH_LEN,
 147                                    expr.span,
 148                                    &format!("It is more idiomatic to use {}.iter().enumerate()",
 149                                             snippet(cx, iter_args[0].span, "_")));
 150                     }
 151                 }
 152             }
 153         }
 154
 155         // exclusive range plus one: x..(y+1)
 156         if_chain! {
 157             if let Some(higher::Range {
 158                 start,
 159                 end: Some(end),
 160                 limits: RangeLimits::HalfOpen
 161             }) = higher::range(cx, expr);
 162             if let Some(y) = y_plus_one(end);
 163             then {
 164                 span_lint_and_then(
 165                     cx,
 166                     RANGE_PLUS_ONE,
 167                     expr.span,
 168                     "an inclusive range would be more readable",
 169                     |db| {
 170                         let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 171                         let end = Sugg::hir(cx, y, "y");
 172                         if let Some(is_wrapped) = &snippet_opt(cx, expr.span) {
 173                             if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
 174                                 db.span_suggestion_with_applicability(
 175                                     expr.span,
 176                                     "use",
 177                                     format!("({}..={})", start, end),
 178                                     Applicability::MaybeIncorrect,
 179                                 );
 180                             } else {
 181                                 db.span_suggestion_with_applicability(
 182                                     expr.span,
 183                                     "use",
 184                                     format!("{}..={}", start, end),
 185                                     Applicability::MachineApplicable, // snippet
 186                                 );
 187                             }
 188                         }
 189                     },
 190                 );
 191             }
 192         }
 193
 194         // inclusive range minus one: x..=(y-1)
 195         if_chain! {
 196             if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr);
 197             if let Some(y) = y_minus_one(end);
 198             then {
 199                 span_lint_and_then(
 200                     cx,
 201                     RANGE_MINUS_ONE,
 202                     expr.span,
 203                     "an exclusive range would be more readable",
 204                     |db| {
 205                         let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 206                         let end = Sugg::hir(cx, y, "y");
 207                         db.span_suggestion_with_applicability(
 208                             expr.span,
 209                             "use",
 210                             format!("{}..{}", start, end),
 211                             Applicability::MachineApplicable, // snippet
 212                         );
 213                     },
 214                 );
 215             }
 216         }
 217     }
 218 }
 219
 220 fn has_step_by(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
 221     // No need for walk_ptrs_ty here because step_by moves self, so it
 222     // can't be called on a borrowed range.
 223     let ty = cx.tables.expr_ty_adjusted(expr);
 224
 225     get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[]))
 226 }
 227
 228 fn y_plus_one(expr: &Expr) -> Option<&Expr> {
 229     match expr.node {
 230         ExprKind::Binary(
 231             Spanned {
 232                 node: BinOpKind::Add, ..
 233             },
 234             ref lhs,
 235             ref rhs,
 236         ) => {
 237             if is_integer_literal(lhs, 1) {
 238                 Some(rhs)
 239             } else if is_integer_literal(rhs, 1) {
 240                 Some(lhs)
 241             } else {
 242                 None
 243             }
 244         },
 245         _ => None,
 246     }
 247 }
 248
 249 fn y_minus_one(expr: &Expr) -> Option<&Expr> {
 250     match expr.node {
 251         ExprKind::Binary(
 252             Spanned {
 253                 node: BinOpKind::Sub, ..
 254             },
 255             ref lhs,
 256             ref rhs,
 257         ) if is_integer_literal(rhs, 1) => Some(lhs),
 258         _ => None,
 259     }
 260 }