Skip to content

Commit 541d0c8

Browse files
y21y21
committed
[slow_vector_initialization]: lint Vec::new() 
1 parent 7a34143 commit 541d0c8

File tree

5 files changed

+145
-57
lines changed

5 files changed

+145
-57
lines changed

‎clippy_lints/src/slow_vector_initialization.rs‎

Lines changed: 70 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::sugg::Sugg;
33
use clippy_utils::ty::is_type_diagnostic_item;
44
use clippy_utils::{
5-
get_enclosing_block, is_integer_literal, is_path_diagnostic_item, path_to_local, path_to_local_id, SpanlessEq,
5+
get_enclosing_block, is_expr_path_def_path, is_integer_literal, is_path_diagnostic_item, path_to_local,
6+
path_to_local_id, paths, SpanlessEq,
67
};
78
use if_chain::if_chain;
89
use rustc_errors::Applicability;
@@ -60,7 +61,11 @@ struct VecAllocation<'tcx> {
6061

6162
/// Reference to the expression used as argument on `with_capacity` call. This is used
6263
/// to only match slow zero-filling idioms of the same length than vector initialization.
63-
len_expr: &'tcx Expr<'tcx>,
64+
///
65+
/// Initially set to `None` if initialized with `Vec::new()`, but will always be `Some(_)` by
66+
/// the time the visitor has looked through the enclosing block and found a slow
67+
/// initialization, so it is safe to unwrap later at lint time.
68+
size_expr: Option<&'tcx Expr<'tcx>>,
6469
}
6570

