Skip to content

Commit b1aa306

Browse files
QwazQwaz
committed
Address PR comments
1 parent dd9c8d3 commit b1aa306

File tree

6 files changed

+85
-63
lines changed

6 files changed

+85
-63
lines changed

‎clippy_lints/src/uninit_vec.rs‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2-
use clippy_utils::get_vec_init_kind;
3-
use clippy_utils::ty::is_type_diagnostic_item;
4-
use clippy_utils::{path_to_local_id, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq};
2+
use clippy_utils::higher::get_vec_init_kind;
3+
use clippy_utils::ty::{is_type_diagnostic_item, is_uninit_value_valid_for_ty};
4+
use clippy_utils::{is_lint_allowed, path_to_local_id, peel_hir_expr_while, SpanlessEq};
55
use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, PathSegment, Stmt, StmtKind};
66
use rustc_lint::{LateContext, LateLintPass};
7+
use rustc_middle::lint::in_external_macro;
78
use rustc_middle::ty;
89
use rustc_session::{declare_lint_pass, declare_tool_lint};
910
use rustc_span::{sym, Span};
@@ -16,10 +17,14 @@ declare_clippy_lint! {
1617
/// `with_capacity()` or `reserve()`.
1718
///
1819
/// ### Why is this bad?
19-
/// It creates a `Vec` with uninitialized data, which leads to an
20+
/// It creates a `Vec` with uninitialized data, which leads to
2021
/// undefined behavior with most safe operations.
22+
///
2123
/// Notably, uninitialized `Vec<u8>` must not be used with generic `Read`.
2224
///
25+
/// ### Known Problems
26+
/// This lint only checks directly adjacent statements.
27+
///
2328
/// ### Example
2429
/// ```rust,ignore
2530
/// let mut vec: Vec<u8> = Vec::with_capacity(1000);
@@ -52,16 +57,20 @@ declare_clippy_lint! {
5257

5358
declare_lint_pass!(UninitVec => [UNINIT_VEC]);
5459

60+
// FIXME: update to a visitor-based implementation.
61+
// Threads: https://github.com/rust-lang/rust-clippy/pull/7682#discussion_r710998368
5562
impl<'tcx> LateLintPass<'tcx> for UninitVec {
5663
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
57-
for w in block.stmts.windows(2) {
58-
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind {
59-
handle_uninit_vec_pair(cx, &w[0], expr);
64+
if !in_external_macro(cx.tcx.sess, block.span) {
65+
for w in block.stmts.windows(2) {
66+
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind {
67+
handle_uninit_vec_pair(cx, &w[0], expr);
68+
}
6069
}
61-
}
6270

63-
if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) {
64-
handle_uninit_vec_pair(cx, stmt, expr);
71+
if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) {
72+
handle_uninit_vec_pair(cx, stmt, expr);
73+
}
6574
}
6675
}
6776
}
@@ -79,6 +88,8 @@ fn handle_uninit_vec_pair(
7988
if let ty::Adt(_, substs) = vec_ty.kind();
8089
// Check T of Vec<T>
8190
if !is_uninit_value_valid_for_ty(cx, substs.type_at(0));
91+
// `#[allow(...)]` attribute can be set on enclosing unsafe block of `set_len()`
92+
if !is_lint_allowed(cx, UNINIT_VEC, maybe_set_len.hir_id);
8293
then {
8394
// FIXME: #7698, false positive of the internal lints
8495
#[allow(clippy::collapsible_span_lint_calls)]

‎clippy_lints/src/vec_init_then_push.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::higher::{get_vec_init_kind, VecInitKind};
23
use clippy_utils::source::snippet;
3-
use clippy_utils::{get_vec_init_kind, path_to_local, path_to_local_id, VecInitKind};
4+
use clippy_utils::{path_to_local, path_to_local_id};
45
use if_chain::if_chain;
56
use rustc_errors::Applicability;
67
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, Stmt, StmtKind};

‎clippy_utils/src/higher.rs‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@
22
33
#![deny(clippy::missing_docs_in_private_items)]
44

5+
use crate::ty::is_type_diagnostic_item;
56
use crate::{is_expn_of, match_def_path, paths};
67
use if_chain::if_chain;
78
use rustc_ast::ast::{self, LitKind};
89
use rustc_hir as hir;
9-
use rustc_hir::{Arm, Block, BorrowKind, Expr, ExprKind, LoopSource, MatchSource, Node, Pat, StmtKind, UnOp};
10+
use rustc_hir::{
11+
Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp,
12+
};
1013
use rustc_lint::LateContext;
1114
use rustc_span::{sym, ExpnKind, Span, Symbol};
1215

@@ -632,3 +635,51 @@ impl PanicExpn<'tcx> {
632635
}
633636
}
634637
}
638+
639+
/// A parsed `Vec` initialization expression
640+
#[derive(Clone, Copy)]
641+
pub enum VecInitKind {
642+
/// `Vec::new()`
643+
New,
644+
/// `Vec::default()` or `Default::default()`
645+
Default,
646+
/// `Vec::with_capacity(123)`
647+
WithLiteralCapacity(u64),
648+
/// `Vec::with_capacity(slice.len())`
649+
WithExprCapacity(HirId),
650+
}
651+
652+
/// Checks if given expression is an initialization of `Vec` and returns its kind.
653+
pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<VecInitKind> {
654+
if let ExprKind::Call(func, args) = expr.kind {
655+
match func.kind {
656+
ExprKind::Path(QPath::TypeRelative(ty, name))
657+
if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec) =>
658+
{
659+
if name.ident.name == sym::new {
660+
return Some(VecInitKind::New);
661+
} else if name.ident.name.as_str() == "with_capacity" {
662+
return args.get(0).and_then(|arg| {
663+
if_chain! {
664+
if let ExprKind::Lit(lit) = &arg.kind;
665+
if let LitKind::Int(num, _) = lit.node;
666+
then {
667+
Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?))
668+
} else {
669+
Some(VecInitKind::WithExprCapacity(arg.hir_id))
670+
}
671+
}
672+
});
673+
}
674+
}
675+
ExprKind::Path(QPath::Resolved(_, path))
676+
if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD)
677+
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) =>
678+
{
679+
return Some(VecInitKind::Default);
680+
}
681+
_ => (),
682+
}
683+
}
684+
None
685+
}

