Lint for Vec<Box<T: Sized>> - Closes #3530

4 files changed, 93 insertions, 2 deletions

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index a862d774174..9e3f0a6505d 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 820f2fdf32d..b85f21ce970 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<Vec<T>>`, vector elements are already on the heap"
 }
 
+/// **What it does:** Checks for use of `Vec<Box<T>>` 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<Box<T: Sized>> makes sense if T is a large type (see #3530, 1st comment).
+///
+/// **Example:**
+/// ```rust
+/// struct X {
+///     values: Vec<Box<i32>>,
+/// }
+/// ```
+///
+/// Better:
+///
+/// ```rust
+/// struct X {
+///     values: Vec<i32>,
+/// }
+/// ```
+declare_clippy_lint! {
+    pub VEC_BOX_SIZED,
+    complexity,
+    "usage of `Vec<Box<T>>` where T: Sized, vector elements are already on the heap"
+}
+
 /// **What it does:** Checks for use of `Option<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<T>, 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<Box<T>>`, but T is Sized. Consider using just `Vec<T>`",
+                                "`Vec<T>` is already on the heap, `Vec<Box<T>>` 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 00000000000..d740f95edfe
--- /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<Box<SizedStruct>>,
+}
+
+struct StructWithVecBoxButItsUnsized {
+	unsized_type: Vec<Box<UnsizedStruct>>,
+}
+
+fn main() {}
diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr
new file mode 100644
index 00000000000..80f54b51a40
--- /dev/null
+++ b/tests/ui/vec_box_sized.stderr
@@ -0,0 +1,11 @@
+error: you seem to be trying to use `Vec<Box<T>>`, but T is Sized. Consider using just `Vec<T>`
+  --> $DIR/vec_box_sized.rs:10:14
+   |
+10 |     sized_type: Vec<Box<SizedStruct>>,
+   |                 ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::vec-box-sized` implied by `-D warnings`
+   = help: `Vec<T>` is already on the heap, `Vec<Box<T>>` makes an extra allocation.
+
+error: aborting due to previous error
+