Skip to content

Move box_default to style, do not suggest turbofishes #12601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Move box_default to style, do not suggest turbofishes #12601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 8 additions & 43 deletions clippy_lints/src/box_default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand All @@ -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()"
}

Expand All @@ -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,
);
}
Expand All @@ -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)
Expand Down
85 changes: 47 additions & 38 deletions tests/ui/box_default.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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();

Expand All @@ -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();
}
Expand All @@ -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();
}

Expand All @@ -70,7 +93,7 @@ fn issue_10089() {
#[derive(Default)]
struct WeirdPathed;

let _ = Box::<WeirdPathed>::default();
let _ = Box::new(WeirdPathed::default());
};
}

Expand All @@ -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
}
Expand All @@ -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() {
Expand All @@ -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());
}
}
75 changes: 42 additions & 33 deletions tests/ui/box_default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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();

Expand All @@ -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();
}
Expand Down Expand Up @@ -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() {
Expand Down
Loading