Skip to content

Commit

Permalink
Auto merge of rust-lang#13085 - J-ZhengLi:issue12973, r=llogiq
Browse files Browse the repository at this point in the history 
make [`or_fun_call`] recursive.

fixes: rust-lang#12973

---

changelog: make [`or_fun_call`] recursive.
  • Loading branch information
@bors
bors committed Jul 13, 2024
2 parents 0cbbee1 + cc1bb8f commit 1aac778
Show file tree
Hide file tree
Showing 4 changed files with 183 additions and 47 deletions.
110 changes: 64 additions & 46 deletions clippy_lints/src/methods/or_fun_call.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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<'_>,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -135,7 +139,7 @@ pub(super) fn check<'tcx>(
span: Span,
// None if lambda is required
fun_span: Option<Span>,
) {
) -> bool {
// (path, fn_has_argument, methods, suffix)
const KNOW_TYPES: [(Symbol, bool, &[&str], &str); 4] = [
(sym::BTreeEntry, false, &["or_insert"], "with"),
Expand Down Expand Up @@ -185,54 +189,68 @@ pub(super) fn check<'tcx>(
format!("{name}_{suffix}({sugg})"),
app,
);
}
}

let extract_inner_arg = |arg: &'tcx hir::Expr<'_>| {
if let hir::ExprKind::Block(
hir::Block {
stmts: [],
expr: Some(expr),
..
},
_,
) = arg.kind
{
expr
true
} else {
arg
false
}
};
}

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(())
});
}
}

Expand Down
35 changes: 35 additions & 0 deletions tests/ui/or_fun_call.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<i32> = 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() {}
35 changes: 35 additions & 0 deletions tests/ui/or_fun_call.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<i32> = 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() {}
50 changes: 49 additions & 1 deletion tests/ui/or_fun_call.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 1aac778

Please sign in to comment.