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