[`flake8-async`] Extend `ASYNC230` to detect blocking `pathlib` I/O methods by aidandj · Pull Request #19619 · astral-sh/ruff

aidandj · 2025-07-29T17:07:48Z

Summary

Add detection for pathlib's blocking I/O methods (read_text, read_bytes, write_text, write_bytes) in async functions, beyond just the open() method. Under the hood, these methods call open, so they have the same issue that open does.

Test Plan

Added test cases to the existing ASYNC230 test suite and updated the snapshot.

Add detection for pathlib's blocking I/O methods (read_text, read_bytes, write_text, write_bytes) in async functions, beyond just the open() method. Signed-off-by: Aidan Jensen <[email protected]>

ntBre

Thanks! This change makes sense to me, but I think we should preview-gate it since the rule itself is stable.

Otherwise I just had a couple of small nits.

ntBre · 2025-07-29T18:42:35Z

crates/ruff_linter/src/rules/flake8_async/rules/blocking_open_call.rs

+    if attr.as_str() != "open"
+        && attr.as_str() != "read_text"
+        && attr.as_str() != "read_bytes"
+        && attr.as_str() != "write_text"
+        && attr.as_str() != "write_bytes"
+    {


I think we could use matches! here:

Suggested change

if attr.as_str() != "open"

&& attr.as_str() != "read_text"

&& attr.as_str() != "read_bytes"

&& attr.as_str() != "write_text"

&& attr.as_str() != "write_bytes"

{

if !matches!(attr.as_str(), "open"

| "read_text"

| "read_bytes"

| "write_text"

| "write_bytes")

{

However, we will need two checks here for now because I think we should gate this behind preview since it's an expansion of a stable rule. You can see examples of these in preview.rs:

ruff/crates/ruff_linter/src/preview.rs

Lines 224 to 227 in d449c54

// https://github.com/astral-sh/ruff/pull/19100

pub(crate) const fn is_add_future_annotations_imports_enabled(settings: &LinterSettings) -> bool {

settings.preview.is_enabled()

}

and references to those functions.

Thanks for the matches suggestion. Rust newbie here, still learning.

I'll look into the preview stuff.

ntBre · 2025-07-29T18:43:08Z

crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC230.py

 def foo_sync():
    open("")

+


Can you revert this? It will make the snapshots a bit easier to review since it won't shift everything down :)

Sorry, I kept trying to revert this but my formatter kept adding it back. I'll revert it again.

No worries, I habitually hit my formatter binding and always have to hold back when modifying snapshots too.

Signed-off-by: Aidan Jensen <[email protected]>

aidandj · 2025-07-29T19:39:39Z

I did a double check in the pathlib library and found touch as well calls io.open. Added that in there.

I found a pattern of enabling preview tests in another file and copied that over, if there is a better method for preview tests let me know. It would be nice to tie them to those helper methods so that when you deleted the helper method you would know to move the test out of the preview call. Maybe that could be as simple as calling the helper function in the preview test to bring attention to it.

Also, who's responsibility is it to take it out of preview? The PR author? Or the ruff team?

Signed-off-by: Aidan Jensen <[email protected]>

MichaReiser · 2025-07-30T07:00:41Z

Also, who's responsibility is it to take it out of preview? The PR author? Or the ruff team?

We'll take it out of preview, following our versioning policy.

Is this change in line with the upstream ASYNC rule?

ntBre · 2025-07-30T12:14:46Z

Is this change in line with the upstream ASYNC rule?

I should have mentioned this somewhere, but I looked into the upstream rule yesterday, and it looks like they only flag open, io.open, and io.open_code, so I concluded that we'd already diverged from upstream by handling pathlib.Path.open. But I'm certainly open to your input.

https://github.com/python-trio/flake8-async/blob/1bf61068d7cdaa25b8bdb91cbff017a785df1220/flake8_async/visitors/visitor2xx.py#L277-L278

MichaReiser · 2025-08-04T08:29:56Z

...es/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC230_ASYNC230.py.snap

+62 | async def func() -> None:
+63 |     p = Path("foo").read_text()  # ASYNC230
+   |         ^^^^^^^^^^^^^^^^^^^^^ ASYNC230
+   |


The diagnostics here are fairly confusing as it's not clear to me where I'm calling open. Can we add a help text saying that read_text calls open internally`. We may also want to update the rule's documentation to mention the new (preview only behavior)

Another option would be to simplify and just remove the mention of open. And just say blocking methods.

MichaReiser · 2025-08-04T08:40:25Z

I should have mentioned this somewhere, but I looked into the upstream rule yesterday, and it looks like they only flag open, io.open, and io.open_code, so I concluded that we'd already diverged from upstream by handling pathlib.Path.open. But I'm certainly open to your input.

Considering Path.open does make sense and seems less of a divergence to me than including extra methods. While the new methods are in the spirit of the rule, it does push its boundaries, e.g., a better rule name might be blocking-file-operation-in-async-function as the problem isn't just open (I suspect it's any file IO operation)?

Unlike other rules, the upstream async linter is very actively maintained. That makes it more likely that we'll run into conflicting rules.

I tried to find a related issue or PR upstream but wasn't successful. @Zac-HD: Is this something that has come up with the async rules before? Is this a change you would be open to also make in flake8-async (I'm not expecting you to make the change yourself, just if it's in line with how you designed the rules)?

Zac-HD · 2025-08-04T16:42:23Z

I think this'd be the first time ruff was ahead of flake8-async, nice!

How about we move this new logic to a new ASYNC233 code, and open an upstream issue to implement it / reserve the number? I think this is a much better experience for users updating their linter too, since they can use some temporary ignores without missing the current 230 issues.

aidandj · 2025-08-04T16:59:00Z

I think this'd be the first time ruff was ahead of flake8-async, nice!

How about we move this new logic to a new ASYNC233 code, and open an upstream issue to implement it / reserve the number? I think this is a much better experience for users updating their linter too, since they can use some temporary ignores without missing the current 230 issues.

This seems reasonable. Let me know if there is consensus and I could take a stab at that as well

Zac-HD · 2025-08-04T20:19:37Z

Upstream issue opened!

Practically speaking, if you're interested in implementing more rules I'd pretty strongly prefer pulling more into ruff relative to moving this one in the other direction; I already mostly use ruff for speed 😁

MichaReiser · 2025-08-05T08:16:56Z

Thanks @Zac-HD

Practically speaking, if you're interested in implementing more rules I'd pretty strongly prefer #8451 relative to moving this one in the other direction; I already mostly use ruff for speed 😁

We're interested, it's just hard to prioritize with everything else going but we could mark some of those as help wanted (CC: @ntBre wdty?)

A new code sounds good to me. But this may justify removing the exsting pathlib check from ASYNC230 (under preview)

jakkdl · 2025-08-05T10:52:48Z

Thanks @Zac-HD

Practically speaking, if you're interested in implementing more rules I'd pretty strongly prefer #8451 relative to moving this one in the other direction; I already mostly use ruff for speed 😁

We're interested, it's just hard to prioritize with everything else going but we could mark some of those as help wanted (CC: @ntBre wdty?)

A new code sounds good to me. But this may justify removing the exsting pathlib check from ASYNC230 (under preview)

I don't think we have any preference between adding pathlib.Path.open to ASYNC230 vs adding all pathlib methods to an ASYNC233 in upstream, so we can simply sync to what you end up deciding to do.

ntBre · 2025-08-05T12:30:20Z

We're interested, it's just hard to prioritize with everything else going but we could mark some of those as help wanted (CC: @ntBre wdty?)

Sounds good to me!

Thank y'all!

ntBre · 2026-01-27T14:41:55Z

Thanks again for working on this! I'll go ahead and close this PR for now as it's gotten stale, but feel free to open another one implementing a new ASYNC233 rule as discussed above.

[flake8-async] Extend ASYNC230 to detect blocking pathlib I/O methods

dc44da1

Add detection for pathlib's blocking I/O methods (read_text, read_bytes, write_text, write_bytes) in async functions, beyond just the open() method. Signed-off-by: Aidan Jensen <[email protected]>

ntBre reviewed Jul 29, 2025

View reviewed changes

ntBre added preview Related to preview mode features rule Implementing or modifying a lint rule labels Jul 29, 2025

ntBre changed the title ~~[flake8-async] Extend ASYNC230 to detect blocking pathlib I/O methods~~ [flake8-async] Extend ASYNC230 to detect blocking pathlib I/O methods Jul 29, 2025

aidandj added 2 commits July 29, 2025 11:55

revert formatter change in test file

ea8c4bb

Signed-off-by: Aidan Jensen <[email protected]>

Put new checks under preview

86af1d5

Signed-off-by: Aidan Jensen <[email protected]>

aidandj requested a review from ntBre July 29, 2025 19:33

Add touch and remove non-preview test case from debugging steps

9b22fc6

Signed-off-by: Aidan Jensen <[email protected]>

fix test file formatting

08d53ae

Signed-off-by: Aidan Jensen <[email protected]>

MichaReiser reviewed Aug 4, 2025

View reviewed changes

Zac-HD mentioned this pull request Aug 4, 2025

New rule: ASYNC233 for blocking pathlib methods python-trio/flake8-async#396

Open

ntBre closed this Jan 27, 2026

ntBre mentioned this pull request Jan 27, 2026

Implement flake8-async rules #8451

Open

38 tasks

	// https://github.com/astral-sh/ruff/pull/19100
	pub(crate) const fn is_add_future_annotations_imports_enabled(settings: &LinterSettings) -> bool {
	settings.preview.is_enabled()
	}

Comments

Conversation

aidandj commented Jul 29, 2025

Summary

Test Plan

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

ntBre Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

aidandj Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

aidandj Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

aidandj commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Jul 30, 2025

Uh oh!

ntBre commented Jul 30, 2025

Uh oh!

MichaReiser Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

aidandj Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zac-HD commented Aug 4, 2025

Uh oh!

aidandj commented Aug 4, 2025

Uh oh!

Zac-HD commented Aug 4, 2025

Uh oh!

MichaReiser commented Aug 5, 2025

Uh oh!

jakkdl commented Aug 5, 2025

Uh oh!

ntBre commented Aug 5, 2025

Uh oh!

ntBre commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aidandj commented Jul 29, 2025 •

edited

Loading

MichaReiser commented Aug 4, 2025 •

edited

Loading