Auto merge of #3545 - Kampfkarren:vec_boxed_sized, r=flip1995

Adds lint for Vec<Box<T: Sized>>

This adds, and subsequently closes #3530. This is the first time I've ever worked with anything remotely close to internal Rust code, so I'm very much unsure about the if_chain! to figure this out!

I can't get rustfmt working on WSL with nightly 2018-12-07:

`error: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is unavailable for download`

CHANGELOG.md

@@ -899,6 +899,7 @@ All notable changes to this project will be documented in this file.
 [`useless_let_if_seq`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_let_if_seq
 [`useless_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_transmute
 [`useless_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec
+[`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
 [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask
 [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition
 [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

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,
         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,
         unused_label::UNUSED_LABEL,
         zero_div_zero::ZERO_DIVIDED_BY_ZERO,
     ]);

clippy_lints/src/types.rs

@@ -68,6 +68,34 @@ 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,
+    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 +176,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, OPTION_OPTION, LINKEDLIST, BORROWED_BOX)
     }
 }
 
@@ -238,6 +266,43 @@ 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(ty.span), cx.param_env);
+                        then {
+                            span_lint_and_sugg(
+                                cx,
+                                VEC_BOX,
+                                ast_ty.span,
+                                "`Vec<T>` is already on the heap, the boxing is unnecessary.",
+                                "try",
+                                format!("Vec<{}>", boxed_type),
+                                Applicability::MaybeIncorrect,
+                            );
+                            return; // don't recurse into the type
+                        }
+                    }
                 } else if match_def_path(cx.tcx, def_id, &paths::OPTION) {
                     if match_type_parameter(cx, qpath, &paths::OPTION) {
                         span_lint(

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() {}

tests/ui/vec_box_sized.stderr

@@ -0,0 +1,10 @@
+error: `Vec<T>` is already on the heap, the boxing is unnecessary.
+  --> $DIR/vec_box_sized.rs:10:14
+   |
+10 |     sized_type: Vec<Box<SizedStruct>>,
+   |                 ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<SizedStruct>`
+   |
+   = note: `-D clippy::vec-box` implied by `-D warnings`
+
+error: aborting due to previous error
+