From e0ccc9d9afa2b7800e288a6367422b2a2389f57e Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Fri, 19 Oct 2018 01:15:48 +0200 Subject: [PATCH 1/7] Add slow zero-filled vector initialization lint Add lint to detect slow zero-filled vector initialization. It detects when a vector is zero-filled with extended with `repeat(0).take(len)` or `resize(len, 0)`. This zero-fillings are usually slower than simply using `vec![0; len]`. --- clippy_lints/src/lib.rs | 3 + .../src/slow_vector_initialization.rs | 304 ++++++++++++++++++ tests/ui/slow_vector_initialization.rs | 63 ++++ tests/ui/slow_vector_initialization.stderr | 101 ++++++ tests/ui/slow_vector_initialization.stdout | 0 5 files changed, 471 insertions(+) create mode 100644 clippy_lints/src/slow_vector_initialization.rs create mode 100644 tests/ui/slow_vector_initialization.rs create mode 100644 tests/ui/slow_vector_initialization.stderr create mode 100644 tests/ui/slow_vector_initialization.stdout diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2dbe448c9..cf754d486 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -190,6 +190,7 @@ pub mod replace_consts; pub mod returns; pub mod serde_api; pub mod shadow; +pub mod slow_vector_initialization; pub mod strings; pub mod suspicious_trait_impl; pub mod swap; @@ -459,6 +460,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box non_copy_const::NonCopyConst); reg.register_late_lint_pass(box ptr_offset_with_cast::Pass); reg.register_late_lint_pass(box redundant_clone::RedundantClone); + reg.register_late_lint_pass(box slow_vector_initialization::Pass); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -980,6 +982,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { methods::SINGLE_CHAR_PATTERN, misc::CMP_OWNED, mutex_atomic::MUTEX_ATOMIC, + slow_vector_initialization::SLOW_VECTOR_INITIALIZATION, trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF, types::BOX_VEC, vec::USELESS_VEC, diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs new file mode 100644 index 000000000..52855031a --- /dev/null +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -0,0 +1,304 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::rustc::hir::*; +use if_chain::if_chain; +use crate::syntax_pos::symbol::Symbol; +use crate::syntax::ast::{LitKind, NodeId}; +use crate::syntax::source_map::Span; +use crate::utils::{match_qpath, span_lint_and_then, SpanlessEq}; +use crate::utils::get_enclosing_block; +use crate::rustc_errors::{Applicability}; + +/// **What it does:** Checks slow zero-filled vector initialization +/// +/// **Why is this bad?** This structures are non-idiomatic and less efficient than simply using +/// `vec![len; 0]`. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let mut vec1 = Vec::with_capacity(len); +/// vec1.resize(len, 0); +/// +/// let mut vec2 = Vec::with_capacity(len); +/// vec2.extend(repeat(0).take(len)) +/// ``` +declare_clippy_lint! { + pub SLOW_VECTOR_INITIALIZATION, + perf, + "slow or unsafe vector initialization" +} + +#[derive(Copy, Clone, Default)] +pub struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(SLOW_VECTOR_INITIALIZATION) + } +} + +/// VecInitialization contains data regarding a vector initialized with `with_capacity` and then +/// assigned to a variable. For example, `let mut vec = Vec::with_capacity(0)` or +/// `vec = Vec::with_capacity(0)` +struct VecInitialization<'tcx> { + /// Symbol of the local variable name + variable_name: Symbol, + + /// Reference to the expression which initializes the vector + initialization_expr: &'tcx Expr, + + /// 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, +} + +/// Type of slow initialization +enum InitializationType<'tcx> { + /// Extend is a slow initialization with the form `vec.extend(repeat(0).take(..))` + Extend(&'tcx Expr), + + /// Resize is a slow initialization with the form `vec.resize(.., 0)` + Resize(&'tcx Expr), +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + // Matches initialization on reassignements. For example: `vec = Vec::with_capacity(100)` + if_chain! { + if let ExprKind::Assign(ref left, ref right) = expr.node; + + // Extract variable name + if let ExprKind::Path(QPath::Resolved(_, ref path)) = left.node; + if let Some(variable_name) = path.segments.get(0); + + // Extract len argument + if let Some(ref len_arg) = Pass::is_vec_with_capacity(right); + + then { + let vi = VecInitialization { + variable_name: variable_name.ident.name, + initialization_expr: right, + len_expr: len_arg, + }; + + Pass::search_slow_zero_filling(cx, vi, expr.id, expr.span); + } + } + } + + fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) { + // Matches statements which initializes vectors. For example: `let mut vec = Vec::with_capacity(10)` + if_chain! { + if let StmtKind::Decl(ref decl, _) = stmt.node; + if let DeclKind::Local(ref local) = decl.node; + if let PatKind::Binding(BindingAnnotation::Mutable, _, variable_name, None) = local.pat.node; + if let Some(ref init) = local.init; + if let Some(ref len_arg) = Pass::is_vec_with_capacity(init); + + then { + let vi = VecInitialization { + variable_name: variable_name.name, + initialization_expr: init, + len_expr: len_arg, + }; + + Pass::search_slow_zero_filling(cx, vi, stmt.node.id(), stmt.span); + } + } + } +} + +impl Pass { + /// 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(expr: &Expr) -> Option<&Expr> { + if_chain! { + if let ExprKind::Call(ref func, ref args) = expr.node; + if let ExprKind::Path(ref path) = func.node; + if match_qpath(path, &["Vec", "with_capacity"]); + if args.len() == 1; + + then { + return Some(&args[0]); + } + } + + None + } + + /// Search for slow zero filling vector initialization for the given vector + fn search_slow_zero_filling<'tcx>( + cx: &LateContext<'_, 'tcx>, + vec_initialization: VecInitialization<'tcx>, + parent_node: NodeId, + parent_span: Span + ) { + let enclosing_body = get_enclosing_block(cx, parent_node); + + if enclosing_body.is_none() { + return; + } + + let mut v = SlowInitializationVisitor { + cx, + vec_ini: vec_initialization, + slow_expression: None, + initialization_found: false, + }; + + v.visit_block(enclosing_body.unwrap()); + + if let Some(ref repeat_expr) = v.slow_expression { + span_lint_and_then( + cx, + SLOW_VECTOR_INITIALIZATION, + parent_span, + "detected slow zero-filling initialization", + |db| { + db.span_suggestion_with_applicability(v.vec_ini.initialization_expr.span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); + + match repeat_expr { + InitializationType::Extend(e) => { + db.span_note(e.span, "extended here with .. 0"); + }, + InitializationType::Resize(e) => { + db.span_note(e.span, "resize here with .. 0"); + } + } + } + ); + } + } +} + +/// SlowInitializationVisitor searches for slow zero filling vector initialization, for the given +/// vector. +struct SlowInitializationVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + + /// Contains the information + vec_ini: VecInitialization<'tcx>, + + /// Contains, if found, the slow initialization expression + slow_expression: Option>, + + /// true if the initialization of the vector has been found on the visited block + initialization_found: bool, +} + +impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { + /// Checks if the given expression is extending a vector with `repeat(0).take(..)` + fn search_slow_extend_filling(&mut self, expr: &'tcx Expr) { + if_chain! { + if self.initialization_found; + if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; + if let ExprKind::Path(ref qpath_subj) = args[0].node; + if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if path.ident.name == "extend"; + if let Some(ref extend_arg) = args.get(1); + if self.is_repeat_take(extend_arg); + + then { + self.slow_expression = Some(InitializationType::Extend(expr)); + } + } + } + + /// Checks if the given expression is resizing a vector with 0 + fn search_slow_resize_filling(&mut self, expr: &'tcx Expr) { + if_chain! { + if self.initialization_found; + if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; + if let ExprKind::Path(ref qpath_subj) = args[0].node; + if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if path.ident.name == "resize"; + if let (Some(ref len_arg), Some(fill_arg)) = (args.get(1), args.get(2)); + + // Check that is filled with 0 + if let ExprKind::Lit(ref lit) = fill_arg.node; + if let LitKind::Int(0, _) = lit.node; + + // Check that len expression is equals to `with_capacity` expression + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + + then { + self.slow_expression = Some(InitializationType::Resize(expr)); + } + } + } + + /// Returns `true` if give expression is `repeat(0).take(...)` + fn is_repeat_take(&self, expr: &Expr) -> bool { + if_chain! { + if let ExprKind::MethodCall(ref take_path, _, ref take_args) = expr.node; + if take_path.ident.name == "take"; + + // Check that take is applied to `repeat(0)` + if let Some(ref repeat_expr) = take_args.get(0); + if self.is_repeat_zero(repeat_expr); + + // Check that len expression is equals to `with_capacity` expression + if let Some(ref len_arg) = take_args.get(1); + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + + then { + return true; + } + } + + false + } + + /// Returns `true` if given expression is `repeat(0)` + fn is_repeat_zero(&self, expr: &Expr) -> bool { + if_chain! { + if let ExprKind::Call(ref fn_expr, ref repeat_args) = expr.node; + if let ExprKind::Path(ref qpath_repeat) = fn_expr.node; + if match_qpath(&qpath_repeat, &["repeat"]); + if let Some(ref repeat_arg) = repeat_args.get(0); + if let ExprKind::Lit(ref lit) = repeat_arg.node; + if let LitKind::Int(0, _) = lit.node; + + then { + return true + } + } + + false + } +} + +impl<'a, 'tcx> Visitor<'tcx> for SlowInitializationVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr) { + // Stop the search if we already found a slow zero-filling initialization + if self.slow_expression.is_some() { + return + } + + // Skip all the expressions previous to the vector initialization + if self.vec_ini.initialization_expr.id == expr.id { + self.initialization_found = true; + } + + self.search_slow_extend_filling(expr); + self.search_slow_resize_filling(expr); + + walk_expr(self, expr); + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} diff --git a/tests/ui/slow_vector_initialization.rs b/tests/ui/slow_vector_initialization.rs new file mode 100644 index 000000000..f79abe832 --- /dev/null +++ b/tests/ui/slow_vector_initialization.rs @@ -0,0 +1,63 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::iter::repeat; + +fn main() { + resize_vector(); + extend_vector(); + mixed_extend_resize_vector(); +} + +fn extend_vector() { + // Extend with constant expression + let len = 300; + let mut vec1 = Vec::with_capacity(len); + vec1.extend(repeat(0).take(len)); + + // Extend with len expression + let mut vec2 = Vec::with_capacity(len - 10); + vec2.extend(repeat(0).take(len - 10)); + + // Extend with mismatching expression should not be warned + let mut vec3 = Vec::with_capacity(24322); + vec3.extend(repeat(0).take(2)); +} + +fn mixed_extend_resize_vector() { + // Mismatching len + let mut mismatching_len = Vec::with_capacity(30); + + // Slow initialization + let mut resized_vec = Vec::with_capacity(30); + let mut extend_vec = Vec::with_capacity(30); + + resized_vec.resize(30, 0); + mismatching_len.extend(repeat(0).take(40)); + extend_vec.extend(repeat(0).take(30)); +} + +fn resize_vector() { + // Resize with constant expression + let len = 300; + let mut vec1 = Vec::with_capacity(len); + vec1.resize(len, 0); + + // Resize mismatch len + let mut vec2 = Vec::with_capacity(200); + vec2.resize(10, 0); + + // Resize with len expression + let mut vec3 = Vec::with_capacity(len - 10); + vec3.resize(len - 10, 0); + + // Reinitialization should be warned + vec1 = Vec::with_capacity(10); + vec1.resize(10, 0); +} diff --git a/tests/ui/slow_vector_initialization.stderr b/tests/ui/slow_vector_initialization.stderr new file mode 100644 index 000000000..4941794d5 --- /dev/null +++ b/tests/ui/slow_vector_initialization.stderr @@ -0,0 +1,101 @@ +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:21:5 + | +21 | let mut vec1 = Vec::with_capacity(len); + | ^^^^^^^^^^^^^^^-----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | + = note: `-D clippy::slow-vector-initialization` implied by `-D warnings` +note: extended here with .. 0 + --> $DIR/slow_vector_initialization.rs:22:5 + | +22 | vec1.extend(repeat(0).take(len)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:25:5 + | +25 | let mut vec2 = Vec::with_capacity(len - 10); + | ^^^^^^^^^^^^^^^----------------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: extended here with .. 0 + --> $DIR/slow_vector_initialization.rs:26:5 + | +26 | vec2.extend(repeat(0).take(len - 10)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:38:5 + | +38 | let mut resized_vec = Vec::with_capacity(30); + | ^^^^^^^^^^^^^^^^^^^^^^----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: resize here with .. 0 + --> $DIR/slow_vector_initialization.rs:41:5 + | +41 | resized_vec.resize(30, 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:39:5 + | +39 | let mut extend_vec = Vec::with_capacity(30); + | ^^^^^^^^^^^^^^^^^^^^^----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: extended here with .. 0 + --> $DIR/slow_vector_initialization.rs:43:5 + | +43 | extend_vec.extend(repeat(0).take(30)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:49:5 + | +49 | let mut vec1 = Vec::with_capacity(len); + | ^^^^^^^^^^^^^^^-----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: resize here with .. 0 + --> $DIR/slow_vector_initialization.rs:50:5 + | +50 | vec1.resize(len, 0); + | ^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:57:5 + | +57 | let mut vec3 = Vec::with_capacity(len - 10); + | ^^^^^^^^^^^^^^^----------------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: resize here with .. 0 + --> $DIR/slow_vector_initialization.rs:58:5 + | +58 | vec3.resize(len - 10, 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:61:5 + | +61 | vec1 = Vec::with_capacity(10); + | ^^^^^^^---------------------- + | | + | help: consider replacing with: `vec![0; ..]` + | +note: resize here with .. 0 + --> $DIR/slow_vector_initialization.rs:62:5 + | +62 | vec1.resize(10, 0); + | ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors + diff --git a/tests/ui/slow_vector_initialization.stdout b/tests/ui/slow_vector_initialization.stdout new file mode 100644 index 000000000..e69de29bb From 9b4bc3b6efcb9daf8c1daf9f7f2a8bfa19973ad8 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Fri, 26 Oct 2018 22:35:16 +0200 Subject: [PATCH 2/7] Add unsafe set_len initialization --- .../src/slow_vector_initialization.rs | 40 +++++-- tests/ui/slow_vector_initialization.rs | 9 ++ tests/ui/slow_vector_initialization.stderr | 102 ++++++++++-------- 3 files changed, 100 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs index 52855031a..5d0b302e0 100644 --- a/clippy_lints/src/slow_vector_initialization.rs +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -71,6 +71,9 @@ enum InitializationType<'tcx> { /// Resize is a slow initialization with the form `vec.resize(.., 0)` Resize(&'tcx Expr), + + /// UnsafeSetLen is a slow initialization with the form `vec.set_len(..)` + UnsafeSetLen(&'tcx Expr), } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { @@ -93,7 +96,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_slow_zero_filling(cx, vi, expr.id, expr.span); + Pass::search_slow_initialization(cx, vi, expr.id, expr.span); } } } @@ -114,7 +117,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_slow_zero_filling(cx, vi, stmt.node.id(), stmt.span); + Pass::search_slow_initialization(cx, vi, stmt.node.id(), stmt.span); } } } @@ -138,8 +141,8 @@ impl Pass { None } - /// Search for slow zero filling vector initialization for the given vector - fn search_slow_zero_filling<'tcx>( + /// Search a slow initialization for the given vector + fn search_slow_initialization<'tcx>( cx: &LateContext<'_, 'tcx>, vec_initialization: VecInitialization<'tcx>, parent_node: NodeId, @@ -171,11 +174,14 @@ impl Pass { match repeat_expr { InitializationType::Extend(e) => { - db.span_note(e.span, "extended here with .. 0"); + db.span_note(e.span, "extended at"); }, InitializationType::Resize(e) => { - db.span_note(e.span, "resize here with .. 0"); - } + db.span_note(e.span, "resized at"); + }, + InitializationType::UnsafeSetLen(e) => { + db.span_note(e.span, "changed len at"); + }, } } ); @@ -239,6 +245,25 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { } } + /// Checks if the given expression is using `set_len` to initialize the vector + fn search_unsafe_set_len(&mut self, expr: &'tcx Expr) { + if_chain! { + if self.initialization_found; + if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; + if let ExprKind::Path(ref qpath_subj) = args[0].node; + if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if path.ident.name == "set_len"; + if let Some(ref len_arg) = args.get(1); + + // Check that len expression is equals to `with_capacity` expression + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + + then { + self.slow_expression = Some(InitializationType::UnsafeSetLen(expr)); + } + } + } + /// Returns `true` if give expression is `repeat(0).take(...)` fn is_repeat_take(&self, expr: &Expr) -> bool { if_chain! { @@ -294,6 +319,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SlowInitializationVisitor<'a, 'tcx> { self.search_slow_extend_filling(expr); self.search_slow_resize_filling(expr); + self.search_unsafe_set_len(expr); walk_expr(self, expr); } diff --git a/tests/ui/slow_vector_initialization.rs b/tests/ui/slow_vector_initialization.rs index f79abe832..991d85046 100644 --- a/tests/ui/slow_vector_initialization.rs +++ b/tests/ui/slow_vector_initialization.rs @@ -13,6 +13,7 @@ fn main() { resize_vector(); extend_vector(); mixed_extend_resize_vector(); + unsafe_vector(); } fn extend_vector() { @@ -61,3 +62,11 @@ fn resize_vector() { vec1 = Vec::with_capacity(10); vec1.resize(10, 0); } + +fn unsafe_vector() { + let mut unsafe_vec: Vec = Vec::with_capacity(200); + + unsafe { + unsafe_vec.set_len(200); + } +} diff --git a/tests/ui/slow_vector_initialization.stderr b/tests/ui/slow_vector_initialization.stderr index 4941794d5..ece6f388c 100644 --- a/tests/ui/slow_vector_initialization.stderr +++ b/tests/ui/slow_vector_initialization.stderr @@ -1,101 +1,115 @@ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:21:5 + --> $DIR/slow_vector_initialization.rs:22:5 | -21 | let mut vec1 = Vec::with_capacity(len); +22 | let mut vec1 = Vec::with_capacity(len); | ^^^^^^^^^^^^^^^-----------------------^ | | | help: consider replacing with: `vec![0; ..]` | = note: `-D clippy::slow-vector-initialization` implied by `-D warnings` -note: extended here with .. 0 - --> $DIR/slow_vector_initialization.rs:22:5 +note: extended at + --> $DIR/slow_vector_initialization.rs:23:5 | -22 | vec1.extend(repeat(0).take(len)); +23 | vec1.extend(repeat(0).take(len)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:25:5 + --> $DIR/slow_vector_initialization.rs:26:5 | -25 | let mut vec2 = Vec::with_capacity(len - 10); +26 | let mut vec2 = Vec::with_capacity(len - 10); | ^^^^^^^^^^^^^^^----------------------------^ | | | help: consider replacing with: `vec![0; ..]` | -note: extended here with .. 0 - --> $DIR/slow_vector_initialization.rs:26:5 +note: extended at + --> $DIR/slow_vector_initialization.rs:27:5 | -26 | vec2.extend(repeat(0).take(len - 10)); +27 | vec2.extend(repeat(0).take(len - 10)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:38:5 - | -38 | let mut resized_vec = Vec::with_capacity(30); - | ^^^^^^^^^^^^^^^^^^^^^^----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: resize here with .. 0 - --> $DIR/slow_vector_initialization.rs:41:5 - | -41 | resized_vec.resize(30, 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ - error: detected slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:39:5 | -39 | let mut extend_vec = Vec::with_capacity(30); +39 | let mut resized_vec = Vec::with_capacity(30); + | ^^^^^^^^^^^^^^^^^^^^^^----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: resized at + --> $DIR/slow_vector_initialization.rs:42:5 + | +42 | resized_vec.resize(30, 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:40:5 + | +40 | let mut extend_vec = Vec::with_capacity(30); | ^^^^^^^^^^^^^^^^^^^^^----------------------^ | | | help: consider replacing with: `vec![0; ..]` | -note: extended here with .. 0 - --> $DIR/slow_vector_initialization.rs:43:5 +note: extended at + --> $DIR/slow_vector_initialization.rs:44:5 | -43 | extend_vec.extend(repeat(0).take(30)); +44 | extend_vec.extend(repeat(0).take(30)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:49:5 + --> $DIR/slow_vector_initialization.rs:50:5 | -49 | let mut vec1 = Vec::with_capacity(len); +50 | let mut vec1 = Vec::with_capacity(len); | ^^^^^^^^^^^^^^^-----------------------^ | | | help: consider replacing with: `vec![0; ..]` | -note: resize here with .. 0 - --> $DIR/slow_vector_initialization.rs:50:5 +note: resized at + --> $DIR/slow_vector_initialization.rs:51:5 | -50 | vec1.resize(len, 0); +51 | vec1.resize(len, 0); | ^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:57:5 + --> $DIR/slow_vector_initialization.rs:58:5 | -57 | let mut vec3 = Vec::with_capacity(len - 10); +58 | let mut vec3 = Vec::with_capacity(len - 10); | ^^^^^^^^^^^^^^^----------------------------^ | | | help: consider replacing with: `vec![0; ..]` | -note: resize here with .. 0 - --> $DIR/slow_vector_initialization.rs:58:5 +note: resized at + --> $DIR/slow_vector_initialization.rs:59:5 | -58 | vec3.resize(len - 10, 0); +59 | vec3.resize(len - 10, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:61:5 + --> $DIR/slow_vector_initialization.rs:62:5 | -61 | vec1 = Vec::with_capacity(10); +62 | vec1 = Vec::with_capacity(10); | ^^^^^^^---------------------- | | | help: consider replacing with: `vec![0; ..]` | -note: resize here with .. 0 - --> $DIR/slow_vector_initialization.rs:62:5 +note: resized at + --> $DIR/slow_vector_initialization.rs:63:5 | -62 | vec1.resize(10, 0); +63 | vec1.resize(10, 0); | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +error: detected slow zero-filling initialization + --> $DIR/slow_vector_initialization.rs:67:5 + | +67 | let mut unsafe_vec: Vec = Vec::with_capacity(200); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------------^ + | | + | help: consider replacing with: `vec![0; ..]` + | +note: changed len at + --> $DIR/slow_vector_initialization.rs:70:9 + | +70 | unsafe_vec.set_len(200); + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors From 2753f1cbd4623063f19ca27caaac67308c94a536 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Tue, 30 Oct 2018 00:25:05 +0100 Subject: [PATCH 3/7] Split lint into slow and unsafe vector initalization --- .../src/slow_vector_initialization.rs | 100 +++++++++++++----- tests/ui/slow_vector_initialization.stderr | 90 +++++----------- 2 files changed, 96 insertions(+), 94 deletions(-) diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs index 5d0b302e0..090125e36 100644 --- a/clippy_lints/src/slow_vector_initialization.rs +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -37,7 +37,25 @@ use crate::rustc_errors::{Applicability}; declare_clippy_lint! { pub SLOW_VECTOR_INITIALIZATION, perf, - "slow or unsafe vector initialization" + "slow vector initialization" +} + +/// **What it does:** Checks unsafe vector initialization +/// +/// **Why is this bad?** Changing the length of a vector may expose uninitialized memory, which +/// can lead to memory safety issues +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let mut vec1 = Vec::with_capacity(len); +/// unsafe { vec1.set_len(len); } +/// ``` +declare_clippy_lint! { + pub UNSAFE_VECTOR_INITIALIZATION, + correctness, + "unsafe vector initialization" } #[derive(Copy, Clone, Default)] @@ -45,7 +63,10 @@ pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { - lint_array!(SLOW_VECTOR_INITIALIZATION) + lint_array!( + SLOW_VECTOR_INITIALIZATION, + UNSAFE_VECTOR_INITIALIZATION, + ) } } @@ -96,7 +117,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_slow_initialization(cx, vi, expr.id, expr.span); + Pass::search_slow_initialization(cx, vi, expr.id); } } } @@ -117,7 +138,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_slow_initialization(cx, vi, stmt.node.id(), stmt.span); + Pass::search_slow_initialization(cx, vi, stmt.node.id()); } } } @@ -145,8 +166,7 @@ impl Pass { fn search_slow_initialization<'tcx>( cx: &LateContext<'_, 'tcx>, vec_initialization: VecInitialization<'tcx>, - parent_node: NodeId, - parent_span: Span + parent_node: NodeId ) { let enclosing_body = get_enclosing_block(cx, parent_node); @@ -163,30 +183,54 @@ impl Pass { v.visit_block(enclosing_body.unwrap()); - if let Some(ref repeat_expr) = v.slow_expression { - span_lint_and_then( - cx, - SLOW_VECTOR_INITIALIZATION, - parent_span, - "detected slow zero-filling initialization", - |db| { - db.span_suggestion_with_applicability(v.vec_ini.initialization_expr.span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); - - match repeat_expr { - InitializationType::Extend(e) => { - db.span_note(e.span, "extended at"); - }, - InitializationType::Resize(e) => { - db.span_note(e.span, "resized at"); - }, - InitializationType::UnsafeSetLen(e) => { - db.span_note(e.span, "changed len at"); - }, - } - } - ); + if let Some(ref initialization_expr) = v.slow_expression { + let alloc_span = v.vec_ini.initialization_expr.span; + Pass::lint_initialization(cx, initialization_expr, alloc_span); } } + + fn lint_initialization<'tcx>(cx: &LateContext<'_, 'tcx>, initialization: &InitializationType<'tcx>, alloc_span: Span) { + match initialization { + InitializationType::UnsafeSetLen(e) => + Pass::lint_unsafe_initialization(cx, e, alloc_span), + + InitializationType::Extend(e) | + InitializationType::Resize(e) => + Pass::lint_slow_initialization(cx, e, alloc_span), + }; + } + + fn lint_slow_initialization<'tcx>( + cx: &LateContext<'_, 'tcx>, + slow_fill: &Expr, + alloc_span: Span, + ) { + span_lint_and_then( + cx, + SLOW_VECTOR_INITIALIZATION, + slow_fill.span, + "detected slow zero-filling initialization", + |db| { + db.span_suggestion_with_applicability(alloc_span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); + } + ); + } + + fn lint_unsafe_initialization<'tcx>( + cx: &LateContext<'_, 'tcx>, + slow_fill: &Expr, + alloc_span: Span, + ) { + span_lint_and_then( + cx, + UNSAFE_VECTOR_INITIALIZATION, + slow_fill.span, + "detected unsafe vector initialization", + |db| { + db.span_suggestion_with_applicability(alloc_span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); + } + ); + } } /// SlowInitializationVisitor searches for slow zero filling vector initialization, for the given diff --git a/tests/ui/slow_vector_initialization.stderr b/tests/ui/slow_vector_initialization.stderr index ece6f388c..74fd9be0a 100644 --- a/tests/ui/slow_vector_initialization.stderr +++ b/tests/ui/slow_vector_initialization.stderr @@ -1,115 +1,73 @@ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:22:5 - | -22 | let mut vec1 = Vec::with_capacity(len); - | ^^^^^^^^^^^^^^^-----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | - = note: `-D clippy::slow-vector-initialization` implied by `-D warnings` -note: extended at --> $DIR/slow_vector_initialization.rs:23:5 | +22 | let mut vec1 = Vec::with_capacity(len); + | ----------------------- help: consider replacing with: `vec![0; ..]` 23 | vec1.extend(repeat(0).take(len)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::slow-vector-initialization` implied by `-D warnings` error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:26:5 - | -26 | let mut vec2 = Vec::with_capacity(len - 10); - | ^^^^^^^^^^^^^^^----------------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: extended at --> $DIR/slow_vector_initialization.rs:27:5 | +26 | let mut vec2 = Vec::with_capacity(len - 10); + | ---------------------------- help: consider replacing with: `vec![0; ..]` 27 | vec2.extend(repeat(0).take(len - 10)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:39:5 - | -39 | let mut resized_vec = Vec::with_capacity(30); - | ^^^^^^^^^^^^^^^^^^^^^^----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: resized at --> $DIR/slow_vector_initialization.rs:42:5 | +39 | let mut resized_vec = Vec::with_capacity(30); + | ---------------------- help: consider replacing with: `vec![0; ..]` +... 42 | resized_vec.resize(30, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:40:5 - | -40 | let mut extend_vec = Vec::with_capacity(30); - | ^^^^^^^^^^^^^^^^^^^^^----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: extended at --> $DIR/slow_vector_initialization.rs:44:5 | +40 | let mut extend_vec = Vec::with_capacity(30); + | ---------------------- help: consider replacing with: `vec![0; ..]` +... 44 | extend_vec.extend(repeat(0).take(30)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:50:5 - | -50 | let mut vec1 = Vec::with_capacity(len); - | ^^^^^^^^^^^^^^^-----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: resized at --> $DIR/slow_vector_initialization.rs:51:5 | +50 | let mut vec1 = Vec::with_capacity(len); + | ----------------------- help: consider replacing with: `vec![0; ..]` 51 | vec1.resize(len, 0); | ^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:58:5 - | -58 | let mut vec3 = Vec::with_capacity(len - 10); - | ^^^^^^^^^^^^^^^----------------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: resized at --> $DIR/slow_vector_initialization.rs:59:5 | +58 | let mut vec3 = Vec::with_capacity(len - 10); + | ---------------------------- help: consider replacing with: `vec![0; ..]` 59 | vec3.resize(len - 10, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^ error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:62:5 - | -62 | vec1 = Vec::with_capacity(10); - | ^^^^^^^---------------------- - | | - | help: consider replacing with: `vec![0; ..]` - | -note: resized at --> $DIR/slow_vector_initialization.rs:63:5 | +62 | vec1 = Vec::with_capacity(10); + | ---------------------- help: consider replacing with: `vec![0; ..]` 63 | vec1.resize(10, 0); | ^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:67:5 - | -67 | let mut unsafe_vec: Vec = Vec::with_capacity(200); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------------^ - | | - | help: consider replacing with: `vec![0; ..]` - | -note: changed len at +error: detected unsafe vector initialization --> $DIR/slow_vector_initialization.rs:70:9 | +67 | let mut unsafe_vec: Vec = Vec::with_capacity(200); + | ----------------------- help: consider replacing with: `vec![0; ..]` +... 70 | unsafe_vec.set_len(200); | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: #[deny(clippy::unsafe_vector_initialization)] on by default error: aborting due to 8 previous errors From 5b77ee95dc6c8cadfba8b5f82b427efa3e0f23c5 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Tue, 30 Oct 2018 21:47:15 +0100 Subject: [PATCH 4/7] Rename some symbols Renamed some symbols in order to make them a little bit more accurate. --- .../src/slow_vector_initialization.rs | 117 +++++++++--------- tests/ui/slow_vector_initialization.stderr | 32 ++--- 2 files changed, 76 insertions(+), 73 deletions(-) diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs index 090125e36..ec9c7ec60 100644 --- a/clippy_lints/src/slow_vector_initialization.rs +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -8,15 +8,14 @@ // except according to those terms. use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; -use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass, Lint}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; use if_chain::if_chain; use crate::syntax_pos::symbol::Symbol; use crate::syntax::ast::{LitKind, NodeId}; -use crate::syntax::source_map::Span; -use crate::utils::{match_qpath, span_lint_and_then, SpanlessEq}; -use crate::utils::get_enclosing_block; +use crate::utils::{match_qpath, span_lint_and_then, SpanlessEq, get_enclosing_block}; +use crate::utils::sugg::Sugg; use crate::rustc_errors::{Applicability}; /// **What it does:** Checks slow zero-filled vector initialization @@ -70,15 +69,15 @@ impl LintPass for Pass { } } -/// VecInitialization contains data regarding a vector initialized with `with_capacity` and then +/// `VecAllocation` contains data regarding a vector allocated with `with_capacity` and then /// assigned to a variable. For example, `let mut vec = Vec::with_capacity(0)` or /// `vec = Vec::with_capacity(0)` -struct VecInitialization<'tcx> { +struct VecAllocation<'tcx> { /// Symbol of the local variable name variable_name: Symbol, - /// Reference to the expression which initializes the vector - initialization_expr: &'tcx Expr, + /// Reference to the expression which allocates the vector + allocation_expr: &'tcx Expr, /// 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. @@ -111,13 +110,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let Some(ref len_arg) = Pass::is_vec_with_capacity(right); then { - let vi = VecInitialization { + let vi = VecAllocation { variable_name: variable_name.ident.name, - initialization_expr: right, + allocation_expr: right, len_expr: len_arg, }; - Pass::search_slow_initialization(cx, vi, expr.id); + Pass::search_initialization(cx, vi, expr.id); } } } @@ -132,13 +131,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let Some(ref len_arg) = Pass::is_vec_with_capacity(init); then { - let vi = VecInitialization { + let vi = VecAllocation { variable_name: variable_name.name, - initialization_expr: init, + allocation_expr: init, len_expr: len_arg, }; - Pass::search_slow_initialization(cx, vi, stmt.node.id()); + Pass::search_initialization(cx, vi, stmt.node.id()); } } } @@ -162,10 +161,10 @@ impl Pass { None } - /// Search a slow initialization for the given vector - fn search_slow_initialization<'tcx>( + /// Search initialization for the given vector + fn search_initialization<'tcx>( cx: &LateContext<'_, 'tcx>, - vec_initialization: VecInitialization<'tcx>, + vec_alloc: VecAllocation<'tcx>, parent_node: NodeId ) { let enclosing_body = get_enclosing_block(cx, parent_node); @@ -174,72 +173,76 @@ impl Pass { return; } - let mut v = SlowInitializationVisitor { + let mut v = VectorInitializationVisitor { cx, - vec_ini: vec_initialization, + vec_alloc, slow_expression: None, initialization_found: false, }; v.visit_block(enclosing_body.unwrap()); - if let Some(ref initialization_expr) = v.slow_expression { - let alloc_span = v.vec_ini.initialization_expr.span; - Pass::lint_initialization(cx, initialization_expr, alloc_span); + if let Some(ref allocation_expr) = v.slow_expression { + Pass::lint_initialization(cx, allocation_expr, &v.vec_alloc); } } - fn lint_initialization<'tcx>(cx: &LateContext<'_, 'tcx>, initialization: &InitializationType<'tcx>, alloc_span: Span) { + fn lint_initialization<'tcx>(cx: &LateContext<'_, 'tcx>, initialization: &InitializationType<'tcx>, vec_alloc: &VecAllocation<'_>) { match initialization { InitializationType::UnsafeSetLen(e) => - Pass::lint_unsafe_initialization(cx, e, alloc_span), + Pass::emit_lint( + cx, + e, + vec_alloc, + "unsafe vector initialization", + UNSAFE_VECTOR_INITIALIZATION + ), InitializationType::Extend(e) | InitializationType::Resize(e) => - Pass::lint_slow_initialization(cx, e, alloc_span), + Pass::emit_lint( + cx, + e, + vec_alloc, + "slow zero-filling initialization", + SLOW_VECTOR_INITIALIZATION + ) }; } - fn lint_slow_initialization<'tcx>( + fn emit_lint<'tcx>( cx: &LateContext<'_, 'tcx>, slow_fill: &Expr, - alloc_span: Span, + vec_alloc: &VecAllocation<'_>, + msg: &str, + lint: &'static Lint ) { - span_lint_and_then( - cx, - SLOW_VECTOR_INITIALIZATION, - slow_fill.span, - "detected slow zero-filling initialization", - |db| { - db.span_suggestion_with_applicability(alloc_span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); - } - ); - } + let len_expr = Sugg::hir(cx, vec_alloc.len_expr, "len"); - fn lint_unsafe_initialization<'tcx>( - cx: &LateContext<'_, 'tcx>, - slow_fill: &Expr, - alloc_span: Span, - ) { span_lint_and_then( cx, - UNSAFE_VECTOR_INITIALIZATION, + lint, slow_fill.span, - "detected unsafe vector initialization", + msg, |db| { - db.span_suggestion_with_applicability(alloc_span, "consider replacing with", "vec![0; ..]".to_string(), Applicability::Unspecified); + db.span_suggestion_with_applicability( + vec_alloc.allocation_expr.span, + "consider replace allocation with", + format!("vec![0; {}]", len_expr), + Applicability::Unspecified + ); } ); } } -/// SlowInitializationVisitor searches for slow zero filling vector initialization, for the given +/// `VectorInitializationVisitor` searches for unsafe or slow vector initializations for the given /// vector. -struct SlowInitializationVisitor<'a, 'tcx: 'a> { +struct VectorInitializationVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, /// Contains the information - vec_ini: VecInitialization<'tcx>, + vec_alloc: VecAllocation<'tcx>, /// Contains, if found, the slow initialization expression slow_expression: Option>, @@ -248,14 +251,14 @@ struct SlowInitializationVisitor<'a, 'tcx: 'a> { initialization_found: bool, } -impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { +impl<'a, 'tcx> VectorInitializationVisitor<'a, 'tcx> { /// Checks if the given expression is extending a vector with `repeat(0).take(..)` fn search_slow_extend_filling(&mut self, expr: &'tcx Expr) { if_chain! { if self.initialization_found; if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; if let ExprKind::Path(ref qpath_subj) = args[0].node; - if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if match_qpath(&qpath_subj, &[&self.vec_alloc.variable_name.to_string()]); if path.ident.name == "extend"; if let Some(ref extend_arg) = args.get(1); if self.is_repeat_take(extend_arg); @@ -272,7 +275,7 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { if self.initialization_found; if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; if let ExprKind::Path(ref qpath_subj) = args[0].node; - if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if match_qpath(&qpath_subj, &[&self.vec_alloc.variable_name.to_string()]); if path.ident.name == "resize"; if let (Some(ref len_arg), Some(fill_arg)) = (args.get(1), args.get(2)); @@ -281,7 +284,7 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { if let LitKind::Int(0, _) = lit.node; // Check that len expression is equals to `with_capacity` expression - if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr); then { self.slow_expression = Some(InitializationType::Resize(expr)); @@ -295,12 +298,12 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { if self.initialization_found; if let ExprKind::MethodCall(ref path, _, ref args) = expr.node; if let ExprKind::Path(ref qpath_subj) = args[0].node; - if match_qpath(&qpath_subj, &[&self.vec_ini.variable_name.to_string()]); + if match_qpath(&qpath_subj, &[&self.vec_alloc.variable_name.to_string()]); if path.ident.name == "set_len"; if let Some(ref len_arg) = args.get(1); // Check that len expression is equals to `with_capacity` expression - if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr); then { self.slow_expression = Some(InitializationType::UnsafeSetLen(expr)); @@ -320,7 +323,7 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { // Check that len expression is equals to `with_capacity` expression if let Some(ref len_arg) = take_args.get(1); - if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_ini.len_expr); + if SpanlessEq::new(self.cx).eq_expr(len_arg, self.vec_alloc.len_expr); then { return true; @@ -349,7 +352,7 @@ impl<'a, 'tcx> SlowInitializationVisitor<'a, 'tcx> { } } -impl<'a, 'tcx> Visitor<'tcx> for SlowInitializationVisitor<'a, 'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for VectorInitializationVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr) { // Stop the search if we already found a slow zero-filling initialization if self.slow_expression.is_some() { @@ -357,7 +360,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SlowInitializationVisitor<'a, 'tcx> { } // Skip all the expressions previous to the vector initialization - if self.vec_ini.initialization_expr.id == expr.id { + if self.vec_alloc.allocation_expr.id == expr.id { self.initialization_found = true; } diff --git a/tests/ui/slow_vector_initialization.stderr b/tests/ui/slow_vector_initialization.stderr index 74fd9be0a..c1e48d42a 100644 --- a/tests/ui/slow_vector_initialization.stderr +++ b/tests/ui/slow_vector_initialization.stderr @@ -1,68 +1,68 @@ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:23:5 | 22 | let mut vec1 = Vec::with_capacity(len); - | ----------------------- help: consider replacing with: `vec![0; ..]` + | ----------------------- help: consider replace allocation with: `vec![0; len]` 23 | vec1.extend(repeat(0).take(len)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::slow-vector-initialization` implied by `-D warnings` -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:27:5 | 26 | let mut vec2 = Vec::with_capacity(len - 10); - | ---------------------------- help: consider replacing with: `vec![0; ..]` + | ---------------------------- help: consider replace allocation with: `vec![0; len - 10]` 27 | vec2.extend(repeat(0).take(len - 10)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:42:5 | 39 | let mut resized_vec = Vec::with_capacity(30); - | ---------------------- help: consider replacing with: `vec![0; ..]` + | ---------------------- help: consider replace allocation with: `vec![0; 30]` ... 42 | resized_vec.resize(30, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:44:5 | 40 | let mut extend_vec = Vec::with_capacity(30); - | ---------------------- help: consider replacing with: `vec![0; ..]` + | ---------------------- help: consider replace allocation with: `vec![0; 30]` ... 44 | extend_vec.extend(repeat(0).take(30)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:51:5 | 50 | let mut vec1 = Vec::with_capacity(len); - | ----------------------- help: consider replacing with: `vec![0; ..]` + | ----------------------- help: consider replace allocation with: `vec![0; len]` 51 | vec1.resize(len, 0); | ^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:59:5 | 58 | let mut vec3 = Vec::with_capacity(len - 10); - | ---------------------------- help: consider replacing with: `vec![0; ..]` + | ---------------------------- help: consider replace allocation with: `vec![0; len - 10]` 59 | vec3.resize(len - 10, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^ -error: detected slow zero-filling initialization +error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:63:5 | 62 | vec1 = Vec::with_capacity(10); - | ---------------------- help: consider replacing with: `vec![0; ..]` + | ---------------------- help: consider replace allocation with: `vec![0; 10]` 63 | vec1.resize(10, 0); | ^^^^^^^^^^^^^^^^^^ -error: detected unsafe vector initialization +error: unsafe vector initialization --> $DIR/slow_vector_initialization.rs:70:9 | 67 | let mut unsafe_vec: Vec = Vec::with_capacity(200); - | ----------------------- help: consider replacing with: `vec![0; ..]` + | ----------------------- help: consider replace allocation with: `vec![0; 200]` ... 70 | unsafe_vec.set_len(200); | ^^^^^^^^^^^^^^^^^^^^^^^ From 39b02fdcd26adc95540e96c82b93beb926b6606f Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Mon, 5 Nov 2018 22:46:07 +0100 Subject: [PATCH 5/7] Fix some warnings related to Self --- clippy_lints/src/slow_vector_initialization.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs index ec9c7ec60..2af657a5a 100644 --- a/clippy_lints/src/slow_vector_initialization.rs +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -107,7 +107,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let Some(variable_name) = path.segments.get(0); // Extract len argument - if let Some(ref len_arg) = Pass::is_vec_with_capacity(right); + if let Some(ref len_arg) = Self::is_vec_with_capacity(right); then { let vi = VecAllocation { @@ -116,7 +116,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_initialization(cx, vi, expr.id); + Self::search_initialization(cx, vi, expr.id); } } } @@ -128,7 +128,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let DeclKind::Local(ref local) = decl.node; if let PatKind::Binding(BindingAnnotation::Mutable, _, variable_name, None) = local.pat.node; if let Some(ref init) = local.init; - if let Some(ref len_arg) = Pass::is_vec_with_capacity(init); + if let Some(ref len_arg) = Self::is_vec_with_capacity(init); then { let vi = VecAllocation { @@ -137,7 +137,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { len_expr: len_arg, }; - Pass::search_initialization(cx, vi, stmt.node.id()); + Self::search_initialization(cx, vi, stmt.node.id()); } } } @@ -183,14 +183,14 @@ impl Pass { v.visit_block(enclosing_body.unwrap()); if let Some(ref allocation_expr) = v.slow_expression { - Pass::lint_initialization(cx, allocation_expr, &v.vec_alloc); + Self::lint_initialization(cx, allocation_expr, &v.vec_alloc); } } fn lint_initialization<'tcx>(cx: &LateContext<'_, 'tcx>, initialization: &InitializationType<'tcx>, vec_alloc: &VecAllocation<'_>) { match initialization { InitializationType::UnsafeSetLen(e) => - Pass::emit_lint( + Self::emit_lint( cx, e, vec_alloc, @@ -200,7 +200,7 @@ impl Pass { InitializationType::Extend(e) | InitializationType::Resize(e) => - Pass::emit_lint( + Self::emit_lint( cx, e, vec_alloc, From 5fa04bc3cdb3071dddbfd9cf9cf33bc099b182f0 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Sun, 18 Nov 2018 23:45:53 +0100 Subject: [PATCH 6/7] Lint only the first statment/expression after alloc Instead of searching for all the successive expressions after a vector allocation, check only the first expression. This is done to minimize the amount of false positives of the lint. --- .../src/slow_vector_initialization.rs | 41 ++++++++++++++----- tests/ui/slow_vector_initialization.rs | 17 ++++++-- tests/ui/slow_vector_initialization.stderr | 10 ++--- 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/slow_vector_initialization.rs b/clippy_lints/src/slow_vector_initialization.rs index 2af657a5a..272047bc7 100644 --- a/clippy_lints/src/slow_vector_initialization.rs +++ b/clippy_lints/src/slow_vector_initialization.rs @@ -7,7 +7,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use crate::rustc::hir::intravisit::{walk_expr, walk_stmt, walk_block, NestedVisitorMap, Visitor}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass, Lint}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; @@ -353,20 +353,41 @@ impl<'a, 'tcx> VectorInitializationVisitor<'a, 'tcx> { } impl<'a, 'tcx> Visitor<'tcx> for VectorInitializationVisitor<'a, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx Expr) { - // Stop the search if we already found a slow zero-filling initialization - if self.slow_expression.is_some() { - return - } + fn visit_stmt(&mut self, stmt: &'tcx Stmt) { + if self.initialization_found { + match stmt.node { + StmtKind::Expr(ref expr, _) | + StmtKind::Semi(ref expr, _) => { + self.search_slow_extend_filling(expr); + self.search_slow_resize_filling(expr); + self.search_unsafe_set_len(expr); + }, + _ => (), + } + self.initialization_found = false; + } else { + walk_stmt(self, stmt); + } + } + + fn visit_block(&mut self, block: &'tcx Block) { + if self.initialization_found { + if let Some(ref s) = block.stmts.get(0) { + self.visit_stmt( s) + } + + self.initialization_found = false; + } else { + walk_block(self, block); + } + } + + fn visit_expr(&mut self, expr: &'tcx Expr) { // Skip all the expressions previous to the vector initialization if self.vec_alloc.allocation_expr.id == expr.id { self.initialization_found = true; } - - self.search_slow_extend_filling(expr); - self.search_slow_resize_filling(expr); - self.search_unsafe_set_len(expr); walk_expr(self, expr); } diff --git a/tests/ui/slow_vector_initialization.rs b/tests/ui/slow_vector_initialization.rs index 991d85046..daa6b9c13 100644 --- a/tests/ui/slow_vector_initialization.rs +++ b/tests/ui/slow_vector_initialization.rs @@ -34,13 +34,13 @@ fn extend_vector() { fn mixed_extend_resize_vector() { // Mismatching len let mut mismatching_len = Vec::with_capacity(30); + mismatching_len.extend(repeat(0).take(40)); // Slow initialization let mut resized_vec = Vec::with_capacity(30); - let mut extend_vec = Vec::with_capacity(30); - resized_vec.resize(30, 0); - mismatching_len.extend(repeat(0).take(40)); + + let mut extend_vec = Vec::with_capacity(30); extend_vec.extend(repeat(0).take(30)); } @@ -70,3 +70,14 @@ fn unsafe_vector() { unsafe_vec.set_len(200); } } + +fn do_stuff(vec: &mut Vec) { + +} + +fn extend_vector_with_manipulations_between() { + let len = 300; + let mut vec1:Vec = Vec::with_capacity(len); + do_stuff(&mut vec1); + vec1.extend(repeat(0).take(len)); +} diff --git a/tests/ui/slow_vector_initialization.stderr b/tests/ui/slow_vector_initialization.stderr index c1e48d42a..577cc82c6 100644 --- a/tests/ui/slow_vector_initialization.stderr +++ b/tests/ui/slow_vector_initialization.stderr @@ -17,20 +17,18 @@ error: slow zero-filling initialization | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: slow zero-filling initialization - --> $DIR/slow_vector_initialization.rs:42:5 + --> $DIR/slow_vector_initialization.rs:41:5 | -39 | let mut resized_vec = Vec::with_capacity(30); +40 | let mut resized_vec = Vec::with_capacity(30); | ---------------------- help: consider replace allocation with: `vec![0; 30]` -... -42 | resized_vec.resize(30, 0); +41 | resized_vec.resize(30, 0); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: slow zero-filling initialization --> $DIR/slow_vector_initialization.rs:44:5 | -40 | let mut extend_vec = Vec::with_capacity(30); +43 | let mut extend_vec = Vec::with_capacity(30); | ---------------------- help: consider replace allocation with: `vec![0; 30]` -... 44 | extend_vec.extend(repeat(0).take(30)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From dc35841be4dd7efad4205e6d2d9f183b5866f6f6 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Sun, 25 Nov 2018 14:36:04 -0800 Subject: [PATCH 7/7] Update lints --- CHANGELOG.md | 2 ++ README.md | 2 +- clippy_lints/src/lib.rs | 3 +++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa05eca89..320a3511e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -842,6 +842,7 @@ All notable changes to this project will be documented in this file. [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else +[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add [`string_add_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add_assign @@ -880,6 +881,7 @@ All notable changes to this project will be documented in this file. [`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern [`unreadable_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal [`unsafe_removed_from_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_removed_from_name +[`unsafe_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_vector_initialization [`unseparated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#unseparated_literal_suffix [`unstable_as_mut_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_mut_slice [`unstable_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_slice diff --git a/README.md b/README.md index f2b0da3ae..9d142a2de 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 288 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cf754d486..35f47c1cd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -711,6 +711,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { returns::NEEDLESS_RETURN, returns::UNUSED_UNIT, serde_api::SERDE_API_MISUSE, + slow_vector_initialization::SLOW_VECTOR_INITIALIZATION, + slow_vector_initialization::UNSAFE_VECTOR_INITIALIZATION, strings::STRING_LIT_AS_BYTES, suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, @@ -957,6 +959,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { ranges::ITERATOR_STEP_BY_ZERO, regex::INVALID_REGEX, serde_api::SERDE_API_MISUSE, + slow_vector_initialization::UNSAFE_VECTOR_INITIALIZATION, suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL, suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, swap::ALMOST_SWAPPED,