Treat sep arguments with effects as unsafe removals (#13165)

## Summary

Closes https://github.com/astral-sh/ruff/issues/13126.

crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py

@@ -18,6 +18,7 @@ print("", *args)
 print("", *args, sep="")
 print("", **kwargs)
 print(sep="\t")
+print(sep=print(1))
 
 # OK.
 

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(
+            diagnostic.set_fix(
-                generate_suggestion(call, Separator::Remove, checker.generator()),
+                EmptyStringFix::from_call(
-                call.start(),
+                    call,
-                call.end(),
+                    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(
+                diagnostic.set_fix(
-                    generate_suggestion(call, Separator::Remove, checker.generator()),
+                    EmptyStringFix::from_call(
-                    call.start(),
+                        call,
-                    call.end(),
+                        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(
+            diagnostic.set_fix(
-                generate_suggestion(call, separator, checker.generator()),
+                EmptyStringFix::from_call(call, separator, checker.semantic(), checker.generator())
-                call.start(),
+                    .into_fix(),
-                call.end(),
+            );
-            )));
 
             checker.diagnostics.push(diagnostic);
         }
@@ -200,37 +209,68 @@ enum Separator {
     Retain,
 }
 
-/// Generate a suggestion to remove the empty string positional argument and
+#[derive(Debug, Clone)]
-/// the `sep` keyword argument, if it exists.
+struct EmptyStringFix(Fix);
-fn generate_suggestion(call: &ast::ExprCall, separator: Separator, generator: Generator) -> String {
-    let mut call = call.clone();
 
-    // Remove all empty string positional arguments.
+impl EmptyStringFix {
-    call.arguments.args = call
+    /// Generate a suggestion to remove the empty string positional argument and
-        .arguments
+    /// the `sep` keyword argument, if it exists.
-        .args
+    fn from_call(
-        .iter()
+        call: &ast::ExprCall,
-        .filter(|arg| !is_empty_string(arg))
+        separator: Separator,
-        .cloned()
+        semantic: &SemanticModel,
-        .collect::<Vec<_>>()
+        generator: Generator,
-        .into_boxed_slice();
+    ) -> Self {
+        let range = call.range();
+        let mut call = call.clone();
+        let mut applicability = Applicability::Safe;
 
-    // Remove the `sep` keyword argument if it exists.
+        // Remove all empty string positional arguments.
-    if separator == Separator::Remove {
+        call.arguments.args = call
-        call.arguments.keywords = call
             .arguments
-            .keywords
+            .args
             .iter()
-            .filter(|keyword| {
+            .filter(|arg| !is_empty_string(arg))
-                keyword
-                    .arg
-                    .as_ref()
-                    .map_or(true, |arg| arg.as_str() != "sep")
-            })
             .cloned()
             .collect::<Vec<_>>()
             .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::<Vec<_>>()
+                .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
+    }
 }

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 | 
+21 21 | print(sep=print(1))
-22 22 | # OK.
+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 | 
+21 | print(sep=print(1))
-22 | # OK.
    |
    = 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 | 
+21 21 | print(sep=print(1))
-22 22 | # OK.
+22 22 | 
-23 23 | 
+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 |