Auto merge of #12601 - Alexendoo:box-default-style, r=llogiq

bors · bors · commit cb87a574fd5d · 2024-04-01T07:57:26.000Z
Move `box_default` to style, do not suggest turbofishes `Box::default()` [had its `#[rustc_box]` attribute removed](https://github.com/rust-lang/rust/pull/108476/files#r1120930164) in 1.69 so is no longer a perf related lint The lint is moved to style but no longer produces suggestions containing turbofishes, as they're often longer/more annoying to type changelog: [`box_default`]: Move to style r? `@llogiq`
diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs
@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::macros::macro_backtrace;
-use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::expr_sig;
 use clippy_utils::{is_default_equivalent, path_def_id};
 use rustc_errors::Applicability;
@@ -9,20 +8,16 @@ use rustc_hir::intravisit::{walk_ty, Visitor};
 use rustc_hir::{Block, Expr, ExprKind, Local, Node, QPath, Ty, TyKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::print::with_forced_trimmed_paths;
-use rustc_middle::ty::IsSuggestable;
 use rustc_session::declare_lint_pass;
 use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// checks for `Box::new(T::default())`, which is better written as
-    /// `Box::<T>::default()`.
+    /// checks for `Box::new(Default::default())`, which can be written as
+    /// `Box::default()`.
     ///
     /// ### Why is this bad?
-    /// 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).
+    /// `Box::default()` is equivalent and more concise.
     ///
     /// ### Example
     /// ```no_run
@@ -34,7 +29,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "1.66.0"]
     pub BOX_DEFAULT,
-    perf,
+    style,
     "Using Box::new(T::default()) instead of Box::default()"
 }
 
@@ -53,38 +48,22 @@ impl LateLintPass<'_> for BoxDefault {
             && path_def_id(cx, ty).map_or(false, |id| Some(id) == cx.tcx.lang_items().owned_box())
             // And the single argument to the call is another function call
             // This is the `T::default()` of `Box::new(T::default())`
-            && let ExprKind::Call(arg_path, inner_call_args) = arg.kind
+            && let ExprKind::Call(arg_path, _) = arg.kind
             // And we are not in a foreign crate's macro
             && !in_external_macro(cx.sess(), expr.span)
             // And the argument expression has the same context as the outer call expression
             // or that we are inside a `vec!` macro expansion
             && (expr.span.eq_ctxt(arg.span) || is_local_vec_expn(cx, arg, expr))
-            // And the argument is equivalent to `Default::default()`
-            && is_default_equivalent(cx, arg)
+            // And the argument is `Default::default()` or the type is specified
+            && (is_plain_default(cx, arg_path) || (given_type(cx, expr) && is_default_equivalent(cx, arg)))
         {
             span_lint_and_sugg(
                 cx,
                 BOX_DEFAULT,
                 expr.span,
                 "`Box::new(_)` of default value",
                 "try",
-                if is_plain_default(cx, arg_path) || given_type(cx, expr) {
-                    "Box::default()".into()
-                } else if let Some(arg_ty) = cx.typeck_results().expr_ty(arg).make_suggestable(cx.tcx, true) {
-                    // Check if we can copy from the source expression in the replacement.
-                    // We need the call to have no argument (see `explicit_default_type`).
-                    if inner_call_args.is_empty()
-                        && let Some(ty) = explicit_default_type(arg_path)
-                        && let Some(s) = snippet_opt(cx, ty.span)
-                    {
-                        format!("Box::<{s}>::default()")
-                    } else {
-                        // Otherwise, use the inferred type's formatting.
-                        with_forced_trimmed_paths!(format!("Box::<{arg_ty}>::default()"))
-                    }
-                } else {
-                    return;
-                },
+                "Box::default()".into(),
                 Applicability::MachineApplicable,
             );
         }
@@ -103,20 +82,6 @@ fn is_plain_default(cx: &LateContext<'_>, arg_path: &Expr<'_>) -> bool {
     }
 }
 
