Add new `redundant_async_block` lint by samueltardieu · Pull Request #10448 · rust-lang/rust-clippy

samueltardieu · 2023-03-04T23:11:33Z

changelog: [redundant_async_block]: new lint to detect async { future.await }

rustbot · 2023-03-04T23:11:38Z

(rustbot has picked a reviewer for you, use r? to override)

llogiq

I like this lint, but I think we can make the message shorter. Also I'd want to see some more tests. Finally, before we introduce a warn-by-default lint, we should at least run a lintcheck to get a handle on churn.

llogiq · 2023-03-04T23:17:13Z

+    /// ### Example
+    /// ```rust
+    /// await {
+    ///     function_returning_a_future().await


Might be good if we had an async fn function_returning_a_future() {} so the example actually compiles. That applies to both code snippets.

llogiq · 2023-03-04T23:19:11Z

+    /// Checks for `async` block that only returns `await` on a future.
+    ///
+    /// ### Why is this bad?
+    /// It is shorter to directly use the future.


I'm curious: Does the async await really get reduced to a single call, or even inlined in release mode? If not, there would even be a perf angle.

I can still see the await process in the MIR expansion in release mode, so I don't think this is optimized away.

llogiq · 2023-03-04T23:23:03Z

+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub REDUNDANT_ASYNC_BLOCK,
+    complexity,


Do we have a lintcheck run to ensure we don't get too much churn when introducing this lint?

Nowadays we are quite cautious when adding warn-by-default lints.

If you are talking about a cargo lintcheck, yes, I ran one (maybe I should mention it in the description). Is there a way to do something similar to a crater run for lint checking?

llogiq · 2023-03-04T23:24:41Z

+                cx,
+                REDUNDANT_ASYNC_BLOCK,
+                expr.span,
+                "this async-await expression is equivalent to the awaited future itself",


Suggested change

"this async-await expression is equivalent to the awaited future itself",

"this async expression only awaits a single future",

llogiq · 2023-03-04T23:31:07Z

+    let fut1 = async { 42 };
+    let fut2 = async move { fut1.await };
+
+    let fut = async { async { 42 }.await };


I'd like to see a test where the future is macro-generated, a test where the whole block outside the future / with the future is macro-generated by an crate-internal/external macro each.

The internal ones may lint, but with the correct snippet; the external ones I think should not lint (because we don't expect people to be able to change code external to their crate). I want to be sure the span.from_expansion() check is sufficient.

Checking inside a macro needs a bit more work, because the awaiten future must not come from a macro parameter, as it might contain .await itself.

Right now, it doesn't look into macros at all. I've added a MISSED OPPORTUNITY marker, but I'm not sure how to check inside the macro that no parameter (or another macro) is used as the .await receiver.

That's all right. 👍

Alexendoo · 2023-03-04T23:45:02Z

+        }
+        if let ExprKind::Async(_, _, block) = &expr.kind && block.stmts.len() == 1 &&
+            let Some(Stmt { kind: StmtKind::Expr(last), .. }) = block.stmts.last() &&
+            let ExprKind::Await(future) = &last.kind


A check for future.span.from_expansion() would be good, and an accompanying test for something like

macro_rules! await_in_macro { ($e:expr) => { std::convert::identity($e).await }; } async { await_in_macro!(foo()) }

This could come up with futures::poll like here but having our own macro definition means we don't have to rely on future's implementation staying the same

You're right, without the test on the future itself the proposed solution involved copying code coming from the macro. Done.

samueltardieu · 2023-03-05T09:49:01Z

In addition to all those comments, there is one check missing: there must be no other .await in the expression, as-in

async { func1(func2().await)).await }

I'll work on that, probably in the next few days.

Edit: done, through a visitor

llogiq · 2023-03-05T19:47:09Z

What did your lintcheck run come up with?

I might do a run to reproduce and see what it turns up.

samueltardieu · 2023-03-05T19:55:34Z

What did your lintcheck run come up with?

Nothing, as expected.

$ cargo lintcheck -j 0 --filter redundant_async_block
[…]
$ cat lintcheck-logs/lintcheck_crates_logs.txt
clippy 0.1.69

