Use multi-fix semantics for inplace removal (#3804)

crates/ruff/resources/test/fixtures/pandas_vet/PD002.py

@@ -18,3 +18,7 @@ if True:
         columns=["a"],
         axis=1,
     )
+
+x.drop(["a"], axis=1, **kwargs, inplace=True)
+x.drop(["a"], axis=1, inplace=True, **kwargs)
+f(x.drop(["a"], axis=1, inplace=True))

crates/ruff/src/autofix/mod.rs

@@ -81,24 +81,6 @@ fn apply_fixes<'a>(
     (output, fixed)
 }
 
-/// Apply a single fix.
-pub(crate) fn apply_fix(fix: &Edit, locator: &Locator) -> String {
-    let mut output = String::with_capacity(locator.len());
-
-    // Add all contents from `last_pos` to `fix.location`.
-    let slice = locator.slice(Range::new(Location::new(1, 0), fix.location));
-    output.push_str(slice);
-
-    // Add the patch itself.
-    output.push_str(&fix.content);
-
-    // Add the remaining content.
-    let slice = locator.skip(fix.end_location);
-    output.push_str(slice);
-
-    output
-}
-
 /// Compare two fixes.
 fn cmp_fix(rule1: Rule, rule2: Rule, fix1: &Fix, fix2: &Fix) -> std::cmp::Ordering {
     fix1.location()
@@ -119,7 +101,7 @@ mod tests {
     use ruff_diagnostics::Edit;
     use ruff_python_ast::source_code::Locator;
 
-    use crate::autofix::{apply_fix, apply_fixes};
+    use crate::autofix::apply_fixes;
     use crate::rules::pycodestyle::rules::MissingNewlineAtEndOfFile;
 
     fn create_diagnostics(edit: impl IntoIterator<Item = Edit>) -> Vec<Diagnostic> {
@@ -262,31 +244,4 @@ class A:
         );
         assert_eq!(fixed.values().sum::<usize>(), 1);
     }
-
-    #[test]
-    fn apply_single_fix() {
-        let locator = Locator::new(
-            r#"
-class A(object):
-    ...
-"#
-            .trim(),
-        );
-        let contents = apply_fix(
-            &Edit {
-                content: String::new(),
-                location: Location::new(1, 7),
-                end_location: Location::new(1, 15),
-            },
-            &locator,
-        );
-        assert_eq!(
-            contents,
-            r#"
-class A:
-    ...
-"#
-            .trim()
-        );
-    }
 }

crates/ruff/src/rules/pandas_vet/fixes.rs

@@ -1,40 +1,37 @@
 use rustpython_parser::ast::{Expr, ExprKind, Keyword, Location};
 
-use ruff_diagnostics::Edit;
+use ruff_diagnostics::{Edit, Fix};
-use ruff_python_ast::helpers;
 use ruff_python_ast::source_code::Locator;
-use ruff_python_ast::types::Range;
 
-use crate::autofix::apply_fix;
 use crate::autofix::helpers::remove_argument;
 
 fn match_name(expr: &Expr) -> Option<&str> {
     if let ExprKind::Call { func, .. } = &expr.node {
         if let ExprKind::Attribute { value, .. } = &func.node {
             if let ExprKind::Name { id, .. } = &value.node {
-                Some(id)
+                return Some(id);
-            } else {
-                None
             }
-        } else {
-            None
         }
-    } else {
-        None
     }
+    None
 }
 
 /// Remove the `inplace` argument from a function call and replace it with an
 /// assignment.
