Skip to content

Commit 2fa4dcb

Browse files
committed
Address code reviews.
This commit contains changes addressing some code reviews: - The `Violation::message` implementation doesn't mention that the linter only look for blocking path methods and non-blocking methods are allowed. I've tweaked the comments and added another code example of async function using non-blocking os/pathlib path methods. - The `is_calling_os_path_method` function could be simpler. I've used the suggested implementation and pulled it out from the function since it's only one line now. - The logic to see if an expression is calling a method form the pathlib.Path module could be simpler if it calls `PathlibPathChecker::match_initializer` directly. - The rule function can return early if `maybe_calling_io_operation` returns false when checking the expressions attribute.
1 parent 439fd2a commit 2fa4dcb

File tree

1 file changed

+35
-36
lines changed

1 file changed

+35
-36
lines changed

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

Lines changed: 35 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,15 @@ use ruff_python_semantic::analyze::typing::{TypeChecker, check_type, traverse_un
66
use ruff_text_size::Ranged;
77

88
/// ## What it does
9-
/// Checks that async functions do not use `os.path` or `pathlib.Path`.
9+
/// Checks that async functions do not call blocking `os.path` or `pathlib.Path`
10+
/// methods.
1011
///
1112
/// ## Why is this bad?
12-
/// Calling `os.path` or `pathlib.Path` methods in an async function will block
13+
/// Calling some `os.path` or `pathlib.Path` methods in an async function will block
1314
/// the entire event loop, preventing it from executing other tasks while waiting
1415
/// for the operation. This negates the benefits of asynchronous programming.
1516
///
16-
/// Instead of `os.path` or `pathlib.Path`, use `trio.Path` or `anyio.path` objects.
17+
/// Instead, use the methods' async equivalents from `trio.Path` or `anyio.Path`.
1718
///
1819
/// ## Example
1920
/// ```python
@@ -34,6 +35,17 @@ use ruff_text_size::Ranged;
3435
/// path = trio.Path("my_file.txt")
3536
/// file_exists = await path.exists()
3637
/// ```
38+
///
39+
/// Non-blocking methods are OK to use:
40+
/// ```python
41+
/// import pathlib
42+
///
43+
///
44+
/// async def func():
45+
/// path = pathlib.Path("my_file.txt")
46+
/// file_dirname = path.dirname()
47+
/// new_path = os.path.join("/tmp/src/", path)
48+
/// ```
3749
#[derive(ViolationMetadata)]
3850
pub(crate) struct BlockingPathMethodInAsyncFunction {
3951
path_library: String,
@@ -106,16 +118,6 @@ impl TypeChecker for PathlibPathChecker {
106118
}
107119
}
108120

109-
fn is_calling_os_path_method(segments: &[&str]) -> bool {
110-
if segments.len() != 3 {
111-
return false;
112-
}
113-
let Some(symbol_name) = segments.get(..2) else {
114-
return false;
115-
};
116-
matches!(symbol_name, ["os", "path"])
117-
}
118-
119121
fn maybe_calling_io_operation(attr: &str) -> bool {
120122
!matches!(
121123
attr,
@@ -170,7 +172,7 @@ pub(crate) fn blocking_os_path(checker: &Checker, call: &ExprCall) {
170172
// early in that scenario.
171173
if let Some(qualified_name) = semantic.resolve_qualified_name(call.func.as_ref()) {
172174
let segments = qualified_name.segments();
173-
if !is_calling_os_path_method(segments) {
175+
if !matches!(segments, ["os", "path", _]) {
174176
return;
175177
}
176178

@@ -189,24 +191,23 @@ pub(crate) fn blocking_os_path(checker: &Checker, call: &ExprCall) {
189191
return;
190192
}
191193

192-
// Check if an expression is a pathlib.Path constructor that directly
193-
// calls an I/O method.
194194
let Some(ast::ExprAttribute { value, attr, .. }) = call.func.as_attribute_expr() else {
195195
return;
196196
};
197197

198-
if let Some(ExprCall { func, .. }) = value.as_call_expr() {
199-
if !PathlibPathChecker::is_pathlib_path_constructor(semantic, func) {
200-
return;
201-
}
202-
if maybe_calling_io_operation(attr.id.as_str()) {
203-
checker.report_diagnostic(
204-
BlockingPathMethodInAsyncFunction {
205-
path_library: "pathlib.Path".to_string(),
206-
},
207-
call.func.range(),
208-
);
209-
}
198+
if !maybe_calling_io_operation(attr.id.as_str()) {
199+
return;
200+
}
201+
202+
// Check if an expression is a pathlib.Path constructor that directly
203+
// calls an I/O method.
204+
if PathlibPathChecker::match_initializer(value, semantic) {
205+
checker.report_diagnostic(
206+
BlockingPathMethodInAsyncFunction {
207+
path_library: "pathlib.Path".to_string(),
208+
},
209+
call.func.range(),
210+
);
210211
return;
211212
}
212213

@@ -221,13 +222,11 @@ pub(crate) fn blocking_os_path(checker: &Checker, call: &ExprCall) {
221222
};
222223

223224
if check_type::<PathlibPathChecker>(binding, semantic) {
224-
if maybe_calling_io_operation(attr.id.as_str()) {
225-
checker.report_diagnostic(
226-
BlockingPathMethodInAsyncFunction {
227-
path_library: "pathlib.Path".to_string(),
228-
},
229-
call.func.range(),
230-
);
231-
}
225+
checker.report_diagnostic(
226+
BlockingPathMethodInAsyncFunction {
227+
path_library: "pathlib.Path".to_string(),
228+
},
229+
call.func.range(),
230+
);
232231
}
233232
}

0 commit comments

Comments
 (0)