mirror of https://github.com/astral-sh/ruff
Exempt `contextlib.ExitStack()` for SIM115 rules (#1946)
Since our binding tracking is somewhat limited, I opted to favor false negatives over false positives. So, e.g., this won't trigger SIM115:
```py
with contextlib.ExitStack():
f = exit_stack.enter_context(open("filename"))
```
(Notice that `exit_stack` is unbound.)
The alternative strategy required us to incorrectly trigger SIM115 on this:
```py
with contextlib.ExitStack() as exit_stack:
exit_stack_ = exit_stack
f = exit_stack_.enter_context(open("filename"))
```
Closes #1945.
This commit is contained in:
parent
c880d744fd
commit
51b917cfbf
|
|
@ -1,6 +1,36 @@
|
|||
f = open('foo.txt') # SIM115
|
||||
import contextlib
|
||||
|
||||
# SIM115
|
||||
f = open("foo.txt")
|
||||
data = f.read()
|
||||
f.close()
|
||||
|
||||
with open('foo.txt') as f: # OK
|
||||
# OK
|
||||
with open("foo.txt") as f:
|
||||
data = f.read()
|
||||
|
||||
# OK
|
||||
with contextlib.ExitStack() as exit_stack:
|
||||
f = exit_stack.enter_context(open("filename"))
|
||||
|
||||
# OK
|
||||
with contextlib.ExitStack() as stack:
|
||||
files = [stack.enter_context(open(fname)) for fname in filenames]
|
||||
close_files = stack.pop_all().close
|
||||
|
||||
# OK
|
||||
with contextlib.AsyncExitStack() as exit_stack:
|
||||
f = await exit_stack.enter_async_context(open("filename"))
|
||||
|
||||
# OK (false negative)
|
||||
with contextlib.ExitStack():
|
||||
f = exit_stack.enter_context(open("filename"))
|
||||
|
||||
# SIM115
|
||||
with contextlib.ExitStack():
|
||||
f = open("filename")
|
||||
|
||||
# OK
|
||||
with contextlib.ExitStack() as exit_stack:
|
||||
exit_stack_ = exit_stack
|
||||
f = exit_stack_.enter_context(open("filename"))
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
use rustpython_ast::Expr;
|
||||
use rustpython_ast::{Expr, ExprKind};
|
||||
use rustpython_parser::ast::StmtKind;
|
||||
|
||||
use crate::ast::types::Range;
|
||||
|
|
@ -6,6 +6,72 @@ use crate::checkers::ast::Checker;
|
|||
use crate::registry::Diagnostic;
|
||||
use crate::violations;
|
||||
|
||||
/// Return `true` if the current expression is nested in an `await
|
||||
/// exit_stack.enter_async_context` call.
|
||||
fn match_async_exit_stack(checker: &Checker) -> bool {
|
||||
let Some(expr) = checker.current_expr_grandparent() else {
|
||||
return false;
|
||||
};
|
||||
let ExprKind::Await { value } = &expr.node else {
|
||||
return false;
|
||||
};
|
||||
let ExprKind::Call { func, .. } = &value.node else {
|
||||
return false;
|
||||
};
|
||||
let ExprKind::Attribute { attr, .. } = &func.node else {
|
||||
return false;
|
||||
};
|
||||
if attr != "enter_async_context" {
|
||||
return false;
|
||||
}
|
||||
for parent in &checker.parents {
|
||||
if let StmtKind::With { items, .. } = &parent.node {
|
||||
for item in items {
|
||||
if let ExprKind::Call { func, .. } = &item.context_expr.node {
|
||||
if checker.resolve_call_path(func).map_or(false, |call_path| {
|
||||
call_path == ["contextlib", "AsyncExitStack"]
|
||||
}) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// Return `true` if the current expression is nested in an
|
||||
/// `exit_stack.enter_context` call.
|
||||
fn match_exit_stack(checker: &Checker) -> bool {
|
||||
let Some(expr) = checker.current_expr_parent() else {
|
||||
return false;
|
||||
};
|
||||
let ExprKind::Call { func, .. } = &expr.node else {
|
||||
return false;
|
||||
};
|
||||
let ExprKind::Attribute { attr, .. } = &func.node else {
|
||||
return false;
|
||||
};
|
||||
if attr != "enter_context" {
|
||||
return false;
|
||||
}
|
||||
for parent in &checker.parents {
|
||||
if let StmtKind::With { items, .. } = &parent.node {
|
||||
for item in items {
|
||||
if let ExprKind::Call { func, .. } = &item.context_expr.node {
|
||||
if checker
|
||||
.resolve_call_path(func)
|
||||
.map_or(false, |call_path| call_path == ["contextlib", "ExitStack"])
|
||||
{
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// SIM115
|
||||
pub fn open_file_with_context_handler(checker: &mut Checker, func: &Expr) {
|
||||
if checker
|
||||
|
|
@ -13,9 +79,21 @@ pub fn open_file_with_context_handler(checker: &mut Checker, func: &Expr) {
|
|||
.map_or(false, |call_path| call_path == ["", "open"])
|
||||
{
|
||||
if checker.is_builtin("open") {
|
||||
match checker.current_stmt().node {
|
||||
StmtKind::With { .. } => (),
|
||||
_ => {
|
||||
// Ex) `with open("foo.txt") as f: ...`
|
||||
if matches!(checker.current_stmt().node, StmtKind::With { .. }) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Ex) `with contextlib.ExitStack() as exit_stack: ...`
|
||||
if match_exit_stack(checker) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Ex) `with contextlib.AsyncExitStack() as exit_stack: ...`
|
||||
if match_async_exit_stack(checker) {
|
||||
return;
|
||||
}
|
||||
|
||||
checker.diagnostics.push(Diagnostic::new(
|
||||
violations::OpenFileWithContextHandler,
|
||||
Range::from_located(func),
|
||||
|
|
@ -23,5 +101,3 @@ pub fn open_file_with_context_handler(checker: &mut Checker, func: &Expr) {
|
|||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -5,11 +5,21 @@ expression: diagnostics
|
|||
- kind:
|
||||
OpenFileWithContextHandler: ~
|
||||
location:
|
||||
row: 1
|
||||
row: 4
|
||||
column: 4
|
||||
end_location:
|
||||
row: 1
|
||||
row: 4
|
||||
column: 8
|
||||
fix: ~
|
||||
parent: ~
|
||||
- kind:
|
||||
OpenFileWithContextHandler: ~
|
||||
location:
|
||||
row: 31
|
||||
column: 8
|
||||
end_location:
|
||||
row: 31
|
||||
column: 12
|
||||
fix: ~
|
||||
parent: ~
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue