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