diff --git a/crates/ruff_linter/src/rules/refurb/helpers.rs b/crates/ruff_linter/src/rules/refurb/helpers.rs index 7d4bc7aaf0..b9e9ece8d2 100644 --- a/crates/ruff_linter/src/rules/refurb/helpers.rs +++ b/crates/ruff_linter/src/rules/refurb/helpers.rs @@ -120,16 +120,13 @@ impl OpenMode { 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: Option<&'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, - /// Pathlib Path object used to open the file, if any. - pub(super) path_obj: Option<&'a Expr>, + pub(super) argument: OpenArgument<'a>, } impl FileOpen<'_> { @@ -140,6 +137,45 @@ impl FileOpen<'_> { } } +#[derive(Debug)] +pub(super) enum OpenArgument<'a> { + /// The filename argument to `open`, e.g. "foo.txt" in: + /// + /// ```py + /// f = open("foo.txt") + /// ``` + Builtin { filename: &'a Expr }, + /// The `Path` receiver of a `pathlib.Path.open` call, e.g. the `p` in the + /// context manager in: + /// + /// ```py + /// p = Path("foo.txt") + /// with p.open() as f: ... + /// ``` + /// + /// or `Path("foo.txt")` in + /// + /// ```py + /// with Path("foo.txt").open() as f: ... + /// ``` + Pathlib { path: &'a Expr }, +} + +impl OpenArgument<'_> { + pub(super) fn display<'src>(&self, source: &'src str) -> &'src str { + &source[self.range()] + } +} + +impl Ranged for OpenArgument<'_> { + fn range(&self) -> TextRange { + match self { + OpenArgument::Builtin { filename } => filename.range(), + OpenArgument::Pathlib { path } => path.range(), + } + } +} + /// Find and return all `open` operations in the given `with` statement. pub(super) fn find_file_opens<'a>( with: &'a ast::StmtWith, @@ -156,16 +192,14 @@ pub(super) fn find_file_opens<'a>( .collect() } -#[expect(clippy::too_many_arguments)] fn resolve_file_open<'a>( item: &'a ast::WithItem, with: &'a ast::StmtWith, semantic: &'a SemanticModel<'a>, read_mode: bool, - filename: Option<&'a Expr>, mode: OpenMode, keywords: Vec<&'a ast::Keyword>, - path_obj: Option<&'a Expr>, + argument: OpenArgument<'a>, ) -> Option> { match mode { OpenMode::ReadText | OpenMode::ReadBytes => { @@ -203,11 +237,10 @@ fn resolve_file_open<'a>( Some(FileOpen { item, - filename, mode, keywords, reference, - path_obj, + argument, }) } @@ -251,10 +284,9 @@ fn find_file_open<'a>( with, semantic, read_mode, - Some(filename), mode, keywords, - None, + OpenArgument::Builtin { filename }, ) } @@ -279,15 +311,6 @@ fn find_path_open<'a>( return None; } let attr = func.as_attribute_expr()?; - let filename = if let Expr::Call(path_call) = attr.value.as_ref() { - if path_call.arguments.args.len() == 1 { - path_call.arguments.args.first() - } else { - None - } - } else { - None - }; let mode = if args.is_empty() { OpenMode::ReadText } else { @@ -301,10 +324,11 @@ fn find_path_open<'a>( with, semantic, read_mode, - filename, mode, keywords, - Some(attr.value.as_ref()), + OpenArgument::Pathlib { + path: attr.value.as_ref(), + }, ) } 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 1c2e39c0d8..eb321cc4d0 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 @@ -10,7 +10,7 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; use crate::importer::ImportRequest; -use crate::rules::refurb::helpers::{FileOpen, find_file_opens}; +use crate::rules::refurb::helpers::{FileOpen, OpenArgument, find_file_opens}; use crate::{FixAvailability, Violation}; /// ## What it does @@ -114,17 +114,11 @@ impl<'a> Visitor<'a> for ReadMatcher<'a, '_> { .position(|open| open.is_ref(read_from)) { let open = self.candidates.remove(open); - let filename_display = if let Some(filename) = open.filename { - self.checker.generator().expr(filename) - } else if let Some(path_obj) = open.path_obj { - self.checker.locator().slice(path_obj.range()).to_string() - } else { - return; - }; + let filename_display = open.argument.display(self.checker.source()); let suggestion = make_suggestion(&open, self.checker.generator()); let mut diagnostic = self.checker.report_diagnostic( ReadWholeFile { - filename: SourceCodeSnippet::from_str(&filename_display), + filename: SourceCodeSnippet::from_str(filename_display), suggestion: SourceCodeSnippet::from_str(&suggestion), }, open.item.range(), @@ -190,7 +184,9 @@ fn generate_fix( if with_stmt.items.len() != 1 { return None; } - let filename = open.filename?; + let OpenArgument::Builtin { filename } = open.argument else { + return None; + }; let locator = checker.locator(); let filename_code = locator.slice(filename.range()); 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 cd0e583f0b..9b77f6fc36 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 @@ -9,7 +9,7 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::fix::snippet::SourceCodeSnippet; use crate::importer::ImportRequest; -use crate::rules::refurb::helpers::{FileOpen, find_file_opens}; +use crate::rules::refurb::helpers::{FileOpen, OpenArgument, find_file_opens}; use crate::{FixAvailability, Locator, Violation}; /// ## What it does @@ -46,7 +46,6 @@ pub(crate) struct WriteWholeFile { filename: SourceCodeSnippet, suggestion: SourceCodeSnippet, is_path_open: bool, - has_filename: bool, } impl Violation for WriteWholeFile { @@ -57,29 +56,23 @@ impl Violation for WriteWholeFile { let filename = self.filename.truncated_display(); let suggestion = self.suggestion.truncated_display(); if self.is_path_open { - if self.has_filename { - format!( - "`Path().open()` followed by `write()` can be replaced by `Path({filename}).{suggestion}`" - ) - } else { - format!( - "`Path.open()` followed by `write()` can be replaced by `{filename}.{suggestion}`" - ) - } + format!( + "`Path.open()` followed by `write()` can be replaced by `{filename}.{suggestion}`" + ) } else { format!("`open` and `write` should be replaced by `Path({filename}).{suggestion}`") } } fn fix_title(&self) -> Option { - let formatted = if self.has_filename { + let formatted = if self.is_path_open { format!( - "Replace with `Path({}).{}`", + "Replace with `{}.{}`", self.filename.truncated_display(), self.suggestion.truncated_display(), ) } else { format!( - "Replace with `{}.{}`", + "Replace with `Path({}).{}`", self.filename.truncated_display(), self.suggestion.truncated_display(), ) @@ -149,27 +142,15 @@ impl<'a> Visitor<'a> for WriteMatcher<'a, '_> { .position(|open| open.is_ref(write_to)) { let open = self.candidates.remove(open); - let has_filename: bool; - let filename_display: String; if self.loop_counter == 0 { - if let Some(filename) = open.filename { - filename_display = self.checker.generator().expr(filename); - has_filename = true; - } else if let Some(path_obj) = open.path_obj { - filename_display = - self.checker.locator().slice(path_obj.range()).to_string(); - has_filename = false; - } else { - return; - } + let filename_display = open.argument.display(self.checker.source()); let suggestion = make_suggestion(&open, content, self.checker.locator()); let mut diagnostic = self.checker.report_diagnostic( WriteWholeFile { - filename: SourceCodeSnippet::from_str(&filename_display), + filename: SourceCodeSnippet::from_str(filename_display), suggestion: SourceCodeSnippet::from_str(&suggestion), - is_path_open: open.path_obj.is_some(), - has_filename, + is_path_open: matches!(open.argument, OpenArgument::Pathlib { .. }), }, open.item.range(), ); @@ -243,12 +224,12 @@ fn generate_fix( ) .ok()?; - let target = if let Some(path_obj) = open.path_obj { - locator.slice(path_obj.range()).to_string() - } else { - let filename = open.filename?; - let filename_code = locator.slice(filename.range()); - format!("{binding}({filename_code})") + let target = match open.argument { + OpenArgument::Builtin { filename } => { + let filename_code = locator.slice(filename.range()); + format!("{binding}({filename_code})") + } + OpenArgument::Pathlib { path } => locator.slice(path.range()).to_string(), }; let replacement = format!("{target}.{suggestion}"); diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_1.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_1.py.snap index 4824664dd8..d30974009c 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_1.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB103_FURB103_1.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs --- -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test")` --> FURB103_1.py:3:6 | 1 | from pathlib import Path @@ -20,7 +20,7 @@ help: Replace with `Path("file.txt").write_text("test")` 5 | with Path("file.txt").open("wb") as f: 6 | f.write(b"test") -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path("file.txt").write_bytes(b"test")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("file.txt").write_bytes(b"test")` --> FURB103_1.py:6:6 | 4 | f.write("test") @@ -40,7 +40,7 @@ help: Replace with `Path("file.txt").write_bytes(b"test")` 8 | with Path("file.txt").open(mode="w") as f: 9 | f.write("test") -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test")` --> FURB103_1.py:9:6 | 7 | f.write(b"test") @@ -60,7 +60,7 @@ help: Replace with `Path("file.txt").write_text("test")` 11 | with Path("file.txt").open("w", encoding="utf8") as f: 12 | f.write("test") -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test", encoding="utf8")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test", encoding="utf8")` --> FURB103_1.py:12:6 | 10 | f.write("test") @@ -80,7 +80,7 @@ help: Replace with `Path("file.txt").write_text("test", encoding="utf8")` 14 | with Path("file.txt").open("w", errors="ignore") as f: 15 | f.write("test") -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test", errors="ignore")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path("file.txt").write_text("test", errors="ignore")` --> FURB103_1.py:15:6 | 13 | f.write("test") @@ -100,7 +100,7 @@ help: Replace with `Path("file.txt").write_text("test", errors="ignore")` 17 | with Path(foo()).open("w") as f: 18 | f.write("test") -FURB103 [*] `Path().open()` followed by `write()` can be replaced by `Path(foo()).write_text("test")` +FURB103 [*] `Path.open()` followed by `write()` can be replaced by `Path(foo()).write_text("test")` --> FURB103_1.py:18:6 | 16 | f.write("test")