src/tools/clippy/clippy_lints/src/ranges.rs

   1 use crate::consts::{constant, Constant};
   2 use if_chain::if_chain;
   3 use rustc_ast::ast::RangeLimits;
   4 use rustc_errors::Applicability;
   5 use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
   6 use rustc_lint::{LateContext, LateLintPass};
   7 use rustc_middle::ty;
   8 use rustc_session::{declare_lint_pass, declare_tool_lint};
   9 use rustc_span::source_map::Spanned;
  10 use std::cmp::Ordering;
  11
  12 use crate::utils::sugg::Sugg;
  13 use crate::utils::{get_parent_expr, is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
  14 use crate::utils::{higher, SpanlessEq};
  15
  16 declare_clippy_lint! {
  17     /// **What it does:** Checks for zipping a collection with the range of
  18     /// `0.._.len()`.
  19     ///
  20     /// **Why is this bad?** The code is better expressed with `.enumerate()`.
  21     ///
  22     /// **Known problems:** None.
  23     ///
  24     /// **Example:**
  25     /// ```rust
  26     /// # let x = vec![1];
  27     /// x.iter().zip(0..x.len());
  28     /// ```
  29     /// Could be written as
  30     /// ```rust
  31     /// # let x = vec![1];
  32     /// x.iter().enumerate();
  33     /// ```
  34     pub RANGE_ZIP_WITH_LEN,
  35     complexity,
  36     "zipping iterator with a range when `enumerate()` would do"
  37 }
  38
  39 declare_clippy_lint! {
  40     /// **What it does:** Checks for exclusive ranges where 1 is added to the
  41     /// upper bound, e.g., `x..(y+1)`.
  42     ///
  43     /// **Why is this bad?** The code is more readable with an inclusive range
  44     /// like `x..=y`.
  45     ///
  46     /// **Known problems:** Will add unnecessary pair of parentheses when the
  47     /// expression is not wrapped in a pair but starts with a opening parenthesis
  48     /// and ends with a closing one.
  49     /// I.e., `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..=f())`.
  50     ///
  51     /// Also in many cases, inclusive ranges are still slower to run than
  52     /// exclusive ranges, because they essentially add an extra branch that
  53     /// LLVM may fail to hoist out of the loop.
  54     ///
  55     /// **Example:**
  56     /// ```rust,ignore
  57     /// for x..(y+1) { .. }
  58     /// ```
  59     /// Could be written as
  60     /// ```rust,ignore
  61     /// for x..=y { .. }
  62     /// ```
  63     pub RANGE_PLUS_ONE,
  64     pedantic,
  65     "`x..(y+1)` reads better as `x..=y`"
  66 }
  67
  68 declare_clippy_lint! {
  69     /// **What it does:** Checks for inclusive ranges where 1 is subtracted from
  70     /// the upper bound, e.g., `x..=(y-1)`.
  71     ///
  72     /// **Why is this bad?** The code is more readable with an exclusive range
  73     /// like `x..y`.
  74     ///
  75     /// **Known problems:** None.
  76     ///
  77     /// **Example:**
  78     /// ```rust,ignore
  79     /// for x..=(y-1) { .. }
  80     /// ```
  81     /// Could be written as
  82     /// ```rust,ignore
  83     /// for x..y { .. }
  84     /// ```
  85     pub RANGE_MINUS_ONE,
  86     complexity,
  87     "`x..=(y-1)` reads better as `x..y`"
  88 }
  89
  90 declare_clippy_lint! {
  91     /// **What it does:** Checks for range expressions `x..y` where both `x` and `y`
  92     /// are constant and `x` is greater or equal to `y`.
  93     ///
  94     /// **Why is this bad?** Empty ranges yield no values so iterating them is a no-op.
  95     /// Moreover, trying to use a reversed range to index a slice will panic at run-time.
  96     ///
  97     /// **Known problems:** None.
  98     ///
  99     /// **Example:**
 100     ///
 101     /// ```rust,no_run
 102     /// fn main() {
 103     ///     (10..=0).for_each(|x| println!("{}", x));
 104     ///
 105     ///     let arr = [1, 2, 3, 4, 5];
 106     ///     let sub = &arr[3..1];
 107     /// }
 108     /// ```
 109     /// Use instead:
 110     /// ```rust
 111     /// fn main() {
 112     ///     (0..=10).rev().for_each(|x| println!("{}", x));
 113     ///
 114     ///     let arr = [1, 2, 3, 4, 5];
 115     ///     let sub = &arr[1..3];
 116     /// }
 117     /// ```
 118     pub REVERSED_EMPTY_RANGES,
 119     correctness,
 120     "reversing the limits of range expressions, resulting in empty ranges"
 121 }
 122
 123 declare_lint_pass!(Ranges => [
 124     RANGE_ZIP_WITH_LEN,
 125     RANGE_PLUS_ONE,
 126     RANGE_MINUS_ONE,
 127     REVERSED_EMPTY_RANGES,
 128 ]);
 129
 130 impl<'tcx> LateLintPass<'tcx> for Ranges {
 131     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
 132         if let ExprKind::MethodCall(ref path, _, ref args, _) = expr.kind {
 133             let name = path.ident.as_str();
 134             if name == "zip" && args.len() == 2 {
 135                 let iter = &args[0].kind;
 136                 let zip_arg = &args[1];
 137                 if_chain! {
 138                     // `.iter()` call
 139                     if let ExprKind::MethodCall(ref iter_path, _, ref iter_args , _) = *iter;
 140                     if iter_path.ident.name == sym!(iter);
 141                     // range expression in `.zip()` call: `0..x.len()`
 142                     if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(cx, zip_arg);
 143                     if is_integer_const(cx, start, 0);
 144                     // `.len()` call
 145                     if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
 146                     if len_path.ident.name == sym!(len) && len_args.len() == 1;
 147                     // `.iter()` and `.len()` called on same `Path`
 148                     if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
 149                     if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
 150                     if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
 151                      then {
 152                          span_lint(cx,
 153                                    RANGE_ZIP_WITH_LEN,
 154                                    expr.span,
 155                                    &format!("It is more idiomatic to use `{}.iter().enumerate()`",
 156                                             snippet(cx, iter_args[0].span, "_")));
 157                     }
 158                 }
 159             }
 160         }
 161
 162         check_exclusive_range_plus_one(cx, expr);
 163         check_inclusive_range_minus_one(cx, expr);
 164         check_reversed_empty_range(cx, expr);
 165     }
 166 }
 167
 168 // exclusive range plus one: `x..(y+1)`
 169 fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
 170     if_chain! {
 171         if let Some(higher::Range {
 172             start,
 173             end: Some(end),
 174             limits: RangeLimits::HalfOpen
 175         }) = higher::range(cx, expr);
 176         if let Some(y) = y_plus_one(cx, end);
 177         then {
 178             let span = if expr.span.from_expansion() {
 179                 expr.span
 180                     .ctxt()
 181                     .outer_expn_data()
 182                     .call_site
 183             } else {
 184                 expr.span
 185             };
 186             span_lint_and_then(
 187                 cx,
 188                 RANGE_PLUS_ONE,
 189                 span,
 190                 "an inclusive range would be more readable",
 191                 |diag| {
 192                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 193                     let end = Sugg::hir(cx, y, "y");
 194                     if let Some(is_wrapped) = &snippet_opt(cx, span) {
 195                         if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
 196                             diag.span_suggestion(
 197                                 span,
 198                                 "use",
 199                                 format!("({}..={})", start, end),
 200                                 Applicability::MaybeIncorrect,
 201                             );
 202                         } else {
 203                             diag.span_suggestion(
 204                                 span,
 205                                 "use",
 206                                 format!("{}..={}", start, end),
 207                                 Applicability::MachineApplicable, // snippet
 208                             );
 209                         }
 210                     }
 211                 },
 212             );
 213         }
 214     }
 215 }
 216
 217 // inclusive range minus one: `x..=(y-1)`
 218 fn check_inclusive_range_minus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
 219     if_chain! {
 220         if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr);
 221         if let Some(y) = y_minus_one(cx, end);
 222         then {
 223             span_lint_and_then(
 224                 cx,
 225                 RANGE_MINUS_ONE,
 226                 expr.span,
 227                 "an exclusive range would be more readable",
 228                 |diag| {
 229                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 230                     let end = Sugg::hir(cx, y, "y");
 231                     diag.span_suggestion(
 232                         expr.span,
 233                         "use",
 234                         format!("{}..{}", start, end),
 235                         Applicability::MachineApplicable, // snippet
 236                     );
 237                 },
 238             );
 239         }
 240     }
 241 }
 242
 243 fn check_reversed_empty_range(cx: &LateContext<'_>, expr: &Expr<'_>) {
 244     fn inside_indexing_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 245         matches!(
 246             get_parent_expr(cx, expr),
 247             Some(Expr {
 248                 kind: ExprKind::Index(..),
 249                 ..
 250             })
 251         )
 252     }
 253
 254     fn is_for_loop_arg(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 255         let mut cur_expr = expr;
 256         while let Some(parent_expr) = get_parent_expr(cx, cur_expr) {
 257             match higher::for_loop(parent_expr) {
 258                 Some((_, args, _)) if args.hir_id == expr.hir_id => return true,
 259                 _ => cur_expr = parent_expr,
 260             }
 261         }
 262
 263         false
 264     }
 265
 266     fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
 267         match limits {
 268             RangeLimits::HalfOpen => ordering != Ordering::Less,
 269             RangeLimits::Closed => ordering == Ordering::Greater,
 270         }
 271     }
 272
 273     if_chain! {
 274         if let Some(higher::Range { start: Some(start), end: Some(end), limits }) = higher::range(cx, expr);
 275         let ty = cx.tables().expr_ty(start);
 276         if let ty::Int(_) | ty::Uint(_) = ty.kind;
 277         if let Some((start_idx, _)) = constant(cx, cx.tables(), start);
 278         if let Some((end_idx, _)) = constant(cx, cx.tables(), end);
 279         if let Some(ordering) = Constant::partial_cmp(cx.tcx, ty, &start_idx, &end_idx);
 280         if is_empty_range(limits, ordering);
 281         then {
 282             if inside_indexing_expr(cx, expr) {
 283                 // Avoid linting `N..N` as it has proven to be useful, see #5689 and #5628 ...
 284                 if ordering != Ordering::Equal {
 285                     span_lint(
 286                         cx,
 287                         REVERSED_EMPTY_RANGES,
 288                         expr.span,
 289                         "this range is reversed and using it to index a slice will panic at run-time",
 290                     );
 291                 }
 292             // ... except in for loop arguments for backwards compatibility with `reverse_range_loop`
 293             } else if ordering != Ordering::Equal || is_for_loop_arg(cx, expr) {
 294                 span_lint_and_then(
 295                     cx,
 296                     REVERSED_EMPTY_RANGES,
 297                     expr.span,
 298                     "this range is empty so it will yield no values",
 299                     |diag| {
 300                         if ordering != Ordering::Equal {
 301                             let start_snippet = snippet(cx, start.span, "_");
 302                             let end_snippet = snippet(cx, end.span, "_");
 303                             let dots = match limits {
 304                                 RangeLimits::HalfOpen => "..",
 305                                 RangeLimits::Closed => "..="
 306                             };
 307
 308                             diag.span_suggestion(
 309                                 expr.span,
 310                                 "consider using the following if you are attempting to iterate over this \
 311                                  range in reverse",
 312                                 format!("({}{}{}).rev()", end_snippet, dots, start_snippet),
 313                                 Applicability::MaybeIncorrect,
 314                             );
 315                         }
 316                     },
 317                 );
 318             }
 319         }
 320     }
 321 }
 322
 323 fn y_plus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 324     match expr.kind {
 325         ExprKind::Binary(
 326             Spanned {
 327                 node: BinOpKind::Add, ..
 328             },
 329             ref lhs,
 330             ref rhs,
 331         ) => {
 332             if is_integer_const(cx, lhs, 1) {
 333                 Some(rhs)
 334             } else if is_integer_const(cx, rhs, 1) {
 335                 Some(lhs)
 336             } else {
 337                 None
 338             }
 339         },
 340         _ => None,
 341     }
 342 }
 343
 344 fn y_minus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 345     match expr.kind {
 346         ExprKind::Binary(
 347             Spanned {
 348                 node: BinOpKind::Sub, ..
 349             },
 350             ref lhs,
 351             ref rhs,
 352         ) if is_integer_const(cx, rhs, 1) => Some(lhs),
 353         _ => None,
 354     }
 355 }