-// Checks whether the call is of the form `A::B::f()`. Returns `A::B` if it is.
-//
-// In the event we have this kind of construct, it's easy to use `A::B` as a replacement in the
-// quickfix. `f` must however have no parameter. Should `f` have some, then some of the type of
-// `A::B` may be inferred from the arguments. This would be the case for `Vec::from([0; false])`,
-// where the argument to `from` allows inferring this is a `Vec<bool>`
-fn explicit_default_type<'a>(arg_path: &'a Expr<'_>) -> Option<&'a Ty<'a>> {
-    if let ExprKind::Path(QPath::TypeRelative(ty, _)) = &arg_path.kind {
-        Some(ty)
-    } else {
-        None
-    }
-}
-
 fn is_local_vec_expn(cx: &LateContext<'_>, expr: &Expr<'_>, ref_expr: &Expr<'_>) -> bool {
     macro_backtrace(expr.span).next().map_or(false, |call| {
         cx.tcx.is_diagnostic_item(sym::vec_macro, call.def_id) && call.span.eq_ctxt(ref_expr.span)
diff --git a/tests/ui/box_default.fixed b/tests/ui/box_default.fixed
@@ -1,5 +1,5 @@
 #![warn(clippy::box_default)]
-#![allow(clippy::default_constructed_unit_structs)]
+#![allow(clippy::boxed_local, clippy::default_constructed_unit_structs)]
 
 #[derive(Default)]
 struct ImplementsDefault;
@@ -12,26 +12,50 @@ impl OwnDefault {
     }
 }
 
-macro_rules! outer {
-    ($e: expr) => {
-        $e
+macro_rules! default {
+    () => {
+        Default::default()
+    };
+}
+
+macro_rules! string_new {
+    () => {
+        String::new()
+    };
+}
+
+macro_rules! box_new {
+    ($e:expr) => {
+        Box::new($e)
     };
 }
 
 fn main() {
-    let _string: Box<String> = Box::default();
-    let _byte = Box::<u8>::default();
-    let _vec = Box::<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::<String>::default());
-    let _string_default = outer!(Box::<String>::default());
-    let _vec2: Box<Vec<ImplementsDefault>> = Box::default();
-    let _vec3: Box<Vec<bool>> = Box::default();
-    let _vec4: Box<_> = Box::<Vec<bool>>::default();
-    let _more = ret_ty_fn();
+    let string1: Box<String> = Box::default();
+    let string2: Box<String> = Box::default();
+    let impl1: Box<ImplementsDefault> = Box::default();
+    let vec: Box<Vec<u8>> = Box::default();
+    let byte: Box<u8> = Box::default();
+    let vec2: Box<Vec<ImplementsDefault>> = Box::default();
+    let vec3: Box<Vec<bool>> = Box::default();
+
+    let plain_default = Box::default();
+    let _: Box<String> = plain_default;
+
+    let _: Box<String> = Box::new(default!());
+    let _: Box<String> = Box::new(string_new!());
+    let _: Box<String> = box_new!(Default::default());
+    let _: Box<String> = box_new!(String::new());
+    let _: Box<String> = box_new!(default!());
+    let _: Box<String> = box_new!(string_new!());
+
+    let own: Box<OwnDefault> = Box::new(OwnDefault::default()); // should not lint
+
+    // Do not suggest where a turbofish would be required
+    let impl2 = Box::new(ImplementsDefault::default());
+    let impl3 = Box::new(<ImplementsDefault as Default>::default());
+    let vec4: Box<_> = Box::new(Vec::from([false; 0]));
+    let more = ret_ty_fn();
     call_ty_fn(Box::default());
     issue_10381();
 
@@ -44,10 +68,9 @@ fn main() {
 }
 
 fn ret_ty_fn() -> Box<bool> {
-    Box::<bool>::default()
+    Box::new(bool::default()) // Could lint, currently doesn't
 }
 
-#[allow(clippy::boxed_local)]
 fn call_ty_fn(_b: Box<u8>) {
     issue_9621_dyn_trait();
 }
@@ -61,7 +84,7 @@ impl Read for ImplementsDefault {
 }
 
 fn issue_9621_dyn_trait() {
-    let _: Box<dyn Read> = Box::<ImplementsDefault>::default();
+    let _: Box<dyn Read> = Box::new(ImplementsDefault::default());
     issue_10089();
 }
 
@@ -70,7 +93,7 @@ fn issue_10089() {
         #[derive(Default)]
         struct WeirdPathed;
 
-        let _ = Box::<WeirdPathed>::default();
+        let _ = Box::new(WeirdPathed::default());
     };
 }
 
@@ -82,7 +105,7 @@ fn issue_10381() {
 
     fn maybe_get_bar(i: u32) -> Option<Box<dyn Bar>> {
         if i % 2 == 0 {
-            Some(Box::<Foo>::default())
+            Some(Box::new(Foo::default()))
         } else {
             None
         }
@@ -91,20 +114,6 @@ fn issue_10381() {
     assert!(maybe_get_bar(2).is_some());
 }
 
-#[allow(unused)]
-fn issue_11868() {
-    fn foo(_: &mut Vec<usize>) {}
-
-    macro_rules! bar {
-        ($baz:expr) => {
-            Box::leak(Box::new($baz))
-        };
-    }
-
-    foo(bar!(vec![]));
-    foo(bar!(vec![1]));
-}
-
 // Issue #11927: The quickfix for the `Box::new` suggests replacing with `Box::<Inner>::default()`,
 // removing the `outer::` segment.
 fn issue_11927() {
@@ -116,7 +125,7 @@ fn issue_11927() {
     }
 
     fn foo() {
-        let _b = Box::<outer::Inner>::default();
-        let _b = Box::<std::collections::HashSet::<i32>>::default();
+        let _b = Box::new(outer::Inner::default());
+        let _b = Box::new(std::collections::HashSet::<i32>::new());
     }
 }
diff --git a/tests/ui/box_default.rs b/tests/ui/box_default.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::box_default)]
-#![allow(clippy::default_constructed_unit_structs)]
+#![allow(clippy::boxed_local, clippy::default_constructed_unit_structs)]
 
 #[derive(Default)]
 struct ImplementsDefault;
@@ -12,26 +12,50 @@ impl OwnDefault {
     }
 }
 
-macro_rules! outer {
-    ($e: expr) => {
-        $e
+macro_rules! default {
+    () => {
+        Default::default()
+    };
+}
+
+macro_rules! string_new {
+    () => {
+        String::new()
+    };
+}
+
+macro_rules! box_new {
+    ($e:expr) => {
+        Box::new($e)
     };
 }
 
 fn main() {
-    let _string: Box<String> = Box::new(Default::default());
-    let _byte = Box::new(u8::default());
-    let _vec = Box::new(Vec::<u8>::new());
-    let _impl = Box::new(ImplementsDefault::default());
-    let _impl2 = Box::new(<ImplementsDefault as Default>::default());
-    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()));
-    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();
+    let string1: Box<String> = Box::new(Default::default());
+    let string2: Box<String> = Box::new(String::new());
+    let impl1: Box<ImplementsDefault> = Box::new(Default::default());
+    let vec: Box<Vec<u8>> = Box::new(Vec::new());
+    let byte: Box<u8> = Box::new(u8::default());
+    let vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
+    let vec3: Box<Vec<bool>> = Box::new(Vec::from([]));
+
+    let plain_default = Box::new(Default::default());
+    let _: Box<String> = plain_default;
+
+    let _: Box<String> = Box::new(default!());
+    let _: Box<String> = Box::new(string_new!());
+    let _: Box<String> = box_new!(Default::default());
+    let _: Box<String> = box_new!(String::new());
+    let _: Box<String> = box_new!(default!());
+    let _: Box<String> = box_new!(string_new!());
+
+    let own: Box<OwnDefault> = Box::new(OwnDefault::default()); // should not lint
+
+    // Do not suggest where a turbofish would be required
+    let impl2 = Box::new(ImplementsDefault::default());
+    let impl3 = Box::new(<ImplementsDefault as Default>::default());
+    let vec4: Box<_> = Box::new(Vec::from([false; 0]));
+    let more = ret_ty_fn();
     call_ty_fn(Box::new(u8::default()));
     issue_10381();
 
@@ -44,10 +68,9 @@ fn main() {
 }
 
 fn ret_ty_fn() -> Box<bool> {
-    Box::new(bool::default())
+    Box::new(bool::default()) // Could lint, currently doesn't
 }
 
-#[allow(clippy::boxed_local)]
 fn call_ty_fn(_b: Box<u8>) {
     issue_9621_dyn_trait();
 }
@@ -91,20 +114,6 @@ fn issue_10381() {
     assert!(maybe_get_bar(2).is_some());
 }
 
-#[allow(unused)]
-fn issue_11868() {
-    fn foo(_: &mut Vec<usize>) {}
-
-    macro_rules! bar {
-        ($baz:expr) => {
-            Box::leak(Box::new($baz))
-        };
-    }
-
-    foo(bar!(vec![]));
-    foo(bar!(vec![1]));
-}
-
 // Issue #11927: The quickfix for the `Box::new` suggests replacing with `Box::<Inner>::default()`,
 // removing the `outer::` segment.
 fn issue_11927() {
diff --git a/tests/ui/box_default.stderr b/tests/ui/box_default.stderr
