Auto merge of #7693 - F3real:vec2, r=Manishearth

Expand box_vec lint to box_collection

fixed #7451

changelog: Expand `box_vec` into [`box_collection`], and have it error on all sorts of boxed collections

Author: bors

CHANGELOG.md

@@ -1050,7 +1050,7 @@ Released 2020-11-19
   [#5913](https://github.com/rust-lang/rust-clippy/pull/5913)
 * Add example of false positive to [`ptr_arg`] docs.
   [#5885](https://github.com/rust-lang/rust-clippy/pull/5885)
-* [`box_vec`], [`vec_box`] and [`borrowed_box`]: add link to the documentation of `Box`
+* [`box_vec`](https://rust-lang.github.io/rust-clippy/master/index.html#box_collection), [`vec_box`] and [`borrowed_box`]: add link to the documentation of `Box`
   [#6023](https://github.com/rust-lang/rust-clippy/pull/6023)
 
 ## Rust 1.47
@@ -2570,7 +2570,7 @@ Released 2018-09-13
 [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
 [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const
 [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box
-[`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec
+[`box_collection`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_collection
 [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local
 [`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
 [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow

clippy_lints/src/lib.rs

@@ -956,7 +956,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         transmuting_null::TRANSMUTING_NULL,
         try_err::TRY_ERR,
         types::BORROWED_BOX,
-        types::BOX_VEC,
+        types::BOX_COLLECTION,
         types::LINKEDLIST,
         types::OPTION_OPTION,
         types::RC_BUFFER,
@@ -1454,7 +1454,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(transmuting_null::TRANSMUTING_NULL),
         LintId::of(try_err::TRY_ERR),
         LintId::of(types::BORROWED_BOX),
-        LintId::of(types::BOX_VEC),
+        LintId::of(types::BOX_COLLECTION),
         LintId::of(types::REDUNDANT_ALLOCATION),
         LintId::of(types::TYPE_COMPLEXITY),
         LintId::of(types::VEC_BOX),
@@ -1792,7 +1792,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(redundant_clone::REDUNDANT_CLONE),
         LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
         LintId::of(stable_sort_primitive::STABLE_SORT_PRIMITIVE),
-        LintId::of(types::BOX_VEC),
+        LintId::of(types::BOX_COLLECTION),
         LintId::of(types::REDUNDANT_ALLOCATION),
         LintId::of(vec::USELESS_VEC),
         LintId::of(vec_init_then_push::VEC_INIT_THEN_PUSH),
@@ -2193,6 +2193,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
     ls.register_renamed("clippy::cyclomatic_complexity", "clippy::cognitive_complexity");
     ls.register_renamed("clippy::const_static_lifetime", "clippy::redundant_static_lifetimes");
     ls.register_renamed("clippy::option_and_then_some", "clippy::bind_instead_of_map");
+    ls.register_renamed("clippy::box_vec", "clippy::box_collection");
     ls.register_renamed("clippy::block_in_if_condition_expr", "clippy::blocks_in_if_conditions");
     ls.register_renamed("clippy::block_in_if_condition_stmt", "clippy::blocks_in_if_conditions");
     ls.register_renamed("clippy::option_map_unwrap_or", "clippy::map_unwrap_or");

clippy_lints/src/types/box_collection.rs

@@ -0,0 +1,50 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::is_ty_param_diagnostic_item;
+use rustc_hir::{self as hir, def_id::DefId, QPath};
+use rustc_lint::LateContext;
+use rustc_span::symbol::sym;
+
+use super::BOX_COLLECTION;
+
+pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_>, def_id: DefId) -> bool {
+    if_chain! {
+        if Some(def_id) == cx.tcx.lang_items().owned_box();
+        if let Some(item_type) = get_std_collection(cx, qpath);
+        then {
+            let generic = if item_type == "String" {
+                ""
+            } else {
+                "<..>"
+            };
+            span_lint_and_help(
+                cx,
+                BOX_COLLECTION,
+                hir_ty.span,
+                &format!(
+                    "you seem to be trying to use `Box<{outer}{generic}>`. Consider using just `{outer}{generic}`",
+                    outer=item_type,
+                    generic = generic),
+                None,
+                &format!(
+                    "`{outer}{generic}` is already on the heap, `Box<{outer}{generic}>` makes an extra allocation",
+                    outer=item_type,
+                    generic = generic)
+            );
+            true
+        } else {
+            false
+        }
+    }
+}
+
+fn get_std_collection(cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option<String> {
+    if is_ty_param_diagnostic_item(cx, qpath, sym::vec_type).is_some() {
+        Some(String::from("Vec"))
+    } else if is_ty_param_diagnostic_item(cx, qpath, sym::string_type).is_some() {
+        Some(String::from("String"))
+    } else if is_ty_param_diagnostic_item(cx, qpath, sym::hashmap_type).is_some() {
+        Some(String::from("HashMap"))
+    } else {
+        None
+    }
+}

clippy_lints/src/types/box_vec.rs

@@ -1,5 +1,5 @@
 mod borrowed_box;
-mod box_vec;
+mod box_collection;
 mod linked_list;
 mod option_option;
 mod rc_buffer;
@@ -21,12 +21,12 @@ use rustc_span::source_map::Span;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for use of `Box<Vec<_>>` anywhere in the code.
+    /// Checks for use of `Box<T>` where T is a collection such as Vec anywhere in the code.
     /// Check the [Box documentation](https://doc.rust-lang.org/std/boxed/index.html) for more information.
     ///
     /// ### Why is this bad?
-    /// `Vec` already keeps its contents in a separate area on
-    /// the heap. So if you `Box` it, you just add another level of indirection
+    /// Collections already keeps their contents in a separate area on
+    /// the heap. So if you `Box` them, you just add another level of indirection
     /// without any benefit whatsoever.
     ///
     /// ### Example
@@ -43,7 +43,7 @@ declare_clippy_lint! {
     ///     values: Vec<Foo>,
     /// }
     /// ```
-    pub BOX_VEC,
+    pub BOX_COLLECTION,
     perf,
     "usage of `Box<Vec<T>>`, vector elements are already on the heap"
 }
@@ -298,7 +298,7 @@ pub struct Types {
     avoid_breaking_exported_api: bool,
 }
 
-impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
+impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
 
 impl<'tcx> LateLintPass<'tcx> for Types {
     fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) {
@@ -447,7 +447,7 @@ impl Types {
                         // in `clippy_lints::utils::conf.rs`
 
                         let mut triggered = false;
-                        triggered |= box_vec::check(cx, hir_ty, qpath, def_id);
+                        triggered |= box_collection::check(cx, hir_ty, qpath, def_id);
                         triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id);
                         triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id);
                         triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold);

clippy_lints/src/types/mod.rs

@@ -136,7 +136,7 @@ macro_rules! define_Conf {
 }
 
 define_Conf! {
-    /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_VEC, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX.
+    /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX.
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),

clippy_lints/src/utils/conf.rs

@@ -6,6 +6,8 @@
     unused
 )]
 
+use std::collections::HashMap;
+
 macro_rules! boxit {
     ($init:expr, $x:ty) => {
         let _: Box<$x> = Box::new($init);
@@ -15,20 +17,26 @@ macro_rules! boxit {
 fn test_macro() {
     boxit!(Vec::new(), Vec<u8>);
 }
+
 fn test(foo: Box<Vec<bool>>) {}
 
 fn test2(foo: Box<dyn Fn(Vec<u32>)>) {
     // pass if #31 is fixed
     foo(vec![1, 2, 3])
 }
 
+fn test3(foo: Box<String>) {}
+
+fn test4(foo: Box<HashMap<String, String>>) {}
+
 fn test_local_not_linted() {
     let _: Box<Vec<bool>>;
 }
 
 // All of these test should be allowed because they are part of the
 // public api and `avoid_breaking_exported_api` is `false` by default.
 pub fn pub_test(foo: Box<Vec<bool>>) {}
+
 pub fn pub_test_ret() -> Box<Vec<bool>> {
     Box::new(Vec::new())
 }

tests/ui/box_vec.rs renamed to tests/ui/box_collection.rs

@@ -0,0 +1,27 @@
+error: you seem to be trying to use `Box<Vec<..>>`. Consider using just `Vec<..>`
+  --> $DIR/box_collection.rs:21:14
+   |
+LL | fn test(foo: Box<Vec<bool>>) {}
+   |              ^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::box-collection` implied by `-D warnings`
+   = help: `Vec<..>` is already on the heap, `Box<Vec<..>>` makes an extra allocation
+
+error: you seem to be trying to use `Box<String>`. Consider using just `String`
+  --> $DIR/box_collection.rs:28:15
+   |
+LL | fn test3(foo: Box<String>) {}
+   |               ^^^^^^^^^^^
+   |
+   = help: `String` is already on the heap, `Box<String>` makes an extra allocation
+
+error: you seem to be trying to use `Box<HashMap<..>>`. Consider using just `HashMap<..>`
+  --> $DIR/box_collection.rs:30:15
+   |
+LL | fn test4(foo: Box<HashMap<String, String>>) {}
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: `HashMap<..>` is already on the heap, `Box<HashMap<..>>` makes an extra allocation
+
+error: aborting due to 3 previous errors
+