From 2ecc2ac864739cff6aed2609021e2467dedb117a Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Fri, 21 Aug 2020 00:07:56 +0200 Subject: [PATCH 1/3] unit-arg - improve suggestion --- clippy_lints/src/types.rs | 88 +++++++++++--------- tests/ui/unit_arg.rs | 7 +- tests/ui/unit_arg.stderr | 111 ++++++++++++-------------- tests/ui/unit_arg_empty_blocks.stderr | 21 ++--- 4 files changed, 115 insertions(+), 112 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 7e9190bef5e7..3f5b3a5bcd5d 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 = args_to_recover + .iter() + .filter_map(|arg| snippet_opt(cx, arg.span)) + .collect(); + let arg_snippets_without_empty_blocks: Vec = 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::>(); - 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::>(), + 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::>(), - 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 2992abae775b..2e2bd054e42a 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 56f6a855dfa5..2a0cc1f18e27 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 bb58483584b3..4cbbc8b8cd43 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 From f3ccbef2af24d5d83f82f1fb50bd97a9b75e609f Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Thu, 27 Aug 2020 01:40:02 +0200 Subject: [PATCH 2/3] unit-arg - pr comments --- clippy_lints/src/types.rs | 41 ++++++++++---- clippy_lints/src/utils/mod.rs | 2 +- tests/ui/unit_arg.rs | 8 ++- tests/ui/unit_arg.stderr | 79 ++++++++++++++------------- tests/ui/unit_arg_empty_blocks.stderr | 11 ++-- 5 files changed, 88 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 3f5b3a5bcd5d..16e48d919164 100644 --- 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::>() + .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 index 2aef995cec44..d20b33c4a1d1 100644 --- 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) -> Cow<'_, str> { +pub fn trim_multiline(s: Cow<'_, str>, ignore_first: bool, indent: Option) -> 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 index 2e2bd054e42a..fec115ff29d6 100644 --- 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 index 2a0cc1f18e27..90fee3aab23b 100644 --- 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 index 4cbbc8b8cd43..456b12a2c6b1 100644 --- 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 From b220ddf146f4c11011b2e1b7f37ecb8e5485555b Mon Sep 17 00:00:00 2001 From: Tim Nielens Date: Wed, 2 Sep 2020 23:30:40 +0200 Subject: [PATCH 3/3] unit-arg - pr remarks --- clippy_lints/src/types.rs | 82 ++++++++++++++++++------------ clippy_lints/src/utils/mod.rs | 96 ++++++++++++++++++++--------------- 2 files changed, 103 insertions(+), 75 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 16e48d919164..e83d491fac3b 100644 --- a/clippy_lints/src/types.rs +++ b/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], + non_empty_block_args_snippets: &[impl AsRef], +) -> 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::>(); + 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::>() - .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( diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 4ce4cdeefb48..f394b980127c 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/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) -> 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) -> 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, ch: char) -> Cow<'_, str> { - let mut x = s +fn reindent_multiline_inner(s: &str, ignore_first: bool, indent: Option, 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 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::>() - .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::>() + .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,14 +1509,14 @@ 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 { @@ -1528,6 +1524,22 @@ mod test { }".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!["/*", "", "*/"]);