mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #10930 - y21:issue9956, r=blyxyas,xFrednet

Browse Source 
[`redundant_closure_call`]: handle nested closures

Fixes #9956.

This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x #10922 anyway, so this is probably fine.
(edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 )

Previously, it would suggest changing `(|| || 42)()()` to `|| 42()`, which is a type error (it needs parens: `(|| 42)()`). In my opinion, though, the suggested fix should have really been `42`, so that's what this PR changes.

changelog: [`redundant_closure_call`]: handle nested closures and rewrite as a late lint pass
This commit is contained in:
bors 2023-06-19 20:30:35 +00:00
commit 5b60388e5a
5 changed files with 274 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
clippy_lints/src/lib.rs
View File

@ -804,7 +804,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));
store.register_early_pass(|| Box::new(formatting::Formatting));
store.register_early_pass(|| Box::new(misc_early::MiscEarlyLints));
store.register_early_pass(|| Box::new(redundant_closure_call::RedundantClosureCall));
store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall));
store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
store.register_late_pass(|_| Box::new(returns::Return));

173
clippy_lints/src/redundant_closure_call.rs
View File

@ -1,14 +1,14 @@
use crate::rustc_lint::LintContext;
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
use clippy_utils::get_parent_expr;
use clippy_utils::sugg::Sugg;
use if_chain::if_chain;
use rustc_ast::ast;
use rustc_ast::visit as ast_visit;
use rustc_ast::visit::Visitor as AstVisitor;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::intravisit as hir_visit;
use rustc_hir::intravisit::Visitor as HirVisitor;
use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
use rustc_hir::intravisit::Visitor;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::nested_filter;
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};
@ -51,59 +51,136 @@ fn new() -> Self {
}
}
impl<'ast> ast_visit::Visitor<'ast> for ReturnVisitor {
fn visit_expr(&mut self, ex: &'ast ast::Expr) {
if let ast::ExprKind::Ret(_) | ast::ExprKind::Try(_) = ex.kind {
impl<'tcx> Visitor<'tcx> for ReturnVisitor {
fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) {
if let hir::ExprKind::Ret(_) | hir::ExprKind::Match(.., hir::MatchSource::TryDesugar) = ex.kind {
self.found_return = true;
} else {
hir_visit::walk_expr(self, ex);
}
ast_visit::walk_expr(self, ex);
}
}
impl EarlyLintPass for RedundantClosureCall {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) {
if in_external_macro(cx.sess(), expr.span) {
return;
}
if_chain! {
if let ast::ExprKind::Call(ref paren, _) = expr.kind;
if let ast::ExprKind::Paren(ref closure) = paren.kind;
if let ast::ExprKind::Closure(box ast::Closure { ref asyncness, ref fn_decl, ref body, .. }) = closure.kind;
then {
let mut visitor = ReturnVisitor::new();
visitor.visit_expr(body);
if !visitor.found_return {
span_lint_and_then(
cx,
REDUNDANT_CLOSURE_CALL,
expr.span,
"try not to call a closure in the expression where it is declared",
|diag| {
if fn_decl.inputs.is_empty() {
let mut app = Applicability::MachineApplicable;
let mut hint = Sugg::ast(cx, body, "..", closure.span.ctxt(), &mut app);
if asyncness.is_async() {
// `async x` is a syntax error, so it becomes `async { x }`
if !matches!(body.kind, ast::ExprKind::Block(_, _)) {
hint = hint.blockify();
}
hint = hint.asyncify();
}
diag.span_suggestion(expr.span, "try doing something like", hint.to_string(), app);
}
},
);
}
}
}
/// Checks if the body is owned by an async closure
fn is_async_closure(body: &hir::Body<'_>) -> bool {
if let hir::ExprKind::Closure(closure) = body.value.kind
&& let [resume_ty] = closure.fn_decl.inputs
&& let hir::TyKind::Path(hir::QPath::LangItem(hir::LangItem::ResumeTy, ..)) = resume_ty.kind
{
true
} else {
false
}
}
/// Tries to find the innermost closure:
/// ```rust,ignore
/// (|| || || || 42)()()()()
/// ^^^^^^^^^^^^^^ given this nested closure expression
/// ^^^^^ we want to return this closure
/// ```
/// It also has a parameter for how many steps to go in at most, so as to
/// not take more closures than there are calls.
fn find_innermost_closure<'tcx>(
cx: &LateContext<'tcx>,
mut expr: &'tcx hir::Expr<'tcx>,
mut steps: usize,
) -> Option<(&'tcx hir::Expr<'tcx>, &'tcx hir::FnDecl<'tcx>, hir::IsAsync)> {
let mut data = None;
while let hir::ExprKind::Closure(closure) = expr.kind
&& let body = cx.tcx.hir().body(closure.body)
&& {
let mut visitor = ReturnVisitor::new();
visitor.visit_expr(body.value);
!visitor.found_return
}
&& steps > 0
{
expr = body.value;
data = Some((body.value, closure.fn_decl, if is_async_closure(body) {
hir::IsAsync::Async
} else {
hir::IsAsync::NotAsync
}));
steps -= 1;
}
data
}
/// "Walks up" the chain of calls to find the outermost call expression, and returns the depth:
/// ```rust,ignore
/// (|| || || 3)()()()
/// ^^ this is the call expression we were given
/// ^^ this is what we want to return (and the depth is 3)
/// ```
fn get_parent_call_exprs<'tcx>(
cx: &LateContext<'tcx>,
mut expr: &'tcx hir::Expr<'tcx>,
) -> (&'tcx hir::Expr<'tcx>, usize) {
let mut depth = 1;
while let Some(parent) = get_parent_expr(cx, expr)
&& let hir::ExprKind::Call(recv, _) = parent.kind
&& expr.span == recv.span
{
expr = parent;
depth += 1;
}
(expr, depth)
}
impl<'tcx> LateLintPass<'tcx> for RedundantClosureCall {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
if in_external_macro(cx.sess(), expr.span) {
return;
}
if let hir::ExprKind::Call(recv, _) = expr.kind
// don't lint if the receiver is a call, too.
// we do this in order to prevent linting multiple times; consider:
// `(|| || 1)()()`
// ^^ we only want to lint for this call (but we walk up the calls to consider both calls).
// without this check, we'd end up linting twice.
&& !matches!(recv.kind, hir::ExprKind::Call(..))
&& let (full_expr, call_depth) = get_parent_call_exprs(cx, expr)
&& let Some((body, fn_decl, generator_kind)) = find_innermost_closure(cx, recv, call_depth)
{
span_lint_and_then(
cx,
REDUNDANT_CLOSURE_CALL,
full_expr.span,
"try not to call a closure in the expression where it is declared",
|diag| {
if fn_decl.inputs.is_empty() {
let mut applicability = Applicability::MachineApplicable;
let mut hint = Sugg::hir_with_context(cx, body, full_expr.span.ctxt(), "..", &mut applicability);
if generator_kind.is_async()
&& let hir::ExprKind::Closure(closure) = body.kind
{
let async_closure_body = cx.tcx.hir().body(closure.body);
// `async x` is a syntax error, so it becomes `async { x }`
if !matches!(async_closure_body.value.kind, hir::ExprKind::Block(_, _)) {
hint = hint.blockify();
}
hint = hint.asyncify();
}
diag.span_suggestion(
full_expr.span,
"try doing something like",
hint.maybe_par(),
applicability
);
}
}
);
}
}
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
fn count_closure_usage<'tcx>(
cx: &LateContext<'tcx>,

47
tests/ui/redundant_closure_call_fixable.fixed
View File

@ -3,6 +3,7 @@
#![feature(async_closure)]
#![warn(clippy::redundant_closure_call)]
#![allow(clippy::redundant_async_block)]
#![allow(clippy::type_complexity)]
#![allow(unused)]
async fn something() -> u32 {
@ -38,4 +39,50 @@ fn main() {
};
}
m2!();
issue9956();
}
fn issue9956() {
assert_eq!(43, 42);
// ... and some more interesting cases I've found while implementing the fix
// not actually immediately calling the closure:
let a = (|| 42);
dbg!(a());
// immediately calling it inside of a macro
dbg!(42);
// immediately calling only one closure, so we can't remove the other ones
let a = (|| || 123);
dbg!(a()());
// nested async closures
let a = async { 1 };
let h = async { a.await };
// macro expansion tests
macro_rules! echo {
($e:expr) => {
$e
};
}
let a = 1;
assert_eq!(a, 1);
let a = 123;
assert_eq!(a, 123);
// chaining calls, but not closures
fn x() -> fn() -> fn() -> fn() -> i32 {
|| || || 42
}
let _ = x()()()();
fn bar() -> fn(i32, i32) {
foo
}
fn foo(_: i32, _: i32) {}
bar()(42, 5);
foo(42, 5);
}

