Merge pull request #1501 from scott-linder/types-borrow-box

Types borrow box

Author: oli-obk

CHANGELOG.md

@@ -364,6 +364,7 @@ All notable changes to this project will be documented in this file.
 [`block_in_if_condition_expr`]: https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr
 [`block_in_if_condition_stmt`]: https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_stmt
 [`bool_comparison`]: https://github.com/Manishearth/rust-clippy/wiki#bool_comparison
+[`borrowed_box`]: https://github.com/Manishearth/rust-clippy/wiki#borrowed_box
 [`box_vec`]: https://github.com/Manishearth/rust-clippy/wiki#box_vec
 [`boxed_local`]: https://github.com/Manishearth/rust-clippy/wiki#boxed_local
 [`builtin_type_shadow`]: https://github.com/Manishearth/rust-clippy/wiki#builtin_type_shadow

README.md

@@ -194,6 +194,7 @@ name
 [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr)               | warn    | braces that can be eliminated in conditions, e.g. `if { true } ...`
 [block_in_if_condition_stmt](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_stmt)               | warn    | complex blocks in conditions, e.g. `if { let x = true; x } ...`
 [bool_comparison](https://github.com/Manishearth/rust-clippy/wiki#bool_comparison)                                     | warn    | comparing a variable to a boolean, e.g. `if x == true`
+[borrowed_box](https://github.com/Manishearth/rust-clippy/wiki#borrowed_box)                                           | warn    | a borrow of a boxed type
 [box_vec](https://github.com/Manishearth/rust-clippy/wiki#box_vec)                                                     | warn    | usage of `Box<Vec<T>>`, vector elements are already on the heap
 [boxed_local](https://github.com/Manishearth/rust-clippy/wiki#boxed_local)                                             | warn    | using `Box<T>` where unnecessary
 [builtin_type_shadow](https://github.com/Manishearth/rust-clippy/wiki#builtin_type_shadow)                             | warn    | shadowing a builtin type

clippy_lints/src/lib.rs

@@ -506,6 +506,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         transmute::USELESS_TRANSMUTE,
         transmute::WRONG_TRANSMUTE,
         types::ABSURD_EXTREME_COMPARISONS,
+        types::BORROWED_BOX,
         types::BOX_VEC,
         types::CHAR_LIT_AS_U8,
         types::LET_UNIT_VALUE,

clippy_lints/src/types.rs

@@ -8,7 +8,7 @@ use std::cmp::Ordering;
 use syntax::ast::{IntTy, UintTy, FloatTy};
 use syntax::attr::IntType;
 use syntax::codemap::Span;
-use utils::{comparisons, higher, in_external_macro, in_macro, match_def_path, snippet, span_help_and_lint, span_lint,
+use utils::{comparisons, higher, in_external_macro, in_macro, match_def_path, snippet, span_help_and_lint, span_lint, span_lint_and_then,
             opt_def_id, last_path_segment, type_size};
 use utils::paths;
 
@@ -65,9 +65,25 @@ declare_lint! {
      structure like a VecDeque"
 }
 
+/// **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
+/// fn foo(bar: &Box<T>) { ... }
+/// ```
+declare_lint! {
+    pub BORROWED_BOX,
+    Warn,
+    "a borrow of a boxed type"
+}
+
 impl LintPass for TypePass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(BOX_VEC, LINKEDLIST)
+        lint_array!(BOX_VEC, LINKEDLIST, BORROWED_BOX)
     }
 }
 
@@ -84,35 +100,46 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TypePass {
     }
 
     fn check_struct_field(&mut self, cx: &LateContext, field: &StructField) {
-        check_ty(cx, &field.ty);
+        check_ty(cx, &field.ty, false);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) {
         match item.node {
             TraitItemKind::Const(ref ty, _) |
-            TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty),
+            TraitItemKind::Type(_, Some(ref ty)) => check_ty(cx, ty, false),
             TraitItemKind::Method(ref sig, _) => check_fn_decl(cx, &sig.decl),
             _ => (),
         }
     }
+
+    fn check_local(&mut self, cx: &LateContext, local: &Local) {
+        if let Some(ref ty) = local.ty {
+            check_ty(cx, ty, true);
+        }
+    }
 }
 
 fn check_fn_decl(cx: &LateContext, decl: &FnDecl) {
     for input in &decl.inputs {
-        check_ty(cx, input);
+        check_ty(cx, input, false);
     }
 
     if let FunctionRetTy::Return(ref ty) = decl.output {
-        check_ty(cx, ty);
+        check_ty(cx, ty, false);
     }
 }
 
-fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) {
+/// Recursively check for `TypePass` lints in the given type. Stop at the first
+/// lint found.
+///
+/// The parameter `is_local` distinguishes the context of the type; types from
+/// local bindings should only be checked for the `BORROWED_BOX` lint.
+fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) {
     if in_macro(ast_ty.span) {
         return;
     }
     match ast_ty.node {
-        TyPath(ref qpath) => {
+        TyPath(ref qpath) if !is_local => {
             let def = cx.tables.qpath_def(qpath, ast_ty.id);
             if let Some(def_id) = opt_def_id(def) {
                 if Some(def_id) == cx.tcx.lang_items.owned_box() {
@@ -143,32 +170,70 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty) {
             }
             match *qpath {
                 QPath::Resolved(Some(ref ty), ref p) => {
-                    check_ty(cx, ty);
+                    check_ty(cx, ty, is_local);
                     for ty in p.segments.iter().flat_map(|seg| seg.parameters.types()) {
-                        check_ty(cx, ty);
+                        check_ty(cx, ty, is_local);
                     }
                 },
                 QPath::Resolved(None, ref p) => {
                     for ty in p.segments.iter().flat_map(|seg| seg.parameters.types()) {
-                        check_ty(cx, ty);
+                        check_ty(cx, ty, is_local);
                     }
                 },
                 QPath::TypeRelative(ref ty, ref seg) => {
-                    check_ty(cx, ty);
+                    check_ty(cx, ty, is_local);
                     for ty in seg.parameters.types() {
-                        check_ty(cx, ty);
+                        check_ty(cx, ty, is_local);
                     }
                 },
             }
         },
+        TyRptr(ref lt, MutTy { ref ty, ref mutbl }) => {
+            match ty.node {
+                TyPath(ref qpath) => {
+                    let def = cx.tables.qpath_def(qpath, ast_ty.id);
+                    if_let_chain! {[
+                        let Some(def_id) = opt_def_id(def),
+                        Some(def_id) == cx.tcx.lang_items.owned_box(),
+                        let QPath::Resolved(None, ref path) = *qpath,
+                        let [ref bx] = *path.segments,
+                        let PathParameters::AngleBracketedParameters(ref ab_data) = bx.parameters,
+                        let [ref inner] = *ab_data.types
+                    ], {
+                        let ltopt = if lt.is_elided() {
+                            "".to_owned()
+                        } else {
+                            format!("{} ", lt.name.as_str())
+                        };
+                        let mutopt = if *mutbl == Mutability::MutMutable {
+                            "mut "
+                        } else {
+                            ""
+                        };
+                        span_lint_and_then(cx,
+                            BORROWED_BOX,
+                            ast_ty.span,
+                            "you seem to be trying to use `&Box<T>`. Consider using just `&T`",
+                            |db| {
+                                db.span_suggestion(ast_ty.span,
+                                    "try",
+                                    format!("&{}{}{}", ltopt, mutopt, &snippet(cx, inner.span, "..")));
+                            }
+                        );
+                        return; // don't recurse into the type
+                    }};
+                    check_ty(cx, ty, is_local);
+                },
+                _ => check_ty(cx, ty, is_local),
+            }
+        },
         // recurse
         TySlice(ref ty) |
         TyArray(ref ty, _) |
-        TyPtr(MutTy { ref ty, .. }) |
-        TyRptr(_, MutTy { ref ty, .. }) => check_ty(cx, ty),
+        TyPtr(MutTy { ref ty, .. }) => check_ty(cx, ty, is_local),
         TyTup(ref tys) => {
             for ty in tys {
-                check_ty(cx, ty);
+                check_ty(cx, ty, is_local);
             }
         },
         _ => {},

clippy_tests/examples/borrow_box.rs

@@ -0,0 +1,34 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(borrowed_box)]
+#![allow(blacklisted_name)]
+#![allow(unused_variables)]
+#![allow(dead_code)]
+
+pub fn test1(foo: &mut Box<bool>) {
+    println!("{:?}", foo)
+}
+
+pub fn test2() {
+    let foo: &Box<bool>;
+}
+
+struct Test3<'a> {
+    foo: &'a Box<bool>
+}
+
+trait Test4 {
+    fn test4(a: &Box<bool>);
+}
+
+impl<'a> Test4 for Test3<'a> {
+    fn test4(a: &Box<bool>) {
+        unimplemented!();
+    }
+}
+
+fn main(){
+    test1(&mut Box::new(false));
+    test2();
+}

clippy_tests/examples/borrow_box.stderr

@@ -0,0 +1,35 @@
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+ --> borrow_box.rs:9:19
+  |
+9 | pub fn test1(foo: &mut Box<bool>) {
+  |                   ^^^^^^^^^^^^^^ help: try `&mut bool`
+  |
+note: lint level defined here
+ --> borrow_box.rs:4:9
+  |
+4 | #![deny(borrowed_box)]
+  |         ^^^^^^^^^^^^
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> borrow_box.rs:14:14
+   |
+14 |     let foo: &Box<bool>;
+   |              ^^^^^^^^^^ help: try `&bool`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> borrow_box.rs:18:10
+   |
+18 |     foo: &'a Box<bool>
+   |          ^^^^^^^^^^^^^ help: try `&'a bool`
+
+error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+  --> borrow_box.rs:22:17
+   |
+22 |     fn test4(a: &Box<bool>);
+   |                 ^^^^^^^^^^ help: try `&bool`
+
+error: aborting due to previous error(s)
+
+error: Could not compile `clippy_tests`.
+
+To learn more, run the command again with --verbose.

clippy_tests/examples/box_vec.rs

@@ -22,8 +22,13 @@ pub fn test2(foo: Box<Fn(Vec<u32>)>) { // pass if #31 is fixed
     foo(vec![1, 2, 3])
 }
 
+pub fn test_local_not_linted() {
+    let _: Box<Vec<bool>>;
+}
+
 fn main(){
     test(Box::new(Vec::new()));
     test2(Box::new(|v| println!("{:?}", v)));
     test_macro();
+    test_local_not_linted();
 }

clippy_tests/examples/dlist.rs

@@ -34,6 +34,11 @@ pub fn test_ret() -> Option<LinkedList<u8>> {
     unimplemented!();
 }
 
+pub fn test_local_not_linted() {
+    let _: LinkedList<u8>;
+}
+
 fn main(){
     test(LinkedList::new());
+    test_local_not_linted();
 }