clippy_lints/src/ranges.rs

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