### Reports



### Stats:

| lint                                               | count |
| -------------------------------------------------- | ----- |


### ICEs:

I might do a run to reproduce and see what it turns up.

You need to rebase this branch on master to use -j 0, tell me if you want me to push my updated branch.

bors · 2023-03-07T13:55:41Z

☔ The latest upstream changes (presumably #10415) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2023-03-07T14:47:33Z

r=me when rebased.

samueltardieu · 2023-03-07T21:54:26Z

Checked with the 500 most recently downloaded crates of crates.io: this lint was not triggered (the complete 470klines report is available at https://rfc1149.net/tmp/top500_logs.txt.gz).

llogiq · 2023-03-08T09:10:47Z

Yeah, I also ran lintcheck. Thanks everyone!

@bors r+

bors · 2023-03-08T09:10:49Z

📌 Commit 3872d17 has been approved by llogiq

It is now in the queue for this repository.

bors · 2023-03-08T09:19:36Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout issue-10444 (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self issue-10444 --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging clippy_lints/src/lib.rs
CONFLICT (content): Merge conflict in clippy_lints/src/lib.rs
Auto-merging clippy_lints/src/declared_lints.rs
Auto-merging CHANGELOG.md
Automatic merge failed; fix conflicts and then commit the result.

samueltardieu · 2023-03-08T09:49:41Z

@llogiq: rebased

llogiq · 2023-03-08T10:15:23Z

Thanks again!

@bors r+

bors · 2023-03-08T10:15:25Z

📌 Commit d5429ea has been approved by llogiq

It is now in the queue for this repository.

bors · 2023-03-08T10:15:31Z

⌛ Testing commit d5429ea with merge 56fbfe5...

bors · 2023-03-08T10:27:40Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 56fbfe5 to master...

ComputerDruid · 2023-03-15T21:55:52Z

#10509 notes that this lint triggers even in cases where the suggestion is a behavior change.

rustbot assigned llogiq Mar 4, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 4, 2023

samueltardieu force-pushed the issue-10444 branch 2 times, most recently from 984422a to 1bd895a Compare March 4, 2023 23:13

llogiq reviewed Mar 4, 2023

View reviewed changes

Alexendoo reviewed Mar 4, 2023

View reviewed changes

samueltardieu force-pushed the issue-10444 branch 8 times, most recently from 09cc034 to cd8f6db Compare March 5, 2023 11:38

samueltardieu force-pushed the issue-10444 branch from cd8f6db to 3872d17 Compare March 7, 2023 17:54

Add new redundant_async_block lint

d5429ea

samueltardieu force-pushed the issue-10444 branch from 3872d17 to d5429ea Compare March 8, 2023 09:49

bors merged commit 56fbfe5 into rust-lang:master Mar 8, 2023

bors mentioned this pull request Mar 8, 2023

Port clippy away from compiletest to ui_test #10426

Merged

1 task

samueltardieu deleted the issue-10444 branch March 24, 2023 23:47

	"this async-await expression is equivalent to the awaited future itself",
	"this async expression only awaits a single future",

Conversation

samueltardieu commented Mar 4, 2023

Uh oh!

rustbot commented Mar 4, 2023

Uh oh!

llogiq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llogiq commented Mar 5, 2023

Uh oh!

samueltardieu commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Mar 7, 2023

Uh oh!

llogiq commented Mar 7, 2023

Uh oh!

samueltardieu commented Mar 7, 2023

Uh oh!

llogiq commented Mar 8, 2023

Uh oh!

bors commented Mar 8, 2023

Uh oh!

bors commented Mar 8, 2023

Uh oh!

samueltardieu commented Mar 8, 2023

Uh oh!

llogiq commented Mar 8, 2023

Uh oh!

bors commented Mar 8, 2023

Uh oh!

bors commented Mar 8, 2023

Uh oh!

bors commented Mar 8, 2023

Uh oh!

ComputerDruid commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

samueltardieu Mar 5, 2023 •

edited

Loading

samueltardieu commented Mar 5, 2023 •

edited

Loading

samueltardieu commented Mar 5, 2023 •

edited

Loading