clippy_lints/src/ranges.rs

   1 use if_chain::if_chain;
   2 use rustc::declare_lint_pass;
   3 use rustc::hir::*;
   4 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
   5 use rustc_errors::Applicability;
   6 use rustc_session::declare_tool_lint;
   7 use syntax::ast::RangeLimits;
   8 use syntax::source_map::Spanned;
   9
  10 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 }
  33
  34 declare_clippy_lint! {
  35     /// **What it does:** Checks for zipping a collection with the range of
  36     /// `0.._.len()`.
  37     ///
  38     /// **Why is this bad?** The code is better expressed with `.enumerate()`.
  39     ///
  40     /// **Known problems:** None.
  41     ///
  42     /// **Example:**
  43     /// ```rust
  44     /// # let x = vec![1];
  45     /// x.iter().zip(0..x.len());
  46     /// ```
  47     /// Could be written as
  48     /// ```rust
  49     /// # let x = vec![1];
  50     /// x.iter().enumerate();
  51     /// ```
  52     pub RANGE_ZIP_WITH_LEN,
  53     complexity,
  54     "zipping iterator with a range when `enumerate()` would do"
  55 }
  56
  57 declare_clippy_lint! {
  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,ignore
  71     /// for x..(y+1) { .. }
  72     /// ```
  73     /// Could be written as
  74     /// ```rust,ignore
  75     /// for x..=y { .. }
  76     /// ```
  77     pub RANGE_PLUS_ONE,
  78     complexity,
  79     "`x..(y+1)` reads better as `x..=y`"
  80 }
  81
  82 declare_clippy_lint! {
  83     /// **What it does:** Checks for inclusive ranges where 1 is subtracted from
  84     /// the upper bound, e.g., `x..=(y-1)`.
  85     ///
  86     /// **Why is this bad?** The code is more readable with an exclusive range
  87     /// like `x..y`.
  88     ///
  89     /// **Known problems:** None.
  90     ///
  91     /// **Example:**
  92     /// ```rust,ignore
  93     /// for x..=(y-1) { .. }
  94     /// ```
  95     /// Could be written as
  96     /// ```rust,ignore
  97     /// for x..y { .. }
  98     /// ```
  99     pub RANGE_MINUS_ONE,
 100     complexity,
 101     "`x..=(y-1)` reads better as `x..y`"
 102 }
 103
 104 declare_lint_pass!(Ranges => [
 105     ITERATOR_STEP_BY_ZERO,
 106     RANGE_ZIP_WITH_LEN,
 107     RANGE_PLUS_ONE,
 108     RANGE_MINUS_ONE
 109 ]);
 110
 111 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
 112     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
 113         if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind {
 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].kind;
 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 == sym!(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_const(cx, start, 0);
 137                     // `.len()` call
 138                     if let ExprKind::MethodCall(ref len_path, _, ref len_args) = end.kind;
 139                     if len_path.ident.name == sym!(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].kind;
 142                     if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
 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         check_exclusive_range_plus_one(cx, expr);
 156         check_inclusive_range_minus_one(cx, expr);
 157     }
 158 }
 159
 160 // exclusive range plus one: `x..(y+1)`
 161 fn check_exclusive_range_plus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
 162     if_chain! {
 163         if let Some(higher::Range {
 164             start,
 165             end: Some(end),
 166             limits: RangeLimits::HalfOpen
 167         }) = higher::range(cx, expr);
 168         if let Some(y) = y_plus_one(cx, end);
 169         then {
 170             let span = if expr.span.from_expansion() {
 171                 expr.span
 172                     .ctxt()
 173                     .outer_expn_data()
 174                     .call_site
 175             } else {
 176                 expr.span
 177             };
 178             span_lint_and_then(
 179                 cx,
 180                 RANGE_PLUS_ONE,
 181                 span,
 182                 "an inclusive range would be more readable",
 183                 |db| {
 184                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 185                     let end = Sugg::hir(cx, y, "y");
 186                     if let Some(is_wrapped) = &snippet_opt(cx, span) {
 187                         if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
 188                             db.span_suggestion(
 189                                 span,
 190                                 "use",
 191                                 format!("({}..={})", start, end),
 192                                 Applicability::MaybeIncorrect,
 193                             );
 194                         } else {
 195                             db.span_suggestion(
 196                                 span,
 197                                 "use",
 198                                 format!("{}..={}", start, end),
 199                                 Applicability::MachineApplicable, // snippet
 200                             );
 201                         }
 202                     }
 203                 },
 204             );
 205         }
 206     }
 207 }
 208
 209 // inclusive range minus one: `x..=(y-1)`
 210 fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) {
 211     if_chain! {
 212         if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr);
 213         if let Some(y) = y_minus_one(cx, end);
 214         then {
 215             span_lint_and_then(
 216                 cx,
 217                 RANGE_MINUS_ONE,
 218                 expr.span,
 219                 "an exclusive range would be more readable",
 220                 |db| {
 221                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 222                     let end = Sugg::hir(cx, y, "y");
 223                     db.span_suggestion(
 224                         expr.span,
 225                         "use",
 226                         format!("{}..{}", start, end),
 227                         Applicability::MachineApplicable, // snippet
 228                     );
 229                 },
 230             );
 231         }
 232     }
 233 }
 234
 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
 243 fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> {
 244     match expr.kind {
 245         ExprKind::Binary(
 246             Spanned {
 247                 node: BinOpKind::Add, ..
 248             },
 249             ref lhs,
 250             ref rhs,
 251         ) => {
 252             if is_integer_const(cx, lhs, 1) {
 253                 Some(rhs)
 254             } else if is_integer_const(cx, rhs, 1) {
 255                 Some(lhs)
 256             } else {
 257                 None
 258             }
 259         },
 260         _ => None,
 261     }
 262 }
 263
 264 fn y_minus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> {
 265     match expr.kind {
 266         ExprKind::Binary(
 267             Spanned {
 268                 node: BinOpKind::Sub, ..
 269             },
 270             ref lhs,
 271             ref rhs,
 272         ) if is_integer_const(cx, rhs, 1) => Some(lhs),
 273         _ => None,
 274     }
 275 }