unit-arg - pr remarks

Author: tnielens

clippy_lints/src/types.rs

@@ -31,8 +31,8 @@ use crate::utils::paths;
 use crate::utils::{
     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, trim_multiline, unsext,
+    qpath_res, reindent_multiline, 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! {
@@ -802,7 +802,45 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg {
     }
 }
 
-#[allow(clippy::too_many_lines)]
+fn fmt_stmts_and_call(
+    cx: &LateContext<'_>,
+    call_expr: &Expr<'_>,
+    call_snippet: &str,
+    args_snippets: &[impl AsRef<str>],
+    non_empty_block_args_snippets: &[impl AsRef<str>],
+) -> String {
+    let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0);
+    let call_snippet_with_replacements = args_snippets
+        .iter()
+        .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1));
+
+    let mut stmts_and_call = non_empty_block_args_snippets
+        .iter()
+        .map(|it| it.as_ref().to_owned())
+        .collect::<Vec<_>>();
+    stmts_and_call.push(call_snippet_with_replacements);
+    stmts_and_call = stmts_and_call
+        .into_iter()
+        .map(|v| reindent_multiline(v.into(), true, Some(call_expr_indent)).into_owned())
+        .collect();
+
+    let mut stmts_and_call_snippet = stmts_and_call.join(&format!("{}{}", ";\n", " ".repeat(call_expr_indent)));
+    // 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(call_expr.hir_id));
+    if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) {
+        let block_indent = call_expr_indent + 4;
+        stmts_and_call_snippet =
+            reindent_multiline(stmts_and_call_snippet.into(), true, Some(block_indent)).into_owned();
+        stmts_and_call_snippet = format!(
+            "{{\n{}{}\n{}}}",
+            " ".repeat(block_indent),
+            &stmts_and_call_snippet,
+            " ".repeat(call_expr_indent)
+        );
+    }
+    stmts_and_call_snippet
+}
+
 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 {
@@ -857,37 +895,15 @@ 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(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));
-                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 let Some(call_snippet) = snippet_opt(cx, expr.span) {
+                let sugg = fmt_stmts_and_call(
+                    cx,
+                    expr,
+                    &call_snippet,
+                    &arg_snippets,
+                    &arg_snippets_without_empty_blocks,
+                );
 
                 if arg_snippets_without_empty_blocks.is_empty() {
                     db.multipart_suggestion(

clippy_lints/src/utils/mod.rs

@@ -19,6 +19,7 @@ pub mod paths;
 pub mod ptr;
 pub mod sugg;
 pub mod usage;
+
 pub use self::attrs::*;
 pub use self::diagnostics::*;
 pub use self::hir_utils::{both, eq_expr_value, over, SpanlessEq, SpanlessHash};
@@ -108,6 +109,7 @@ pub fn in_macro(span: Span) -> bool {
         false
     }
 }
+
 // If the snippet is empty, it's an attribute that was inserted during macro
 // expansion and we want to ignore those, because they could come from external
 // sources that the user has no control over.
@@ -571,7 +573,7 @@ pub fn snippet_block<'a, T: LintContext>(
 ) -> Cow<'a, str> {
     let snip = snippet(cx, span, default);
     let indent = indent_relative_to.and_then(|s| indent_of(cx, s));
-    trim_multiline(snip, true, indent)
+    reindent_multiline(snip, true, indent)
 }
 
 /// Same as `snippet_block`, but adapts the applicability level by the rules of
@@ -585,7 +587,7 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>(
 ) -> Cow<'a, str> {
     let snip = snippet_with_applicability(cx, span, default, applicability);
     let indent = indent_relative_to.and_then(|s| indent_of(cx, s));
-    trim_multiline(snip, true, indent)
+    reindent_multiline(snip, true, indent)
 }
 
 /// Returns a new Span that extends the original Span to the first non-whitespace char of the first
@@ -661,16 +663,16 @@ pub fn expr_block<'a, T: LintContext>(
     }
 }
 
-/// Trim indentation from a multiline string with possibility of ignoring the
-/// first line.
-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, ' ')
+/// Reindent a multiline string with possibility of ignoring the first line.
+#[allow(clippy::needless_pass_by_value)]
+pub fn reindent_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option<usize>) -> Cow<'_, str> {
+    let s_space = reindent_multiline_inner(&s, ignore_first, indent, ' ');
+    let s_tab = reindent_multiline_inner(&s_space, ignore_first, indent, '\t');
+    reindent_multiline_inner(&s_tab, ignore_first, indent, ' ').into()
 }
 
