diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs index 886c325b355f..a48e4d2fbd57 100644 --- a/clippy_lints/src/default.rs +++ b/clippy_lints/src/default.rs @@ -1,10 +1,9 @@ use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_sugg}; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::{has_drop, is_copy}; -use clippy_utils::{contains_name, get_parent_expr, in_automatically_derived, is_from_proc_macro}; +use clippy_utils::{contains_name, get_parent_expr, in_automatically_derived, is_expr_default, is_from_proc_macro}; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; -use rustc_hir::def::Res; use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind, StructTailExpr}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; @@ -129,7 +128,7 @@ impl<'tcx> LateLintPass<'tcx> for Default { // only take bindings to identifiers && let PatKind::Binding(_, binding_id, ident, _) = local.pat.kind // only when assigning `... = Default::default()` - && is_expr_default(expr, cx) + && is_expr_default(cx, expr) && let binding_type = cx.typeck_results().node_type(binding_id) && let ty::Adt(adt, args) = *binding_type.kind() && adt.is_struct() @@ -251,19 +250,6 @@ impl<'tcx> LateLintPass<'tcx> for Default { } } -/// Checks if the given expression is the `default` method belonging to the `Default` trait. -fn is_expr_default<'tcx>(expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> bool { - if let ExprKind::Call(fn_expr, []) = &expr.kind - && let ExprKind::Path(qpath) = &fn_expr.kind - && let Res::Def(_, def_id) = cx.qpath_res(qpath, fn_expr.hir_id) - { - // right hand side of assignment is `Default::default` - cx.tcx.is_diagnostic_item(sym::default_fn, def_id) - } else { - false - } -} - /// Returns the reassigned field and the assigning expression (right-hand side of assign). fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Option<(Ident, &'tcx Expr<'tcx>)> { if let StmtKind::Semi(later_expr) = this.kind diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index 019ae16ca859..ae6d8a1c1aa3 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -1,9 +1,14 @@ +use std::iter; + use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::is_from_proc_macro; -use clippy_utils::source::{SourceText, SpanRangeExt, indent_of, reindent_multiline}; +use clippy_utils::source::{SpanRangeExt, indent_of, reindent_multiline}; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::expr_type_is_certain; +use clippy_utils::{is_expr_default, is_from_proc_macro}; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, MatchSource, Node, StmtKind}; use rustc_lint::LateContext; +use rustc_span::SyntaxContext; use super::{UNIT_ARG, utils}; @@ -59,7 +64,7 @@ fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { } } -fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { +fn lint_unit_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, args_to_recover: &[&'tcx Expr<'tcx>]) { let mut applicability = Applicability::MachineApplicable; let (singular, plural) = if args_to_recover.len() > 1 { ("", "s") @@ -100,34 +105,41 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp let arg_snippets: Vec<_> = args_to_recover .iter() - .filter_map(|arg| arg.span.get_source_text(cx)) + // If the argument is from an expansion and is a `Default::default()`, we skip it + .filter(|arg| !arg.span.from_expansion() || !is_expr_default_nested(cx, arg)) + .filter_map(|arg| get_expr_snippet(cx, arg)) .collect(); - let arg_snippets_without_empty_blocks: Vec<_> = args_to_recover + + // If the argument is an empty block or `Default::default()`, we can replace it with `()`. + let arg_snippets_without_redundant_exprs: Vec<_> = args_to_recover .iter() - .filter(|arg| !is_empty_block(arg)) - .filter_map(|arg| arg.span.get_source_text(cx)) + .filter(|arg| !is_expr_default_nested(cx, arg) && (arg.span.from_expansion() || !is_empty_block(arg))) + .filter_map(|arg| get_expr_snippet_with_type_certainty(cx, arg)) .collect(); if let Some(call_snippet) = expr.span.get_source_text(cx) { - 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() { + if arg_snippets_without_redundant_exprs.is_empty() + && let suggestions = args_to_recover + .iter() + .filter(|arg| !arg.span.from_expansion() || !is_expr_default_nested(cx, arg)) + .map(|arg| (arg.span.parent_callsite().unwrap_or(arg.span), "()".to_string())) + .collect::>() + && !suggestions.is_empty() + { db.multipart_suggestion( format!("use {singular}unit literal{plural} instead"), - args_to_recover - .iter() - .map(|arg| (arg.span, "()".to_string())) - .collect::>(), + suggestions, applicability, ); } else { - let plural = arg_snippets_without_empty_blocks.len() > 1; + let plural = arg_snippets_without_redundant_exprs.len() > 1; + let sugg = fmt_stmts_and_call( + cx, + expr, + &call_snippet, + arg_snippets, + arg_snippets_without_redundant_exprs, + ); let empty_or_s = if plural { "s" } else { "" }; let it_or_them = if plural { "them" } else { "it" }; db.span_suggestion( @@ -144,6 +156,55 @@ fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Exp ); } +fn is_expr_default_nested<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + is_expr_default(cx, expr) + || matches!(expr.kind, ExprKind::Block(block, _) + if block.expr.is_some() && is_expr_default_nested(cx, block.expr.unwrap())) +} + +enum MaybeTypeUncertain<'tcx> { + Certain(Sugg<'tcx>), + Uncertain(Sugg<'tcx>), +} + +impl From> for String { + fn from(value: MaybeTypeUncertain<'_>) -> Self { + match value { + MaybeTypeUncertain::Certain(sugg) => sugg.to_string(), + MaybeTypeUncertain::Uncertain(sugg) => format!("let _: () = {sugg}"), + } + } +} + +fn get_expr_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + let mut app = Applicability::MachineApplicable; + let snip = Sugg::hir_with_context(cx, expr, SyntaxContext::root(), "..", &mut app); + if app != Applicability::MachineApplicable { + return None; + } + + Some(snip) +} + +fn get_expr_snippet_with_type_certainty<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, +) -> Option> { + get_expr_snippet(cx, expr).map(|snip| { + // If the type of the expression is certain, we can use it directly. + // Otherwise, we wrap it in a `let _: () = ...` to ensure the type is correct. + if !expr_type_is_certain(cx, expr) && !is_block_with_no_expr(expr) { + MaybeTypeUncertain::Uncertain(snip) + } else { + MaybeTypeUncertain::Certain(snip) + } + }) +} + +fn is_block_with_no_expr(expr: &Expr<'_>) -> bool { + matches!(expr.kind, ExprKind::Block(Block { expr: None, .. }, _)) +} + fn is_empty_block(expr: &Expr<'_>) -> bool { matches!( expr.kind, @@ -162,23 +223,20 @@ fn fmt_stmts_and_call( cx: &LateContext<'_>, call_expr: &Expr<'_>, call_snippet: &str, - args_snippets: &[SourceText], - non_empty_block_args_snippets: &[SourceText], + args_snippets: Vec>, + non_empty_block_args_snippets: Vec>, ) -> 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 call_snippet_with_replacements = args_snippets.into_iter().fold(call_snippet.to_owned(), |acc, arg| { + acc.replacen(&arg.to_string(), "()", 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 + let stmts_and_call = non_empty_block_args_snippets .into_iter() + .map(Into::into) + .chain(iter::once(call_snippet_with_replacements)) .map(|v| reindent_multiline(&v, true, Some(call_expr_indent))) - .collect(); + .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 diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 3a3191765710..4e588adf43eb 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -3471,3 +3471,15 @@ pub fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { None } } + +/// Checks if the given expression is a call to `Default::default()`. +pub fn is_expr_default<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if let ExprKind::Call(fn_expr, []) = &expr.kind + && let ExprKind::Path(qpath) = &fn_expr.kind + && let Res::Def(_, def_id) = cx.qpath_res(qpath, fn_expr.hir_id) + { + cx.tcx.is_diagnostic_item(sym::default_fn, def_id) + } else { + false + } +} diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs index 22a6a26dab62..4208efad6774 100644 --- a/tests/ui/unit_arg.rs +++ b/tests/ui/unit_arg.rs @@ -151,3 +151,27 @@ fn main() { bad(); ok(); } + +fn issue14857() { + let fn_take_unit = |_: ()| {}; + fn some_other_fn(_: &i32) {} + + macro_rules! mac { + (def) => { + Default::default() + }; + (func $f:expr) => { + $f() + }; + (nonempty_block $e:expr) => {{ + some_other_fn(&$e); + $e + }}; + } + fn_take_unit(mac!(def)); + //~^ unit_arg + fn_take_unit(mac!(func Default::default)); + //~^ unit_arg + fn_take_unit(mac!(nonempty_block Default::default())); + //~^ unit_arg +} diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr index 6c333d9792d4..0dcfb02b9fa0 100644 --- a/tests/ui/unit_arg.stderr +++ b/tests/ui/unit_arg.stderr @@ -199,5 +199,26 @@ LL ~ foo(1); LL + Some(()) | -error: aborting due to 10 previous errors +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:171:5 + | +LL | fn_take_unit(mac!(def)); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:173:5 + | +LL | fn_take_unit(mac!(func Default::default)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: passing a unit value to a function + --> tests/ui/unit_arg.rs:175:5 + | +LL | fn_take_unit(mac!(nonempty_block Default::default())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + +error: aborting due to 13 previous errors diff --git a/tests/ui/unit_arg_empty_blocks.fixed b/tests/ui/unit_arg_empty_blocks.fixed deleted file mode 100644 index b045a33608d7..000000000000 --- a/tests/ui/unit_arg_empty_blocks.fixed +++ /dev/null @@ -1,34 +0,0 @@ -#![warn(clippy::unit_arg)] -#![allow(unused_must_use, unused_variables)] -#![allow(clippy::no_effect, clippy::uninlined_format_args)] - -use std::fmt::Debug; - -fn foo(t: T) { - println!("{:?}", t); -} - -fn foo3(t1: T1, t2: T2, t3: T3) { - println!("{:?}, {:?}, {:?}", t1, t2, t3); -} - -fn bad() { - foo(()); - //~^ unit_arg - foo3((), 2, 2); - //~^ unit_arg - foo(0); - taking_two_units((), ()); - //~^ unit_arg - foo(0); - foo(1); - taking_three_units((), (), ()); - //~^ unit_arg -} - -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.rs b/tests/ui/unit_arg_empty_blocks.rs deleted file mode 100644 index ab305913f3f6..000000000000 --- a/tests/ui/unit_arg_empty_blocks.rs +++ /dev/null @@ -1,31 +0,0 @@ -#![warn(clippy::unit_arg)] -#![allow(unused_must_use, unused_variables)] -#![allow(clippy::no_effect, clippy::uninlined_format_args)] - -use std::fmt::Debug; - -fn foo(t: T) { - println!("{:?}", t); -} - -fn foo3(t1: T1, t2: T2, t3: T3) { - println!("{:?}, {:?}, {:?}", t1, t2, t3); -} - -fn bad() { - foo({}); - //~^ unit_arg - foo3({}, 2, 2); - //~^ unit_arg - taking_two_units({}, foo(0)); - //~^ unit_arg - taking_three_units({}, foo(0), foo(1)); - //~^ unit_arg -} - -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 deleted file mode 100644 index 2c686d58ecc1..000000000000 --- a/tests/ui/unit_arg_empty_blocks.stderr +++ /dev/null @@ -1,46 +0,0 @@ -error: passing a unit value to a function - --> tests/ui/unit_arg_empty_blocks.rs:16:5 - | -LL | foo({}); - | ^^^^--^ - | | - | help: use a unit literal instead: `()` - | - = note: `-D clippy::unit-arg` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::unit_arg)]` - -error: passing a unit value to a function - --> tests/ui/unit_arg_empty_blocks.rs:18:5 - | -LL | foo3({}, 2, 2); - | ^^^^^--^^^^^^^ - | | - | help: use a unit literal instead: `()` - -error: passing unit values to a function - --> tests/ui/unit_arg_empty_blocks.rs:20:5 - | -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); -LL ~ taking_two_units((), ()); - | - -error: passing unit values to a function - --> tests/ui/unit_arg_empty_blocks.rs:22:5 - | -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); -LL + foo(1); -LL ~ taking_three_units((), (), ()); - | - -error: aborting due to 4 previous errors - diff --git a/tests/ui/unit_arg_fixable.fixed b/tests/ui/unit_arg_fixable.fixed new file mode 100644 index 000000000000..03353a14081b --- /dev/null +++ b/tests/ui/unit_arg_fixable.fixed @@ -0,0 +1,78 @@ +#![warn(clippy::unit_arg)] +#![allow(unused_must_use, unused_variables)] +#![allow(clippy::no_effect, clippy::uninlined_format_args)] + +use std::fmt::Debug; + +fn foo(t: T) { + println!("{:?}", t); +} + +fn foo3(t1: T1, t2: T2, t3: T3) { + println!("{:?}, {:?}, {:?}", t1, t2, t3); +} + +fn bad() { + foo(()); + //~^ unit_arg + foo3((), 2, 2); + //~^ unit_arg + foo(0); + taking_two_units((), ()); + //~^ unit_arg + foo(0); + foo(1); + taking_three_units((), (), ()); + //~^ unit_arg +} + +fn taking_two_units(a: (), b: ()) {} +fn taking_three_units(a: (), b: (), c: ()) {} + +fn main() { + bad(); +} + +fn issue14857() { + let fn_take_unit = |_: ()| {}; + fn_take_unit(()); + //~^ unit_arg + + fn some_other_fn(_: &i32) {} + + macro_rules! another_mac { + () => { + some_other_fn(&Default::default()) + }; + ($e:expr) => { + some_other_fn(&$e) + }; + } + + another_mac!(); + fn_take_unit(()); + //~^ unit_arg + another_mac!(1); + fn_take_unit(()); + //~^ unit_arg + + macro_rules! mac { + (nondef $e:expr) => { + $e + }; + (empty_block) => {{}}; + } + fn_take_unit(mac!(nondef ())); + //~^ unit_arg + mac!(empty_block); + fn_take_unit(()); + //~^ unit_arg + + fn def() -> T { + Default::default() + } + + let _: () = def(); + fn_take_unit(()); + //~^ unit_arg +} diff --git a/tests/ui/unit_arg_fixable.rs b/tests/ui/unit_arg_fixable.rs new file mode 100644 index 000000000000..03fd96efdf90 --- /dev/null +++ b/tests/ui/unit_arg_fixable.rs @@ -0,0 +1,71 @@ +#![warn(clippy::unit_arg)] +#![allow(unused_must_use, unused_variables)] +#![allow(clippy::no_effect, clippy::uninlined_format_args)] + +use std::fmt::Debug; + +fn foo(t: T) { + println!("{:?}", t); +} + +fn foo3(t1: T1, t2: T2, t3: T3) { + println!("{:?}, {:?}, {:?}", t1, t2, t3); +} + +fn bad() { + foo({}); + //~^ unit_arg + foo3({}, 2, 2); + //~^ unit_arg + taking_two_units({}, foo(0)); + //~^ unit_arg + taking_three_units({}, foo(0), foo(1)); + //~^ unit_arg +} + +fn taking_two_units(a: (), b: ()) {} +fn taking_three_units(a: (), b: (), c: ()) {} + +fn main() { + bad(); +} + +fn issue14857() { + let fn_take_unit = |_: ()| {}; + fn_take_unit(Default::default()); + //~^ unit_arg + + fn some_other_fn(_: &i32) {} + + macro_rules! another_mac { + () => { + some_other_fn(&Default::default()) + }; + ($e:expr) => { + some_other_fn(&$e) + }; + } + + fn_take_unit(another_mac!()); + //~^ unit_arg + fn_take_unit(another_mac!(1)); + //~^ unit_arg + + macro_rules! mac { + (nondef $e:expr) => { + $e + }; + (empty_block) => {{}}; + } + fn_take_unit(mac!(nondef Default::default())); + //~^ unit_arg + fn_take_unit(mac!(empty_block)); + //~^ unit_arg + + fn def() -> T { + Default::default() + } + + fn_take_unit(def()); + //~^ unit_arg +} diff --git a/tests/ui/unit_arg_fixable.stderr b/tests/ui/unit_arg_fixable.stderr new file mode 100644 index 000000000000..ccd5aa8322f9 --- /dev/null +++ b/tests/ui/unit_arg_fixable.stderr @@ -0,0 +1,110 @@ +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:16:5 + | +LL | foo({}); + | ^^^^--^ + | | + | help: use a unit literal instead: `()` + | + = note: `-D clippy::unit-arg` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unit_arg)]` + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:18:5 + | +LL | foo3({}, 2, 2); + | ^^^^^--^^^^^^^ + | | + | help: use a unit literal instead: `()` + +error: passing unit values to a function + --> tests/ui/unit_arg_fixable.rs:20:5 + | +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); +LL ~ taking_two_units((), ()); + | + +error: passing unit values to a function + --> tests/ui/unit_arg_fixable.rs:22:5 + | +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); +LL + foo(1); +LL ~ taking_three_units((), (), ()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:35:5 + | +LL | fn_take_unit(Default::default()); + | ^^^^^^^^^^^^^------------------^ + | | + | help: use a unit literal instead: `()` + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:49:5 + | +LL | fn_take_unit(another_mac!()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ another_mac!(); +LL ~ fn_take_unit(()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:51:5 + | +LL | fn_take_unit(another_mac!(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ another_mac!(1); +LL ~ fn_take_unit(()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:60:5 + | +LL | fn_take_unit(mac!(nondef Default::default())); + | ^^^^^^^^^^^^^^^^^^^^^^^^^------------------^^ + | | + | help: use a unit literal instead: `()` + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:62:5 + | +LL | fn_take_unit(mac!(empty_block)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ mac!(empty_block); +LL ~ fn_take_unit(()); + | + +error: passing a unit value to a function + --> tests/ui/unit_arg_fixable.rs:69:5 + | +LL | fn_take_unit(def()); + | ^^^^^^^^^^^^^^^^^^^ + | +help: move the expression in front of the call and replace it with the unit literal `()` + | +LL ~ let _: () = def(); +LL ~ fn_take_unit(()); + | + +error: aborting due to 10 previous errors +