Skip to content

Commit 2c8e473

Browse files
borsbors
committed
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
2 parents 65ae666 + d3c041a commit 2c8e473

File tree

6 files changed

+212
-48
lines changed

6 files changed

+212
-48
lines changed

‎clippy_lints/src/box_default.rs‎

Lines changed: 81 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
1-
use clippy_utils::{diagnostics::span_lint_and_help, is_default_equivalent, path_def_id};
2-
use rustc_hir::{Expr, ExprKind, QPath};
1+
use clippy_utils::{
2+
diagnostics::span_lint_and_sugg, get_parent_node, is_default_equivalent, macros::macro_backtrace, match_path,
3+
path_def_id, paths, ty::expr_sig,
4+
};
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{
7+
intravisit::{walk_ty, Visitor},
8+
Block, Expr, ExprKind, Local, Node, QPath, TyKind,
9+
};
310
use rustc_lint::{LateContext, LateLintPass, LintContext};
411
use rustc_middle::lint::in_external_macro;
512
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -15,12 +22,6 @@ declare_clippy_lint! {
1522
/// Second, `Box::default()` can be faster
1623
/// [in certain cases](https://nnethercote.github.io/perf-book/standard-library-types.html#box).
1724
///
18-
/// ### Known problems
19-
/// The lint may miss some cases (e.g. Box::new(String::from(""))).
20-
/// On the other hand, it will trigger on cases where the `default`
21-
/// code comes from a macro that does something different based on
22-
/// e.g. target operating system.
23-
///
2425
/// ### Example
2526
/// ```rust
2627
/// let x: Box<String> = Box::new(Default::default());
@@ -41,21 +42,88 @@ impl LateLintPass<'_> for BoxDefault {
4142
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
4243
if let ExprKind::Call(box_new, [arg]) = expr.kind
4344
&& let ExprKind::Path(QPath::TypeRelative(ty, seg)) = box_new.kind
44-
&& let ExprKind::Call(..) = arg.kind
45+
&& let ExprKind::Call(arg_path, ..) = arg.kind
4546
&& !in_external_macro(cx.sess(), expr.span)
46-
&& expr.span.eq_ctxt(arg.span)
47+
&& (expr.span.eq_ctxt(arg.span) || is_vec_expn(cx, arg))
4748
&& seg.ident.name == sym::new
4849
&& path_def_id(cx, ty) == cx.tcx.lang_items().owned_box()
4950
&& is_default_equivalent(cx, arg)
5051
{
51-
span_lint_and_help(
52+
let arg_ty = cx.typeck_results().expr_ty(arg);
53+
span_lint_and_sugg(
5254
cx,
5355
BOX_DEFAULT,
5456
expr.span,
5557
"`Box::new(_)` of default value",
56-
None,
57-
"use `Box::default()` instead",
58+
"try",
59+
if is_plain_default(arg_path) || given_type(cx, expr) {
60+
"Box::default()".into()
61+
} else {
62+
format!("Box::<{arg_ty}>::default()")
63+
},
64+
Applicability::MachineApplicable
5865
);
5966
}
6067
}
6168
}
69+
70+
fn is_plain_default(arg_path: &Expr<'_>) -> bool {
71+
// we need to match the actual path so we don't match e.g. "u8::default"
72+
if let ExprKind::Path(QPath::Resolved(None, path)) = &arg_path.kind {
73+
// avoid generic parameters
74+
match_path(path, &paths::DEFAULT_TRAIT_METHOD) && path.segments.iter().all(|seg| seg.args.is_none())
75+
} else {
76+
false
77+
}
78+
}
79+
80+
fn is_vec_expn(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
81+
macro_backtrace(expr.span)
82+
.next()
83+
.map_or(false, |call| cx.tcx.is_diagnostic_item(sym::vec_macro, call.def_id))
84+
}
85+
86+
#[derive(Default)]
87+
struct InferVisitor(bool);
88+
89+
impl<'tcx> Visitor<'tcx> for InferVisitor {
90+
fn visit_ty(&mut self, t: &rustc_hir::Ty<'_>) {
91+
self.0 |= matches!(t.kind, TyKind::Infer);
92+
if !self.0 {
93+
walk_ty(self, t);
94+
}
95+
}
96+
}
97+
98+
fn given_type(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
99+
match get_parent_node(cx.tcx, expr.hir_id) {
100+
Some(Node::Local(Local { ty: Some(ty), .. })) => {
101+
let mut v = InferVisitor::default();
102+
v.visit_ty(ty);
103+
!v.0
104+
},
105+
Some(
106+
Node::Expr(Expr {
107+
kind: ExprKind::Call(path, args),
108+
..
109+
}) | Node::Block(Block {
110+
expr:
111+
Some(Expr {
112+
kind: ExprKind::Call(path, args),
113+
..
114+
}),
115+
..
116+
}),
117+
) => {
118+
if let Some(index) = args.iter().position(|arg| arg.hir_id == expr.hir_id) &&
119+
let Some(sig) = expr_sig(cx, path) &&
120+
let Some(input) = sig.input(index)
121+
{
122+
input.no_bound_vars().is_some()
123+
} else {
124+
false
125+
}
126+
},
127+
_ => false,
128+
}
129+
}