-fn trim_multiline_inner(s: Cow<'_, str>, ignore_first: bool, indent: Option<usize>, ch: char) -> Cow<'_, str> {
-    let mut x = s
+fn reindent_multiline_inner(s: &str, ignore_first: bool, indent: Option<usize>, ch: char) -> String {
+    let x = s
         .lines()
         .skip(ignore_first as usize)
         .filter_map(|l| {
@@ -683,26 +685,20 @@ fn trim_multiline_inner(s: Cow<'_, str>, ignore_first: bool, indent: Option<usiz
         })
         .min()
         .unwrap_or(0);
-    if let Some(indent) = indent {
-        x = x.saturating_sub(indent);
-    }
-    if x > 0 {
-        Cow::Owned(
-            s.lines()
-                .enumerate()
-                .map(|(i, l)| {
-                    if (ignore_first && i == 0) || l.is_empty() {
-                        l
-                    } else {
-                        l.split_at(x).1
-                    }
-                })
-                .collect::<Vec<_>>()
-                .join("\n"),
-        )
-    } else {
-        s
-    }
+    let indent = indent.unwrap_or(0);
+    s.lines()
+        .enumerate()
+        .map(|(i, l)| {
+            if (ignore_first && i == 0) || l.is_empty() {
+                l.to_owned()
+            } else if x > indent {
+                l.split_at(x - indent).1.to_owned()
+            } else {
+                " ".repeat(indent - x) + l
+            }
+        })
+        .collect::<Vec<String>>()
+        .join("\n")
 }
 
 /// Gets the parent expression, if any –- this is useful to constrain a lint.
@@ -1475,26 +1471,26 @@ macro_rules! unwrap_cargo_metadata {
 
 #[cfg(test)]
 mod test {
-    use super::{trim_multiline, without_block_comments};
+    use super::{reindent_multiline, without_block_comments};
 
     #[test]
-    fn test_trim_multiline_single_line() {
-        assert_eq!("", trim_multiline("".into(), false, None));
-        assert_eq!("...", trim_multiline("...".into(), false, None));
-        assert_eq!("...", trim_multiline("    ...".into(), false, None));
-        assert_eq!("...", trim_multiline("\t...".into(), false, None));
-        assert_eq!("...", trim_multiline("\t\t...".into(), false, None));
+    fn test_reindent_multiline_single_line() {
+        assert_eq!("", reindent_multiline("".into(), false, None));
+        assert_eq!("...", reindent_multiline("...".into(), false, None));
+        assert_eq!("...", reindent_multiline("    ...".into(), false, None));
+        assert_eq!("...", reindent_multiline("\t...".into(), false, None));
+        assert_eq!("...", reindent_multiline("\t\t...".into(), false, None));
     }
 
     #[test]
     #[rustfmt::skip]
-    fn test_trim_multiline_block() {
+    fn test_reindent_multiline_block() {
         assert_eq!("\
     if x {
         y
     } else {
         z
-    }", trim_multiline("    if x {
+    }", reindent_multiline("    if x {
             y
         } else {
             z
@@ -1504,7 +1500,7 @@ mod test {
     \ty
     } else {
     \tz
-    }", trim_multiline("    if x {
+    }", reindent_multiline("    if x {
         \ty
         } else {
         \tz
@@ -1513,21 +1509,37 @@ mod test {
 
     #[test]
     #[rustfmt::skip]
-    fn test_trim_multiline_empty_line() {
+    fn test_reindent_multiline_empty_line() {
         assert_eq!("\
     if x {
         y
 
     } else {
         z
-    }", trim_multiline("    if x {
+    }", reindent_multiline("    if x {
             y
 
         } else {
             z
         }".into(), false, None));
     }
 
+    #[test]
+    #[rustfmt::skip]
+    fn test_reindent_multiline_lines_deeper() {
+        assert_eq!("\
+        if x {
+            y
+        } else {
+            z
+        }", reindent_multiline("\
+    if x {
+        y
+    } else {
+        z
+    }".into(), true, Some(8)));
+    }
+
     #[test]
     fn test_without_block_comments_lines_without_block_comments() {
         let result = without_block_comments(vec!["/*", "", "*/"]);