Skip to content

fix [redundant_closure] suggesting incorrect code with F: Fn() #12865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions clippy_lints/src/eta_reduction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
ExprKind::Path(QPath::Resolved(..) | QPath::TypeRelative(..))
) =>
{
let callee_ty = typeck.expr_ty(callee).peel_refs();
let callee_ty_raw = typeck.expr_ty(callee);
let callee_ty = callee_ty_raw.peel_refs();
if matches!(type_diagnostic_name(cx, callee_ty), Some(sym::Arc | sym::Rc))
|| !check_inputs(typeck, body.params, None, args)
{
Expand Down Expand Up @@ -170,15 +171,25 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
{
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
if let Ok((ClosureKind::FnMut, _)) = cx.tcx.infer_ctxt().build().type_implements_fn_trait(
cx.param_env,
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
ty::PredicatePolarity::Positive,
) && path_to_local(callee).map_or(false, |l| {
if path_to_local(callee).map_or(false, |l| {
// FIXME: Do we really need this `local_used_in` check?
// Isn't it checking something like... `callee(callee)`?
// If somehow this check is needed, add some test for it,
// 'cuz currently nothing changes after deleting this check.
local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr)
}) {
Comment on lines +174 to 180
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confused, but too scared to remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm also not entirely sure what that is for. Seems like that additional check was introduced in #8685, but that was a rather large refactor. Before this, there was only the local_used_after_expr check. Unfortunate that there's no test for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it might mean situation like this?

let fun = |x: i32| -> i32 {
    x
};

let _ = fun(fun(0));

but I'm not sure, I'll test it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. There is a check somewhere in the if chain that makes sure that the fn arguments are only paths to locals. So in your example that wouldn't happen because fun(0) is more than just a path.

|| !check_inputs(typeck, body.params, None, args)
{
return;
}

matches!(
p.pat.kind,
PatKind::Binding(BindingMode::NONE, id, _, None)
if path_to_local_id(arg, id)
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right... then I'm afraid that I can't think of any examples that pass the compilation check😂

// Mutable closure is used after current expr; we cannot consume it.
snippet = format!("&mut {snippet}");
match cx.tcx.infer_ctxt().build().type_implements_fn_trait(
cx.param_env,
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
ty::PredicatePolarity::Positive,
) {
// Mutable closure is used after current expr; we cannot consume it.
Ok((ClosureKind::FnMut, _)) => snippet = format!("&mut {snippet}"),
Ok((ClosureKind::Fn, _)) if !callee_ty_raw.is_ref() => {
snippet = format!("&{snippet}");
},
_ => (),
}
}
diag.span_suggestion(
expr.span,
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/eta.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -471,3 +471,14 @@ mod issue_10854 {
}
}
}

mod issue_12853 {
fn f_by_value<F: Fn(u32)>(f: F) {
let x = Box::new(|| None.map(&f));
x();
}
fn f_by_ref<F: Fn(u32)>(f: &F) {
let x = Box::new(|| None.map(f));
x();
}
}
11 changes: 11 additions & 0 deletions tests/ui/eta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,3 +471,14 @@ mod issue_10854 {
}
}
}

mod issue_12853 {
fn f_by_value<F: Fn(u32)>(f: F) {
let x = Box::new(|| None.map(|x| f(x)));
x();
}
fn f_by_ref<F: Fn(u32)>(f: &F) {
let x = Box::new(|| None.map(|x| f(x)));
x();
}
}
14 changes: 13 additions & 1 deletion tests/ui/eta.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -190,5 +190,17 @@ error: redundant closure
LL | test.map(|t| t.method())
| ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `crate::issue_10854::d::Test::method`

error: aborting due to 31 previous errors
error: redundant closure
--> tests/ui/eta.rs:477:38
|
LL | let x = Box::new(|| None.map(|x| f(x)));
| ^^^^^^^^ help: replace the closure with the function itself: `&f`

error: redundant closure
--> tests/ui/eta.rs:481:38
|
LL | let x = Box::new(|| None.map(|x| f(x)));
| ^^^^^^^^ help: replace the closure with the function itself: `f`

error: aborting due to 33 previous errors