Skip to content

Commit 8b59542

Browse files
shnewtoshnewto
committed
Second pass at addressing changes requested
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_.
1 parent a7c0ff3 commit 8b59542

File tree

3 files changed

+70
-117
lines changed

3 files changed

+70
-117
lines changed

‎clippy_lints/src/indexing_slicing.rs

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ declare_clippy_lint! {
3434
}
3535

3636
/// **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
37+
/// on arrays if we can tell that the indexing or slicing operations are in
3838
/// bounds.
3939
///
4040
/// **Why is this bad?** Indexing and slicing can panic at runtime and there are
@@ -44,18 +44,39 @@ declare_clippy_lint! {
4444
///
4545
/// **Example:**
4646
/// ```rust
47+
/// // Vector
4748
/// let x = vec![0; 5];
49+
///
4850
/// // Bad
4951
/// x[2];
5052
/// &x[2..100];
5153
/// &x[2..];
5254
/// &x[..100];
5355
///
5456
/// // Good
55-
/// x.get(2)
56-
/// x.get(2..100)
57-
/// x.get(2..)
58-
/// x.get(..100)
57+
/// x.get(2);
58+
/// x.get(2..100);
59+
/// x.get(2..);
60+
/// x.get(..100);
61+
///
62+
/// // Array
63+
/// let y = [0, 1, 2, 3];
64+
///
65+
/// // Bad
66+
/// y[10];
67+
/// &y[10..100];
68+
/// &y[10..];
69+
/// &y[..100];
70+
///
71+
/// // Good
72+
/// y[2];
73+
/// &y[2..];
74+
/// &y[..2];
75+
/// &y[0..3];
76+
/// y.get(10);
77+
/// y.get(10..100);
78+
/// y.get(10..);
79+
/// y.get(..100);
5980
/// ```
6081
declare_clippy_lint! {
6182
pub INDEXING_SLICING,
@@ -75,14 +96,14 @@ impl LintPass for IndexingSlicing {
7596
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
7697
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
7798
if let ExprIndex(ref array, ref index) = &expr.node {
99+
let ty = cx.tables.expr_ty(array);
78100
match &index.node {
79101
// Both ExprStruct and ExprPath require this approach's checks
80102
// on the `range` returned by `higher::range(cx, index)`.
81103
// ExprStruct handles &x[n..m], &x[n..] and &x[..n].
82104
// ExprPath handles &x[..] and x[var]
83105
ExprStruct(..) | ExprPath(..) => {
84106
if let Some(range) = higher::range(cx, index) {
85-
let ty = cx.tables.expr_ty(array);
86107
if let ty::TyArray(_, s) = ty.sty {
87108
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
88109
// Index is a constant range.
@@ -94,27 +115,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
94115
expr.span,
95116
"range is out of bounds",
96117
);
97-
} else {
98-
// Range is in bounds, ok.
99-
return;
100-
}
118+
} // Else range is in bounds, ok.
119+
120+
return;
101121
}
102122
}
103123

104-
let help_msg;
105-
match (range.start, range.end) {
124+
let help_msg = match (range.start, range.end) {
106125
(None, Some(_)) => {
107-
help_msg = "Consider using `.get(..n)`or `.get_mut(..n)` instead";
126+
"Consider using `.get(..n)`or `.get_mut(..n)` instead"
108127
}
109128
(Some(_), None) => {
110-
help_msg = "Consider using `.get(n..)` or .get_mut(n..)` instead";
129+
"Consider using `.get(n..)` or .get_mut(n..)` instead"
111130
}
112131
(Some(_), Some(_)) => {
113-
help_msg =
114-
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead";
132+
"Consider using `.get(n..m)` or `.get_mut(n..m)` instead"
115133
}
116-
(None, None) => return, // [..] is ok
117-
}
134+
(None, None) => return, // [..] is ok.
135+
};
118136

119137
utils::span_help_and_lint(
120138
cx,
@@ -135,7 +153,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing {
135153
}
136154
ExprLit(..) => {
137155
// [n]
138-
let ty = cx.tables.expr_ty(array);
139156
if let ty::TyArray(_, s) = ty.sty {
140157
let size: u128 = s.assert_usize(cx.tcx).unwrap().into();
141158
// Index is a constant uint.

‎tests/ui/indexing_slicing.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ fn main() {
3030
&x[5..];
3131
&x[..4];
3232
&x[..5];
33+
&x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>();
34+
&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>(); // Ok
3335

3436
let y = &x;
3537
y[0];

‎tests/ui/indexing_slicing.stderr

Lines changed: 32 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,6 @@ error: range is out of bounds
6161
20 | &x[1..5];
6262
| ^^^^^^^
6363

64-
error: slicing may panic.
65-
--> $DIR/indexing_slicing.rs:20:6
66-
|
67-
20 | &x[1..5];
68-
| ^^^^^^^
69-
|
70-
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
71-
7264
error: slicing may panic.
7365
--> $DIR/indexing_slicing.rs:21:6
7466
|
@@ -91,181 +83,123 @@ error: range is out of bounds
9183
26 | &x[..=4];
9284
| ^^^^^^^
9385

94-
error: slicing may panic.
95-
--> $DIR/indexing_slicing.rs:26:6
96-
|
97-
26 | &x[..=4];
98-
| ^^^^^^^
99-
|
100-
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
101-
10286
error: range is out of bounds
10387
--> $DIR/indexing_slicing.rs:30:6
10488
|
10589
30 | &x[5..];
10690
| ^^^^^^
10791

108-
error: slicing may panic.
109-
--> $DIR/indexing_slicing.rs:30:6
110-
|
111-
30 | &x[5..];
112-
| ^^^^^^
113-
|
114-
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
115-
11692
error: range is out of bounds
11793
--> $DIR/indexing_slicing.rs:32:6
11894
|
11995
32 | &x[..5];
12096
| ^^^^^^
12197

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

130104
error: indexing may panic.
131-
--> $DIR/indexing_slicing.rs:35:5
105+
--> $DIR/indexing_slicing.rs:37:5
132106
|
133-
35 | y[0];
107+
37 | y[0];
134108
| ^^^^
135109
|
136110
= help: Consider using `.get(n)` or `.get_mut(n)` instead
137111

138112
error: slicing may panic.
139-
--> $DIR/indexing_slicing.rs:36:6
113+
--> $DIR/indexing_slicing.rs:38:6
140114
|
141-
36 | &y[1..2];
115+
38 | &y[1..2];
142116
| ^^^^^^^
143117
|
144118
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
145119

146120
error: slicing may panic.
147-
--> $DIR/indexing_slicing.rs:39:6
121+
--> $DIR/indexing_slicing.rs:41:6
148122
|
149-
39 | &y[..=4];
123+
41 | &y[..=4];
150124
| ^^^^^^^
151125
|
152126
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
153127

154128
error: const index is out of bounds
155-
--> $DIR/indexing_slicing.rs:42:5
129+
--> $DIR/indexing_slicing.rs:44:5
156130
|
157-
42 | empty[0];
131+
44 | empty[0];
158132
| ^^^^^^^^
159133

160-
error: range is out of bounds
161-
--> $DIR/indexing_slicing.rs:43:6
162-
|
163-
43 | &empty[1..5];
164-
| ^^^^^^^^^^^
165-
166-
error: slicing may panic.
167-
--> $DIR/indexing_slicing.rs:43:6
168-
|
169-
43 | &empty[1..5];
170-
| ^^^^^^^^^^^
171-
|
172-
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
173-
174134
error: range is out of bounds
175135
--> $DIR/indexing_slicing.rs:45:6
176136
|
177-
45 | &empty[..=4];
137+
45 | &empty[1..5];
178138
| ^^^^^^^^^^^
179139

180-
error: slicing may panic.
181-
--> $DIR/indexing_slicing.rs:45:6
182-
|
183-
45 | &empty[..=4];
184-
| ^^^^^^^^^^^
185-
|
186-
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
187-
188140
error: range is out of bounds
189-
--> $DIR/indexing_slicing.rs:50:6
190-
|
191-
50 | &empty[..=0];
192-
| ^^^^^^^^^^^
193-
194-
error: slicing may panic.
195-
--> $DIR/indexing_slicing.rs:50:6
141+
--> $DIR/indexing_slicing.rs:47:6
196142
|
197-
50 | &empty[..=0];
143+
47 | &empty[..=4];
198144
| ^^^^^^^^^^^
199-
|
200-
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
201145

202146
error: range is out of bounds
203147
--> $DIR/indexing_slicing.rs:52:6
204148
|
205-
52 | &empty[1..];
206-
| ^^^^^^^^^^
207-
208-
error: slicing may panic.
209-
--> $DIR/indexing_slicing.rs:52:6
210-
|
211-
52 | &empty[1..];
212-
| ^^^^^^^^^^
213-
|
214-
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
149+
52 | &empty[..=0];
150+
| ^^^^^^^^^^^
215151

216152
error: range is out of bounds
217-
--> $DIR/indexing_slicing.rs:53:6
153+
--> $DIR/indexing_slicing.rs:54:6
218154
|
219-
53 | &empty[..4];
155+
54 | &empty[1..];
220156
| ^^^^^^^^^^
221157

222-
error: slicing may panic.
223-
--> $DIR/indexing_slicing.rs:53:6
158+
error: range is out of bounds
159+
--> $DIR/indexing_slicing.rs:55:6
224160
|
225-
53 | &empty[..4];
161+
55 | &empty[..4];
226162
| ^^^^^^^^^^
227-
|
228-
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
229163

230164
error: indexing may panic.
231-
--> $DIR/indexing_slicing.rs:56:5
165+
--> $DIR/indexing_slicing.rs:58:5
232166
|
233-
56 | v[0];
167+
58 | v[0];
234168
| ^^^^
235169
|
236170
= help: Consider using `.get(n)` or `.get_mut(n)` instead
237171

238172
error: indexing may panic.
239-
--> $DIR/indexing_slicing.rs:57:5
173+
--> $DIR/indexing_slicing.rs:59:5
240174
|
241-
57 | v[10];
175+
59 | v[10];
242176
| ^^^^^
243177
|
244178
= help: Consider using `.get(n)` or `.get_mut(n)` instead
245179

246180
error: slicing may panic.
247-
--> $DIR/indexing_slicing.rs:58:6
181+
--> $DIR/indexing_slicing.rs:60:6
248182
|
249-
58 | &v[10..100];
183+
60 | &v[10..100];
250184
| ^^^^^^^^^^
251185
|
252186
= help: Consider using `.get(n..m)` or `.get_mut(n..m)` instead
253187

254188
error: slicing may panic.
255-
--> $DIR/indexing_slicing.rs:59:6
189+
--> $DIR/indexing_slicing.rs:61:6
256190
|
257-
59 | &v[10..];
191+
61 | &v[10..];
258192
| ^^^^^^^
259193
|
260194
= help: Consider using `.get(n..)` or .get_mut(n..)` instead
261195

262196
error: slicing may panic.
263-
--> $DIR/indexing_slicing.rs:60:6
197+
--> $DIR/indexing_slicing.rs:62:6
264198
|
265-
60 | &v[..100];
199+
62 | &v[..100];
266200
| ^^^^^^^^
267201
|
268202
= help: Consider using `.get(..n)`or `.get_mut(..n)` instead
269203

270-
error: aborting due to 36 previous errors
204+
error: aborting due to 28 previous errors
271205

0 commit comments

Comments
 (0)