new lint that detects blocking operations in async by J-ZhengLi · Pull Request #11578 · rust-lang/rust-clippy

J-ZhengLi · 2023-09-28T09:44:41Z

changelog: new lint [unnecessary_blocking_ops]

Ok, this is a rough one, I have no confident in many things (the name ofc), and there are certain things I wanna address before I put too much effort in it.

rustbot · 2023-09-28T09:44:45Z

r? @Jarcho

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

bors · 2023-09-29T03:17:06Z

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

Jarcho

You can avoid the extra HIR traversals by keeping a stack of bodies and whether that body is async. check_body and check_body_post would maintain the stack and check_expr would be used to detect the calls.

You'll also need to make a distinction between async and maybe async contexts. Closures and nested functions inside an async context might run in an async context, but they may also not.

Jarcho · 2023-09-29T13:35:23Z

clippy_lints/src/unnecessary_blocking_ops.rs

+impl<'tcx> LateLintPass<'tcx> for UnnecessaryBlockingOps {
+    fn check_crate(&mut self, cx: &LateContext<'tcx>) {
+        // Avoids processing and storing a long list of paths if this lint was allowed entirely
+        if is_lint_allowed(cx, UNNECESSARY_BLOCKING_OPS, CRATE_HIR_ID) {


Lints levels can change anywhere.

@Jarcho Sorry for late response, I was a bit busy dealing with other stuffs.

I'm not sure if I fully understand this phrase:

Closures and nested functions inside an async context might run in an async context, but they may also not.

So, if there's an async function like this

async fn foo() { fn bar() { // hehe, sleep! std::time::sleep(INF) } }

Then it shouldn't trigger any warning unless the function bar was called in foo later, correct?

That is correct. Unless the call occurs directly within foo there's no way to know if the function is called in an async context. Something like thread::spawn(|| sleep(10)) or thread::spawn(bar) would be fine since the sleep call doesn't block the current thread.

That is correct. Unless the call occurs directly within foo there's no way to know if the function is called in an async context. Something like thread::spawn(|| sleep(10)) or thread::spawn(bar) would be fine since the sleep call doesn't block the current thread.

~~Ok, I think my code logic already prevent linting on such case~~, but I'm not sure, I'll add a few test cases tomorrow just in case

Edit: Actually, calling nested function in async function would have FN, so in the above example code, even if the bar gets called in foo, there wouldn't be any warnings... To lint those I'm afraid I have to loop through each body's parent, take note of the function id if there is one, then keep looping the parent to find an upper block and search every expr inside that block until a call to the noted function id is found? And ofc ignore the calls inside thread::spawn, but I got a feeling this whole thing could take a lot of time 🤔

clippy_lints/src/utils/conf.rs

bors · 2023-10-24T12:12:05Z

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

bors · 2023-11-14T16:10:47Z

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

bors · 2023-12-29T11:06:11Z

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

clippy_lints/src/unnecessary_blocking_ops.rs

teor2345 · 2024-01-20T00:58:14Z

clippy_lints/src/unnecessary_blocking_ops.rs

+    "std::io::copy",
+    "std::io::read_to_string",


These are only blocking if the underlying reader or writer are blocking. For example, copying between Vecs or reading from a Vec into a string isn't blocking in practice.

...only blocking if the underlying reader or writer are blocking.

That's interesting, can you further explain this?

Initially, I list those methods purely base on the observation from the tokio crate, since there are AsyncRead and AsyncWrite in the crate, I assume the Read and Write traits from std::io are blocking 😄 , but now that you mention it, I wonder if copying between slices would actually affect anything.

bors · 2024-01-20T23:11:11Z

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

xFrednet · 2024-03-31T11:53:13Z

Hey @Jarcho, this is a ping from triage. Can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@J-ZhengLi could you rebase on master again? Since only conf.rs and lib.rs need to be updated it should be safe to already review this :D

@rustbot ready

J-ZhengLi · 2024-04-01T02:57:56Z

...
@J-ZhengLi could you rebase on master again? Since only conf.rs and lib.rs need to be updated it should be safe to already review this :D
...

yeah sure, I forgot about this one, still feel very unconfident about this one, running lintcheck gives nothing lol (maybe because not many of those use async at all).

xFrednet · 2024-04-01T09:46:52Z

I believe none of the lints in lintcheck use async extensively, but I could be wrong.

bors · 2024-04-12T19:12:52Z

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

xFrednet · 2024-06-20T20:23:10Z

Hey, this is a ping from triage. @Jarcho can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@J-ZhengLi Sorry for the wait, could you rebase this PR?

@rustbot ready

J-ZhengLi · 2024-07-08T02:11:37Z

Looking at current implementation, combining with the other issue #4377, I kinda want to close this and maybe re-work on it later 🤔

rustbot assigned Jarcho Sep 28, 2023

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

J-ZhengLi force-pushed the issue10794 branch from 1ccb7d4 to 285d0d7 Compare September 28, 2023 09:46

J-ZhengLi changed the title ~~Issue10794~~ new lint that detects blocking operations in async Sep 28, 2023

Jarcho reviewed Sep 29, 2023

View reviewed changes

Jarcho added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 8, 2023

J-ZhengLi force-pushed the issue10794 branch from 285d0d7 to f41378a Compare October 9, 2023 02:17

J-ZhengLi marked this pull request as draft October 23, 2023 10:33

J-ZhengLi force-pushed the issue10794 branch 3 times, most recently from 95ff7c8 to 0bf6239 Compare October 25, 2023 01:55

J-ZhengLi marked this pull request as ready for review October 25, 2023 02:41

J-ZhengLi force-pushed the issue10794 branch 4 times, most recently from 976b2a2 to f26775c Compare November 16, 2023 01:27

J-ZhengLi force-pushed the issue10794 branch 2 times, most recently from 3c822ed to 24f6dcd Compare December 25, 2023 08:30

J-ZhengLi commented Jan 18, 2024

View reviewed changes

clippy_lints/src/unnecessary_blocking_ops.rs Outdated Show resolved Hide resolved

J-ZhengLi force-pushed the issue10794 branch 2 times, most recently from 399780b to 6ac03a4 Compare January 19, 2024 07:00

teor2345 reviewed Jan 20, 2024

View reviewed changes

J-ZhengLi force-pushed the issue10794 branch from 6ac03a4 to 58bc302 Compare January 23, 2024 08:39

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 31, 2024

J-ZhengLi force-pushed the issue10794 branch 3 times, most recently from b1914c3 to 3194a51 Compare April 1, 2024 02:33

J-ZhengLi marked this pull request as draft April 1, 2024 07:59

J-ZhengLi force-pushed the issue10794 branch 2 times, most recently from f90251e to 607ab6b Compare April 1, 2024 09:36

J-ZhengLi marked this pull request as ready for review April 1, 2024 09:46

J-ZhengLi added 3 commits July 8, 2024 10:03

implemented basic lint logic for [unnecessary_blocking_ops]

f50f102

remove hard-coded suggestion, add ui-toml test

fd61204

add SuggestedPath config type;

7beca23

J-ZhengLi force-pushed the issue10794 branch from 607ab6b to 7beca23 Compare July 8, 2024 02:05

J-ZhengLi closed this Jul 8, 2024

Conversation

J-ZhengLi commented Sep 28, 2023

Uh oh!

rustbot commented Sep 28, 2023

Uh oh!

bors commented Sep 29, 2023

Uh oh!

Jarcho left a comment

Choose a reason for hiding this comment

Uh oh!

Jarcho Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

Jarcho Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Nov 1, 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!

Uh oh!

bors commented Oct 24, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

bors commented Dec 29, 2023

Uh oh!

Uh oh!

teor2345 Jan 20, 2024

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 20, 2024

Uh oh!

xFrednet commented Mar 31, 2024

Uh oh!

J-ZhengLi commented Apr 1, 2024

Uh oh!

xFrednet commented Apr 1, 2024

Uh oh!

bors commented Apr 12, 2024

Uh oh!

xFrednet commented Jun 20, 2024

Uh oh!

J-ZhengLi commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

J-ZhengLi Nov 1, 2023 •

edited

Loading