Skip to content

Commit

Permalink
Second pass at addressing changes requested
Browse files Browse the repository at this point in the history 
The changes reflected in this commit (requested in PR #2790) are as follows:

- Extended `INDEXING_SLICING` documentation to include the array type so that it is clearer when indexing operations are allowed.
- Variable `ty` defined identically in multiple scopes was moved to an outer scope so it's only defined once.
- Added a missing return statement to ensure only one lint is triggered by a scenario.
- Prettified match statement with a `let` clause. (I learned something new!)
- Added `&x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>()` and `&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>()` to the test cases. The first _should trigger the lint/stderr_ and the second _should not_.
  • Loading branch information
@shnewto
shnewto committed Jun 19, 2018
1 parent a7c0ff3 commit 8b59542
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 117 deletions.
55 changes: 36 additions & 19 deletions clippy_lints/src/indexing_slicing.rs
View file
Expand Up @@ -34,7 +34,7 @@ declare_clippy_lint! {
}

/// **What it does:** Checks for usage of indexing or slicing. Does not report
/// if we can tell that the indexing or slicing operations on an array are in
/// on arrays if we can tell that the indexing or slicing operations are in
/// bounds.
///
/// **Why is this bad?** Indexing and slicing can panic at runtime and there are
Expand All @@ -44,18 +44,39 @@ declare_clippy_lint! {
///
/// **Example:**
/// ```rust
/// // Vector
/// let x = vec![0; 5];
///
/// // Bad
/// x[2];
/// &x[2..100];
/// &x[2..];
/// &x[..100];
///
/// // Good
/// x.get(2)
/// x.get(2..100)
/// x.get(2..)
/// x.get(..100)
/// x.get(2);
/// x.get(2..100);
/// x.get(2..);
/// x.get(..100);
///
/// // Array
/// let y = [0, 1, 2, 3];
///
/// // Bad
/// y[10];
/// &y[10..100];
/// &y[10..];
/// &y[..100];
///
/// // Good
/// y[2];
/// &y[2..];
/// &y[..2];
/// &y[0..3];
/// y.get(10);
/// y.get(10..100);
/// y.get(10..);
/// y.get(..100);
/// ```
declare_clippy_lint! {
pub INDEXING_SLICING,
Expand All @@ -75,14 +96,14 @@ impl LintPass for IndexingSlicing {
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if let ExprIndex(ref array, ref index) = &expr.node {
let ty = cx.tables.expr_ty(array);
match &index.node {
// Both ExprStruct and ExprPath require this approach's checks
// on the `range` returned by `higher::range(cx, index)`.
// ExprStruct handles &x[n..m], &x[n..] and &x[..n].
// ExprPath handles &x[..] and x[var]
ExprStruct(..) | ExprPath(..) => {
if let Some(range) = higher::range(cx, index) {
let ty = cx.tables.expr_ty(array);
if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant range.
Expand All @@ -94,27 +115,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
expr.span,
"range is out of bounds",
);
} else {
// Range is in bounds, ok.
return;
}
} // Else range is in bounds, ok.

return;
}
}

let help_msg;
match (range.start, range.end) {
let help_msg = match (range.start, range.end) {
(None, Some(_)) => {
help_msg = "Consider using `.get(..n)`or `.get_mut(..n)` instead";
"Consider using `.get(..n)`or `.get_mut(..n)` instead"
}
(Some(_), None) => {
help_msg = "Consider using `.get(n..)` or .get_mut(n..)` instead";
"Consider using `.get(n..)` or .get_mut(n..)` instead"
}
(Some(_), Some(_)) => {
help_msg =
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead";
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead"
}
(None, None) => return, // [..] is ok
}
(None, None) => return, // [..] is ok.
};

utils::span_help_and_lint(
cx,
Expand All @@ -135,7 +153,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
}
ExprLit(..) => {
// [n]
let ty = cx.tables.expr_ty(array);
if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant uint.
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/indexing_slicing.rs
View file
Expand Up @@ -30,6 +30,8 @@ fn main() {
&x[5..];
&x[..4];
&x[..5];
&x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok

let y = &x;
y[0];
Expand Down
130 changes: 32 additions & 98 deletions tests/ui/indexing_slicing.stderr
View file
Expand Up @@ -61,14 +61,6 @@ error: range is out of bounds
20 | &x[1..5];
| ^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:20:6
|
20 | &x[1..5];
| ^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:21:6
|
Expand All @@ -91,181 +83,123 @@ error: range is out of bounds
26 | &x[..=4];
| ^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:26:6
|
26 | &x[..=4];
| ^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: range is out of bounds
--> $DIR/indexing_slicing.rs:30:6
|
30 | &x[5..];
| ^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:30:6
|
30 | &x[5..];
| ^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead

error: range is out of bounds
--> $DIR/indexing_slicing.rs:32:6
|
32 | &x[..5];
| ^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:32:6
error: range is out of bounds
--> $DIR/indexing_slicing.rs:33:6
|
32 | &x[..5];
33 | &x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
| ^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: indexing may panic.
--> $DIR/indexing_slicing.rs:35:5
--> $DIR/indexing_slicing.rs:37:5
|
35 | y[0];
37 | y[0];
| ^^^^
|
= help: Consider using `.get(n)` or `.get_mut(n)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:36:6
--> $DIR/indexing_slicing.rs:38:6
|
36 | &y[1..2];
38 | &y[1..2];
| ^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:39:6
--> $DIR/indexing_slicing.rs:41:6
|
39 | &y[..=4];
41 | &y[..=4];
| ^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: const index is out of bounds
--> $DIR/indexing_slicing.rs:42:5
--> $DIR/indexing_slicing.rs:44:5
|
42 | empty[0];
44 | empty[0];
| ^^^^^^^^

error: range is out of bounds
--> $DIR/indexing_slicing.rs:43:6
|
43 | &empty[1..5];
| ^^^^^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:43:6
|
43 | &empty[1..5];
| ^^^^^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead

error: range is out of bounds
--> $DIR/indexing_slicing.rs:45:6
|
45 | &empty[..=4];
45 | &empty[1..5];
| ^^^^^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:45:6
|
45 | &empty[..=4];
| ^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: range is out of bounds
--> $DIR/indexing_slicing.rs:50:6
|
50 | &empty[..=0];
| ^^^^^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:50:6
--> $DIR/indexing_slicing.rs:47:6
|
50 | &empty[..=0];
47 | &empty[..=4];
| ^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: range is out of bounds
--> $DIR/indexing_slicing.rs:52:6
|
52 | &empty[1..];
| ^^^^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:52:6
|
52 | &empty[1..];
| ^^^^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
52 | &empty[..=0];
| ^^^^^^^^^^^

error: range is out of bounds
--> $DIR/indexing_slicing.rs:53:6
--> $DIR/indexing_slicing.rs:54:6
|
53 | &empty[..4];
54 | &empty[1..];
| ^^^^^^^^^^

error: slicing may panic.
--> $DIR/indexing_slicing.rs:53:6
error: range is out of bounds
--> $DIR/indexing_slicing.rs:55:6
|
53 | &empty[..4];
55 | &empty[..4];
| ^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: indexing may panic.
--> $DIR/indexing_slicing.rs:56:5
--> $DIR/indexing_slicing.rs:58:5
|
56 | v[0];
58 | v[0];
| ^^^^
|
= help: Consider using `.get(n)` or `.get_mut(n)` instead

error: indexing may panic.
--> $DIR/indexing_slicing.rs:57:5
--> $DIR/indexing_slicing.rs:59:5
|
57 | v[10];
59 | v[10];
| ^^^^^
|
= help: Consider using `.get(n)` or `.get_mut(n)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:58:6
--> $DIR/indexing_slicing.rs:60:6
|
58 | &v[10..100];
60 | &v[10..100];
| ^^^^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:59:6
--> $DIR/indexing_slicing.rs:61:6
|
59 | &v[10..];
61 | &v[10..];
| ^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead

error: slicing may panic.
--> $DIR/indexing_slicing.rs:60:6
--> $DIR/indexing_slicing.rs:62:6
|
60 | &v[..100];
62 | &v[..100];
| ^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead

error: aborting due to 36 previous errors
error: aborting due to 28 previous errors

0 comments on commit 8b59542

Please sign in to comment.