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

add `box-default` lint

This adds a `box-default` lint to suggest using `Box::default()` instead of `Box::new(Default::default())`, which offers less moving parts and potentially better performance according to [the perf book](https://nnethercote.github.io/perf-book/standard-library-types.html#box).

---

changelog: add [`box_default`] lint

Author: bors

CHANGELOG.md

@@ -3759,6 +3759,7 @@ Released 2018-09-13
 [`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_collection`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_collection
+[`box_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_default
 [`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
 [`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code

clippy_lints/src/box_default.rs

@@ -0,0 +1,61 @@
+use clippy_utils::{diagnostics::span_lint_and_help, is_default_equivalent, path_def_id};
+use rustc_hir::{Expr, ExprKind, QPath};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// checks for `Box::new(T::default())`, which is better written as
+    /// `Box::<T>::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).
+    ///
+    /// ### 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());
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let x: Box<String> = Box::default();
+    /// ```
+    #[clippy::version = "1.65.0"]
+    pub BOX_DEFAULT,
+    perf,
+    "Using Box::new(T::default()) instead of Box::default()"
+}
+
+declare_lint_pass!(BoxDefault => [BOX_DEFAULT]);
+
+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
+            && !in_external_macro(cx.sess(), expr.span)
+            && expr.span.eq_ctxt(arg.span)
+            && 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(
+                cx,
+                BOX_DEFAULT,
+                expr.span,
+                "`Box::new(_)` of default value",
+                None,
+                "use `Box::default()` instead",
+            );
+        }
+    }
+}

clippy_lints/src/lib.register_all.rs

@@ -21,6 +21,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(booleans::NONMINIMAL_BOOL),
     LintId::of(booleans::OVERLY_COMPLEX_BOOL_EXPR),
     LintId::of(borrow_deref_ref::BORROW_DEREF_REF),
+    LintId::of(box_default::BOX_DEFAULT),
     LintId::of(casts::CAST_ABS_TO_UNSIGNED),
     LintId::of(casts::CAST_ENUM_CONSTRUCTOR),
     LintId::of(casts::CAST_ENUM_TRUNCATION),

clippy_lints/src/lib.register_lints.rs

@@ -60,6 +60,7 @@ store.register_lints(&[
     booleans::NONMINIMAL_BOOL,
     booleans::OVERLY_COMPLEX_BOOL_EXPR,
     borrow_deref_ref::BORROW_DEREF_REF,
+    box_default::BOX_DEFAULT,
     cargo::CARGO_COMMON_METADATA,
     cargo::MULTIPLE_CRATE_VERSIONS,
     cargo::NEGATIVE_FEATURE_NAMES,

clippy_lints/src/lib.register_perf.rs

@@ -3,6 +3,7 @@
 // Manual edits will be overwritten.
 
 store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
+    LintId::of(box_default::BOX_DEFAULT),
     LintId::of(entry::MAP_ENTRY),
     LintId::of(escape::BOXED_LOCAL),
     LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),

clippy_lints/src/lib.rs

@@ -181,6 +181,7 @@ mod bool_assert_comparison;
 mod bool_to_int_with_if;
 mod booleans;
 mod borrow_deref_ref;
+mod box_default;
 mod cargo;
 mod casts;
 mod checked_conversions;
@@ -533,8 +534,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         store.register_late_pass(|| Box::new(utils::internal_lints::CompilerLintFunctions::new()));
         store.register_late_pass(|| Box::new(utils::internal_lints::IfChainStyle));
         store.register_late_pass(|| Box::new(utils::internal_lints::InvalidPaths));
-        store.register_late_pass(|| Box::new(utils::internal_lints::InterningDefinedSymbol::default()));
-        store.register_late_pass(|| Box::new(utils::internal_lints::LintWithoutLintPass::default()));
+        store.register_late_pass(|| Box::<utils::internal_lints::InterningDefinedSymbol>::default());
+        store.register_late_pass(|| Box::<utils::internal_lints::LintWithoutLintPass>::default());
         store.register_late_pass(|| Box::new(utils::internal_lints::MatchTypeOnDiagItem));
         store.register_late_pass(|| Box::new(utils::internal_lints::OuterExpnDataPass));
         store.register_late_pass(|| Box::new(utils::internal_lints::MsrvAttrImpl));
@@ -627,10 +628,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             msrv,
         ))
     });
