[refurb] Add fixes for FURB101, FURB103 (#20520)

## Summary Part of `PTH-*` fixes: #19404 (comment) ## Test Plan `cargo nextest run furb`

Author: chirizxc

crates/ruff_linter/src/preview.rs

@@ -259,3 +259,13 @@ pub(crate) const fn is_b006_unsafe_fix_preserve_assignment_expr_enabled(
 ) -> bool {
  settings.preview.is_enabled()
 }
+
+// https://github.com/astral-sh/ruff/pull/20520
+pub(crate) const fn is_fix_read_whole_file_enabled(settings: &LinterSettings) -> bool {
+ settings.preview.is_enabled()
+}
+
+// https://github.com/astral-sh/ruff/pull/20520
+pub(crate) const fn is_fix_write_whole_file_enabled(settings: &LinterSettings) -> bool {
+ settings.preview.is_enabled()
+}

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

@@ -1,14 +1,13 @@
 use std::borrow::Cow;
 
-use ruff_python_ast::name::Name;
-use ruff_python_ast::{self as ast, Expr, parenthesize::parenthesized_range};
+use ruff_python_ast::PythonVersion;
+use ruff_python_ast::{self as ast, Expr, name::Name, parenthesize::parenthesized_range};
 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::{Applicability, Edit, Fix};
-use ruff_python_ast::PythonVersion;
 
 /// Format a code snippet to call `name.method()`.
 pub(super) fn generate_method_call(name: Name, method: &str, generator: Generator) -> String {
@@ -345,12 +344,8 @@ pub(super) fn parenthesize_loop_iter_if_necessary<'a>(
  let iter_in_source = locator.slice(iter);
 
  match iter {
- ast::Expr::Tuple(tuple) if !tuple.parenthesized => {
- Cow::Owned(format!("({iter_in_source})"))
- }
- ast::Expr::Lambda(_) | ast::Expr::If(_)
- if matches!(location, IterLocation::Comprehension) =>
- {
+ Expr::Tuple(tuple) if !tuple.parenthesized => Cow::Owned(format!("({iter_in_source})")),
+ Expr::Lambda(_) | Expr::If(_) if matches!(location, IterLocation::Comprehension) => {
  Cow::Owned(format!("({iter_in_source})"))
  }
  _ => Cow::Borrowed(iter_in_source),

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

@@ -12,6 +12,7 @@ mod tests {
  use test_case::test_case;
 
  use crate::registry::Rule;
+ use crate::settings::types::PreviewMode;
  use crate::test::test_path;
  use crate::{assert_diagnostics, settings};
 
@@ -62,6 +63,25 @@ mod tests {
  Ok(())
  }
 
+ #[test_case(Rule::ReadWholeFile, Path::new("FURB101.py"))]
+ #[test_case(Rule::WriteWholeFile, Path::new("FURB103.py"))]
+ fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
+ let snapshot = format!(
+ "preview_{}_{}",
+ rule_code.noqa_code(),
+ path.to_string_lossy()
+ );
+ let diagnostics = test_path(
+ Path::new("refurb").join(path).as_path(),
+ &settings::LinterSettings {
+ preview: PreviewMode::Enabled,
+ ..settings::LinterSettings::for_rule(rule_code)
+ },
+ )?;
+ assert_diagnostics!(snapshot, diagnostics);
+ Ok(())
+ }
+
  #[test]
  fn write_whole_file_python_39() -> Result<()> {
  let diagnostics = test_path(

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

@@ -1,14 +1,17 @@
+use ruff_diagnostics::{Applicability, Edit, Fix};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
-use ruff_python_ast::visitor::{self, Visitor};
-use ruff_python_ast::{self as ast, Expr};
+use ruff_python_ast::{
+ self as ast, Expr, Stmt,
+ visitor::{self, Visitor},
+};
 use ruff_python_codegen::Generator;
 use ruff_text_size::{Ranged, TextRange};
 
-use crate::Violation;
 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::{FixAvailability, Violation};
 
 /// ## What it does
 /// Checks for uses of `open` and `read` that can be replaced by `pathlib`
@@ -31,6 +34,8 @@ use crate::rules::refurb::helpers::{FileOpen, find_file_opens};
 ///
 /// contents = Path(filename).read_text()
 /// ```
+/// ## Fix Safety
+/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
 ///
 /// ## References
 /// - [Python documentation: `Path.read_bytes`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.read_bytes)
@@ -42,12 +47,22 @@ pub(crate) struct ReadWholeFile {
 }
 
 impl Violation for ReadWholeFile {
+ const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
  #[derive_message_formats]
  fn message(&self) -> String {
  let filename = self.filename.truncated_display();
  let suggestion = self.suggestion.truncated_display();
  format!("`open` and `read` should be replaced by `Path({filename}).{suggestion}`")
  }
+
+ fn fix_title(&self) -> Option<String> {
+ Some(format!(
+ "Replace with `Path({}).{}`",
+ self.filename.truncated_display(),
+ self.suggestion.truncated_display(),
+ ))
+ }
 }
 
 /// FURB101
@@ -64,21 +79,27 @@ pub(crate) fn read_whole_file(checker: &Checker, with: &ast::StmtWith) {
  }
 
  // Then we need to match each `open` operation with exactly one `read` call.
- let mut matcher = ReadMatcher::new(checker, candidates);
+ let mut matcher = ReadMatcher::new(checker, candidates, with);
  visitor::walk_body(&mut matcher, &with.body);
 }
 
 /// AST visitor that matches `open` operations with the corresponding `read` calls.
 struct ReadMatcher<'a, 'b> {
  checker: &'a Checker<'b>,
  candidates: Vec<FileOpen<'a>>,
+ with_stmt: &'a ast::StmtWith,
 }
 
 impl<'a, 'b> ReadMatcher<'a, 'b> {
- fn new(checker: &'a Checker<'b>, candidates: Vec<FileOpen<'a>>) -> Self {
+ fn new(
+ checker: &'a Checker<'b>,
+ candidates: Vec<FileOpen<'a>>,
+ with_stmt: &'a ast::StmtWith,
+ ) -> Self {
  Self {
  checker,
  candidates,
+ with_stmt,
  }
  }
 }
@@ -92,15 +113,38 @@ impl<'a> Visitor<'a> for ReadMatcher<'a, '_> {
  .position(|open| open.is_ref(read_from))
  {
  let open = self.candidates.remove(open);
- self.checker.report_diagnostic(
+ let suggestion = make_suggestion(&open, self.checker.generator());
+ let mut diagnostic = self.checker.report_diagnostic(
  ReadWholeFile {
  filename: SourceCodeSnippet::from_str(
  &self.checker.generator().expr(open.filename),
  ),
- suggestion: make_suggestion(&open, self.checker.generator()),
+ suggestion: SourceCodeSnippet::from_str(&suggestion),
  },
  open.item.range(),
  );
+
+ if !crate::preview::is_fix_read_whole_file_enabled(self.checker.settings()) {
+ return;
+ }
+
+ let target = match self.with_stmt.body.first() {
+ Some(Stmt::Assign(assign))
+ if assign.value.range().contains_range(expr.range()) =>
+ {
+ match assign.targets.first() {
+ Some(Expr::Name(name)) => Some(name.id.as_str()),
+ _ => None,
+ }
+ }
+ _ => None,
+ };
+
+ if let Some(fix) =
+ generate_fix(self.checker, &open, target, self.with_stmt, &suggestion)
+ {
+ diagnostic.set_fix(fix);
+ }
  }
  return;
  }
@@ -125,7 +169,7 @@ fn match_read_call(expr: &Expr) -> Option<&Expr> {
  Some(&*attr.value)
 }
 
-fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> SourceCodeSnippet {
+fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> String {
  let name = ast::ExprName {
  id: open.mode.pathlib_method(),
  ctx: ast::ExprContext::Load,
@@ -143,5 +187,46 @@ fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> SourceCodeSnipp
  range: TextRange::default(),
  node_index: ruff_python_ast::AtomicNodeIndex::NONE,
  };
- SourceCodeSnippet::from_str(&generator.expr(&call.into()))
+ generator.expr(&call.into())
+}
+
+fn generate_fix(
+ checker: &Checker,
+ open: &FileOpen,
+ target: Option<&str>,
+ with_stmt: &ast::StmtWith,
+ suggestion: &str,
+) -> Option<Fix> {
+ if !(with_stmt.items.len() == 1 && matches!(with_stmt.body.as_slice(), [Stmt::Assign(_)])) {
+ return None;
+ }
+
+ let locator = checker.locator();
+ let filename_code = locator.slice(open.filename.range());
+
+ let (import_edit, binding) = checker
+ .importer()
+ .get_or_import_symbol(
+ &ImportRequest::import("pathlib", "Path"),
+ with_stmt.start(),
+ checker.semantic(),
+ )
+ .ok()?;
+
+ let replacement = match target {
+ Some(var) => format!("{var} = {binding}({filename_code}).{suggestion}"),
+ None => format!("{binding}({filename_code}).{suggestion}"),
+ };
+
+ let applicability = if checker.comment_ranges().intersects(with_stmt.range()) {
+ Applicability::Unsafe
+ } else {
+ Applicability::Safe
+ };
+
+ Some(Fix::applicable_edits(
+ Edit::range_replacement(replacement, with_stmt.range()),
+ [import_edit],
+ applicability,
+ ))
 }