Skip to content

Commit 1e8881f

Browse files
dylwil3dylwil3
authored
[refurb] Mark fix as safe for readlines-in-for (FURB129) (#17644)
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 (#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!
1 parent 152a0b6 commit 1e8881f

File tree

5 files changed

+277
-4
lines changed

5 files changed

+277
-4
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ def func():
5757
pass
5858

5959

60-
# False positives
60+
# Ok
6161
def func(f):
6262
for _line in f.readlines():
6363
pass

‎crates/ruff_linter/src/preview.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,3 +116,8 @@ pub(crate) const fn is_allow_nested_roots_enabled(settings: &LinterSettings) ->
116116
pub(crate) const fn is_check_file_level_directives_enabled(settings: &LinterSettings) -> bool {
117117
settings.preview.is_enabled()
118118
}
119+
120+
// https://github.com/astral-sh/ruff/pull/17644
121+
pub(crate) const fn is_readlines_in_for_fix_safe(settings: &LinterSettings) -> bool {
122+
settings.preview.is_enabled()
123+
}

‎crates/ruff_linter/src/rules/refurb/mod.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,24 @@ mod tests {
6161
Ok(())
6262
}
6363

64+
#[test_case(Rule::ReadlinesInFor, Path::new("FURB129.py"))]
65+
fn preview(rule_code: Rule, path: &Path) -> Result<()> {
66+
let snapshot = format!(
67+
"preview__{}_{}",
68+
rule_code.noqa_code(),
69+
path.to_string_lossy()
70+
);
71+
let diagnostics = test_path(
72+
Path::new("refurb").join(path).as_path(),
73+
&settings::LinterSettings {
74+
preview: settings::types::PreviewMode::Enabled,
75+
..settings::LinterSettings::for_rule(rule_code)
76+
},
77+
)?;
78+
assert_messages!(snapshot, diagnostics);
79+
Ok(())
80+
}
81+
6482
#[test]
6583
fn write_whole_file_python_39() -> Result<()> {
6684
let diagnostics = test_path(

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use crate::preview::is_readlines_in_for_fix_safe;
12
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
23
use ruff_macros::{derive_message_formats, ViolationMetadata};
34
use ruff_python_ast::{Comprehension, Expr, StmtFor};
@@ -85,8 +86,14 @@ fn readlines_in_iter(checker: &Checker, iter_expr: &Expr) {
8586
}
8687

8788
let mut diagnostic = Diagnostic::new(ReadlinesInFor, expr_call.range());
88-
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_deletion(
89-
expr_call.range().add_start(expr_attr.value.range().len()),
90-
)));
89+
diagnostic.set_fix(if is_readlines_in_for_fix_safe(checker.settings) {
90+
Fix::safe_edit(Edit::range_deletion(
91+
expr_call.range().add_start(expr_attr.value.range().len()),
92+
))
93+
} else {
94+
Fix::unsafe_edit(Edit::range_deletion(
95+
expr_call.range().add_start(expr_attr.value.range().len()),
96+
))
97+
});
9198
checker.report_diagnostic(diagnostic);
9299
}

‎crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview__FURB129_FURB129.py.snap‎

Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
---
2+
source: crates/ruff_linter/src/rules/refurb/mod.rs
3+
---
4+
FURB129.py:7:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
5+
|
6+
5 | # Errors
7+
6 | with open("FURB129.py") as f:
8+
7 | for _line in f.readlines():
9+
| ^^^^^^^^^^^^^ FURB129
10+
8 | pass
11+
9 | a = [line.lower() for line in f.readlines()]
12+
|
13+
= help: Remove `readlines()`
14+
15+
ℹ Safe fix
16+
4 4 |
17+
5 5 | # Errors
18+
6 6 | with open("FURB129.py") as f:
19+
7 |- for _line in f.readlines():
20+
7 |+ for _line in f:
21+
8 8 | pass
22+
9 9 | a = [line.lower() for line in f.readlines()]
23+
10 10 | b = {line.upper() for line in f.readlines()}
24+
25+
FURB129.py:9:35: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
26+
|
27+
7 | for _line in f.readlines():
28+
8 | pass
29+
9 | a = [line.lower() for line in f.readlines()]
30+
| ^^^^^^^^^^^^^ FURB129
31+
10 | b = {line.upper() for line in f.readlines()}
32+
11 | c = {line.lower(): line.upper() for line in f.readlines()}
33+
|
34+
= help: Remove `readlines()`
35+
36+
ℹ Safe fix
37+
6 6 | with open("FURB129.py") as f:
38+
7 7 | for _line in f.readlines():
39+
8 8 | pass
40+
9 |- a = [line.lower() for line in f.readlines()]
41+
9 |+ a = [line.lower() for line in f]
42+
10 10 | b = {line.upper() for line in f.readlines()}
43+
11 11 | c = {line.lower(): line.upper() for line in f.readlines()}
44+
12 12 |
45+
46+
FURB129.py:10:35: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
47+
|
48+
8 | pass
49+
9 | a = [line.lower() for line in f.readlines()]
50+
10 | b = {line.upper() for line in f.readlines()}
51+
| ^^^^^^^^^^^^^ FURB129
52+
11 | c = {line.lower(): line.upper() for line in f.readlines()}
53+
|
54+
= help: Remove `readlines()`
55+
56+
ℹ Safe fix
57+
7 7 | for _line in f.readlines():
58+
8 8 | pass
59+
9 9 | a = [line.lower() for line in f.readlines()]
60+
10 |- b = {line.upper() for line in f.readlines()}
61+
10 |+ b = {line.upper() for line in f}
62+
11 11 | c = {line.lower(): line.upper() for line in f.readlines()}
63+
12 12 |
64+
13 13 | with Path("FURB129.py").open() as f:
65+
66+
FURB129.py:11:49: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
67+
|
68+
9 | a = [line.lower() for line in f.readlines()]
69+
10 | b = {line.upper() for line in f.readlines()}
70+
11 | c = {line.lower(): line.upper() for line in f.readlines()}
71+
| ^^^^^^^^^^^^^ FURB129
72+
12 |
73+
13 | with Path("FURB129.py").open() as f:
74+
|
75+
= help: Remove `readlines()`
76+
77+
ℹ Safe fix
78+
8 8 | pass
79+
9 9 | a = [line.lower() for line in f.readlines()]
80+
10 10 | b = {line.upper() for line in f.readlines()}
81+
11 |- c = {line.lower(): line.upper() for line in f.readlines()}
82+
11 |+ c = {line.lower(): line.upper() for line in f}
83+
12 12 |
84+
13 13 | with Path("FURB129.py").open() as f:
85+
14 14 | for _line in f.readlines():
86+
87+
FURB129.py:14:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
88+
|
89+
13 | with Path("FURB129.py").open() as f:
90+
14 | for _line in f.readlines():
91+
| ^^^^^^^^^^^^^ FURB129
92+
15 | pass
93+
|
94+
= help: Remove `readlines()`
95+
96+
ℹ Safe fix
97+
11 11 | c = {line.lower(): line.upper() for line in f.readlines()}
98+
12 12 |
99+
13 13 | with Path("FURB129.py").open() as f:
100+
14 |- for _line in f.readlines():
101+
14 |+ for _line in f:
102+
15 15 | pass
103+
16 16 |
104+
17 17 | for _line in open("FURB129.py").readlines():
105+
106+
FURB129.py:17:14: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
107+
|
108+
15 | pass
109+
16 |
110+
17 | for _line in open("FURB129.py").readlines():
111+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129
112+
18 | pass
113+
|
114+
= help: Remove `readlines()`
115+
116+
ℹ Safe fix
117+
14 14 | for _line in f.readlines():
118+
15 15 | pass
119+
16 16 |
120+
17 |-for _line in open("FURB129.py").readlines():
121+
17 |+for _line in open("FURB129.py"):
122+
18 18 | pass
123+
19 19 |
124+
20 20 | for _line in Path("FURB129.py").open().readlines():
125+
126+
FURB129.py:20:14: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
127+
|
128+
18 | pass
129+
19 |
130+
20 | for _line in Path("FURB129.py").open().readlines():
131+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB129
132+
21 | pass
133+
|
134+
= help: Remove `readlines()`
135+
136+
ℹ Safe fix
137+
17 17 | for _line in open("FURB129.py").readlines():
138+
18 18 | pass
139+
19 19 |
140+
20 |-for _line in Path("FURB129.py").open().readlines():
141+
20 |+for _line in Path("FURB129.py").open():
142+
21 21 | pass
143+
22 22 |
144+
23 23 |
145+
146+
FURB129.py:26:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
147+
|
148+
24 | def func():
149+
25 | f = Path("FURB129.py").open()
150+
26 | for _line in f.readlines():
151+
| ^^^^^^^^^^^^^ FURB129
152+
27 | pass
153+
28 | f.close()
154+
|
155+
= help: Remove `readlines()`
156+
157+
ℹ Safe fix
158+
23 23 |
159+
24 24 | def func():
160+
25 25 | f = Path("FURB129.py").open()
161+
26 |- for _line in f.readlines():
162+
26 |+ for _line in f:
163+
27 27 | pass
164+
28 28 | f.close()
165+
29 29 |
166+
167+
FURB129.py:32:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
168+
|
169+
31 | def func(f: io.BytesIO):
170+
32 | for _line in f.readlines():
171+
| ^^^^^^^^^^^^^ FURB129
172+
33 | pass
173+
|
174+
= help: Remove `readlines()`
175+
176+
ℹ Safe fix
177+
29 29 |
178+
30 30 |
179+
31 31 | def func(f: io.BytesIO):
180+
32 |- for _line in f.readlines():
181+
32 |+ for _line in f:
182+
33 33 | pass
183+
34 34 |
184+
35 35 |
185+
186+
FURB129.py:38:22: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
187+
|
188+
36 | def func():
189+
37 | with (open("FURB129.py") as f, foo as bar):
190+
38 | for _line in f.readlines():
191+
| ^^^^^^^^^^^^^ FURB129
192+
39 | pass
193+
40 | for _line in bar.readlines():
194+
|
195+
= help: Remove `readlines()`
196+
197+
ℹ Safe fix
198+
35 35 |
199+
36 36 | def func():
200+
37 37 | with (open("FURB129.py") as f, foo as bar):
201+
38 |- for _line in f.readlines():
202+
38 |+ for _line in f:
203+
39 39 | pass
204+
40 40 | for _line in bar.readlines():
205+
41 41 | pass
206+
207+
FURB129.py:48:17: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
208+
|
209+
47 | with builtins.open("FURB129.py") as f:
210+
48 | for line in f.readlines():
211+
| ^^^^^^^^^^^^^ FURB129
212+
49 | pass
213+
|
214+
= help: Remove `readlines()`
215+
216+
ℹ Safe fix
217+
45 45 |
218+
46 46 |
219+
47 47 | with builtins.open("FURB129.py") as f:
220+
48 |- for line in f.readlines():
221+
48 |+ for line in f:
222+
49 49 | pass
223+
50 50 |
224+
51 51 |
225+
226+
FURB129.py:56:17: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
227+
|
228+
55 | with o("FURB129.py") as f:
229+
56 | for line in f.readlines():
230+
| ^^^^^^^^^^^^^ FURB129
231+
57 | pass
232+
|
233+
= help: Remove `readlines()`
234+
235+
ℹ Safe fix
236+
53 53 |
237+
54 54 |
238+
55 55 | with o("FURB129.py") as f:
239+
56 |- for line in f.readlines():
240+
56 |+ for line in f:
241+
57 57 | pass
242+
58 58 |
243+
59 59 |

0 commit comments

Comments
 (0)