Skip to content

Commit 7af0c67

Browse files
shnewtoshnewto
committed
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
1 parent c573186 commit 7af0c67

File tree

4 files changed

+302
-107
lines changed

4 files changed

+302
-107
lines changed

‎clippy_lints/src/array_indexing.rs

Lines changed: 123 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
//! lint on indexing and slicing operations
2+
13
use crate::consts::{constant, Constant};
24
use crate::utils::higher::Range;
35
use crate::utils::{self, higher};
@@ -16,29 +18,44 @@ use syntax::ast::RangeLimits;
1618
/// **Example:**
1719
/// ```rust
1820
/// let x = [1,2,3,4];
19-
/// ...
21+
///
22+
/// // Bad
2023
/// x[9];
2124
/// &x[2..9];
25+
///
26+
/// // Good
27+
/// x[0];
28+
/// x[3];
2229
/// ```
2330
declare_clippy_lint! {
2431
pub OUT_OF_BOUNDS_INDEXING,
2532
correctness,
2633
"out of bounds constant indexing"
2734
}
2835

29-
/// **What it does:** Checks for usage of indexing or slicing.
36+
/// **What it does:** Checks for usage of indexing or slicing. Does not report
37+
/// if we can tell that the indexing or slicing operations on an array are in
38+
/// bounds.
3039
///
31-
/// **Why is this bad?** Usually, this can be safely allowed. However, in some
32-
/// domains such as kernel development, a panic can cause the whole operating
33-
/// system to crash.
40+
/// **Why is this bad?** Indexing and slicing can panic at runtime and there are
41+
/// safe alternatives.
3442
///
3543
/// **Known problems:** Hopefully none.
3644
///
3745
/// **Example:**
3846
/// ```rust
39-
/// ...
47+
/// let x = vec![0; 5];
48+
/// // Bad
4049
/// x[2];
41-
/// &x[0..2];
50+
/// &x[2..100];
51+
/// &x[2..];
52+
/// &x[..100];
53+
///
54+
/// // Good
55+
/// x.get(2)
56+
/// x.get(2..100)
57+
/// x.get(2..)
58+
/// x.get(..100)
4259
/// ```
4360
declare_clippy_lint! {
4461
pub INDEXING_SLICING,
@@ -47,68 +64,129 @@ declare_clippy_lint! {
4764
}
4865

4966
#[derive(Copy, Clone)]
50-
pub struct ArrayIndexing;
67+
pub struct IndexingSlicingPass;
5168

52-
impl LintPass for ArrayIndexing {
69+
impl LintPass for IndexingSlicingPass {
5370
fn get_lints(&self) -> LintArray {
5471
lint_array!(INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING)
5572
}
5673
}
5774

58-
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing {
59-
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx hir::Expr) {
60-
if let hir::ExprIndex(ref array, ref index) = e.node {
61-
// Array with known size can be checked statically
62-
let ty = cx.tables.expr_ty(array);
63-
if let ty::TyArray(_, size) = ty.sty {
64-
let size = size.assert_usize(cx.tcx).unwrap().into();
65-
66-
// Index is a constant uint
67-
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, index) {
68-
if size <= const_index {
69-
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index is out of bounds");
75+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass {
76+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
77+
if let ExprIndex(ref a, ref b) = &expr.node {
78+
match &b.node {
79+
// Both ExprStruct and ExprPath require this approach's checks
80+
// on the `range` returned by `higher::range(cx, b)`.
81+
// ExprStruct handles &x[n..m], &x[n..] and &x[..n].
82+
// ExprPath handles &x[..] and x[var]
83+
ExprStruct(_, _, _) | ExprPath(_) => {
84+
if let Some(range) = higher::range(cx, b) {
85+
let ty = cx.tables.expr_ty(a);
86+
if let ty::TyArray(_, s) = ty.sty {
87+
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
88+
// Index is a constant range.
89+
if let Some((start, end)) = to_const_range(cx, range, size) {
90+
if start > size || end > size {
91+
utils::span_lint(
92+
cx,
93+
OUT_OF_BOUNDS_INDEXING,
94+
expr.span,
95+
"range is out of bounds",
96+
);
97+
} else {
98+
// Range is in bounds, ok.
99+
return;
100+
}
101+
}
102+
}
103+
match (range.start, range.end) {
104+
(None, Some(_)) => {
105+
cx.span_lint(
106+
INDEXING_SLICING,
107+
expr.span,
108+
"slicing may panic. Consider using \
109+
`.get(..n)`or `.get_mut(..n)` instead",
110+
);
111+
}
112+
(Some(_), None) => {
113+
cx.span_lint(
114+
INDEXING_SLICING,
115+
expr.span,
116+
"slicing may panic. Consider using \
117+
`.get(n..)` or .get_mut(n..)` instead",
118+
);
119+
}
120+
(Some(_), Some(_)) => {
121+
cx.span_lint(
122+
INDEXING_SLICING,
123+
expr.span,
124+
"slicing may panic. Consider using \
125+
`.get(n..m)` or `.get_mut(n..m)` instead",
126+
);
127+
}
128+
(None, None) => (),
129+
}
130+
} else {
131+
cx.span_lint(
132+
INDEXING_SLICING,
133+
expr.span,
134+
"indexing may panic. Consider using `.get(n)` or \
135+
`.get_mut(n)` instead",
136+
);
70137
}
71-
72-
return;
73138
}
74-
75-
// Index is a constant range
76-
if let Some(range) = higher::range(cx, index) {
77-
if let Some((start, end)) = to_const_range(cx, range, size) {
78-
if start > size || end > size {
79-
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds");
139+
ExprLit(_) => {
140+
// [n]
141+
let ty = cx.tables.expr_ty(a);
142+
if let ty::TyArray(_, s) = ty.sty {
143+
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
144+
// Index is a constant uint.
145+
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, b) {
146+
if size <= const_index {
147+
utils::span_lint(
148+
cx,
149+
OUT_OF_BOUNDS_INDEXING,
150+
expr.span,
151+
"const index is out of bounds",
152+
);
153+
}
154+
// Else index is in bounds, ok.
80155
}
81-
return;
156+
} else {
157+
cx.span_lint(
158+
INDEXING_SLICING,
159+
expr.span,
160+
"indexing may panic. Consider using `.get(n)` or \
161+
`.get_mut(n)` instead",
162+
);
82163
}
83164
}
84-
}
85-
86-
if let Some(range) = higher::range(cx, index) {
87-
// Full ranges are always valid
88-
if range.start.is_none() && range.end.is_none() {
89-
return;
90-
}
91-
92-
// Impossible to know if indexing or slicing is correct
93-
utils::span_lint(cx, INDEXING_SLICING, e.span, "slicing may panic");
94-
} else {
95-
utils::span_lint(cx, INDEXING_SLICING, e.span, "indexing may panic");
165+
_ => (),
96166
}
97167
}
98168
}
99169
}
100170

101171
/// Returns an option containing a tuple with the start and end (exclusive) of
102172
/// the range.
103-
fn to_const_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, range: Range, array_size: u128) -> Option<(u128, u128)> {
104-
let s = range.start.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
173+
fn to_const_range<'a, 'tcx>(
174+
cx: &LateContext<'a, 'tcx>,
175+
range: Range,
176+
array_size: u128,
177+
) -> Option<(u128, u128)> {
178+
let s = range
179+
.start
180+
.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
105181
let start = match s {
106182
Some(Some(Constant::Int(x))) => x,
107183
Some(_) => return None,
108184
None => 0,
109185
};
110186

111-
let e = range.end.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
187+
let e = range
188+
.end
189+
.map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c));
112190
let end = match e {
113191
Some(Some(Constant::Int(x))) => if range.limits == RangeLimits::Closed {
114192
x + 1

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -355,8 +355,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
355355
);
356356
reg.register_late_lint_pass(box escape::Pass{too_large_for_stack: conf.too_large_for_stack});
357357
reg.register_early_lint_pass(box misc_early::MiscEarly);
358-
reg.register_late_lint_pass(box array_indexing::ArrayIndexing);
359-
reg.register_late_lint_pass(box panic_unimplemented::Pass);
358+
reg.register_late_lint_pass(box array_indexing::IndexingSlicingPass);
359+
reg.register_late_lint_pass(box panic::Pass);
360360
reg.register_late_lint_pass(box strings::StringLitAsBytes);
361361
reg.register_late_lint_pass(box derive::Derive);
362362
reg.register_late_lint_pass(box types::CharLitAsU8);

‎tests/ui/array_indexing.rs

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,26 @@
11
#![feature(plugin)]
2-
3-
42
#![warn(indexing_slicing)]
53
#![warn(out_of_bounds_indexing)]
64
#![allow(no_effect, unnecessary_operation)]
75

86
fn main() {
9-
let x = [1,2,3,4];
7+
let x = [1, 2, 3, 4];
8+
let index: usize = 1;
9+
let index_from: usize = 2;
10+
let index_to: usize = 3;
11+
x[index];
12+
&x[index_from..index_to];
13+
&x[index_from..][..index_to];
14+
&x[index..];
15+
&x[..index];
1016
x[0];
1117
x[3];
1218
x[4];
1319
x[1 << 3];
1420
&x[1..5];
21+
&x[1..][..5];
1522
&x[0..3];
23+
&x[0..][..3];
1624
&x[0..=4];
1725
&x[..=4];
1826
&x[..];
@@ -42,4 +50,11 @@ fn main() {
4250
&empty[..0];
4351
&empty[1..];
4452
&empty[..4];
53+
54+
let v = vec![0; 5];
55+
v[0];
56+
v[10];
57+
&v[10..100];
58+
&v[10..];
59+
&v[..100];
4560
}

0 commit comments

Comments
 (0)