From 63f441ec855f0de342ccc2b9ff90776c633ca9e6 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 21 Sep 2022 23:43:49 +0200 Subject: [PATCH] add `box-default` lint --- CHANGELOG.md | 1 + clippy_lints/src/box_default.rs | 61 ++++++++++++++++++++++++++ clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_perf.rs | 1 + clippy_lints/src/lib.rs | 30 +++++++------ src/docs.rs | 1 + src/docs/box_default.txt | 23 ++++++++++ tests/ui/box_collection.rs | 4 +- tests/ui/box_default.rs | 31 +++++++++++++ tests/ui/box_default.stderr | 59 +++++++++++++++++++++++++ 11 files changed, 197 insertions(+), 16 deletions(-) create mode 100644 clippy_lints/src/box_default.rs create mode 100644 src/docs/box_default.txt create mode 100644 tests/ui/box_default.rs create mode 100644 tests/ui/box_default.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 044cbff4b..f1e2e6f46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3609,6 +3609,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 diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs new file mode 100644 index 000000000..792183ac4 --- /dev/null +++ b/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::::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 = Box::new(Default::default()); + /// ``` + /// Use instead: + /// ```rust + /// let x: Box = 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", + ); + } + } +} diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index ae57a9a24..33490d1cc 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 02fcc8de5..93ea32fa0 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index 195ce41e3..8e927470e 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/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), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 83fdc15c9..7bd49b110 100644 --- a/clippy_lints/src/lib.rs +++ b/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; @@ -536,8 +537,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::::default()); + store.register_late_pass(|| Box::::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)); @@ -630,10 +631,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::::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::::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)); @@ -667,7 +668,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::::default()); let disallowed_names = conf.disallowed_names.iter().cloned().collect::>(); store.register_late_pass(move || Box::new(disallowed_names::DisallowedNames::new(disallowed_names.clone()))); let too_many_arguments_threshold = conf.too_many_arguments_threshold; @@ -706,7 +707,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::::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)); @@ -776,7 +777,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()); 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)); @@ -799,7 +800,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::::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)); @@ -817,7 +818,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); let macro_matcher = conf.standard_macro_braces.iter().cloned().collect::>(); store.register_early_pass(move || Box::new(nonstandard_macro_braces::MacroBraces::new(¯o_matcher))); - store.register_late_pass(|| Box::new(macro_use::MacroUseImports::default())); + store.register_late_pass(|| Box::::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)); @@ -830,7 +831,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::::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))); @@ -868,7 +869,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::::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; @@ -877,7 +878,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::::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)); @@ -887,7 +888,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::::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)); @@ -899,13 +900,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::::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` } diff --git a/src/docs.rs b/src/docs.rs index 6c89b4dde..8ffe1281f 100644 --- a/src/docs.rs +++ b/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", diff --git a/src/docs/box_default.txt b/src/docs/box_default.txt new file mode 100644 index 000000000..ffac894d0 --- /dev/null +++ b/src/docs/box_default.txt @@ -0,0 +1,23 @@ +### What it does +checks for `Box::new(T::default())`, which is better 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). + +### 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 = Box::new(Default::default()); +``` +Use instead: +``` +let x: Box = Box::default(); +``` \ No newline at end of file diff --git a/tests/ui/box_collection.rs b/tests/ui/box_collection.rs index 0780c8f05..4c9947b9a 100644 --- a/tests/ui/box_collection.rs +++ b/tests/ui/box_collection.rs @@ -15,7 +15,7 @@ macro_rules! boxit { } fn test_macro() { - boxit!(Vec::new(), Vec); + boxit!(vec![1], Vec); } fn test1(foo: Box>) {} @@ -50,7 +50,7 @@ fn test_local_not_linted() { pub fn pub_test(foo: Box>) {} pub fn pub_test_ret() -> Box> { - Box::new(Vec::new()) + Box::default() } fn main() {} diff --git a/tests/ui/box_default.rs b/tests/ui/box_default.rs new file mode 100644 index 000000000..dc522705b --- /dev/null +++ b/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 = Box::new(Default::default()); + let _byte = Box::new(u8::default()); + let _vec = Box::new(Vec::::new()); + let _impl = Box::new(ImplementsDefault::default()); + let _impl2 = Box::new(::default()); + let _impl3: Box = 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> = Box::new(vec![]); +} diff --git a/tests/ui/box_default.stderr b/tests/ui/box_default.stderr new file mode 100644 index 000000000..341766a50 --- /dev/null +++ b/tests/ui/box_default.stderr @@ -0,0 +1,59 @@ +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:21:32 + | +LL | let _string: Box = Box::new(Default::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::box-default` implied by `-D warnings` + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:22:17 + | +LL | let _byte = Box::new(u8::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:23:16 + | +LL | let _vec = Box::new(Vec::::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:24:17 + | +LL | let _impl = Box::new(ImplementsDefault::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:25:18 + | +LL | let _impl2 = Box::new(::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:26:42 + | +LL | let _impl3: Box = Box::new(Default::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:28:28 + | +LL | let _in_macro = outer!(Box::new(String::new())); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use `Box::default()` instead + +error: aborting due to 7 previous errors +