-pub fn fix_inplace_argument(
+pub(super) fn convert_inplace_argument_to_assignment(
     locator: &Locator,
     expr: &Expr,
     violation_at: Location,
     violation_end: Location,
     args: &[Expr],
     keywords: &[Keyword],
-) -> Option<Edit> {
+) -> Option<Fix> {
-    if let Ok(fix) = remove_argument(
+    // Add the assignment.
+    let name = match_name(expr)?;
+    let insert_assignment = Edit::insertion(format!("{name} = "), expr.location);
+
+    // Remove the `inplace` argument.
+    let remove_argument = remove_argument(
         locator,
         expr.location,
         violation_at,
@@ -42,31 +39,8 @@ pub fn fix_inplace_argument(
         args,
         keywords,
         false,
-    ) {
+    )
-        // Reset the line index.
+    .ok()?;
-        let fix_me = Edit::deletion(
-            helpers::to_relative(fix.location, expr.location),
-            helpers::to_relative(fix.end_location, expr.location),
-        );
 
-        // Apply the deletion step.
+    Some(Fix::from_iter([insert_assignment, remove_argument]))
-        // TODO(charlie): Find a way to
-        let contents = locator.slice(Range::new(expr.location, expr.end_location.unwrap()));
-        let output = apply_fix(&fix_me, &Locator::new(contents));
-
-        // Obtain the name prefix.
-        let name = match_name(expr)?;
-
-        // Apply the assignment.
-        let new_contents = format!("{name} = {output}");
-
-        // Create the new fix.
-        Some(Edit::replacement(
-            new_contents,
-            expr.location,
-            expr.end_location.unwrap(),
-        ))
-    } else {
-        None
-    }
 }

crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs

@@ -1,12 +1,12 @@
-use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword};
+use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword, StmtKind};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic};
+use ruff_diagnostics::{AutofixKind, Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::types::Range;
 
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
-use crate::rules::pandas_vet::fixes::fix_inplace_argument;
+use crate::rules::pandas_vet::fixes::convert_inplace_argument_to_assignment;
 
 /// ## What it does
 /// Checks for `inplace=True` usages in `pandas` function and method
@@ -33,16 +33,21 @@ use crate::rules::pandas_vet::fixes::fix_inplace_argument;
 /// ## References
 /// - [_Why You Should Probably Never Use pandas inplace=True_](https://towardsdatascience.com/why-you-should-probably-never-use-pandas-inplace-true-9f9f211849e4)
 #[violation]
-pub struct PandasUseOfInplaceArgument;
+pub struct PandasUseOfInplaceArgument {
+    pub fixable: bool,
+}
+
+impl Violation for PandasUseOfInplaceArgument {
+    const AUTOFIX: AutofixKind = AutofixKind::Sometimes;
 
-impl AlwaysAutofixableViolation for PandasUseOfInplaceArgument {
     #[derive_message_formats]
     fn message(&self) -> String {
         format!("`inplace=True` should be avoided; it has inconsistent behavior")
     }
 
-    fn autofix_title(&self) -> String {
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
-        format!("Assign to variable; remove `inplace` arg")
+        self.fixable
+            .then_some(|_| format!("Assign to variable; remove `inplace` arg"))
     }
 }
 
@@ -53,9 +58,12 @@ pub fn inplace_argument(
     args: &[Expr],
     keywords: &[Keyword],
 ) -> Option<Diagnostic> {
-    for keyword in keywords {
+    let mut seen_star = false;
-        let arg = keyword.node.arg.as_ref()?;
+    for keyword in keywords.iter().rev() {
-
+        let Some(arg) = &keyword.node.arg else {
+            seen_star = true;
+            continue;
+        };
         if arg == "inplace" {
             let is_true_literal = match &keyword.node.value.node {
                 ExprKind::Constant {
@@ -65,10 +73,18 @@ pub fn inplace_argument(
                 _ => false,
             };
             if is_true_literal {
+                // Avoid applying the fix if:
+                // 1. The keyword argument is followed by a star argument (we can't be certain that
+                //    the star argument _doesn't_ contain an override).
+                // 2. The call is part of a larger expression (we're converting an expression to a
+                //    statement, and expressions can't contain statements).
+                let fixable = !seen_star
+                    && matches!(checker.ctx.current_stmt().node, StmtKind::Expr { .. })
+                    && checker.ctx.current_expr_parent().is_none();
                 let mut diagnostic =
-                    Diagnostic::new(PandasUseOfInplaceArgument, Range::from(keyword));
+                    Diagnostic::new(PandasUseOfInplaceArgument { fixable }, Range::from(keyword));
-                if checker.patch(diagnostic.kind.rule()) {
+                if fixable && checker.patch(diagnostic.kind.rule()) {
-                    if let Some(fix) = fix_inplace_argument(
+                    if let Some(fix) = convert_inplace_argument_to_assignment(
                         checker.locator,
                         expr,
                         diagnostic.location,
@@ -81,6 +97,9 @@ pub fn inplace_argument(
                 }
                 return Some(diagnostic);
             }
+
+            // Duplicate keywords is a syntax error, so we can stop here.
+            break;
         }
     }
     None

crates/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap

@@ -15,13 +15,20 @@ expression: diagnostics
     column: 34
   fix:
     edits:
-      - content: "x = x.drop([\"a\"], axis=1)"
+      - content: "x = "
         location:
           row: 5
           column: 0
         end_location:
           row: 5
-          column: 35
+          column: 0
+      - content: ""
+        location:
+          row: 5
+          column: 20
+        end_location:
+          row: 5
+          column: 34
   parent: ~
 - kind:
     name: PandasUseOfInplaceArgument
@@ -36,13 +43,20 @@ expression: diagnostics
     column: 34
   fix:
     edits:
-      - content: "x = x.drop([\"a\"], axis=1)"
+      - content: "x = "
         location:
           row: 7
           column: 0
         end_location:
           row: 7
-          column: 35
+          column: 0
+      - content: ""
+        location:
+          row: 7
+          column: 20
+        end_location:
+          row: 7
+          column: 34
   parent: ~
 - kind:
     name: PandasUseOfInplaceArgument
@@ -57,13 +71,20 @@ expression: diagnostics
     column: 16
   fix:
     edits:
-      - content: "x = x.drop(\n    columns=[\"a\"],\n    axis=1,\n)"
+      - content: "x = "
         location:
           row: 9
           column: 0
         end_location:
-          row: 13
+          row: 9
-          column: 1
+          column: 0
+      - content: ""
+        location:
+          row: 10
+          column: 4
+        end_location:
+          row: 11
+          column: 4
   parent: ~
 - kind:
     name: PandasUseOfInplaceArgument
@@ -78,12 +99,75 @@ expression: diagnostics
     column: 20
   fix:
     edits:
-      - content: "x = x.drop(\n        columns=[\"a\"],\n        axis=1,\n    )"
+      - content: "x = "
         location:
           row: 16
           column: 4
         end_location:
-          row: 20
+          row: 16
-          column: 5
+          column: 4
+      - content: ""
+        location:
+          row: 17
+          column: 8
+        end_location:
+          row: 18
+          column: 8
+  parent: ~
+- kind:
+    name: PandasUseOfInplaceArgument
+    body: "`inplace=True` should be avoided; it has inconsistent behavior"
+    suggestion: "Assign to variable; remove `inplace` arg"
+    fixable: true
+  location:
+    row: 22
+    column: 32
+  end_location:
+    row: 22
+    column: 44
+  fix:
+    edits:
+      - content: "x = "
+        location:
+          row: 22
+          column: 0
+        end_location:
+          row: 22
+          column: 0
+      - content: ""
+        location:
+          row: 22
+          column: 30
+        end_location:
+          row: 22
+          column: 44
+  parent: ~
+- kind:
+    name: PandasUseOfInplaceArgument
+    body: "`inplace=True` should be avoided; it has inconsistent behavior"
+    suggestion: ~
+    fixable: false
+  location:
+    row: 23
+    column: 22
+  end_location:
+    row: 23
+    column: 34
+  fix:
+    edits: []
+  parent: ~
+- kind:
+    name: PandasUseOfInplaceArgument
+    body: "`inplace=True` should be avoided; it has inconsistent behavior"
+    suggestion: ~
+    fixable: false
+  location:
+    row: 24
+    column: 24
+  end_location:
+    row: 24
+    column: 36
+  fix:
+    edits: []
   parent: ~
 

crates/ruff/src/rules/pyupgrade/rules/redundant_open_modes.rs

@@ -129,7 +129,7 @@ fn create_check(
 }
 
 fn create_remove_param_fix(locator: &Locator, expr: &Expr, mode_param: &Expr) -> Result<Edit> {
-    let content = locator.slice(Range::new(expr.location, expr.end_location.unwrap()));
+    let content = locator.slice(expr);
     // Find the last comma before mode_param and create a deletion fix
     // starting from the comma and ending after mode_param.
     let mut fix_start: Option<Location> = None;