Skip to content

[slow_vector_initialization]: catch Vec::new() followed by .resize(len, 0) #11198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 25, 2023
Merged

[slow_vector_initialization]: catch Vec::new() followed by .resize(len, 0) #11198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
541d0c8
[`slow_vector_initialization`]: lint `Vec::new()`
y21 Jul 20, 2023
1fe2762
use a dedicated enum for vec initializer
y21 Jul 25, 2023
c0484b7
simplify looking for `Vec::with_capacity` exprs
y21 Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 72 additions & 41 deletions clippy_lints/src/slow_vector_initialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{
get_enclosing_block, is_integer_literal, is_path_diagnostic_item, path_to_local, path_to_local_id, SpanlessEq,
get_enclosing_block, is_expr_path_def_path, is_integer_literal, is_path_diagnostic_item, path_to_local,
path_to_local_id, paths, SpanlessEq,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_block, walk_expr, walk_stmt, Visitor};
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, PatKind, QPath, Stmt, StmtKind};
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, PatKind, Stmt, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::sym;
Expand Down Expand Up @@ -60,7 +60,24 @@ struct VecAllocation<'tcx> {

/// Reference to the expression used as argument on `with_capacity` call. This is used
/// to only match slow zero-filling idioms of the same length than vector initialization.
len_expr: &'tcx Expr<'tcx>,
size_expr: InitializedSize<'tcx>,
}

/// Initializer for the creation of the vector.
///
/// When `Vec::with_capacity(size)` is found, the `size` expression will be in
/// `InitializedSize::Initialized`.
///
/// Otherwise, for `Vec::new()` calls, there is no allocation initializer yet, so
/// `InitializedSize::Uninitialized` is used.
/// Later, when a call to `.resize(size, 0)` or similar is found, it's set
/// to `InitializedSize::Initialized(size)`.
///
/// Since it will be set to `InitializedSize::Initialized(size)` when a slow initialization is
/// found, it is always safe to "unwrap" it at lint time.
enum InitializedSize<'tcx> {
Initialized(&'tcx Expr<'tcx>),
Uninitialized,
}

