about summary refs log tree commit diff
diff options
context:
space:
mode:
authorYechan Bae <yechan@gatech.edu>2021-10-05 11:43:44 -0400
committerYechan Bae <yechan@gatech.edu>2021-10-09 05:47:06 -0400
commitb1aa3064b6d86c5ae90a67e420d88d826bafe8be (patch)
tree61e209d33154673478fe6f51dcea0ff9603a9528
parentdd9c8d32f2d7984545816ee07e1e7213b36013f0 (diff)
downloadrust-b1aa3064b6d86c5ae90a67e420d88d826bafe8be.tar.gz
rust-b1aa3064b6d86c5ae90a67e420d88d826bafe8be.zip
Address PR comments
-rw-r--r--clippy_lints/src/uninit_vec.rs31
-rw-r--r--clippy_lints/src/vec_init_then_push.rs3
-rw-r--r--clippy_utils/src/higher.rs53
-rw-r--r--clippy_utils/src/lib.rs49
-rw-r--r--clippy_utils/src/paths.rs1
-rw-r--r--tests/ui/uninit_vec.rs11
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<u8>` must not be used with generic `Read`.
     ///
+    /// ### Known Problems
+    /// This lint only checks directly adjacent statements.
+    ///
     /// ### Example
     /// ```rust,ignore
     /// let mut vec: Vec<u8> = 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<T>
         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<VecInitKind> {
+    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<Span>) -> Option<RustcVersion> {
     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<VecInitKind> {
-    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<Node<'tcx>> {
     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<u8> = 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<MaybeUninit<u8>> = Vec::with_capacity(1000);
@@ -64,7 +71,7 @@ fn main() {
     let mut vec1: Vec<u8> = Vec::with_capacity(1000);
     let mut vec2: Vec<u8> = Vec::with_capacity(1000);
     unsafe {
-        vec1.reserve(200);
-        vec2.reserve(200);
+        vec1.set_len(200);
+        vec2.set_len(200);
     }
 }