unit-arg - improve suggestion

4 files changed, 115 insertions, 112 deletions

diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs
index 7e9190bef5e..3f5b3a5bcd5 100644
--- a/clippy_lints/src/types.rs
+++ b/clippy_lints/src/types.rs
@@ -11,8 +11,8 @@ use rustc_hir as hir;
 use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor};
 use rustc_hir::{
     BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem,
-    ImplItemKind, Item, ItemKind, Lifetime, Local, MatchSource, MutTy, Mutability, QPath, Stmt, StmtKind, TraitFn,
-    TraitItem, TraitItemKind, TyKind, UnOp,
+    ImplItemKind, Item, ItemKind, Lifetime, Local, MatchSource, MutTy, Mutability, Node, QPath, Stmt, StmtKind,
+    TraitFn, TraitItem, TraitItemKind, TyKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
@@ -29,10 +29,10 @@ use rustc_typeck::hir_ty_to_ty;
 use crate::consts::{constant, Constant};
 use crate::utils::paths;
 use crate::utils::{
-    clip, comparisons, differing_macro_contexts, higher, in_constant, indent_of, int_bits, is_type_diagnostic_item,
+    clip, comparisons, differing_macro_contexts, higher, in_constant, int_bits, is_type_diagnostic_item,
     last_path_segment, match_def_path, match_path, method_chain_args, multispan_sugg, numeric_literal::NumericLiteral,
-    qpath_res, sext, snippet, snippet_block_with_applicability, snippet_opt, snippet_with_applicability,
-    snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, unsext,
+    qpath_res, sext, snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
+    span_lint_and_help, span_lint_and_sugg, span_lint_and_then, unsext,
 };
 
 declare_clippy_lint! {
@@ -844,43 +844,54 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp
                         Applicability::MaybeIncorrect,
                     );
                     or = "or ";
+                    applicability = Applicability::MaybeIncorrect;
                 });
-            let sugg = args_to_recover
+
+            let arg_snippets: Vec<String> = args_to_recover
+                .iter()
+                .filter_map(|arg| snippet_opt(cx, arg.span))
+                .collect();
+            let arg_snippets_without_empty_blocks: Vec<String> = args_to_recover
                 .iter()
                 .filter(|arg| !is_empty_block(arg))
-                .enumerate()
-                .map(|(i, arg)| {
-                    let indent = if i == 0 {
-                        0
-                    } else {
-                        indent_of(cx, expr.span).unwrap_or(0)
-                    };
-                    format!(
-                        "{}{};",
-                        " ".repeat(indent),
-                        snippet_block_with_applicability(cx, arg.span, "..", Some(expr.span), &mut applicability)
-                    )
-                })
-                .collect::<Vec<String>>();
-            let mut and = "";
-            if !sugg.is_empty() {
-                let plural = if sugg.len() > 1 { "s" } else { "" };
-                db.span_suggestion(
-                    expr.span.with_hi(expr.span.lo()),
-                    &format!("{}move the expression{} in front of the call...", or, plural),
-                    format!("{}\n", sugg.join("\n")),
-                    applicability,
-                );
-                and = "...and "
+                .filter_map(|arg| snippet_opt(cx, arg.span))
+                .collect();
+
+            if let Some(mut sugg) = snippet_opt(cx, expr.span) {
+                arg_snippets.iter().for_each(|arg| {
+                    sugg = sugg.replacen(arg, "()", 1);
+                });
+                sugg = format!("{}{}{}", arg_snippets_without_empty_blocks.join("; "), "; ", sugg);
+                let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(expr.hir_id));
+                if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) {
+                    // expr is not in a block statement or result expression position, wrap in a block
+                    sugg = format!("{{ {} }}", sugg);
+                }
+
+                if arg_snippets_without_empty_blocks.is_empty() {
+                    db.multipart_suggestion(
+                        &format!("use {}unit literal{} instead", singular, plural),
+                        args_to_recover
+                            .iter()
+                            .map(|arg| (arg.span, "()".to_string()))
+                            .collect::<Vec<_>>(),
+                        applicability,
+                    );
+                } else {
+                    let plural = arg_snippets_without_empty_blocks.len() > 1;
+                    let empty_or_s = if plural { "s" } else { "" };
+                    let it_or_them = if plural { "them" } else { "it" };
+                    db.span_suggestion(
+                        expr.span,
+                        &format!(
+                            "{}move the expression{} in front of the call and replace {} with the unit literal `()`",
+                            or, empty_or_s, it_or_them
+                        ),
+                        sugg,
+                        applicability,
+                    );
+                }
             }
