From 452181c69d2a106d9d5a1262b69e68a765c4b3e3 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Fri, 17 Sep 2021 02:55:26 -0400 Subject: [PATCH 01/11] Implement uninit_vec lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_correctness.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + .../src/methods/uninit_assumed_init.rs | 14 +- clippy_lints/src/uninit_vec.rs | 174 ++++++++++++++++++ clippy_utils/src/lib.rs | 10 + clippy_utils/src/paths.rs | 3 + tests/ui/uninit_vec.rs | 32 ++++ tests/ui/uninit_vec.stderr | 51 +++++ 11 files changed, 278 insertions(+), 12 deletions(-) create mode 100644 clippy_lints/src/uninit_vec.rs create mode 100644 tests/ui/uninit_vec.rs create mode 100644 tests/ui/uninit_vec.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 13067e4e92ae..2c69432f31c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3038,6 +3038,7 @@ Released 2018-09-13 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented [`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init +[`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg [`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp [`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 3e6e0244754f..f0ef0befea03 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -278,6 +278,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(types::VEC_BOX), LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(unicode::INVISIBLE_CHARACTERS), + LintId::of(uninit_vec::UNINIT_VEC), LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), LintId::of(unit_types::UNIT_ARG), LintId::of(unit_types::UNIT_CMP), diff --git a/clippy_lints/src/lib.register_correctness.rs b/clippy_lints/src/lib.register_correctness.rs index e0ef7b3b8af9..8eb662195e23 100644 --- a/clippy_lints/src/lib.register_correctness.rs +++ b/clippy_lints/src/lib.register_correctness.rs @@ -63,6 +63,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve LintId::of(transmuting_null::TRANSMUTING_NULL), LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(unicode::INVISIBLE_CHARACTERS), + LintId::of(uninit_vec::UNINIT_VEC), LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), LintId::of(unit_types::UNIT_CMP), LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 3c4b720671a6..401c5b78b383 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -470,6 +470,7 @@ store.register_lints(&[ unicode::INVISIBLE_CHARACTERS, unicode::NON_ASCII_LITERAL, unicode::UNICODE_NOT_NFC, + uninit_vec::UNINIT_VEC, unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD, unit_types::LET_UNIT_VALUE, unit_types::UNIT_ARG, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7e1bcbbd0ed0..829d5acdfde3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -361,6 +361,7 @@ mod types; mod undocumented_unsafe_blocks; mod undropped_manually_drops; mod unicode; +mod uninit_vec; mod unit_return_expecting_ord; mod unit_types; mod unnamed_address; @@ -518,6 +519,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(blocks_in_if_conditions::BlocksInIfConditions)); store.register_late_pass(|| Box::new(collapsible_match::CollapsibleMatch)); store.register_late_pass(|| Box::new(unicode::Unicode)); + store.register_late_pass(|| Box::new(uninit_vec::UninitVec)); store.register_late_pass(|| Box::new(unit_return_expecting_ord::UnitReturnExpectingOrd)); store.register_late_pass(|| Box::new(strings::StringAdd)); store.register_late_pass(|| Box::new(implicit_return::ImplicitReturn)); diff --git a/clippy_lints/src/methods/uninit_assumed_init.rs b/clippy_lints/src/methods/uninit_assumed_init.rs index 1a5894e48d14..dbd91fb6d9de 100644 --- a/clippy_lints/src/methods/uninit_assumed_init.rs +++ b/clippy_lints/src/methods/uninit_assumed_init.rs @@ -1,9 +1,8 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::{is_expr_path_def_path, match_def_path, paths}; +use clippy_utils::{is_expr_path_def_path, is_uninit_ty_valid, paths}; use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::LateContext; -use rustc_middle::ty::{self, Ty}; use super::UNINIT_ASSUMED_INIT; @@ -13,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr if let hir::ExprKind::Call(callee, args) = recv.kind; if args.is_empty(); if is_expr_path_def_path(cx, callee, &paths::MEM_MAYBEUNINIT_UNINIT); - if !is_maybe_uninit_ty_valid(cx, cx.typeck_results().expr_ty_adjusted(expr)); + if !is_uninit_ty_valid(cx, cx.typeck_results().expr_ty_adjusted(expr)); then { span_lint( cx, @@ -24,12 +23,3 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr } } } - -fn is_maybe_uninit_ty_valid(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { - match ty.kind() { - ty::Array(component, _) => is_maybe_uninit_ty_valid(cx, component), - ty::Tuple(types) => types.types().all(|ty| is_maybe_uninit_ty_valid(cx, ty)), - ty::Adt(adt, _) => match_def_path(cx, adt.did, &paths::MEM_MAYBEUNINIT), - _ => false, - } -} diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs new file mode 100644 index 000000000000..ff302289a1fe --- /dev/null +++ b/clippy_lints/src/uninit_vec.rs @@ -0,0 +1,174 @@ +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::{is_uninit_ty_valid, match_def_path, path_to_local_id, paths, peel_hir_expr_while, SpanlessEq}; +use rustc_hir::def::Res; +use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Checks for the creation of uninitialized `Vec` by calling `set_len()` + /// immediately after `with_capacity()` or `reserve()`. + /// + /// ### Why is this bad? + /// It creates `Vec` that contains uninitialized data, which leads to an + /// undefined behavior with most safe operations. + /// Notably, using uninitialized `Vec` with generic `Read` is unsound. + /// + /// ### Example + /// ```rust,ignore + /// let mut vec: Vec = Vec::with_capacity(1000); + /// unsafe { vec.set_len(1000); } + /// reader.read(&mut vec); // undefined behavior! + /// ``` + /// Use an initialized buffer: + /// ```rust,ignore + /// let mut vec: Vec = vec![0; 1000]; + /// reader.read(&mut vec); + /// ``` + /// Or, wrap the content in `MaybeUninit`: + /// ```rust,ignore + /// let mut vec: Vec> = Vec::with_capacity(1000); + /// unsafe { vec.set_len(1000); } + /// ``` + pub UNINIT_VEC, + correctness, + "Vec with uninitialized data" +} + +declare_lint_pass!(UninitVec => [UNINIT_VEC]); + +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_pair(cx, &w[0], expr); + } + } + + if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) { + handle_pair(cx, stmt, expr); + } + } +} + +fn handle_pair(cx: &LateContext<'tcx>, first: &'tcx Stmt<'tcx>, second: &'tcx Expr<'tcx>) { + if_chain! { + if let Some(vec) = extract_with_capacity_or_reserve_target(cx, first); + if let Some((set_len_self, call_span)) = extract_set_len_self(cx, second); + if vec.eq_expr(cx, set_len_self); + if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind(); + if let ty::Adt(_, substs) = vec_ty.kind(); + // Check T of Vec + if !is_uninit_ty_valid(cx, substs.type_at(0)); + then { + span_lint_and_note( + cx, + UNINIT_VEC, + call_span, + "calling `set_len()` immediately after reserving a buffer creates uninitialized values", + Some(first.span), + "the buffer is reserved here" + ); + } + } +} + +#[derive(Clone, Copy, Debug)] +enum LocalOrExpr<'tcx> { + Local(HirId), + Expr(&'tcx Expr<'tcx>), +} + +impl<'tcx> LocalOrExpr<'tcx> { + fn eq_expr(self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + match self { + LocalOrExpr::Local(hir_id) => path_to_local_id(expr, hir_id), + LocalOrExpr::Expr(self_expr) => SpanlessEq::new(cx).eq_expr(self_expr, expr), + } + } +} + +/// Returns the target vec of Vec::with_capacity() or Vec::reserve() +fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stmt<'_>) -> Option> { + match stmt.kind { + StmtKind::Local(local) => { + // let mut x = Vec::with_capacity() + if_chain! { + if let Some(init_expr) = local.init; + if let PatKind::Binding(_, hir_id, _, None) = local.pat.kind; + if is_with_capacity(cx, init_expr); + then { + Some(LocalOrExpr::Local(hir_id)) + } else { + None + } + } + }, + StmtKind::Expr(expr) | StmtKind::Semi(expr) => { + match expr.kind { + ExprKind::Assign(lhs, rhs, _span) if is_with_capacity(cx, rhs) => { + // self.vec = Vec::with_capacity() + Some(LocalOrExpr::Expr(lhs)) + }, + ExprKind::MethodCall(_, _, [vec_expr, _], _) => { + // self.vec.reserve() + if_chain! { + if let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); + if match_def_path(cx, id, &paths::VEC_RESERVE); + then { + Some(LocalOrExpr::Expr(vec_expr)) + } else { + None + } + } + }, + _ => None, + } + }, + _ => None, + } +} + +fn is_with_capacity(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> bool { + if_chain! { + if let ExprKind::Call(path_expr, _) = &expr.kind; + if let ExprKind::Path(qpath) = &path_expr.kind; + if let Res::Def(_, def_id) = cx.qpath_res(qpath, path_expr.hir_id); + then { + match_def_path(cx, def_id, &paths::VEC_WITH_CAPACITY) + } else { + false + } + } +} + +/// Returns self if the expression is Vec::set_len() +fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(&'tcx Expr<'tcx>, Span)> { + // peel unsafe blocks in `unsafe { vec.set_len() }` + let expr = peel_hir_expr_while(expr, |e| { + if let ExprKind::Block(block, _) = e.kind { + match (block.stmts.get(0).map(|stmt| &stmt.kind), block.expr) { + (None, Some(expr)) => Some(expr), + (Some(StmtKind::Expr(expr) | StmtKind::Semi(expr)), None) => Some(expr), + _ => None, + } + } else { + None + } + }); + match expr.kind { + ExprKind::MethodCall(_, _, [vec_expr, _], _) => { + cx.typeck_results().type_dependent_def_id(expr.hir_id).and_then(|id| { + if match_def_path(cx, id, &paths::VEC_SET_LEN) { + Some((vec_expr, expr.span)) + } else { + None + } + }) + }, + _ => None, + } +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 09eee78f0d1f..00d2098a021b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1470,6 +1470,16 @@ pub fn is_self_ty(slf: &hir::Ty<'_>) -> bool { false } +/// Checks if a given type looks safe to be uninitialized. +pub fn is_uninit_ty_valid(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + match ty.kind() { + rustc_ty::Array(component, _) => is_uninit_ty_valid(cx, component), + rustc_ty::Tuple(types) => types.types().all(|ty| is_uninit_ty_valid(cx, ty)), + rustc_ty::Adt(adt, _) => match_def_path(cx, adt.did, &paths::MEM_MAYBEUNINIT), + _ => false, + } +} + pub fn iter_input_pats<'tcx>(decl: &FnDecl<'_>, body: &'tcx Body<'_>) -> impl Iterator> { (0..decl.inputs.len()).map(move |i| &body.params[i]) } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index e43c57560214..dffe30910dd5 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -183,5 +183,8 @@ 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_RESERVE: [&str; 4] = ["alloc", "vec", "Vec", "reserve"]; +pub const VEC_WITH_CAPACITY: [&str; 4] = ["alloc", "vec", "Vec", "with_capacity"]; +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 new file mode 100644 index 000000000000..5e67288405ad --- /dev/null +++ b/tests/ui/uninit_vec.rs @@ -0,0 +1,32 @@ +#![warn(clippy::uninit_vec)] + +use std::mem::MaybeUninit; + +fn main() { + // with_capacity() -> set_len() should be detected + let mut vec: Vec = Vec::with_capacity(1000); + unsafe { + vec.set_len(200); + } + + // reserve() -> set_len() should be detected + vec.reserve(1000); + unsafe { + vec.set_len(200); + } + + // test when both calls are enclosed in the same unsafe block + unsafe { + let mut vec: Vec = Vec::with_capacity(1000); + vec.set_len(200); + + vec.reserve(1000); + vec.set_len(200); + } + + // MaybeUninit-wrapped types should not be detected + let mut vec: Vec> = Vec::with_capacity(1000); + unsafe { + vec.set_len(200); + } +} diff --git a/tests/ui/uninit_vec.stderr b/tests/ui/uninit_vec.stderr new file mode 100644 index 000000000000..1d7807fcf7d0 --- /dev/null +++ b/tests/ui/uninit_vec.stderr @@ -0,0 +1,51 @@ +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:9:9 + | +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::uninit-vec` implied by `-D warnings` +note: the buffer is reserved here + --> $DIR/uninit_vec.rs:7:5 + | +LL | let mut vec: Vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:15:9 + | +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | +note: the buffer is reserved here + --> $DIR/uninit_vec.rs:13:5 + | +LL | vec.reserve(1000); + | ^^^^^^^^^^^^^^^^^^ + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:21:9 + | +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | +note: the buffer is reserved here + --> $DIR/uninit_vec.rs:20:9 + | +LL | let mut vec: Vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:24:9 + | +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | +note: the buffer is reserved here + --> $DIR/uninit_vec.rs:23:9 + | +LL | vec.reserve(1000); + | ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors + From b8ba7269cd10843e4aebf855c4f72e980a9c0ed6 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Fri, 17 Sep 2021 03:27:31 -0400 Subject: [PATCH 02/11] Fix clippy lints --- clippy_lints/src/uninit_vec.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index ff302289a1fe..8b1254b60f91 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -91,7 +91,7 @@ impl<'tcx> LocalOrExpr<'tcx> { } } -/// Returns the target vec of Vec::with_capacity() or Vec::reserve() +/// Returns the target vec of `Vec::with_capacity()` or `Vec::reserve()` fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stmt<'_>) -> Option> { match stmt.kind { StmtKind::Local(local) => { @@ -128,7 +128,7 @@ fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stm _ => None, } }, - _ => None, + StmtKind::Item(_) => None, } } @@ -145,7 +145,7 @@ fn is_with_capacity(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> bool { } } -/// Returns self if the expression is Vec::set_len() +/// Returns self if the expression is `Vec::set_len()` fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(&'tcx Expr<'tcx>, Span)> { // peel unsafe blocks in `unsafe { vec.set_len() }` let expr = peel_hir_expr_while(expr, |e| { From 759200b6991b5dac5fdb12bc6c366b16850add00 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Fri, 17 Sep 2021 14:42:32 -0400 Subject: [PATCH 03/11] Handle PR feedbacks first round --- .../src/methods/uninit_assumed_init.rs | 4 +- clippy_lints/src/uninit_vec.rs | 41 +++++++++++-------- clippy_utils/src/lib.rs | 10 ----- clippy_utils/src/paths.rs | 1 - clippy_utils/src/ty.rs | 10 +++++ tests/ui/uninit_vec.rs | 8 +++- 6 files changed, 42 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/methods/uninit_assumed_init.rs b/clippy_lints/src/methods/uninit_assumed_init.rs index dbd91fb6d9de..ce89189bce97 100644 --- a/clippy_lints/src/methods/uninit_assumed_init.rs +++ b/clippy_lints/src/methods/uninit_assumed_init.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::{is_expr_path_def_path, is_uninit_ty_valid, paths}; +use clippy_utils::{is_expr_path_def_path, paths, ty::is_uninit_value_valid_for_ty}; use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::LateContext; @@ -12,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr if let hir::ExprKind::Call(callee, args) = recv.kind; if args.is_empty(); if is_expr_path_def_path(cx, callee, &paths::MEM_MAYBEUNINIT_UNINIT); - if !is_uninit_ty_valid(cx, cx.typeck_results().expr_ty_adjusted(expr)); + if !is_uninit_value_valid_for_ty(cx, cx.typeck_results().expr_ty_adjusted(expr)); then { span_lint( cx, diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 8b1254b60f91..37084f57043b 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -1,11 +1,14 @@ use clippy_utils::diagnostics::span_lint_and_note; -use clippy_utils::{is_uninit_ty_valid, match_def_path, path_to_local_id, paths, peel_hir_expr_while, SpanlessEq}; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{ + match_def_path, path_to_local_id, paths, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq, +}; use rustc_hir::def::Res; use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::Span; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -44,32 +47,36 @@ 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_pair(cx, &w[0], expr); + handle_uninit_vec_pair(cx, &w[0], expr); } } if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) { - handle_pair(cx, stmt, expr); + handle_uninit_vec_pair(cx, stmt, expr); } } } -fn handle_pair(cx: &LateContext<'tcx>, first: &'tcx Stmt<'tcx>, second: &'tcx Expr<'tcx>) { +fn handle_uninit_vec_pair( + cx: &LateContext<'tcx>, + maybe_with_capacity_or_reserve: &'tcx Stmt<'tcx>, + maybe_set_len: &'tcx Expr<'tcx>, +) { if_chain! { - if let Some(vec) = extract_with_capacity_or_reserve_target(cx, first); - if let Some((set_len_self, call_span)) = extract_set_len_self(cx, second); + if let Some(vec) = extract_with_capacity_or_reserve_target(cx, maybe_with_capacity_or_reserve); + if let Some((set_len_self, call_span)) = extract_set_len_self(cx, maybe_set_len); if vec.eq_expr(cx, set_len_self); if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind(); if let ty::Adt(_, substs) = vec_ty.kind(); // Check T of Vec - if !is_uninit_ty_valid(cx, substs.type_at(0)); + if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)); then { span_lint_and_note( cx, UNINIT_VEC, call_span, "calling `set_len()` immediately after reserving a buffer creates uninitialized values", - Some(first.span), + Some(maybe_with_capacity_or_reserve.span), "the buffer is reserved here" ); } @@ -113,16 +120,14 @@ fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stm // self.vec = Vec::with_capacity() Some(LocalOrExpr::Expr(lhs)) }, - ExprKind::MethodCall(_, _, [vec_expr, _], _) => { + ExprKind::MethodCall(path, _, [self_expr, _], _) => { // self.vec.reserve() - if_chain! { - if let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); - if match_def_path(cx, id, &paths::VEC_RESERVE); - then { - Some(LocalOrExpr::Expr(vec_expr)) - } else { - None - } + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), sym::vec_type) + && path.ident.name.as_str() == "reserve" + { + Some(LocalOrExpr::Expr(self_expr)) + } else { + None } }, _ => None, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 00d2098a021b..09eee78f0d1f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1470,16 +1470,6 @@ pub fn is_self_ty(slf: &hir::Ty<'_>) -> bool { false } -/// Checks if a given type looks safe to be uninitialized. -pub fn is_uninit_ty_valid(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { - match ty.kind() { - rustc_ty::Array(component, _) => is_uninit_ty_valid(cx, component), - rustc_ty::Tuple(types) => types.types().all(|ty| is_uninit_ty_valid(cx, ty)), - rustc_ty::Adt(adt, _) => match_def_path(cx, adt.did, &paths::MEM_MAYBEUNINIT), - _ => false, - } -} - pub fn iter_input_pats<'tcx>(decl: &FnDecl<'_>, body: &'tcx Body<'_>) -> impl Iterator> { (0..decl.inputs.len()).map(move |i| &body.params[i]) } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index dffe30910dd5..63ec10a31786 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -183,7 +183,6 @@ 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_RESERVE: [&str; 4] = ["alloc", "vec", "Vec", "reserve"]; pub const VEC_WITH_CAPACITY: [&str; 4] = ["alloc", "vec", "Vec", "with_capacity"]; pub const VEC_SET_LEN: [&str; 4] = ["alloc", "vec", "Vec", "set_len"]; pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"]; diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 6ebe1a0028a3..ca64ac7de3ee 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -367,3 +367,13 @@ pub fn same_type_and_consts(a: Ty<'tcx>, b: Ty<'tcx>) -> bool { _ => a == b, } } + +/// Checks if a given type looks safe to be uninitialized. +pub fn is_uninit_value_valid_for_ty(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Array(component, _) => is_uninit_value_valid_for_ty(cx, component), + ty::Tuple(types) => types.types().all(|ty| is_uninit_value_valid_for_ty(cx, ty)), + ty::Adt(adt, _) => cx.tcx.lang_items().maybe_uninit() == Some(adt.did), + _ => false, + } +} diff --git a/tests/ui/uninit_vec.rs b/tests/ui/uninit_vec.rs index 5e67288405ad..34b9e07ef1d4 100644 --- a/tests/ui/uninit_vec.rs +++ b/tests/ui/uninit_vec.rs @@ -25,8 +25,14 @@ fn main() { } // MaybeUninit-wrapped types should not be detected - let mut vec: Vec> = Vec::with_capacity(1000); unsafe { + let mut vec: Vec> = Vec::with_capacity(1000); + vec.set_len(200); + + let mut vec: Vec<(MaybeUninit, MaybeUninit)> = Vec::with_capacity(1000); + vec.set_len(200); + + let mut vec: Vec<(MaybeUninit, [MaybeUninit; 2])> = Vec::with_capacity(1000); vec.set_len(200); } } From fdc06d9b2b3e858878b6fd8f6bd815d947e9950a Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Mon, 20 Sep 2021 15:32:53 -0400 Subject: [PATCH 04/11] Improve error messages --- clippy_lints/src/uninit_vec.rs | 14 ++++--- tests/ui/uninit_vec.rs | 32 ++++++++++++++++ tests/ui/uninit_vec.stderr | 69 +++++++++++++++++++++++----------- 3 files changed, 88 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 37084f57043b..b9b575a452b1 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{ match_def_path, path_to_local_id, paths, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq, @@ -71,13 +71,14 @@ fn handle_uninit_vec_pair( // Check T of Vec if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)); then { - span_lint_and_note( + span_lint_and_then( cx, UNINIT_VEC, - call_span, + vec![call_span, maybe_with_capacity_or_reserve.span], "calling `set_len()` immediately after reserving a buffer creates uninitialized values", - Some(maybe_with_capacity_or_reserve.span), - "the buffer is reserved here" + |diag| { + diag.help("initialize the buffer or wrap the content in `MaybeUninit`"); + }, ); } } @@ -155,9 +156,10 @@ fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(& // peel unsafe blocks in `unsafe { vec.set_len() }` let expr = peel_hir_expr_while(expr, |e| { if let ExprKind::Block(block, _) = e.kind { + // Extract the first statement/expression match (block.stmts.get(0).map(|stmt| &stmt.kind), block.expr) { (None, Some(expr)) => Some(expr), - (Some(StmtKind::Expr(expr) | StmtKind::Semi(expr)), None) => Some(expr), + (Some(StmtKind::Expr(expr) | StmtKind::Semi(expr)), _) => Some(expr), _ => None, } } else { diff --git a/tests/ui/uninit_vec.rs b/tests/ui/uninit_vec.rs index 34b9e07ef1d4..e60b73a1e308 100644 --- a/tests/ui/uninit_vec.rs +++ b/tests/ui/uninit_vec.rs @@ -2,6 +2,11 @@ use std::mem::MaybeUninit; +#[derive(Default)] +struct MyVec { + vec: Vec, +} + fn main() { // with_capacity() -> set_len() should be detected let mut vec: Vec = Vec::with_capacity(1000); @@ -24,6 +29,25 @@ fn main() { vec.set_len(200); } + let mut vec: Vec = Vec::with_capacity(1000); + unsafe { + // test the case where there are other statements in the following unsafe block + vec.set_len(200); + assert!(vec.len() == 200); + } + + // handle vec stored in the field of a struct + let mut my_vec = MyVec::default(); + my_vec.vec.reserve(1000); + unsafe { + my_vec.vec.set_len(200); + } + + my_vec.vec = Vec::with_capacity(1000); + unsafe { + my_vec.vec.set_len(200); + } + // MaybeUninit-wrapped types should not be detected unsafe { let mut vec: Vec> = Vec::with_capacity(1000); @@ -35,4 +59,12 @@ fn main() { let mut vec: Vec<(MaybeUninit, [MaybeUninit; 2])> = Vec::with_capacity(1000); vec.set_len(200); } + + // known false negative + let mut vec1: Vec = Vec::with_capacity(1000); + let mut vec2: Vec = Vec::with_capacity(1000); + unsafe { + vec1.reserve(200); + vec2.reserve(200); + } } diff --git a/tests/ui/uninit_vec.stderr b/tests/ui/uninit_vec.stderr index 1d7807fcf7d0..31f8ae40350c 100644 --- a/tests/ui/uninit_vec.stderr +++ b/tests/ui/uninit_vec.stderr @@ -1,51 +1,78 @@ error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:9:9 + --> $DIR/uninit_vec.rs:12:5 | +LL | let mut vec: Vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ | = note: `-D clippy::uninit-vec` implied by `-D warnings` -note: the buffer is reserved here - --> $DIR/uninit_vec.rs:7:5 - | -LL | let mut vec: Vec = Vec::with_capacity(1000); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:15:9 + --> $DIR/uninit_vec.rs:18:5 | +LL | vec.reserve(1000); + | ^^^^^^^^^^^^^^^^^^ +LL | unsafe { LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ | -note: the buffer is reserved here - --> $DIR/uninit_vec.rs:13:5 - | -LL | vec.reserve(1000); - | ^^^^^^^^^^^^^^^^^^ + = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:21:9 + --> $DIR/uninit_vec.rs:32:5 | +LL | let mut vec: Vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ | -note: the buffer is reserved here - --> $DIR/uninit_vec.rs:20:9 + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:41:5 + | +LL | my_vec.vec.reserve(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { +LL | my_vec.vec.set_len(200); + | ^^^^^^^^^^^^^^^^^^^^^^^ | -LL | let mut vec: Vec = Vec::with_capacity(1000); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:46:5 + | +LL | my_vec.vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { +LL | my_vec.vec.set_len(200); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:24:9 + --> $DIR/uninit_vec.rs:25:9 | +LL | let mut vec: Vec = Vec::with_capacity(1000); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ | -note: the buffer is reserved here - --> $DIR/uninit_vec.rs:23:9 + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:28:9 | LL | vec.reserve(1000); | ^^^^^^^^^^^^^^^^^^ +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | + = help: initialize the buffer or wrap the content in `MaybeUninit` -error: aborting due to 4 previous errors +error: aborting due to 7 previous errors From fec20bf61799764fab2b0d00d4666f4a9a9632c2 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Tue, 21 Sep 2021 09:56:56 -0400 Subject: [PATCH 05/11] Add #allow attribute to suppress FP #7698 --- clippy_lints/src/uninit_vec.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index b9b575a452b1..4d556d62c9bd 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -71,6 +71,8 @@ fn handle_uninit_vec_pair( // Check T of Vec if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)); then { + // FIXME: false positive #7698 + #[allow(clippy::collapsible_span_lint_calls)] span_lint_and_then( cx, UNINIT_VEC, From dd9c8d32f2d7984545816ee07e1e7213b36013f0 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Sun, 3 Oct 2021 00:23:57 -0400 Subject: [PATCH 06/11] Extract get_vec_init_kind and share it --- clippy_lints/src/uninit_vec.rs | 116 +++++++++++-------------- clippy_lints/src/vec_init_then_push.rs | 51 ++--------- clippy_utils/src/lib.rs | 49 ++++++++++- clippy_utils/src/paths.rs | 1 - 4 files changed, 105 insertions(+), 112 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 4d556d62c9bd..833c95f49403 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -1,24 +1,24 @@ 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::{ - match_def_path, path_to_local_id, paths, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq, -}; -use rustc_hir::def::Res; -use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, Stmt, StmtKind}; +use clippy_utils::{path_to_local_id, peel_hir_expr_while, ty::is_uninit_value_valid_for_ty, SpanlessEq}; +use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, PathSegment, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{sym, Span}; +// TODO: add `ReadBuf` (RFC 2930) in "How to fix" once it is available in std declare_clippy_lint! { /// ### What it does - /// Checks for the creation of uninitialized `Vec` by calling `set_len()` - /// immediately after `with_capacity()` or `reserve()`. + /// Checks for `set_len()` call that creates `Vec` with uninitialized elements. + /// This is commonly caused by calling `set_len()` right after after calling + /// `with_capacity()` or `reserve()`. /// /// ### Why is this bad? - /// It creates `Vec` that contains uninitialized data, which leads to an + /// It creates a `Vec` with uninitialized data, which leads to an /// undefined behavior with most safe operations. - /// Notably, using uninitialized `Vec` with generic `Read` is unsound. + /// Notably, uninitialized `Vec` must not be used with generic `Read`. /// /// ### Example /// ```rust,ignore @@ -26,16 +26,25 @@ declare_clippy_lint! { /// unsafe { vec.set_len(1000); } /// reader.read(&mut vec); // undefined behavior! /// ``` - /// Use an initialized buffer: - /// ```rust,ignore - /// let mut vec: Vec = vec![0; 1000]; - /// reader.read(&mut vec); - /// ``` - /// Or, wrap the content in `MaybeUninit`: - /// ```rust,ignore - /// let mut vec: Vec> = Vec::with_capacity(1000); - /// unsafe { vec.set_len(1000); } - /// ``` + /// + /// ### How to fix? + /// 1. Use an initialized buffer: + /// ```rust,ignore + /// let mut vec: Vec = vec![0; 1000]; + /// reader.read(&mut vec); + /// ``` + /// 2. Wrap the content in `MaybeUninit`: + /// ```rust,ignore + /// let mut vec: Vec> = Vec::with_capacity(1000); + /// vec.set_len(1000); // `MaybeUninit` can be uninitialized + /// ``` + /// 3. If you are on nightly, `Vec::spare_capacity_mut()` is available: + /// ```rust,ignore + /// let mut vec: Vec = Vec::with_capacity(1000); + /// let remaining = vec.spare_capacity_mut(); // `&mut [MaybeUninit]` + /// // perform initialization with `remaining` + /// vec.set_len(...); // Safe to call `set_len()` on initialized part + /// ``` pub UNINIT_VEC, correctness, "Vec with uninitialized data" @@ -59,11 +68,11 @@ impl<'tcx> LateLintPass<'tcx> for UninitVec { fn handle_uninit_vec_pair( cx: &LateContext<'tcx>, - maybe_with_capacity_or_reserve: &'tcx Stmt<'tcx>, + maybe_init_or_reserve: &'tcx Stmt<'tcx>, maybe_set_len: &'tcx Expr<'tcx>, ) { if_chain! { - if let Some(vec) = extract_with_capacity_or_reserve_target(cx, maybe_with_capacity_or_reserve); + if let Some(vec) = extract_init_or_reserve_target(cx, maybe_init_or_reserve); if let Some((set_len_self, call_span)) = extract_set_len_self(cx, maybe_set_len); if vec.eq_expr(cx, set_len_self); if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind(); @@ -71,12 +80,12 @@ fn handle_uninit_vec_pair( // Check T of Vec if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)); then { - // FIXME: false positive #7698 + // FIXME: #7698, false positive of the internal lints #[allow(clippy::collapsible_span_lint_calls)] span_lint_and_then( cx, UNINIT_VEC, - vec![call_span, maybe_with_capacity_or_reserve.span], + vec![call_span, maybe_init_or_reserve.span], "calling `set_len()` immediately after reserving a buffer creates uninitialized values", |diag| { diag.help("initialize the buffer or wrap the content in `MaybeUninit`"); @@ -101,15 +110,15 @@ impl<'tcx> LocalOrExpr<'tcx> { } } -/// Returns the target vec of `Vec::with_capacity()` or `Vec::reserve()` -fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stmt<'_>) -> Option> { +/// Finds the target location where the result of `Vec` initialization is stored +/// or `self` expression for `Vec::reserve()`. +fn extract_init_or_reserve_target<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) -> Option> { match stmt.kind { StmtKind::Local(local) => { - // let mut x = Vec::with_capacity() if_chain! { if let Some(init_expr) = local.init; if let PatKind::Binding(_, hir_id, _, None) = local.pat.kind; - if is_with_capacity(cx, init_expr); + if get_vec_init_kind(cx, init_expr).is_some(); then { Some(LocalOrExpr::Local(hir_id)) } else { @@ -117,40 +126,20 @@ fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stm } } }, - StmtKind::Expr(expr) | StmtKind::Semi(expr) => { - match expr.kind { - ExprKind::Assign(lhs, rhs, _span) if is_with_capacity(cx, rhs) => { - // self.vec = Vec::with_capacity() - Some(LocalOrExpr::Expr(lhs)) - }, - ExprKind::MethodCall(path, _, [self_expr, _], _) => { - // self.vec.reserve() - if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), sym::vec_type) - && path.ident.name.as_str() == "reserve" - { - Some(LocalOrExpr::Expr(self_expr)) - } else { - None - } - }, - _ => None, - } + StmtKind::Expr(expr) | StmtKind::Semi(expr) => match expr.kind { + ExprKind::Assign(lhs, rhs, _span) if get_vec_init_kind(cx, rhs).is_some() => Some(LocalOrExpr::Expr(lhs)), + ExprKind::MethodCall(path, _, [self_expr, _], _) if is_reserve(cx, path, self_expr) => { + Some(LocalOrExpr::Expr(self_expr)) + }, + _ => None, }, StmtKind::Item(_) => None, } } -fn is_with_capacity(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> bool { - if_chain! { - if let ExprKind::Call(path_expr, _) = &expr.kind; - if let ExprKind::Path(qpath) = &path_expr.kind; - if let Res::Def(_, def_id) = cx.qpath_res(qpath, path_expr.hir_id); - then { - match_def_path(cx, def_id, &paths::VEC_WITH_CAPACITY) - } else { - false - } - } +fn is_reserve(cx: &LateContext<'_>, path: &PathSegment<'_>, self_expr: &Expr<'_>) -> bool { + is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), sym::Vec) + && path.ident.name.as_str() == "reserve" } /// Returns self if the expression is `Vec::set_len()` @@ -169,14 +158,13 @@ fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(& } }); match expr.kind { - ExprKind::MethodCall(_, _, [vec_expr, _], _) => { - cx.typeck_results().type_dependent_def_id(expr.hir_id).and_then(|id| { - if match_def_path(cx, id, &paths::VEC_SET_LEN) { - Some((vec_expr, expr.span)) - } else { - None - } - }) + ExprKind::MethodCall(path, _, [self_expr, _], _) => { + let self_type = cx.typeck_results().expr_ty(self_expr).peel_refs(); + if is_type_diagnostic_item(cx, self_type, sym::Vec) && path.ident.name.as_str() == "set_len" { + Some((self_expr, expr.span)) + } else { + None + } }, _ => None, } diff --git a/clippy_lints/src/vec_init_then_push.rs b/clippy_lints/src/vec_init_then_push.rs index d8e241d72af4..478314c08368 100644 --- a/clippy_lints/src/vec_init_then_push.rs +++ b/clippy_lints/src/vec_init_then_push.rs @@ -1,16 +1,13 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{match_def_path, path_to_local, path_to_local_id, paths}; +use clippy_utils::{get_vec_init_kind, path_to_local, path_to_local_id, VecInitKind}; use if_chain::if_chain; -use rustc_ast::ast::LitKind; use rustc_errors::Applicability; -use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, QPath, Stmt, StmtKind}; +use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span}; -use std::convert::TryInto; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -41,11 +38,6 @@ pub struct VecInitThenPush { searcher: Option, } -#[derive(Clone, Copy)] -enum VecInitKind { - New, - WithCapacity(u64), -} struct VecPushSearcher { local_id: HirId, init: VecInitKind, @@ -58,7 +50,8 @@ impl VecPushSearcher { fn display_err(&self, cx: &LateContext<'_>) { match self.init { _ if self.found == 0 => return, - VecInitKind::WithCapacity(x) if x > self.found => return, + VecInitKind::WithLiteralCapacity(x) if x > self.found => return, + VecInitKind::WithExprCapacity(_) => return, _ => (), }; @@ -152,37 +145,3 @@ impl LateLintPass<'_> for VecInitThenPush { } } } - -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::WithCapacity(num.try_into().ok()?)) - } else { - None - } - } - }); - } - } - 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::New); - } - _ => (), - } - } - None -} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 09eee78f0d1f..b450059e18a9 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}; +use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type, is_type_diagnostic_item}; pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -1789,6 +1789,53 @@ 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 63ec10a31786..a39c0fc0b10c 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -183,7 +183,6 @@ 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_WITH_CAPACITY: [&str; 4] = ["alloc", "vec", "Vec", "with_capacity"]; 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"]; From b1aa3064b6d86c5ae90a67e420d88d826bafe8be Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Tue, 5 Oct 2021 11:43:44 -0400 Subject: [PATCH 07/11] 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 833c95f49403..5d6409874d8f 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 478314c08368..b92b6ca4f438 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 ba4d50bf7446..d60ddbd3c562 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 b450059e18a9..09eee78f0d1f 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 a39c0fc0b10c..e43c57560214 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 e60b73a1e308..c8f9067e549f 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); } } From de0d2b15008f0333e432aab9a7b7045ab0157897 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Thu, 7 Oct 2021 11:18:01 -0400 Subject: [PATCH 08/11] Add testcases --- clippy_utils/src/higher.rs | 20 ++++++++--------- tests/ui/uninit_vec.rs | 17 ++++++++++++++ tests/ui/uninit_vec.stderr | 45 +++++++++++++++++++++++++++++++++----- 3 files changed, 66 insertions(+), 16 deletions(-) diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index d60ddbd3c562..30f6831dcf40 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -658,18 +658,18 @@ pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) - { if name.ident.name == sym::new { return Some(VecInitKind::New); + } else if name.ident.name.as_str() == "default" { + return Some(VecInitKind::Default); } 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)) - } + let arg = args.get(0)?; + if_chain! { + if let ExprKind::Lit(lit) = &arg.kind; + if let LitKind::Int(num, _) = lit.node; + then { + return Some(VecInitKind::WithLiteralCapacity(num.try_into().ok()?)) } - }); + } + return Some(VecInitKind::WithExprCapacity(arg.hir_id)); } } ExprKind::Path(QPath::Resolved(_, path)) diff --git a/tests/ui/uninit_vec.rs b/tests/ui/uninit_vec.rs index c8f9067e549f..dc150cf28f2c 100644 --- a/tests/ui/uninit_vec.rs +++ b/tests/ui/uninit_vec.rs @@ -20,6 +20,23 @@ fn main() { vec.set_len(200); } + // new() -> set_len() should be detected + let mut vec: Vec = Vec::new(); + unsafe { + vec.set_len(200); + } + + // default() -> set_len() should be detected + let mut vec: Vec = Default::default(); + unsafe { + vec.set_len(200); + } + + let mut vec: Vec = Vec::default(); + unsafe { + vec.set_len(200); + } + // test when both calls are enclosed in the same unsafe block unsafe { let mut vec: Vec = Vec::with_capacity(1000); diff --git a/tests/ui/uninit_vec.stderr b/tests/ui/uninit_vec.stderr index 31f8ae40350c..81b91cef92ab 100644 --- a/tests/ui/uninit_vec.stderr +++ b/tests/ui/uninit_vec.stderr @@ -22,7 +22,40 @@ LL | vec.set_len(200); = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:32:5 + --> $DIR/uninit_vec.rs:24:5 + | +LL | let mut vec: Vec = Vec::new(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:30:5 + | +LL | let mut vec: Vec = Default::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:35:5 + | +LL | let mut vec: Vec = Vec::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { +LL | vec.set_len(200); + | ^^^^^^^^^^^^^^^^ + | + = help: initialize the buffer or wrap the content in `MaybeUninit` + +error: calling `set_len()` immediately after reserving a buffer creates uninitialized values + --> $DIR/uninit_vec.rs:49:5 | LL | let mut vec: Vec = Vec::with_capacity(1000); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -33,7 +66,7 @@ LL | vec.set_len(200); = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:41:5 + --> $DIR/uninit_vec.rs:58:5 | LL | my_vec.vec.reserve(1000); | ^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -44,7 +77,7 @@ LL | my_vec.vec.set_len(200); = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:46:5 + --> $DIR/uninit_vec.rs:63:5 | LL | my_vec.vec = Vec::with_capacity(1000); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -55,7 +88,7 @@ LL | my_vec.vec.set_len(200); = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:25:9 + --> $DIR/uninit_vec.rs:42:9 | LL | let mut vec: Vec = Vec::with_capacity(1000); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -65,7 +98,7 @@ LL | vec.set_len(200); = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values - --> $DIR/uninit_vec.rs:28:9 + --> $DIR/uninit_vec.rs:45:9 | LL | vec.reserve(1000); | ^^^^^^^^^^^^^^^^^^ @@ -74,5 +107,5 @@ LL | vec.set_len(200); | = help: initialize the buffer or wrap the content in `MaybeUninit` -error: aborting due to 7 previous errors +error: aborting due to 10 previous errors From 2181387c3ab4d7dc4d6e2c46c8158201a1b1045e Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Thu, 7 Oct 2021 11:53:08 -0400 Subject: [PATCH 09/11] Improved error message for set_len() on empty Vec --- clippy_lints/src/uninit_vec.rs | 101 +++++++++++++++++++++++---------- tests/ui/uninit_vec.stderr | 12 +--- 2 files changed, 73 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 5d6409874d8f..15c79da6fa99 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -1,5 +1,5 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::higher::get_vec_init_kind; +use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; +use clippy_utils::higher::{get_vec_init_kind, VecInitKind}; 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}; @@ -83,69 +83,108 @@ fn handle_uninit_vec_pair( if_chain! { if let Some(vec) = extract_init_or_reserve_target(cx, maybe_init_or_reserve); if let Some((set_len_self, call_span)) = extract_set_len_self(cx, maybe_set_len); - if vec.eq_expr(cx, set_len_self); + if vec.location.eq_expr(cx, set_len_self); if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind(); 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)] - span_lint_and_then( - cx, - UNINIT_VEC, - vec![call_span, maybe_init_or_reserve.span], - "calling `set_len()` immediately after reserving a buffer creates uninitialized values", - |diag| { - diag.help("initialize the buffer or wrap the content in `MaybeUninit`"); - }, - ); + if vec.has_capacity() { + // with_capacity / reserve -> set_len + + // Check T of Vec + if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)) { + // FIXME: #7698, false positive of the internal lints + #[allow(clippy::collapsible_span_lint_calls)] + span_lint_and_then( + cx, + UNINIT_VEC, + vec![call_span, maybe_init_or_reserve.span], + "calling `set_len()` immediately after reserving a buffer creates uninitialized values", + |diag| { + diag.help("initialize the buffer or wrap the content in `MaybeUninit`"); + }, + ); + } + } else { + // new / default -> set_len + span_lint( + cx, + UNINIT_VEC, + vec![call_span, maybe_init_or_reserve.span], + "calling `set_len()` on empty `Vec` creates out-of-bound values", + ) + } } } } -#[derive(Clone, Copy, Debug)] -enum LocalOrExpr<'tcx> { +/// The target `Vec` that is initialized or reserved +#[derive(Clone, Copy)] +struct TargetVec<'tcx> { + location: VecLocation<'tcx>, + /// `None` if `reserve()` + init_kind: Option, +} + +impl TargetVec<'_> { + pub fn has_capacity(self) -> bool { + !matches!(self.init_kind, Some(VecInitKind::New | VecInitKind::Default)) + } +} + +#[derive(Clone, Copy)] +enum VecLocation<'tcx> { Local(HirId), Expr(&'tcx Expr<'tcx>), } -impl<'tcx> LocalOrExpr<'tcx> { - fn eq_expr(self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { +impl<'tcx> VecLocation<'tcx> { + pub fn eq_expr(self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { match self { - LocalOrExpr::Local(hir_id) => path_to_local_id(expr, hir_id), - LocalOrExpr::Expr(self_expr) => SpanlessEq::new(cx).eq_expr(self_expr, expr), + VecLocation::Local(hir_id) => path_to_local_id(expr, hir_id), + VecLocation::Expr(self_expr) => SpanlessEq::new(cx).eq_expr(self_expr, expr), } } } /// Finds the target location where the result of `Vec` initialization is stored /// or `self` expression for `Vec::reserve()`. -fn extract_init_or_reserve_target<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) -> Option> { +fn extract_init_or_reserve_target<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) -> Option> { match stmt.kind { StmtKind::Local(local) => { if_chain! { if let Some(init_expr) = local.init; if let PatKind::Binding(_, hir_id, _, None) = local.pat.kind; - if get_vec_init_kind(cx, init_expr).is_some(); + if let Some(init_kind) = get_vec_init_kind(cx, init_expr); then { - Some(LocalOrExpr::Local(hir_id)) - } else { - None + return Some(TargetVec { + location: VecLocation::Local(hir_id), + init_kind: Some(init_kind), + }) } } }, StmtKind::Expr(expr) | StmtKind::Semi(expr) => match expr.kind { - ExprKind::Assign(lhs, rhs, _span) if get_vec_init_kind(cx, rhs).is_some() => Some(LocalOrExpr::Expr(lhs)), + ExprKind::Assign(lhs, rhs, _span) => { + if let Some(init_kind) = get_vec_init_kind(cx, rhs) { + return Some(TargetVec { + location: VecLocation::Expr(lhs), + init_kind: Some(init_kind), + }); + } + }, ExprKind::MethodCall(path, _, [self_expr, _], _) if is_reserve(cx, path, self_expr) => { - Some(LocalOrExpr::Expr(self_expr)) + return Some(TargetVec { + location: VecLocation::Expr(self_expr), + init_kind: None, + }); }, - _ => None, + _ => (), }, - StmtKind::Item(_) => None, + StmtKind::Item(_) => (), } + None } fn is_reserve(cx: &LateContext<'_>, path: &PathSegment<'_>, self_expr: &Expr<'_>) -> bool { diff --git a/tests/ui/uninit_vec.stderr b/tests/ui/uninit_vec.stderr index 81b91cef92ab..520bfb26b62e 100644 --- a/tests/ui/uninit_vec.stderr +++ b/tests/ui/uninit_vec.stderr @@ -21,7 +21,7 @@ LL | vec.set_len(200); | = help: initialize the buffer or wrap the content in `MaybeUninit` -error: calling `set_len()` immediately after reserving a buffer creates uninitialized values +error: calling `set_len()` on empty `Vec` creates out-of-bound values --> $DIR/uninit_vec.rs:24:5 | LL | let mut vec: Vec = Vec::new(); @@ -29,10 +29,8 @@ LL | let mut vec: Vec = Vec::new(); LL | unsafe { LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ - | - = help: initialize the buffer or wrap the content in `MaybeUninit` -error: calling `set_len()` immediately after reserving a buffer creates uninitialized values +error: calling `set_len()` on empty `Vec` creates out-of-bound values --> $DIR/uninit_vec.rs:30:5 | LL | let mut vec: Vec = Default::default(); @@ -40,10 +38,8 @@ LL | let mut vec: Vec = Default::default(); LL | unsafe { LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ - | - = help: initialize the buffer or wrap the content in `MaybeUninit` -error: calling `set_len()` immediately after reserving a buffer creates uninitialized values +error: calling `set_len()` on empty `Vec` creates out-of-bound values --> $DIR/uninit_vec.rs:35:5 | LL | let mut vec: Vec = Vec::default(); @@ -51,8 +47,6 @@ LL | let mut vec: Vec = Vec::default(); LL | unsafe { LL | vec.set_len(200); | ^^^^^^^^^^^^^^^^ - | - = help: initialize the buffer or wrap the content in `MaybeUninit` error: calling `set_len()` immediately after reserving a buffer creates uninitialized values --> $DIR/uninit_vec.rs:49:5 From 03fed75c89880e5ed919eec1ba93dbe769d463a2 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Sat, 9 Oct 2021 05:59:53 -0400 Subject: [PATCH 10/11] Address internal lints --- clippy_lints/src/uninit_vec.rs | 2 +- clippy_utils/src/higher.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 15c79da6fa99..2573209d1b62 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -113,7 +113,7 @@ fn handle_uninit_vec_pair( UNINIT_VEC, vec![call_span, maybe_init_or_reserve.span], "calling `set_len()` on empty `Vec` creates out-of-bound values", - ) + ); } } } diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 30f6831dcf40..1ff282de9509 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -11,7 +11,7 @@ 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}; +use rustc_span::{sym, symbol, ExpnKind, Span, Symbol}; /// The essential nodes of a desugared for loop as well as the entire span: /// `for pat in arg { body }` becomes `(pat, arg, body)`. Return `(pat, arg, body, span)`. @@ -658,7 +658,7 @@ pub fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) - { if name.ident.name == sym::new { return Some(VecInitKind::New); - } else if name.ident.name.as_str() == "default" { + } else if name.ident.name == symbol::kw::Default { return Some(VecInitKind::Default); } else if name.ident.name.as_str() == "with_capacity" { let arg = args.get(0)?; From 4ed3a4fe2f681660cac9a4fad6385c6d92a89de1 Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Tue, 12 Oct 2021 16:01:58 -0400 Subject: [PATCH 11/11] Update lint description for new() and default() --- clippy_lints/src/uninit_vec.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/uninit_vec.rs b/clippy_lints/src/uninit_vec.rs index 2573209d1b62..f3e8b6881058 100644 --- a/clippy_lints/src/uninit_vec.rs +++ b/clippy_lints/src/uninit_vec.rs @@ -13,14 +13,16 @@ use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does /// Checks for `set_len()` call that creates `Vec` with uninitialized elements. - /// This is commonly caused by calling `set_len()` right after after calling - /// `with_capacity()` or `reserve()`. + /// This is commonly caused by calling `set_len()` right after allocating or + /// reserving a buffer with `new()`, `default()`, `with_capacity()`, or `reserve()`. /// /// ### Why is this bad? /// It creates a `Vec` with uninitialized data, which leads to - /// undefined behavior with most safe operations. + /// undefined behavior with most safe operations. Notably, uninitialized + /// `Vec` must not be used with generic `Read`. /// - /// Notably, uninitialized `Vec` must not be used with generic `Read`. + /// Moreover, calling `set_len()` on a `Vec` created with `new()` or `default()` + /// creates out-of-bound values that lead to heap memory corruption when used. /// /// ### Known Problems /// This lint only checks directly adjacent statements.