New lint: reversed_empty_ranges

ebroto · ebroto · commit 8ffa0bfaa245 · 2020-05-13T20:33:32.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1546,6 +1546,7 @@ Released 2018-09-13
 [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else
 [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used
 [`reverse_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#reverse_range_loop
+[`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
 [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition
 [`search_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#search_is_some
 [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -770,6 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &ranges::RANGE_MINUS_ONE,
         &ranges::RANGE_PLUS_ONE,
         &ranges::RANGE_ZIP_WITH_LEN,
+        &ranges::REVERSED_EMPTY_RANGES,
         &redundant_clone::REDUNDANT_CLONE,
         &redundant_field_names::REDUNDANT_FIELD_NAMES,
         &redundant_pattern_matching::REDUNDANT_PATTERN_MATCHING,
@@ -1384,6 +1385,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&question_mark::QUESTION_MARK),
         LintId::of(&ranges::RANGE_MINUS_ONE),
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
+        LintId::of(&ranges::REVERSED_EMPTY_RANGES),
         LintId::of(&redundant_clone::REDUNDANT_CLONE),
         LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
         LintId::of(&redundant_pattern_matching::REDUNDANT_PATTERN_MATCHING),
@@ -1675,6 +1677,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
         LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP),
         LintId::of(&ptr::MUT_FROM_REF),
+        LintId::of(&ranges::REVERSED_EMPTY_RANGES),
         LintId::of(&regex::INVALID_REGEX),
         LintId::of(&serde_api::SERDE_API_MISUSE),
         LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs
@@ -1,14 +1,17 @@
+use crate::consts::{constant, Constant};
 use if_chain::if_chain;
 use rustc_ast::ast::RangeLimits;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Spanned;
+use std::cmp::Ordering;
 
 use crate::utils::sugg::Sugg;
+use crate::utils::{get_parent_expr, is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
 use crate::utils::{higher, SpanlessEq};
-use crate::utils::{is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for zipping a collection with the range of
@@ -84,10 +87,44 @@ declare_clippy_lint! {
     "`x..=(y-1)` reads better as `x..y`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for range expressions `x..y` where both `x` and `y`
+    /// are constant and `x` is greater or equal to `y`.
+    ///
+    /// **Why is this bad?** Empty ranges yield no values so iterating them is a no-op.
+    /// Moreover, trying to use a reversed range to index a slice will panic at run-time.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// fn main() {
+    ///     (10..=0).for_each(|x| println!("{}", x));
+    ///
+    ///     let arr = [1, 2, 3, 4, 5];
+    ///     let sub = &arr[3..1];
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn main() {
+    ///     (0..=10).rev().for_each(|x| println!("{}", x));
+    ///
+    ///     let arr = [1, 2, 3, 4, 5];
+    ///     let sub = &arr[1..3];
+    /// }
+    /// ```
+    pub REVERSED_EMPTY_RANGES,
+    correctness,
+    "reversing the limits of range expressions, resulting in empty ranges"
+}
+
 declare_lint_pass!(Ranges => [
     RANGE_ZIP_WITH_LEN,
     RANGE_PLUS_ONE,
-    RANGE_MINUS_ONE
+    RANGE_MINUS_ONE,
+    REVERSED_EMPTY_RANGES,
 ]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
@@ -124,6 +161,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges {
 
         check_exclusive_range_plus_one(cx, expr);
         check_inclusive_range_minus_one(cx, expr);
+        check_reversed_empty_range(cx, expr);
     }
 }
 
@@ -202,6 +240,76 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
     }
 }
 
+fn check_reversed_empty_range(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
+    fn inside_indexing_expr(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+        matches!(
+            get_parent_expr(cx, expr),
+            Some(Expr {
+                kind: ExprKind::Index(..),
+                ..
+            })
+        )
+    }
+
+    fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
+        match limits {
+            RangeLimits::HalfOpen => ordering != Ordering::Less,
+            RangeLimits::Closed => ordering == Ordering::Greater,
+        }
+    }
+
+    if_chain! {
+        if let Some(higher::Range { start: Some(start), end: Some(end), limits }) = higher::range(cx, expr);
+        let ty = cx.tables.expr_ty(start);
+        if let ty::Int(_) | ty::Uint(_) = ty.kind;
+        if let Some((start_idx, _)) = constant(cx, cx.tables, start);
+        if let Some((end_idx, _)) = constant(cx, cx.tables, end);
+        if let Some(ordering) = Constant::partial_cmp(cx.tcx, ty, &start_idx, &end_idx);
+        if is_empty_range(limits, ordering);
+        then {
+            if inside_indexing_expr(cx, expr) {
+                let (reason, outcome) = if ordering == Ordering::Equal {
+                    ("empty", "always yield an empty slice")
+                } else {
+                    ("reversed", "panic at run-time")
+                };
+
+                span_lint(
+                    cx,
+                    REVERSED_EMPTY_RANGES,
+                    expr.span,
+                    &format!("this range is {} and using it to index a slice will {}", reason, outcome),
+                );
+            } else {
+                span_lint_and_then(
+                    cx,
+                    REVERSED_EMPTY_RANGES,
+                    expr.span,
+                    "this range is empty so it will yield no values",
+                    |diag| {
+                        if ordering != Ordering::Equal {
+                            let start_snippet = snippet(cx, start.span, "_");
+                            let end_snippet = snippet(cx, end.span, "_");
+                            let dots = match limits {
+                                RangeLimits::HalfOpen => "..",
+                                RangeLimits::Closed => "..="
+                            };
+
+                            diag.span_suggestion(
+                                expr.span,
+                                "consider using the following if you are attempting to iterate over this \
+                                 range in reverse",
+                                format!("({}{}{}).rev()", end_snippet, dots, start_snippet),
+                                Applicability::MaybeIncorrect,
+                            );
+                        }
+                    },
+                );
+            }
+        }
+    }
+}
+
 fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
     match expr.kind {
         ExprKind::Binary(
diff --git a/tests/ui/reversed_empty_ranges_fixable.fixed b/tests/ui/reversed_empty_ranges_fixable.fixed
@@ -0,0 +1,24 @@
+// run-rustfix
+#![warn(clippy::reversed_empty_ranges)]
+
+const ANSWER: i32 = 42;
+
+fn main() {
+    (21..=42).rev().for_each(|x| println!("{}", x));
+    let _ = (21..ANSWER).rev().filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
+
+    for _ in (-42..=-21).rev() {}
+    for _ in (21u32..42u32).rev() {}
+
+    // These should be ignored as they are not empty ranges:
+
+    (21..=42).for_each(|x| println!("{}", x));
+    (21..42).for_each(|x| println!("{}", x));
+
+    let arr = [1, 2, 3, 4, 5];
+    let _ = &arr[1..=3];
+    let _ = &arr[1..3];
+
+    for _ in 21..=42 {}
+    for _ in 21..42 {}
+}
diff --git a/tests/ui/reversed_empty_ranges_fixable.rs b/tests/ui/reversed_empty_ranges_fixable.rs
@@ -0,0 +1,24 @@
+// run-rustfix
+#![warn(clippy::reversed_empty_ranges)]
+
+const ANSWER: i32 = 42;
+
+fn main() {
+    (42..=21).for_each(|x| println!("{}", x));
+    let _ = (ANSWER..21).filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
+
+    for _ in -21..=-42 {}
+    for _ in 42u32..21u32 {}
+
+    // These should be ignored as they are not empty ranges:
+
+    (21..=42).for_each(|x| println!("{}", x));
+    (21..42).for_each(|x| println!("{}", x));
+
+    let arr = [1, 2, 3, 4, 5];
+    let _ = &arr[1..=3];
+    let _ = &arr[1..3];
+
+    for _ in 21..=42 {}
+    for _ in 21..42 {}
+}
diff --git a/tests/ui/reversed_empty_ranges_fixable.stderr b/tests/ui/reversed_empty_ranges_fixable.stderr
@@ -0,0 +1,47 @@
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_fixable.rs:7:5
+   |
+LL |     (42..=21).for_each(|x| println!("{}", x));
+   |     ^^^^^^^^^
+   |
+   = note: `-D clippy::reversed-empty-ranges` implied by `-D warnings`
+help: consider using the following if you are attempting to iterate over this range in reverse
+   |
+LL |     (21..=42).rev().for_each(|x| println!("{}", x));
+   |     ^^^^^^^^^^^^^^^
+
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_fixable.rs:8:13
+   |
+LL |     let _ = (ANSWER..21).filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
+   |             ^^^^^^^^^^^^
+   |
+help: consider using the following if you are attempting to iterate over this range in reverse
+   |
+LL |     let _ = (21..ANSWER).rev().filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
+   |             ^^^^^^^^^^^^^^^^^^
+
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_fixable.rs:10:14
+   |
+LL |     for _ in -21..=-42 {}
+   |              ^^^^^^^^^
+   |
+help: consider using the following if you are attempting to iterate over this range in reverse
+   |
+LL |     for _ in (-42..=-21).rev() {}
+   |              ^^^^^^^^^^^^^^^^^
+
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_fixable.rs:11:14
+   |
+LL |     for _ in 42u32..21u32 {}
+   |              ^^^^^^^^^^^^
+   |
+help: consider using the following if you are attempting to iterate over this range in reverse
+   |
+LL |     for _ in (21u32..42u32).rev() {}
+   |              ^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 4 previous errors
+
diff --git a/tests/ui/reversed_empty_ranges_unfixable.rs b/tests/ui/reversed_empty_ranges_unfixable.rs
@@ -0,0 +1,15 @@
+#![warn(clippy::reversed_empty_ranges)]
+
+const ANSWER: i32 = 42;
+const SOME_NUM: usize = 3;
+
+fn main() {
+    let _ = (42 + 10..42 + 10).map(|x| x / 2).find(|&x| x == 21);
+
+    let arr = [1, 2, 3, 4, 5];
+    let _ = &arr[3usize..=1usize];
+    let _ = &arr[SOME_NUM..1];
+    let _ = &arr[3..3];
+
+    for _ in ANSWER..ANSWER {}
+}
diff --git a/tests/ui/reversed_empty_ranges_unfixable.stderr b/tests/ui/reversed_empty_ranges_unfixable.stderr
@@ -0,0 +1,34 @@
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_unfixable.rs:7:13
+   |
+LL |     let _ = (42 + 10..42 + 10).map(|x| x / 2).find(|&x| x == 21);
+   |             ^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::reversed-empty-ranges` implied by `-D warnings`
+
+error: this range is reversed and using it to index a slice will panic at run-time
+  --> $DIR/reversed_empty_ranges_unfixable.rs:10:18
+   |
+LL |     let _ = &arr[3usize..=1usize];
+   |                  ^^^^^^^^^^^^^^^
+
+error: this range is reversed and using it to index a slice will panic at run-time
+  --> $DIR/reversed_empty_ranges_unfixable.rs:11:18
+   |
+LL |     let _ = &arr[SOME_NUM..1];
+   |                  ^^^^^^^^^^^
+
+error: this range is empty and using it to index a slice will always yield an empty slice
+  --> $DIR/reversed_empty_ranges_unfixable.rs:12:18
+   |
+LL |     let _ = &arr[3..3];
+   |                  ^^^^
+
+error: this range is empty so it will yield no values
+  --> $DIR/reversed_empty_ranges_unfixable.rs:14:14
+   |
+LL |     for _ in ANSWER..ANSWER {}
+   |              ^^^^^^^^^^^^^^
+
+error: aborting due to 5 previous errors
+