-            db.multipart_suggestion(
-                &format!("{}use {}unit literal{} instead", and, singular, plural),
-                args_to_recover
-                    .iter()
-                    .map(|arg| (arg.span, "()".to_string()))
-                    .collect::<Vec<_>>(),
-                applicability,
-            );
         },
     );
 }
@@ -2055,6 +2066,7 @@ impl PartialOrd for FullInt {
         })
     }
 }
+
 impl Ord for FullInt {
     #[must_use]
     fn cmp(&self, other: &Self) -> Ordering {
diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs
index 2992abae775..2e2bd054e42 100644
--- a/tests/ui/unit_arg.rs
+++ b/tests/ui/unit_arg.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::unit_arg)]
-#![allow(clippy::no_effect, unused_must_use, unused_variables)]
+#![allow(clippy::no_effect, unused_must_use, unused_variables, clippy::unused_unit)]
 
 use std::fmt::Debug;
 
@@ -47,6 +47,11 @@ fn bad() {
             foo(3);
         },
     );
+    // here Some(foo(2)) isn't the top level statement expression, wrap the suggestion in a block
+    None.or(Some(foo(2)));
+    // in this case, the suggestion can be inlined, no need for a surrounding block
+    // foo(()); foo(()) instead of { foo(()); foo(()) }
+    foo(foo(()))
 }
 
 fn ok() {
diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr
index 56f6a855dfa..2a0cc1f18e2 100644
--- a/tests/ui/unit_arg.stderr
+++ b/tests/ui/unit_arg.stderr
@@ -11,16 +11,12 @@ help: remove the semicolon from the last statement in the block
    |
 LL |         1
    |
-help: or move the expression in front of the call...
+help: or move the expression in front of the call and replace it with the unit literal `()`
    |
 LL |     {
 LL |         1;
-LL |     };
+LL |     }; foo(());
    |
-help: ...and use a unit literal instead
-   |
-LL |     foo(());
-   |         ^^
 
 error: passing a unit value to a function
   --> $DIR/unit_arg.rs:26:5
@@ -28,14 +24,10 @@ error: passing a unit value to a function
 LL |     foo(foo(1));
    |     ^^^^^^^^^^^
    |
-help: move the expression in front of the call...
-   |
-LL |     foo(1);
+help: move the expression in front of the call and replace it with the unit literal `()`
    |
-help: ...and use a unit literal instead
-   |
-LL |     foo(());
-   |         ^^
+LL |     foo(1); foo(());
+   |     ^^^^^^^^^^^^^^^
 
 error: passing a unit value to a function
   --> $DIR/unit_arg.rs:27:5
@@ -50,17 +42,13 @@ help: remove the semicolon from the last statement in the block
    |
 LL |         foo(2)
    |
-help: or move the expression in front of the call...
+help: or move the expression in front of the call and replace it with the unit literal `()`
    |
 LL |     {
 LL |         foo(1);
 LL |         foo(2);
-LL |     };
-   |
-help: ...and use a unit literal instead
+LL |     }; foo(());
    |
-LL |     foo(());
-   |         ^^
 
 error: passing a unit value to a function
   --> $DIR/unit_arg.rs:32:5
@@ -74,16 +62,12 @@ help: remove the semicolon from the last statement in the block
    |
 LL |         1
    |
-help: or move the expression in front of the call...
+help: or move the expression in front of the call and replace it with the unit literal `()`
    |
 LL |     {
 LL |         1;
-LL |     };
+LL |     }; b.bar(());
    |
-help: ...and use a unit literal instead
-   |
-LL |     b.bar(());
-   |           ^^
 
 error: passing unit values to a function
   --> $DIR/unit_arg.rs:35:5
@@ -91,15 +75,10 @@ error: passing unit values to a function
 LL |     taking_multiple_units(foo(0), foo(1));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-help: move the expressions in front of the call...
