diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB101_2.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101_2.py new file mode 100644 index 0000000000000..11b66cf125544 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101_2.py @@ -0,0 +1,21 @@ +# FURB101 +with open("file.txt", encoding="utf-8") as f: + _ = f.read() +f = object() +print(f) + +# See: https://github.com/astral-sh/ruff/issues/21483 +with open("file.txt", encoding="utf-8") as f: + _ = f.read() +print(f.mode) + +# Rebinding in a later `with ... as config_file` should not suppress this one. +with open("config.yaml", encoding="utf-8") as config_file: + config_raw = config_file.read() + +if "tts:" in config_raw: + try: + with open("config.yaml", "w", encoding="utf-8") as config_file: + config_file.write(config_raw.replace("tts:", "google_translate:")) + except OSError: + pass diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB103_2.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103_2.py new file mode 100644 index 0000000000000..0de353e0bdf69 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103_2.py @@ -0,0 +1,26 @@ +# FURB103 +# should trigger +with open("file.txt", "w", encoding="utf-8") as f: + f.write("\n") +f = object() +print(f) + +# See: https://github.com/astral-sh/ruff/issues/21483 +with open("file.txt", "w") as f: + f.write("\n") +print(f.encoding) + + +def _(): + # should trigger + with open("file.txt", "w") as f: + f.write("\n") + return (f.name for _ in [0]) + + +def _set(): + # should trigger + with open("file.txt", "w") as f: + f.write("\n") + g = {f.name for _ in [0]} + return g diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_with_statements.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_with_statements.rs new file mode 100644 index 0000000000000..db2afcafc6a70 --- /dev/null +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_with_statements.rs @@ -0,0 +1,23 @@ +use ruff_python_ast::Stmt; + +use crate::{checkers::ast::Checker, codes::Rule, rules::refurb}; + +/// Run lint rules over all deferred with-statements in the [`SemanticModel`]. +pub(crate) fn deferred_with_statements(checker: &mut Checker) { + while !checker.analyze.with_statements.is_empty() { + let with_statements = std::mem::take(&mut checker.analyze.with_statements); + for snapshot in with_statements { + checker.semantic.restore(snapshot); + + let Stmt::With(stmt_with) = checker.semantic.current_statement() else { + unreachable!("Expected Stmt::With"); + }; + if checker.is_rule_enabled(Rule::ReadWholeFile) { + refurb::rules::read_whole_file(checker, stmt_with); + } + if checker.is_rule_enabled(Rule::WriteWholeFile) { + refurb::rules::write_whole_file(checker, stmt_with); + } + } + } +} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs index ce1d2f08aec76..b3c149690fd5a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/mod.rs @@ -4,6 +4,7 @@ pub(super) use deferred_comprehensions::deferred_comprehensions; pub(super) use deferred_for_loops::deferred_for_loops; pub(super) use deferred_lambdas::deferred_lambdas; pub(super) use deferred_scopes::deferred_scopes; +pub(super) use deferred_with_statements::deferred_with_statements; pub(super) use definitions::definitions; pub(super) use except_handler::except_handler; pub(super) use expression::expression; @@ -21,6 +22,7 @@ mod deferred_comprehensions; mod deferred_for_loops; mod deferred_lambdas; mod deferred_scopes; +mod deferred_with_statements; mod definitions; mod except_handler; mod expression; diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 1fa1042233451..51f2fa74f5383 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1181,11 +1181,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.is_rule_enabled(Rule::RedefinedLoopName) { pylint::rules::redefined_loop_name(checker, stmt); } - if checker.is_rule_enabled(Rule::ReadWholeFile) { - refurb::rules::read_whole_file(checker, with_stmt); - } - if checker.is_rule_enabled(Rule::WriteWholeFile) { - refurb::rules::write_whole_file(checker, with_stmt); + if checker.any_rule_enabled(&[Rule::ReadWholeFile, Rule::WriteWholeFile]) { + checker + .analyze + .with_statements + .push(checker.semantic.snapshot()); } if checker.is_rule_enabled(Rule::UselessWithLock) { pylint::rules::useless_with_lock(checker, with_stmt); diff --git a/crates/ruff_linter/src/checkers/ast/deferred.rs b/crates/ruff_linter/src/checkers/ast/deferred.rs index aa4ec80094c25..5dc9bfe789114 100644 --- a/crates/ruff_linter/src/checkers/ast/deferred.rs +++ b/crates/ruff_linter/src/checkers/ast/deferred.rs @@ -34,5 +34,6 @@ pub(crate) struct Analyze { pub(crate) scopes: Vec, pub(crate) lambdas: Vec, pub(crate) for_loops: Vec, + pub(crate) with_statements: Vec, pub(crate) comprehensions: Vec, } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index cee5ce35bdc96..475839030296a 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -3321,6 +3321,7 @@ pub(crate) fn check_ast( analyze::definitions(&mut checker); analyze::bindings(&checker); analyze::unresolved_references(&checker); + analyze::deferred_with_statements(&mut checker); // Reset the scope to module-level, and check all consumed scopes. checker.semantic.scope_id = ScopeId::global(); diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index 3b1d20e7844ce..70666e43b49b0 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/crates/ruff_linter/src/rules/refurb/helpers.rs @@ -217,23 +217,29 @@ fn resolve_file_open<'a>( if matches!(mode, OpenMode::ReadBytes | OpenMode::WriteBytes) && !keywords.is_empty() { return None; } + let var = item.optional_vars.as_deref()?.as_name_expr()?; let scope = semantic.current_scope(); - let binding = scope.get_all(var.id.as_str()).find_map(|id| { - let b = semantic.binding(id); - (b.range() == var.range()).then_some(b) + let binding = semantic.binding(id); + (binding.range() == var.range()).then_some(binding) })?; - let references: Vec<&ResolvedReference> = binding - .references - .iter() - .map(|id| semantic.reference(*id)) - .filter(|reference| with.range().contains_range(reference.range())) - .collect(); - - let [reference] = references.as_slice() else { + let mut binding_references = binding + .references() + .map(|id| semantic.reference(id)) + // Reassignments in the same scope can carry forward older references. Ignore anything + // that appears before this `with` statement and only consider references from this point. + .filter(|reference| { + reference.scope_id() == binding.scope && reference.start() >= with.start() + }); + + let reference = binding_references.next()?; + if binding_references.next().is_some() { return None; - }; + } + if !with.range().contains_range(reference.range()) { + return None; + } Some(FileOpen { item, @@ -279,6 +285,7 @@ fn find_file_open<'a>( let (keywords, kw_mode) = match_open_keywords(keywords, read_mode, python_version)?; let mode = kw_mode.unwrap_or(pos_mode); + resolve_file_open( item, with, @@ -307,9 +314,11 @@ fn find_path_open<'a>( { return None; } + if !is_open_call_from_pathlib(func, semantic) { return None; } + let attr = func.as_attribute_expr()?; let mode = if args.is_empty() { OpenMode::ReadText @@ -319,6 +328,7 @@ fn find_path_open<'a>( let (keywords, kw_mode) = match_open_keywords(keywords, read_mode, python_version)?; let mode = kw_mode.unwrap_or(mode); + resolve_file_open( item, with, diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index a98a770c93ce2..2e32970a4028b 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -17,6 +17,7 @@ mod tests { #[test_case(Rule::ReadWholeFile, Path::new("FURB101_0.py"))] #[test_case(Rule::ReadWholeFile, Path::new("FURB101_1.py"))] + #[test_case(Rule::ReadWholeFile, Path::new("FURB101_2.py"))] #[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))] #[test_case(Rule::IfExpInsteadOfOrOperator, Path::new("FURB110.py"))] #[test_case(Rule::ReimplementedOperator, Path::new("FURB118.py"))] @@ -49,6 +50,7 @@ mod tests { #[test_case(Rule::ListReverseCopy, Path::new("FURB187.py"))] #[test_case(Rule::WriteWholeFile, Path::new("FURB103_0.py"))] #[test_case(Rule::WriteWholeFile, Path::new("FURB103_1.py"))] + #[test_case(Rule::WriteWholeFile, Path::new("FURB103_2.py"))] #[test_case(Rule::FStringNumberFormat, Path::new("FURB116.py"))] #[test_case(Rule::SortedMinMax, Path::new("FURB192.py"))] #[test_case(Rule::SliceToRemovePrefixOrSuffix, Path::new("FURB188.py"))] diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101_2.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101_2.py.snap new file mode 100644 index 0000000000000..00d2098e90620 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101_2.py.snap @@ -0,0 +1,47 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +assertion_line: 65 +--- +FURB101 [*] `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101_2.py:2:6 + | +1 | # FURB101 +2 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +3 | _ = f.read() +4 | f = object() + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")` +1 | # FURB101 + - with open("file.txt", encoding="utf-8") as f: + - _ = f.read() +2 + import pathlib +3 + _ = pathlib.Path("file.txt").read_text(encoding="utf-8") +4 | f = object() +5 | print(f) +6 | + +FURB101 [*] `open` and `read` should be replaced by `Path("config.yaml").read_text(encoding="utf-8")` + --> FURB101_2.py:13:6 + | +12 | # Rebinding in a later `with ... as config_file` should not suppress this one. +13 | with open("config.yaml", encoding="utf-8") as config_file: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +14 | config_raw = config_file.read() + | +help: Replace with `Path("config.yaml").read_text(encoding="utf-8")` +1 | # FURB101 +2 + import pathlib +3 | with open("file.txt", encoding="utf-8") as f: +4 | _ = f.read() +5 | f = object() +-------------------------------------------------------------------------------- +11 | print(f.mode) +12 | +13 | # Rebinding in a later `with ... as config_file` should not suppress this one. + - with open("config.yaml", encoding="utf-8") as config_file: + - config_raw = config_file.read() +14 + config_raw = pathlib.Path("config.yaml").read_text(encoding="utf-8") +15 | +16 | if "tts:" in config_raw: +17 | try: diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_2.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_2.py.snap new file mode 100644 index 0000000000000..8564dd17b0789 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_2.py.snap @@ -0,0 +1,78 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB103 [*] `open` and `write` should be replaced by `Path("file.txt").write_text("\n", encoding="utf-8")` + --> FURB103_2.py:3:6 + | +1 | # FURB103 +2 | # should trigger +3 | with open("file.txt", "w", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +4 | f.write("\n") +5 | f = object() + | +help: Replace with `Path("file.txt").write_text("\n", encoding="utf-8")` +1 | # FURB103 +2 | # should trigger + - with open("file.txt", "w", encoding="utf-8") as f: + - f.write("\n") +3 + import pathlib +4 + pathlib.Path("file.txt").write_text("\n", encoding="utf-8") +5 | f = object() +6 | print(f) +7 | + +FURB103 [*] `open` and `write` should be replaced by `Path("file.txt").write_text("\n")` + --> FURB103_2.py:16:10 + | +14 | def _(): +15 | # should trigger +16 | with open("file.txt", "w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +17 | f.write("\n") +18 | return (f.name for _ in [0]) + | +help: Replace with `Path("file.txt").write_text("\n")` +1 | # FURB103 +2 | # should trigger +3 + import pathlib +4 | with open("file.txt", "w", encoding="utf-8") as f: +5 | f.write("\n") +6 | f = object() +-------------------------------------------------------------------------------- +14 | +15 | def _(): +16 | # should trigger + - with open("file.txt", "w") as f: + - f.write("\n") +17 + pathlib.Path("file.txt").write_text("\n") +18 | return (f.name for _ in [0]) +19 | +20 | + +FURB103 [*] `open` and `write` should be replaced by `Path("file.txt").write_text("\n")` + --> FURB103_2.py:23:10 + | +21 | def _set(): +22 | # should trigger +23 | with open("file.txt", "w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +24 | f.write("\n") +25 | g = {f.name for _ in [0]} + | +help: Replace with `Path("file.txt").write_text("\n")` +1 | # FURB103 +2 | # should trigger +3 + import pathlib +4 | with open("file.txt", "w", encoding="utf-8") as f: +5 | f.write("\n") +6 | f = object() +-------------------------------------------------------------------------------- +21 | +22 | def _set(): +23 | # should trigger + - with open("file.txt", "w") as f: + - f.write("\n") +24 + pathlib.Path("file.txt").write_text("\n") +25 | g = {f.name for _ in [0]} +26 | return g