Auto merge of #9585 - rust-lang:extend-box-default, r=Alexendoo

extend `box-default` lint, add suggestion

This extends the recently added `box-default` lint to also cover `Box::new(vec![])`, `Box::new(String::from(""))` and `Box::new(Vec::from([]))`. Also the lint now suggests a suitable replacement. I did not find a simple way to check whether the type is fully determined by the outside, so I at least checked for some variations to remove the turbofish in those cases.

---

changelog: none

Author: bors

clippy_lints/src/box_default.rs

@@ -1,5 +1,12 @@
-use clippy_utils::{diagnostics::span_lint_and_help, is_default_equivalent, path_def_id};
-use rustc_hir::{Expr, ExprKind, QPath};
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg, get_parent_node, is_default_equivalent, macros::macro_backtrace, match_path,
+    path_def_id, paths, ty::expr_sig,
+};
+use rustc_errors::Applicability;
+use rustc_hir::{
+    intravisit::{walk_ty, Visitor},
+    Block, Expr, ExprKind, Local, Node, QPath, TyKind,
+};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -15,12 +22,6 @@ declare_clippy_lint! {
     /// Second, `Box::default()` can be faster
     /// [in certain cases](https://nnethercote.github.io/perf-book/standard-library-types.html#box).
     ///
-    /// ### Known problems
-    /// The lint may miss some cases (e.g. Box::new(String::from(""))).
-    /// On the other hand, it will trigger on cases where the `default`
-    /// code comes from a macro that does something different based on
-    /// e.g. target operating system.
-    ///
     /// ### Example
     /// ```rust
     /// let x: Box<String> = Box::new(Default::default());
@@ -41,21 +42,88 @@ impl LateLintPass<'_> for BoxDefault {
     fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
         if let ExprKind::Call(box_new, [arg]) = expr.kind
             && let ExprKind::Path(QPath::TypeRelative(ty, seg)) = box_new.kind
-            && let ExprKind::Call(..) = arg.kind
+            && let ExprKind::Call(arg_path, ..) = arg.kind
             && !in_external_macro(cx.sess(), expr.span)
-            && expr.span.eq_ctxt(arg.span)
+            && (expr.span.eq_ctxt(arg.span) || is_vec_expn(cx, arg))
             && seg.ident.name == sym::new
             && path_def_id(cx, ty) == cx.tcx.lang_items().owned_box()
             && is_default_equivalent(cx, arg)
         {
-            span_lint_and_help(
+            let arg_ty = cx.typeck_results().expr_ty(arg);
+            span_lint_and_sugg(
                 cx,
                 BOX_DEFAULT,
                 expr.span,
                 "`Box::new(_)` of default value",
-                None,
-                "use `Box::default()` instead",
+                "try",
+                if is_plain_default(arg_path) || given_type(cx, expr) {
+                    "Box::default()".into()
+                } else {
+                    format!("Box::<{arg_ty}>::default()")
+                },
+                Applicability::MachineApplicable
             );
         }
     }
 }
+
+fn is_plain_default(arg_path: &Expr<'_>) -> bool {
+    // we need to match the actual path so we don't match e.g. "u8::default"
+    if let ExprKind::Path(QPath::Resolved(None, path)) = &arg_path.kind {
+        // avoid generic parameters
+        match_path(path, &paths::DEFAULT_TRAIT_METHOD) && path.segments.iter().all(|seg| seg.args.is_none())
+    } else {
+        false
+    }
+}
+
+fn is_vec_expn(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    macro_backtrace(expr.span)
+        .next()
+        .map_or(false, |call| cx.tcx.is_diagnostic_item(sym::vec_macro, call.def_id))
+}
+
+#[derive(Default)]
+struct InferVisitor(bool);
+
+impl<'tcx> Visitor<'tcx> for InferVisitor {
+    fn visit_ty(&mut self, t: &rustc_hir::Ty<'_>) {
+        self.0 |= matches!(t.kind, TyKind::Infer);
+        if !self.0 {
+            walk_ty(self, t);
+        }
+    }
+}
+
+fn given_type(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    match get_parent_node(cx.tcx, expr.hir_id) {
+        Some(Node::Local(Local { ty: Some(ty), .. })) => {
+            let mut v = InferVisitor::default();
+            v.visit_ty(ty);
+            !v.0
+        },
+        Some(
+            Node::Expr(Expr {
+                kind: ExprKind::Call(path, args),
+                ..
+            }) | Node::Block(Block {
+                expr:
+                    Some(Expr {
+                        kind: ExprKind::Call(path, args),
+                        ..
+                    }),
+                ..
+            }),
+        ) => {
+            if let Some(index) = args.iter().position(|arg| arg.hir_id == expr.hir_id) &&
+                let Some(sig) = expr_sig(cx, path) &&
+                let Some(input) = sig.input(index)
+            {
+                input.no_bound_vars().is_some()
+            } else {
+                false
+            }
+        },
+        _ => false,
+    }
+}

