[`refurb`] Fix `FURB101` and `FURB103` false positives when I/O variable is used later by chirizxc · Pull Request #23542 · astral-sh/ruff

chirizxc · 2026-02-24T17:07:19Z

Summary

Test Plan

cargo nextest run furb

astral-sh-bot · 2026-02-24T17:16:47Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -6 violations, +0 -0 fixes in 4 projects; 52 projects unchanged)

apache/superset (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

+ superset/db_engine_specs/lint_metadata.py:676:18: FURB103 [*] `open` and `write` should be replaced by `Path(args.output).write_text(output_text)`
- superset/db_engine_specs/lint_metadata.py:676:18: FURB103 [*] `open` and `write` should be replaced by `Path(args.output).write_text(output_text)`

ibis-project/ibis (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ .github/workflows/algolia/upload-algolia-api.py:165:14: FURB101 `open` and `read` should be replaced by `Path(qmd).read_text()`
- .github/workflows/algolia/upload-algolia-api.py:165:14: FURB101 `open` and `read` should be replaced by `Path(qmd).read_text()`

zulip/zulip (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

+ zerver/lib/test_helpers.py:766:10: FURB103 [*] `open` and `write` should be replaced by `Path(filepath).write_text("zulip!")`
- zerver/lib/test_helpers.py:766:10: FURB103 [*] `open` and `write` should be replaced by `Path(filepath).write_text("zulip!")`

astropy/astropy (+2 -3 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview

+ astropy/io/ascii/tests/test_read.py:1776:30: FURB101 `open` and `read` should be replaced by `Path(fpath).read_text()`
- astropy/io/ascii/tests/test_read.py:1776:30: FURB101 `open` and `read` should be replaced by `Path(fpath).read_text()`
+ astropy/io/ascii/tests/test_read.py:2164:14: FURB101 [*] `open` and `read` should be replaced by `Path(filename).read_text()`
- astropy/io/ascii/tests/test_read.py:2164:14: FURB101 [*] `open` and `read` should be replaced by `Path(filename).read_text()`
- astropy/utils/tests/test_data.py:605:14: FURB103 [*] `open` and `write` should be replaced by `Path(f_name).write_text("old")`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
FURB101	6	3	3	0	0
FURB103	5	2	3	0	0

amyreese

Thanks for working on this. Overall I think this is a very large amount of code. Is all of this actually necessary just to see if a variable is used somewhere else? Can we skip checking if it's rebound elsewhere? Erring on the side of a false negative would generally be better than false positives, and if that could greatly reduce the amount of logic necessary, that may be worth it.

amyreese · 2026-02-24T22:20:30Z

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

+    let following_statements = following_statements_after_with(
+        with,
+        checker.semantic().current_statement_parent(),
+        checker.python_ast(),
+    );
+    let candidates = find_file_opens(


Rather than doing up-front work to find all the statements for every with block, even if it has nothing to do with opening a file, this should first see if there are any candidates, and only then fetch the following statements and filter the list of candidates afterwards.

Or at the very least, the call to get following statements should happen further inside of say, resolve_file_open where the data will actually be used, at the point that it will actually be needed and no sooner.

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

crates/ruff_linter/src/checkers/ast/mod.rs

amyreese · 2026-02-24T22:53:51Z

Also, looking at the ecosystem results, it seems we're already pushing towards false negatives here, so I'm not sure how much this is actually benefiting the rule. Were there any false positives in the ecosystem report that should have been resolved with this PR?

chirizxc · 2026-02-24T23:05:07Z

Also, looking at the ecosystem results, it seems we're already pushing towards false negatives here, so I'm not sure how much this is actually benefiting the rule. Were there any false positives in the ecosystem report that should have been resolved with this PR?

I looked at the changes in the ecosystem from the bot's post, none of these changes are false negatives, since the variables below are used in f.name or f.is_file()

amyreese · 2026-02-24T23:19:33Z

I looked at the changes in the ecosystem from the bot's post, none of these changes are false negatives, since the variables below are used in f.name or f.is_file()

maybe I'm missing something, but f in the airflow ecosystem examples are rebound as part of the comprehensions, so they were legit positives that have been silenced with this PR:

with open(file_path, "w") as f:
    f.write("hello world")

...

files_in_repo = {f.name for f in bundle.path.iterdir() if f.is_file()}

note the for f in ... part of the comprehension means f.name and f.is_file() are not referring to the same f from the context manager above.

chirizxc · 2026-02-25T07:42:21Z

I looked at the changes in the ecosystem from the bot's post, none of these changes are false negatives, since the variables below are used in f.name or f.is_file()

maybe I'm missing something, but f in the airflow ecosystem examples are rebound as part of the comprehensions, so they were legit positives that have been silenced with this PR:
with open(file_path, "w") as f:
    f.write("hello world")

...

files_in_repo = {f.name for f in bundle.path.iterdir() if f.is_file()}
note the for f in ... part of the comprehension means f.name and f.is_file() are not referring to the same f from the context manager above.

Yes, you were right, when I looked at these bot ecosystem results, I didn't pay attention

amyreese · 2026-02-28T00:18:01Z

Overall I'm still concerned about the amount of code here, especially when it resolves only a single false positive in the ecosystem report. Would prefer that this is simplified, and lean more on the existing features of the semantic model rather than reimplementing its own version of semantic model and control flow. Brent suggested that only_binding might be helpful here, or maybe consider just return early if the binding is used for anything beyond the read call, without trying to map control flow?

chirizxc · 2026-02-28T20:59:38Z

Overall I'm still concerned about the amount of code here, especially when it resolves only a single false positive in the ecosystem report. Would prefer that this is simplified, and lean more on the existing features of the semantic model rather than reimplementing its own version of semantic model and control flow. Brent suggested that only_binding might be helpful here, or maybe consider just return early if the binding is used for anything beyond the read call, without trying to map control flow?

I will try to reduce the amount of code, but ecosystem analysis projects do not involve all Python code, so I think there is more than one case.

ntBre · 2026-03-02T20:01:42Z

I was mostly surprised that we need a new Visitor here. Would moving the rule to a different analysis phase help? I thought we had pretty nice helpers for checking if a variable is used later without tracking that information in a new visitor.

chirizxc · 2026-03-02T22:50:33Z

I was mostly surprised that we need a new Visitor here. Would moving the rule to a different analysis phase help? I thought we had pretty nice helpers for checking if a variable is used later without tracking that information in a new visitor.

Hmm, I didn't even know about that. It seems more logical now. Thank you very much.

amyreese

Looks much better to me, but I'm not quite familiar with the analysis bits.

amyreese · 2026-03-03T20:11:56Z

Will wait for @ntBre to take a look.

ntBre

Thank you! Yes, this makes sense to me. I'm slightly wary of deferring too many checks, especially since I made a similar suggestion twice in quick succession after not using it much before, but this does seem better than trying to restructure the rules to fit into deferred_scopes. I also don't see any negative perf effects of the deferral, which makes sense to me given how lightweight the snapshots are.

I think this is fine as-is, but if we defer any more types of statements, it probably makes sense to have a single deferred_statements with a match on the statement kind instead of adding more deferred_for_loops, deferred_with_statements type functions and fields.

chirizxc added 2 commits February 24, 2026 20:06

[refurb] Fix FURB101, FURB103

1ea6f03

cargo fmt --all

0c57022

fix clippy

9403238

chirizxc changed the title ~~[refurb] Fix FURB101, FURB103~~ [refurb] Fix FURB101 and FURB103 false positives when I/O variable is used later Feb 24, 2026

add snap's

db240c2

chirizxc changed the title ~~[refurb] Fix FURB101 and FURB103 false positives when I/O variable is used later~~ [refurb] Fix FURB101 and FURB103 false positives when I/O variable is used later Feb 24, 2026

This comment was marked as resolved.

Sign in to view

fix

9421266

amyreese requested changes Feb 24, 2026

View reviewed changes

amyreese added the fixes Related to suggested fixes for violations label Feb 24, 2026

amyreese self-assigned this Feb 25, 2026

chirizxc added 2 commits February 25, 2026 10:06

some fixes

660e94a

fmt

5e42d3d

chirizxc marked this pull request as draft February 25, 2026 07:08

chirizxc added 3 commits February 25, 2026 10:15

fix clippy

9b38563

refactor

a731a67

update

cc6df68

chirizxc marked this pull request as ready for review February 26, 2026 18:55

chirizxc added 5 commits March 1, 2026 14:42

update

ce117a5

update

3121d9c

update

0068158

update

73218bb

fix clippy

89f08bb

chirizxc added 4 commits March 3, 2026 01:50

analysis phase

f14a311

unused pub(crate)

0f78569

update

ffa9c4a

minimize diff

0b33f60

amyreese approved these changes Mar 3, 2026

View reviewed changes

amyreese requested a review from ntBre March 3, 2026 20:11

Merge branch 'main' into FURB10_{1,3}

153f9ee

ntBre approved these changes Mar 3, 2026

View reviewed changes

ntBre added the preview Related to preview mode features label Mar 3, 2026

amyreese merged commit d740e84 into astral-sh:main Mar 4, 2026
44 checks passed

BrewTestBot mentioned this pull request Mar 5, 2026

ruff 0.15.5 Homebrew/homebrew-core#270824

Merged

Conversation

chirizxc commented Feb 24, 2026

Summary

Test Plan

Uh oh!

astral-sh-bot bot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

This comment was marked as resolved.

amyreese left a comment

Choose a reason for hiding this comment

Uh oh!

amyreese Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amyreese commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chirizxc commented Feb 24, 2026

Uh oh!

amyreese commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chirizxc commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amyreese commented Feb 28, 2026

Uh oh!

chirizxc commented Feb 28, 2026

Uh oh!

ntBre commented Mar 2, 2026

Uh oh!

chirizxc commented Mar 2, 2026

Uh oh!

amyreese left a comment

Choose a reason for hiding this comment

Uh oh!

amyreese commented Mar 3, 2026

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

astral-sh-bot bot commented Feb 24, 2026 •

edited

Loading

`ruff-ecosystem` results

amyreese commented Feb 24, 2026 •

edited

Loading

amyreese commented Feb 24, 2026 •

edited

Loading

chirizxc commented Feb 25, 2026 •

edited

Loading