diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py new file mode 100644 index 00000000000000..efe099bfee64cf --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py @@ -0,0 +1,67 @@ +# Lint +import codecs +import io +from pathlib import Path + +with open('FURB129.py') as f: + for _line in f.readlines(): + pass + a = [line.lower() for line in f.readlines()] + b = {line.upper() for line in f.readlines()} + c = {line.lower(): line.upper() for line in f.readlines()} + +with Path('FURB129.py').open() as f: + for _line in f.readlines(): + pass + +for _line in open('FURB129.py').readlines(): + pass + +for _line in Path('FURB129.py').open().readlines(): + pass + + +def good1(): + f = Path('FURB129.py').open() + for _line in f.readlines(): + pass + f.close() + + +def good2(f: io.BytesIO): + for _line in f.readlines(): + pass + + +# False positives +def bad(f): + for _line in f.readlines(): + pass + + +def worse(f: codecs.StreamReader): + for _line in f.readlines(): + pass + + +def foo(): + class A: + def readlines(self) -> list[str]: + return ["a", "b", "c"] + + return A() + + +for _line in foo().readlines(): + pass + +# OK +for _line in ["a", "b", "c"]: + pass +with open('FURB129.py') as f: + for _line in f: + pass + for _line in f.readlines(10): + pass + for _not_line in f.readline(): + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/comprehension.rs b/crates/ruff_linter/src/checkers/ast/analyze/comprehension.rs index c3d368be2954af..5aa71b01632022 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/comprehension.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/comprehension.rs @@ -2,11 +2,14 @@ use ruff_python_ast::Comprehension; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::flake8_simplify; +use crate::rules::{flake8_simplify, refurb}; /// Run lint rules over a [`Comprehension`] syntax nodes. pub(crate) fn comprehension(comprehension: &Comprehension, checker: &mut Checker) { if checker.enabled(Rule::InDictKeys) { flake8_simplify::rules::key_in_dict_comprehension(checker, comprehension); } + if checker.enabled(Rule::ReadlinesInFor) { + refurb::rules::readlines_in_comprehension(checker, comprehension) + } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 3ceac945740fc4..e8476a8331edbd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1312,6 +1312,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryDictIndexLookup) { pylint::rules::unnecessary_dict_index_lookup(checker, for_stmt); } + if checker.enabled(Rule::ReadlinesInFor) { + refurb::rules::readlines_in_for(checker, for_stmt); + } if !is_async { if checker.enabled(Rule::ReimplementedBuiltin) { flake8_simplify::rules::convert_for_loop_to_any_all(checker, stmt); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index ac97ac50ba1f21..76ef18b48674f4 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1019,6 +1019,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { #[allow(deprecated)] (Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend), (Refurb, "118") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedOperator), + (Refurb, "129") => (RuleGroup::Preview, rules::refurb::rules::ReadlinesInFor), #[allow(deprecated)] (Refurb, "131") => (RuleGroup::Nursery, rules::refurb::rules::DeleteFullSlice), #[allow(deprecated)] diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 26ba58041933b8..de5f0bdde06ddb 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -17,6 +17,7 @@ mod tests { #[test_case(Rule::ReadWholeFile, Path::new("FURB101.py"))] #[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))] #[test_case(Rule::ReimplementedOperator, Path::new("FURB118.py"))] + #[test_case(Rule::ReadlinesInFor, Path::new("FURB129.py"))] #[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))] #[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))] #[test_case(Rule::IfExprMinMax, Path::new("FURB136.py"))] diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index 532003ee143df7..a69a798cf5ec82 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -9,6 +9,7 @@ pub(crate) use math_constant::*; pub(crate) use metaclass_abcmeta::*; pub(crate) use print_empty_string::*; pub(crate) use read_whole_file::*; +pub(crate) use readlines_in_for::*; pub(crate) use redundant_log_base::*; pub(crate) use regex_flag_alias::*; pub(crate) use reimplemented_operator::*; @@ -30,6 +31,7 @@ mod math_constant; mod metaclass_abcmeta; mod print_empty_string; mod read_whole_file; +mod readlines_in_for; mod redundant_log_base; mod regex_flag_alias; mod reimplemented_operator; diff --git a/crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs b/crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs new file mode 100644 index 00000000000000..42981237cff1cb --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs @@ -0,0 +1,71 @@ +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{Comprehension, Expr, StmtFor}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of `readlines()` when iterating a file object line-by-line. +/// +/// ## Why is this bad? +/// Instead of iterating through `list[str]` which is returned from `readlines()`, use the iteration +/// through a file object which is a more convenient and performant way. +/// +/// ## Example +/// ```python +/// with open("file.txt") as f: +/// for line in f.readlines(): +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// with open("file.txt") as f: +/// for line in f: +/// ... +/// ``` +/// +/// ## References +/// - [Python documentation: `io.IOBase.readlines`](https://docs.python.org/3/library/io.html#io.IOBase.readlines) +/// - [Python documentation: methods of file objects](https://docs.python.org/3/tutorial/inputoutput.html#methods-of-file-objects) +/// +#[violation] +pub(crate) struct ReadlinesInFor; + +impl AlwaysFixableViolation for ReadlinesInFor { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use of readlines() in for loop") + } + + fn fix_title(&self) -> String { + "Remove readlines()".into() + } +} + +fn readlines_in_iter(checker: &mut Checker, iter_expr: &Expr) -> Option<()> { + let expr_call = iter_expr.as_call_expr()?; + let attr_expr = expr_call.func.as_attribute_expr()?; + (attr_expr.attr.as_str() == "readlines" && expr_call.arguments.is_empty()).then(|| { + checker + .diagnostics + .push( + Diagnostic::new(ReadlinesInFor, expr_call.range()).with_fix(Fix::unsafe_edit( + Edit::range_deletion( + expr_call.range().add_start(attr_expr.value.range().len()), + ), + )), + ); + }) +} + +// FURB129 +pub(crate) fn readlines_in_for(checker: &mut Checker, for_stmt: &StmtFor) { + readlines_in_iter(checker, for_stmt.iter.as_ref()); +} + +// FURB129 +pub(crate) fn readlines_in_comprehension(checker: &mut Checker, comprehension: &Comprehension) { + readlines_in_iter(checker, &comprehension.iter); +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap new file mode 100644 index 00000000000000..23da81d71d7c5a --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap @@ -0,0 +1,242 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB129.py:7:18: FURB129 [*] Use of readlines() in for loop + | +6 | with open('FURB129.py') as f: +7 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +8 | pass +9 | a = [line.lower() for line in f.readlines()] + | + = help: Remove readlines() + +ℹ Unsafe fix +4 4 | from pathlib import Path +5 5 | +6 6 | with open('FURB129.py') as f: +7 |- for _line in f.readlines(): + 7 |+ for _line in f: +8 8 | pass +9 9 | a = [line.lower() for line in f.readlines()] +10 10 | b = {line.upper() for line in f.readlines()} + +FURB129.py:9:35: FURB129 [*] Use of readlines() in for loop + | + 7 | for _line in f.readlines(): + 8 | pass + 9 | a = [line.lower() for line in f.readlines()] + | ^^^^^^^^^^^^^ FURB129 +10 | b = {line.upper() for line in f.readlines()} +11 | c = {line.lower(): line.upper() for line in f.readlines()} + | + = help: Remove readlines() + +ℹ Unsafe fix +6 6 | with open('FURB129.py') as f: +7 7 | for _line in f.readlines(): +8 8 | pass +9 |- a = [line.lower() for line in f.readlines()] + 9 |+ a = [line.lower() for line in f] +10 10 | b = {line.upper() for line in f.readlines()} +11 11 | c = {line.lower(): line.upper() for line in f.readlines()} +12 12 | + +FURB129.py:10:35: FURB129 [*] Use of readlines() in for loop + | + 8 | pass + 9 | a = [line.lower() for line in f.readlines()] +10 | b = {line.upper() for line in f.readlines()} + | ^^^^^^^^^^^^^ FURB129 +11 | c = {line.lower(): line.upper() for line in f.readlines()} + | + = help: Remove readlines() + +ℹ Unsafe fix +7 7 | for _line in f.readlines(): +8 8 | pass +9 9 | a = [line.lower() for line in f.readlines()] +10 |- b = {line.upper() for line in f.readlines()} + 10 |+ b = {line.upper() for line in f} +11 11 | c = {line.lower(): line.upper() for line in f.readlines()} +12 12 | +13 13 | with Path('FURB129.py').open() as f: + +FURB129.py:11:49: FURB129 [*] Use of readlines() in for loop + | + 9 | a = [line.lower() for line in f.readlines()] +10 | b = {line.upper() for line in f.readlines()} +11 | c = {line.lower(): line.upper() for line in f.readlines()} + | ^^^^^^^^^^^^^ FURB129 +12 | +13 | with Path('FURB129.py').open() as f: + | + = help: Remove readlines() + +ℹ Unsafe fix +8 8 | pass +9 9 | a = [line.lower() for line in f.readlines()] +10 10 | b = {line.upper() for line in f.readlines()} +11 |- c = {line.lower(): line.upper() for line in f.readlines()} + 11 |+ c = {line.lower(): line.upper() for line in f} +12 12 | +13 13 | with Path('FURB129.py').open() as f: +14 14 | for _line in f.readlines(): + +FURB129.py:14:18: FURB129 [*] Use of readlines() in for loop + | +13 | with Path('FURB129.py').open() as f: +14 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +15 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +11 11 | c = {line.lower(): line.upper() for line in f.readlines()} +12 12 | +13 13 | with Path('FURB129.py').open() as f: +14 |- for _line in f.readlines(): + 14 |+ for _line in f: +15 15 | pass +16 16 | +17 17 | for _line in open('FURB129.py').readlines(): + +FURB129.py:17:14: FURB129 [*] Use of readlines() in for loop + | +15 | pass +16 | +17 | for _line in open('FURB129.py').readlines(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129 +18 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +14 14 | for _line in f.readlines(): +15 15 | pass +16 16 | +17 |-for _line in open('FURB129.py').readlines(): + 17 |+for _line in open('FURB129.py'): +18 18 | pass +19 19 | +20 20 | for _line in Path('FURB129.py').open().readlines(): + +FURB129.py:20:14: FURB129 [*] Use of readlines() in for loop + | +18 | pass +19 | +20 | for _line in Path('FURB129.py').open().readlines(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129 +21 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +17 17 | for _line in open('FURB129.py').readlines(): +18 18 | pass +19 19 | +20 |-for _line in Path('FURB129.py').open().readlines(): + 20 |+for _line in Path('FURB129.py').open(): +21 21 | pass +22 22 | +23 23 | + +FURB129.py:26:18: FURB129 [*] Use of readlines() in for loop + | +24 | def good1(): +25 | f = Path('FURB129.py').open() +26 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +27 | pass +28 | f.close() + | + = help: Remove readlines() + +ℹ Unsafe fix +23 23 | +24 24 | def good1(): +25 25 | f = Path('FURB129.py').open() +26 |- for _line in f.readlines(): + 26 |+ for _line in f: +27 27 | pass +28 28 | f.close() +29 29 | + +FURB129.py:32:18: FURB129 [*] Use of readlines() in for loop + | +31 | def good2(f: io.BytesIO): +32 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +33 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +29 29 | +30 30 | +31 31 | def good2(f: io.BytesIO): +32 |- for _line in f.readlines(): + 32 |+ for _line in f: +33 33 | pass +34 34 | +35 35 | + +FURB129.py:38:18: FURB129 [*] Use of readlines() in for loop + | +36 | # False positives +37 | def bad(f): +38 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +39 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +35 35 | +36 36 | # False positives +37 37 | def bad(f): +38 |- for _line in f.readlines(): + 38 |+ for _line in f: +39 39 | pass +40 40 | +41 41 | + +FURB129.py:43:18: FURB129 [*] Use of readlines() in for loop + | +42 | def worse(f: codecs.StreamReader): +43 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +44 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +40 40 | +41 41 | +42 42 | def worse(f: codecs.StreamReader): +43 |- for _line in f.readlines(): + 43 |+ for _line in f: +44 44 | pass +45 45 | +46 46 | + +FURB129.py:55:14: FURB129 [*] Use of readlines() in for loop + | +55 | for _line in foo().readlines(): + | ^^^^^^^^^^^^^^^^^ FURB129 +56 | pass + | + = help: Remove readlines() + +ℹ Unsafe fix +52 52 | return A() +53 53 | +54 54 | +55 |-for _line in foo().readlines(): + 55 |+for _line in foo(): +56 56 | pass +57 57 | +58 58 | # OK + + diff --git a/ruff.schema.json b/ruff.schema.json index c5c9a126a985bd..67b451f1a55110 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2980,6 +2980,8 @@ "FURB11", "FURB113", "FURB118", + "FURB12", + "FURB129", "FURB13", "FURB131", "FURB132",