Skip to content
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.

Sign up for GitHub

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

redundant_closure fix FP on coerced closure #8431

Merged
merged 1 commit into from
Apr 27, 2022
Merged

redundant_closure fix FP on coerced closure #8431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion clippy_lints/src/eta_reduction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use clippy_utils::higher::VecArgs;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::usage::local_used_after_expr;
use clippy_utils::{higher, path_to_local, path_to_local_id};
use clippy_utils::{higher, is_adjusted, path_to_local, path_to_local_id};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
Expand Down Expand Up @@ -103,6 +103,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
let closure_ty = cx.typeck_results().expr_ty(expr);

if_chain!(
if !is_adjusted(cx, &body.value);
if let ExprKind::Call(callee, args) = body.value.kind;
if let ExprKind::Path(_) = callee.kind;
if check_inputs(cx, body.params, args);
Expand Down Expand Up @@ -144,6 +145,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
);

if_chain!(
if !is_adjusted(cx, &body.value);
Copy link
Member

@xFrednet xFrednet Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think that is_adjusted is a bit too restrictive. The example cases all are related to the never type. Therefore, I would only suppress the lint if the wrapped function returns the never type, in all other cases it seems like the normal adjustment has worked its magic.

Have you tried the following two options?

  1. Check the return type of the body expression of the closure. The expression might still return the never type if it gets adjusted, but I'm not sure about that.
  2. Otherwise we might have to resolve the referenced function and check what that returns.

Copy link
Member Author

Choose a reason for hiding this comment

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

The example cases all are related to the never type.

The problem is not never type, but it's when the wrapped function can only be resolved by typechecks by adjusting it to return () type.

Check the return type of the body expression of the closure. The expression might still return the never type if it gets adjusted, but I'm not sure about that.

It gets adjusted to () type.

Otherwise we might have to resolve the referenced function and check what that returns.

Yes, the only way that I see for a complete solution is to check if the function typecheck can be resolved to accept the original return type of the closure. I'm not sure this is trivial to do.

Copy link
Member

Choose a reason for hiding this comment

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

The example cases all are related to the never type.

The problem is not never type, but it's when the wrapped function can only be resolved by typechecks by adjusting it to return () type.

Looks like I also misunderstood the issue at first. The type adjustment to () sounds like a quirk of the type checking system.

In that case, I think that your fix is the correct one. It will have some FNs but those are better than FPs IMO. 🙃

if let ExprKind::MethodCall(path, args, _) = body.value.kind;
if check_inputs(cx, body.params, args);
let method_def_id = cx.typeck_results().type_dependent_def_id(body.value.hir_id).unwrap();
Expand Down
16 changes: 14 additions & 2 deletions tests/ui/eta.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn main() {
}

// See #815
let e = Some(1u8).map(divergent);
let e = Some(1u8).map(|a| divergent(a));
let e = Some(1u8).map(generic);
let e = Some(1u8).map(generic);
// See #515
Expand Down Expand Up @@ -233,7 +233,7 @@ fn late_bound_lifetimes() {
{
}
map_str(|s| take_asref_path(s));
map_str_to_path(std::convert::AsRef::as_ref);
map_str_to_path(|s| s.as_ref());
}

mod type_param_bound {
Expand Down Expand Up @@ -279,3 +279,15 @@ mod bind_by_ref {
Some(A).map(|ref a| B::from(a));
}
}

// #7812 False positive on coerced closure
fn coerced_closure() {
fn function_returning_unit<F: FnMut(i32)>(f: F) {}
function_returning_unit(|x| std::process::exit(x));

fn arr() -> &'static [u8; 0] {
&[]
}
fn slice_fn(_: impl FnOnce() -> &'static [u8]) {}
slice_fn(|| arr());
}
12 changes: 12 additions & 0 deletions tests/ui/eta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,15 @@ mod bind_by_ref {
Some(A).map(|ref a| B::from(a));
}
}

// #7812 False positive on coerced closure
fn coerced_closure() {
fn function_returning_unit<F: FnMut(i32)>(f: F) {}
function_returning_unit(|x| std::process::exit(x));

fn arr() -> &'static [u8; 0] {
&[]
}
fn slice_fn(_: impl FnOnce() -> &'static [u8]) {}
slice_fn(|| arr());
}
14 changes: 1 addition & 13 deletions tests/ui/eta.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ error: redundant closure
LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
| ^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `below`

error: redundant closure
--> $DIR/eta.rs:40:27
|
LL | let e = Some(1u8).map(|a| divergent(a));
| ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `divergent`

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a false negative because map allows a function with any return type but the bug in the example expects a function that returns nothing (or unit). It seems like it should be possible to detect the difference. Maybe there is an expr type adjustment to coerce ! to () on the closure or the function call within the closure. Or maybe you can detect this scenario by comparing call_ty and closure_ty (one of them has -> () and the other has -> !?).

Copy link
Member Author

@dswij dswij Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I definitely misunderstood the bug in the example. I will take a further look. Thanks!

Copy link
Member Author

@dswij dswij Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like checking for coerced closure is not enough. In the case above, it seems like the closure is coerced to () before being passed on to map (not sure why this is the case, though). AFAIU, in the example above, map accepts divergent nonetheless, probably because of generics. For non-generics, this is not the case and type check wouldn't resolve (hence the problem with issue #8416 and similar, see playground).

Ignoring coerced closures is not hard, but it does produce false negatives such as the case above. Although I'd say that this is still an improvement over all the false positives.

A full solution would be to check if type-check is resolvable for the call/method-call taking Fn(..) -> T without the coerced closure, but I'm not sure that this is trivial to do.

error: redundant closure
--> $DIR/eta.rs:41:27
|
Expand Down Expand Up @@ -122,11 +116,5 @@ error: redundant closure
LL | Some(1).map(|n| in_loop(n));
| ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `in_loop`

error: redundant closure
--> $DIR/eta.rs:236:21
|
LL | map_str_to_path(|s| s.as_ref());
| ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::convert::AsRef::as_ref`

error: aborting due to 21 previous errors
error: aborting due to 19 previous errors