47
tests/ui/redundant_closure_call_fixable.rs
View File

@ -3,6 +3,7 @@
#![feature(async_closure)]
#![warn(clippy::redundant_closure_call)]
#![allow(clippy::redundant_async_block)]
#![allow(clippy::type_complexity)]
#![allow(unused)]
async fn something() -> u32 {
@ -38,4 +39,50 @@ macro_rules! m2 {
};
}
m2!();
issue9956();
}
fn issue9956() {
assert_eq!((|| || 43)()(), 42);
// ... and some more interesting cases I've found while implementing the fix
// not actually immediately calling the closure:
let a = (|| 42);
dbg!(a());
// immediately calling it inside of a macro
dbg!((|| 42)());
// immediately calling only one closure, so we can't remove the other ones
let a = (|| || || 123)();
dbg!(a()());
// nested async closures
let a = (|| || || || async || 1)()()()()();
let h = async { a.await };
// macro expansion tests
macro_rules! echo {
($e:expr) => {
$e
};
}
let a = (|| echo!(|| echo!(|| 1)))()()();
assert_eq!(a, 1);
let a = (|| echo!((|| 123)))()();
assert_eq!(a, 123);
// chaining calls, but not closures
fn x() -> fn() -> fn() -> fn() -> i32 {
|| || || 42
}
let _ = x()()()();
fn bar() -> fn(i32, i32) {
foo
}
fn foo(_: i32, _: i32) {}
bar()((|| || 42)()(), 5);
foo((|| || 42)()(), 5);
}

62
tests/ui/redundant_closure_call_fixable.stderr
View File

@ -1,5 +1,5 @@
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:17:13
--> $DIR/redundant_closure_call_fixable.rs:18:13
|
LL | let a = (|| 42)();
| ^^^^^^^^^ help: try doing something like: `42`
@ -7,7 +7,7 @@ LL | let a = (|| 42)();
= note: `-D clippy::redundant-closure-call` implied by `-D warnings`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:18:13
--> $DIR/redundant_closure_call_fixable.rs:19:13
|
LL | let b = (async || {
| _____________^
@ -27,7 +27,7 @@ LL ~ };
|
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:23:13
--> $DIR/redundant_closure_call_fixable.rs:24:13
|
LL | let c = (|| {
| _____________^
@ -47,13 +47,13 @@ LL ~ };
|
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:28:13
--> $DIR/redundant_closure_call_fixable.rs:29:13
|
LL | let d = (async || something().await)();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `async { something().await }`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:37:13
--> $DIR/redundant_closure_call_fixable.rs:38:13
|
LL | (|| m!())()
| ^^^^^^^^^^^ help: try doing something like: `m!()`
@ -64,7 +64,7 @@ LL | m2!();
= note: this error originates in the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:32:13
--> $DIR/redundant_closure_call_fixable.rs:33:13
|
LL | (|| 0)()
| ^^^^^^^^ help: try doing something like: `0`
@ -74,5 +74,53 @@ LL | m2!();
|
= note: this error originates in the macro `m` which comes from the expansion of the macro `m2` (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 6 previous errors
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:46:16
|
LL | assert_eq!((|| || 43)()(), 42);
| ^^^^^^^^^^^^^^ help: try doing something like: `43`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:55:10
|
LL | dbg!((|| 42)());
| ^^^^^^^^^ help: try doing something like: `42`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:58:13
|
LL | let a = (|| || || 123)();
| ^^^^^^^^^^^^^^^^ help: try doing something like: `(|| || 123)`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:62:13
|
LL | let a = (|| || || || async || 1)()()()()();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `async { 1 }`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:71:13
|
LL | let a = (|| echo!(|| echo!(|| 1)))()()();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `1`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:73:13
|
LL | let a = (|| echo!((|| 123)))()();
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try doing something like: `123`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:86:11
|
LL | bar()((|| || 42)()(), 5);
| ^^^^^^^^^^^^^^ help: try doing something like: `42`
error: try not to call a closure in the expression where it is declared
--> $DIR/redundant_closure_call_fixable.rs:87:9
|
LL | foo((|| || 42)()(), 5);
| ^^^^^^^^^^^^^^ help: try doing something like: `42`
error: aborting due to 14 previous errors