-    store.register_late_pass(|| Box::new(shadow::Shadow::default()));
+    store.register_late_pass(|| Box::<shadow::Shadow>::default());
     store.register_late_pass(|| Box::new(unit_types::UnitTypes));
     store.register_late_pass(|| Box::new(loops::Loops));
-    store.register_late_pass(|| Box::new(main_recursion::MainRecursion::default()));
+    store.register_late_pass(|| Box::<main_recursion::MainRecursion>::default());
     store.register_late_pass(|| Box::new(lifetimes::Lifetimes));
     store.register_late_pass(|| Box::new(entry::HashMapPass));
     store.register_late_pass(|| Box::new(minmax::MinMaxPass));
@@ -664,7 +665,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(format::UselessFormat));
     store.register_late_pass(|| Box::new(swap::Swap));
     store.register_late_pass(|| Box::new(overflow_check_conditional::OverflowCheckConditional));
-    store.register_late_pass(|| Box::new(new_without_default::NewWithoutDefault::default()));
+    store.register_late_pass(|| Box::<new_without_default::NewWithoutDefault>::default());
     let disallowed_names = conf.disallowed_names.iter().cloned().collect::<FxHashSet<_>>();
     store.register_late_pass(move || Box::new(disallowed_names::DisallowedNames::new(disallowed_names.clone())));
     let too_many_arguments_threshold = conf.too_many_arguments_threshold;
@@ -703,7 +704,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(ref_option_ref::RefOptionRef));
     store.register_late_pass(|| Box::new(infinite_iter::InfiniteIter));
     store.register_late_pass(|| Box::new(inline_fn_without_body::InlineFnWithoutBody));
-    store.register_late_pass(|| Box::new(useless_conversion::UselessConversion::default()));
+    store.register_late_pass(|| Box::<useless_conversion::UselessConversion>::default());
     store.register_late_pass(|| Box::new(implicit_hasher::ImplicitHasher));
     store.register_late_pass(|| Box::new(fallible_impl_from::FallibleImplFrom));
     store.register_late_pass(|| Box::new(question_mark::QuestionMark));
@@ -773,7 +774,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             upper_case_acronyms_aggressive,
         ))
     });
-    store.register_late_pass(|| Box::new(default::Default::default()));
+    store.register_late_pass(|| Box::<default::Default>::default());
     store.register_late_pass(move || Box::new(unused_self::UnusedSelf::new(avoid_breaking_exported_api)));
     store.register_late_pass(|| Box::new(mutable_debug_assertion::DebugAssertWithMutCall));
     store.register_late_pass(|| Box::new(exit::Exit));
@@ -796,7 +797,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| Box::new(option_env_unwrap::OptionEnvUnwrap));
     let warn_on_all_wildcard_imports = conf.warn_on_all_wildcard_imports;
     store.register_late_pass(move || Box::new(wildcard_imports::WildcardImports::new(warn_on_all_wildcard_imports)));
-    store.register_late_pass(|| Box::new(redundant_pub_crate::RedundantPubCrate::default()));
+    store.register_late_pass(|| Box::<redundant_pub_crate::RedundantPubCrate>::default());
     store.register_late_pass(|| Box::new(unnamed_address::UnnamedAddress));
     store.register_late_pass(move || Box::new(dereference::Dereferencing::new(msrv)));
     store.register_late_pass(|| Box::new(option_if_let_else::OptionIfLetElse));
@@ -814,7 +815,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     });
     let macro_matcher = conf.standard_macro_braces.iter().cloned().collect::<FxHashSet<_>>();
     store.register_early_pass(move || Box::new(nonstandard_macro_braces::MacroBraces::new(&macro_matcher)));
-    store.register_late_pass(|| Box::new(macro_use::MacroUseImports::default()));
+    store.register_late_pass(|| Box::<macro_use::MacroUseImports>::default());
     store.register_late_pass(|| Box::new(pattern_type_mismatch::PatternTypeMismatch));
     store.register_late_pass(|| Box::new(unwrap_in_result::UnwrapInResult));
     store.register_late_pass(|| Box::new(semicolon_if_nothing_returned::SemicolonIfNothingReturned));
@@ -827,7 +828,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(strings::StrToString));
     store.register_late_pass(|| Box::new(strings::StringToString));
     store.register_late_pass(|| Box::new(zero_sized_map_values::ZeroSizedMapValues));
