Skip to content

Commit

Permalink
Auto merge of #5638 - ebroto:issue_5628_add_suggestion_for_reversed_e…
Browse files Browse the repository at this point in the history
…mpty_ranges, r=phansch

reversed_empty_ranges: add suggestion for &slice[N..N]

As discussed in the issue thread, the user accepted this solution. Let me know if this is what we want, or if changing the way we lint the N..N case is prefered.

changelog: reversed_empty_ranges: add suggestion for &slice[N..N]

Closes #5628
  • Loading branch information
bors committed May 26, 2020
2 parents 578692d + 60d38ee commit a00025a
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 24 deletions.
30 changes: 22 additions & 8 deletions clippy_lints/src/ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,14 +241,14 @@ 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 {
fn inside_indexing_expr<'a>(cx: &'a LateContext<'_, '_>, expr: &Expr<'_>) -> Option<&'a Expr<'a>> {
match get_parent_expr(cx, expr) {
parent_expr @ Some(Expr {
kind: ExprKind::Index(..),
..
})
)
}) => parent_expr,
_ => None,
}
}

fn is_empty_range(limits: RangeLimits, ordering: Ordering) -> bool {
Expand All @@ -267,18 +267,32 @@ fn check_reversed_empty_range(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
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) {
if let Some(parent_expr) = 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(
span_lint_and_then(
cx,
REVERSED_EMPTY_RANGES,
expr.span,
&format!("this range is {} and using it to index a slice will {}", reason, outcome),
|diag| {
if_chain! {
if ordering == Ordering::Equal;
if let ty::Slice(slice_ty) = cx.tables.expr_ty(parent_expr).kind;
then {
diag.span_suggestion(
parent_expr.span,
"if you want an empty slice, use",
format!("[] as &[{}]", slice_ty),
Applicability::MaybeIncorrect
);
}
}
}
);
} else {
span_lint_and_then(
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/reversed_empty_ranges_fixable.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,23 @@
const ANSWER: i32 = 42;

fn main() {
let arr = [1, 2, 3, 4, 5];

// These should be linted:

(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() {}

let _ = &[] as &[i32];

// 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];

Expand Down
7 changes: 6 additions & 1 deletion tests/ui/reversed_empty_ranges_fixable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,23 @@
const ANSWER: i32 = 42;

fn main() {
let arr = [1, 2, 3, 4, 5];

// These should be linted:

(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 {}

let _ = &arr[3..3];

// 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];

Expand Down
16 changes: 11 additions & 5 deletions tests/ui/reversed_empty_ranges_fixable.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this range is empty so it will yield no values
--> $DIR/reversed_empty_ranges_fixable.rs:7:5
--> $DIR/reversed_empty_ranges_fixable.rs:11:5
|
LL | (42..=21).for_each(|x| println!("{}", x));
| ^^^^^^^^^
Expand All @@ -11,7 +11,7 @@ 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
--> $DIR/reversed_empty_ranges_fixable.rs:12:13
|
LL | let _ = (ANSWER..21).filter(|x| x % 2 == 0).take(10).collect::<Vec<_>>();
| ^^^^^^^^^^^^
Expand All @@ -22,7 +22,7 @@ LL | let _ = (21..ANSWER).rev().filter(|x| x % 2 == 0).take(10).collect::<Ve
| ^^^^^^^^^^^^^^^^^^

error: this range is empty so it will yield no values
--> $DIR/reversed_empty_ranges_fixable.rs:10:14
--> $DIR/reversed_empty_ranges_fixable.rs:14:14
|
LL | for _ in -21..=-42 {}
| ^^^^^^^^^
Expand All @@ -33,7 +33,7 @@ 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
--> $DIR/reversed_empty_ranges_fixable.rs:15:14
|
LL | for _ in 42u32..21u32 {}
| ^^^^^^^^^^^^
Expand All @@ -43,5 +43,11 @@ help: consider using the following if you are attempting to iterate over this ra
LL | for _ in (21u32..42u32).rev() {}
| ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors
error: this range is empty and using it to index a slice will always yield an empty slice
--> $DIR/reversed_empty_ranges_fixable.rs:17:18
|
LL | let _ = &arr[3..3];
| ----^^^^- help: if you want an empty slice, use: `[] as &[i32]`

error: aborting due to 5 previous errors

1 change: 0 additions & 1 deletion tests/ui/reversed_empty_ranges_unfixable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ fn main() {
let arr = [1, 2, 3, 4, 5];
let _ = &arr[3usize..=1usize];
let _ = &arr[SOME_NUM..1];
let _ = &arr[3..3];

for _ in ANSWER..ANSWER {}
}
10 changes: 2 additions & 8 deletions tests/ui/reversed_empty_ranges_unfixable.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,11 @@ error: this range is reversed and using it to index a slice will panic at run-ti
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
--> $DIR/reversed_empty_ranges_unfixable.rs:13:14
|
LL | for _ in ANSWER..ANSWER {}
| ^^^^^^^^^^^^^^

error: aborting due to 5 previous errors
error: aborting due to 4 previous errors

0 comments on commit a00025a

Please sign in to comment.