From b1aa3064b6d86c5ae90a67e420d88d826bafe8be Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Tue, 5 Oct 2021 11:43:44 -0400 Subject: [PATCH] Address PR comments --- clippy_lints/src/uninit_vec.rs | 31 ++++++++++----- clippy_lints/src/vec_init_then_push.rs | 3 +- clippy_utils/src/higher.rs | 53 +++++++++++++++++++++++++- clippy_utils/src/lib.rs | 49 +----------------------- clippy_utils/src/paths.rs | 1 - tests/ui/uninit_vec.rs | 11 +++++- 6 files changed, 85 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 833c95f4940..5d6409874d8 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -1,9 +1,10 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::get_vec_init_kind; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{path_to_local_id, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq}; +use clippy_utils::higher::get_vec_init_kind; +use clippy_utils::ty::{is_type_diagnostic_item, is_uninit_value_valid_for_ty}; +use clippy_utils::{is_lint_allowed, path_to_local_id, peel_hir_expr_while, SpanlessEq}; use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, PathSegment, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::lint::in_external_macro; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{sym, Span}; @@ -16,10 +17,14 @@ declare_clippy_lint! { /// `with_capacity()` or `reserve()`. /// /// ### Why is this bad? - /// It creates a `Vec` with uninitialized data, which leads to an + /// It creates a `Vec` with uninitialized data, which leads to /// undefined behavior with most safe operations. + /// /// Notably, uninitialized `Vec` must not be used with generic `Read`. /// + /// ### Known Problems + /// This lint only checks directly adjacent statements. + /// /// ### Example /// ```rust,ignore /// let mut vec: Vec = Vec::with_capacity(1000); @@ -52,16 +57,20 @@ declare_clippy_lint! { declare_lint_pass!(UninitVec => [UNINIT_VEC]); +// FIXME: update to a visitor-based implementation. +// Threads: https://github.com/rust-lang/rust-clippy/pull/7682#discussion_r710998368 impl<'tcx> LateLintPass<'tcx> for UninitVec { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) { - for w in block.stmts.windows(2) { - if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind { - handle_uninit_vec_pair(cx, &w[0], expr); + if !in_external_macro(cx.tcx.sess, block.span) { + for w in block.stmts.windows(2) { + if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind { + handle_uninit_vec_pair(cx, &w[0], expr); + } } - } - if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) { - handle_uninit_vec_pair(cx, stmt, expr); + if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) { + handle_uninit_vec_pair(cx, stmt, expr); + } } } } @@ -79,6 +88,8 @@ fn handle_uninit_vec_pair( if let ty::Adt(_, substs) = vec_ty.kind(); // Check T of Vec if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)); + // `#[allow(...)]` attribute can be set on enclosing unsafe block of `set_len()` + if !is_lint_allowed(cx, UNINIT_VEC, maybe_set_len.hir_id); then { // FIXME: #7698, false positive of the internal lints #[allow(clippy::collapsible_span_lint_calls)] diff --git a/clippy_lints/src/vec_init_then_push.rs b/clippy_lints/src/vec_init_then_push.rs index 478314c0836..b92b6ca4f43 100644 --- a/clippy_lints/src/vec_init_then_push.rs +++ b/clippy_lints/src/vec_init_then_push.rs @@ -1,6 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::higher::{get_vec_init_kind, VecInitKind}; use clippy_utils::source::snippet; -use clippy_utils::{get_vec_init_kind, path_to_local, path_to_local_id, VecInitKind}; +use clippy_utils::{path_to_local, path_to_local_id}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, Stmt, StmtKind}; diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index ba4d50bf744..d60ddbd3c56 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -2,11 +2,14 @@ #![deny(clippy::missing_docs_in_private_items)] +use crate::ty::is_type_diagnostic_item; use crate::{is_expn_of, match_def_path, paths}; use if_chain::if_chain; use rustc_ast::ast::{self, LitKind}; use rustc_hir as hir; -use rustc_hir::{Arm, Block, BorrowKind, Expr, ExprKind, LoopSource, MatchSource, Node, Pat, StmtKind, UnOp}; +use rustc_hir::{ + Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp, +}; use rustc_lint::LateContext; use rustc_span::{sym, ExpnKind, Span, Symbol}; @@ -632,3 +635,51 @@ impl PanicExpn<'tcx> { } } } + +/// A parsed `Vec` initialization expression +#[derive(Clone, Copy)] +pub enum VecInitKind { + /// `Vec::new()` + New, + /// `Vec::default()` or `Default::default()` + Default, + /// `Vec::with_capacity(123)` + WithLiteralCapacity(u64), + /// `Vec::with_capacity(slice.len())` + WithExprCapacity(HirId), +} + +/// Checks if given expression is an initialization of `Vec` and returns its kind. +pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option { + if let ExprKind::Call(func, args) = expr.kind { + match func.kind { + ExprKind::Path(QPath::TypeRelative(ty, name)) + if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec) => + { + if name.ident.name == sym::new { + return Some(VecInitKind::New); + } else if name.ident.name.as_str() == "with_capacity" { + return args.get(0).and_then(|arg| { + if_chain! { + if let ExprKind::Lit(lit) = &arg.kind; + if let LitKind::Int(num, _) = lit.node; + then { + Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?)) + } else { + Some(VecInitKind::WithExprCapacity(arg.hir_id)) + } + } + }); + } + } + ExprKind::Path(QPath::Resolved(_, path)) + if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD) + && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) => + { + return Some(VecInitKind::Default); + } + _ => (), + } + } + None +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index b450059e18a..09eee78f0d1 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -93,7 +93,7 @@ use rustc_span::{Span, DUMMY_SP}; use rustc_target::abi::Integer; use crate::consts::{constant, Constant}; -use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type, is_type_diagnostic_item}; +use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type}; pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -1789,53 +1789,6 @@ pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool } } -#[derive(Clone, Copy)] -pub enum VecInitKind { - /// `Vec::new()` - New, - /// `Vec::default()` or `Default::default()` - Default, - /// `Vec::with_capacity(123)` - WithLiteralCapacity(u64), - /// `Vec::with_capacity(slice.len())` - WithExprCapacity(HirId), -} - -/// Checks if given expression is an initialization of `Vec` and returns its kind. -pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option { - if let ExprKind::Call(func, args) = expr.kind { - match func.kind { - ExprKind::Path(QPath::TypeRelative(ty, name)) - if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec) => - { - if name.ident.name == sym::new { - return Some(VecInitKind::New); - } else if name.ident.name.as_str() == "with_capacity" { - return args.get(0).and_then(|arg| { - if_chain! { - if let ExprKind::Lit(lit) = &arg.kind; - if let LitKind::Int(num, _) = lit.node; - then { - Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?)) - } else { - Some(VecInitKind::WithExprCapacity(arg.hir_id)) - } - } - }); - } - } - ExprKind::Path(QPath::Resolved(_, path)) - if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD) - && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) => - { - return Some(VecInitKind::Default); - } - _ => (), - } - } - None -} - /// Gets the node where an expression is either used, or it's type is unified with another branch. pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option> { let mut child_id = expr.hir_id; diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index a39c0fc0b10..e43c5756021 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -183,6 +183,5 @@ pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"]; pub const VEC_FROM_ELEM: [&str; 3] = ["alloc", "vec", "from_elem"]; pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"]; pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"]; -pub const VEC_SET_LEN: [&str; 4] = ["alloc", "vec", "Vec", "set_len"]; pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"]; pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"]; diff --git a/tests/ui/uninit_vec.rs b/tests/ui/uninit_vec.rs index e60b73a1e30..c8f9067e549 100644 --- a/tests/ui/uninit_vec.rs +++ b/tests/ui/uninit_vec.rs @@ -48,6 +48,13 @@ fn main() { my_vec.vec.set_len(200); } + // Test `#[allow(...)]` attributes on inner unsafe block (shouldn't trigger) + let mut vec: Vec = Vec::with_capacity(1000); + #[allow(clippy::uninit_vec)] + unsafe { + vec.set_len(200); + } + // MaybeUninit-wrapped types should not be detected unsafe { let mut vec: Vec> = Vec::with_capacity(1000); @@ -64,7 +71,7 @@ fn main() { let mut vec1: Vec = Vec::with_capacity(1000); let mut vec2: Vec = Vec::with_capacity(1000); unsafe { - vec1.reserve(200); - vec2.reserve(200); + vec1.set_len(200); + vec2.set_len(200); } }