From cc1bb8f57a167d2e01db4c2175bfea1a8f48e772 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 11 Jul 2024 15:08:22 +0800 Subject: [PATCH] make [`or_fun_call`] and [`unwrap_or_default`] recursive. --- clippy_lints/src/methods/or_fun_call.rs | 110 ++++++++++++++---------- tests/ui/or_fun_call.fixed | 35 ++++++++ tests/ui/or_fun_call.rs | 35 ++++++++ tests/ui/or_fun_call.stderr | 50 ++++++++++- 4 files changed, 183 insertions(+), 47 deletions(-) diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index baa60454419..e4326cb958e 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -1,8 +1,13 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::eager_or_lazy::switch_to_lazy_eval; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::{expr_type_is_certain, implements_trait, is_type_diagnostic_item}; -use clippy_utils::{contains_return, is_default_equivalent, is_default_equivalent_call, last_path_segment}; +use clippy_utils::visitors::for_each_expr; +use clippy_utils::{ + contains_return, is_default_equivalent, is_default_equivalent_call, last_path_segment, peel_blocks, +}; use rustc_errors::Applicability; use rustc_lint::LateContext; use rustc_middle::ty; @@ -13,7 +18,7 @@ use {rustc_ast as ast, rustc_hir as hir}; use super::{OR_FUN_CALL, UNWRAP_OR_DEFAULT}; /// Checks for the `OR_FUN_CALL` lint. -#[allow(clippy::too_many_lines)] +#[expect(clippy::too_many_lines)] pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, @@ -26,7 +31,6 @@ pub(super) fn check<'tcx>( /// `or_insert(T::new())` or `or_insert(T::default())`. /// Similarly checks for `unwrap_or_else(T::new)`, `unwrap_or_else(T::default)`, /// `or_insert_with(T::new)` or `or_insert_with(T::default)`. - #[allow(clippy::too_many_arguments)] fn check_unwrap_or_default( cx: &LateContext<'_>, name: &str, @@ -123,8 +127,8 @@ pub(super) fn check<'tcx>( } /// Checks for `*or(foo())`. - #[allow(clippy::too_many_arguments)] - fn check_general_case<'tcx>( + #[expect(clippy::too_many_arguments)] + fn check_or_fn_call<'tcx>( cx: &LateContext<'tcx>, name: &str, method_span: Span, @@ -135,7 +139,7 @@ pub(super) fn check<'tcx>( span: Span, // None if lambda is required fun_span: Option, - ) { + ) -> bool { // (path, fn_has_argument, methods, suffix) const KNOW_TYPES: [(Symbol, bool, &[&str], &str); 4] = [ (sym::BTreeEntry, false, &["or_insert"], "with"), @@ -185,54 +189,68 @@ pub(super) fn check<'tcx>( format!("{name}_{suffix}({sugg})"), app, ); + true + } else { + false } } - let extract_inner_arg = |arg: &'tcx hir::Expr<'_>| { - if let hir::ExprKind::Block( - hir::Block { - stmts: [], - expr: Some(expr), - .. - }, - _, - ) = arg.kind - { - expr - } else { - arg - } - }; - if let [arg] = args { - let inner_arg = extract_inner_arg(arg); - match inner_arg.kind { - hir::ExprKind::Call(fun, or_args) => { - let or_has_args = !or_args.is_empty(); - if or_has_args - || !check_unwrap_or_default(cx, name, receiver, fun, Some(inner_arg), expr.span, method_span) - { - let fun_span = if or_has_args { None } else { Some(fun.span) }; - check_general_case(cx, name, method_span, receiver, arg, None, expr.span, fun_span); - } - }, - hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) => { - check_unwrap_or_default(cx, name, receiver, inner_arg, None, expr.span, method_span); - }, - hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => { - check_general_case(cx, name, method_span, receiver, arg, None, expr.span, None); - }, - _ => (), - } + let inner_arg = peel_blocks(arg); + for_each_expr(cx, inner_arg, |ex| { + // `or_fun_call` lint needs to take nested expr into account, + // but `unwrap_or_default` lint doesn't, we don't want something like: + // `opt.unwrap_or(Foo { inner: String::default(), other: 1 })` to get replaced by + // `opt.unwrap_or_default()`. + let is_nested_expr = ex.hir_id != inner_arg.hir_id; + + let is_triggered = match ex.kind { + hir::ExprKind::Call(fun, fun_args) => { + let inner_fun_has_args = !fun_args.is_empty(); + let fun_span = if inner_fun_has_args || is_nested_expr { + None + } else { + Some(fun.span) + }; + (!inner_fun_has_args + && !is_nested_expr + && check_unwrap_or_default(cx, name, receiver, fun, Some(ex), expr.span, method_span)) + || check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, fun_span) + }, + hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) if !is_nested_expr => { + check_unwrap_or_default(cx, name, receiver, ex, None, expr.span, method_span) + }, + hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => { + check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, None) + }, + _ => false, + }; + + if is_triggered { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + }); } // `map_or` takes two arguments if let [arg, lambda] = args { - let inner_arg = extract_inner_arg(arg); - if let hir::ExprKind::Call(fun, or_args) = inner_arg.kind { - let fun_span = if or_args.is_empty() { Some(fun.span) } else { None }; - check_general_case(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span); - } + let inner_arg = peel_blocks(arg); + for_each_expr(cx, inner_arg, |ex| { + let is_top_most_expr = ex.hir_id == inner_arg.hir_id; + if let hir::ExprKind::Call(fun, fun_args) = ex.kind { + let fun_span = if fun_args.is_empty() && is_top_most_expr { + Some(fun.span) + } else { + None + }; + if check_or_fn_call(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span) { + return ControlFlow::Break(()); + } + } + ControlFlow::Continue(()) + }); } } diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index 35015592971..7452eb77688 100644 --- a/tests/ui/or_fun_call.fixed +++ b/tests/ui/or_fun_call.fixed @@ -330,4 +330,39 @@ mod issue_10228 { } } +// issue #12973 +fn fn_call_in_nested_expr() { + struct Foo { + val: String, + } + + fn f() -> i32 { + 1 + } + let opt: Option = Some(1); + + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or_else(f); // suggest `.unwrap_or_else(f)` + // + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or_else(|| f() + 1); // suggest `.unwrap_or_else(|| f() + 1)` + // + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or_else(|| { + let x = f(); + x + 1 + }); + //~v ERROR: use of `map_or` followed by a function call + let _ = opt.map_or_else(|| f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)` + // + //~v ERROR: use of `unwrap_or` to construct default value + let _ = opt.unwrap_or_default(); + + let opt_foo = Some(Foo { + val: String::from("123"), + }); + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt_foo.unwrap_or_else(|| Foo { val: String::default() }); +} + fn main() {} diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index 3dcf657d743..cd6f7bb2070 100644 --- a/tests/ui/or_fun_call.rs +++ b/tests/ui/or_fun_call.rs @@ -330,4 +330,39 @@ mod issue_10228 { } } +// issue #12973 +fn fn_call_in_nested_expr() { + struct Foo { + val: String, + } + + fn f() -> i32 { + 1 + } + let opt: Option = Some(1); + + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or({ f() }); // suggest `.unwrap_or_else(f)` + // + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or(f() + 1); // suggest `.unwrap_or_else(|| f() + 1)` + // + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt.unwrap_or({ + let x = f(); + x + 1 + }); + //~v ERROR: use of `map_or` followed by a function call + let _ = opt.map_or(f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)` + // + //~v ERROR: use of `unwrap_or` to construct default value + let _ = opt.unwrap_or({ i32::default() }); + + let opt_foo = Some(Foo { + val: String::from("123"), + }); + //~v ERROR: use of `unwrap_or` followed by a function call + let _ = opt_foo.unwrap_or(Foo { val: String::default() }); +} + fn main() {} diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index 3070db22fc5..06f804fb41e 100644 --- a/tests/ui/or_fun_call.stderr +++ b/tests/ui/or_fun_call.stderr @@ -196,5 +196,53 @@ error: use of `unwrap_or_else` to construct default value LL | let _ = stringy.unwrap_or_else(String::new); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` -error: aborting due to 32 previous errors +error: use of `unwrap_or` followed by a function call + --> tests/ui/or_fun_call.rs:345:17 + | +LL | let _ = opt.unwrap_or({ f() }); // suggest `.unwrap_or_else(f)` + | ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(f)` + +error: use of `unwrap_or` followed by a function call + --> tests/ui/or_fun_call.rs:348:17 + | +LL | let _ = opt.unwrap_or(f() + 1); // suggest `.unwrap_or_else(|| f() + 1)` + | ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| f() + 1)` + +error: use of `unwrap_or` followed by a function call + --> tests/ui/or_fun_call.rs:351:17 + | +LL | let _ = opt.unwrap_or({ + | _________________^ +LL | | let x = f(); +LL | | x + 1 +LL | | }); + | |______^ + | +help: try + | +LL ~ let _ = opt.unwrap_or_else(|| { +LL + let x = f(); +LL + x + 1 +LL ~ }); + | + +error: use of `map_or` followed by a function call + --> tests/ui/or_fun_call.rs:356:17 + | +LL | let _ = opt.map_or(f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)` + | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `map_or_else(|| f() + 1, |v| v)` + +error: use of `unwrap_or` to construct default value + --> tests/ui/or_fun_call.rs:359:17 + | +LL | let _ = opt.unwrap_or({ i32::default() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` + +error: use of `unwrap_or` followed by a function call + --> tests/ui/or_fun_call.rs:365:21 + | +LL | let _ = opt_foo.unwrap_or(Foo { val: String::default() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| Foo { val: String::default() })` + +error: aborting due to 38 previous errors