Fix unused_async_trait_impl suggestions with return statements#17181
Conversation
|
r? @dswij rustbot has assigned @dswij. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
I'd say feel free to assign fixes to this lint to me? I can generally go through them pretty quickly
r? @ada4a
| // Unlikely, but someone could potentially hide another return statement in this expression. | ||
| walk_expr(self, expr); |
There was a problem hiding this comment.
wanna have a test case for this as well? Something like:
async fn foo() -> u32 {
if a {
return {
if b {
return 1;
}
2
};
}
3
}| let mut visitor = ReturnValueVisitor { exprs: vec![] }; | ||
| visitor.visit_block(block); | ||
|
|
||
| sugg.extend(visitor.exprs.into_iter().filter_map(|expr| { | ||
| walk_span_to_context(expr.span, ctxt).map(|expr_span| { | ||
| let expr_snippet = | ||
| snippet_with_applicability(cx, expr_span, "_", &mut app).to_string(); | ||
|
|
||
| (expr_span, format!("{builtin_crate}::future::ready({expr_snippet})")) | ||
| }) | ||
| })); |
There was a problem hiding this comment.
this would be great to put a comment on, something like:
// Additionally, find all early-returns in the function body, and wrap those in `ready` as well| | |_________^ | ||
| | | ||
| = note: `std::future::ready` creates a `Future` which returns the value immediately when `poll`ed | ||
| help: consider removing the `async` from this function and returning `impl Future<Output = u32>` instead |
There was a problem hiding this comment.
since we're changing more things here, it imo becomes less clear what's actually happening -- maybe we should add , using `{std_or_core}::future::ready` to the suggestion message?
| // Test that local functions are not touched by the suggestion. | ||
| fn local_func() -> u32 { | ||
| if 5 == 2 { | ||
| return 1; | ||
| } | ||
| 2 | ||
| } | ||
|
|
||
| // Test that we do not change the tail expr or return in a (unrelated) closure. | ||
| let f = || { | ||
| if 5 == 2 { | ||
| return 1; | ||
| } | ||
| 2 | ||
| }; |
There was a problem hiding this comment.
huh, I wouldn't even have thought of this! well done:)
| visitor.visit_block(block); | ||
|
|
||
| sugg.extend(visitor.exprs.into_iter().filter_map(|expr| { | ||
| walk_span_to_context(expr.span, ctxt).map(|expr_span| { |
There was a problem hiding this comment.
hm, wouldn't be great to just ignore expressions where span walking doesn't work.. maybe do something like this?
for expr in visitor.exprs {
if let Some(expr_span) = walk_span_to_context(expr.span, ctxt) {
/* add snippet, as before */
} else {
app = app.min(Applicability::Unspecified);
}
}|
Reminder, once the PR becomes ready for a review, use |
|
oh no, I was too late... |
changelog: [
unused_async_trait_impl]: fix suggestions for statements containingreturnFixes #17179.