This commit represents an attempt to address changes requested in the process of reviewing PR #2790.

The changes reflected in this commit are as follows:

- Revised `IndexingSlicingPass` struct name to IndexingSlicing for consistency with the rest of the code base.
- Revised match arm condition to use `(..)` shorthand in favor of `(_, _, _)`.
- Restored a couple telling variable names.
- Calls to `cx.span_lint` were revised to use `utils::span_help_and_lint`.
- Took a stab at refactoring some generalizable calls to `utils::span_help_and_lint` to minimize duplicate code.
- Revised INDEXING_SLICING declaration to pedantic rather than restriction.
- Added `&x[0..].get(..3)` to the test cases.
This commit is contained in:
Shea Newton 2018-06-13 23:28:57 +00:00
parent 5b759efa4c
commit a7c0ff3fa6
No known key found for this signature in database
GPG key ID: 17EB9122DC958643
4 changed files with 174 additions and 124 deletions

View file

@ -59,30 +59,30 @@ declare_clippy_lint! {
/// ``` /// ```
declare_clippy_lint! { declare_clippy_lint! {
pub INDEXING_SLICING, pub INDEXING_SLICING,
restriction, pedantic,
"indexing/slicing usage" "indexing/slicing usage"
} }
#[derive(Copy, Clone)] #[derive(Copy, Clone)]
pub struct IndexingSlicingPass; pub struct IndexingSlicing;
impl LintPass for IndexingSlicingPass { impl LintPass for IndexingSlicing {
fn get_lints(&self) -> LintArray { fn get_lints(&self) -> LintArray {
lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING) lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING)
} }
} }
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if let ExprIndex(ref a, ref b) = &expr.node { if let ExprIndex(ref array, ref index) = &expr.node {
match &b.node { match &index.node {
// Both ExprStruct and ExprPath require this approach's checks // Both ExprStruct and ExprPath require this approach's checks
// on the `range` returned by `higher::range(cx, b)`. // on the `range` returned by `higher::range(cx, index)`.
// ExprStruct handles &x[n..m], &x[n..] and &x[..n]. // ExprStruct handles &x[n..m], &x[n..] and &x[..n].
// ExprPath handles &x[..] and x[var] // ExprPath handles &x[..] and x[var]
ExprStruct(_, _, _) | ExprPath(_) => { ExprStruct(..) | ExprPath(..) => {
if let Some(range) = higher::range(cx, b) { if let Some(range) = higher::range(cx, index) {
let ty = cx.tables.expr_ty(a); let ty = cx.tables.expr_ty(array);
if let ty::TyArray(_, s) = ty.sty { if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant range. // Index is a constant range.
@ -100,49 +100,48 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass {
} }
} }
} }
let help_msg;
match (range.start, range.end) { match (range.start, range.end) {
(None, Some(_)) => { (None, Some(_)) => {
cx.span_lint( help_msg = "Consider using `.get(..n)`or `.get_mut(..n)` instead";
INDEXING_SLICING,
expr.span,
"slicing may panic. Consider using \
`.get(..n)`or `.get_mut(..n)` instead",
);
} }
(Some(_), None) => { (Some(_), None) => {
cx.span_lint( help_msg = "Consider using `.get(n..)` or .get_mut(n..)` instead";
INDEXING_SLICING,
expr.span,
"slicing may panic. Consider using \
`.get(n..)` or .get_mut(n..)` instead",
);
} }
(Some(_), Some(_)) => { (Some(_), Some(_)) => {
cx.span_lint( help_msg =
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead";
}
(None, None) => return, // [..] is ok
}
utils::span_help_and_lint(
cx,
INDEXING_SLICING, INDEXING_SLICING,
expr.span, expr.span,
"slicing may panic. Consider using \ "slicing may panic.",
`.get(n..m)` or `.get_mut(n..m)` instead", help_msg,
); );
}
(None, None) => (),
}
} else { } else {
cx.span_lint( utils::span_help_and_lint(
cx,
INDEXING_SLICING, INDEXING_SLICING,
expr.span, expr.span,
"indexing may panic. Consider using `.get(n)` or \ "indexing may panic.",
`.get_mut(n)` instead", "Consider using `.get(n)` or `.get_mut(n)` instead",
); );
} }
} }
ExprLit(_) => { ExprLit(..) => {
// [n] // [n]
let ty = cx.tables.expr_ty(a); let ty = cx.tables.expr_ty(array);
if let ty::TyArray(_, s) = ty.sty { if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant uint. // Index is a constant uint.
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, b) { if let Some((Constant::Int(const_index), _)) =
constant(cx, cx.tables, index)
{
if size <= const_index { if size <= const_index {
utils::span_lint( utils::span_lint(
cx, cx,
@ -154,11 +153,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass {
// Else index is in bounds, ok. // Else index is in bounds, ok.
} }
} else { } else {
cx.span_lint( utils::span_help_and_lint(
cx,
INDEXING_SLICING, INDEXING_SLICING,
expr.span, expr.span,
"indexing may panic. Consider using `.get(n)` or \ "indexing may panic.",
`.get_mut(n)` instead", "Consider using `.get(n)` or `.get_mut(n)` instead",
); );
} }
} }

View file

@ -431,7 +431,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box unwrap::Pass); reg.register_late_lint_pass(box unwrap::Pass);
reg.register_late_lint_pass(box duration_subsec::DurationSubsec); reg.register_late_lint_pass(box duration_subsec::DurationSubsec);
reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess); reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess);
reg.register_late_lint_pass(box indexing_slicing::IndexingSlicingPass); reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing);
reg.register_lint_group("clippy_restriction", vec![ reg.register_lint_group("clippy_restriction", vec![
arithmetic::FLOAT_ARITHMETIC, arithmetic::FLOAT_ARITHMETIC,

View file

@ -21,6 +21,7 @@ fn main() {
&x[1..][..5]; &x[1..][..5];
&x[0..3]; &x[0..3];
&x[0..][..3]; &x[0..][..3];
&x[0..].get(..3); // Ok
&x[0..=4]; &x[0..=4];
&x[..=4]; &x[..=4];
&x[..]; &x[..];

View file

@ -1,40 +1,51 @@
error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead error: indexing may panic.
--> $DIR/indexing_slicing.rs:11:5 --> $DIR/indexing_slicing.rs:11:5
| |
11 | x[index]; 11 | x[index];
| ^^^^^^^^ | ^^^^^^^^
| |
= note: `-D indexing-slicing` implied by `-D warnings` = note: `-D indexing-slicing` implied by `-D warnings`
= help: Consider using `.get(n)` or `.get_mut(n)` instead
error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:12:6 --> $DIR/indexing_slicing.rs:12:6
| |
12 | &x[index_from..index_to]; 12 | &x[index_from..index_to];
| ^^^^^^^^^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^^^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:13:6 --> $DIR/indexing_slicing.rs:13:6
| |
13 | &x[index_from..][..index_to]; 13 | &x[index_from..][..index_to];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:13:6 --> $DIR/indexing_slicing.rs:13:6
| |
13 | &x[index_from..][..index_to]; 13 | &x[index_from..][..index_to];
| ^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:14:6 --> $DIR/indexing_slicing.rs:14:6
| |
14 | &x[index..]; 14 | &x[index..];
| ^^^^^^^^^^ | ^^^^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:15:6 --> $DIR/indexing_slicing.rs:15:6
| |
15 | &x[..index]; 15 | &x[..index];
| ^^^^^^^^^^ | ^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: const index is out of bounds error: const index is out of bounds
--> $DIR/indexing_slicing.rs:18:5 --> $DIR/indexing_slicing.rs:18:5
@ -50,173 +61,211 @@ error: range is out of bounds
20 | &x[1..5]; 20 | &x[1..5];
| ^^^^^^^ | ^^^^^^^
error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:20:6 --> $DIR/indexing_slicing.rs:20:6
| |
20 | &x[1..5]; 20 | &x[1..5];
| ^^^^^^^ | ^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:21:6 --> $DIR/indexing_slicing.rs:21:6
| |
21 | &x[1..][..5]; 21 | &x[1..][..5];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:23:6 --> $DIR/indexing_slicing.rs:23:6
| |
23 | &x[0..][..3]; 23 | &x[0..][..3];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:25:6 --> $DIR/indexing_slicing.rs:26:6
| |
25 | &x[..=4]; 26 | &x[..=4];
| ^^^^^^^ | ^^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:25:6 --> $DIR/indexing_slicing.rs:26:6
| |
25 | &x[..=4]; 26 | &x[..=4];
| ^^^^^^^ | ^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:29:6 --> $DIR/indexing_slicing.rs:30:6
| |
29 | &x[5..]; 30 | &x[5..];
| ^^^^^^ | ^^^^^^
error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:29:6 --> $DIR/indexing_slicing.rs:30:6
| |
29 | &x[5..]; 30 | &x[5..];
| ^^^^^^ | ^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:31:6 --> $DIR/indexing_slicing.rs:32:6
| |
31 | &x[..5]; 32 | &x[..5];
| ^^^^^^ | ^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:31:6 --> $DIR/indexing_slicing.rs:32:6
| |
31 | &x[..5]; 32 | &x[..5];
| ^^^^^^ | ^^^^^^
error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead
--> $DIR/indexing_slicing.rs:34:5
| |
34 | y[0]; = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: indexing may panic.
--> $DIR/indexing_slicing.rs:35:5
|
35 | y[0];
| ^^^^ | ^^^^
error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead
--> $DIR/indexing_slicing.rs:35:6
| |
35 | &y[1..2]; = help: Consider using `.get(n)` or `.get_mut(n)` instead
| ^^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:38:6 --> $DIR/indexing_slicing.rs:36:6
| |
38 | &y[..=4]; 36 | &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
|
39 | &y[..=4];
| ^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: const index is out of bounds error: const index is out of bounds
--> $DIR/indexing_slicing.rs:41:5 --> $DIR/indexing_slicing.rs:42:5
| |
41 | empty[0]; 42 | empty[0];
| ^^^^^^^^ | ^^^^^^^^
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:42:6 --> $DIR/indexing_slicing.rs:43:6
| |
42 | &empty[1..5]; 43 | &empty[1..5];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:42:6 --> $DIR/indexing_slicing.rs:43:6
| |
42 | &empty[1..5]; 43 | &empty[1..5];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:44:6 --> $DIR/indexing_slicing.rs:45:6
| |
44 | &empty[..=4]; 45 | &empty[..=4];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:44:6 --> $DIR/indexing_slicing.rs:45:6
| |
44 | &empty[..=4]; 45 | &empty[..=4];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:49:6 --> $DIR/indexing_slicing.rs:50:6
| |
49 | &empty[..=0]; 50 | &empty[..=0];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:49:6 --> $DIR/indexing_slicing.rs:50:6
| |
49 | &empty[..=0]; 50 | &empty[..=0];
| ^^^^^^^^^^^ | ^^^^^^^^^^^
error: range is out of bounds
--> $DIR/indexing_slicing.rs:51:6
| |
51 | &empty[1..]; = help: Consider using `.get(..n)`or `.get_mut(..n)` instead
| ^^^^^^^^^^
error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead
--> $DIR/indexing_slicing.rs:51:6
|
51 | &empty[1..];
| ^^^^^^^^^^
error: range is out of bounds error: range is out of bounds
--> $DIR/indexing_slicing.rs:52:6 --> $DIR/indexing_slicing.rs:52:6
| |
52 | &empty[..4]; 52 | &empty[1..];
| ^^^^^^^^^^ | ^^^^^^^^^^
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:52:6 --> $DIR/indexing_slicing.rs:52:6
| |
52 | &empty[..4]; 52 | &empty[1..];
| ^^^^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
error: range is out of bounds
--> $DIR/indexing_slicing.rs:53:6
|
53 | &empty[..4];
| ^^^^^^^^^^ | ^^^^^^^^^^
error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:55:5 --> $DIR/indexing_slicing.rs:53:6
| |
55 | v[0]; 53 | &empty[..4];
| ^^^^ | ^^^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: indexing may panic. Consider using `.get(n)` or `.get_mut(n)` instead error: indexing may panic.
--> $DIR/indexing_slicing.rs:56:5 --> $DIR/indexing_slicing.rs:56:5
| |
56 | v[10]; 56 | v[0];
| ^^^^^ | ^^^^
error: slicing may panic. Consider using `.get(n..m)` or `.get_mut(n..m)` instead
--> $DIR/indexing_slicing.rs:57:6
| |
57 | &v[10..100]; = help: Consider using `.get(n)` or `.get_mut(n)` instead
| ^^^^^^^^^^
error: slicing may panic. Consider using `.get(n..)` or .get_mut(n..)` instead error: indexing may panic.
--> $DIR/indexing_slicing.rs:57:5
|
57 | 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:58:6
| |
58 | &v[10..]; 58 | &v[10..100];
| ^^^^^^^ | ^^^^^^^^^^
|
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)` instead error: slicing may panic.
--> $DIR/indexing_slicing.rs:59:6 --> $DIR/indexing_slicing.rs:59:6
| |
59 | &v[..100]; 59 | &v[10..];
| ^^^^^^^
|
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
error: slicing may panic.
--> $DIR/indexing_slicing.rs:60:6
|
60 | &v[..100];
| ^^^^^^^^ | ^^^^^^^^
|
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
error: aborting due to 36 previous errors error: aborting due to 36 previous errors