Extend `indexing_slicing` lint

Hey there clippy team! I've made some assumptions in this PR and I'm not at all certain they'll look like the right approach to you. I'm looking forward to any feedback or revision requests you have, thanks!

    Prior to this commit the `indexing_slicing` lint was limited to indexing/slicing operations on arrays. This meant that the scope of a really useful lint didn't include vectors. In order to include vectors in the `indexing_slicing` lint a few steps were taken.

    The `array_indexing.rs` source file in `clippy_lints` was renamed to `indexing_slicing.rs` to more accurately reflect the lint's new scope. The `OUT_OF_BOUNDS_INDEXING` lint persists through these changes so if we can know that a constant index or slice on an array is in bounds no lint is triggered.

    The `array_indexing` tests in the `tests/ui` directory were also extended and moved to `indexing_slicing.rs` and `indexing_slicing.stderr`.

    The `indexing_slicing` lint was moved to the `clippy_pedantic` lint group.

    A specific "Consider using" string was added to each of the `indexing_slicing` lint reports.

    At least one of the test scenarios might look peculiar and I'll leave it up to y'all to decide if it's palatable. It's the result of indexing the array `x` after `let x = [1, 2, 3, 4];`

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

    The error string reports only on the second half's range-to, because the range-from is in bounds!

    Again, thanks for taking a look.

    Closes #2536
This commit is contained in:
Shea Newton 2018-05-22 21:56:02 -07:00
parent c573186245
commit 7af0c67855
No known key found for this signature in database
GPG Key ID: 17EB9122DC958643
4 changed files with 321 additions and 126 deletions

View File