‎clippy_utils/src/lib.rs‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -815,13 +815,37 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
815815
false
816816
}
817817
},
818-
ExprKind::Call(repl_func, _) => is_default_equivalent_call(cx, repl_func),
818+
ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func),
819+
ExprKind::Call(from_func, [ref arg]) => is_default_equivalent_from(cx, from_func, arg),
819820
ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone),
820821
ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])),
821822
_ => false,
822823
}
823824
}
824825

826+
fn is_default_equivalent_from(cx: &LateContext<'_>, from_func: &Expr<'_>, arg: &Expr<'_>) -> bool {
827+
if let ExprKind::Path(QPath::TypeRelative(ty, seg)) = from_func.kind &&
828+
seg.ident.name == sym::from
829+
{
830+
match arg.kind {
831+
ExprKind::Lit(hir::Lit {
832+
node: LitKind::Str(ref sym, _),
833+
..
834+
}) => return sym.is_empty() && is_path_diagnostic_item(cx, ty, sym::String),
835+
ExprKind::Array([]) => return is_path_diagnostic_item(cx, ty, sym::Vec),
836+
ExprKind::Repeat(_, ArrayLen::Body(len)) => {
837+
if let ExprKind::Lit(ref const_lit) = cx.tcx.hir().body(len.body).value.kind &&
838+
let LitKind::Int(v, _) = const_lit.node
839+
{
840+
return v == 0 && is_path_diagnostic_item(cx, ty, sym::Vec);
841+
}
842+
}
843+
_ => (),
844+
}
845+
}
846+
false
847+
}
848+
825849
/// Checks if the top level expression can be moved into a closure as is.
826850
/// Currently checks for:
827851
/// * Break/Continue outside the given loop HIR ids.

