From ab070508be3fbf02619f5f109ece829243a751e8 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 07:43:13 -0800 Subject: [PATCH] Lint for Vec> - Closes #3530 --- clippy_lints/src/lib.rs | 2 ++ clippy_lints/src/types.rs | 65 +++++++++++++++++++++++++++++++++-- tests/ui/vec_box_sized.rs | 17 +++++++++ tests/ui/vec_box_sized.stderr | 11 ++++++ 4 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 tests/ui/vec_box_sized.rs create mode 100644 tests/ui/vec_box_sized.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a862d774174b..9e3f0a6505dc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -766,6 +766,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::UNIT_ARG, types::UNIT_CMP, types::UNNECESSARY_CAST, + types::VEC_BOX_SIZED, unicode::ZERO_WIDTH_SPACE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, unused_io_amount::UNUSED_IO_AMOUNT, @@ -931,6 +932,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::TYPE_COMPLEXITY, types::UNIT_ARG, types::UNNECESSARY_CAST, + types::VEC_BOX_SIZED, unused_label::UNUSED_LABEL, zero_div_zero::ZERO_DIVIDED_BY_ZERO, ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 820f2fdf32db..b85f21ce9703 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -24,7 +24,7 @@ use crate::rustc_target::spec::abi::Abi; use crate::rustc_typeck::hir_ty_to_ty; use crate::syntax::ast::{FloatTy, IntTy, UintTy}; use crate::syntax::errors::DiagnosticBuilder; -use crate::syntax::source_map::Span; +use crate::syntax::source_map::{DUMMY_SP, Span}; use crate::utils::paths; use crate::utils::{ clip, comparisons, differing_macro_contexts, higher, in_constant, in_macro, int_bits, last_path_segment, @@ -68,6 +68,33 @@ declare_clippy_lint! { "usage of `Box>`, vector elements are already on the heap" } +/// **What it does:** Checks for use of `Vec>` where T: Sized anywhere in the code. +/// +/// **Why is this bad?** `Vec` already keeps its contents in a separate area on +/// the heap. So if you `Box` its contents, you just add another level of indirection. +/// +/// **Known problems:** Vec> makes sense if T is a large type (see #3530, 1st comment). +/// +/// **Example:** +/// ```rust +/// struct X { +/// values: Vec>, +/// } +/// ``` +/// +/// Better: +/// +/// ```rust +/// struct X { +/// values: Vec, +/// } +/// ``` +declare_clippy_lint! { + pub VEC_BOX_SIZED, + complexity, + "usage of `Vec>` where T: Sized, vector elements are already on the heap" +} + /// **What it does:** Checks for use of `Option>` in function signatures and type /// definitions /// @@ -148,7 +175,7 @@ declare_clippy_lint! { impl LintPass for TypePass { fn get_lints(&self) -> LintArray { - lint_array!(BOX_VEC, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) + lint_array!(BOX_VEC, VEC_BOX_SIZED, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) } } @@ -238,6 +265,40 @@ fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { ); return; // don't recurse into the type } + } else if match_def_path(cx.tcx, def_id, &paths::VEC) { + if_chain! { + // Get the _ part of Vec<_> + if let Some(ref last) = last_path_segment(qpath).args; + if let Some(ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + GenericArg::Lifetime(_) => None, + }); + // ty is now _ at this point + if let TyKind::Path(ref ty_qpath) = ty.node; + let def = cx.tables.qpath_def(ty_qpath, ty.hir_id); + if let Some(def_id) = opt_def_id(def); + if Some(def_id) == cx.tcx.lang_items().owned_box(); + // At this point, we know ty is Box, now get T + if let Some(ref last) = last_path_segment(ty_qpath).args; + if let Some(ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + GenericArg::Lifetime(_) => None, + }); + if let TyKind::Path(ref ty_qpath) = ty.node; + let def = cx.tables.qpath_def(ty_qpath, ty.hir_id); + if let Some(def_id) = opt_def_id(def); + let boxed_type = cx.tcx.type_of(def_id); + if boxed_type.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env); + then { + span_help_and_lint( + cx, + VEC_BOX_SIZED, + ast_ty.span, + "you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec`", + "`Vec` is already on the heap, `Vec>` makes an extra allocation.", + ) + } + } } else if match_def_path(cx.tcx, def_id, &paths::OPTION) { if match_type_parameter(cx, qpath, &paths::OPTION) { span_lint( diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs new file mode 100644 index 000000000000..d740f95edfe2 --- /dev/null +++ b/tests/ui/vec_box_sized.rs @@ -0,0 +1,17 @@ +struct SizedStruct { + _a: i32, +} + +struct UnsizedStruct { + _a: [i32], +} + +struct StructWithVecBox { + sized_type: Vec>, +} + +struct StructWithVecBoxButItsUnsized { + unsized_type: Vec>, +} + +fn main() {} diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr new file mode 100644 index 000000000000..80f54b51a40b --- /dev/null +++ b/tests/ui/vec_box_sized.stderr @@ -0,0 +1,11 @@ +error: you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec` + --> $DIR/vec_box_sized.rs:10:14 + | +10 | sized_type: Vec>, + | ^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::vec-box-sized` implied by `-D warnings` + = help: `Vec` is already on the heap, `Vec>` makes an extra allocation. + +error: aborting due to previous error +