Fix false positives in PD002 (#4337)

astral-sh · May 10, 2023 · 04097d1 · 04097d1
1 parent a2b8487
commit 04097d1
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 1 deletion.
diff --git a/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py b/crates/ruff/resources/test/fixtures/pandas_vet/PD002.py
@@ -24,3 +24,5 @@
 f(x.drop(["a"], axis=1, inplace=True))
 
 x.apply(lambda x: x.sort_values('a', inplace=True))
+import torch
+torch.m.ReLU(inplace=True) # safe because this isn't a pandas call
diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs
@@ -2902,7 +2902,8 @@ where
                     .enabled(Rule::PandasUseOfInplaceArgument)
                 {
                     self.diagnostics.extend(
-                        pandas_vet::rules::inplace_argument(self, expr, args, keywords).into_iter(),
+                        pandas_vet::rules::inplace_argument(self, expr, func, args, keywords)
+                            .into_iter(),
                     );
                 }
                 pandas_vet::rules::check_call(self, func);

diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs
@@ -1,3 +1,4 @@
+use ruff_python_semantic::binding::{BindingKind, Importation};
 use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword, StmtKind};
 
 use ruff_diagnostics::{AutofixKind, Diagnostic, Violation};
@@ -51,10 +52,29 @@ impl Violation for PandasUseOfInplaceArgument {
 pub fn inplace_argument(
     checker: &Checker,
     expr: &Expr,
+    func: &Expr,
     args: &[Expr],
     keywords: &[Keyword],
 ) -> Option<Diagnostic> {
     let mut seen_star = false;
+    let mut is_checkable = false;
+    let mut is_pandas = false;
+
+    if let Some(call_path) = checker.ctx.resolve_call_path(func) {
+        is_checkable = true;
+
+        let module = call_path[0];
+        is_pandas = checker.ctx.find_binding(module).map_or(false, |binding| {
+            matches!(
+                binding.kind,
+                BindingKind::Importation(Importation {
+                    full_name: "pandas",
+                    ..
+                })
+            )
+        });
+    }
+
     for keyword in keywords.iter().rev() {
         let Some(arg) = &keyword.node.arg else {
             seen_star = true;
@@ -94,6 +114,13 @@ pub fn inplace_argument(
                         diagnostic.set_fix(fix);
                     }
                 }
+
+                // Without a static type system, only module-level functions could potentially be
+                // non-pandas calls. If they're not, `inplace` should be considered safe.
+                if is_checkable && !is_pandas {
+                    return None;
+                }
+
                 return Some(diagnostic);
             }
 

diff --git a/...s/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap b/...s/ruff/src/rules/pandas_vet/snapshots/ruff__rules__pandas_vet__tests__PD002_PD002.py.snap
@@ -133,6 +133,8 @@ PD002.py:26:38: PD002 `inplace=True` should be avoided; it has inconsistent beha
 27 | 
 28 | x.apply(lambda x: x.sort_values('a', inplace=True))
    |                                      ^^^^^^^^^^^^ PD002
+29 | import torch
+30 | torch.m.ReLU(inplace=True) # safe because this isn't a pandas call
    |
    = help: Assign to variable; remove `inplace` arg