From 0c347d3d0686d8892512dcc1e13102d15afcc6dd Mon Sep 17 00:00:00 2001 From: Marc Dominik Migge Date: Sun, 17 Jan 2021 14:42:36 +0100 Subject: [PATCH 1/4] Fix false positive for unit_arg lint --- clippy_lints/src/types.rs | 11 +++++++++- tests/ui/unit_arg.rs | 20 ++++++++++++++++- tests/ui/unit_arg.stderr | 46 +++++++++++++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 3b5a83d2a0be..7d0eea37bc02 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -955,7 +955,16 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg { .iter() .filter(|arg| { if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { - !matches!(&arg.kind, ExprKind::Match(.., MatchSource::TryDesugar)) + match &arg.kind { + ExprKind::Block(..) + | ExprKind::Call(..) + | ExprKind::If(..) + | ExprKind::MethodCall(..) => true, + ExprKind::Match(..) => { + !matches!(&arg.kind, ExprKind::Match(.., MatchSource::TryDesugar)) + }, + _ => false, + } } else { false } diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index b6a7bc5a1cc9..79dac925f08c 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -59,7 +59,18 @@ fn bad() { 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(())) + foo(foo(())); + foo(if true { + 1; + }); + foo(match Some(1) { + Some(_) => { + 1; + }, + None => { + 0; + }, + }); } fn ok() { @@ -71,6 +82,13 @@ fn ok() { b.bar({ 1 }); b.bar(()); question_mark(); + let named_unit_arg = (); + foo(named_unit_arg); + foo(if true { 1 } else { 0 }); + foo(match Some(1) { + Some(_) => 1, + None => 0, + }); } fn question_mark() -> Result<(), ()> { diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 094cff8c9859..8679706f8ec8 100644 --- a/tests/ui/unit_arg.stderr +++ b/tests/ui/unit_arg.stderr @@ -156,17 +156,55 @@ LL | }); error: passing a unit value to a function --> $DIR/unit_arg.rs:62:5 | -LL | foo(foo(())) +LL | foo(foo(())); | ^^^^^^^^^^^^ | help: move the expression in front of the call and replace it with the unit literal `()` | LL | foo(()); -LL | foo(()) +LL | foo(()); + | + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:63:5 + | +LL | / foo(if true { +LL | | 1; +LL | | }); + | |______^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL | if true { +LL | 1; +LL | }; +LL | foo(()); + | + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:66:5 | +LL | / foo(match Some(1) { +LL | | Some(_) => { +LL | | 1; +LL | | }, +... | +LL | | }, +LL | | }); + | |______^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL | match Some(1) { +LL | Some(_) => { +LL | 1; +LL | }, +LL | None => { +LL | 0; + ... error: passing a unit value to a function - --> $DIR/unit_arg.rs:95:5 + --> $DIR/unit_arg.rs:113:5 | LL | Some(foo(1)) | ^^^^^^^^^^^^ @@ -177,5 +215,5 @@ LL | foo(1); LL | Some(()) | -error: aborting due to 10 previous errors +error: aborting due to 12 previous errors From eb476c6c70bccb87378462e4854f8b8fa12c40be Mon Sep 17 00:00:00 2001 From: Marc Dominik Migge Date: Mon, 18 Jan 2021 20:18:56 +0100 Subject: [PATCH 2/4] Split up tests for unit arg expressions --- tests/ui/unit_arg.rs | 16 ----------- tests/ui/unit_arg.stderr | 42 ++-------------------------- tests/ui/unit_arg_expressions.rs | 35 +++++++++++++++++++++++ tests/ui/unit_arg_expressions.stderr | 41 +++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 56 deletions(-) create mode 100644 tests/ui/unit_arg_expressions.rs create mode 100644 tests/ui/unit_arg_expressions.stderr diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index 79dac925f08c..cce543006d77 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -60,17 +60,6 @@ fn bad() { // in this case, the suggestion can be inlined, no need for a surrounding block // foo(()); foo(()) instead of { foo(()); foo(()) } foo(foo(())); - foo(if true { - 1; - }); - foo(match Some(1) { - Some(_) => { - 1; - }, - None => { - 0; - }, - }); } fn ok() { @@ -84,11 +73,6 @@ fn ok() { question_mark(); let named_unit_arg = (); foo(named_unit_arg); - foo(if true { 1 } else { 0 }); - foo(match Some(1) { - Some(_) => 1, - None => 0, - }); } fn question_mark() -> Result<(), ()> { diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 8679706f8ec8..8e3f1811c659 100644 --- a/tests/ui/unit_arg.stderr +++ b/tests/ui/unit_arg.stderr @@ -166,45 +166,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:63:5 - | -LL | / foo(if true { -LL | | 1; -LL | | }); - | |______^ - | -help: move the expression in front of the call and replace it with the unit literal `()` - | -LL | if true { -LL | 1; -LL | }; -LL | foo(()); - | - -error: passing a unit value to a function - --> $DIR/unit_arg.rs:66:5 - | -LL | / foo(match Some(1) { -LL | | Some(_) => { -LL | | 1; -LL | | }, -... | -LL | | }, -LL | | }); - | |______^ - | -help: move the expression in front of the call and replace it with the unit literal `()` - | -LL | match Some(1) { -LL | Some(_) => { -LL | 1; -LL | }, -LL | None => { -LL | 0; - ... - -error: passing a unit value to a function - --> $DIR/unit_arg.rs:113:5 + --> $DIR/unit_arg.rs:97:5 | LL | Some(foo(1)) | ^^^^^^^^^^^^ @@ -215,5 +177,5 @@ LL | foo(1); LL | Some(()) | -error: aborting due to 12 previous errors +error: aborting due to 10 previous errors diff --git a/tests/ui/unit_arg_expressions.rs b/tests/ui/unit_arg_expressions.rs new file mode 100644 index 000000000000..a6807cb2e973 --- /dev/null +++ b/tests/ui/unit_arg_expressions.rs @@ -0,0 +1,35 @@ +#![warn(clippy::unit_arg)] +#![allow(clippy::no_effect)] + +use std::fmt::Debug; + +fn foo(t: T) { + println!("{:?}", t); +} + +fn bad() { + foo(if true { + 1; + }); + foo(match Some(1) { + Some(_) => { + 1; + }, + None => { + 0; + }, + }); +} + +fn ok() { + foo(if true { 1 } else { 0 }); + foo(match Some(1) { + Some(_) => 1, + None => 0, + }); +} + +fn main() { + bad(); + ok(); +} diff --git a/tests/ui/unit_arg_expressions.stderr b/tests/ui/unit_arg_expressions.stderr new file mode 100644 index 000000000000..9fb08106b723 --- /dev/null +++ b/tests/ui/unit_arg_expressions.stderr @@ -0,0 +1,41 @@ +error: passing a unit value to a function + --> $DIR/unit_arg_expressions.rs:11:5 + | +LL | / foo(if true { +LL | | 1; +LL | | }); + | |______^ + | + = note: `-D clippy::unit-arg` implied by `-D warnings` +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL | if true { +LL | 1; +LL | }; +LL | foo(()); + | + +error: passing a unit value to a function + --> $DIR/unit_arg_expressions.rs:14:5 + | +LL | / foo(match Some(1) { +LL | | Some(_) => { +LL | | 1; +LL | | }, +... | +LL | | }, +LL | | }); + | |______^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL | match Some(1) { +LL | Some(_) => { +LL | 1; +LL | }, +LL | None => { +LL | 0; + ... + +error: aborting due to 2 previous errors + From 9fe9d94abda8d2698d1dede6961007e0026760ea Mon Sep 17 00:00:00 2001 From: Marc Dominik Migge Date: Wed, 24 Feb 2021 13:31:04 +0100 Subject: [PATCH 3/4] Don't lint unit args if expression kind is path --- clippy_lints/src/types.rs | 14 +++------- tests/ui/unit_arg.rs | 5 ++++ tests/ui/unit_arg.stderr | 20 +++++++------- tests/ui/unit_arg_expressions.rs | 35 ------------------------ tests/ui/unit_arg_expressions.stderr | 41 ---------------------------- 5 files changed, 19 insertions(+), 96 deletions(-) delete mode 100644 tests/ui/unit_arg_expressions.rs delete mode 100644 tests/ui/unit_arg_expressions.stderr diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 7d0eea37bc02..77cde8b60c16 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -955,16 +955,10 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg { .iter() .filter(|arg| { if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { - match &arg.kind { - ExprKind::Block(..) - | ExprKind::Call(..) - | ExprKind::If(..) - | ExprKind::MethodCall(..) => true, - ExprKind::Match(..) => { - !matches!(&arg.kind, ExprKind::Match(.., MatchSource::TryDesugar)) - }, - _ => false, - } + !matches!( + &arg.kind, + ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) + ) } else { false } diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index cce543006d77..5ea2e5d65c51 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -27,6 +27,10 @@ impl Bar { } } +fn baz(t: T) { + foo(t); +} + fn bad() { foo({ 1; @@ -73,6 +77,7 @@ fn ok() { question_mark(); let named_unit_arg = (); foo(named_unit_arg); + baz(()); } fn question_mark() -> Result<(), ()> { diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 8e3f1811c659..b3fe9addb62f 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:31:5 + --> $DIR/unit_arg.rs:35:5 | LL | / foo({ LL | | 1; @@ -20,7 +20,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:34:5 + --> $DIR/unit_arg.rs:38:5 | LL | foo(foo(1)); | ^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:35:5 + --> $DIR/unit_arg.rs:39:5 | LL | / foo({ LL | | foo(1); @@ -54,7 +54,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:40:5 + --> $DIR/unit_arg.rs:44:5 | LL | / b.bar({ LL | | 1; @@ -74,7 +74,7 @@ LL | b.bar(()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:43:5 + --> $DIR/unit_arg.rs:47:5 | LL | taking_multiple_units(foo(0), foo(1)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -87,7 +87,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:44:5 + --> $DIR/unit_arg.rs:48:5 | LL | / taking_multiple_units(foo(0), { LL | | foo(1); @@ -110,7 +110,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:48:5 + --> $DIR/unit_arg.rs:52:5 | LL | / taking_multiple_units( LL | | { @@ -140,7 +140,7 @@ LL | foo(2); ... error: passing a unit value to a function - --> $DIR/unit_arg.rs:59:13 + --> $DIR/unit_arg.rs:63:13 | LL | None.or(Some(foo(2))); | ^^^^^^^^^^^^ @@ -154,7 +154,7 @@ LL | }); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:62:5 + --> $DIR/unit_arg.rs:66:5 | LL | foo(foo(())); | ^^^^^^^^^^^^ @@ -166,7 +166,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:97:5 + --> $DIR/unit_arg.rs:102:5 | LL | Some(foo(1)) | ^^^^^^^^^^^^ diff --git a/tests/ui/unit_arg_expressions.rs b/tests/ui/unit_arg_expressions.rs deleted file mode 100644 index a6807cb2e973..000000000000 --- a/tests/ui/unit_arg_expressions.rs +++ /dev/null @@ -1,35 +0,0 @@ -#![warn(clippy::unit_arg)] -#![allow(clippy::no_effect)] - -use std::fmt::Debug; - -fn foo(t: T) { - println!("{:?}", t); -} - -fn bad() { - foo(if true { - 1; - }); - foo(match Some(1) { - Some(_) => { - 1; - }, - None => { - 0; - }, - }); -} - -fn ok() { - foo(if true { 1 } else { 0 }); - foo(match Some(1) { - Some(_) => 1, - None => 0, - }); -} - -fn main() { - bad(); - ok(); -} diff --git a/tests/ui/unit_arg_expressions.stderr b/tests/ui/unit_arg_expressions.stderr deleted file mode 100644 index 9fb08106b723..000000000000 --- a/tests/ui/unit_arg_expressions.stderr +++ /dev/null @@ -1,41 +0,0 @@ -error: passing a unit value to a function - --> $DIR/unit_arg_expressions.rs:11:5 - | -LL | / foo(if true { -LL | | 1; -LL | | }); - | |______^ - | - = note: `-D clippy::unit-arg` implied by `-D warnings` -help: move the expression in front of the call and replace it with the unit literal `()` - | -LL | if true { -LL | 1; -LL | }; -LL | foo(()); - | - -error: passing a unit value to a function - --> $DIR/unit_arg_expressions.rs:14:5 - | -LL | / foo(match Some(1) { -LL | | Some(_) => { -LL | | 1; -LL | | }, -... | -LL | | }, -LL | | }); - | |______^ - | -help: move the expression in front of the call and replace it with the unit literal `()` - | -LL | match Some(1) { -LL | Some(_) => { -LL | 1; -LL | }, -LL | None => { -LL | 0; - ... - -error: aborting due to 2 previous errors - From cbe6eec3e67347af130769ad4a3bd2168997b411 Mon Sep 17 00:00:00 2001 From: Marc Dominik Migge Date: Thu, 25 Feb 2021 22:03:11 +0100 Subject: [PATCH 4/4] Add original test case from issue --- tests/ui/unit_arg.rs | 21 +++++++++++++++++++++ tests/ui/unit_arg.stderr | 20 ++++++++++---------- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index 5ea2e5d65c51..938cc3c78597 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -31,6 +31,26 @@ fn baz(t: T) { foo(t); } +trait Tr { + type Args; + fn do_it(args: Self::Args); +} + +struct A; +impl Tr for A { + type Args = (); + fn do_it(_: Self::Args) {} +} + +struct B; +impl Tr for B { + type Args = ::Args; + + fn do_it(args: Self::Args) { + A::do_it(args) + } +} + fn bad() { foo({ 1; @@ -78,6 +98,7 @@ fn ok() { let named_unit_arg = (); foo(named_unit_arg); baz(()); + B::do_it(()); } fn question_mark() -> Result<(), ()> { diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index b3fe9addb62f..354fd51cd6b6 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:35:5 + --> $DIR/unit_arg.rs:55:5 | LL | / foo({ LL | | 1; @@ -20,7 +20,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:38:5 + --> $DIR/unit_arg.rs:58:5 | LL | foo(foo(1)); | ^^^^^^^^^^^ @@ -32,7 +32,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:39:5 + --> $DIR/unit_arg.rs:59:5 | LL | / foo({ LL | | foo(1); @@ -54,7 +54,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:44:5 + --> $DIR/unit_arg.rs:64:5 | LL | / b.bar({ LL | | 1; @@ -74,7 +74,7 @@ LL | b.bar(()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:47:5 + --> $DIR/unit_arg.rs:67:5 | LL | taking_multiple_units(foo(0), foo(1)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -87,7 +87,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:48:5 + --> $DIR/unit_arg.rs:68:5 | LL | / taking_multiple_units(foo(0), { LL | | foo(1); @@ -110,7 +110,7 @@ LL | taking_multiple_units((), ()); | error: passing unit values to a function - --> $DIR/unit_arg.rs:52:5 + --> $DIR/unit_arg.rs:72:5 | LL | / taking_multiple_units( LL | | { @@ -140,7 +140,7 @@ LL | foo(2); ... error: passing a unit value to a function - --> $DIR/unit_arg.rs:63:13 + --> $DIR/unit_arg.rs:83:13 | LL | None.or(Some(foo(2))); | ^^^^^^^^^^^^ @@ -154,7 +154,7 @@ LL | }); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:66:5 + --> $DIR/unit_arg.rs:86:5 | LL | foo(foo(())); | ^^^^^^^^^^^^ @@ -166,7 +166,7 @@ LL | foo(()); | error: passing a unit value to a function - --> $DIR/unit_arg.rs:102:5 + --> $DIR/unit_arg.rs:123:5 | LL | Some(foo(1)) | ^^^^^^^^^^^^