diff options
| -rw-r--r-- | clippy_lints/src/box_default.rs | 94 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 26 | ||||
| -rw-r--r-- | src/docs/box_default.txt | 6 | ||||
| -rw-r--r-- | tests/ui/box_default.fixed | 43 | ||||
| -rw-r--r-- | tests/ui/box_default.rs | 14 | ||||
| -rw-r--r-- | tests/ui/box_default.stderr | 77 |
6 files changed, 212 insertions, 48 deletions
diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs index 792183ac408..f35a79dcc73 100644 --- a/clippy_lints/src/box_default.rs +++ b/clippy_lints/src/box_default.rs @@ -1,5 +1,12 @@ -use clippy_utils::{diagnostics::span_lint_and_help, is_default_equivalent, path_def_id}; -use rustc_hir::{Expr, ExprKind, QPath}; +use clippy_utils::{ + diagnostics::span_lint_and_sugg, get_parent_node, is_default_equivalent, macros::macro_backtrace, match_path, + path_def_id, paths, ty::expr_sig, +}; +use rustc_errors::Applicability; +use rustc_hir::{ + intravisit::{walk_ty, Visitor}, + Block, Expr, ExprKind, Local, Node, QPath, TyKind, +}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -15,12 +22,6 @@ declare_clippy_lint! { /// 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()); @@ -41,21 +42,88 @@ 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 + && let ExprKind::Call(arg_path, ..) = arg.kind && !in_external_macro(cx.sess(), expr.span) - && expr.span.eq_ctxt(arg.span) + && (expr.span.eq_ctxt(arg.span) || is_vec_expn(cx, arg)) && 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( + let arg_ty = cx.typeck_results().expr_ty(arg); + span_lint_and_sugg( cx, BOX_DEFAULT, expr.span, "`Box::new(_)` of default value", - None, - "use `Box::default()` instead", + "try", + if is_plain_default(arg_path) || given_type(cx, expr) { + "Box::default()".into() + } else { + format!("Box::<{arg_ty}>::default()") + }, + Applicability::MachineApplicable ); } } } + +fn is_plain_default(arg_path: &Expr<'_>) -> bool { + // we need to match the actual path so we don't match e.g. "u8::default" + if let ExprKind::Path(QPath::Resolved(None, path)) = &arg_path.kind { + // avoid generic parameters + match_path(path, &paths::DEFAULT_TRAIT_METHOD) && path.segments.iter().all(|seg| seg.args.is_none()) + } else { + false + } +} + +fn is_vec_expn(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + macro_backtrace(expr.span) + .next() + .map_or(false, |call| cx.tcx.is_diagnostic_item(sym::vec_macro, call.def_id)) +} + +#[derive(Default)] +struct InferVisitor(bool); + +impl<'tcx> Visitor<'tcx> for InferVisitor { + fn visit_ty(&mut self, t: &rustc_hir::Ty<'_>) { + self.0 |= matches!(t.kind, TyKind::Infer); + if !self.0 { + walk_ty(self, t); + } + } +} + +fn given_type(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + match get_parent_node(cx.tcx, expr.hir_id) { + Some(Node::Local(Local { ty: Some(ty), .. })) => { + let mut v = InferVisitor::default(); + v.visit_ty(ty); + !v.0 + }, + Some( + Node::Expr(Expr { + kind: ExprKind::Call(path, args), + .. + }) | Node::Block(Block { + expr: + Some(Expr { + kind: ExprKind::Call(path, args), + .. + }), + .. + }), + ) => { + if let Some(index) = args.iter().position(|arg| arg.hir_id == expr.hir_id) && + let Some(sig) = expr_sig(cx, path) && + let Some(input) = sig.input(index) + { + input.no_bound_vars().is_some() + } else { + false + } + }, + _ => false, + } +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 42374fdd7ba..e6492d76260 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -815,13 +815,37 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { false } }, - ExprKind::Call(repl_func, _) => is_default_equivalent_call(cx, repl_func), + ExprKind::Call(repl_func, []) => is_default_equivalent_call(cx, repl_func), + ExprKind::Call(from_func, [ref arg]) => is_default_equivalent_from(cx, from_func, arg), ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone), ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])), _ => false, } } +fn is_default_equivalent_from(cx: &LateContext<'_>, from_func: &Expr<'_>, arg: &Expr<'_>) -> bool { + if let ExprKind::Path(QPath::TypeRelative(ty, seg)) = from_func.kind && + seg.ident.name == sym::from + { + match arg.kind { + ExprKind::Lit(hir::Lit { + node: LitKind::Str(ref sym, _), + .. + }) => return sym.is_empty() && is_path_diagnostic_item(cx, ty, sym::String), + ExprKind::Array([]) => return is_path_diagnostic_item(cx, ty, sym::Vec), + ExprKind::Repeat(_, ArrayLen::Body(len)) => { + if let ExprKind::Lit(ref const_lit) = cx.tcx.hir().body(len.body).value.kind && + let LitKind::Int(v, _) = const_lit.node + { + return v == 0 && is_path_diagnostic_item(cx, ty, sym::Vec); + } + } + _ => (), + } + } + false +} + /// Checks if the top level expression can be moved into a closure as is. /// Currently checks for: /// * Break/Continue outside the given loop HIR ids. diff --git a/src/docs/box_default.txt b/src/docs/box_default.txt index ffac894d0c5..1c670c77333 100644 --- a/src/docs/box_default.txt +++ b/src/docs/box_default.txt @@ -7,12 +7,6 @@ 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()); diff --git a/tests/ui/box_default.fixed b/tests/ui/box_default.fixed new file mode 100644 index 00000000000..7fbb272ce5a --- /dev/null +++ b/tests/ui/box_default.fixed @@ -0,0 +1,43 @@ +// run-rustfix +#![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::default(); + let _byte = Box::<u8>::default(); + let _vec = Box::<std::vec::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::<std::string::String>::default()); + let _string_default = outer!(Box::<std::string::String>::default()); + let _vec2: Box<Vec<ImplementsDefault>> = Box::default(); + let _vec3: Box<Vec<bool>> = Box::default(); + let _vec4: Box<_> = Box::<std::vec::Vec<bool>>::default(); + let _more = ret_ty_fn(); + call_ty_fn(Box::default()); +} + +fn ret_ty_fn() -> Box<bool> { + Box::<bool>::default() +} + +#[allow(clippy::boxed_local)] +fn call_ty_fn(_b: Box<u8>) {} diff --git a/tests/ui/box_default.rs b/tests/ui/box_default.rs index dc522705bc6..64c4f3887af 100644 --- a/tests/ui/box_default.rs +++ b/tests/ui/box_default.rs @@ -1,3 +1,4 @@ +// run-rustfix #![warn(clippy::box_default)] #[derive(Default)] @@ -26,6 +27,17 @@ fn main() { 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 _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(); + call_ty_fn(Box::new(u8::default())); } + +fn ret_ty_fn() -> Box<bool> { + Box::new(bool::default()) +} + +#[allow(clippy::boxed_local)] +fn call_ty_fn(_b: Box<u8>) {} diff --git a/tests/ui/box_default.stderr b/tests/ui/box_default.stderr index b2030e95acb..313255fc950 100644 --- a/tests/ui/box_default.stderr +++ b/tests/ui/box_default.stderr @@ -1,59 +1,82 @@ error: `Box::new(_)` of default value - --> $DIR/box_default.rs:21:32 + --> $DIR/box_default.rs:22:32 | LL | let _string: Box<String> = Box::new(Default::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` | - = help: use `Box::default()` instead = note: `-D clippy::box-default` implied by `-D warnings` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:22:17 + --> $DIR/box_default.rs:23:17 | LL | let _byte = Box::new(u8::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: use `Box::default()` instead + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<u8>::default()` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:23:16 + --> $DIR/box_default.rs:24:16 | LL | let _vec = Box::new(Vec::<u8>::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: use `Box::default()` instead + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<u8>>::default()` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:24:17 + --> $DIR/box_default.rs:25:17 | LL | let _impl = Box::new(ImplementsDefault::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: use `Box::default()` instead + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:25:18 + --> $DIR/box_default.rs:26:18 | LL | let _impl2 = Box::new(<ImplementsDefault as Default>::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: use `Box::default()` instead + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<ImplementsDefault>::default()` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:26:42 + --> $DIR/box_default.rs:27:42 | LL | let _impl3: Box<ImplementsDefault> = Box::new(Default::default()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: use `Box::default()` instead + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` error: `Box::new(_)` of default value - --> $DIR/box_default.rs:28:28 + --> $DIR/box_default.rs:29:28 | LL | let _in_macro = outer!(Box::new(String::new())); - | ^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:30:34 + | +LL | let _string_default = outer!(Box::new(String::from(""))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::string::String>::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:31:46 + | +LL | let _vec2: Box<Vec<ImplementsDefault>> = Box::new(vec![]); + | ^^^^^^^^^^^^^^^^ help: try: `Box::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:32:33 + | +LL | let _vec3: Box<Vec<bool>> = Box::new(Vec::from([])); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:33:25 + | +LL | let _vec4: Box<_> = Box::new(Vec::from([false; 0])); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::vec::Vec<bool>>::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:35:16 + | +LL | call_ty_fn(Box::new(u8::default())); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::default()` + +error: `Box::new(_)` of default value + --> $DIR/box_default.rs:39:5 | - = help: use `Box::default()` instead +LL | Box::new(bool::default()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<bool>::default()` -error: aborting due to 7 previous errors +error: aborting due to 13 previous errors |