clippy_utils/src/lib.rs

@@ -815,13 +815,37 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
                 false
             }
         },
-        ExprKind::Call(repl_func, _) => is_default_equivalent_call(cx, repl_func),
+        ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func),
+        ExprKind::Call(from_func, [ref arg]) => is_default_equivalent_from(cx, from_func, arg),
         ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone),
         ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),
         _ => false,
     }
 }
 
+fn is_default_equivalent_from(cx: &LateContext<'_>, from_func: &Expr<'_>, arg: &Expr<'_>) -> bool {
+    if let ExprKind::Path(QPath::TypeRelative(ty, seg)) = from_func.kind &&
+        seg.ident.name == sym::from
+    {
+        match arg.kind {
+            ExprKind::Lit(hir::Lit {
+                node: LitKind::Str(ref sym, _),
+                ..
+            }) => return sym.is_empty() && is_path_diagnostic_item(cx, ty, sym::String),
+            ExprKind::Array([]) => return is_path_diagnostic_item(cx, ty, sym::Vec),
+            ExprKind::Repeat(_, ArrayLen::Body(len)) => {
+                if let ExprKind::Lit(ref const_lit) = cx.tcx.hir().body(len.body).value.kind &&
+                    let LitKind::Int(v, _) = const_lit.node
+                {
+                        return v == 0 && is_path_diagnostic_item(cx, ty, sym::Vec);
+                }
+            }
+            _ => (),
+        }
+    }
+    false
+}
+
 /// Checks if the top level expression can be moved into a closure as is.
 /// Currently checks for:
 /// * Break/Continue outside the given loop HIR ids.

src/docs/box_default.txt

