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..2ba42f4ad823c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py @@ -0,0 +1,132 @@ +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) + +# 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/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 43bbf14b4bbb1..141a0ec9e96ef 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1225,6 +1225,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 efe2ed63c6057..988f0118b1329 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1037,6 +1037,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), (Refurb, "110") => (RuleGroup::Preview, rules::refurb::rules::IfExpInsteadOfOrOperator), #[allow(deprecated)] diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index d429dd2d5d5b4..83ef1e5706cc6 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::{BindingId, ResolvedReference, SemanticModel}; +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,217 @@ pub(super) fn generate_none_identity_comparison( }; generator.expr(&compare.into()) } + +// Helpers for read-whole-file and write-whole-file +#[derive(Debug, Copy, Clone)] +pub(super) enum OpenMode { + /// "r" + ReadText, + /// "rb" + ReadBytes, + /// "w" + WriteText, + /// "wb" + WriteBytes, +} + +impl OpenMode { + pub(super) 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(super) struct FileOpen<'a> { + /// With item where the open happens, we use it for the reporting range. + pub(super) item: &'a ast::WithItem, + /// Filename expression used as the first argument in `open`, we use it in the diagnostic message. + pub(super) filename: &'a Expr, + /// The file open mode. + pub(super) mode: OpenMode, + /// The file open keywords. + pub(super) keywords: Vec<&'a ast::Keyword>, + /// We only check `open` operations whose file handles are used exactly once. + 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(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(super) 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. +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 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. +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/mod.rs b/crates/ruff_linter/src/rules/refurb/mod.rs index dbf556c629b5d..812e2f30309dd 100644 --- a/crates/ruff_linter/src/rules/refurb/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/mod.rs @@ -41,6 +41,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 bff4b082c2d88..6f86341e7a8ec 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/mod.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/mod.rs @@ -25,6 +25,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; @@ -53,3 +54,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/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index 009bdf1c54d4f..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 @@ -3,12 +3,13 @@ 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; +use super::super::helpers::{find_file_opens, FileOpen}; + /// ## What it does /// Checks for uses of `open` and `read` that can be replaced by `pathlib` /// methods, like `Path.read_text` and `Path.read_bytes`. @@ -57,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 = find_file_opens(with, checker.semantic()); + let candidates = find_file_opens(with, checker.semantic(), true); if candidates.is_empty() { return; } @@ -85,180 +86,6 @@ 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> { - 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) = 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, ReadMode::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 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)> { - 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(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() { - "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> { @@ -309,17 +136,12 @@ fn match_read_call(expr: &Expr) -> Option<&Expr> { return None; } - Some(attr.value.as_ref()) + Some(&*attr.value) } -/// 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", - }; 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 new file mode 100644 index 0000000000000..84265317f83d1 --- /dev/null +++ b/crates/ruff_linter/src/rules/refurb/rules/write_whole_file.rs @@ -0,0 +1,182 @@ +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_python_codegen::Generator; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; +use crate::fix::snippet::SourceCodeSnippet; + +use super::super::helpers::{find_file_opens, FileOpen}; + +/// ## 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(), false); + if candidates.is_empty() { + return; + } + + // Then we need to match each `open` operation with exactly one `write` call. + 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 = 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); +} + +/// AST visitor that matches `open` operations with the corresponding `write` calls. +#[derive(Debug)] +struct WriteMatcher<'a> { + candidates: Vec>, + matches: Vec>, + contents: Vec<&'a Expr>, + loop_counter: u32, +} + +impl<'a> WriteMatcher<'a> { + fn new(candidates: Vec>) -> Self { + Self { + candidates, + matches: vec![], + contents: vec![], + loop_counter: 0, + } + } + + fn finish(self) -> (Vec>, Vec<&'a Expr>) { + (self.matches, self.contents) + } +} + +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 + .candidates + .iter() + .position(|open| open.is_ref(write_to)) + { + if self.loop_counter == 0 { + self.matches.push(self.candidates.remove(open)); + self.contents.push(content); + } else { + self.candidates.remove(open); + } + } + 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, &Expr)> { + 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; + } + + // `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 new file mode 100644 index 0000000000000..e919362cda06b --- /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(foobar, encoding="utf8")` + | +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(foobar, errors="ignore")` + | +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(foobar, newline="\r\n")` + | +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 c057b616c8e72..a7c2d7cea8986 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3041,6 +3041,7 @@ "FURB1", "FURB10", "FURB101", + "FURB103", "FURB105", "FURB11", "FURB110", diff --git a/scripts/add_rule.py b/scripts/add_rule.py index 42928fb7aa60f..d324e8001b4e5 100755 --- a/scripts/add_rule.py +++ b/scripts/add_rule.py @@ -27,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) @@ -141,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)