Auto merge of rust-lang#5583 - ebroto:reversed_empty_ranges, r=yaahc,flip1995

Reversed empty ranges

This lint checks range expressions with inverted limits which result in empty ranges. This includes also the ranges used to index slices.

The lint reverse_range_loop was covering iteration of reversed ranges in a for loop, which is a subset of what this new lint covers, so it has been removed. I'm not sure if that's the best choice. It would be doable to check in the new lint that we are not in the arguments of a for loop; I went for removing it because the logic was too similar to keep them separated.

changelog: Added reversed_empty_ranges lint that checks for ranges where the limits have been inverted, resulting in empty ranges. Removed reverse_range_loop which was covering a subset of the new lint.

Closes rust-lang#4192
Closes rust-lang#96

Author: bors

Diff for: CHANGELOG.md

@@ -1545,7 +1545,7 @@ Released 2018-09-13
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
 [`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 for: clippy_lints/src/lib.rs

@@ -624,7 +624,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &loops::NEEDLESS_COLLECT,
         &loops::NEEDLESS_RANGE_LOOP,
         &loops::NEVER_LOOP,
-        &loops::REVERSE_RANGE_LOOP,
         &loops::WHILE_IMMUTABLE_CONDITION,
         &loops::WHILE_LET_LOOP,
         &loops::WHILE_LET_ON_ITERATOR,
@@ -770,6 +769,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,
@@ -1283,7 +1283,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::NEEDLESS_COLLECT),
         LintId::of(&loops::NEEDLESS_RANGE_LOOP),
         LintId::of(&loops::NEVER_LOOP),
-        LintId::of(&loops::REVERSE_RANGE_LOOP),
         LintId::of(&loops::WHILE_IMMUTABLE_CONDITION),
         LintId::of(&loops::WHILE_LET_LOOP),
         LintId::of(&loops::WHILE_LET_ON_ITERATOR),
@@ -1384,6 +1383,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),
@@ -1656,7 +1656,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::FOR_LOOP_OVER_RESULT),
         LintId::of(&loops::ITER_NEXT_LOOP),
         LintId::of(&loops::NEVER_LOOP),
-        LintId::of(&loops::REVERSE_RANGE_LOOP),
         LintId::of(&loops::WHILE_IMMUTABLE_CONDITION),
         LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM),
         LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT),
@@ -1675,6 +1674,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),
@@ -1785,6 +1785,10 @@ fn register_removed_non_tool_lints(store: &mut rustc_lint::LintStore) {
         "unsafe_vector_initialization",
         "the replacement suggested by this lint had substantially different behavior",
     );
+    store.register_removed(
+        "reverse_range_loop",
+        "this lint is now included in reversed_empty_ranges",
+    );
 }
 
 /// Register renamed lints.

Diff for: clippy_lints/src/loops.rs

@@ -1,4 +1,4 @@
-use crate::consts::{constant, Constant};
+use crate::consts::constant;
 use crate::reexport::Name;
 use crate::utils::paths;
 use crate::utils::usage::{is_unused, mutated_variables};
@@ -8,7 +8,7 @@ use crate::utils::{
     multispan_sugg, snippet, snippet_opt, snippet_with_applicability, span_lint, span_lint_and_help,
     span_lint_and_sugg, span_lint_and_then, SpanlessEq,
 };
-use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg};
+use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sugg};
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
@@ -270,30 +270,6 @@ declare_clippy_lint! {
     "collecting an iterator when collect is not needed"
 }
 
-declare_clippy_lint! {
-    /// **What it does:** Checks for loops over ranges `x..y` where both `x` and `y`
-    /// are constant and `x` is greater or equal to `y`, unless the range is
-    /// reversed or has a negative `.step_by(_)`.
-    ///
-    /// **Why is it bad?** Such loops will either be skipped or loop until
-    /// wrap-around (in debug code, this may `panic!()`). Both options are probably
-    /// not intended.
-    ///
-    /// **Known problems:** The lint cannot catch loops over dynamically defined
-    /// ranges. Doing this would require simulating all possible inputs and code
-    /// paths through the program, which would be complex and error-prone.
-    ///
-    /// **Example:**
-    /// ```ignore
-    /// for x in 5..10 - 5 {
-    ///     ..
-    /// } // oops, stray `-`
-    /// ```
-    pub REVERSE_RANGE_LOOP,
-    correctness,
-    "iteration over an empty range, such as `10..0` or `5..5`"
-}
-
 declare_clippy_lint! {
     /// **What it does:** Checks `for` loops over slices with an explicit counter
     /// and suggests the use of `.enumerate()`.
@@ -463,7 +439,6 @@ declare_lint_pass!(Loops => [
     FOR_LOOP_OVER_OPTION,
     WHILE_LET_LOOP,
     NEEDLESS_COLLECT,
-    REVERSE_RANGE_LOOP,
     EXPLICIT_COUNTER_LOOP,
     EMPTY_LOOP,
     WHILE_LET_ON_ITERATOR,
@@ -761,7 +736,6 @@ fn check_for_loop<'a, 'tcx>(
     expr: &'tcx Expr<'_>,
 ) {
     check_for_loop_range(cx, pat, arg, body, expr);
-    check_for_loop_reverse_range(cx, arg, expr);
     check_for_loop_arg(cx, pat, arg, expr);
     check_for_loop_explicit_counter(cx, pat, arg, body, expr);
     check_for_loop_over_map_kv(cx, pat, arg, body, expr);
@@ -1248,78 +1222,6 @@ fn is_end_eq_array_len<'tcx>(
     false
 }
 
-fn check_for_loop_reverse_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arg: &'tcx Expr<'_>, expr: &'tcx Expr<'_>) {
-    // if this for loop is iterating over a two-sided range...
-    if let Some(higher::Range {
-        start: Some(start),
-        end: Some(end),
-        limits,
-    }) = higher::range(cx, arg)
-    {
-        // ...and both sides are compile-time constant integers...
-        if let Some((start_idx, _)) = constant(cx, cx.tables, start) {
-            if let Some((end_idx, _)) = constant(cx, cx.tables, end) {
-                // ...and the start index is greater than the end index,
-                // this loop will never run. This is often confusing for developers
-                // who think that this will iterate from the larger value to the
-                // smaller value.
-                let ty = cx.tables.expr_ty(start);
-                let (sup, eq) = match (start_idx, end_idx) {
-                    (Constant::Int(start_idx), Constant::Int(end_idx)) => (
-                        match ty.kind {
-                            ty::Int(ity) => sext(cx.tcx, start_idx, ity) > sext(cx.tcx, end_idx, ity),
-                            ty::Uint(_) => start_idx > end_idx,
-                            _ => false,
-                        },
-                        start_idx == end_idx,
-                    ),
-                    _ => (false, false),
-                };
-
-                if sup {
-                    let start_snippet = snippet(cx, start.span, "_");
-                    let end_snippet = snippet(cx, end.span, "_");
-                    let dots = if limits == ast::RangeLimits::Closed {
-                        "..="
-                    } else {
-                        ".."
-                    };
-
-                    span_lint_and_then(
-                        cx,
-                        REVERSE_RANGE_LOOP,
-                        expr.span,
-                        "this range is empty so this for loop will never run",
-                        |diag| {
-                            diag.span_suggestion(
-                                arg.span,
-                                "consider using the following if you are attempting to iterate over this \
-                                 range in reverse",
-                                format!(
-                                    "({end}{dots}{start}).rev()",
-                                    end = end_snippet,
-                                    dots = dots,
-                                    start = start_snippet
-                                ),
-                                Applicability::MaybeIncorrect,
-                            );
-                        },
-                    );
-                } else if eq && limits != ast::RangeLimits::Closed {
-                    // if they are equal, it's also problematic - this loop
-                    // will never run.
-                    span_lint(
-                        cx,
-                        REVERSE_RANGE_LOOP,
-                        expr.span,
-                        "this range is empty so this for loop will never run",
-                    );
-                }
-            }
-        }
-    }
-}
-
 fn lint_iter_method(cx: &LateContext<'_, '_>, args: &[Expr<'_>], arg: &Expr<'_>, method_name: &str) {
     let mut applicability = Applicability::MachineApplicable;
     let object = snippet_with_applicability(cx, args[0].span, "_", &mut applicability);

Diff for: 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,no_run
+    /// 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 for: src/lintlist/mod.rs

@@ -1922,11 +1922,11 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         module: "methods",
     },
     Lint {
-        name: "reverse_range_loop",
+        name: "reversed_empty_ranges",
         group: "correctness",
-        desc: "iteration over an empty range, such as `10..0` or `5..5`",
+        desc: "reversing the limits of range expressions, resulting in empty ranges",
         deprecation: None,
-        module: "loops",
+        module: "ranges",
     },
     Lint {
         name: "same_functions_in_if_condition",