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<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
 131     fn check_expr(&mut self, cx: &LateContext<'a, '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<'a>(cx: &'a LateContext<'_, '_>, expr: &Expr<'_>) -> Option<&'a Expr<'a>> {
 245         match get_parent_expr(cx, expr) {
 246             parent_expr @ Some(Expr {
 247                 kind: ExprKind::Index(..),
 248                 ..
 249             }) => parent_expr,
 250             _ => None,
 251         }
 252     }
 253
 254     fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
 255         match limits {
 256             RangeLimits::HalfOpen => ordering != Ordering::Less,
 257             RangeLimits::Closed => ordering == Ordering::Greater,
 258         }
 259     }
 260
 261     if_chain! {
 262         if let Some(higher::Range { start: Some(start), end: Some(end), limits }) = higher::range(cx, expr);
 263         let ty = cx.tables.expr_ty(start);
 264         if let ty::Int(_) | ty::Uint(_) = ty.kind;
 265         if let Some((start_idx, _)) = constant(cx, cx.tables, start);
 266         if let Some((end_idx, _)) = constant(cx, cx.tables, end);
 267         if let Some(ordering) = Constant::partial_cmp(cx.tcx, ty, &start_idx, &end_idx);
 268         if is_empty_range(limits, ordering);
 269         then {
 270             if let Some(parent_expr) = inside_indexing_expr(cx, expr) {
 271                 let (reason, outcome) = if ordering == Ordering::Equal {
 272                     ("empty", "always yield an empty slice")
 273                 } else {
 274                     ("reversed", "panic at run-time")
 275                 };
 276
 277                 span_lint_and_then(
 278                     cx,
 279                     REVERSED_EMPTY_RANGES,
 280                     expr.span,
 281                     &format!("this range is {} and using it to index a slice will {}", reason, outcome),
 282                     |diag| {
 283                         if_chain! {
 284                             if ordering == Ordering::Equal;
 285                             if let ty::Slice(slice_ty) = cx.tables.expr_ty(parent_expr).kind;
 286                             then {
 287                                 diag.span_suggestion(
 288                                     parent_expr.span,
 289                                     "if you want an empty slice, use",
 290                                     format!("[] as &[{}]", slice_ty),
 291                                     Applicability::MaybeIncorrect
 292                                 );
 293                             }
 294                         }
 295                     }
 296                 );
 297             } else {
 298                 span_lint_and_then(
 299                     cx,
 300                     REVERSED_EMPTY_RANGES,
 301                     expr.span,
 302                     "this range is empty so it will yield no values",
 303                     |diag| {
 304                         if ordering != Ordering::Equal {
 305                             let start_snippet = snippet(cx, start.span, "_");
 306                             let end_snippet = snippet(cx, end.span, "_");
 307                             let dots = match limits {
 308                                 RangeLimits::HalfOpen => "..",
 309                                 RangeLimits::Closed => "..="
 310                             };
 311
 312                             diag.span_suggestion(
 313                                 expr.span,
 314                                 "consider using the following if you are attempting to iterate over this \
 315                                  range in reverse",
 316                                 format!("({}{}{}).rev()", end_snippet, dots, start_snippet),
 317                                 Applicability::MaybeIncorrect,
 318                             );
 319                         }
 320                     },
 321                 );
 322             }
 323         }
 324     }
 325 }
 326
 327 fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 328     match expr.kind {
 329         ExprKind::Binary(
 330             Spanned {
 331                 node: BinOpKind::Add, ..
 332             },
 333             ref lhs,
 334             ref rhs,
 335         ) => {
 336             if is_integer_const(cx, lhs, 1) {
 337                 Some(rhs)
 338             } else if is_integer_const(cx, rhs, 1) {
 339                 Some(lhs)
 340             } else {
 341                 None
 342             }
 343         },
 344         _ => None,
 345     }
 346 }
 347
 348 fn y_minus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 349     match expr.kind {
 350         ExprKind::Binary(
 351             Spanned {
 352                 node: BinOpKind::Sub, ..
 353             },
 354             ref lhs,
 355             ref rhs,
 356         ) if is_integer_const(cx, rhs, 1) => Some(lhs),
 357         _ => None,
 358     }
 359 }