unit-arg - pr comments

tnielens · tnielens · commit f3ccbef2af24 · 2020-08-27T19:36:28.000+02:00
diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs
@@ -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, 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,
+    span_lint_and_help, span_lint_and_sugg, span_lint_and_then, trim_multiline, unsext,
 };
 
 declare_clippy_lint! {
@@ -802,6 +802,7 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg {
     }
 }
 
+#[allow(clippy::too_many_lines)]
 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 {
@@ -856,18 +857,38 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp
                 .filter(|arg| !is_empty_block(arg))
                 .filter_map(|arg| snippet_opt(cx, arg.span))
                 .collect();
+            let indent = indent_of(cx, expr.span).unwrap_or(0);
 
-            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);
+            if let Some(expr_str) = snippet_opt(cx, expr.span) {
+                let expr_with_replacements = arg_snippets
+                    .iter()
+                    .fold(expr_str, |acc, arg| acc.replacen(arg, "()", 1));
+
+                // expr is not in a block statement or result expression position, wrap in a block
                 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);
+                let wrap_in_block =
+                    !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_)));
+
+                let stmts_indent = if wrap_in_block { indent + 4 } else { indent };
+                let mut stmts_and_call = arg_snippets_without_empty_blocks.clone();
+                stmts_and_call.push(expr_with_replacements);
+                let mut stmts_and_call_str = stmts_and_call
+                    .into_iter()
+                    .enumerate()
+                    .map(|(i, v)| {
+                        let with_indent_prefix = if i > 0 { " ".repeat(stmts_indent) + &v } else { v };
+                        trim_multiline(with_indent_prefix.into(), true, Some(stmts_indent)).into_owned()
+                    })
+                    .collect::<Vec<String>>()
+                    .join(";\n");
+
+                if wrap_in_block {
+                    stmts_and_call_str = " ".repeat(stmts_indent) + &stmts_and_call_str;
+                    stmts_and_call_str = format!("{{\n{}\n{}}}", &stmts_and_call_str, " ".repeat(indent));
                 }
 
+                let sugg = stmts_and_call_str;
+
                 if arg_snippets_without_empty_blocks.is_empty() {
                     db.multipart_suggestion(
                         &format!("use {}unit literal{} instead", singular, plural),
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -662,7 +662,7 @@ pub fn expr_block<'a, T: LintContext>(
 
 /// Trim indentation from a multiline string with possibility of ignoring the
 /// first line.
-fn trim_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option<usize>) -> Cow<'_, str> {
+pub fn trim_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option<usize>) -> Cow<'_, str> {
     let s_space = trim_multiline_inner(s, ignore_first, indent, ' ');
     let s_tab = trim_multiline_inner(s_space, ignore_first, indent, '\t');
     trim_multiline_inner(s_tab, ignore_first, indent, ' ')
diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs
@@ -1,5 +1,11 @@
 #![warn(clippy::unit_arg)]
-#![allow(clippy::no_effect, unused_must_use, unused_variables, clippy::unused_unit)]
+#![allow(
+    clippy::no_effect,
+    unused_must_use,
+    unused_variables,
+    clippy::unused_unit,
+    clippy::or_fun_call
+)]
 
 use std::fmt::Debug;
 
diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr
@@ -1,5 +1,5 @@
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:23:5
+  --> $DIR/unit_arg.rs:29:5
    |
 LL | /     foo({
 LL | |         1;
@@ -15,22 +15,24 @@ help: or move the expression in front of the call and replace it with the unit l
    |
 LL |     {
 LL |         1;
-LL |     }; foo(());
+LL |     };
+LL |     foo(());
    |
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:26:5
+  --> $DIR/unit_arg.rs:32:5
    |
 LL |     foo(foo(1));
    |     ^^^^^^^^^^^
    |
 help: move the expression in front of the call and replace it with the unit literal `()`
    |
-LL |     foo(1); foo(());
-   |     ^^^^^^^^^^^^^^^
+LL |     foo(1);
+LL |     foo(());
+   |
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:27:5
+  --> $DIR/unit_arg.rs:33:5
    |
 LL | /     foo({
 LL | |         foo(1);
@@ -47,11 +49,12 @@ help: or move the expression in front of the call and replace it with the unit l
 LL |     {
 LL |         foo(1);
 LL |         foo(2);
-LL |     }; foo(());
+LL |     };
+LL |     foo(());
    |
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:32:5
+  --> $DIR/unit_arg.rs:38:5
    |
 LL | /     b.bar({
 LL | |         1;
@@ -66,22 +69,25 @@ help: or move the expression in front of the call and replace it with the unit l
    |
 LL |     {
 LL |         1;
-LL |     }; b.bar(());
+LL |     };
+LL |     b.bar(());
    |
 
 error: passing unit values to a function
-  --> $DIR/unit_arg.rs:35:5
+  --> $DIR/unit_arg.rs:41:5
    |
 LL |     taking_multiple_units(foo(0), foo(1));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 help: move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     foo(0); foo(1); taking_multiple_units((), ());
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |     foo(0);
+LL |     foo(1);
+LL |     taking_multiple_units((), ());
+   |
 
 error: passing unit values to a function
-  --> $DIR/unit_arg.rs:36:5
+  --> $DIR/unit_arg.rs:42:5
    |
 LL | /     taking_multiple_units(foo(0), {
 LL | |         foo(1);
@@ -95,14 +101,16 @@ LL |         foo(2)
    |
 help: or move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     foo(0); {
+LL |     foo(0);
+LL |     {
 LL |         foo(1);
 LL |         foo(2);
-LL |     }; taking_multiple_units((), ());
+LL |     };
+LL |     taking_multiple_units((), ());
    |
 
 error: passing unit values to a function
-  --> $DIR/unit_arg.rs:40:5
+  --> $DIR/unit_arg.rs:46:5
    |
 LL | /     taking_multiple_units(
 LL | |         {
@@ -124,53 +132,50 @@ LL |             foo(3)
 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 |             foo(2);
-LL |             foo(3);
+LL |         foo(0);
+LL |         foo(1);
+LL |     };
+LL |     {
+LL |         foo(2);
  ...
 
-error: use of `or` followed by a function call
-  --> $DIR/unit_arg.rs:51:10
-   |
-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:51:13
+  --> $DIR/unit_arg.rs:57:13
    |
 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(()) });
-   |             ^^^^^^^^^^^^^^^^^^^^
+LL |     None.or({
+LL |         foo(2);
+LL |         Some(())
+LL |     });
+   |
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:54:5
+  --> $DIR/unit_arg.rs:60:5
    |
 LL |     foo(foo(()))
    |     ^^^^^^^^^^^^
    |
 help: move the expression in front of the call and replace it with the unit literal `()`
    |
-LL |     foo(()); foo(())
+LL |     foo(());
+LL |     foo(())
    |
 
 error: passing a unit value to a function
-  --> $DIR/unit_arg.rs:87:5
+  --> $DIR/unit_arg.rs:93:5
    |
 LL |     Some(foo(1))
    |     ^^^^^^^^^^^^
    |
 help: move the expression in front of the call and replace it with the unit literal `()`
    |
-LL |     foo(1); Some(())
+LL |     foo(1);
+LL |     Some(())
    |
 
-error: aborting due to 11 previous errors
+error: aborting due to 10 previous errors
 
diff --git a/tests/ui/unit_arg_empty_blocks.stderr b/tests/ui/unit_arg_empty_blocks.stderr
@@ -24,8 +24,9 @@ LL |     taking_two_units({}, foo(0));
    |
 help: move the expression in front of the call and replace it with the unit literal `()`
    |
-LL |     foo(0); taking_two_units((), ());
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |     foo(0);
+LL |     taking_two_units((), ());
+   |
 
 error: passing unit values to a function
   --> $DIR/unit_arg_empty_blocks.rs:18:5
@@ -35,8 +36,10 @@ LL |     taking_three_units({}, foo(0), foo(1));
    |
 help: move the expressions in front of the call and replace them with the unit literal `()`
    |
-LL |     foo(0); foo(1); taking_three_units((), (), ());
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |     foo(0);
+LL |     foo(1);
+LL |     taking_three_units((), (), ());
+   |
 
 error: aborting due to 4 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`error: passing a unit value to a function`
`2`		`- --> $DIR/unit_arg.rs:23:5`
	`2`	`+ --> $DIR/unit_arg.rs:29:5`
`3`	`3`	`\|`
`4`	`4`	`LL \| / foo({`
`5`	`5`	`LL \| \| 1;`
`@@ -15,22 +15,24 @@ help: or move the expression in front of the call and replace it with the unit l`
`15`	`15`	`\|`
`16`	`16`	`LL \| {`
`17`	`17`	`LL \| 1;`
`18`		`-LL \| }; foo(());`
	`18`	`+LL \| };`
	`19`	`+LL \| foo(());`
`19`	`20`	`\|`
`20`	`21`
`21`	`22`	`error: passing a unit value to a function`
`22`		`- --> $DIR/unit_arg.rs:26:5`
	`23`	`+ --> $DIR/unit_arg.rs:32:5`
`23`	`24`	`\|`
`24`	`25`	`LL \| foo(foo(1));`
`25`	`26`	`\| ^^^^^^^^^^^`
`26`	`27`	`\|`
`27`	`28`	help: move the expression in front of the call and replace it with the unit literal `()`
`28`	`29`	`\|`
`29`		`-LL \| foo(1); foo(());`
`30`		`- \| ^^^^^^^^^^^^^^^`
	`30`	`+LL \| foo(1);`
	`31`	`+LL \| foo(());`
	`32`	`+ \|`
`31`	`33`
`32`	`34`	`error: passing a unit value to a function`
`33`		`- --> $DIR/unit_arg.rs:27:5`
	`35`	`+ --> $DIR/unit_arg.rs:33:5`
`34`	`36`	`\|`
`35`	`37`	`LL \| / foo({`
`36`	`38`	`LL \| \| foo(1);`
`@@ -47,11 +49,12 @@ help: or move the expression in front of the call and replace it with the unit l`
`47`	`49`	`LL \| {`
`48`	`50`	`LL \| foo(1);`
`49`	`51`	`LL \| foo(2);`
`50`		`-LL \| }; foo(());`
	`52`	`+LL \| };`
	`53`	`+LL \| foo(());`
`51`	`54`	`\|`
`52`	`55`
`53`	`56`	`error: passing a unit value to a function`
`54`		`- --> $DIR/unit_arg.rs:32:5`
	`57`	`+ --> $DIR/unit_arg.rs:38:5`
`55`	`58`	`\|`
`56`	`59`	`LL \| / b.bar({`
`57`	`60`	`LL \| \| 1;`
`@@ -66,22 +69,25 @@ help: or move the expression in front of the call and replace it with the unit l`
`66`	`69`	`\|`
`67`	`70`	`LL \| {`
`68`	`71`	`LL \| 1;`
`69`		`-LL \| }; b.bar(());`
	`72`	`+LL \| };`
	`73`	`+LL \| b.bar(());`
`70`	`74`	`\|`
`71`	`75`
`72`	`76`	`error: passing unit values to a function`
`73`		`- --> $DIR/unit_arg.rs:35:5`
	`77`	`+ --> $DIR/unit_arg.rs:41:5`
`74`	`78`	`\|`
`75`	`79`	`LL \| taking_multiple_units(foo(0), foo(1));`
`76`	`80`	`\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^`
`77`	`81`	`\|`
`78`	`82`	help: move the expressions in front of the call and replace them with the unit literal `()`
`79`	`83`	`\|`
`80`		`-LL \| foo(0); foo(1); taking_multiple_units((), ());`
`81`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^`
	`84`	`+LL \| foo(0);`
	`85`	`+LL \| foo(1);`
	`86`	`+LL \| taking_multiple_units((), ());`
	`87`	`+ \|`
`82`	`88`
`83`	`89`	`error: passing unit values to a function`
`84`		`- --> $DIR/unit_arg.rs:36:5`
	`90`	`+ --> $DIR/unit_arg.rs:42:5`
`85`	`91`	`\|`
`86`	`92`	`LL \| / taking_multiple_units(foo(0), {`
`87`	`93`	`LL \| \| foo(1);`
`@@ -95,14 +101,16 @@ LL \| foo(2)`
`95`	`101`	`\|`
`96`	`102`	help: or move the expressions in front of the call and replace them with the unit literal `()`
`97`	`103`	`\|`
`98`		`-LL \| foo(0); {`
	`104`	`+LL \| foo(0);`
	`105`	`+LL \| {`
`99`	`106`	`LL \| foo(1);`
`100`	`107`	`LL \| foo(2);`
`101`		`-LL \| }; taking_multiple_units((), ());`
	`108`	`+LL \| };`
	`109`	`+LL \| taking_multiple_units((), ());`
`102`	`110`	`\|`
`103`	`111`
`104`	`112`	`error: passing unit values to a function`
`105`		`- --> $DIR/unit_arg.rs:40:5`
	`113`	`+ --> $DIR/unit_arg.rs:46:5`
`106`	`114`	`\|`
`107`	`115`	`LL \| / taking_multiple_units(`
`108`	`116`	`LL \| \| {`
`@@ -124,53 +132,50 @@ LL \| foo(3)`
`124`	`132`	help: or move the expressions in front of the call and replace them with the unit literal `()`
`125`	`133`	`\|`
`126`	`134`	`LL \| {`
`127`		`-LL \| foo(0);`
`128`		`-LL \| foo(1);`
`129`		`-LL \| }; {`
`130`		`-LL \| foo(2);`
`131`		`-LL \| foo(3);`
	`135`	`+LL \| foo(0);`
	`136`	`+LL \| foo(1);`
	`137`	`+LL \| };`
	`138`	`+LL \| {`
	`139`	`+LL \| foo(2);`
`132`	`140`	`...`
`133`	`141`
`134`		-error: use of `or` followed by a function call
`135`		`- --> $DIR/unit_arg.rs:51:10`
`136`		`- \|`
`137`		`-LL \| None.or(Some(foo(2)));`
`138`		- \| ^^^^^^^^^^^^^^^^ help: try this: `or_else(\|\| Some(foo(2)))`
`139`		`- \|`
`140`		- = note: `-D clippy::or-fun-call` implied by `-D warnings`
`141`		`-`
`142`	`142`	`error: passing a unit value to a function`
`143`		`- --> $DIR/unit_arg.rs:51:13`
	`143`	`+ --> $DIR/unit_arg.rs:57:13`
`144`	`144`	`\|`
`145`	`145`	`LL \| None.or(Some(foo(2)));`
`146`	`146`	`\| ^^^^^^^^^^^^`
`147`	`147`	`\|`
`148`	`148`	help: move the expression in front of the call and replace it with the unit literal `()`
`149`	`149`	`\|`
`150`		`-LL \| None.or({ foo(2); Some(()) });`
`151`		`- \| ^^^^^^^^^^^^^^^^^^^^`
	`150`	`+LL \| None.or({`
	`151`	`+LL \| foo(2);`
	`152`	`+LL \| Some(())`
	`153`	`+LL \| });`
	`154`	`+ \|`
`152`	`155`
`153`	`156`	`error: passing a unit value to a function`
`154`		`- --> $DIR/unit_arg.rs:54:5`
	`157`	`+ --> $DIR/unit_arg.rs:60:5`
`155`	`158`	`\|`
`156`	`159`	`LL \| foo(foo(()))`
`157`	`160`	`\| ^^^^^^^^^^^^`
`158`	`161`	`\|`
`159`	`162`	help: move the expression in front of the call and replace it with the unit literal `()`
`160`	`163`	`\|`
`161`		`-LL \| foo(()); foo(())`
	`164`	`+LL \| foo(());`
	`165`	`+LL \| foo(())`
`162`	`166`	`\|`
`163`	`167`
`164`	`168`	`error: passing a unit value to a function`
`165`		`- --> $DIR/unit_arg.rs:87:5`
	`169`	`+ --> $DIR/unit_arg.rs:93:5`
`166`	`170`	`\|`
`167`	`171`	`LL \| Some(foo(1))`
`168`	`172`	`\| ^^^^^^^^^^^^`
`169`	`173`	`\|`
`170`	`174`	help: move the expression in front of the call and replace it with the unit literal `()`
`171`	`175`	`\|`
`172`		`-LL \| foo(1); Some(())`
	`176`	`+LL \| foo(1);`
	`177`	`+LL \| Some(())`
`173`	`178`	`\|`
`174`	`179`
`175`		`-error: aborting due to 11 previous errors`
	`180`	`+error: aborting due to 10 previous errors`
`176`	`181`
Original file line number	Diff line number	Diff line change
`@@ -24,8 +24,9 @@ LL \| taking_two_units({}, foo(0));`
`24`	`24`	`\|`
`25`	`25`	help: move the expression in front of the call and replace it with the unit literal `()`
`26`	`26`	`\|`
`27`		`-LL \| foo(0); taking_two_units((), ());`
`28`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^`
	`27`	`+LL \| foo(0);`
	`28`	`+LL \| taking_two_units((), ());`
	`29`	`+ \|`
`29`	`30`
`30`	`31`	`error: passing unit values to a function`
`31`	`32`	`--> $DIR/unit_arg_empty_blocks.rs:18:5`
`@@ -35,8 +36,10 @@ LL \| taking_three_units({}, foo(0), foo(1));`
`35`	`36`	`\|`
`36`	`37`	help: move the expressions in front of the call and replace them with the unit literal `()`
`37`	`38`	`\|`
`38`		`-LL \| foo(0); foo(1); taking_three_units((), (), ());`
`39`		`- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^`
	`39`	`+LL \| foo(0);`
	`40`	`+LL \| foo(1);`
	`41`	`+LL \| taking_three_units((), (), ());`
	`42`	`+ \|`
`40`	`43`
`41`	`44`	`error: aborting due to 4 previous errors`
`42`	`45`