Skip to content

Commit

Permalink
[flake8-simplify] Extend open-file-with-context-handler to work wit…
Browse files Browse the repository at this point in the history
…h other standard-library IO modules (`SIM115`) (#12959)

Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
  • Loading branch information
diceroll123 and AlexWaygood authored Aug 22, 2024
1 parent d1d0678 commit d37e2e5
Show file tree
Hide file tree
Showing 6 changed files with 602 additions and 22 deletions.
187 changes: 186 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM115.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
open("filename", "w").close()
pathlib.Path("filename").open("w").close()


# OK (custom context manager)
class MyFile:
def __init__(self, filename: str):
Expand All @@ -58,3 +57,189 @@ def __enter__(self):

def __exit__(self, exc_type, exc_val, exc_tb):
self.file.close()


import tempfile
import tarfile
from tarfile import TarFile
import zipfile
import io
import codecs
import bz2
import gzip
import dbm
import dbm.gnu
import dbm.ndbm
import dbm.dumb
import lzma
import shelve
import tokenize
import wave
import fileinput

f = tempfile.NamedTemporaryFile()
f = tempfile.TemporaryFile()
f = tempfile.SpooledTemporaryFile()
f = tarfile.open("foo.tar")
f = TarFile("foo.tar").open()
f = tarfile.TarFile("foo.tar").open()
f = tarfile.TarFile().open()
f = zipfile.ZipFile("foo.zip").open("foo.txt")
f = io.open("foo.txt")
f = io.open_code("foo.txt")
f = codecs.open("foo.txt")
f = bz2.open("foo.txt")
f = gzip.open("foo.txt")
f = dbm.open("foo.db")
f = dbm.gnu.open("foo.db")
f = dbm.ndbm.open("foo.db")
f = dbm.dumb.open("foo.db")
f = lzma.open("foo.xz")
f = lzma.LZMAFile("foo.xz")
f = shelve.open("foo.db")
f = tokenize.open("foo.py")
f = wave.open("foo.wav")
f = tarfile.TarFile.taropen("foo.tar")
f = fileinput.input("foo.txt")
f = fileinput.FileInput("foo.txt")

with contextlib.suppress(Exception):
# The following line is for example's sake.
# For some f's above, this would raise an error (since it'd be f.readline() etc.)
data = f.read()

f.close()

# OK
with tempfile.TemporaryFile() as f:
data = f.read()

# OK
with tarfile.open("foo.tar") as f:
data = f.add("foo.txt")

# OK
with tarfile.TarFile("foo.tar") as f:
data = f.add("foo.txt")

# OK
with tarfile.TarFile("foo.tar").open() as f:
data = f.add("foo.txt")

# OK
with zipfile.ZipFile("foo.zip") as f:
data = f.read("foo.txt")

# OK
with zipfile.ZipFile("foo.zip").open("foo.txt") as f:
data = f.read()

# OK
with zipfile.ZipFile("foo.zip") as zf:
with zf.open("foo.txt") as f:
data = f.read()

# OK
with io.open("foo.txt") as f:
data = f.read()

# OK
with io.open_code("foo.txt") as f:
data = f.read()

# OK
with codecs.open("foo.txt") as f:
data = f.read()

# OK
with bz2.open("foo.txt") as f:
data = f.read()

# OK
with gzip.open("foo.txt") as f:
data = f.read()

# OK
with dbm.open("foo.db") as f:
data = f.get("foo")

# OK
with dbm.gnu.open("foo.db") as f:
data = f.get("foo")

# OK
with dbm.ndbm.open("foo.db") as f:
data = f.get("foo")

# OK
with dbm.dumb.open("foo.db") as f:
data = f.get("foo")

# OK
with lzma.open("foo.xz") as f:
data = f.read()

# OK
with lzma.LZMAFile("foo.xz") as f:
data = f.read()

# OK
with shelve.open("foo.db") as f:
data = f["foo"]

# OK
with tokenize.open("foo.py") as f:
data = f.read()

# OK
with wave.open("foo.wav") as f:
data = f.readframes(1024)

# OK
with tarfile.TarFile.taropen("foo.tar") as f:
data = f.add("foo.txt")

# OK
with fileinput.input("foo.txt") as f:
data = f.readline()

# OK
with fileinput.FileInput("foo.txt") as f:
data = f.readline()

# OK (quick one-liner to clear file contents)
tempfile.NamedTemporaryFile().close()
tempfile.TemporaryFile().close()
tempfile.SpooledTemporaryFile().close()
tarfile.open("foo.tar").close()
tarfile.TarFile("foo.tar").close()
tarfile.TarFile("foo.tar").open().close()
tarfile.TarFile.open("foo.tar").close()
zipfile.ZipFile("foo.zip").close()
zipfile.ZipFile("foo.zip").open("foo.txt").close()
io.open("foo.txt").close()
io.open_code("foo.txt").close()
codecs.open("foo.txt").close()
bz2.open("foo.txt").close()
gzip.open("foo.txt").close()
dbm.open("foo.db").close()
dbm.gnu.open("foo.db").close()
dbm.ndbm.open("foo.db").close()
dbm.dumb.open("foo.db").close()
lzma.open("foo.xz").close()
lzma.LZMAFile("foo.xz").close()
shelve.open("foo.db").close()
tokenize.open("foo.py").close()
wave.open("foo.wav").close()
tarfile.TarFile.taropen("foo.tar").close()
fileinput.input("foo.txt").close()
fileinput.FileInput("foo.txt").close()

def aliased():
from shelve import open as open_shelf
x = open_shelf("foo.dbm")
x.close()

from tarfile import TarFile as TF
f = TF("foo").open()
f.close()
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -883,7 +883,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_simplify::rules::use_capital_environment_variables(checker, expr);
}
if checker.enabled(Rule::OpenFileWithContextHandler) {
flake8_simplify::rules::open_file_with_context_handler(checker, func);
flake8_simplify::rules::open_file_with_context_handler(checker, call);
}
if checker.enabled(Rule::DictGetWithNoneDefault) {
flake8_simplify::rules::dict_get_with_none_default(checker, expr);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_simplify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ mod tests {
}

#[test_case(Rule::IfElseBlockInsteadOfIfExp, Path::new("SIM108.py"))]
#[test_case(Rule::OpenFileWithContextHandler, Path::new("SIM115.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,20 @@ use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for uses of the builtin `open()` function without an associated context
/// manager.
/// Checks for cases where files are opened (e.g., using the builtin `open()` function)
/// without using a context manager.
///
/// ## Why is this bad?
/// If a file is opened without a context manager, it is not guaranteed that
/// the file will be closed (e.g., if an exception is raised), which can cause
/// resource leaks.
///
/// ## Preview-mode behavior
/// If [preview] mode is enabled, this rule will detect a wide array of IO calls where
/// context managers could be used, such as `tempfile.TemporaryFile()` or
/// `tarfile.TarFile(...).gzopen()`. If preview mode is not enabled, only `open()`,
/// `builtins.open()` and `pathlib.Path(...).open()` are detected.
///
/// ## Example
/// ```python
/// file = open("foo.txt")
Expand All @@ -37,7 +43,7 @@ pub struct OpenFileWithContextHandler;
impl Violation for OpenFileWithContextHandler {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use context handler for opening files")
format!("Use a context manager for opening files")
}
}

Expand Down Expand Up @@ -113,14 +119,14 @@ fn match_exit_stack(semantic: &SemanticModel) -> bool {
}

/// Return `true` if `func` is the builtin `open` or `pathlib.Path(...).open`.
fn is_open(semantic: &SemanticModel, func: &Expr) -> bool {
fn is_open(semantic: &SemanticModel, call: &ast::ExprCall) -> bool {
// Ex) `open(...)`
if semantic.match_builtin_expr(func, "open") {
if semantic.match_builtin_expr(&call.func, "open") {
return true;
}

// Ex) `pathlib.Path(...).open()`
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else {
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = &*call.func else {
return false;
};

Expand All @@ -140,6 +146,63 @@ fn is_open(semantic: &SemanticModel, func: &Expr) -> bool {
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["pathlib", "Path"]))
}

/// Return `true` if the expression is an `open` call or temporary file constructor.
fn is_open_preview(semantic: &SemanticModel, call: &ast::ExprCall) -> bool {
let func = &*call.func;

// Ex) `open(...)`
if let Some(qualified_name) = semantic.resolve_qualified_name(func) {
return matches!(
qualified_name.segments(),
[
"" | "builtins"
| "bz2"
| "codecs"
| "dbm"
| "gzip"
| "tarfile"
| "shelve"
| "tokenize"
| "wave",
"open"
] | ["dbm", "gnu" | "ndbm" | "dumb", "open"]
| ["fileinput", "FileInput" | "input"]
| ["io", "open" | "open_code"]
| ["lzma", "LZMAFile" | "open"]
| ["tarfile", "TarFile", "taropen"]
| [
"tempfile",
"TemporaryFile" | "NamedTemporaryFile" | "SpooledTemporaryFile"
]
);
}

// Ex) `pathlib.Path(...).open()`
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else {
return false;
};

let Expr::Call(ast::ExprCall { func, .. }) = &**value else {
return false;
};

// E.g. for `pathlib.Path(...).open()`, `qualified_name_of_instance.segments() == ["pathlib", "Path"]`
let Some(qualified_name_of_instance) = semantic.resolve_qualified_name(func) else {
return false;
};

matches!(
(qualified_name_of_instance.segments(), &**attr),
(
["pathlib", "Path"] | ["zipfile", "ZipFile"] | ["lzma", "LZMAFile"],
"open"
) | (
["tarfile", "TarFile"],
"open" | "taropen" | "gzopen" | "bz2open" | "xzopen"
)
)
}

/// Return `true` if the current expression is followed by a `close` call.
fn is_closed(semantic: &SemanticModel) -> bool {
let Some(expr) = semantic.current_expression_grandparent() else {
Expand All @@ -165,11 +228,17 @@ fn is_closed(semantic: &SemanticModel) -> bool {
}

/// SIM115
pub(crate) fn open_file_with_context_handler(checker: &mut Checker, func: &Expr) {
pub(crate) fn open_file_with_context_handler(checker: &mut Checker, call: &ast::ExprCall) {
let semantic = checker.semantic();

if !is_open(semantic, func) {
return;
if checker.settings.preview.is_disabled() {
if !is_open(semantic, call) {
return;
}
} else {
if !is_open_preview(semantic, call) {
return;
}
}

// Ex) `open("foo.txt").close()`
Expand Down Expand Up @@ -201,7 +270,8 @@ pub(crate) fn open_file_with_context_handler(checker: &mut Checker, func: &Expr)
}
}

checker
.diagnostics
.push(Diagnostic::new(OpenFileWithContextHandler, func.range()));
checker.diagnostics.push(Diagnostic::new(
OpenFileWithContextHandler,
call.func.range(),
));
}
Loading

0 comments on commit d37e2e5

Please sign in to comment.