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

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