about summary refs log tree commit diff
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2024-02-26 17:25:43 +0000
committerbors <bors@rust-lang.org>2024-02-26 17:25:43 +0000
commitd71899573a2826392ef08dc3bea796f5e077ecdf (patch)
treeaf3b3ff4af2aadd1a95c59490695ceb11e0d2da8
parenta11875bd226a5ecedd87b766bfe4f7bd2df32299 (diff)
parent97dc4b22c61ab52de80b495ca0e583c4c5c73a9f (diff)
downloadrust-d71899573a2826392ef08dc3bea796f5e077ecdf.tar.gz
rust-d71899573a2826392ef08dc3bea796f5e077ecdf.zip
Auto merge of #12355 - Ethiraric:fix-11927, r=Alexendoo
[`box_default`]: Preserve required path segments

When encountering code such as:
```rs
Box::new(outer::Inner::default())
```
clippy would suggest replacing with `Box::<Inner>::default()`, dropping the `outer::` segment. This behavior is incorrect and that commit fixes it.

What it does is it checks the contents of the `Box::new` and, if it is of the form `A::B::default`, does a text replacement, inserting `A::B` in the `Box`'s quickfix generic list.
If the source does not match that pattern (including `Vec::from(..)` or other `T::new()` calls), we then fallback to the original code.

Fixes #11927

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`box_default`]: Preserve required path segments
-rw-r--r--clippy_lints/src/box_default.rs46
-rw-r--r--tests/ui/box_default.fixed18
-rw-r--r--tests/ui/box_default.rs16
-rw-r--r--tests/ui/box_default.stderr16
4 files changed, 88 insertions, 8 deletions
diff --git a/clippy_lints/src/box_default.rs b/clippy_lints/src/box_default.rs
index d9e63f0d047..779ae03c464 100644
--- a/clippy_lints/src/box_default.rs
+++ b/clippy_lints/src/box_default.rs
@@ -1,11 +1,12 @@
 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;
 use rustc_hir::def::Res;
 use rustc_hir::intravisit::{walk_ty, Visitor};
-use rustc_hir::{Block, Expr, ExprKind, Local, Node, QPath, TyKind};
+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;
@@ -41,13 +42,24 @@ declare_lint_pass!(BoxDefault => [BOX_DEFAULT]);
 
 impl LateLintPass<'_> for BoxDefault {
     fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        // If the expression is a call (`Box::new(...)`)
         if let ExprKind::Call(box_new, [arg]) = expr.kind
+            // And call is of the form `<T>::something`
+            // Here, it would be `<Box>::new`
             && let ExprKind::Path(QPath::TypeRelative(ty, seg)) = box_new.kind
-            && let ExprKind::Call(arg_path, ..) = arg.kind
-            && !in_external_macro(cx.sess(), expr.span)
-            && (expr.span.eq_ctxt(arg.span) || is_local_vec_expn(cx, arg, expr))
+            // And that method is `new`
             && seg.ident.name == sym::new
+            // And the call is that of a `Box` method
             && 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
+            // 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)
         {
             span_lint_and_sugg(
@@ -59,7 +71,17 @@ impl LateLintPass<'_> for BoxDefault {
                 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) {
-                    with_forced_trimmed_paths!(format!("Box::<{arg_ty}>::default()"))
+                    // 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;
                 },
@@ -81,6 +103,20 @@ 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)
diff --git a/tests/ui/box_default.fixed b/tests/ui/box_default.fixed
index 48408e19125..fea7405c685 100644
--- a/tests/ui/box_default.fixed
+++ b/tests/ui/box_default.fixed
@@ -21,7 +21,7 @@ macro_rules! outer {
 fn main() {
     let _string: Box<String> = Box::default();
     let _byte = Box::<u8>::default();
-    let _vec = Box::<Vec<u8>>::default();
+    let _vec = Box::<Vec::<u8>>::default();
     let _impl = Box::<ImplementsDefault>::default();
     let _impl2 = Box::<ImplementsDefault>::default();
     let _impl3: Box<ImplementsDefault> = Box::default();
@@ -104,3 +104,19 @@ fn issue_11868() {
     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() {
+    mod outer {
+        #[derive(Default)]
+        pub struct Inner {
+            _i: usize,
+        }
+    }
+
+    fn foo() {
+        let _b = Box::<outer::Inner>::default();
+        let _b = Box::<std::collections::HashSet::<i32>>::default();
+    }
+}
diff --git a/tests/ui/box_default.rs b/tests/ui/box_default.rs
index 58b91270747..eecba7464ec 100644
--- a/tests/ui/box_default.rs
+++ b/tests/ui/box_default.rs
@@ -104,3 +104,19 @@ fn issue_11868() {
     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() {
+    mod outer {
+        #[derive(Default)]
+        pub struct Inner {
+            _i: usize,
+        }
+    }
+
+    fn foo() {
+        let _b = Box::new(outer::Inner::default());
+        let _b = Box::new(std::collections::HashSet::<i32>::new());
+    }
+}
diff --git a/tests/ui/box_default.stderr b/tests/ui/box_default.stderr
index f727b377319..8bb5917a627 100644
--- a/tests/ui/box_default.stderr
+++ b/tests/ui/box_default.stderr
@@ -17,7 +17,7 @@ error: `Box::new(_)` of default value
   --> tests/ui/box_default.rs:24:16
    |
 LL |     let _vec = Box::new(Vec::<u8>::new());
-   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<Vec<u8>>::default()`
+   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<Vec::<u8>>::default()`
 
 error: `Box::new(_)` of default value
   --> tests/ui/box_default.rs:25:17
@@ -97,5 +97,17 @@ error: `Box::new(_)` of default value
 LL |             Some(Box::new(Foo::default()))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<Foo>::default()`
 
-error: aborting due to 16 previous errors
+error: `Box::new(_)` of default value
+  --> tests/ui/box_default.rs:119:18
+   |
+LL |         let _b = Box::new(outer::Inner::default());
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<outer::Inner>::default()`
+
+error: `Box::new(_)` of default value
+  --> tests/ui/box_default.rs:120:18
+   |
+LL |         let _b = Box::new(std::collections::HashSet::<i32>::new());
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Box::<std::collections::HashSet::<i32>>::default()`
+
+error: aborting due to 18 previous errors