@@ -7,12 +7,6 @@ First, it's more complex, involving two calls instead of one.
 Second, `Box::default()` can be faster
 [in certain cases](https://nnethercote.github.io/perf-book/standard-library-types.html#box).
 
-### Known problems
-The lint may miss some cases (e.g. Box::new(String::from(""))).
-On the other hand, it will trigger on cases where the `default`
-code comes from a macro that does something different based on
-e.g. target operating system.
-
 ### Example
 ```
 let x: Box<String> = Box::new(Default::default());

tests/ui/box_default.fixed

@@ -0,0 +1,43 @@
+// run-rustfix
+#![warn(clippy::box_default)]
+
+#[derive(Default)]
+struct ImplementsDefault;
+
+struct OwnDefault;
+
+impl OwnDefault {
+    fn default() -> Self {
+        Self
+    }
+}
+
+macro_rules! outer {
+    ($e: expr) => {
+        $e
+    };
+}
+
+fn main() {
+    let _string: Box<String> = Box::default();
+    let _byte = Box::<u8>::default();
+    let _vec = Box::<std::vec::Vec<u8>>::default();
+    let _impl = Box::<ImplementsDefault>::default();
+    let _impl2 = Box::<ImplementsDefault>::default();
+    let _impl3: Box<ImplementsDefault> = Box::default();
+    let _own = Box::new(OwnDefault::default()); // should not lint
+    let _in_macro = outer!(Box::<std::string::String>::default());
+    let _string_default = outer!(Box::<std::string::String>::default());
+    let _vec2: Box<Vec<ImplementsDefault>> = Box::default();
+    let _vec3: Box<Vec<bool>> = Box::default();
+    let _vec4: Box<_> = Box::<std::vec::Vec<bool>>::default();
+    let _more = ret_ty_fn();
+    call_ty_fn(Box::default());
+}
+
+fn ret_ty_fn() -> Box<bool> {
+    Box::<bool>::default()
+}
+
+#[allow(clippy::boxed_local)]
+fn call_ty_fn(_b: Box<u8>) {}

tests/ui/box_default.rs

@@ -1,3 +1,4 @@
+// run-rustfix
 #![warn(clippy::box_default)]
 
 #[derive(Default)]
@@ -26,6 +27,17 @@ fn main() {
     let _impl3: Box<ImplementsDefault> = Box::new(Default::default());
     let _own = Box::new(OwnDefault::default()); // should not lint
     let _in_macro = outer!(Box::new(String::new()));
-    // false negative: default is from different expansion
+    let _string_default = outer!(Box::new(String::from("")));
     let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
+    let _vec3: Box<Vec<bool>> = Box::new(Vec::from([]));
+    let _vec4: Box<_> = Box::new(Vec::from([false; 0]));
+    let _more = ret_ty_fn();
+    call_ty_fn(Box::new(u8::default()));
 }
+
+fn ret_ty_fn() -> Box<bool> {
+    Box::new(bool::default())
+}
+
+#[allow(clippy::boxed_local)]
+fn call_ty_fn(_b: Box<u8>) {}

tests/ui/box_default.stderr

@@ -1,59 +1,82 @@
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:21:32
+  --> $DIR/box_default.rs:22:32
    |
 LL |     let _string: Box<String> = Box::new(Default::default());
-   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
    |
-   = help: use `Box::default()` instead
    = note: `-D clippy::box-default` implied by `-D warnings`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:22:17
+  --> $DIR/box_default.rs:23:17
    |
 LL |     let _byte = Box::new(u8::default());
-   |                 ^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: use `Box::default()` instead
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<u8>::default()`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:23:16
+  --> $DIR/box_default.rs:24:16
    |
 LL |     let _vec = Box::new(Vec::<u8>::new());
-   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: use `Box::default()` instead
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<u8>>::default()`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:24:17
+  --> $DIR/box_default.rs:25:17
    |
 LL |     let _impl = Box::new(ImplementsDefault::default());
-   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: use `Box::default()` instead
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:25:18
+  --> $DIR/box_default.rs:26:18
    |
 LL |     let _impl2 = Box::new(<ImplementsDefault as Default>::default());
-   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: use `Box::default()` instead
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:26:42
+  --> $DIR/box_default.rs:27:42
    |
 LL |     let _impl3: Box<ImplementsDefault> = Box::new(Default::default());
-   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = help: use `Box::default()` instead
+   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
 
 error: `Box::new(_)` of default value
-  --> $DIR/box_default.rs:28:28
+  --> $DIR/box_default.rs:29:28
    |
 LL |     let _in_macro = outer!(Box::new(String::new()));
-   |                            ^^^^^^^^^^^^^^^^^^^^^^^
+   |                            ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:30:34
+   |
+LL |     let _string_default = outer!(Box::new(String::from("")));
+   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:31:46
+   |
+LL |     let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
+   |                                              ^^^^^^^^^^^^^^^^ help: try: `Box::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:32:33
+   |
+LL |     let _vec3: Box<Vec<bool>> = Box::new(Vec::from([]));
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:33:25
+   |
+LL |     let _vec4: Box<_> = Box::new(Vec::from([false; 0]));
+   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<bool>>::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:35:16
+   |
+LL |     call_ty_fn(Box::new(u8::default()));
+   |                ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
+
+error: `Box::new(_)` of default value
+  --> $DIR/box_default.rs:39:5
    |
-   = help: use `Box::default()` instead
+LL |     Box::new(bool::default())
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<bool>::default()`
 
-error: aborting due to 7 previous errors
+error: aborting due to 13 previous errors