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