Skip to content

Commit 4ec439b

Browse files
shnewtoshnewto
committed
Revisiting indexing_slicing test cases
This commit contains a few changes. In an attempt to clarify which test cases should and should not produce stderr it became clear that some cases were being handled incorrectly. In order to address these test cases, a minor re-factor was made to the linting logic itself. The re-factor was driven by edge case handling including a need for additional match conditions for `ExprCall` (`&x[0..=4]`) and `ExprBinary` (`x[1 << 3]`). Rather than attempt to account for each potential `Expr*` the code was re-factored into simply "if ranged index" and an "otherwise" conditions.
1 parent 8b59542 commit 4ec439b

File tree

3 files changed

+220
-167
lines changed

3 files changed

+220
-167
lines changed

‎clippy_lints/src/indexing_slicing.rs

Lines changed: 55 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
//! lint on indexing and slicing operations
22
33
use crate::consts::{constant, Constant};
4+
use crate::utils;
5+
use crate::utils::higher;
46
use crate::utils::higher::Range;
5-
use crate::utils::{self, higher};
67
use rustc::hir::*;
78
use rustc::lint::*;
89
use rustc::ty;
@@ -97,89 +98,65 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
9798
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
9899
if let ExprIndex(ref array, ref index) = &expr.node {
99100
let ty = cx.tables.expr_ty(array);
100-
match &index.node {
101-
// Both ExprStruct and ExprPath require this approach's checks
102-
// on the `range` returned by `higher::range(cx, index)`.
103-
// ExprStruct handles &x[n..m], &x[n..] and &x[..n].
104-
// ExprPath handles &x[..] and x[var]
105-
ExprStruct(..) | ExprPath(..) => {
106-
if let Some(range) = higher::range(cx, index) {
107-
if let ty::TyArray(_, s) = ty.sty {
108-
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
109-
// Index is a constant range.
110-
if let Some((start, end)) = to_const_range(cx, range, size) {
111-
if start > size || end > size {
112-
utils::span_lint(
113-
cx,
114-
OUT_OF_BOUNDS_INDEXING,
115-
expr.span,
116-
"range is out of bounds",
117-
);
118-
} // Else range is in bounds, ok.
119-
120-
return;
121-
}
101+
if let Some(range) = higher::range(cx, index) {
102+
// Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..]
103+
if let ty::TyArray(_, s) = ty.sty {
104+
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
105+
// Index is a constant range.
106+
if let Some((start, end)) = to_const_range(cx, range, size) {
107+
if start > size || end > size {
108+
utils::span_lint(
109+
cx,
110+
OUT_OF_BOUNDS_INDEXING,
111+
expr.span,
112+
"range is out of bounds",
113+
);
122114
}
123-
124-
let help_msg = match (range.start, range.end) {
125-
(None, Some(_)) => {
126-
"Consider using `.get(..n)`or `.get_mut(..n)` instead"
127-
}
128-
(Some(_), None) => {
129-
"Consider using `.get(n..)` or .get_mut(n..)` instead"
130-
}
131-
(Some(_), Some(_)) => {
132-
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead"
133-
}
134-
(None, None) => return, // [..] is ok.
135-
};
136-
137-
utils::span_help_and_lint(
138-
cx,
139-
INDEXING_SLICING,
140-
expr.span,
141-
"slicing may panic.",
142-
help_msg,
143-
);
144-
} else {
145-
utils::span_help_and_lint(
146-
cx,
147-
INDEXING_SLICING,
148-
expr.span,
149-
"indexing may panic.",
150-
"Consider using `.get(n)` or `.get_mut(n)` instead",
151-
);
115+
return;
152116
}
153117
}
154-
ExprLit(..) => {
155-
// [n]
156-
if let ty::TyArray(_, s) = ty.sty {
157-
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
158-
// Index is a constant uint.
159-
if let Some((Constant::Int(const_index), _)) =
160-
constant(cx, cx.tables, index)
161-
{
162-
if size <= const_index {
163-
utils::span_lint(
164-
cx,
165-
OUT_OF_BOUNDS_INDEXING,
166-
expr.span,
167-
"const index is out of bounds",
168-
);
169-
}
170-
// Else index is in bounds, ok.
118+
119+
let help_msg = match (range.start, range.end) {
120+
(None, Some(_)) => "Consider using `.get(..n)`or `.get_mut(..n)` instead",
121+
(Some(_), None) => "Consider using `.get(n..)` or .get_mut(n..)` instead",
122+
(Some(_), Some(_)) => "Consider using `.get(n..m)` or `.get_mut(n..m)` instead",
123+
(None, None) => return, // [..] is ok.
124+
};
125+
126+
utils::span_help_and_lint(
127+
cx,
128+
INDEXING_SLICING,
129+
expr.span,
130+
"slicing may panic.",
131+
help_msg,
132+
);
133+
} else {
134+
// Catchall non-range index, i.e. [n] or [n << m]
135+
if let ty::TyArray(_, s) = ty.sty {
136+
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
137+
// Index is a constant uint.
138+
if let Some((Constant::Int(const_index), _)) = constant(cx, cx.tables, index) {
139+
if size <= const_index {
140+
utils::span_lint(
141+
cx,
142+
OUT_OF_BOUNDS_INDEXING,
143+
expr.span,
144+
"const index is out of bounds",
145+
);
171146
}
172-
} else {
173-
utils::span_help_and_lint(
174-
cx,
175-
INDEXING_SLICING,
176-
expr.span,
177-
"indexing may panic.",
178-
"Consider using `.get(n)` or `.get_mut(n)` instead",
179-
);
147+
// Else index is in bounds, ok.
148+
149+
return;
180150
}
181151
}
182-
_ => (),
152+
153+
utils::span_help_and_lint(
154+
cx,
155+
INDEXING_SLICING,
156+
expr.span,
157+
"indexing may panic.",
158+
"Consider using `.get(n)` or `.get_mut(n)` instead",
159+
);
183160
}
184161
}
185162
}

‎tests/ui/indexing_slicing.rs

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,55 +9,63 @@ fn main() {
99
let index_from: usize = 2;
1010
let index_to: usize = 3;
1111
x[index];
12-
&x[index_from..index_to];
13-
&x[index_from..][..index_to];
1412
&x[index..];
1513
&x[..index];
16-
x[0];
17-
x[3];
14+
&x[index_from..index_to];
15+
&x[index_from..][..index_to]; // Two lint reports, one for [index_from..] and another for [..index_to].
1816
x[4];
1917
x[1 << 3];
20-
&x[1..5];
21-
&x[1..][..5];
22-
&x[0..3];
23-
&x[0..][..3];
24-
&x[0..].get(..3); // Ok
25-
&x[0..=4];
2618
&x[..=4];
27-
&x[..];
28-
&x[1..];
29-
&x[4..];
19+
&x[1..5];
20+
&x[5..][..10]; // Two lint reports, one for [5..] and another for [..10].
3021
&x[5..];
31-
&x[..4];
3222
&x[..5];
3323
&x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
34-
&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok
24+
&x[0..=4];
25+
&x[0..][..3];
26+
&x[1..][..5];
27+
28+
&x[4..]; // Ok, should not produce stderr.
29+
&x[..4]; // Ok, should not produce stderr.
30+
&x[..]; // Ok, should not produce stderr.
31+
&x[1..]; // Ok, should not produce stderr.
32+
&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok, should not produce stderr.
33+
&x[0..].get(..3); // Ok, should not produce stderr.
34+
x[0]; // Ok, should not produce stderr.
35+
x[3]; // Ok, should not produce stderr.
36+
&x[0..3]; // Ok, should not produce stderr.
3537

3638
let y = &x;
3739
y[0];
3840
&y[1..2];
39-
&y[..];
4041
&y[0..=4];
4142
&y[..=4];
4243

44+
&y[..]; // Ok, should not produce stderr.
45+
4346
let empty: [i8; 0] = [];
4447
empty[0];
4548
&empty[1..5];
4649
&empty[0..=4];
4750
&empty[..=4];
48-
&empty[..];
49-
&empty[0..];
50-
&empty[0..0];
51-
&empty[0..=0];
52-
&empty[..=0];
53-
&empty[..0];
5451
&empty[1..];
5552
&empty[..4];
53+
&empty[0..=0];
54+
&empty[..=0];
55+
56+
&empty[0..]; // Ok, should not produce stderr.
57+
&empty[0..0]; // Ok, should not produce stderr.
58+
&empty[..0]; // Ok, should not produce stderr.
59+
&empty[..]; // Ok, should not produce stderr.
5660

5761
let v = vec![0; 5];
5862
v[0];
5963
v[10];
64+
v[1 << 3];
6065
&v[10..100];
66+
&x[10..][..100]; // Two lint reports, one for [10..] and another for [..100].
6167
&v[10..];
6268
&v[..100];
69+
70+
&v[..]; // Ok, should not produce stderr.
6371
}

0 commit comments

Comments
 (0)