diff options
| -rw-r--r-- | clippy_lints/src/types.rs | 133 | ||||
| -rw-r--r-- | tests/ui/unit_arg.fixed | 64 | ||||
| -rw-r--r-- | tests/ui/unit_arg.rs | 27 | ||||
| -rw-r--r-- | tests/ui/unit_arg.stderr | 178 | ||||
| -rw-r--r-- | tests/ui/unit_arg_empty_blocks.rs | 26 | ||||
| -rw-r--r-- | tests/ui/unit_arg_empty_blocks.stderr | 51 |
6 files changed, 353 insertions, 126 deletions
diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 3ac99e24684..5ca30d598eb 100644 --- a/clippy_lints/src/types.rs +++ b/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,24 +779,22 @@ 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); } }, _ => (), @@ -804,6 +802,101 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitArg { } } +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 { diff --git a/tests/ui/unit_arg.fixed b/tests/ui/unit_arg.fixed deleted file mode 100644 index a739cf7ad81..00000000000 --- a/tests/ui/unit_arg.fixed +++ /dev/null @@ -1,64 +0,0 @@ -// run-rustfix -#![warn(clippy::unit_arg)] -#![allow(unused_braces, clippy::no_effect, unused_must_use)] - -use std::fmt::Debug; - -fn foo<T: Debug>(t: T) { - println!("{:?}", t); -} - -fn foo3<T1: Debug, T2: Debug, T3: Debug>(t1: T1, t2: T2, t3: T3) { - println!("{:?}, {:?}, {:?}", t1, t2, t3); -} - -struct Bar; - -impl Bar { - fn bar<T: Debug>(&self, t: T) { - println!("{:?}", t); - } -} - -fn bad() { - foo(()); - foo(()); - foo(()); - foo(()); - foo3((), 2, 2); - let b = Bar; - b.bar(()); -} - -fn ok() { - foo(()); - foo(1); - foo({ 1 }); - foo3("a", 3, vec![3]); - let b = Bar; - b.bar({ 1 }); - b.bar(()); - question_mark(); -} - -fn question_mark() -> Result<(), ()> { - Ok(Ok(())?)?; - Ok(Ok(()))??; - Ok(()) -} - -#[allow(dead_code)] -mod issue_2945 { - fn unit_fn() -> Result<(), i32> { - Ok(()) - } - - fn fallible() -> Result<(), i32> { - Ok(unit_fn()?) - } -} - -fn main() { - bad(); - ok(); -} diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index d90c49f79de..2992abae775 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -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(); diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 21ccc684ea9..56f6a855dfa 100644 --- a/tests/ui/unit_arg.stderr +++ b/tests/ui/unit_arg.stderr @@ -1,79 +1,181 @@ error: passing a unit value to a function - --> $DIR/unit_arg.rs:24:9 + --> $DIR/unit_arg.rs:23:5 | -LL | foo({}); - | ^^ +LL | / foo({ +LL | | 1; +LL | | }); + | |______^ | = note: `-D clippy::unit-arg` implied by `-D warnings` -help: if you intended to pass a unit value, use a unit literal instead +help: remove the semicolon from the last statement in the block | -LL | foo(()); - | ^^ - -error: passing a unit value to a function - --> $DIR/unit_arg.rs:25:9 +LL | 1 | -LL | foo({ - | _________^ -LL | | 1; -LL | | }); - | |_____^ +help: or move the expression in front of the call... | -help: if you intended to pass a unit value, use a unit literal instead +LL | { +LL | 1; +LL | }; + | +help: ...and use a unit literal instead | LL | foo(()); | ^^ error: passing a unit value to a function - --> $DIR/unit_arg.rs:28:9 + --> $DIR/unit_arg.rs:26:5 | LL | foo(foo(1)); - | ^^^^^^ + | ^^^^^^^^^^^ + | +help: move the expression in front of the call... | -help: if you intended to pass a unit value, use a unit literal instead +LL | foo(1); + | +help: ...and use a unit literal instead | LL | foo(()); | ^^ error: passing a unit value to a function - --> $DIR/unit_arg.rs:29:9 + --> $DIR/unit_arg.rs:27:5 | -LL | foo({ - | _________^ +LL | / foo({ LL | | foo(1); LL | | foo(2); LL | | }); - | |_____^ + | |______^ + | +help: remove the semicolon from the last statement in the block + | +LL | foo(2) | -help: if you intended to pass a unit value, use a unit literal instead +help: or move the expression in front of the call... + | +LL | { +LL | foo(1); +LL | foo(2); +LL | }; + | +help: ...and use a unit literal instead | LL | foo(()); | ^^ error: passing a unit value to a function - --> $DIR/unit_arg.rs:33:10 + --> $DIR/unit_arg.rs:32:5 | -LL | foo3({}, 2, 2); - | ^^ +LL | / b.bar({ +LL | | 1; +LL | | }); + | |______^ | -help: if you intended to pass a unit value, use a unit literal instead +help: remove the semicolon from the last statement in the block | -LL | foo3((), 2, 2); - | ^^ +LL | 1 + | +help: or move the expression in front of the call... + | +LL | { +LL | 1; +LL | }; + | +help: ...and use a unit literal instead + | +LL | b.bar(()); + | ^^ -error: passing a unit value to a function - --> $DIR/unit_arg.rs:35:11 +error: passing unit values to a function + --> $DIR/unit_arg.rs:35:5 | -LL | b.bar({ - | ___________^ -LL | | 1; +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 + | +LL | taking_multiple_units((), ()); + | ^^ ^^ + +error: passing unit values to a function + --> $DIR/unit_arg.rs:36:5 + | +LL | / taking_multiple_units(foo(0), { +LL | | foo(1); +LL | | foo(2); LL | | }); + | |______^ + | +help: remove the semicolon from the last statement in the block + | +LL | foo(2) + | +help: or move the expressions in front of the call... + | +LL | foo(0); +LL | { +LL | foo(1); +LL | foo(2); +LL | }; + | +help: ...and use unit literals instead + | +LL | taking_multiple_units((), ()); + | ^^ ^^ + +error: passing unit values to a function + --> $DIR/unit_arg.rs:40:5 + | +LL | / taking_multiple_units( +LL | | { +LL | | foo(0); +LL | | foo(1); +... | +LL | | }, +LL | | ); | |_____^ | -help: if you intended to pass a unit value, use a unit literal instead +help: remove the semicolon from the last statement in the block | -LL | b.bar(()); - | ^^ +LL | foo(1) + | +help: remove the semicolon from the last statement in the block + | +LL | foo(3) + | +help: or move the expressions in front of the call... + | +LL | { +LL | foo(0); +LL | foo(1); +LL | }; +LL | { +LL | foo(2); + ... +help: ...and use unit literals instead + | +LL | (), +LL | (), + | + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:82:5 + | +LL | Some(foo(1)) + | ^^^^^^^^^^^^ + | +help: move the expression in front of the call... + | +LL | foo(1); + | +help: ...and use a unit literal instead + | +LL | Some(()) + | ^^ -error: aborting due to 6 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/unit_arg_empty_blocks.rs b/tests/ui/unit_arg_empty_blocks.rs new file mode 100644 index 00000000000..18a31eb3dee --- /dev/null +++ b/tests/ui/unit_arg_empty_blocks.rs @@ -0,0 +1,26 @@ +#![warn(clippy::unit_arg)] +#![allow(clippy::no_effect, unused_must_use, unused_variables)] + +use std::fmt::Debug; + +fn foo<T: Debug>(t: T) { + println!("{:?}", t); +} + +fn foo3<T1: Debug, T2: Debug, T3: Debug>(t1: T1, t2: T2, t3: T3) { + println!("{:?}, {:?}, {:?}", t1, t2, t3); +} + +fn bad() { + foo({}); + foo3({}, 2, 2); + taking_two_units({}, foo(0)); + taking_three_units({}, foo(0), foo(1)); +} + +fn taking_two_units(a: (), b: ()) {} +fn taking_three_units(a: (), b: (), c: ()) {} + +fn main() { + bad(); +} diff --git a/tests/ui/unit_arg_empty_blocks.stderr b/tests/ui/unit_arg_empty_blocks.stderr new file mode 100644 index 00000000000..bb58483584b --- /dev/null +++ b/tests/ui/unit_arg_empty_blocks.stderr @@ -0,0 +1,51 @@ +error: passing a unit value to a function + --> $DIR/unit_arg_empty_blocks.rs:15:5 + | +LL | foo({}); + | ^^^^--^ + | | + | help: use a unit literal instead: `()` + | + = note: `-D clippy::unit-arg` implied by `-D warnings` + +error: passing a unit value to a function + --> $DIR/unit_arg_empty_blocks.rs:16:5 + | +LL | foo3({}, 2, 2); + | ^^^^^--^^^^^^^ + | | + | help: use a unit literal instead: `()` + +error: passing unit values to a function + --> $DIR/unit_arg_empty_blocks.rs:17:5 + | +LL | taking_two_units({}, foo(0)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call... + | +LL | foo(0); + | +help: ...and use unit literals instead + | +LL | taking_two_units((), ()); + | ^^ ^^ + +error: passing unit values to a function + --> $DIR/unit_arg_empty_blocks.rs:18:5 + | +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 + | +LL | taking_three_units((), (), ()); + | ^^ ^^ ^^ + +error: aborting due to 4 previous errors + |