@ -1,3 +1,5 @@
//! lint on indexing and slicing operations
use crate::consts::{constant, Constant};
use crate::utils::higher::Range;
use crate::utils::{self, higher};
@ -16,9 +18,14 @@ use syntax::ast::RangeLimits;
/// **Example:**
/// ```rust
/// let x = [1,2,3,4];
/// ...
///
/// // Bad
/// x[9];
/// &x[2..9];
///
/// // Good
/// x[0];
/// x[3];
/// ```
declare_clippy_lint! {
pub OUT_OF_BOUNDS_INDEXING,
@ -26,19 +33,29 @@ declare_clippy_lint! {
"out of bounds constant indexing"
}
/// **What it does:** Checks for usage of indexing or slicing.
/// **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
/// bounds.
///
/// **Why is this bad?** Usually, this can be safely allowed. However, in some
/// domains such as kernel development, a panic can cause the whole operating
/// system to crash.
/// **Why is this bad?** Indexing and slicing can panic at runtime and there are
/// safe alternatives.
///
/// **Known problems:** Hopefully none.
///
/// **Example:**
/// ```rust
/// ...
/// let x = vec![0; 5];
/// // Bad
/// x[2];
/// &x[0..2];
/// &x[2..100];
/// &x[2..];
/// &x[..100];
///
/// // Good
/// x.get(2)
/// x.get(2..100)
/// x.get(2..)
/// x.get(..100)
/// ```
declare_clippy_lint! {
pub INDEXING_SLICING,
@ -47,52 +64,105 @@ declare_clippy_lint! {
}
#[derive(Copy, Clone)]
pub struct ArrayIndexing;
pub struct IndexingSlicingPass;
impl LintPass for ArrayIndexing {
impl LintPass for IndexingSlicingPass {
fn get_lints(&self) -> LintArray {
lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING)
}
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx hir::Expr) {
if let hir::ExprIndex(ref array, ref index) = e.node {
// Array with known size can be checked statically
let ty = cx.tables.expr_ty(array);
if let ty::TyArray(_, size) = ty.sty {
let size = size.assert_usize(cx.tcx).unwrap().into();
// Index is a constant uint
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, index) {
if size <= const_index {
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index is out of bounds");
}
return;
}
// Index is a constant range
if let Some(range) = higher::range(cx, index) {
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if let ExprIndex(ref a, ref b) = &expr.node {
match &b.node {
// Both ExprStruct and ExprPath require this approach's checks
// on the `range` returned by `higher::range(cx, b)`.
// 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, b) {
let ty = cx.tables.expr_ty(a);
if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant range.
if let Some((start, end)) = to_const_range(cx, range, size) {
if start > size || end > size {
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds");
}
return;
}
}
}
if let Some(range) = higher::range(cx, index) {
// Full ranges are always valid
if range.start.is_none() && range.end.is_none() {
return;
}
// Impossible to know if indexing or slicing is correct
utils::span_lint(cx, INDEXING_SLICING, e.span, "slicing may panic");
utils::span_lint(
cx,
OUT_OF_BOUNDS_INDEXING,
expr.span,
"range is out of bounds",
);
} else {
utils::span_lint(cx, INDEXING_SLICING, e.span, "indexing may panic");
// Range is in bounds, ok.
return;
}
}
}
match (range.start, range.end) {
(None, Some(_)) => {
cx.span_lint(
INDEXING_SLICING,
expr.span,
"slicing may panic. Consider using \
`.get(..n)`or `.get_mut(..n)` instead",
);
}
(Some(_), None) => {
cx.span_lint(
INDEXING_SLICING,
expr.span,
"slicing may panic. Consider using \
`.get(n..)` or .get_mut(n..)` instead",
);
}
(Some(_), Some(_)) => {
cx.span_lint(
INDEXING_SLICING,
expr.span,
"slicing may panic. Consider using \
`.get(n..m)` or `.get_mut(n..m)` instead",
);
}
(None, None) => (),
}
} else {
cx.span_lint(
INDEXING_SLICING,
expr.span,
"indexing may panic. Consider using `.get(n)` or \
`.get_mut(n)` instead",
);
}
}
ExprLit(_) => {
// [n]
let ty = cx.tables.expr_ty(a);
if let ty::TyArray(_, s) = ty.sty {
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
// Index is a constant uint.
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, b) {
if size <= const_index {
utils::span_lint(
cx,
OUT_OF_BOUNDS_INDEXING,
expr.span,
"const index is out of bounds",
);
}
// Else index is in bounds, ok.
}
} else {
cx.span_lint(
INDEXING_SLICING,
expr.span,
"indexing may panic. Consider using `.get(n)` or \
`.get_mut(n)` instead",
);
}
}
_ => (),
}
}
}
@ -100,15 +170,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing {
/// Returns an option containing a tuple with the start and end (exclusive) of
/// the range.
fn to_const_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, range: Range, array_size: u128) -> Option<(u128, u128)> {
let s = range.start.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
fn to_const_range<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
range: Range,
array_size: u128,
) -> Option<(u128, u128)> {
let s = range
.start
.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
let start = match s {
Some(Some(Constant::Int(x))) => x,
Some(_) => return None,
None => 0,
};
let e = range.end.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
let e = range
.end
.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
let end = match e {
Some(Some(Constant::Int(x))) => if range.limits == RangeLimits::Closed {
x + 1

View File

@ -355,8 +355,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
);
reg.register_late_lint_pass(box escape::Pass{too_large_for_stack: conf.too_large_for_stack});
reg.register_early_lint_pass(box misc_early::MiscEarly);
reg.register_late_lint_pass(box array_indexing::ArrayIndexing);
reg.register_late_lint_pass(box panic_unimplemented::Pass);
reg.register_late_lint_pass(box array_indexing::IndexingSlicingPass);
reg.register_late_lint_pass(box panic::Pass);
reg.register_late_lint_pass(box strings::StringLitAsBytes);
reg.register_late_lint_pass(box derive::Derive);
reg.register_late_lint_pass(box types::CharLitAsU8);

View File

@ -1,18 +1,26 @@
#![feature(plugin)]
#![warn(indexing_slicing)]
#![warn(out_of_bounds_indexing)]
#![allow(no_effect, unnecessary_operation)]
fn main() {
let x = [1, 2, 3, 4];
let index: usize = 1;
let index_from: usize = 2;
let index_to: usize = 3;
x[index];
&x[index_from..index_to];
&x[index_from..][..index_to];
&x[index..];
&x[..index];
x[0];
x[3];
x[4];
x[1 << 3];
&x[1..5];
&x[1..][..5];
&x[0..3];
&x[0..][..3];
&x[0..=4];
&x[..=4];
&x[..];
@ -42,4 +50,11 @@ fn main() {
&empty[..0];
&empty[1..];
&empty[..4];
let v = vec![0; 5];
v[0];
v[10];
&v[10..100];
&v[10..];
&v[..100];
}

View File

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