clippy_lints/src/ranges.rs

   1 use crate::consts::{constant, Constant};
   2 use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then};
   3 use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
   4 use if_chain::if_chain;
   5 use rustc_ast::ast::RangeLimits;
   6 use rustc_errors::Applicability;
   7 use rustc_hir::{BinOpKind, Expr, ExprKind, PathSegment, QPath};
   8 use rustc_lint::{LateContext, LateLintPass, LintContext};
   9 use rustc_middle::ty;
  10 use rustc_semver::RustcVersion;
  11 use rustc_session::{declare_tool_lint, impl_lint_pass};
  12 use rustc_span::source_map::{Span, Spanned};
  13 use rustc_span::sym;
  14 use rustc_span::symbol::Ident;
  15 use std::cmp::Ordering;
  16
  17 use crate::utils::sugg::Sugg;
  18 use crate::utils::{get_parent_expr, in_constant, is_integer_const, meets_msrv, single_segment_path};
  19 use crate::utils::{higher, SpanlessEq};
  20
  21 declare_clippy_lint! {
  22     /// **What it does:** Checks for zipping a collection with the range of
  23     /// `0.._.len()`.
  24     ///
  25     /// **Why is this bad?** The code is better expressed with `.enumerate()`.
  26     ///
  27     /// **Known problems:** None.
  28     ///
  29     /// **Example:**
  30     /// ```rust
  31     /// # let x = vec![1];
  32     /// x.iter().zip(0..x.len());
  33     /// ```
  34     /// Could be written as
  35     /// ```rust
  36     /// # let x = vec![1];
  37     /// x.iter().enumerate();
  38     /// ```
  39     pub RANGE_ZIP_WITH_LEN,
  40     complexity,
  41     "zipping iterator with a range when `enumerate()` would do"
  42 }
  43
  44 declare_clippy_lint! {
  45     /// **What it does:** Checks for exclusive ranges where 1 is added to the
  46     /// upper bound, e.g., `x..(y+1)`.
  47     ///
  48     /// **Why is this bad?** The code is more readable with an inclusive range
  49     /// like `x..=y`.
  50     ///
  51     /// **Known problems:** Will add unnecessary pair of parentheses when the
  52     /// expression is not wrapped in a pair but starts with a opening parenthesis
  53     /// and ends with a closing one.
  54     /// I.e., `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..=f())`.
  55     ///
  56     /// Also in many cases, inclusive ranges are still slower to run than
  57     /// exclusive ranges, because they essentially add an extra branch that
  58     /// LLVM may fail to hoist out of the loop.
  59     ///
  60     /// This will cause a warning that cannot be fixed if the consumer of the
  61     /// range only accepts a specific range type, instead of the generic
  62     /// `RangeBounds` trait
  63     /// ([#3307](https://github.com/rust-lang/rust-clippy/issues/3307)).
  64     ///
  65     /// **Example:**
  66     /// ```rust,ignore
  67     /// for x..(y+1) { .. }
  68     /// ```
  69     /// Could be written as
  70     /// ```rust,ignore
  71     /// for x..=y { .. }
  72     /// ```
  73     pub RANGE_PLUS_ONE,
  74     pedantic,
  75     "`x..(y+1)` reads better as `x..=y`"
  76 }
  77
  78 declare_clippy_lint! {
  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:** This will cause a warning that cannot be fixed if
  86     /// the consumer of the range only accepts a specific range type, instead of
  87     /// the generic `RangeBounds` trait
  88     /// ([#3307](https://github.com/rust-lang/rust-clippy/issues/3307)).
  89     ///
  90     /// **Example:**
  91     /// ```rust,ignore
  92     /// for x..=(y-1) { .. }
  93     /// ```
  94     /// Could be written as
  95     /// ```rust,ignore
  96     /// for x..y { .. }
  97     /// ```
  98     pub RANGE_MINUS_ONE,
  99     pedantic,
 100     "`x..=(y-1)` reads better as `x..y`"
 101 }
 102
 103 declare_clippy_lint! {
 104     /// **What it does:** Checks for range expressions `x..y` where both `x` and `y`
 105     /// are constant and `x` is greater or equal to `y`.
 106     ///
 107     /// **Why is this bad?** Empty ranges yield no values so iterating them is a no-op.
 108     /// Moreover, trying to use a reversed range to index a slice will panic at run-time.
 109     ///
 110     /// **Known problems:** None.
 111     ///
 112     /// **Example:**
 113     ///
 114     /// ```rust,no_run
 115     /// fn main() {
 116     ///     (10..=0).for_each(|x| println!("{}", x));
 117     ///
 118     ///     let arr = [1, 2, 3, 4, 5];
 119     ///     let sub = &arr[3..1];
 120     /// }
 121     /// ```
 122     /// Use instead:
 123     /// ```rust
 124     /// fn main() {
 125     ///     (0..=10).rev().for_each(|x| println!("{}", x));
 126     ///
 127     ///     let arr = [1, 2, 3, 4, 5];
 128     ///     let sub = &arr[1..3];
 129     /// }
 130     /// ```
 131     pub REVERSED_EMPTY_RANGES,
 132     correctness,
 133     "reversing the limits of range expressions, resulting in empty ranges"
 134 }
 135
 136 declare_clippy_lint! {
 137     /// **What it does:** Checks for expressions like `x >= 3 && x < 8` that could
 138     /// be more readably expressed as `(3..8).contains(x)`.
 139     ///
 140     /// **Why is this bad?** `contains` expresses the intent better and has less
 141     /// failure modes (such as fencepost errors or using `||` instead of `&&`).
 142     ///
 143     /// **Known problems:** None.
 144     ///
 145     /// **Example:**
 146     ///
 147     /// ```rust
 148     /// // given
 149     /// let x = 6;
 150     ///
 151     /// assert!(x >= 3 && x < 8);
 152     /// ```
 153     /// Use instead:
 154     /// ```rust
 155     ///# let x = 6;
 156     /// assert!((3..8).contains(&x));
 157     /// ```
 158     pub MANUAL_RANGE_CONTAINS,
 159     style,
 160     "manually reimplementing {`Range`, `RangeInclusive`}`::contains`"
 161 }
 162
 163 const MANUAL_RANGE_CONTAINS_MSRV: RustcVersion = RustcVersion::new(1, 35, 0);
 164
 165 pub struct Ranges {
 166     msrv: Option<RustcVersion>,
 167 }
 168
 169 impl Ranges {
 170     #[must_use]
 171     pub fn new(msrv: Option<RustcVersion>) -> Self {
 172         Self { msrv }
 173     }
 174 }
 175
 176 impl_lint_pass!(Ranges => [
 177     RANGE_ZIP_WITH_LEN,
 178     RANGE_PLUS_ONE,
 179     RANGE_MINUS_ONE,
 180     REVERSED_EMPTY_RANGES,
 181     MANUAL_RANGE_CONTAINS,
 182 ]);
 183
 184 impl<'tcx> LateLintPass<'tcx> for Ranges {
 185     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
 186         match expr.kind {
 187             ExprKind::MethodCall(ref path, _, ref args, _) => {
 188                 check_range_zip_with_len(cx, path, args, expr.span);
 189             },
 190             ExprKind::Binary(ref op, ref l, ref r) => {
 191                 if meets_msrv(self.msrv.as_ref(), &MANUAL_RANGE_CONTAINS_MSRV) {
 192                     check_possible_range_contains(cx, op.node, l, r, expr);
 193                 }
 194             },
 195             _ => {},
 196         }
 197
 198         check_exclusive_range_plus_one(cx, expr);
 199         check_inclusive_range_minus_one(cx, expr);
 200         check_reversed_empty_range(cx, expr);
 201     }
 202     extract_msrv_attr!(LateContext);
 203 }
 204
 205 fn check_possible_range_contains(cx: &LateContext<'_>, op: BinOpKind, l: &Expr<'_>, r: &Expr<'_>, expr: &Expr<'_>) {
 206     if in_constant(cx, expr.hir_id) {
 207         return;
 208     }
 209
 210     let span = expr.span;
 211     let combine_and = match op {
 212         BinOpKind::And | BinOpKind::BitAnd => true,
 213         BinOpKind::Or | BinOpKind::BitOr => false,
 214         _ => return,
 215     };
 216     // value, name, order (higher/lower), inclusiveness
 217     if let (Some((lval, lname, name_span, lval_span, lord, linc)), Some((rval, rname, _, rval_span, rord, rinc))) =
 218         (check_range_bounds(cx, l), check_range_bounds(cx, r))
 219     {
 220         // we only lint comparisons on the same name and with different
 221         // direction
 222         if lname != rname || lord == rord {
 223             return;
 224         }
 225         let ord = Constant::partial_cmp(cx.tcx, cx.typeck_results().expr_ty(l), &lval, &rval);
 226         if combine_and && ord == Some(rord) {
 227             // order lower bound and upper bound
 228             let (l_span, u_span, l_inc, u_inc) = if rord == Ordering::Less {
 229                 (lval_span, rval_span, linc, rinc)
 230             } else {
 231                 (rval_span, lval_span, rinc, linc)
 232             };
 233             // we only lint inclusive lower bounds
 234             if !l_inc {
 235                 return;
 236             }
 237             let (range_type, range_op) = if u_inc {
 238                 ("RangeInclusive", "..=")
 239             } else {
 240                 ("Range", "..")
 241             };
 242             let mut applicability = Applicability::MachineApplicable;
 243             let name = snippet_with_applicability(cx, name_span, "_", &mut applicability);
 244             let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability);
 245             let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability);
 246             let space = if lo.ends_with('.') { " " } else { "" };
 247             span_lint_and_sugg(
 248                 cx,
 249                 MANUAL_RANGE_CONTAINS,
 250                 span,
 251                 &format!("manual `{}::contains` implementation", range_type),
 252                 "use",
 253                 format!("({}{}{}{}).contains(&{})", lo, space, range_op, hi, name),
 254                 applicability,
 255             );
 256         } else if !combine_and && ord == Some(lord) {
 257             // `!_.contains(_)`
 258             // order lower bound and upper bound
 259             let (l_span, u_span, l_inc, u_inc) = if lord == Ordering::Less {
 260                 (lval_span, rval_span, linc, rinc)
 261             } else {
 262                 (rval_span, lval_span, rinc, linc)
 263             };
 264             if l_inc {
 265                 return;
 266             }
 267             let (range_type, range_op) = if u_inc {
 268                 ("Range", "..")
 269             } else {
 270                 ("RangeInclusive", "..=")
 271             };
 272             let mut applicability = Applicability::MachineApplicable;
 273             let name = snippet_with_applicability(cx, name_span, "_", &mut applicability);
 274             let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability);
 275             let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability);
 276             let space = if lo.ends_with('.') { " " } else { "" };
 277             span_lint_and_sugg(
 278                 cx,
 279                 MANUAL_RANGE_CONTAINS,
 280                 span,
 281                 &format!("manual `!{}::contains` implementation", range_type),
 282                 "use",
 283                 format!("!({}{}{}{}).contains(&{})", lo, space, range_op, hi, name),
 284                 applicability,
 285             );
 286         }
 287     }
 288 }
 289
 290 fn check_range_bounds(cx: &LateContext<'_>, ex: &Expr<'_>) -> Option<(Constant, Ident, Span, Span, Ordering, bool)> {
 291     if let ExprKind::Binary(ref op, ref l, ref r) = ex.kind {
 292         let (inclusive, ordering) = match op.node {
 293             BinOpKind::Gt => (false, Ordering::Greater),
 294             BinOpKind::Ge => (true, Ordering::Greater),
 295             BinOpKind::Lt => (false, Ordering::Less),
 296             BinOpKind::Le => (true, Ordering::Less),
 297             _ => return None,
 298         };
 299         if let Some(id) = match_ident(l) {
 300             if let Some((c, _)) = constant(cx, cx.typeck_results(), r) {
 301                 return Some((c, id, l.span, r.span, ordering, inclusive));
 302             }
 303         } else if let Some(id) = match_ident(r) {
 304             if let Some((c, _)) = constant(cx, cx.typeck_results(), l) {
 305                 return Some((c, id, r.span, l.span, ordering.reverse(), inclusive));
 306             }
 307         }
 308     }
 309     None
 310 }
 311
 312 fn match_ident(e: &Expr<'_>) -> Option<Ident> {
 313     if let ExprKind::Path(ref qpath) = e.kind {
 314         if let Some(seg) = single_segment_path(qpath) {
 315             if seg.args.is_none() {
 316                 return Some(seg.ident);
 317             }
 318         }
 319     }
 320     None
 321 }
 322
 323 fn check_range_zip_with_len(cx: &LateContext<'_>, path: &PathSegment<'_>, args: &[Expr<'_>], span: Span) {
 324     let name = path.ident.as_str();
 325     if name == "zip" && args.len() == 2 {
 326         let iter = &args[0].kind;
 327         let zip_arg = &args[1];
 328         if_chain! {
 329             // `.iter()` call
 330             if let ExprKind::MethodCall(ref iter_path, _, ref iter_args, _) = *iter;
 331             if iter_path.ident.name == sym::iter;
 332             // range expression in `.zip()` call: `0..x.len()`
 333             if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(zip_arg);
 334             if is_integer_const(cx, start, 0);
 335             // `.len()` call
 336             if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
 337             if len_path.ident.name == sym!(len) && len_args.len() == 1;
 338             // `.iter()` and `.len()` called on same `Path`
 339             if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
 340             if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
 341             if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
 342             then {
 343                 span_lint(cx,
 344                     RANGE_ZIP_WITH_LEN,
 345                     span,
 346                     &format!("it is more idiomatic to use `{}.iter().enumerate()`",
 347                         snippet(cx, iter_args[0].span, "_"))
 348                 );
 349             }
 350         }
 351     }
 352 }
 353
 354 // exclusive range plus one: `x..(y+1)`
 355 fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
 356     if_chain! {
 357         if let Some(higher::Range {
 358             start,
 359             end: Some(end),
 360             limits: RangeLimits::HalfOpen
 361         }) = higher::range(expr);
 362         if let Some(y) = y_plus_one(cx, end);
 363         then {
 364             let span = if expr.span.from_expansion() {
 365                 expr.span
 366                     .ctxt()
 367                     .outer_expn_data()
 368                     .call_site
 369             } else {
 370                 expr.span
 371             };
 372             span_lint_and_then(
 373                 cx,
 374                 RANGE_PLUS_ONE,
 375                 span,
 376                 "an inclusive range would be more readable",
 377                 |diag| {
 378                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 379                     let end = Sugg::hir(cx, y, "y");
 380                     if let Some(is_wrapped) = &snippet_opt(cx, span) {
 381                         if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
 382                             diag.span_suggestion(
 383                                 span,
 384                                 "use",
 385                                 format!("({}..={})", start, end),
 386                                 Applicability::MaybeIncorrect,
 387                             );
 388                         } else {
 389                             diag.span_suggestion(
 390                                 span,
 391                                 "use",
 392                                 format!("{}..={}", start, end),
 393                                 Applicability::MachineApplicable, // snippet
 394                             );
 395                         }
 396                     }
 397                 },
 398             );
 399         }
 400     }
 401 }
 402
 403 // inclusive range minus one: `x..=(y-1)`
 404 fn check_inclusive_range_minus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
 405     if_chain! {
 406         if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(expr);
 407         if let Some(y) = y_minus_one(cx, end);
 408         then {
 409             span_lint_and_then(
 410                 cx,
 411                 RANGE_MINUS_ONE,
 412                 expr.span,
 413                 "an exclusive range would be more readable",
 414                 |diag| {
 415                     let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string());
 416                     let end = Sugg::hir(cx, y, "y");
 417                     diag.span_suggestion(
 418                         expr.span,
 419                         "use",
 420                         format!("{}..{}", start, end),
 421                         Applicability::MachineApplicable, // snippet
 422                     );
 423                 },
 424             );
 425         }
 426     }
 427 }
 428
 429 fn check_reversed_empty_range(cx: &LateContext<'_>, expr: &Expr<'_>) {
 430     fn inside_indexing_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 431         matches!(
 432             get_parent_expr(cx, expr),
 433             Some(Expr {
 434                 kind: ExprKind::Index(..),
 435                 ..
 436             })
 437         )
 438     }
 439
 440     fn is_for_loop_arg(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 441         let mut cur_expr = expr;
 442         while let Some(parent_expr) = get_parent_expr(cx, cur_expr) {
 443             match higher::for_loop(parent_expr) {
 444                 Some((_, args, _, _)) if args.hir_id == expr.hir_id => return true,
 445                 _ => cur_expr = parent_expr,
 446             }
 447         }
 448
 449         false
 450     }
 451
 452     fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
 453         match limits {
 454             RangeLimits::HalfOpen => ordering != Ordering::Less,
 455             RangeLimits::Closed => ordering == Ordering::Greater,
 456         }
 457     }
 458
 459     if_chain! {
 460         if let Some(higher::Range { start: Some(start), end: Some(end), limits }) = higher::range(expr);
 461         let ty = cx.typeck_results().expr_ty(start);
 462         if let ty::Int(_) | ty::Uint(_) = ty.kind();
 463         if let Some((start_idx, _)) = constant(cx, cx.typeck_results(), start);
 464         if let Some((end_idx, _)) = constant(cx, cx.typeck_results(), end);
 465         if let Some(ordering) = Constant::partial_cmp(cx.tcx, ty, &start_idx, &end_idx);
 466         if is_empty_range(limits, ordering);
 467         then {
 468             if inside_indexing_expr(cx, expr) {
 469                 // Avoid linting `N..N` as it has proven to be useful, see #5689 and #5628 ...
 470                 if ordering != Ordering::Equal {
 471                     span_lint(
 472                         cx,
 473                         REVERSED_EMPTY_RANGES,
 474                         expr.span,
 475                         "this range is reversed and using it to index a slice will panic at run-time",
 476                     );
 477                 }
 478             // ... except in for loop arguments for backwards compatibility with `reverse_range_loop`
 479             } else if ordering != Ordering::Equal || is_for_loop_arg(cx, expr) {
 480                 span_lint_and_then(
 481                     cx,
 482                     REVERSED_EMPTY_RANGES,
 483                     expr.span,
 484                     "this range is empty so it will yield no values",
 485                     |diag| {
 486                         if ordering != Ordering::Equal {
 487                             let start_snippet = snippet(cx, start.span, "_");
 488                             let end_snippet = snippet(cx, end.span, "_");
 489                             let dots = match limits {
 490                                 RangeLimits::HalfOpen => "..",
 491                                 RangeLimits::Closed => "..="
 492                             };
 493
 494                             diag.span_suggestion(
 495                                 expr.span,
 496                                 "consider using the following if you are attempting to iterate over this \
 497                                  range in reverse",
 498                                 format!("({}{}{}).rev()", end_snippet, dots, start_snippet),
 499                                 Applicability::MaybeIncorrect,
 500                             );
 501                         }
 502                     },
 503                 );
 504             }
 505         }
 506     }
 507 }
 508
 509 fn y_plus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 510     match expr.kind {
 511         ExprKind::Binary(
 512             Spanned {
 513                 node: BinOpKind::Add, ..
 514             },
 515             ref lhs,
 516             ref rhs,
 517         ) => {
 518             if is_integer_const(cx, lhs, 1) {
 519                 Some(rhs)
 520             } else if is_integer_const(cx, rhs, 1) {
 521                 Some(lhs)
 522             } else {
 523                 None
 524             }
 525         },
 526         _ => None,
 527     }
 528 }
 529
 530 fn y_minus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
 531     match expr.kind {
 532         ExprKind::Binary(
 533             Spanned {
 534                 node: BinOpKind::Sub, ..
 535             },
 536             ref lhs,
 537             ref rhs,
 538         ) if is_integer_const(cx, rhs, 1) => Some(lhs),
 539         _ => None,
 540     }
 541 }