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