box_vec: dont use Box<&T>

Author: jpospychala

CHANGELOG.md

@@ -1166,6 +1166,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_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_borrow
 [`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec
 [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local
 [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 362 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

@@ -788,6 +788,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &try_err::TRY_ERR,
         &types::ABSURD_EXTREME_COMPARISONS,
         &types::BORROWED_BOX,
+        &types::BOX_BORROW,
         &types::BOX_VEC,
         &types::CAST_LOSSLESS,
         &types::CAST_POSSIBLE_TRUNCATION,
@@ -1358,6 +1359,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&try_err::TRY_ERR),
         LintId::of(&types::ABSURD_EXTREME_COMPARISONS),
         LintId::of(&types::BORROWED_BOX),
+        LintId::of(&types::BOX_BORROW),
         LintId::of(&types::BOX_VEC),
         LintId::of(&types::CAST_PTR_ALIGNMENT),
         LintId::of(&types::CAST_REF_TO_MUT),
@@ -1650,6 +1652,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(&trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF),
+        LintId::of(&types::BOX_BORROW),
         LintId::of(&types::BOX_VEC),
         LintId::of(&vec::USELESS_VEC),
     ]);

clippy_lints/src/types.rs

@@ -171,11 +171,38 @@ declare_clippy_lint! {
     "a borrow of a boxed type"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for use of `Box<&T>` anywhere in the code.
+    ///
+    /// **Why is this bad?** Any `Box<&T>` can also be a `&T`, which is more
+    /// general.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust,ignore
+    /// struct X {
+    ///     values: Box<&T>,
+    /// }
+    /// ```
+    ///
+    /// Better:
+    ///
+    /// ```rust,ignore
+    /// struct X {
+    ///     values: &T,
+    /// }
+    /// ```
+    pub BOX_BORROW,
+    perf,
+    "a box of borrowed type"
+}
+
 pub struct Types {
     vec_box_size_threshold: u64,
 }
 
-impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX]);
+impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, BOX_BORROW]);
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Types {
     fn check_fn(
@@ -257,6 +284,7 @@ impl Types {
     /// The parameter `is_local` distinguishes the context of the type; types from
     /// local bindings should only be checked for the `BORROWED_BOX` lint.
     #[allow(clippy::too_many_lines)]
+    #[allow(clippy::cognitive_complexity)]
     fn check_ty(&mut self, cx: &LateContext<'_, '_>, hir_ty: &hir::Ty<'_>, is_local: bool) {
         if hir_ty.span.from_expansion() {
             return;
@@ -267,6 +295,27 @@ impl Types {
                 let res = qpath_res(cx, qpath, hir_id);
                 if let Some(def_id) = res.opt_def_id() {
                     if Some(def_id) == cx.tcx.lang_items().owned_box() {
+                        if_chain! {
+                            let last = last_path_segment(qpath);
+                            if let Some(ref params) = last.args;
+                            if !params.parenthesized;
+                            if let Some(ty) = params.args.iter().find_map(|arg| match arg {
+                                GenericArg::Type(ty) => Some(ty),
+                                _ => None,
+                            });
+                            if let TyKind::Rptr(..) = ty.kind;
+                            let ty_ty = hir_ty_to_ty(cx.tcx, ty);
+                            then {
+                                span_lint_and_help(
+                                    cx,
+                                    BOX_BORROW,
+                                    hir_ty.span,
+                                    "usage of `Box<&T>`",
+                                    format!("try `{}`", ty_ty).as_str(),
+                                );
+                                return; // don't recurse into the type
+                            }
+                        }
                         if match_type_parameter(cx, qpath, &paths::VEC) {
                             span_lint_and_help(
                                 cx,

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 361] = [
+pub const ALL_LINTS: [Lint; 362] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -98,6 +98,13 @@ pub const ALL_LINTS: [Lint; 361] = [
         deprecation: None,
         module: "types",
     },
+    Lint {
+        name: "box_borrow",
+        group: "perf",
+        desc: "a box of borrowed type",
+        deprecation: None,
+        module: "types",
+    },
     Lint {
         name: "box_vec",
         group: "perf",

tests/ui/box_vec.rs

@@ -8,6 +8,17 @@ macro_rules! boxit {
     };
 }
 
+struct MyStruct {}
+
+struct SubT<T> {
+    foo: T
+}
+
+enum MyEnum {
+    One,
+    Two
+}
+
 fn test_macro() {
     boxit!(Vec::new(), Vec<u8>);
 }
@@ -24,6 +35,20 @@ pub fn test_local_not_linted() {
     let _: Box<Vec<bool>>;
 }
 
+pub fn test3<T>(foo: Box<&T>) {}
+
+pub fn test4(foo: Box<&usize>) {}
+
+pub fn test5(foo: Box<&MyStruct>) {}
+
+pub fn test6(foo: Box<&MyEnum>) {}
+
+pub fn test7(foo: Box<&&MyEnum>) {}
+
+pub fn test8(foo: Box<Box<&usize>>) {}
+
+pub fn test9(foo: Box<SubT<&usize>>) {}
+
 fn main() {
     test(Box::new(Vec::new()));
     test2(Box::new(|v| println!("{:?}", v)));

tests/ui/box_vec.stderr

@@ -1,11 +1,97 @@
+error[E0446]: private type `My` in public interface
+  --> $DIR/box_vec.rs:42:1
+   |
+LL | struct My {}
+   | - `My` declared as private
+...
+LL | pub fn test5(foo: Box<&My>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type
+
+error[E0446]: private type `OneOrTwo` in public interface
+  --> $DIR/box_vec.rs:44:1
+   |
+LL | enum OneOrTwo {
+   | - `OneOrTwo` declared as private
+...
+LL | pub fn test6(foo: Box<&OneOrTwo>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type
+
+error[E0446]: private type `OneOrTwo` in public interface
+  --> $DIR/box_vec.rs:46:1
+   |
+LL | enum OneOrTwo {
+   | - `OneOrTwo` declared as private
+...
+LL | pub fn test7(foo: Box<&&OneOrTwo>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type
+
+error[E0446]: private type `SubT<&usize>` in public interface
+  --> $DIR/box_vec.rs:50:1
+   |
+LL | struct SubT<T> {
+   | - `SubT<&usize>` declared as private
+...
+LL | pub fn test9(foo: Box<SubT<&usize>>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type
+
 error: you seem to be trying to use `Box<Vec<T>>`. Consider using just `Vec<T>`
-  --> $DIR/box_vec.rs:14:18
+  --> $DIR/box_vec.rs:25:18
    |
 LL | pub fn test(foo: Box<Vec<bool>>) {
    |                  ^^^^^^^^^^^^^^
    |
    = note: `-D clippy::box-vec` implied by `-D warnings`
    = help: `Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation.
 
-error: aborting due to previous error
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:38:22
+   |
+LL | pub fn test3<T>(foo: Box<&T>) {}
+   |                      ^^^^^^^
+   |
+   = note: `-D clippy::box-borrow` implied by `-D warnings`
+   = help: try `&T`
+
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:40:19
+   |
+LL | pub fn test4(foo: Box<&usize>) {}
+   |                   ^^^^^^^^^^^
+   |
+   = help: try `&usize`
+
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:42:19
+   |
+LL | pub fn test5(foo: Box<&My>) {}
+   |                   ^^^^^^^^
+   |
+   = help: try `&My`
+
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:44:19
+   |
+LL | pub fn test6(foo: Box<&OneOrTwo>) {}
+   |                   ^^^^^^^^^^^^^^
+   |
+   = help: try `&OneOrTwo`
+
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:46:19
+   |
+LL | pub fn test7(foo: Box<&&OneOrTwo>) {}
+   |                   ^^^^^^^^^^^^^^^
+   |
+   = help: try `&&OneOrTwo`
+
+error: usage of `Box<&T>`
+  --> $DIR/box_vec.rs:48:23
+   |
+LL | pub fn test8(foo: Box<Box<&usize>>) {}
+   |                       ^^^^^^^^^^^
+   |
+   = help: try `&usize`
+
+error: aborting due to 11 previous errors
 
+For more information about this error, try `rustc --explain E0446`.