From 51b917cfbfb070f13b9109ca062133222a1adc59 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 17 Jan 2023 22:39:54 -0500 Subject: [PATCH] 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. --- .../test/fixtures/flake8_simplify/SIM115.py | 34 ++++++- .../rules/open_file_with_context_handler.rs | 94 +++++++++++++++++-- ...ke8_simplify__tests__SIM115_SIM115.py.snap | 14 ++- 3 files changed, 129 insertions(+), 13 deletions(-) diff --git a/resources/test/fixtures/flake8_simplify/SIM115.py b/resources/test/fixtures/flake8_simplify/SIM115.py index 5bc6998564..2171843136 100644 --- a/resources/test/fixtures/flake8_simplify/SIM115.py +++ b/resources/test/fixtures/flake8_simplify/SIM115.py @@ -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")) diff --git a/src/rules/flake8_simplify/rules/open_file_with_context_handler.rs b/src/rules/flake8_simplify/rules/open_file_with_context_handler.rs index b169473529..95f3929ddd 100644 --- a/src/rules/flake8_simplify/rules/open_file_with_context_handler.rs +++ b/src/rules/flake8_simplify/rules/open_file_with_context_handler.rs @@ -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,15 +79,25 @@ 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 { .. } => (), - _ => { - checker.diagnostics.push(Diagnostic::new( - violations::OpenFileWithContextHandler, - Range::from_located(func), - )); - } + // 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), + )); } } } diff --git a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM115_SIM115.py.snap b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM115_SIM115.py.snap index 16c4b7ad01..03d2fd87a4 100644 --- a/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM115_SIM115.py.snap +++ b/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM115_SIM115.py.snap @@ -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: ~