[refurb] Implement readlines_in_for lint (FURB129) (#9880)

## Summary
Implement [implicit readlines
(FURB129)](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py)
lint.

## Notes
I need a help/an opinion about suggested implementations.

This implementation differs from the original one from `refurb` in the
following way. This implementation checks syntactically the call of the
method with the name `readlines()` inside `for` {loop|generator
expression}. The implementation from refurb also
[checks](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py#L43)
that callee is a variable with a type `io.TextIOWrapper` or
`io.BufferedReader`.

- I do not see a simple way to implement the same logic.
- The best I can have is something like
```rust
checker.semantic().binding(checker.semantic().resolve_name(attr_expr.value.as_name_expr()?)?).statement(checker.semantic())
```
and analyze cases. But this will be not about types, but about guessing
the type by assignment (or with) expression.
- Also this logic has several false negatives, when the callee is not a
variable, but the result of function call (e.g. `open(...)`).
- On the other side, maybe it is good to lint this on other things,
where this suggestion is not safe, and push the developers to change
their interfaces to be less surprising, comparing with the standard
library.
- Anyway while the current implementation has false-positives (I
mentioned some of them in the test) I marked the fixes to be unsafe.

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