[refurb] Implement readlines_in_for lint (FURB129)

crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py

@@ -0,0 +1,67 @@
+import codecs
+import io
+from pathlib import Path
+
+# Errors
+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

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);
+    }
 }

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -1317,6 +1317,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);

crates/ruff_linter/src/codes.rs

@@ -1025,6 +1025,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)]

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"))]

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;

crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs

@@ -0,0 +1,92 @@
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{Comprehension, Expr, StmtFor};
+use ruff_python_semantic::analyze::typing;
+use ruff_python_semantic::analyze::typing::is_io_base_expr;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for uses of `readlines()` when iterating over a file line-by-line.
+///
+/// ## Why is this bad?
+/// Rather than iterating over all lines in a file by calling `readlines()`,
+/// it's more convenient and performant to iterate over the file object
+/// directly.
+///
+/// ## Example
+/// ```python
+/// with open("file.txt") as fp:
+///     for line in fp.readlines():
+///         ...
+/// ```
+///
+/// Use instead:
+/// ```python
+/// with open("file.txt") as fp:
+///     for line in fp:
+///         ...
+/// ```
+///
+/// ## References
+/// - [Python documentation: `io.IOBase.readlines`](https://docs.python.org/3/library/io.html#io.IOBase.readlines)
+#[violation]
+pub(crate) struct ReadlinesInFor;
+
+impl AlwaysFixableViolation for ReadlinesInFor {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Instead of calling `readlines()`, iterate over file object directly")
+    }
+
+    fn fix_title(&self) -> String {
+        "Remove `readlines()`".into()
+    }
+}
+
+/// 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);
+}
+
+fn readlines_in_iter(checker: &mut Checker, iter_expr: &Expr) {
+    let Expr::Call(expr_call) = iter_expr else {
+        return;
+    };
+
+    let Expr::Attribute(expr_attr) = expr_call.func.as_ref() else {
+        return;
+    };
+
+    if expr_attr.attr.as_str() != "readlines" || !expr_call.arguments.is_empty() {
+        return;
+    }
+
+    // Determine whether `fp` in `fp.readlines()` was bound to a file object.
+    if let Expr::Name(name) = expr_attr.value.as_ref() {
+        if !checker
+            .semantic()
+            .resolve_name(name)
+            .map(|id| checker.semantic().binding(id))
+            .is_some_and(|binding| typing::is_io_base(binding, checker.semantic()))
+        {
+            return;
+        }
+    } else {
+        if !is_io_base_expr(expr_attr.value.as_ref(), checker.semantic()) {
+            return;
+        }
+    }
+
+    let mut diagnostic = Diagnostic::new(ReadlinesInFor, expr_call.range());
+    diagnostic.set_fix(Fix::unsafe_edit(Edit::range_deletion(
+        expr_call.range().add_start(expr_attr.value.range().len()),
+    )));
+    checker.diagnostics.push(diagnostic);
+}