From 1e8881f9afe5daa7450e5e2a1f4b4a77497e9212 Mon Sep 17 00:00:00 2001 From: Dylan Date: Mon, 28 Apr 2025 09:39:55 -0500 Subject: [PATCH] [`refurb`] Mark fix as safe for `readlines-in-for` (`FURB129`) (#17644) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR promotes the fix applicability of [readlines-in-for (FURB129)](https://docs.astral.sh/ruff/rules/readlines-in-for/#readlines-in-for-furb129) to always safe. In the original PR (https://github.com/astral-sh/ruff/pull/9880), the author marked the rule as unsafe because Ruff's type inference couldn't quite guarantee that we had an `IOBase` object in hand. Some false positives were recorded in the test fixture. However, before the PR was merged, Charlie added the necessary type inference and the false positives went away. According to the [Python documentation](https://docs.python.org/3/library/io.html#io.IOBase), I believe this fix is safe for any proper implementation of `IOBase`: >[IOBase](https://docs.python.org/3/library/io.html#io.IOBase) (and its subclasses) supports the iterator protocol, meaning that an [IOBase](https://docs.python.org/3/library/io.html#io.IOBase) object can be iterated over yielding the lines in a stream. Lines are defined slightly differently depending on whether the stream is a binary stream (yielding bytes), or a text stream (yielding character strings). See [readline()](https://docs.python.org/3/library/io.html#io.IOBase.readline) below. and then in the [documentation for `readlines`](https://docs.python.org/3/library/io.html#io.IOBase.readlines): >Read and return a list of lines from the stream. hint can be specified to control the number of lines read: no more lines will be read if the total size (in bytes/characters) of all lines so far exceeds hint. [...] >Note that it’s already possible to iterate on file objects using for line in file: ... without calling file.readlines(). I believe that a careful reading of our [versioning policy](https://docs.astral.sh/ruff/versioning/#version-changes) requires that this change be deferred to a minor release - but please correct me if I'm wrong! --- .../resources/test/fixtures/refurb/FURB129.py | 2 +- crates/ruff_linter/src/preview.rs | 5 + crates/ruff_linter/src/rules/refurb/mod.rs | 18 ++ .../rules/refurb/rules/readlines_in_for.rs | 13 +- ...b__tests__preview__FURB129_FURB129.py.snap | 243 ++++++++++++++++++ 5 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview__FURB129_FURB129.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py index 51a3cf73a5..c5c0307098 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py @@ -57,7 +57,7 @@ with o("FURB129.py") as f: pass -# False positives +# Ok def func(f): for _line in f.readlines(): pass diff --git a/crates/ruff_linter/src/preview.rs b/crates/ruff_linter/src/preview.rs index f64821f80d..9973eada40 100644 --- a/crates/ruff_linter/src/preview.rs +++ b/crates/ruff_linter/src/preview.rs @@ -116,3 +116,8 @@ pub(crate) const fn is_allow_nested_roots_enabled(settings: &LinterSettings) -> pub(crate) const fn is_check_file_level_directives_enabled(settings: &LinterSettings) -> bool { settings.preview.is_enabled() } + +// https://github.com/astral-sh/ruff/pull/17644 +pub(crate) const fn is_readlines_in_for_fix_safe(settings: &LinterSettings) -> bool { + settings.preview.is_enabled() +} diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index 830581d49f..1793e0e964 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -61,6 +61,24 @@ mod tests { Ok(()) } + #[test_case(Rule::ReadlinesInFor, Path::new("FURB129.py"))] + fn preview(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("refurb").join(path).as_path(), + &settings::LinterSettings { + preview: settings::types::PreviewMode::Enabled, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test] fn write_whole_file_python_39() -> Result<()> { let diagnostics = test_path( 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 index e06626da9c..23b6e337db 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/readlines_in_for.rs @@ -1,3 +1,4 @@ +use crate::preview::is_readlines_in_for_fix_safe; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::{Comprehension, Expr, StmtFor}; @@ -85,8 +86,14 @@ fn readlines_in_iter(checker: &Checker, iter_expr: &Expr) { } 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()), - ))); + diagnostic.set_fix(if is_readlines_in_for_fix_safe(checker.settings) { + Fix::safe_edit(Edit::range_deletion( + expr_call.range().add_start(expr_attr.value.range().len()), + )) + } else { + Fix::unsafe_edit(Edit::range_deletion( + expr_call.range().add_start(expr_attr.value.range().len()), + )) + }); checker.report_diagnostic(diagnostic); } diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview__FURB129_FURB129.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview__FURB129_FURB129.py.snap new file mode 100644 index 0000000000..f0fdde19a8 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview__FURB129_FURB129.py.snap @@ -0,0 +1,243 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB129.py:7:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly + | +5 | # Errors +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()` + +ℹ Safe fix +4 4 | +5 5 | # Errors +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 [*] Instead of calling `readlines()`, iterate over file object directly + | + 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()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | + 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()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | + 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()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | +13 | with Path("FURB129.py").open() as f: +14 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +15 | pass + | + = help: Remove `readlines()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | +15 | pass +16 | +17 | for _line in open("FURB129.py").readlines(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129 +18 | pass + | + = help: Remove `readlines()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | +18 | pass +19 | +20 | for _line in Path("FURB129.py").open().readlines(): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129 +21 | pass + | + = help: Remove `readlines()` + +ℹ Safe 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 [*] Instead of calling `readlines()`, iterate over file object directly + | +24 | def func(): +25 | f = Path("FURB129.py").open() +26 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +27 | pass +28 | f.close() + | + = help: Remove `readlines()` + +ℹ Safe fix +23 23 | +24 24 | def func(): +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 [*] Instead of calling `readlines()`, iterate over file object directly + | +31 | def func(f: io.BytesIO): +32 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +33 | pass + | + = help: Remove `readlines()` + +ℹ Safe fix +29 29 | +30 30 | +31 31 | def func(f: io.BytesIO): +32 |- for _line in f.readlines(): + 32 |+ for _line in f: +33 33 | pass +34 34 | +35 35 | + +FURB129.py:38:22: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly + | +36 | def func(): +37 | with (open("FURB129.py") as f, foo as bar): +38 | for _line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +39 | pass +40 | for _line in bar.readlines(): + | + = help: Remove `readlines()` + +ℹ Safe fix +35 35 | +36 36 | def func(): +37 37 | with (open("FURB129.py") as f, foo as bar): +38 |- for _line in f.readlines(): + 38 |+ for _line in f: +39 39 | pass +40 40 | for _line in bar.readlines(): +41 41 | pass + +FURB129.py:48:17: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly + | +47 | with builtins.open("FURB129.py") as f: +48 | for line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +49 | pass + | + = help: Remove `readlines()` + +ℹ Safe fix +45 45 | +46 46 | +47 47 | with builtins.open("FURB129.py") as f: +48 |- for line in f.readlines(): + 48 |+ for line in f: +49 49 | pass +50 50 | +51 51 | + +FURB129.py:56:17: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly + | +55 | with o("FURB129.py") as f: +56 | for line in f.readlines(): + | ^^^^^^^^^^^^^ FURB129 +57 | pass + | + = help: Remove `readlines()` + +ℹ Safe fix +53 53 | +54 54 | +55 55 | with o("FURB129.py") as f: +56 |- for line in f.readlines(): + 56 |+ for line in f: +57 57 | pass +58 58 | +59 59 |