Auto merge of rust-lang#4455 - flip1995:unit_arg_appl, r=phansch

Rework suggestion generation of `unit_arg` lint

Found this bug while running `cargo fix --clippy` on quite a big codebase:

This would replace something like:
```rust
Some(fn_that_actually_does_something(&a, b))
```
with
```rust
Some(())
```
which obviously suppresses side effects.

Since pretty much every expression could have side effects, I think making this suggestion `MaybeIncorrect` is the best thing to do here.

A correct suggestion would be:

```rust
fn_that_actually_does_something(&a, b);
Some(())
```

Somehow the suggestion is not correctly applied to the arguments, when more than one argument is a unit value. I have to look into this a little more, though.

changelog: Fixes suggestion of `unit_arg` lint, so that it suggests semantic equivalent code

Fixes rust-lang#4741

Author: bors

clippy_lints/src/types.rs

@@ -10,7 +10,7 @@ use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    BinOpKind, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem,
+    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,
 };
@@ -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, int_bits, is_type_diagnostic_item,
+    clip, comparisons, differing_macro_contexts, higher, in_constant, indent_of, int_bits, is_type_diagnostic_item,
     last_path_segment, match_def_path, match_path, method_chain_args, multispan_sugg, numeric_literal::NumericLiteral,
-    qpath_res, same_tys, 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,
+    qpath_res, same_tys, 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,
 };
 
 declare_clippy_lint! {
@@ -779,31 +779,124 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitArg {
 
         match expr.kind {
             ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args) => {
-                for arg in args {
-                    if is_unit(cx.tables.expr_ty(arg)) && !is_unit_literal(arg) {
-                        if let ExprKind::Match(.., match_source) = &arg.kind {
-                            if *match_source == MatchSource::TryDesugar {
-                                continue;
+                let args_to_recover = args
+                    .iter()
+                    .filter(|arg| {
+                        if is_unit(cx.tables.expr_ty(arg)) && !is_unit_literal(arg) {
+                            if let ExprKind::Match(.., MatchSource::TryDesugar) = &arg.kind {
+                                false
+                            } else {
+                                true
                             }
+                        } else {
+                            false
                         }
-
-                        span_lint_and_sugg(
-                            cx,
-                            UNIT_ARG,
-                            arg.span,
-                            "passing a unit value to a function",
-                            "if you intended to pass a unit value, use a unit literal instead",
-                            "()".to_string(),
-                            Applicability::MaybeIncorrect,
-                        );
-                    }
+                    })
+                    .collect::<Vec<_>>();
+                if !args_to_recover.is_empty() {
+                    lint_unit_args(cx, expr, &args_to_recover);
                 }
             },
             _ => (),
         }
     }
 }
 
+fn lint_unit_args(cx: &LateContext<'_, '_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) {
+    let mut applicability = Applicability::MachineApplicable;
+    let (singular, plural) = if args_to_recover.len() > 1 {
+        ("", "s")
+    } else {
+        ("a ", "")
+    };
+    span_lint_and_then(
+        cx,
+        UNIT_ARG,
+        expr.span,
+        &format!("passing {}unit value{} to a function", singular, plural),
+        |db| {
+            let mut or = "";
+            args_to_recover
+                .iter()
+                .filter_map(|arg| {
+                    if_chain! {
+                        if let ExprKind::Block(block, _) = arg.kind;
+                        if block.expr.is_none();
+                        if let Some(last_stmt) = block.stmts.iter().last();
+                        if let StmtKind::Semi(last_expr) = last_stmt.kind;
+                        if let Some(snip) = snippet_opt(cx, last_expr.span);
+                        then {
+                            Some((
+                                last_stmt.span,
+                                snip,
+                            ))
+                        }
+                        else {
+                            None
+                        }
+                    }
+                })
+                .for_each(|(span, sugg)| {
+                    db.span_suggestion(
+                        span,
+                        "remove the semicolon from the last statement in the block",
+                        sugg,
+                        Applicability::MaybeIncorrect,
+                    );
+                    or = "or ";
+                });
+            let sugg = 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 "
+            }
+            db.multipart_suggestion(
+                &format!("{}use {}unit literal{} instead", and, singular, plural),
+                args_to_recover
+                    .iter()
+                    .map(|arg| (arg.span, "()".to_string()))
+                    .collect::<Vec<_>>(),
+                applicability,
+            );
+        },
+    );
+}
+
+fn is_empty_block(expr: &Expr<'_>) -> bool {
+    matches!(
+        expr.kind,
+        ExprKind::Block(
+            Block {
+                stmts: &[], expr: None, ..
+            },
+            _,
+        )
+    )
+}
+
 fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool {
     use rustc_span::hygiene::DesugaringKind;
     if let ExprKind::Call(ref callee, _) = expr.kind {

tests/ui/unit_arg.fixed

@@ -1,6 +1,5 @@
-// run-rustfix
 #![warn(clippy::unit_arg)]
-#![allow(unused_braces, clippy::no_effect, unused_must_use)]
+#![allow(clippy::no_effect, unused_must_use, unused_variables)]
 
 use std::fmt::Debug;
 
@@ -21,7 +20,6 @@ impl Bar {
 }
 
 fn bad() {
-    foo({});
     foo({
         1;
     });
@@ -30,11 +28,25 @@ fn bad() {
         foo(1);
         foo(2);
     });
-    foo3({}, 2, 2);
     let b = Bar;
     b.bar({
         1;
     });
+    taking_multiple_units(foo(0), foo(1));
+    taking_multiple_units(foo(0), {
+        foo(1);
+        foo(2);
+    });
+    taking_multiple_units(
+        {
+            foo(0);
+            foo(1);
+        },
+        {
+            foo(2);
+            foo(3);
+        },
+    );
 }
 
 fn ok() {
@@ -65,6 +77,13 @@ mod issue_2945 {
     }
 }
 
+#[allow(dead_code)]
+fn returning_expr() -> Option<()> {
+    Some(foo(1))
+}
+
+fn taking_multiple_units(a: (), b: ()) {}
+
 fn main() {
     bad();
     ok();