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