From d0cd4aefd6c2a29523412385dc70518dc862122a Mon Sep 17 00:00:00 2001 From: augustelalande Date: Fri, 5 Apr 2024 22:05:22 -0400 Subject: [PATCH 1/5] implement write-whole-file --- .../resources/test/fixtures/refurb/FURB103.py | 123 ++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/refurb/mod.rs | 1 + .../ruff_linter/src/rules/refurb/rules/mod.rs | 2 + .../rules/refurb/rules/write_whole_file.rs | 355 ++++++++++++++++++ ...es__refurb__tests__FURB103_FURB103.py.snap | 94 +++++ ruff.schema.json | 1 + scripts/add_rule.py | 7 +- 9 files changed, 583 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py create mode 100644 crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs create mode 100644 crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py new file mode 100644 index 0000000000000..d715dd8ef55ca --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py @@ -0,0 +1,123 @@ +def foo(): + ... + + +def bar(x): + ... + + +# Errors. + +# FURB103 +with open("file.txt", "w") as f: + f.write("test") + +# FURB103 +with open("file.txt", "wb") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", mode="wb") as f: + f.write(b"abc") + +# FURB103 +with open("file.txt", "w", encoding="utf8") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", "w", errors="ignore") as f: + f.write(foobar) + +# FURB103 +with open("file.txt", mode="w") as f: + f.write(foobar) + +# FURB103 +with open(foo(), "wb") as f: + # The body of `with` is non-trivial, but the recommendation holds. + bar("pre") + f.write(bar()) + bar("post") + print("Done") + +# FURB103 +with open("a.txt", "w") as a, open("b.txt", "wb") as b: + a.write(x) + b.write(y) + +# FURB103 +with foo() as a, open("file.txt", "w") as b, foo() as c: + # We have other things in here, multiple with items, but the user + # writes a single time to file and that bit they can replace. + bar(a) + b.write(bar(bar(a + x))) + bar(c) + + +# FURB103 +with open("file.txt", "w", newline="\r\n") as f: + f.write(foobar) + + +# Non-errors. + +with open("file.txt", errors="ignore", mode="wb") as f: + # Path.write_bytes() does not support errors + f.write(foobar) + +f2 = open("file2.txt", "w") +with open("file.txt", "w") as f: + f2.write(x) + +# mode is dynamic +with open("file.txt", foo()) as f: + f.write(x) + +# keyword mode is incorrect +with open("file.txt", mode="a+") as f: + f.write(x) + +# enables line buffering, not supported in write_text() +with open("file.txt", buffering=1) as f: + f.write(x) + +# dont mistake "newline" for "mode" +with open("file.txt", newline="wb") as f: + f.write(x) + +# I guess we can possibly also report this case, but the question +# is why the user would put "w+" here in the first place. +with open("file.txt", "w+") as f: + f.write(x) + +# Even though we write the whole file, we do other things. +with open("file.txt", "w") as f: + f.write(x) + f.seek(0) + x += f.read(100) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open(*filename, mode="w") as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open(**kwargs) as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open("file.txt", **kwargs) as f: + f.write(x) + +# This shouldn't error, since it could contain unsupported arguments, like `buffering`. +with open("file.txt", mode="w", **kwargs) as f: + f.write(x) + +# This could error (but doesn't), since it can't contain unsupported arguments, like +# `buffering`. +with open(*filename, mode="w") as f: + f.write(x) + +# This could error (but doesn't), since it can't contain unsupported arguments, like +# `buffering`. +with open(*filename, file="file.txt", mode="w") as f: + f.write(x) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 7e8293172916f..bff7e5658b6fb 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1217,6 +1217,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ReadWholeFile) { refurb::rules::read_whole_file(checker, with_stmt); } + if checker.enabled(Rule::WriteWholeFile) { + refurb::rules::write_whole_file(checker, with_stmt); + } if checker.enabled(Rule::UselessWithLock) { pylint::rules::useless_with_lock(checker, with_stmt); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 10a7fb752592f..c83ee76b3bef9 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1035,6 +1035,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // refurb (Refurb, "101") => (RuleGroup::Preview, rules::refurb::rules::ReadWholeFile), + (Refurb, "103") => (RuleGroup::Preview, rules::refurb::rules::WriteWholeFile), (Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString), #[allow(deprecated)] (Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend), diff --git a/crates/ruff_linter/src/rules/refurb/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index f27f2291d1135..37a91ca51dfbf 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -40,6 +40,7 @@ mod tests { #[test_case(Rule::MetaClassABCMeta, Path::new("FURB180.py"))] #[test_case(Rule::HashlibDigestHex, Path::new("FURB181.py"))] #[test_case(Rule::ListReverseCopy, Path::new("FURB187.py"))] + #[test_case(Rule::WriteWholeFile, Path::new("FURB103.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/refurb/rules/mod.rs b/crates/ruff_linter/src/rules/refurb/rules/mod.rs index c18f2350ecd4d..34d760b48dd05 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -24,6 +24,7 @@ pub(crate) use type_none_comparison::*; pub(crate) use unnecessary_enumerate::*; pub(crate) use unnecessary_from_float::*; pub(crate) use verbose_decimal_constructor::*; +pub(crate) use write_whole_file::*; mod bit_count; mod check_and_remove_from_set; @@ -51,3 +52,4 @@ mod type_none_comparison; mod unnecessary_enumerate; mod unnecessary_from_float; mod verbose_decimal_constructor; +mod write_whole_file; diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs new file mode 100644 index 0000000000000..066e1fb737d22 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -0,0 +1,355 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::visitor::{self, Visitor}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_codegen::Generator; +use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; +use crate::fix::snippet::SourceCodeSnippet; + +/// ## What it does +/// Checks for uses of `open` and `write` that can be replaced by `pathlib` +/// methods, like `Path.write_text` and `Path.write_bytes`. +/// +/// ## Why is this bad? +/// When writing a single string to a file, it's simpler and more concise +/// to use `pathlib` methods like `Path.write_text` and `Path.write_bytes` +/// instead of `open` and `write` calls via `with` statements. +/// +/// ## Example +/// ```python +/// with open(filename, "w") as f: +/// f.write(contents) +/// ``` +/// +/// Use instead: +/// ```python +/// from pathlib import Path +/// +/// Path(filename).write_text(contents) +/// ``` +/// +/// ## References +/// - [Python documentation: `Path.write_bytes`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_bytes) +/// - [Python documentation: `Path.write_text`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_text) +#[violation] +pub struct WriteWholeFile { + filename: SourceCodeSnippet, + suggestion: SourceCodeSnippet, +} + +impl Violation for WriteWholeFile { + #[derive_message_formats] + fn message(&self) -> String { + let filename = self.filename.truncated_display(); + let suggestion = self.suggestion.truncated_display(); + format!("`open` and `write` should be replaced by `Path({filename}).{suggestion}`") + } +} + +/// FURB103 +pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { + // `async` check here is more of a precaution. + if with.is_async || !checker.semantic().is_builtin("open") { + return; + } + + // First we go through all the items in the statement and find all `open` operations. + let candidates = find_file_opens(with, checker.semantic()); + if candidates.is_empty() { + return; + } + + // Then we need to match each `open` operation with exactly one `write` call. + let mut matcher = WriteMatcher::new(candidates); + visitor::walk_body(&mut matcher, &with.body); + let contents = matcher.contents(); + + // All the matched operations should be reported. + let diagnostics: Vec = matcher + .into_matches() + .iter() + .zip(contents) + .map(|(open, content)| { + Diagnostic::new( + WriteWholeFile { + filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), + suggestion: make_suggestion(open, content, checker.generator()), + }, + open.item.range(), + ) + }) + .collect(); + checker.diagnostics.extend(diagnostics); +} + +#[derive(Debug)] +enum WriteMode { + /// "w" -> `write_text` + Text, + /// "wb" -> `write_bytes` + Bytes, +} + +/// A grab bag struct that joins together every piece of information we need to track +/// about a file open operation. +#[derive(Debug)] +struct FileOpen<'a> { + /// With item where the open happens, we use it for the reporting range. + item: &'a ast::WithItem, + /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. + filename: &'a Expr, + /// The type of write to choose `write_text` or `write_bytes`. + mode: WriteMode, + /// Keywords that can be used in the new write call. + keywords: Vec<&'a ast::Keyword>, + /// We only check `open` operations whose file handles are used exactly once. + reference: &'a ResolvedReference, +} + +impl<'a> FileOpen<'a> { + /// Determine whether an expression is a reference to the file handle, by comparing + /// their ranges. If two expressions have the same range, they must be the same expression. + fn is_ref(&self, expr: &Expr) -> bool { + expr.range() == self.reference.range() + } +} + +/// Find and return all `open` operations in the given `with` statement. +fn find_file_opens<'a>( + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, +) -> Vec> { + with.items + .iter() + .filter_map(|item| find_file_open(item, with, semantic)) + .collect() +} + +/// Find `open` operation in the given `with` item. +fn find_file_open<'a>( + item: &'a ast::WithItem, + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, +) -> Option> { + // We want to match `open(...) as var`. + let ast::ExprCall { + func, + arguments: ast::Arguments { args, keywords, .. }, + .. + } = item.context_expr.as_call_expr()?; + + if func.as_name_expr()?.id != "open" { + return None; + } + + let var = item.optional_vars.as_deref()?.as_name_expr()?; + + // Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`, + // it could be a match; but in all other cases, the call _could_ contain unsupported keyword + // arguments, like `buffering`. + if args.iter().any(Expr::is_starred_expr) + || keywords.iter().any(|keyword| keyword.arg.is_none()) + { + return None; + } + + // Match positional arguments, get filename and mode. + let (filename, pos_mode) = match_open_args(args)?; + + // Match keyword arguments, get keyword arguments to forward and possibly mode. + let (keywords, kw_mode) = match_open_keywords(keywords)?; + + let mode = match kw_mode { + Some(kw) => kw, + None => match pos_mode { + Some(pos) => pos, + None => return None, // Indicates default mode "r", which is incompatible. + }, + }; + + // Path.write_bytes does not support any kwargs. + if matches!(mode, WriteMode::Bytes) && !keywords.is_empty() { + return None; + } + + // Now we need to find what is this variable bound to... + let scope = semantic.current_scope(); + let bindings: Vec = scope.get_all(var.id.as_str()).collect(); + + let binding = bindings + .iter() + .map(|x| semantic.binding(*x)) + // We might have many bindings with the same name, but we only care + // for the one we are looking at right now. + .find(|binding| binding.range() == var.range())?; + + // Since many references can share the same binding, we can limit our attention span + // exclusively to the body of the current `with` statement. + let references: Vec<&ResolvedReference> = binding + .references + .iter() + .map(|id| semantic.reference(*id)) + .filter(|reference| with.range().contains_range(reference.range())) + .collect(); + + // And even with all these restrictions, if the file handle gets used not exactly once, + // it doesn't fit the bill. + let [reference] = references.as_slice() else { + return None; + }; + + Some(FileOpen { + item, + filename, + mode, + keywords, + reference, + }) +} + +/// Match positional arguments. Return expression for the file name and mode. +fn match_open_args(args: &[Expr]) -> Option<(&Expr, Option)> { + match args { + // If only one argument indicating read mode, the mode could still be set as a kwarg. + [filename] => Some((filename, None)), + [filename, mode_literal] => { + match_open_mode(mode_literal).map(|mode| (filename, Some(mode))) + } + // The third positional argument is `buffering` and `write_text` doesn't support it. + _ => None, + } +} + +/// Match keyword arguments. Return keyword arguments to forward and mode. +fn match_open_keywords( + keywords: &[ast::Keyword], +) -> Option<(Vec<&ast::Keyword>, Option)> { + let mut result: Vec<&ast::Keyword> = vec![]; + let mut mode: Option = None; + + for keyword in keywords { + match keyword.arg.as_ref()?.as_str() { + "encoding" | "errors" | "newline" => result.push(keyword), + + // This might look bizarre, - why do we re-wrap this optional? + // + // The answer is quite simple, in the result of the current function + // mode being `None` is a possible and correct option meaning that there + // was NO "mode" keyword argument. + // + // The result of `match_open_mode` on the other hand is None + // in the cases when the mode is not compatible with `write_text`/`write_bytes`. + // + // So, here we return None from this whole function if the mode + // is incompatible. + "mode" => mode = Some(match_open_mode(&keyword.value)?), + + // All other keywords cannot be directly forwarded. + _ => return None, + }; + } + Some((result, mode)) +} + +/// Match open mode to see if it is supported. +fn match_open_mode(mode: &Expr) -> Option { + let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; + if value.is_implicit_concatenated() { + return None; + } + match value.to_str() { + "w" => Some(WriteMode::Text), + "wb" => Some(WriteMode::Bytes), + _ => None, + } +} + +/// AST visitor that matches `open` operations with the corresponding `write` calls. +#[derive(Debug)] +struct WriteMatcher<'a> { + candidates: Vec>, + matches: Vec>, + contents: Vec>, +} + +impl<'a> WriteMatcher<'a> { + fn new(candidates: Vec>) -> Self { + Self { + candidates, + matches: vec![], + contents: vec![], + } + } + + fn into_matches(self) -> Vec> { + self.matches + } + + fn contents(&self) -> Vec> { + self.contents.clone() + } +} + +impl<'a> Visitor<'a> for WriteMatcher<'a> { + fn visit_expr(&mut self, expr: &'a Expr) { + if let Some((write_to, content)) = match_write_call(expr) { + if let Some(open) = self + .candidates + .iter() + .position(|open| open.is_ref(write_to)) + { + self.matches.push(self.candidates.remove(open)); + self.contents.push(content); + } + return; + } + visitor::walk_expr(self, expr); + } +} + +/// Match `x.write(foo)` expression and return expression `x` and `foo` on success. +fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { + let call = expr.as_call_expr()?; + let attr = call.func.as_attribute_expr()?; + let method_name = &attr.attr; + + if method_name != "write" + || !attr.value.is_name_expr() + || call.arguments.args.len() != 1 + || !call.arguments.keywords.is_empty() + { + return None; + } + + Some((attr.value.as_ref(), call.arguments.args.as_ref().to_vec())) +} + +/// Construct the replacement suggestion call. +fn make_suggestion( + open: &FileOpen<'_>, + write_arguments: Vec, + generator: Generator, +) -> SourceCodeSnippet { + let method_name = match open.mode { + WriteMode::Text => "write_text", + WriteMode::Bytes => "write_bytes", + }; + let name = ast::ExprName { + id: method_name.to_string(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + let call = ast::ExprCall { + func: Box::new(name.into()), + arguments: ast::Arguments { + args: Box::from(write_arguments), + keywords: open.keywords.iter().copied().cloned().collect(), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + SourceCodeSnippet::from_str(&generator.expr(&call.into())) +} diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap new file mode 100644 index 0000000000000..9a37a5447d495 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap @@ -0,0 +1,94 @@ +--- +source: crates/ruff_linter/src/rules/refurb/mod.rs +--- +FURB103.py:12:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text("test")` + | +11 | # FURB103 +12 | with open("file.txt", "w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +13 | f.write("test") + | + +FURB103.py:16:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_bytes(foobar)` + | +15 | # FURB103 +16 | with open("file.txt", "wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +17 | f.write(foobar) + | + +FURB103.py:20:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_bytes(b"abc")` + | +19 | # FURB103 +20 | with open("file.txt", mode="wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +21 | f.write(b"abc") + | + +FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(encoding="utf8", foobar)` + | +23 | # FURB103 +24 | with open("file.txt", "w", encoding="utf8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +25 | f.write(foobar) + | + +FURB103.py:28:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(errors="ignore", foobar)` + | +27 | # FURB103 +28 | with open("file.txt", "w", errors="ignore") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +29 | f.write(foobar) + | + +FURB103.py:32:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(foobar)` + | +31 | # FURB103 +32 | with open("file.txt", mode="w") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +33 | f.write(foobar) + | + +FURB103.py:36:6: FURB103 `open` and `write` should be replaced by `Path(foo()).write_bytes(bar())` + | +35 | # FURB103 +36 | with open(foo(), "wb") as f: + | ^^^^^^^^^^^^^^^^^^^^^^ FURB103 +37 | # The body of `with` is non-trivial, but the recommendation holds. +38 | bar("pre") + | + +FURB103.py:44:6: FURB103 `open` and `write` should be replaced by `Path("a.txt").write_text(x)` + | +43 | # FURB103 +44 | with open("a.txt", "w") as a, open("b.txt", "wb") as b: + | ^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +45 | a.write(x) +46 | b.write(y) + | + +FURB103.py:44:31: FURB103 `open` and `write` should be replaced by `Path("b.txt").write_bytes(y)` + | +43 | # FURB103 +44 | with open("a.txt", "w") as a, open("b.txt", "wb") as b: + | ^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +45 | a.write(x) +46 | b.write(y) + | + +FURB103.py:49:18: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(bar(bar(a + x)))` + | +48 | # FURB103 +49 | with foo() as a, open("file.txt", "w") as b, foo() as c: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +50 | # We have other things in here, multiple with items, but the user +51 | # writes a single time to file and that bit they can replace. + | + +FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(newline="\r\n", foobar)` + | +57 | # FURB103 +58 | with open("file.txt", "w", newline="\r\n") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB103 +59 | f.write(foobar) + | diff --git a/ruff.schema.json b/ruff.schema.json index 4aa04519ad805..ab68b675b3bd1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3041,6 +3041,7 @@ "FURB1", "FURB10", "FURB101", + "FURB103", "FURB105", "FURB11", "FURB113", diff --git a/scripts/add_rule.py b/scripts/add_rule.py index bfc61b733e8ae..d324e8001b4e5 100755 --- a/scripts/add_rule.py +++ b/scripts/add_rule.py @@ -9,6 +9,7 @@ --code 807 \ --linter flake8-pie """ + from __future__ import annotations import argparse @@ -26,9 +27,7 @@ def main(*, name: str, prefix: str, code: str, linter: str) -> None: / "crates/ruff_linter/resources/test/fixtures" / dir_name(linter) / f"{filestem}.py" - ).open( - "a", - ): + ).open("a"): pass plugin_module = ROOT_DIR / "crates/ruff_linter/src/rules" / dir_name(linter) @@ -140,7 +139,7 @@ def main(*, name: str, prefix: str, code: str, linter: str) -> None: variant = pascal_case(linter) rule = f"""rules::{linter.split(" ")[0]}::rules::{name}""" lines.append( - " " * 8 + f"""({variant}, "{code}") => (RuleGroup::Stable, {rule}),\n""", + " " * 8 + f"""({variant}, "{code}") => (RuleGroup::Preview, {rule}),\n""", ) lines.sort() text += "".join(lines) From f8e6e9840a3459f9b24e5e1f8964eb61acf111a1 Mon Sep 17 00:00:00 2001 From: augustelalande Date: Tue, 9 Apr 2024 20:20:08 -0400 Subject: [PATCH 2/5] ignore loops when detecting violations --- .../resources/test/fixtures/refurb/FURB103.py | 9 ++++++++ .../rules/refurb/rules/write_whole_file.rs | 22 ++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py index d715dd8ef55ca..2ba42f4ad823c 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py @@ -121,3 +121,12 @@ def bar(x): # `buffering`. with open(*filename, file="file.txt", mode="w") as f: f.write(x) + +# Loops imply multiple writes +with open("file.txt", "w") as f: + while x < 0: + f.write(foobar) + +with open("file.txt", "w") as f: + for line in text: + f.write(line) diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs index 066e1fb737d22..8fa64d7b1d653 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor::{self, Visitor}; -use ruff_python_ast::{self as ast, Expr}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_codegen::Generator; use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; use ruff_text_size::{Ranged, TextRange}; @@ -273,6 +273,7 @@ struct WriteMatcher<'a> { candidates: Vec>, matches: Vec>, contents: Vec>, + loop_counter: u32, } impl<'a> WriteMatcher<'a> { @@ -281,6 +282,7 @@ impl<'a> WriteMatcher<'a> { candidates, matches: vec![], contents: vec![], + loop_counter: 0, } } @@ -294,6 +296,16 @@ impl<'a> WriteMatcher<'a> { } impl<'a> Visitor<'a> for WriteMatcher<'a> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + if matches!(stmt, ast::Stmt::While(_) | ast::Stmt::For(_)) { + self.loop_counter += 1; + visitor::walk_stmt(self, stmt); + self.loop_counter -= 1; + } else { + visitor::walk_stmt(self, stmt); + } + } + fn visit_expr(&mut self, expr: &'a Expr) { if let Some((write_to, content)) = match_write_call(expr) { if let Some(open) = self @@ -301,8 +313,12 @@ impl<'a> Visitor<'a> for WriteMatcher<'a> { .iter() .position(|open| open.is_ref(write_to)) { - self.matches.push(self.candidates.remove(open)); - self.contents.push(content); + if self.loop_counter == 0 { + self.matches.push(self.candidates.remove(open)); + self.contents.push(content); + } else { + self.candidates.remove(open); + } } return; } From e1a531b4f69b18749e8de5f14c68e3cc0d8d8715 Mon Sep 17 00:00:00 2001 From: augustelalande Date: Wed, 10 Apr 2024 16:18:55 -0400 Subject: [PATCH 3/5] move common structs to helpers --- .../ruff_linter/src/rules/refurb/helpers.rs | 77 ++++++++++++- .../src/rules/refurb/rules/read_whole_file.rs | 93 ++++----------- .../rules/refurb/rules/write_whole_file.rs | 106 ++++-------------- 3 files changed, 119 insertions(+), 157 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index d429dd2d5d5b4..a784ca9f0e12d 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/crates/ruff_linter/src/rules/refurb/helpers.rs @@ -1,6 +1,7 @@ -use ruff_python_ast as ast; +use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::Generator; -use ruff_text_size::TextRange; +use ruff_python_semantic::ResolvedReference; +use ruff_text_size::{Ranged, TextRange}; /// Format a code snippet to call `name.method()`. pub(super) fn generate_method_call(name: &str, method: &str, generator: Generator) -> String { @@ -61,3 +62,75 @@ pub(super) fn generate_none_identity_comparison( }; generator.expr(&compare.into()) } + +#[derive(Debug)] +pub(crate) enum OpenMode { + /// "r" + ReadText, + /// "rb" + ReadBytes, + /// "w" + WriteText, + /// "wb" + WriteBytes, +} + +impl OpenMode { + pub(crate) fn pathlib_method(&self) -> String { + match self { + OpenMode::ReadText => "read_text".to_string(), + OpenMode::ReadBytes => "read_bytes".to_string(), + OpenMode::WriteText => "write_text".to_string(), + OpenMode::WriteBytes => "write_bytes".to_string(), + } + } +} + +/// A grab bag struct that joins together every piece of information we need to track +/// about a file open operation. +#[derive(Debug)] +pub(crate) struct FileOpen<'a> { + /// With item where the open happens, we use it for the reporting range. + pub(crate) item: &'a ast::WithItem, + /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. + pub(crate) filename: &'a Expr, + /// The file open mode. + pub(crate) mode: OpenMode, + /// The file open keywords. + pub(crate) keywords: Vec<&'a ast::Keyword>, + /// We only check `open` operations whose file handles are used exactly once. + pub(crate) reference: &'a ResolvedReference, +} + +impl<'a> FileOpen<'a> { + /// Determine whether an expression is a reference to the file handle, by comparing + /// their ranges. If two expressions have the same range, they must be the same expression. + pub(crate) fn is_ref(&self, expr: &Expr) -> bool { + expr.range() == self.reference.range() + } +} + +/// Match positional arguments. Return expression for the file name and open mode. +pub(crate) fn match_open_args(args: &[Expr]) -> Option<(&Expr, OpenMode)> { + match args { + [filename] => Some((filename, OpenMode::ReadText)), + [filename, mode_literal] => match_open_mode(mode_literal).map(|mode| (filename, mode)), + // The third positional argument is `buffering` and the pathlib methods don't support it. + _ => None, + } +} + +/// Match open mode to see if it is supported. +pub(crate) fn match_open_mode(mode: &Expr) -> Option { + let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; + if value.is_implicit_concatenated() { + return None; + } + match value.to_str() { + "r" => Some(OpenMode::ReadText), + "rb" => Some(OpenMode::ReadBytes), + "w" => Some(OpenMode::WriteText), + "wb" => Some(OpenMode::WriteBytes), + _ => None, + } +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index 518d701c79038..c6e1f31cde00f 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -8,6 +8,7 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; +use crate::rules::refurb::helpers; /// ## What it does /// Checks for uses of `open` and `read` that can be replaced by `pathlib` @@ -85,43 +86,11 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { checker.diagnostics.extend(diagnostics); } -#[derive(Debug)] -enum ReadMode { - /// "r" -> `read_text` - Text, - /// "rb" -> `read_bytes` - Bytes, -} - -/// A grab bag struct that joins together every piece of information we need to track -/// about a file open operation. -#[derive(Debug)] -struct FileOpen<'a> { - /// With item where the open happens, we use it for the reporting range. - item: &'a ast::WithItem, - /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. - filename: &'a Expr, - /// The type of read to choose `read_text` or `read_bytes`. - mode: ReadMode, - /// Keywords that can be used in the new read call. - keywords: Vec<&'a ast::Keyword>, - /// We only check `open` operations whose file handles are used exactly once. - reference: &'a ResolvedReference, -} - -impl<'a> FileOpen<'a> { - /// Determine whether an expression is a reference to the file handle, by comparing - /// their ranges. If two expressions have the same range, they must be the same expression. - fn is_ref(&self, expr: &Expr) -> bool { - expr.range() == self.reference.range() - } -} - /// Find and return all `open` operations in the given `with` statement. fn find_file_opens<'a>( with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, -) -> Vec> { +) -> Vec> { with.items .iter() .filter_map(|item| find_file_open(item, with, semantic)) @@ -133,7 +102,7 @@ fn find_file_open<'a>( item: &'a ast::WithItem, with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, -) -> Option> { +) -> Option> { // We want to match `open(...) as var`. let ast::ExprCall { func, @@ -157,7 +126,7 @@ fn find_file_open<'a>( } // Match positional arguments, get filename and read mode. - let (filename, pos_mode) = match_open_args(args)?; + let (filename, pos_mode) = helpers::match_open_args(args)?; // Match keyword arguments, get keyword arguments to forward and possibly read mode. let (keywords, kw_mode) = match_open_keywords(keywords)?; @@ -166,6 +135,13 @@ fn find_file_open<'a>( // keyword mode should override that. let mode = kw_mode.unwrap_or(pos_mode); + if !matches!( + mode, + helpers::OpenMode::ReadText | helpers::OpenMode::ReadBytes, + ) { + return None; + } + // Now we need to find what is this variable bound to... let scope = semantic.current_scope(); let bindings: Vec = scope.get_all(var.id.as_str()).collect(); @@ -192,7 +168,7 @@ fn find_file_open<'a>( return None; }; - Some(FileOpen { + Some(helpers::FileOpen { item, filename, mode, @@ -201,22 +177,12 @@ fn find_file_open<'a>( }) } -/// Match positional arguments. Return expression for the file name and read mode. -fn match_open_args(args: &[Expr]) -> Option<(&Expr, ReadMode)> { - match args { - [filename] => Some((filename, ReadMode::Text)), - [filename, mode_literal] => match_open_mode(mode_literal).map(|mode| (filename, mode)), - // The third positional argument is `buffering` and `read_text` doesn't support it. - _ => None, - } -} - /// Match keyword arguments. Return keyword arguments to forward and read mode. fn match_open_keywords( keywords: &[ast::Keyword], -) -> Option<(Vec<&ast::Keyword>, Option)> { +) -> Option<(Vec<&ast::Keyword>, Option)> { let mut result: Vec<&ast::Keyword> = vec![]; - let mut mode: Option = None; + let mut mode: Option = None; for keyword in keywords { match keyword.arg.as_ref()?.as_str() { @@ -233,7 +199,7 @@ fn match_open_keywords( // // So, here we return None from this whole function if the mode // is incompatible. - "mode" => mode = Some(match_open_mode(&keyword.value)?), + "mode" => mode = Some(helpers::match_open_mode(&keyword.value)?), // All other keywords cannot be directly forwarded. _ => return None, @@ -242,35 +208,22 @@ fn match_open_keywords( Some((result, mode)) } -/// Match open mode to see if it is supported. -fn match_open_mode(mode: &Expr) -> Option { - let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; - if value.is_implicit_concatenated() { - return None; - } - match value.to_str() { - "r" => Some(ReadMode::Text), - "rb" => Some(ReadMode::Bytes), - _ => None, - } -} - /// AST visitor that matches `open` operations with the corresponding `read` calls. #[derive(Debug)] struct ReadMatcher<'a> { - candidates: Vec>, - matches: Vec>, + candidates: Vec>, + matches: Vec>, } impl<'a> ReadMatcher<'a> { - fn new(candidates: Vec>) -> Self { + fn new(candidates: Vec>) -> Self { Self { candidates, matches: vec![], } } - fn into_matches(self) -> Vec> { + fn into_matches(self) -> Vec> { self.matches } } @@ -309,13 +262,9 @@ fn match_read_call(expr: &Expr) -> Option<&Expr> { } /// Construct the replacement suggestion call. -fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> SourceCodeSnippet { - let method_name = match open.mode { - ReadMode::Text => "read_text", - ReadMode::Bytes => "read_bytes", - }; +fn make_suggestion(open: &helpers::FileOpen<'_>, generator: Generator) -> SourceCodeSnippet { let name = ast::ExprName { - id: method_name.to_string(), + id: open.mode.pathlib_method(), ctx: ast::ExprContext::Load, range: TextRange::default(), }; diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs index 8fa64d7b1d653..bdb2852259c68 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -8,6 +8,7 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; +use crate::rules::refurb::helpers; /// ## What it does /// Checks for uses of `open` and `write` that can be replaced by `pathlib` @@ -85,43 +86,11 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { checker.diagnostics.extend(diagnostics); } -#[derive(Debug)] -enum WriteMode { - /// "w" -> `write_text` - Text, - /// "wb" -> `write_bytes` - Bytes, -} - -/// A grab bag struct that joins together every piece of information we need to track -/// about a file open operation. -#[derive(Debug)] -struct FileOpen<'a> { - /// With item where the open happens, we use it for the reporting range. - item: &'a ast::WithItem, - /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. - filename: &'a Expr, - /// The type of write to choose `write_text` or `write_bytes`. - mode: WriteMode, - /// Keywords that can be used in the new write call. - keywords: Vec<&'a ast::Keyword>, - /// We only check `open` operations whose file handles are used exactly once. - reference: &'a ResolvedReference, -} - -impl<'a> FileOpen<'a> { - /// Determine whether an expression is a reference to the file handle, by comparing - /// their ranges. If two expressions have the same range, they must be the same expression. - fn is_ref(&self, expr: &Expr) -> bool { - expr.range() == self.reference.range() - } -} - /// Find and return all `open` operations in the given `with` statement. fn find_file_opens<'a>( with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, -) -> Vec> { +) -> Vec> { with.items .iter() .filter_map(|item| find_file_open(item, with, semantic)) @@ -133,7 +102,7 @@ fn find_file_open<'a>( item: &'a ast::WithItem, with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, -) -> Option> { +) -> Option> { // We want to match `open(...) as var`. let ast::ExprCall { func, @@ -157,21 +126,22 @@ fn find_file_open<'a>( } // Match positional arguments, get filename and mode. - let (filename, pos_mode) = match_open_args(args)?; + let (filename, pos_mode) = helpers::match_open_args(args)?; // Match keyword arguments, get keyword arguments to forward and possibly mode. let (keywords, kw_mode) = match_open_keywords(keywords)?; - let mode = match kw_mode { - Some(kw) => kw, - None => match pos_mode { - Some(pos) => pos, - None => return None, // Indicates default mode "r", which is incompatible. - }, - }; + let mode = kw_mode.unwrap_or(pos_mode); + + if !matches!( + mode, + helpers::OpenMode::WriteText | helpers::OpenMode::WriteBytes, + ) { + return None; + } // Path.write_bytes does not support any kwargs. - if matches!(mode, WriteMode::Bytes) && !keywords.is_empty() { + if matches!(mode, helpers::OpenMode::WriteBytes) && !keywords.is_empty() { return None; } @@ -201,7 +171,7 @@ fn find_file_open<'a>( return None; }; - Some(FileOpen { + Some(helpers::FileOpen { item, filename, mode, @@ -210,25 +180,12 @@ fn find_file_open<'a>( }) } -/// Match positional arguments. Return expression for the file name and mode. -fn match_open_args(args: &[Expr]) -> Option<(&Expr, Option)> { - match args { - // If only one argument indicating read mode, the mode could still be set as a kwarg. - [filename] => Some((filename, None)), - [filename, mode_literal] => { - match_open_mode(mode_literal).map(|mode| (filename, Some(mode))) - } - // The third positional argument is `buffering` and `write_text` doesn't support it. - _ => None, - } -} - /// Match keyword arguments. Return keyword arguments to forward and mode. fn match_open_keywords( keywords: &[ast::Keyword], -) -> Option<(Vec<&ast::Keyword>, Option)> { +) -> Option<(Vec<&ast::Keyword>, Option)> { let mut result: Vec<&ast::Keyword> = vec![]; - let mut mode: Option = None; + let mut mode: Option = None; for keyword in keywords { match keyword.arg.as_ref()?.as_str() { @@ -245,7 +202,7 @@ fn match_open_keywords( // // So, here we return None from this whole function if the mode // is incompatible. - "mode" => mode = Some(match_open_mode(&keyword.value)?), + "mode" => mode = Some(helpers::match_open_mode(&keyword.value)?), // All other keywords cannot be directly forwarded. _ => return None, @@ -254,30 +211,17 @@ fn match_open_keywords( Some((result, mode)) } -/// Match open mode to see if it is supported. -fn match_open_mode(mode: &Expr) -> Option { - let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; - if value.is_implicit_concatenated() { - return None; - } - match value.to_str() { - "w" => Some(WriteMode::Text), - "wb" => Some(WriteMode::Bytes), - _ => None, - } -} - /// AST visitor that matches `open` operations with the corresponding `write` calls. #[derive(Debug)] struct WriteMatcher<'a> { - candidates: Vec>, - matches: Vec>, + candidates: Vec>, + matches: Vec>, contents: Vec>, loop_counter: u32, } impl<'a> WriteMatcher<'a> { - fn new(candidates: Vec>) -> Self { + fn new(candidates: Vec>) -> Self { Self { candidates, matches: vec![], @@ -286,7 +230,7 @@ impl<'a> WriteMatcher<'a> { } } - fn into_matches(self) -> Vec> { + fn into_matches(self) -> Vec> { self.matches } @@ -345,16 +289,12 @@ fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { /// Construct the replacement suggestion call. fn make_suggestion( - open: &FileOpen<'_>, + open: &helpers::FileOpen<'_>, write_arguments: Vec, generator: Generator, ) -> SourceCodeSnippet { - let method_name = match open.mode { - WriteMode::Text => "write_text", - WriteMode::Bytes => "write_bytes", - }; let name = ast::ExprName { - id: method_name.to_string(), + id: open.mode.pathlib_method(), ctx: ast::ExprContext::Load, range: TextRange::default(), }; From bc059af42ef38b07f5f7c0117335e324a4175ba8 Mon Sep 17 00:00:00 2001 From: augustelalande Date: Wed, 10 Apr 2024 17:01:35 -0400 Subject: [PATCH 4/5] move more common functions to helpers --- .../ruff_linter/src/rules/refurb/helpers.rs | 175 +++++++++++++++++- .../src/rules/refurb/rules/read_whole_file.rs | 153 +-------------- .../rules/refurb/rules/write_whole_file.rs | 156 +--------------- 3 files changed, 177 insertions(+), 307 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index a784ca9f0e12d..2b95da7c34fa1 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/crates/ruff_linter/src/rules/refurb/helpers.rs @@ -1,8 +1,10 @@ use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::Generator; -use ruff_python_semantic::ResolvedReference; +use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; use ruff_text_size::{Ranged, TextRange}; +use crate::fix::snippet::SourceCodeSnippet; + /// Format a code snippet to call `name.method()`. pub(super) fn generate_method_call(name: &str, method: &str, generator: Generator) -> String { // Construct `name`. @@ -63,6 +65,7 @@ pub(super) fn generate_none_identity_comparison( generator.expr(&compare.into()) } +// Helpers for read-whole-file and write-whole-file #[derive(Debug)] pub(crate) enum OpenMode { /// "r" @@ -76,7 +79,7 @@ pub(crate) enum OpenMode { } impl OpenMode { - pub(crate) fn pathlib_method(&self) -> String { + fn pathlib_method(&self) -> String { match self { OpenMode::ReadText => "read_text".to_string(), OpenMode::ReadBytes => "read_bytes".to_string(), @@ -110,8 +113,110 @@ impl<'a> FileOpen<'a> { } } +/// Find and return all `open` operations in the given `with` statement. +pub(crate) fn find_file_opens<'a>( + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, + read_mode: bool, +) -> Vec> { + with.items + .iter() + .filter_map(|item| find_file_open(item, with, semantic, read_mode)) + .collect() +} + +/// Find `open` operation in the given `with` item. +fn find_file_open<'a>( + item: &'a ast::WithItem, + with: &'a ast::StmtWith, + semantic: &'a SemanticModel<'a>, + read_mode: bool, +) -> Option> { + // We want to match `open(...) as var`. + let ast::ExprCall { + func, + arguments: ast::Arguments { args, keywords, .. }, + .. + } = item.context_expr.as_call_expr()?; + + if func.as_name_expr()?.id != "open" { + return None; + } + + let var = item.optional_vars.as_deref()?.as_name_expr()?; + + // Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`, + // it could be a match; but in all other cases, the call _could_ contain unsupported keyword + // arguments, like `buffering`. + if args.iter().any(Expr::is_starred_expr) + || keywords.iter().any(|keyword| keyword.arg.is_none()) + { + return None; + } + + // Match positional arguments, get filename and mode. + let (filename, pos_mode) = match_open_args(args)?; + + // Match keyword arguments, get keyword arguments to forward and possibly mode. + let (keywords, kw_mode) = match_open_keywords(keywords, read_mode)?; + + let mode = kw_mode.unwrap_or(pos_mode); + + match mode { + OpenMode::ReadText | OpenMode::ReadBytes => { + if !read_mode { + return None; + } + } + OpenMode::WriteText | OpenMode::WriteBytes => { + if read_mode { + return None; + } + } + } + + // Path.read_bytes and Path.write_bytes do not support any kwargs. + if matches!(mode, OpenMode::ReadBytes | OpenMode::WriteBytes) && !keywords.is_empty() { + return None; + } + + // Now we need to find what is this variable bound to... + let scope = semantic.current_scope(); + let bindings: Vec = scope.get_all(var.id.as_str()).collect(); + + let binding = bindings + .iter() + .map(|x| semantic.binding(*x)) + // We might have many bindings with the same name, but we only care + // for the one we are looking at right now. + .find(|binding| binding.range() == var.range())?; + + // Since many references can share the same binding, we can limit our attention span + // exclusively to the body of the current `with` statement. + let references: Vec<&ResolvedReference> = binding + .references + .iter() + .map(|id| semantic.reference(*id)) + .filter(|reference| with.range().contains_range(reference.range())) + .collect(); + + // And even with all these restrictions, if the file handle gets used not exactly once, + // it doesn't fit the bill. + let [reference] = references.as_slice() else { + return None; + }; + + Some(FileOpen { + item, + filename, + mode, + keywords, + reference, + }) +} + /// Match positional arguments. Return expression for the file name and open mode. -pub(crate) fn match_open_args(args: &[Expr]) -> Option<(&Expr, OpenMode)> { +fn match_open_args(args: &[Expr]) -> Option<(&Expr, OpenMode)> { match args { [filename] => Some((filename, OpenMode::ReadText)), [filename, mode_literal] => match_open_mode(mode_literal).map(|mode| (filename, mode)), @@ -120,8 +225,47 @@ pub(crate) fn match_open_args(args: &[Expr]) -> Option<(&Expr, OpenMode)> { } } +/// Match keyword arguments. Return keyword arguments to forward and mode. +fn match_open_keywords( + keywords: &[ast::Keyword], + read_mode: bool, +) -> Option<(Vec<&ast::Keyword>, Option)> { + let mut result: Vec<&ast::Keyword> = vec![]; + let mut mode: Option = None; + + for keyword in keywords { + match keyword.arg.as_ref()?.as_str() { + "encoding" | "errors" => result.push(keyword), + // newline is only valid for write_text + "newline" => { + if read_mode { + return None; + } + result.push(keyword); + } + + // This might look bizarre, - why do we re-wrap this optional? + // + // The answer is quite simple, in the result of the current function + // mode being `None` is a possible and correct option meaning that there + // was NO "mode" keyword argument. + // + // The result of `match_open_mode` on the other hand is None + // in the cases when the mode is not compatible with `write_text`/`write_bytes`. + // + // So, here we return None from this whole function if the mode + // is incompatible. + "mode" => mode = Some(match_open_mode(&keyword.value)?), + + // All other keywords cannot be directly forwarded. + _ => return None, + }; + } + Some((result, mode)) +} + /// Match open mode to see if it is supported. -pub(crate) fn match_open_mode(mode: &Expr) -> Option { +fn match_open_mode(mode: &Expr) -> Option { let ast::ExprStringLiteral { value, .. } = mode.as_string_literal_expr()?; if value.is_implicit_concatenated() { return None; @@ -134,3 +278,26 @@ pub(crate) fn match_open_mode(mode: &Expr) -> Option { _ => None, } } + +/// Construct the replacement suggestion call. +pub(crate) fn make_suggestion( + open: &FileOpen<'_>, + arguments: Vec, + generator: Generator, +) -> SourceCodeSnippet { + let name = ast::ExprName { + id: open.mode.pathlib_method(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + let call = ast::ExprCall { + func: Box::new(name.into()), + arguments: ast::Arguments { + args: Box::from(arguments), + keywords: open.keywords.iter().copied().cloned().collect(), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + SourceCodeSnippet::from_str(&generator.expr(&call.into())) +} diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index dd653ffcf154d..c1e51c34d56f3 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -2,9 +2,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor::{self, Visitor}; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_codegen::Generator; -use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; @@ -58,7 +56,7 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { } // First we go through all the items in the statement and find all `open` operations. - let candidates = find_file_opens(with, checker.semantic()); + let candidates = helpers::find_file_opens(with, checker.semantic(), true); if candidates.is_empty() { return; } @@ -77,7 +75,7 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { Diagnostic::new( ReadWholeFile { filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), - suggestion: make_suggestion(open, checker.generator()), + suggestion: helpers::make_suggestion(open, vec![], checker.generator()), }, open.item.range(), ) @@ -86,132 +84,6 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { checker.diagnostics.extend(diagnostics); } -/// Find and return all `open` operations in the given `with` statement. -fn find_file_opens<'a>( - with: &'a ast::StmtWith, - semantic: &'a SemanticModel<'a>, -) -> Vec> { - with.items - .iter() - .filter_map(|item| find_file_open(item, with, semantic)) - .collect() -} - -/// Find `open` operation in the given `with` item. -fn find_file_open<'a>( - item: &'a ast::WithItem, - with: &'a ast::StmtWith, - semantic: &'a SemanticModel<'a>, -) -> Option> { - // We want to match `open(...) as var`. - let ast::ExprCall { - func, - arguments: ast::Arguments { args, keywords, .. }, - .. - } = item.context_expr.as_call_expr()?; - - if func.as_name_expr()?.id != "open" { - return None; - } - - let var = item.optional_vars.as_deref()?.as_name_expr()?; - - // Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="r")`, - // it could be a match; but in all other cases, the call _could_ contain unsupported keyword - // arguments, like `buffering`. - if args.iter().any(Expr::is_starred_expr) - || keywords.iter().any(|keyword| keyword.arg.is_none()) - { - return None; - } - - // Match positional arguments, get filename and read mode. - let (filename, pos_mode) = helpers::match_open_args(args)?; - - // Match keyword arguments, get keyword arguments to forward and possibly read mode. - let (keywords, kw_mode) = match_open_keywords(keywords)?; - - // `pos_mode` could've been assigned default value corresponding to "r", while - // keyword mode should override that. - let mode = kw_mode.unwrap_or(pos_mode); - - if !matches!( - mode, - helpers::OpenMode::ReadText | helpers::OpenMode::ReadBytes, - ) { - return None; - } - - if matches!(mode, helpers::OpenMode::ReadBytes) && !keywords.is_empty() { - return None; - } - - // Now we need to find what is this variable bound to... - let scope = semantic.current_scope(); - let bindings: Vec = scope.get_all(var.id.as_str()).collect(); - - let binding = bindings - .iter() - .map(|x| semantic.binding(*x)) - // We might have many bindings with the same name, but we only care - // for the one we are looking at right now. - .find(|binding| binding.range() == var.range())?; - - // Since many references can share the same binding, we can limit our attention span - // exclusively to the body of the current `with` statement. - let references: Vec<&ResolvedReference> = binding - .references - .iter() - .map(|id| semantic.reference(*id)) - .filter(|reference| with.range().contains_range(reference.range())) - .collect(); - - // And even with all these restrictions, if the file handle gets used not exactly once, - // it doesn't fit the bill. - let [reference] = references.as_slice() else { - return None; - }; - - Some(helpers::FileOpen { - item, - filename, - mode, - keywords, - reference, - }) -} - -/// Match keyword arguments. Return keyword arguments to forward and read mode. -fn match_open_keywords( - keywords: &[ast::Keyword], -) -> Option<(Vec<&ast::Keyword>, Option)> { - let mut result: Vec<&ast::Keyword> = vec![]; - let mut mode: Option = None; - - for keyword in keywords { - match keyword.arg.as_ref()?.as_str() { - "encoding" | "errors" => result.push(keyword), - - // This might look bizarre, - why do we re-wrap this optional? - // - // The answer is quite simple, in the result of the current function - // mode being `None` is a possible and correct option meaning that there - // was NO "mode" keyword argument. - // - // The result of `match_open_mode` on the other hand is None - // in the cases when the mode is not compatible with `read_text`/`read_bytes`. - // - // So, here we return None from this whole function if the mode - // is incompatible. - "mode" => mode = Some(helpers::match_open_mode(&keyword.value)?), - - // All other keywords cannot be directly forwarded. - _ => return None, - }; - } - Some((result, mode)) -} - /// AST visitor that matches `open` operations with the corresponding `read` calls. #[derive(Debug)] struct ReadMatcher<'a> { @@ -264,22 +136,3 @@ fn match_read_call(expr: &Expr) -> Option<&Expr> { Some(attr.value.as_ref()) } - -/// Construct the replacement suggestion call. -fn make_suggestion(open: &helpers::FileOpen<'_>, generator: Generator) -> SourceCodeSnippet { - let name = ast::ExprName { - id: open.mode.pathlib_method(), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - let call = ast::ExprCall { - func: Box::new(name.into()), - arguments: ast::Arguments { - args: Box::from([]), - keywords: open.keywords.iter().copied().cloned().collect(), - range: TextRange::default(), - }, - range: TextRange::default(), - }; - SourceCodeSnippet::from_str(&generator.expr(&call.into())) -} diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs index bdb2852259c68..9830f33641f8a 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -2,9 +2,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor::{self, Visitor}; use ruff_python_ast::{self as ast, Expr, Stmt}; -use ruff_python_codegen::Generator; -use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; @@ -58,7 +56,7 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { } // First we go through all the items in the statement and find all `open` operations. - let candidates = find_file_opens(with, checker.semantic()); + let candidates = helpers::find_file_opens(with, checker.semantic(), false); if candidates.is_empty() { return; } @@ -77,7 +75,7 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { Diagnostic::new( WriteWholeFile { filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), - suggestion: make_suggestion(open, content, checker.generator()), + suggestion: helpers::make_suggestion(open, content, checker.generator()), }, open.item.range(), ) @@ -86,131 +84,6 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { checker.diagnostics.extend(diagnostics); } -/// Find and return all `open` operations in the given `with` statement. -fn find_file_opens<'a>( - with: &'a ast::StmtWith, - semantic: &'a SemanticModel<'a>, -) -> Vec> { - with.items - .iter() - .filter_map(|item| find_file_open(item, with, semantic)) - .collect() -} - -/// Find `open` operation in the given `with` item. -fn find_file_open<'a>( - item: &'a ast::WithItem, - with: &'a ast::StmtWith, - semantic: &'a SemanticModel<'a>, -) -> Option> { - // We want to match `open(...) as var`. - let ast::ExprCall { - func, - arguments: ast::Arguments { args, keywords, .. }, - .. - } = item.context_expr.as_call_expr()?; - - if func.as_name_expr()?.id != "open" { - return None; - } - - let var = item.optional_vars.as_deref()?.as_name_expr()?; - - // Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`, - // it could be a match; but in all other cases, the call _could_ contain unsupported keyword - // arguments, like `buffering`. - if args.iter().any(Expr::is_starred_expr) - || keywords.iter().any(|keyword| keyword.arg.is_none()) - { - return None; - } - - // Match positional arguments, get filename and mode. - let (filename, pos_mode) = helpers::match_open_args(args)?; - - // Match keyword arguments, get keyword arguments to forward and possibly mode. - let (keywords, kw_mode) = match_open_keywords(keywords)?; - - let mode = kw_mode.unwrap_or(pos_mode); - - if !matches!( - mode, - helpers::OpenMode::WriteText | helpers::OpenMode::WriteBytes, - ) { - return None; - } - - // Path.write_bytes does not support any kwargs. - if matches!(mode, helpers::OpenMode::WriteBytes) && !keywords.is_empty() { - return None; - } - - // Now we need to find what is this variable bound to... - let scope = semantic.current_scope(); - let bindings: Vec = scope.get_all(var.id.as_str()).collect(); - - let binding = bindings - .iter() - .map(|x| semantic.binding(*x)) - // We might have many bindings with the same name, but we only care - // for the one we are looking at right now. - .find(|binding| binding.range() == var.range())?; - - // Since many references can share the same binding, we can limit our attention span - // exclusively to the body of the current `with` statement. - let references: Vec<&ResolvedReference> = binding - .references - .iter() - .map(|id| semantic.reference(*id)) - .filter(|reference| with.range().contains_range(reference.range())) - .collect(); - - // And even with all these restrictions, if the file handle gets used not exactly once, - // it doesn't fit the bill. - let [reference] = references.as_slice() else { - return None; - }; - - Some(helpers::FileOpen { - item, - filename, - mode, - keywords, - reference, - }) -} - -/// Match keyword arguments. Return keyword arguments to forward and mode. -fn match_open_keywords( - keywords: &[ast::Keyword], -) -> Option<(Vec<&ast::Keyword>, Option)> { - let mut result: Vec<&ast::Keyword> = vec![]; - let mut mode: Option = None; - - for keyword in keywords { - match keyword.arg.as_ref()?.as_str() { - "encoding" | "errors" | "newline" => result.push(keyword), - - // This might look bizarre, - why do we re-wrap this optional? - // - // The answer is quite simple, in the result of the current function - // mode being `None` is a possible and correct option meaning that there - // was NO "mode" keyword argument. - // - // The result of `match_open_mode` on the other hand is None - // in the cases when the mode is not compatible with `write_text`/`write_bytes`. - // - // So, here we return None from this whole function if the mode - // is incompatible. - "mode" => mode = Some(helpers::match_open_mode(&keyword.value)?), - - // All other keywords cannot be directly forwarded. - _ => return None, - }; - } - Some((result, mode)) -} - /// AST visitor that matches `open` operations with the corresponding `write` calls. #[derive(Debug)] struct WriteMatcher<'a> { @@ -286,26 +159,3 @@ fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { Some((attr.value.as_ref(), call.arguments.args.as_ref().to_vec())) } - -/// Construct the replacement suggestion call. -fn make_suggestion( - open: &helpers::FileOpen<'_>, - write_arguments: Vec, - generator: Generator, -) -> SourceCodeSnippet { - let name = ast::ExprName { - id: open.mode.pathlib_method(), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - let call = ast::ExprCall { - func: Box::new(name.into()), - arguments: ast::Arguments { - args: Box::from(write_arguments), - keywords: open.keywords.iter().copied().cloned().collect(), - range: TextRange::default(), - }, - range: TextRange::default(), - }; - SourceCodeSnippet::from_str(&generator.expr(&call.into())) -} From 07c72c50f9f2bfb44b1c705f84db0bc59a9bd377 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 11 Apr 2024 13:56:07 +0530 Subject: [PATCH 5/5] Code review changes --- .../ruff_linter/src/rules/refurb/helpers.rs | 47 ++++---------- .../src/rules/refurb/rules/read_whole_file.rs | 38 ++++++++--- .../rules/refurb/rules/write_whole_file.rs | 63 ++++++++++++------- ...es__refurb__tests__FURB103_FURB103.py.snap | 6 +- 4 files changed, 85 insertions(+), 69 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index 2b95da7c34fa1..83ef1e5706cc6 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/crates/ruff_linter/src/rules/refurb/helpers.rs @@ -3,8 +3,6 @@ use ruff_python_codegen::Generator; use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel}; use ruff_text_size::{Ranged, TextRange}; -use crate::fix::snippet::SourceCodeSnippet; - /// Format a code snippet to call `name.method()`. pub(super) fn generate_method_call(name: &str, method: &str, generator: Generator) -> String { // Construct `name`. @@ -66,8 +64,8 @@ pub(super) fn generate_none_identity_comparison( } // Helpers for read-whole-file and write-whole-file -#[derive(Debug)] -pub(crate) enum OpenMode { +#[derive(Debug, Copy, Clone)] +pub(super) enum OpenMode { /// "r" ReadText, /// "rb" @@ -79,7 +77,7 @@ pub(crate) enum OpenMode { } impl OpenMode { - fn pathlib_method(&self) -> String { + pub(super) fn pathlib_method(self) -> String { match self { OpenMode::ReadText => "read_text".to_string(), OpenMode::ReadBytes => "read_bytes".to_string(), @@ -92,29 +90,29 @@ impl OpenMode { /// A grab bag struct that joins together every piece of information we need to track /// about a file open operation. #[derive(Debug)] -pub(crate) struct FileOpen<'a> { +pub(super) struct FileOpen<'a> { /// With item where the open happens, we use it for the reporting range. - pub(crate) item: &'a ast::WithItem, + pub(super) item: &'a ast::WithItem, /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. - pub(crate) filename: &'a Expr, + pub(super) filename: &'a Expr, /// The file open mode. - pub(crate) mode: OpenMode, + pub(super) mode: OpenMode, /// The file open keywords. - pub(crate) keywords: Vec<&'a ast::Keyword>, + pub(super) keywords: Vec<&'a ast::Keyword>, /// We only check `open` operations whose file handles are used exactly once. - pub(crate) reference: &'a ResolvedReference, + pub(super) reference: &'a ResolvedReference, } impl<'a> FileOpen<'a> { /// Determine whether an expression is a reference to the file handle, by comparing /// their ranges. If two expressions have the same range, they must be the same expression. - pub(crate) fn is_ref(&self, expr: &Expr) -> bool { + pub(super) fn is_ref(&self, expr: &Expr) -> bool { expr.range() == self.reference.range() } } /// Find and return all `open` operations in the given `with` statement. -pub(crate) fn find_file_opens<'a>( +pub(super) fn find_file_opens<'a>( with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, read_mode: bool, @@ -278,26 +276,3 @@ fn match_open_mode(mode: &Expr) -> Option { _ => None, } } - -/// Construct the replacement suggestion call. -pub(crate) fn make_suggestion( - open: &FileOpen<'_>, - arguments: Vec, - generator: Generator, -) -> SourceCodeSnippet { - let name = ast::ExprName { - id: open.mode.pathlib_method(), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - let call = ast::ExprCall { - func: Box::new(name.into()), - arguments: ast::Arguments { - args: Box::from(arguments), - keywords: open.keywords.iter().copied().cloned().collect(), - range: TextRange::default(), - }, - range: TextRange::default(), - }; - SourceCodeSnippet::from_str(&generator.expr(&call.into())) -} diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index c1e51c34d56f3..50fdd85aa8556 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -2,11 +2,13 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::visitor::{self, Visitor}; use ruff_python_ast::{self as ast, Expr}; -use ruff_text_size::Ranged; +use ruff_python_codegen::Generator; +use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; -use crate::rules::refurb::helpers; + +use super::super::helpers::{find_file_opens, FileOpen}; /// ## What it does /// Checks for uses of `open` and `read` that can be replaced by `pathlib` @@ -56,7 +58,7 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { } // First we go through all the items in the statement and find all `open` operations. - let candidates = helpers::find_file_opens(with, checker.semantic(), true); + let candidates = find_file_opens(with, checker.semantic(), true); if candidates.is_empty() { return; } @@ -75,7 +77,7 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { Diagnostic::new( ReadWholeFile { filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), - suggestion: helpers::make_suggestion(open, vec![], checker.generator()), + suggestion: make_suggestion(open, checker.generator()), }, open.item.range(), ) @@ -87,19 +89,19 @@ pub(crate) fn read_whole_file(checker: &mut Checker, with: &ast::StmtWith) { /// AST visitor that matches `open` operations with the corresponding `read` calls. #[derive(Debug)] struct ReadMatcher<'a> { - candidates: Vec>, - matches: Vec>, + candidates: Vec>, + matches: Vec>, } impl<'a> ReadMatcher<'a> { - fn new(candidates: Vec>) -> Self { + fn new(candidates: Vec>) -> Self { Self { candidates, matches: vec![], } } - fn into_matches(self) -> Vec> { + fn into_matches(self) -> Vec> { self.matches } } @@ -134,5 +136,23 @@ fn match_read_call(expr: &Expr) -> Option<&Expr> { return None; } - Some(attr.value.as_ref()) + Some(&*attr.value) +} + +fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> SourceCodeSnippet { + let name = ast::ExprName { + id: open.mode.pathlib_method(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + let call = ast::ExprCall { + func: Box::new(name.into()), + arguments: ast::Arguments { + args: Box::from([]), + keywords: open.keywords.iter().copied().cloned().collect(), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + SourceCodeSnippet::from_str(&generator.expr(&call.into())) } diff --git a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs index 9830f33641f8a..84265317f83d1 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -1,12 +1,15 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::relocate::relocate_expr; use ruff_python_ast::visitor::{self, Visitor}; use ruff_python_ast::{self as ast, Expr, Stmt}; -use ruff_text_size::Ranged; +use ruff_python_codegen::Generator; +use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; -use crate::rules::refurb::helpers; + +use super::super::helpers::{find_file_opens, FileOpen}; /// ## What it does /// Checks for uses of `open` and `write` that can be replaced by `pathlib` @@ -56,26 +59,27 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { } // First we go through all the items in the statement and find all `open` operations. - let candidates = helpers::find_file_opens(with, checker.semantic(), false); + let candidates = find_file_opens(with, checker.semantic(), false); if candidates.is_empty() { return; } // Then we need to match each `open` operation with exactly one `write` call. - let mut matcher = WriteMatcher::new(candidates); - visitor::walk_body(&mut matcher, &with.body); - let contents = matcher.contents(); + let (matches, contents) = { + let mut matcher = WriteMatcher::new(candidates); + visitor::walk_body(&mut matcher, &with.body); + matcher.finish() + }; // All the matched operations should be reported. - let diagnostics: Vec = matcher - .into_matches() + let diagnostics: Vec = matches .iter() .zip(contents) .map(|(open, content)| { Diagnostic::new( WriteWholeFile { filename: SourceCodeSnippet::from_str(&checker.generator().expr(open.filename)), - suggestion: helpers::make_suggestion(open, content, checker.generator()), + suggestion: make_suggestion(open, content, checker.generator()), }, open.item.range(), ) @@ -87,14 +91,14 @@ pub(crate) fn write_whole_file(checker: &mut Checker, with: &ast::StmtWith) { /// AST visitor that matches `open` operations with the corresponding `write` calls. #[derive(Debug)] struct WriteMatcher<'a> { - candidates: Vec>, - matches: Vec>, - contents: Vec>, + candidates: Vec>, + matches: Vec>, + contents: Vec<&'a Expr>, loop_counter: u32, } impl<'a> WriteMatcher<'a> { - fn new(candidates: Vec>) -> Self { + fn new(candidates: Vec>) -> Self { Self { candidates, matches: vec![], @@ -103,12 +107,8 @@ impl<'a> WriteMatcher<'a> { } } - fn into_matches(self) -> Vec> { - self.matches - } - - fn contents(&self) -> Vec> { - self.contents.clone() + fn finish(self) -> (Vec>, Vec<&'a Expr>) { + (self.matches, self.contents) } } @@ -144,7 +144,7 @@ impl<'a> Visitor<'a> for WriteMatcher<'a> { } /// Match `x.write(foo)` expression and return expression `x` and `foo` on success. -fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { +fn match_write_call(expr: &Expr) -> Option<(&Expr, &Expr)> { let call = expr.as_call_expr()?; let attr = call.func.as_attribute_expr()?; let method_name = &attr.attr; @@ -157,5 +157,26 @@ fn match_write_call(expr: &Expr) -> Option<(&Expr, Vec)> { return None; } - Some((attr.value.as_ref(), call.arguments.args.as_ref().to_vec())) + // `write` only takes in a single positional argument. + Some((&*attr.value, call.arguments.args.first()?)) +} + +fn make_suggestion(open: &FileOpen<'_>, arg: &Expr, generator: Generator) -> SourceCodeSnippet { + let name = ast::ExprName { + id: open.mode.pathlib_method(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + let mut arg = arg.clone(); + relocate_expr(&mut arg, TextRange::default()); + let call = ast::ExprCall { + func: Box::new(name.into()), + arguments: ast::Arguments { + args: Box::new([arg]), + keywords: open.keywords.iter().copied().cloned().collect(), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + SourceCodeSnippet::from_str(&generator.expr(&call.into())) } diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap index 9a37a5447d495..e919362cda06b 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103.py.snap @@ -25,7 +25,7 @@ FURB103.py:20:6: FURB103 `open` and `write` should be replaced by `Path("file.tx 21 | f.write(b"abc") | -FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(encoding="utf8", foobar)` +FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(foobar, encoding="utf8")` | 23 | # FURB103 24 | with open("file.txt", "w", encoding="utf8") as f: @@ -33,7 +33,7 @@ FURB103.py:24:6: FURB103 `open` and `write` should be replaced by `Path("file.tx 25 | f.write(foobar) | -FURB103.py:28:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(errors="ignore", foobar)` +FURB103.py:28:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(foobar, errors="ignore")` | 27 | # FURB103 28 | with open("file.txt", "w", errors="ignore") as f: @@ -85,7 +85,7 @@ FURB103.py:49:18: FURB103 `open` and `write` should be replaced by `Path("file.t 51 | # writes a single time to file and that bit they can replace. | -FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(newline="\r\n", foobar)` +FURB103.py:58:6: FURB103 `open` and `write` should be replaced by `Path("file.txt").write_text(foobar, newline="\r\n")` | 57 | # FURB103 58 | with open("file.txt", "w", newline="\r\n") as f: