Skip to content

Commit

Permalink
[refurb] Implement read-whole-file [FURB101] (#7682)
Browse files Browse the repository at this point in the history
## Summary

This PR is part of a bigger effort of re-implementing `refurb` rules
#1348. It adds support for
[FURB101](https://github.com/dosisod/refurb/blob/master/refurb/checks/pathlib/read_text.py)

## Test Plan

I included a new test + checked that all other tests pass.
  • Loading branch information
SavchenkoValeriy authored Oct 20, 2023
1 parent c8464c3 commit bc49492
Show file tree
Hide file tree
Showing 8 changed files with 566 additions and 0 deletions.
126 changes: 126 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
def foo():
...


def bar(x):
...


# Errors.

# FURB101
with open("file.txt") as f:
x = f.read()

# FURB101
with open("file.txt", "rb") as f:
x = f.read()

# FURB101
with open("file.txt", mode="rb") as f:
x = f.read()

# FURB101
with open("file.txt", encoding="utf8") as f:
x = f.read()

# FURB101
with open("file.txt", errors="ignore") as f:
x = f.read()

# FURB101
with open("file.txt", errors="ignore", mode="rb") as f:
x = f.read()

# FURB101
with open("file.txt", mode="r") as f: # noqa: FURB120
x = f.read()

# FURB101
with open(foo(), "rb") as f:
# The body of `with` is non-trivial, but the recommendation holds.
bar("pre")
bar(f.read())
bar("post")
print("Done")

# FURB101
with open("a.txt") as a, open("b.txt", "rb") as b:
x = a.read()
y = b.read()

# FURB101
with foo() as a, open("file.txt") as b, foo() as c:
# We have other things in here, multiple with items, but
# the user reads the whole file and that bit they can replace.
bar(a)
bar(bar(a + b.read()))
bar(c)


# Non-errors.

f2 = open("file2.txt")
with open("file.txt") as f:
x = f2.read()

with open("file.txt") as f:
# Path.read_text() does not support size, so ignore this
x = f.read(100)

# mode is dynamic
with open("file.txt", foo()) as f:
x = f.read()

# keyword mode is incorrect
with open("file.txt", mode="a+") as f:
x = f.read()

# enables line buffering, not supported in read_text()
with open("file.txt", buffering=1) as f:
x = f.read()

# force CRLF, not supported in read_text()
with open("file.txt", newline="\r\n") as f:
x = f.read()

# dont mistake "newline" for "mode"
with open("file.txt", newline="b") as f:
x = f.read()

# I guess we can possibly also report this case, but the question
# is why the user would put "r+" here in the first place.
with open("file.txt", "r+") as f:
x = f.read()

# Even though we read the whole file, we do other things.
with open("file.txt") as f:
x = f.read()
f.seek(0)
x += f.read(100)

# This shouldn't error, since it could contain unsupported arguments, like `buffering`.
with open(*filename) as f:
x = f.read()

# This shouldn't error, since it could contain unsupported arguments, like `buffering`.
with open(**kwargs) as f:
x = f.read()

# This shouldn't error, since it could contain unsupported arguments, like `buffering`.
with open("file.txt", **kwargs) as f:
x = f.read()

# This shouldn't error, since it could contain unsupported arguments, like `buffering`.
with open("file.txt", mode="r", **kwargs) as f:
x = f.read()

# This could error (but doesn't), since it can't contain unsupported arguments, like
# `buffering`.
with open(*filename, mode="r") as f:
x = f.read()

# This could error (but doesn't), since it can't contain unsupported arguments, like
# `buffering`.
with open(*filename, file="file.txt", mode="r") as f:
x = f.read()
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1175,6 +1175,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::RedefinedLoopName) {
pylint::rules::redefined_loop_name(checker, stmt);
}
if checker.enabled(Rule::ReadWholeFile) {
refurb::rules::read_whole_file(checker, with_stmt);
}
}
Stmt::While(ast::StmtWhile { body, orelse, .. }) => {
if checker.enabled(Rule::FunctionUsesLoopVariable) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Slots, "002") => (RuleGroup::Stable, rules::flake8_slots::rules::NoSlotsInNamedtupleSubclass),

// refurb
(Refurb, "101") => (RuleGroup::Preview, rules::refurb::rules::ReadWholeFile),
(Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString),
#[allow(deprecated)]
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ mod tests {
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::ReadWholeFile, Path::new("FURB101.py"))]
#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
#[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
#[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use implicit_cwd::*;
pub(crate) use print_empty_string::*;
pub(crate) use read_whole_file::*;
pub(crate) use reimplemented_starmap::*;
pub(crate) use repeated_append::*;
pub(crate) use single_item_membership_test::*;
Expand All @@ -12,6 +13,7 @@ mod check_and_remove_from_set;
mod delete_full_slice;
mod implicit_cwd;
mod print_empty_string;
mod read_whole_file;
mod reimplemented_starmap;
mod repeated_append;
mod single_item_membership_test;
Expand Down
Loading

0 comments on commit bc49492

Please sign in to comment.