Skip to content

Commit

Permalink
Auto merge of rust-lang#5583 - ebroto:reversed_empty_ranges, r=yaahc,…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@bors
bors committed May 14, 2020
2 parents 7147068 + 671c1e3 commit e1842b0
Show file tree
Hide file tree
Showing 21 changed files with 500 additions and 313 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions clippy_lints/src/lib.rs
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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.
Expand Down
102 changes: 2 additions & 100 deletions clippy_lints/src/loops.rs
View file
@@ -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};
Expand All @@ -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};
Expand Down Expand Up @@ -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()`.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
112 changes: 110 additions & 2 deletions clippy_lints/src/ranges.rs
View file
@@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions src/lintlist/mod.rs
View file
Expand Up @@ -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",
Expand Down

0 comments on commit e1842b0

Please sign in to comment.