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