6671
/// Type of slow initialization
@@ -77,18 +82,14 @@ impl<'tcx> LateLintPass<'tcx> for SlowVectorInit {
7782
// Matches initialization on reassignments. For example: `vec = Vec::with_capacity(100)`
7883
if_chain! {
7984
if let ExprKind::Assign(left, right, _) = expr.kind;
80-
81-
// Extract variable
8285
if let Some(local_id) = path_to_local(left);
83-
84-
// Extract len argument
85-
if let Some(len_arg) = Self::is_vec_with_capacity(cx, right);
86+
if let Some(size_expr) = Self::as_vec_initializer(cx, right);
8687

8788
then {
8889
let vi = VecAllocation {
8990
local_id,
9091
allocation_expr: right,
91-
len_expr: len_arg,
92+
size_expr,
9293
};
9394

9495
Self::search_initialization(cx, vi, expr.hir_id);
@@ -98,17 +99,18 @@ impl<'tcx> LateLintPass<'tcx> for SlowVectorInit {
9899

99100
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
100101
// Matches statements which initializes vectors. For example: `let mut vec = Vec::with_capacity(10)`
102+
// or `Vec::new()`
101103
if_chain! {
102104
if let StmtKind::Local(local) = stmt.kind;
103105
if let PatKind::Binding(BindingAnnotation::MUT, local_id, _, None) = local.pat.kind;
104106
if let Some(init) = local.init;
105-
if let Some(len_arg) = Self::is_vec_with_capacity(cx, init);
107+
if let Some(size_expr) = Self::as_vec_initializer(cx, init);
106108

107109
then {
108110
let vi = VecAllocation {
109111
local_id,
110112
allocation_expr: init,
111-
len_expr: len_arg,
113+
size_expr,
112114
};
113115

114116
Self::search_initialization(cx, vi, stmt.hir_id);
@@ -118,6 +120,25 @@ impl<'tcx> LateLintPass<'tcx> for SlowVectorInit {
118120
}
119121

120122
impl SlowVectorInit {
123+
/// Given an expression, returns:
124+
/// - `Some(Some(size))` if it is a function call to `Vec::with_capacity(size)`
125+
/// - `Some(None)` if it is a call to `Vec::new()`
126+
/// - `None` if it is neither
127+
#[allow(
128+
clippy::option_option,
129+
reason = "outer option is immediately removed at call site and the inner option is kept around, \
130+
so extracting into a dedicated enum seems unnecessarily complicated"
131+
)]
132+
fn as_vec_initializer<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<Option<&'tcx Expr<'tcx>>> {
133+
if let Some(len_expr) = Self::is_vec_with_capacity(cx, expr) {
134+
Some(Some(len_expr))
135+
} else if Self::is_vec_new(cx, expr) {
136+
Some(None)
137+
} else {
138+
None
139+
}
140+
}
141+
121142
/// Checks if the given expression is `Vec::with_capacity(..)`. It will return the expression
122143
/// of the first argument of `with_capacity` call if it matches or `None` if it does not.
123144
fn is_vec_with_capacity<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
@@ -134,6 +155,14 @@ impl SlowVectorInit {
134155
}
135156
}
136157

158+
/// Checks if the given expression is `Vec::new()`
159+
fn is_vec_new(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
160+
matches!(
161+
expr.kind,
162+
ExprKind::Call(func, _) if is_expr_path_def_path(cx, func, &paths::VEC_NEW)
163+
)
164+
}
165+
137166
/// Search initialization for the given vector
138167
fn search_initialization<'tcx>(cx: &LateContext<'tcx>, vec_alloc: VecAllocation<'tcx>, parent_node: HirId) {
139168
let enclosing_body = get_enclosing_block(cx, parent_node);
@@ -169,12 +198,18 @@ impl SlowVectorInit {
169198
}
170199

171200
fn emit_lint(cx: &LateContext<'_>, slow_fill: &Expr<'_>, vec_alloc: &VecAllocation<'_>, msg: &str) {
172-
let len_expr = Sugg::hir(cx, vec_alloc.len_expr, "len");
201+
let len_expr = Sugg::hir(
202+
cx,
203+
vec_alloc
204+
.size_expr
205+
.expect("length expression must be set by this point"),
206+
"len",
207+
);
173208

174209
span_lint_and_then(cx, SLOW_VECTOR_INITIALIZATION, slow_fill.span, msg, |diag| {
175210
diag.span_suggestion(
176211
vec_alloc.allocation_expr.span,
177-
"consider replace allocation with",
212+
"consider replacing this with",
178213
format!("vec![0; {len_expr}]"),
179214
Applicability::Unspecified,
180215
);
@@ -214,36 +249,45 @@ impl<'a, 'tcx> VectorInitializationVisitor<'a, 'tcx> {
214249
}
215250

216251
/// Checks if the given expression is resizing a vector with 0
217-
fn search_slow_resize_filling(&mut self, expr: &'tcx Expr<'_>) {
252+
fn search_slow_resize_filling(&mut self, expr: &'tcx Expr<'tcx>) {
218253
if self.initialization_found
219254
&& let ExprKind::MethodCall(path, self_arg, [len_arg, fill_arg], _) = expr.kind
220255
&& path_to_local_id(self_arg, self.vec_alloc.local_id)
221256
&& path.ident.name == sym!(resize)
222257
// Check that is filled with 0
223-
&& is_integer_literal(fill_arg, 0) {
224-
// Check that len expression is equals to `with_capacity` expression
225-
if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr) {
226-
self.slow_expression = Some(InitializationType::Resize(expr));
227-
} else if let ExprKind::MethodCall(path, ..) = len_arg.kind && path.ident.as_str() == "capacity" {
228-
self.slow_expression = Some(InitializationType::Resize(expr));
229-
}
258+
&& is_integer_literal(fill_arg, 0)
259+
{
260+
let is_matching_resize = if let Some(size_expr) = self.vec_alloc.size_expr {
261+
// If we have a size expression, check that it is equal to what's passed to `resize`
262+
SpanlessEq::new(self.cx).eq_expr(len_arg, size_expr)
263+
|| matches!(len_arg.kind, ExprKind::MethodCall(path, ..) if path.ident.as_str() == "capacity")
264+
} else {
265+
self.vec_alloc.size_expr = Some(len_arg);
266+
true
267+
};
268+
269+
if is_matching_resize {
270+
self.slow_expression = Some(InitializationType::Resize(expr));
230271
}
272+
}
231273
}
232274

233275
/// Returns `true` if give expression is `repeat(0).take(...)`
234-
fn is_repeat_take(&self, expr: &Expr<'_>) -> bool {
276+
fn is_repeat_take(&mut self, expr: &'tcx Expr<'tcx>) -> bool {
235277
if_chain! {
236278
if let ExprKind::MethodCall(take_path, recv, [len_arg, ..], _) = expr.kind;
237279
if take_path.ident.name == sym!(take);
238280
// Check that take is applied to `repeat(0)`
239281
if self.is_repeat_zero(recv);
240282
then {
241-
// Check that len expression is equals to `with_capacity` expression
242-
if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr) {
243-
return true;
244-
} else if let ExprKind::MethodCall(path, ..) = len_arg.kind && path.ident.as_str() == "capacity" {
245-
return true;
283+
if let Some(size_expr) = self.vec_alloc.size_expr {
284+
// Check that len expression is equals to `with_capacity` expression
285+
return SpanlessEq::new(self.cx).eq_expr(len_arg, size_expr)
286+
|| matches!(len_arg.kind, ExprKind::MethodCall(path, ..) if path.ident.as_str() == "capacity")
246287
}
288+
289+
self.vec_alloc.size_expr = Some(len_arg);
290+
return true;
247291
}
248292
}
249293

‎tests/ui/read_zero_byte_vec.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#![warn(clippy::read_zero_byte_vec)]
2-
#![allow(clippy::unused_io_amount, clippy::needless_pass_by_ref_mut)]
2+
#![allow(
3+
clippy::unused_io_amount,
4+
clippy::needless_pass_by_ref_mut,
5+
clippy::slow_vector_initialization
6+
)]
37
use std::fs::File;
48
use std::io;
59
use std::io::prelude::*;

‎tests/ui/read_zero_byte_vec.stderr‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,61 @@
11
error: reading zero byte data to `Vec`
2-
--> $DIR/read_zero_byte_vec.rs:17:5
2+
--> $DIR/read_zero_byte_vec.rs:21:5
33
|
44
LL | f.read_exact(&mut data).unwrap();
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data.resize(20, 0); f.read_exact(&mut data).unwrap();`
66
|
77
= note: `-D clippy::read-zero-byte-vec` implied by `-D warnings`
88

99
error: reading zero byte data to `Vec`
10-
--> $DIR/read_zero_byte_vec.rs:21:5
10+
--> $DIR/read_zero_byte_vec.rs:25:5
1111
|
1212
LL | f.read_exact(&mut data2)?;
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data2.resize(cap, 0); f.read_exact(&mut data2)?;`
1414

1515
error: reading zero byte data to `Vec`
16-
--> $DIR/read_zero_byte_vec.rs:25:5
16+
--> $DIR/read_zero_byte_vec.rs:29:5
1717
|
1818
LL | f.read_exact(&mut data3)?;
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
2020

2121
error: reading zero byte data to `Vec`
22-
--> $DIR/read_zero_byte_vec.rs:29:5
22+
--> $DIR/read_zero_byte_vec.rs:33:5
2323
|
2424
LL | let _ = f.read(&mut data4)?;
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2626

2727
error: reading zero byte data to `Vec`
28-
--> $DIR/read_zero_byte_vec.rs:34:9
28+
--> $DIR/read_zero_byte_vec.rs:38:9
2929
|
3030
LL | f.read(&mut data5)
3131
| ^^^^^^^^^^^^^^^^^^
3232

3333
error: reading zero byte data to `Vec`
34-
--> $DIR/read_zero_byte_vec.rs:40:9
34+
--> $DIR/read_zero_byte_vec.rs:44:9
3535
|
3636
LL | f.read(&mut data6)
3737
| ^^^^^^^^^^^^^^^^^^
3838

3939
error: reading zero byte data to `Vec`
40-
--> $DIR/read_zero_byte_vec.rs:70:5
40+
--> $DIR/read_zero_byte_vec.rs:74:5
4141
|
4242
LL | r.read(&mut data).await.unwrap();
4343
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4444

4545
error: reading zero byte data to `Vec`
46-
--> $DIR/read_zero_byte_vec.rs:74:5
46+
--> $DIR/read_zero_byte_vec.rs:78:5
4747
|
4848
LL | r.read_exact(&mut data2).await.unwrap();
4949
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5050

5151
error: reading zero byte data to `Vec`
52-
--> $DIR/read_zero_byte_vec.rs:80:5
52+
--> $DIR/read_zero_byte_vec.rs:84:5
5353
|
5454
LL | r.read(&mut data).await.unwrap();
5555
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5656

5757
error: reading zero byte data to `Vec`
58-
--> $DIR/read_zero_byte_vec.rs:84:5
58+
--> $DIR/read_zero_byte_vec.rs:88:5
5959
|
6060
LL | r.read_exact(&mut data2).await.unwrap();
6161
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

‎tests/ui/slow_vector_initialization.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ fn main() {
44
resize_vector();
55
extend_vector();
66
mixed_extend_resize_vector();
7+
from_empty_vec();
78
}
89

910
fn extend_vector() {
@@ -59,6 +60,21 @@ fn resize_vector() {
5960
vec1.resize(10, 0);
6061
}
6162

63+
fn from_empty_vec() {
64+
// Resize with constant expression
65+
let len = 300;
66+
let mut vec1 = Vec::new();
67+
vec1.resize(len, 0);
68+
69+
// Resize with len expression
70+
let mut vec3 = Vec::new();
71+
vec3.resize(len - 10, 0);
72+
73+
// Reinitialization should be warned
74+
vec1 = Vec::new();
75+
vec1.resize(10, 0);
76+
}
77+
6278
fn do_stuff(vec: &mut [u8]) {}
6379

6480
fn extend_vector_with_manipulations_between() {

0 commit comments

Comments
 (0)