-    store.register_late_pass(|| Box::new(vec_init_then_push::VecInitThenPush::default()));
+    store.register_late_pass(|| Box::<vec_init_then_push::VecInitThenPush>::default());
     store.register_late_pass(|| Box::new(redundant_slicing::RedundantSlicing));
     store.register_late_pass(|| Box::new(from_str_radix_10::FromStrRadix10));
     store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv)));
@@ -865,7 +866,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(manual_bits::ManualBits::new(msrv)));
     store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation));
     store.register_early_pass(|| Box::new(doc_link_with_quotes::DocLinkWithQuotes));
-    store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion::default()));
+    store.register_late_pass(|| Box::<only_used_in_recursion::OnlyUsedInRecursion>::default());
     let allow_dbg_in_tests = conf.allow_dbg_in_tests;
     store.register_late_pass(move || Box::new(dbg_macro::DbgMacro::new(allow_dbg_in_tests)));
     let cargo_ignore_publish = conf.cargo_ignore_publish;
@@ -874,7 +875,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             ignore_publish: cargo_ignore_publish,
         })
     });
-    store.register_late_pass(|| Box::new(write::Write::default()));
+    store.register_late_pass(|| Box::<write::Write>::default());
     store.register_early_pass(|| Box::new(crate_in_macro_def::CrateInMacroDef));
     store.register_early_pass(|| Box::new(empty_structs_with_brackets::EmptyStructsWithBrackets));
     store.register_late_pass(|| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings));
@@ -884,7 +885,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(large_include_file::LargeIncludeFile::new(max_include_file_size)));
     store.register_late_pass(|| Box::new(strings::TrimSplitWhitespace));
     store.register_late_pass(|| Box::new(rc_clone_in_vec_init::RcCloneInVecInit));
-    store.register_early_pass(|| Box::new(duplicate_mod::DuplicateMod::default()));
+    store.register_early_pass(|| Box::<duplicate_mod::DuplicateMod>::default());
     store.register_early_pass(|| Box::new(unused_rounding::UnusedRounding));
     store.register_early_pass(move || Box::new(almost_complete_letter_range::AlmostCompleteLetterRange::new(msrv)));
     store.register_late_pass(|| Box::new(swap_ptr_to_ref::SwapPtrToRef));
@@ -896,13 +897,14 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let verbose_bit_mask_threshold = conf.verbose_bit_mask_threshold;
     store.register_late_pass(move || Box::new(operators::Operators::new(verbose_bit_mask_threshold)));
     store.register_late_pass(|| Box::new(invalid_utf8_in_unchecked::InvalidUtf8InUnchecked));
-    store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports::default()));
+    store.register_late_pass(|| Box::<std_instead_of_core::StdReexports>::default());
     store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed));
     store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone));
     store.register_late_pass(|| Box::new(manual_string_new::ManualStringNew));
     store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable));
     store.register_early_pass(|| Box::new(multi_assignments::MultiAssignments));
     store.register_late_pass(|| Box::new(bool_to_int_with_if::BoolToIntWithIf));
+    store.register_late_pass(|| Box::new(box_default::BoxDefault));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

src/docs.rs

@@ -48,6 +48,7 @@ docs! {
     "borrow_interior_mutable_const",
     "borrowed_box",
     "box_collection",
+    "box_default",
     "boxed_local",
     "branches_sharing_code",
     "builtin_type_shadow",

src/docs/box_default.txt

@@ -0,0 +1,23 @@
+### What it does
+checks for `Box::new(T::default())`, which is better written as
+`Box::<T>::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).
+
+### 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());
+```
+Use instead:
+```
+let x: Box<String> = Box::default();
+```

tests/ui/box_collection.rs

@@ -15,7 +15,7 @@ macro_rules! boxit {
 }
 
 fn test_macro() {
-    boxit!(Vec::new(), Vec<u8>);
+    boxit!(vec![1], Vec<u8>);
 }
 
 fn test1(foo: Box<Vec<bool>>) {}
@@ -50,7 +50,7 @@ fn test_local_not_linted() {
 pub fn pub_test(foo: Box<Vec<bool>>) {}
 
 pub fn pub_test_ret() -> Box<Vec<bool>> {
-    Box::new(Vec::new())
+    Box::default()
 }
 
 fn main() {}

tests/ui/box_default.rs

@@ -0,0 +1,31 @@
+#![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::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()));
+    // false negative: default is from different expansion
+    let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
+}