From 4328448a2fa602baf893f3ae24a0eefaa687103c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 29 Mar 2023 20:16:43 -0400 Subject: [PATCH] Use multi-fix semantics for `inplace` removal (#3804) --- .../test/fixtures/pandas_vet/PD002.py | 4 + crates/ruff/src/autofix/mod.rs | 47 +------- crates/ruff/src/rules/pandas_vet/fixes.rs | 54 +++------ .../pandas_vet/rules/inplace_argument.rs | 45 +++++--- ...es__pandas_vet__tests__PD002_PD002.py.snap | 104 ++++++++++++++++-- .../pyupgrade/rules/redundant_open_modes.rs | 2 +- 6 files changed, 146 insertions(+), 110 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py b/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py index 33c5ffe91d..094fd0e5b0 100644 --- a/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py +++ b/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)) diff --git a/crates/ruff/src/autofix/mod.rs b/crates/ruff/src/autofix/mod.rs index 308b819a9f..f86daf7fc0 100644 --- a/crates/ruff/src/autofix/mod.rs +++ b/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) -> Vec { @@ -262,31 +244,4 @@ class A: ); assert_eq!(fixed.values().sum::(), 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() - ); - } } diff --git a/crates/ruff/src/rules/pandas_vet/fixes.rs b/crates/ruff/src/rules/pandas_vet/fixes.rs index 3ee7425805..437c5de674 100644 --- a/crates/ruff/src/rules/pandas_vet/fixes.rs +++ b/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_python_ast::helpers; +use ruff_diagnostics::{Edit, Fix}; 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) - } else { - None + return Some(id); } - } 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 { - if let Ok(fix) = remove_argument( +) -> Option { + // 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. - let fix_me = Edit::deletion( - helpers::to_relative(fix.location, expr.location), - helpers::to_relative(fix.end_location, expr.location), - ); + ) + .ok()?; - // Apply the deletion step. - // 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 - } + Some(Fix::from_iter([insert_assignment, remove_argument])) } diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs index 316e5e3776..83b4b4b5b2 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs +++ b/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 { - format!("Assign to variable; remove `inplace` arg") + fn autofix_title_formatter(&self) -> Option String> { + self.fixable + .then_some(|_| format!("Assign to variable; remove `inplace` arg")) } } @@ -53,9 +58,12 @@ pub fn inplace_argument( args: &[Expr], keywords: &[Keyword], ) -> Option { - for keyword in keywords { - let arg = keyword.node.arg.as_ref()?; - + let mut seen_star = false; + 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)); - if checker.patch(diagnostic.kind.rule()) { - if let Some(fix) = fix_inplace_argument( + Diagnostic::new(PandasUseOfInplaceArgument { fixable }, Range::from(keyword)); + if fixable && checker.patch(diagnostic.kind.rule()) { + 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 diff --git a/crates/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap b/crates/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap index 6d1127a1b6..a49beaa9fd 100644 --- a/crates/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap +++ b/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 - column: 1 + row: 9 + 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 - column: 5 + row: 16 + 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: ~ diff --git a/crates/ruff/src/rules/pyupgrade/rules/redundant_open_modes.rs b/crates/ruff/src/rules/pyupgrade/rules/redundant_open_modes.rs index 21e28724dd..fca436bdf0 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/redundant_open_modes.rs +++ b/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 { - 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 = None;