‎clippy_utils/src/lib.rs‎

Lines changed: 1 addition & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ use rustc_span::{Span, DUMMY_SP};
9393
use rustc_target::abi::Integer;
9494

9595
use crate::consts::{constant, Constant};
96-
use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type, is_type_diagnostic_item};
96+
use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type};
9797

9898
pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option<Span>) -> Option<RustcVersion> {
9999
if let Ok(version) = RustcVersion::parse(msrv) {
@@ -1789,53 +1789,6 @@ pub fn is_expr_identity_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
17891789
}
17901790
}
17911791

1792-
#[derive(Clone, Copy)]
1793-
pub enum VecInitKind {
1794-
/// `Vec::new()`
1795-
New,
1796-
/// `Vec::default()` or `Default::default()`
1797-
Default,
1798-
/// `Vec::with_capacity(123)`
1799-
WithLiteralCapacity(u64),
1800-
/// `Vec::with_capacity(slice.len())`
1801-
WithExprCapacity(HirId),
1802-
}
1803-
1804-
/// Checks if given expression is an initialization of `Vec` and returns its kind.
1805-
pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<VecInitKind> {
1806-
if let ExprKind::Call(func, args) = expr.kind {
1807-
match func.kind {
1808-
ExprKind::Path(QPath::TypeRelative(ty, name))
1809-
if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec) =>
1810-
{
1811-
if name.ident.name == sym::new {
1812-
return Some(VecInitKind::New);
1813-
} else if name.ident.name.as_str() == "with_capacity" {
1814-
return args.get(0).and_then(|arg| {
1815-
if_chain! {
1816-
if let ExprKind::Lit(lit) = &arg.kind;
1817-
if let LitKind::Int(num, _) = lit.node;
1818-
then {
1819-
Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?))
1820-
} else {
1821-
Some(VecInitKind::WithExprCapacity(arg.hir_id))
1822-
}
1823-
}
1824-
});
1825-
}
1826-
}
1827-
ExprKind::Path(QPath::Resolved(_, path))
1828-
if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD)
1829-
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) =>
1830-
{
1831-
return Some(VecInitKind::Default);
1832-
}
1833-
_ => (),
1834-
}
1835-
}
1836-
None
1837-
}
1838-
18391792
/// Gets the node where an expression is either used, or it's type is unified with another branch.
18401793
pub fn get_expr_use_or_unification_node(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<Node<'tcx>> {
18411794
let mut child_id = expr.hir_id;

‎clippy_utils/src/paths.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,5 @@ pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];
183183
pub const VEC_FROM_ELEM: [&str; 3] = ["alloc", "vec", "from_elem"];
184184
pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"];
185185
pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
186-
pub const VEC_SET_LEN: [&str; 4] = ["alloc", "vec", "Vec", "set_len"];
187186
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
188187
pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];

‎tests/ui/uninit_vec.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@ fn main() {
4848
my_vec.vec.set_len(200);
4949
}
5050

51+
// Test `#[allow(...)]` attributes on inner unsafe block (shouldn't trigger)
52+
let mut vec: Vec<u8> = Vec::with_capacity(1000);
53+
#[allow(clippy::uninit_vec)]
54+
unsafe {
55+
vec.set_len(200);
56+
}
57+
5158
// MaybeUninit-wrapped types should not be detected
5259
unsafe {
5360
let mut vec: Vec<MaybeUninit<u8>> = Vec::with_capacity(1000);
@@ -64,7 +71,7 @@ fn main() {
6471
let mut vec1: Vec<u8> = Vec::with_capacity(1000);
6572
let mut vec2: Vec<u8> = Vec::with_capacity(1000);
6673
unsafe {
67-
vec1.reserve(200);
68-
vec2.reserve(200);
74+
vec1.set_len(200);
75+
vec2.set_len(200);
6976
}
7077
}

0 commit comments

Comments
 (0)