‎src/docs/box_default.txt‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,6 @@ First, it's more complex, involving two calls instead of one.
77
Second, `Box::default()` can be faster
88
[in certain cases](https://nnethercote.github.io/perf-book/standard-library-types.html#box).
99

10-
### Known problems
11-
The lint may miss some cases (e.g. Box::new(String::from(""))).
12-
On the other hand, it will trigger on cases where the `default`
13-
code comes from a macro that does something different based on
14-
e.g. target operating system.
15-
1610
### Example
1711
```
1812
let x: Box<String> = Box::new(Default::default());

‎tests/ui/box_default.fixed‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// run-rustfix
2+
#![warn(clippy::box_default)]
3+
4+
#[derive(Default)]
5+
struct ImplementsDefault;
6+
7+
struct OwnDefault;
8+
9+
impl OwnDefault {
10+
fn default() -> Self {
11+
Self
12+
}
13+
}
14+
15+
macro_rules! outer {
16+
($e: expr) => {
17+
$e
18+
};
19+
}
20+
21+
fn main() {
22+
let _string: Box<String> = Box::default();
23+
let _byte = Box::<u8>::default();
24+
let _vec = Box::<std::vec::Vec<u8>>::default();
25+
let _impl = Box::<ImplementsDefault>::default();
26+
let _impl2 = Box::<ImplementsDefault>::default();
27+
let _impl3: Box<ImplementsDefault> = Box::default();
28+
let _own = Box::new(OwnDefault::default()); // should not lint
29+
let _in_macro = outer!(Box::<std::string::String>::default());
30+
let _string_default = outer!(Box::<std::string::String>::default());
31+
let _vec2: Box<Vec<ImplementsDefault>> = Box::default();
32+
let _vec3: Box<Vec<bool>> = Box::default();
33+
let _vec4: Box<_> = Box::<std::vec::Vec<bool>>::default();
34+
let _more = ret_ty_fn();
35+
call_ty_fn(Box::default());
36+
}
37+
38+
fn ret_ty_fn() -> Box<bool> {
39+
Box::<bool>::default()
40+
}
41+
42+
#[allow(clippy::boxed_local)]
43+
fn call_ty_fn(_b: Box<u8>) {}

‎tests/ui/box_default.rs‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// run-rustfix
12
#![warn(clippy::box_default)]
23

34
#[derive(Default)]
@@ -26,6 +27,17 @@ fn main() {
2627
let _impl3: Box<ImplementsDefault> = Box::new(Default::default());
2728
let _own = Box::new(OwnDefault::default()); // should not lint
2829
let _in_macro = outer!(Box::new(String::new()));
29-
// false negative: default is from different expansion
30+
let _string_default = outer!(Box::new(String::from("")));
3031
let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
32+
let _vec3: Box<Vec<bool>> = Box::new(Vec::from([]));
33+
let _vec4: Box<_> = Box::new(Vec::from([false; 0]));
34+
let _more = ret_ty_fn();
35+
call_ty_fn(Box::new(u8::default()));
3136
}
37+
38+
fn ret_ty_fn() -> Box<bool> {
39+
Box::new(bool::default())
40+
}
41+
42+
#[allow(clippy::boxed_local)]
43+
fn call_ty_fn(_b: Box<u8>) {}

‎tests/ui/box_default.stderr‎

Lines changed: 50 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,59 +1,82 @@
11
error: `Box::new(_)` of default value
2-
--> $DIR/box_default.rs:21:32
2+
--> $DIR/box_default.rs:22:32
33
|
44
LL | let _string: Box<String> = Box::new(Default::default());
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
66
|
7-
= help: use `Box::default()` instead
87
= note: `-D clippy::box-default` implied by `-D warnings`
98

109
error: `Box::new(_)` of default value
11-
--> $DIR/box_default.rs:22:17
10+
--> $DIR/box_default.rs:23:17
1211
|
1312
LL | let _byte = Box::new(u8::default());
14-
| ^^^^^^^^^^^^^^^^^^^^^^^
15-
|
16-
= help: use `Box::default()` instead
13+
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<u8>::default()`
1714

1815
error: `Box::new(_)` of default value
19-
--> $DIR/box_default.rs:23:16
16+
--> $DIR/box_default.rs:24:16
2017
|
2118
LL | let _vec = Box::new(Vec::<u8>::new());
22-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
23-
|
24-
= help: use `Box::default()` instead
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<u8>>::default()`
2520

2621
error: `Box::new(_)` of default value
27-
--> $DIR/box_default.rs:24:17
22+
--> $DIR/box_default.rs:25:17
2823
|
2924
LL | let _impl = Box::new(ImplementsDefault::default());
30-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31-
|
32-
= help: use `Box::default()` instead
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()`
3326

3427
error: `Box::new(_)` of default value
35-
--> $DIR/box_default.rs:25:18
28+
--> $DIR/box_default.rs:26:18
3629
|
3730
LL | let _impl2 = Box::new(<ImplementsDefault as Default>::default());
38-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
39-
|
40-
= help: use `Box::default()` instead
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()`
4132

4233
error: `Box::new(_)` of default value
43-
--> $DIR/box_default.rs:26:42
34+
--> $DIR/box_default.rs:27:42
4435
|
4536
LL | let _impl3: Box<ImplementsDefault> = Box::new(Default::default());
46-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
47-
|
48-
= help: use `Box::default()` instead
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
4938

5039
error: `Box::new(_)` of default value
51-
--> $DIR/box_default.rs:28:28
40+
--> $DIR/box_default.rs:29:28
5241
|
5342
LL | let _in_macro = outer!(Box::new(String::new()));
54-
| ^^^^^^^^^^^^^^^^^^^^^^^
43+
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()`
44+
45+
error: `Box::new(_)` of default value
46+
--> $DIR/box_default.rs:30:34
47+
|
48+
LL | let _string_default = outer!(Box::new(String::from("")));
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()`
50+
51+
error: `Box::new(_)` of default value
52+
--> $DIR/box_default.rs:31:46
53+
|
54+
LL | let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]);
55+
| ^^^^^^^^^^^^^^^^ help: try: `Box::default()`
56+
57+
error: `Box::new(_)` of default value
58+
--> $DIR/box_default.rs:32:33
59+
|
60+
LL | let _vec3: Box<Vec<bool>> = Box::new(Vec::from([]));
61+
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
62+
63+
error: `Box::new(_)` of default value
64+
--> $DIR/box_default.rs:33:25
65+
|
66+
LL | let _vec4: Box<_> = Box::new(Vec::from([false; 0]));
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<bool>>::default()`
68+
69+
error: `Box::new(_)` of default value
70+
--> $DIR/box_default.rs:35:16
71+
|
72+
LL | call_ty_fn(Box::new(u8::default()));
73+
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()`
74+
75+
error: `Box::new(_)` of default value
76+
--> $DIR/box_default.rs:39:5
5577
|
56-
= help: use `Box::default()` instead
78+
LL | Box::new(bool::default())
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<bool>::default()`
5780

58-
error: aborting due to 7 previous errors
81+
error: aborting due to 13 previous errors
5982

0 commit comments

Comments
 (0)