-   |
-LL |     foo(0);
-LL |     foo(1);
-   |
-help: ...and use unit literals instead
+help: move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     taking_multiple_units((), ());
-   |                           ^^  ^^
+LL |     foo(0); foo(1); taking_multiple_units((), ());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: passing unit values to a function
   --> $DIR/unit_arg.rs:36:5
@@ -114,18 +93,13 @@ help: remove the semicolon from the last statement in the block
    |
 LL |         foo(2)
    |
-help: or move the expressions in front of the call...
+help: or move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     foo(0);
-LL |     {
+LL |     foo(0); {
 LL |         foo(1);
 LL |         foo(2);
-LL |     };
-   |
-help: ...and use unit literals instead
+LL |     }; taking_multiple_units((), ());
    |
-LL |     taking_multiple_units((), ());
-   |                           ^^  ^^
 
 error: passing unit values to a function
   --> $DIR/unit_arg.rs:40:5
@@ -147,35 +121,56 @@ help: remove the semicolon from the last statement in the block
    |
 LL |             foo(3)
    |
-help: or move the expressions in front of the call...
+help: or move the expressions in front of the call and replace them with the unit literal `()`
    |
 LL |     {
-LL |         foo(0);
-LL |         foo(1);
-LL |     };
-LL |     {
-LL |         foo(2);
+LL |             foo(0);
+LL |             foo(1);
+LL |         }; {
+LL |             foo(2);
+LL |             foo(3);
  ...
-help: ...and use unit literals instead
+
+error: use of `or` followed by a function call
+  --> $DIR/unit_arg.rs:51:10
    |
-LL |         (),
-LL |         (),
+LL |     None.or(Some(foo(2)));
+   |          ^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(foo(2)))`
    |
+   = note: `-D clippy::or-fun-call` implied by `-D warnings`
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:82:5
+  --> $DIR/unit_arg.rs:51:13
    |
-LL |     Some(foo(1))
+LL |     None.or(Some(foo(2)));
+   |             ^^^^^^^^^^^^
+   |
+help: move the expression in front of the call and replace it with the unit literal `()`
+   |
+LL |     None.or({ foo(2); Some(()) });
+   |             ^^^^^^^^^^^^^^^^^^^^
+
+error: passing a unit value to a function
+  --> $DIR/unit_arg.rs:54:5
+   |
+LL |     foo(foo(()))
    |     ^^^^^^^^^^^^
    |
-help: move the expression in front of the call...
+help: move the expression in front of the call and replace it with the unit literal `()`
+   |
+LL |     foo(()); foo(())
+   |
+
+error: passing a unit value to a function
+  --> $DIR/unit_arg.rs:87:5
+   |
+LL |     Some(foo(1))
+   |     ^^^^^^^^^^^^
    |
-LL |     foo(1);
+help: move the expression in front of the call and replace it with the unit literal `()`
    |
-help: ...and use a unit literal instead
+LL |     foo(1); Some(())
    |
-LL |     Some(())
-   |          ^^
 
-error: aborting due to 8 previous errors
+error: aborting due to 11 previous errors
 
diff --git a/tests/ui/unit_arg_empty_blocks.stderr b/tests/ui/unit_arg_empty_blocks.stderr
index bb58483584b..4cbbc8b8cd4 100644
--- a/tests/ui/unit_arg_empty_blocks.stderr
+++ b/tests/ui/unit_arg_empty_blocks.stderr
@@ -22,14 +22,10 @@ error: passing unit values to a function
 LL |     taking_two_units({}, foo(0));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-help: move the expression in front of the call...
+help: move the expression in front of the call and replace it with the unit literal `()`
    |
-LL |     foo(0);
-   |
-help: ...and use unit literals instead
-   |
-LL |     taking_two_units((), ());
-   |                      ^^  ^^
+LL |     foo(0); taking_two_units((), ());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: passing unit values to a function
   --> $DIR/unit_arg_empty_blocks.rs:18:5
@@ -37,15 +33,10 @@ error: passing unit values to a function
 LL |     taking_three_units({}, foo(0), foo(1));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-help: move the expressions in front of the call...
-   |
-LL |     foo(0);
-LL |     foo(1);
-   |
-help: ...and use unit literals instead
+help: move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     taking_three_units((), (), ());
-   |                        ^^  ^^  ^^
+LL |     foo(0); foo(1); taking_three_units((), (), ());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: aborting due to 4 previous errors