From 4ce3037fd8f97f23cabd39f87d86bf19e3b859dc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 30 Jan 2024 12:37:06 -0500 Subject: [PATCH] Tweak semantic lookup --- .../test/fixtures/flake8_async/ASYNC101.py | 60 +++++---- .../rules/open_sleep_or_subprocess_call.rs | 116 ++++++++---------- ...e8_async__tests__ASYNC101_ASYNC101.py.snap | 84 ++++++------- 3 files changed, 132 insertions(+), 128 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC101.py b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC101.py index 4d7efc432a8da..4c4f78bd452a7 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC101.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC101.py @@ -5,69 +5,83 @@ # Violation cases: -async def foo(): + +async def func(): open("foo") -async def foo(): +async def func(): time.sleep(1) -async def foo(): +async def func(): subprocess.run("foo") -async def foo(): +async def func(): subprocess.call("foo") -async def foo(): +async def func(): subprocess.foo(0) -async def foo(): +async def func(): os.wait4(10) -async def foo(): +async def func(): os.wait(12) + # Violation cases for pathlib: - -async def foo(): - Path("foo").open() # ASYNC101 -async def foo(): + +async def func(): + Path("foo").open() # ASYNC101 + + +async def func(): p = Path("foo") - p.open() # ASYNC101 + p.open() # ASYNC101 + -async def foo(): - with Path("foo").open() as f: # ASYNC101 +async def func(): + with Path("foo").open() as f: # ASYNC101 pass -async def foo() -> None: +async def func() -> None: p = Path("foo") async def bar(): - p.open() # ASYNC101 + p.open() # ASYNC101 + + +async def func() -> None: + (p1, p2) = (Path("foo"), Path("bar")) + + p1.open() # ASYNC101 # Non-violation cases for pathlib: - + class Foo: def open(self): pass -async def foo(): - Foo().open() # OK +async def func(): + Foo().open() # OK -async def foo(): + +async def func(): def open(): pass - open() # OK -def foo(): - Path("foo").open() # OK \ No newline at end of file + open() # OK + + +def func(): + Path("foo").open() # OK diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/open_sleep_or_subprocess_call.rs b/crates/ruff_linter/src/rules/flake8_async/rules/open_sleep_or_subprocess_call.rs index 5a879e63f9d22..604d89b3253bc 100644 --- a/crates/ruff_linter/src/rules/flake8_async/rules/open_sleep_or_subprocess_call.rs +++ b/crates/ruff_linter/src/rules/flake8_async/rules/open_sleep_or_subprocess_call.rs @@ -1,9 +1,7 @@ -use ruff_python_ast::{Expr, ExprAttribute, ExprCall, ExprName, Stmt, StmtAssign}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::call_path::CallPath; -use ruff_python_semantic::SemanticModel; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::{analyze, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -42,50 +40,47 @@ impl Violation for OpenSleepOrSubprocessInAsyncFunction { } /// ASYNC101 -pub(crate) fn open_sleep_or_subprocess_call(checker: &mut Checker, call: &ExprCall) { - let should_add_to_diagnostics = if checker.semantic().in_async_context() { - if let Some(call_path) = checker.semantic().resolve_call_path(call.func.as_ref()) { - is_open_sleep_or_subprocess_call(&call_path) - || is_open_call_from_pathlib(call.func.as_ref(), checker.semantic()) - } else { - is_open_call_from_pathlib(call.func.as_ref(), checker.semantic()) - } - } else { - false - }; - - if !should_add_to_diagnostics { +pub(crate) fn open_sleep_or_subprocess_call(checker: &mut Checker, call: &ast::ExprCall) { + if !checker.semantic().in_async_context() { return; } - checker.diagnostics.push(Diagnostic::new( - OpenSleepOrSubprocessInAsyncFunction, - call.func.range(), - )); + + if is_open_sleep_or_subprocess_call(&call.func, checker.semantic()) + || is_open_call_from_pathlib(call.func.as_ref(), checker.semantic()) + { + checker.diagnostics.push(Diagnostic::new( + OpenSleepOrSubprocessInAsyncFunction, + call.func.range(), + )); + } } -fn is_open_sleep_or_subprocess_call(call_path: &CallPath) -> bool { - matches!( - call_path.as_slice(), - ["", "open"] - | ["time", "sleep"] - | [ - "subprocess", - "run" - | "Popen" - | "call" - | "check_call" - | "check_output" - | "getoutput" - | "getstatusoutput" - ] - | ["os", "wait" | "wait3" | "wait4" | "waitid" | "waitpid"] - ) +/// Returns `true` if the expression resolves to a blocking call, like `time.sleep` or +/// `subprocess.run`. +fn is_open_sleep_or_subprocess_call(func: &Expr, semantic: &SemanticModel) -> bool { + semantic.resolve_call_path(func).is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["", "open"] + | ["time", "sleep"] + | [ + "subprocess", + "run" + | "Popen" + | "call" + | "check_call" + | "check_output" + | "getoutput" + | "getstatusoutput" + ] + | ["os", "wait" | "wait3" | "wait4" | "waitid" | "waitpid"] + ) + }) } -/// Analyze if the open call is from `pathlib.Path` -/// PTH123 (builtin-open) suggests to use `pathlib.Path.open` instead of `open` +/// Returns `true` if an expression resolves to a call to `pathlib.Path.open`. fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool { - let Expr::Attribute(ExprAttribute { attr, value, .. }) = func else { + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else { return false; }; @@ -93,10 +88,10 @@ fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool { return false; } - // Check first if the call is from `pathlib.Path.open`: + // First: is this an inlined call to `pathlib.Path.open`? // ```python - // from pathlib import Path - // Path("foo").open() + // from pathlib import Path + // Path("foo").open() // ``` if let Expr::Call(call) = value.as_ref() { let Some(call_path) = semantic.resolve_call_path(call.func.as_ref()) else { @@ -107,35 +102,28 @@ fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool { } } - // Otherwise, check if Path.open call is bind to a variable: + // Second, is this a call to `pathlib.Path.open` via a variable? // ```python - // from pathlib import Path - // p = Path("foo") - // p.open() - // - let Expr::Name(ExprName { id, .. }) = value.as_ref() else { + // from pathlib import Path + // path = Path("foo") + // path.open() + // ``` + let Expr::Name(name) = value.as_ref() else { return false; }; - let Some(binding_id) = semantic.lookup_symbol(id) else { + let Some(binding_id) = semantic.resolve_name(name) else { return false; }; - let Some(node_id) = semantic.binding(binding_id).source else { - return false; - }; + let binding = semantic.binding(binding_id); - let Stmt::Assign(StmtAssign { value, .. }) = semantic.statement(node_id) else { + let Some(Expr::Call(call)) = analyze::typing::find_binding_value(&name.id, binding, semantic) + else { return false; }; - if let Expr::Call(call) = value.as_ref() { - let Some(call_path) = semantic.resolve_call_path(call.func.as_ref()) else { - return false; - }; - if call_path.as_slice() == ["pathlib", "Path"] { - return true; - } - } - false + semantic + .resolve_call_path(call.func.as_ref()) + .is_some_and(|call_path| call_path.as_slice() == ["pathlib", "Path"]) } diff --git a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC101_ASYNC101.py.snap b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC101_ASYNC101.py.snap index 9a0a13411e73c..969e9ec1f48f2 100644 --- a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC101_ASYNC101.py.snap +++ b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC101_ASYNC101.py.snap @@ -1,82 +1,84 @@ --- source: crates/ruff_linter/src/rules/flake8_async/mod.rs --- -ASYNC101.py:9:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods - | -8 | async def foo(): -9 | open("foo") - | ^^^^ ASYNC101 - | +ASYNC101.py:10:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods + | + 9 | async def func(): +10 | open("foo") + | ^^^^ ASYNC101 + | -ASYNC101.py:13:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:14:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -12 | async def foo(): -13 | time.sleep(1) +13 | async def func(): +14 | time.sleep(1) | ^^^^^^^^^^ ASYNC101 | -ASYNC101.py:17:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:18:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -16 | async def foo(): -17 | subprocess.run("foo") +17 | async def func(): +18 | subprocess.run("foo") | ^^^^^^^^^^^^^^ ASYNC101 | -ASYNC101.py:21:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:22:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -20 | async def foo(): -21 | subprocess.call("foo") +21 | async def func(): +22 | subprocess.call("foo") | ^^^^^^^^^^^^^^^ ASYNC101 | -ASYNC101.py:29:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:30:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -28 | async def foo(): -29 | os.wait4(10) +29 | async def func(): +30 | os.wait4(10) | ^^^^^^^^ ASYNC101 | -ASYNC101.py:33:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:34:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -32 | async def foo(): -33 | os.wait(12) +33 | async def func(): +34 | os.wait(12) | ^^^^^^^ ASYNC101 -34 | -35 | # Violation cases for pathlib: | -ASYNC101.py:38:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:41:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -37 | async def foo(): -38 | Path("foo").open() # ASYNC101 +40 | async def func(): +41 | Path("foo").open() # ASYNC101 | ^^^^^^^^^^^^^^^^ ASYNC101 -39 | -40 | async def foo(): | -ASYNC101.py:42:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:46:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -40 | async def foo(): -41 | p = Path("foo") -42 | p.open() # ASYNC101 +44 | async def func(): +45 | p = Path("foo") +46 | p.open() # ASYNC101 | ^^^^^^ ASYNC101 -43 | -44 | async def foo(): | -ASYNC101.py:45:10: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:50:10: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -44 | async def foo(): -45 | with Path("foo").open() as f: # ASYNC101 +49 | async def func(): +50 | with Path("foo").open() as f: # ASYNC101 | ^^^^^^^^^^^^^^^^ ASYNC101 -46 | pass +51 | pass | -ASYNC101.py:53:9: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods +ASYNC101.py:58:9: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods | -52 | async def bar(): -53 | p.open() # ASYNC101 +57 | async def bar(): +58 | p.open() # ASYNC101 | ^^^^^^ ASYNC101 | +ASYNC101.py:64:5: ASYNC101 Async functions should not call `open`, `time.sleep`, or `subprocess` methods + | +62 | (p1, p2) = (Path("foo"), Path("bar")) +63 | +64 | p1.open() # ASYNC101 + | ^^^^^^^ ASYNC101 + | +