/// Type of slow initialization
Expand All @@ -77,18 +94,14 @@ impl<'tcx> LateLintPass<'tcx> for SlowVectorInit {
// Matches initialization on reassignments. For example: `vec = Vec::with_capacity(100)`
if_chain! {
if let ExprKind::Assign(left, right, _) = expr.kind;

// Extract variable
if let Some(local_id) = path_to_local(left);

// Extract len argument
if let Some(len_arg) = Self::is_vec_with_capacity(cx, right);
if let Some(size_expr) = Self::as_vec_initializer(cx, right);

then {
let vi = VecAllocation {
local_id,
allocation_expr: right,
len_expr: len_arg,
size_expr,
};

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

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

then {
let vi = VecAllocation {
local_id,
allocation_expr: init,
len_expr: len_arg,
size_expr,
};

Self::search_initialization(cx, vi, stmt.hir_id);
Expand All @@ -118,19 +132,20 @@ impl<'tcx> LateLintPass<'tcx> for SlowVectorInit {
}

impl SlowVectorInit {
/// Checks if the given expression is `Vec::with_capacity(..)`. It will return the expression
/// of the first argument of `with_capacity` call if it matches or `None` if it does not.
fn is_vec_with_capacity<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
if_chain! {
if let ExprKind::Call(func, [arg]) = expr.kind;
if let ExprKind::Path(QPath::TypeRelative(ty, name)) = func.kind;
if name.ident.as_str() == "with_capacity";
if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec);
then {
Some(arg)
} else {
None
}
/// Looks for `Vec::with_capacity(size)` or `Vec::new()` calls and returns the initialized size,
/// if any. More specifically, it returns:
/// - `Some(InitializedSize::Initialized(size))` for `Vec::with_capacity(size)`
/// - `Some(InitializedSize::Uninitialized)` for `Vec::new()`
/// - `None` for other, unrelated kinds of expressions
fn as_vec_initializer<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<InitializedSize<'tcx>> {
if let ExprKind::Call(func, [len_expr]) = expr.kind
&& is_expr_path_def_path(cx, func, &paths::VEC_WITH_CAPACITY)
{
Some(InitializedSize::Initialized(len_expr))
} else if matches!(expr.kind, ExprKind::Call(func, _) if is_expr_path_def_path(cx, func, &paths::VEC_NEW)) {
Some(InitializedSize::Uninitialized)
} else {
None
}
}

Expand Down Expand Up @@ -169,12 +184,19 @@ impl SlowVectorInit {
}

fn emit_lint(cx: &LateContext<'_>, slow_fill: &Expr<'_>, vec_alloc: &VecAllocation<'_>, msg: &str) {
let len_expr = Sugg::hir(cx, vec_alloc.len_expr, "len");
let len_expr = Sugg::hir(
cx,
match vec_alloc.size_expr {
InitializedSize::Initialized(expr) => expr,
InitializedSize::Uninitialized => unreachable!("size expression must be set by this point"),
},
"len",
);

span_lint_and_then(cx, SLOW_VECTOR_INITIALIZATION, slow_fill.span, msg, |diag| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of your PR, but if this could also suggest to remove the following call that'd be nice. Not sure if that'd have adverse side effects though

(should probably also be a separate PR ^^)

diag.span_suggestion(
vec_alloc.allocation_expr.span,
"consider replace allocation with",
"consider replacing this with",
format!("vec![0; {len_expr}]"),
Applicability::Unspecified,
);
Expand Down Expand Up @@ -214,36 +236,45 @@ impl<'a, 'tcx> VectorInitializationVisitor<'a, 'tcx> {
}

/// Checks if the given expression is resizing a vector with 0
fn search_slow_resize_filling(&mut self, expr: &'tcx Expr<'_>) {
fn search_slow_resize_filling(&mut self, expr: &'tcx Expr<'tcx>) {
if self.initialization_found
&& let ExprKind::MethodCall(path, self_arg, [len_arg, fill_arg], _) = expr.kind
&& path_to_local_id(self_arg, self.vec_alloc.local_id)
&& path.ident.name == sym!(resize)
// Check that is filled with 0
&& is_integer_literal(fill_arg, 0) {
// Check that len expression is equals to `with_capacity` expression
if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr) {
self.slow_expression = Some(InitializationType::Resize(expr));
} else if let ExprKind::MethodCall(path, ..) = len_arg.kind && path.ident.as_str() == "capacity" {
self.slow_expression = Some(InitializationType::Resize(expr));
}
&& is_integer_literal(fill_arg, 0)
{
let is_matching_resize = if let InitializedSize::Initialized(size_expr) = self.vec_alloc.size_expr {
// If we have a size expression, check that it is equal to what's passed to `resize`
SpanlessEq::new(self.cx).eq_expr(len_arg, size_expr)
|| matches!(len_arg.kind, ExprKind::MethodCall(path, ..) if path.ident.as_str() == "capacity")
} else {
self.vec_alloc.size_expr = InitializedSize::Initialized(len_arg);
true
};

if is_matching_resize {
self.slow_expression = Some(InitializationType::Resize(expr));
}
}
}

/// Returns `true` if give expression is `repeat(0).take(...)`
fn is_repeat_take(&self, expr: &Expr<'_>) -> bool {
fn is_repeat_take(&mut self, expr: &'tcx Expr<'tcx>) -> bool {
if_chain! {
if let ExprKind::MethodCall(take_path, recv, [len_arg, ..], _) = expr.kind;
if take_path.ident.name == sym!(take);
// Check that take is applied to `repeat(0)`
if self.is_repeat_zero(recv);
then {
// Check that len expression is equals to `with_capacity` expression
if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr) {
return true;
} else if let ExprKind::MethodCall(path, ..) = len_arg.kind && path.ident.as_str() == "capacity" {
return true;
if let InitializedSize::Initialized(size_expr) = self.vec_alloc.size_expr {
// Check that len expression is equals to `with_capacity` expression
return SpanlessEq::new(self.cx).eq_expr(len_arg, size_expr)
|| matches!(len_arg.kind, ExprKind::MethodCall(path, ..) if path.ident.as_str() == "capacity")
}

self.vec_alloc.size_expr = InitializedSize::Initialized(len_arg);
return true;
}
}

Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];
pub const VEC_DEQUE_ITER: [&str; 5] = ["alloc", "collections", "vec_deque", "VecDeque", "iter"];
pub const VEC_FROM_ELEM: [&str; 3] = ["alloc", "vec", "from_elem"];
pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"];
pub const VEC_WITH_CAPACITY: [&str; 4] = ["alloc", "vec", "Vec", "with_capacity"];
pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];
Expand Down
6 changes: 5 additions & 1 deletion tests/ui/read_zero_byte_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
#![warn(clippy::read_zero_byte_vec)]
#![allow(clippy::unused_io_amount, clippy::needless_pass_by_ref_mut)]
#![allow(
clippy::unused_io_amount,
clippy::needless_pass_by_ref_mut,
clippy::slow_vector_initialization
)]
use std::fs::File;
use std::io;
use std::io::prelude::*;
Expand Down
20 changes: 10 additions & 10 deletions tests/ui/read_zero_byte_vec.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,61 @@
error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:17:5
--> $DIR/read_zero_byte_vec.rs:21:5
|
LL | f.read_exact(&mut data).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `data.resize(20, 0); f.read_exact(&mut data).unwrap();`
|
= note: `-D clippy::read-zero-byte-vec` implied by `-D warnings`

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

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

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

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

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

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

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

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

error: reading zero byte data to `Vec`
--> $DIR/read_zero_byte_vec.rs:84:5
--> $DIR/read_zero_byte_vec.rs:88:5
|
LL | r.read_exact(&mut data2).await.unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
16 changes: 16 additions & 0 deletions tests/ui/slow_vector_initialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ fn main() {
resize_vector();
extend_vector();
mixed_extend_resize_vector();
from_empty_vec();
}

fn extend_vector() {
Expand Down Expand Up @@ -59,6 +60,21 @@ fn resize_vector() {
vec1.resize(10, 0);
}

fn from_empty_vec() {
// Resize with constant expression
let len = 300;
let mut vec1 = Vec::new();
vec1.resize(len, 0);

// Resize with len expression
let mut vec3 = Vec::new();
vec3.resize(len - 10, 0);

// Reinitialization should be warned
vec1 = Vec::new();
vec1.resize(10, 0);
}

fn do_stuff(vec: &mut [u8]) {}

fn extend_vector_with_manipulations_between() {
Expand Down
Loading