diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py index 2e58dc46b1766..86a6584f3a542 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py @@ -18,6 +18,7 @@ print("", *args, sep="") print("", **kwargs) print(sep="\t") +print(sep=print(1)) # OK. diff --git a/crates/ruff_linter/src/rules/refurb/rules/print_empty_string.rs b/crates/ruff_linter/src/rules/refurb/rules/print_empty_string.rs index 095d3331fbc40..e076f9a9989cb 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/print_empty_string.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/print_empty_string.rs @@ -1,7 +1,9 @@ -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::contains_effect; use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::Generator; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -85,11 +87,15 @@ pub(crate) fn print_empty_string(checker: &mut Checker, call: &ast::ExprCall) { let mut diagnostic = Diagnostic::new(PrintEmptyString { reason }, call.range()); - diagnostic.set_fix(Fix::safe_edit(Edit::replacement( - generate_suggestion(call, Separator::Remove, checker.generator()), - call.start(), - call.end(), - ))); + diagnostic.set_fix( + EmptyStringFix::from_call( + call, + Separator::Remove, + checker.semantic(), + checker.generator(), + ) + .into_fix(), + ); checker.diagnostics.push(diagnostic); } @@ -109,11 +115,15 @@ pub(crate) fn print_empty_string(checker: &mut Checker, call: &ast::ExprCall) { call.range(), ); - diagnostic.set_fix(Fix::safe_edit(Edit::replacement( - generate_suggestion(call, Separator::Remove, checker.generator()), - call.start(), - call.end(), - ))); + diagnostic.set_fix( + EmptyStringFix::from_call( + call, + Separator::Remove, + checker.semantic(), + checker.generator(), + ) + .into_fix(), + ); checker.diagnostics.push(diagnostic); } @@ -172,11 +182,10 @@ pub(crate) fn print_empty_string(checker: &mut Checker, call: &ast::ExprCall) { call.range(), ); - diagnostic.set_fix(Fix::safe_edit(Edit::replacement( - generate_suggestion(call, separator, checker.generator()), - call.start(), - call.end(), - ))); + diagnostic.set_fix( + EmptyStringFix::from_call(call, separator, checker.semantic(), checker.generator()) + .into_fix(), + ); checker.diagnostics.push(diagnostic); } @@ -200,37 +209,68 @@ enum Separator { Retain, } -/// Generate a suggestion to remove the empty string positional argument and -/// the `sep` keyword argument, if it exists. -fn generate_suggestion(call: &ast::ExprCall, separator: Separator, generator: Generator) -> String { - let mut call = call.clone(); - - // Remove all empty string positional arguments. - call.arguments.args = call - .arguments - .args - .iter() - .filter(|arg| !is_empty_string(arg)) - .cloned() - .collect::>() - .into_boxed_slice(); - - // Remove the `sep` keyword argument if it exists. - if separator == Separator::Remove { - call.arguments.keywords = call +#[derive(Debug, Clone)] +struct EmptyStringFix(Fix); + +impl EmptyStringFix { + /// Generate a suggestion to remove the empty string positional argument and + /// the `sep` keyword argument, if it exists. + fn from_call( + call: &ast::ExprCall, + separator: Separator, + semantic: &SemanticModel, + generator: Generator, + ) -> Self { + let range = call.range(); + let mut call = call.clone(); + let mut applicability = Applicability::Safe; + + // Remove all empty string positional arguments. + call.arguments.args = call .arguments - .keywords + .args .iter() - .filter(|keyword| { - keyword - .arg - .as_ref() - .map_or(true, |arg| arg.as_str() != "sep") - }) + .filter(|arg| !is_empty_string(arg)) .cloned() .collect::>() .into_boxed_slice(); + + // Remove the `sep` keyword argument if it exists. + if separator == Separator::Remove { + call.arguments.keywords = call + .arguments + .keywords + .iter() + .filter(|keyword| { + let Some(arg) = keyword.arg.as_ref() else { + return true; + }; + + if arg.as_str() != "sep" { + return true; + } + + if contains_effect(&keyword.value, |id| semantic.has_builtin_binding(id)) { + applicability = Applicability::Unsafe; + } + + false + }) + .cloned() + .collect::>() + .into_boxed_slice(); + } + + let contents = generator.expr(&call.into()); + + Self(Fix::applicable_edit( + Edit::range_replacement(contents, range), + applicability, + )) } - generator.expr(&call.into()) + /// Return the [`Fix`] contained in this [`EmptyStringFix`]. + fn into_fix(self) -> Fix { + self.0 + } } diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB105_FURB105.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB105_FURB105.py.snap index f37d6398a1119..fc552a58e7e0e 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB105_FURB105.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB105_FURB105.py.snap @@ -312,7 +312,7 @@ FURB105.py:18:1: FURB105 [*] Unnecessary empty string passed to `print` 18 |+print(*args, sep="") 19 19 | print("", **kwargs) 20 20 | print(sep="\t") -21 21 | +21 21 | print(sep=print(1)) FURB105.py:19:1: FURB105 [*] Unnecessary empty string passed to `print` | @@ -321,6 +321,7 @@ FURB105.py:19:1: FURB105 [*] Unnecessary empty string passed to `print` 19 | print("", **kwargs) | ^^^^^^^^^^^^^^^^^^^ FURB105 20 | print(sep="\t") +21 | print(sep=print(1)) | = help: Remove empty string @@ -331,8 +332,8 @@ FURB105.py:19:1: FURB105 [*] Unnecessary empty string passed to `print` 19 |-print("", **kwargs) 19 |+print(**kwargs) 20 20 | print(sep="\t") -21 21 | -22 22 | # OK. +21 21 | print(sep=print(1)) +22 22 | FURB105.py:20:1: FURB105 [*] Unnecessary separator passed to `print` | @@ -340,8 +341,7 @@ FURB105.py:20:1: FURB105 [*] Unnecessary separator passed to `print` 19 | print("", **kwargs) 20 | print(sep="\t") | ^^^^^^^^^^^^^^^ FURB105 -21 | -22 | # OK. +21 | print(sep=print(1)) | = help: Remove separator @@ -351,8 +351,27 @@ FURB105.py:20:1: FURB105 [*] Unnecessary separator passed to `print` 19 19 | print("", **kwargs) 20 |-print(sep="\t") 20 |+print() -21 21 | -22 22 | # OK. -23 23 | +21 21 | print(sep=print(1)) +22 22 | +23 23 | # OK. +FURB105.py:21:1: FURB105 [*] Unnecessary separator passed to `print` + | +19 | print("", **kwargs) +20 | print(sep="\t") +21 | print(sep=print(1)) + | ^^^^^^^^^^^^^^^^^^^ FURB105 +22 | +23 | # OK. + | + = help: Remove separator +ℹ Unsafe fix +18 18 | print("", *args, sep="") +19 19 | print("", **kwargs) +20 20 | print(sep="\t") +21 |-print(sep=print(1)) + 21 |+print() +22 22 | +23 23 | # OK. +24 24 |