Add unused_async_trait_impl lint#16244
Conversation
07c84ff to
29282db
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
265ec66 to
cf7dc77
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ac92856 to
2c59897
Compare
3a8be29 to
c9ee761
Compare
|
Ah, one small thing regarding the PR description:
This is a bit long of a changelog message 😅 You could leave it at just
and put the actual description onto a separate paragraph.
I think this is resolved now? If so, feel free to remove (to reduce clutter in the eventual merge commit message) |
Thank you for your amazing help & feedback! I learned a lot about Clippy thanks to you! |
cdff8eb to
aa69ef5
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
There was a problem hiding this comment.
| // Fetch body snippet and truncate excess indentation. Like this: | ||
| // { | ||
| // 4 | ||
| // } | ||
| let body_snippet = snippet_block_with_applicability(cx, body.value.span, "_", None, &mut app); |
There was a problem hiding this comment.
One way to make this even nicer would be to strip away the curlies if the function body is just a single expression. I'd be insane to ask you to do that now, on top of everything you've already done, so could you please just leave a TODO comment for this?
There was a problem hiding this comment.
Changed it but the expansion is a bit messy. Initially I used the span of the expression, but we are losing any comments that might be been put in the body. I opted to remove the curly braces manually from the original body span.
There was a problem hiding this comment.
See this commit for the difference between expressions & manually trimming the braces
|
This lint has been nominated for inclusion. |
Co-authored-by: Ada Alakbarova <58857108+ada4a@users.noreply.github.com>
c34bc19 to
e1696b9
Compare
Co-authored-by: Ada Alakbarova <58857108+ada4a@users.noreply.github.com>
b90a5ad to
b56d50a
Compare
| && block.stmts.is_empty() | ||
| && block.rules == BlockCheckMode::DefaultBlock | ||
| { | ||
| // Async body is just a simple expression: strip any curly braces. |
There was a problem hiding this comment.
He just went ahead and did it, what a madlad.
Looking at the tests though, it looks like the initial version of this change made it so that the suggestion eats away any comments (and cfg-d out code, etc.), and the current version results in some rather awkward output as well..
I think a nice solution would be to drill down to the final expression (which is what's returned), and just wrap that in a std::future::ready call? One advantage of this is that it would work regardless of whether the function body contains any statements before the expression. And I don't think this would change the behavior of the function any more than the current fix does: if you wrap the whole body in an std::future::ready, its contents are still evaluated eagerly, right?
There was a problem hiding this comment.
... that is way better... in hindsight that's so obvious!
| && let ExprKind::Closure(closure) = body.value.kind | ||
| && let ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)) = closure.kind | ||
| && let body = cx.tcx.hir_body(closure.body) | ||
| && let ExprKind::Block(block, _) = body.value.kind | ||
| && let Some(expr) = block.expr | ||
| && let ExprKind::DropTemps(inner) = expr.kind | ||
| && let ExprKind::Block(block, _) = inner.kind | ||
| && let Some(tail_expr) = block.expr |
There was a problem hiding this comment.
Async fn desugaring is somewhat unintuitive imo... What do you think about adding some documentation, like this perhaps:
| && let ExprKind::Closure(closure) = body.value.kind | |
| && let ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)) = closure.kind | |
| && let body = cx.tcx.hir_body(closure.body) | |
| && let ExprKind::Block(block, _) = body.value.kind | |
| && let Some(expr) = block.expr | |
| && let ExprKind::DropTemps(inner) = expr.kind | |
| && let ExprKind::Block(block, _) = inner.kind | |
| && let Some(tail_expr) = block.expr | |
| // An async function like | |
| // ```rs | |
| // async fn get_random_number() -> i64 { | |
| // do_something(); | |
| // 4 | |
| // } | |
| // ``` | |
| // (roughly) desugars to | |
| // ```rs | |
| // fn get_random_number() -> impl Future<Output = i64> { | |
| // async move { | |
| // do_something(); | |
| // 4 | |
| // } | |
| // } | |
| // ``` | |
| // | |
| // We first get to the `async move {}` block, | |
| // which is the one and only expression in the body of the function.. | |
| && let ExprKind::Closure(closure) = body.value.kind | |
| && let ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)) = closure.kind | |
| && let body = cx.tcx.hir_body(closure.body) | |
| && let ExprKind::Block(block, _) = body.value.kind | |
| && let Some(expr) = block.expr | |
| && let ExprKind::DropTemps(async_move_block) = expr.kind | |
| // .. and then extract the trailing expression of the block (`4` in the | |
| // example above), to wrap it in `std::future::ready` | |
| && let ExprKind::Block(block, _) = async_move_block.kind | |
| && let Some(tail_expr) = block.expr |
| let signature_snippet = snippet_with_applicability(cx, sig.decl.output.span(), "_", &mut app); | ||
| let tail_snippet = snippet_with_applicability(cx, tail_expr.span, "_", &mut app).to_string(); |
There was a problem hiding this comment.
Just in theory, both of these might be macro calls.
For the type, this could look like this: (I concur that this is somewhat contrived)
macro_rules! opt {
($t:ty) => { Option<$t> }
}
async fn foo() -> opt!(u32) {
Some(4)
}And for the trailing expression:
async fn foo() -> Vec<u32> {
vec![]
}Thankfully, the fix is pretty simple:
| let signature_snippet = snippet_with_applicability(cx, sig.decl.output.span(), "_", &mut app); | |
| let tail_snippet = snippet_with_applicability(cx, tail_expr.span, "_", &mut app).to_string(); | |
| let ctxt = impl_item.span.ctxt(); | |
| let signature_snippet = snippet_with_context(cx, sig.decl.output.span(), "_", ctxt, &mut app); | |
| let tail_snippet = snippet_with_context(cx, tail_expr.span, "_", ctxt, &mut app).to_string(); |
(See the documentation on snippet_with_context and the contents of #16809 for more information)
Could you please add the examples above as test cases?
| format!("{builtin_crate}::future::ready({tail_snippet})"), | ||
| ), | ||
| ]; | ||
| diag.help(format!( |
There was a problem hiding this comment.
I think this should be a note, as it doesn't by itself say what to do, but rather gives some context around the fix. See https://doc.rust-lang.org/clippy/development/emitting_lints.html#how-to-choose-between-notes-help-messages-and-suggestions
| diag.help(format!( | |
| diag.note(format!( |
| format!("consider removing the `async` from this function and returning `impl Future<Output = {signature_snippet}>` instead"), | ||
| sugg, | ||
| app | ||
| ); |
There was a problem hiding this comment.
nit: it seems like rustfmt gave up on formatting this part because of a format literal that's too long. Breaking it up should fix this:
| format!("consider removing the `async` from this function and returning `impl Future<Output = {signature_snippet}>` instead"), | |
| sugg, | |
| app | |
| ); | |
| format!("consider removing the `async` from this function \ | |
| and returning `impl Future<Output = {signature_snippet}>` instead"), | |
| sugg, | |
| app | |
| ); |
View all comments
changelog: [
unused_async_trait_impl]: new lintAdds Lint that checks for trait impl functions that are unnecessarily
async. By rewriting them to return acore::future::readyan unnecessary coroutine is prevented.In an embedded context having many trivial async blocks potentially hurts code size significantly. This lint enables projects to quickly find the cases that this applies to.