logic improvements

astral-sh · Jan 29, 2024 · 9875f6a · 9875f6a
1 parent 7f3f81d
commit 9875f6a
Show file tree

Hide file tree

Showing 3 changed files with 173 additions and 50 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dunder_call.py b/crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dunder_call.py
@@ -37,6 +37,13 @@
     +
     4
 ))
+print(2 * a.__add__(3))  # PLC2801
+x = 2 * a.__add__(3)  # PLC2801
+x = 2 * -a.__add__(3)  # PLC2801
+x = a.__add__(3)  # PLC2801
+x = -a.__add__(3)  # PLC2801
+x = (-a).__add__(3)  # PLC2801
+x = -(-a).__add__(3)  # PLC2801
 
 
 class Thing:

diff --git a/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dunder_call.rs b/crates/ruff_linter/src/rules/pylint/rules/unnecessary_dunder_call.rs
@@ -2,7 +2,6 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::{self as ast, Expr, Stmt};
 use ruff_python_semantic::SemanticModel;
-use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
@@ -167,53 +166,24 @@ pub(crate) fn unnecessary_dunder_call(checker: &mut Checker, call: &ast::ExprCal
     );
 
     if let Some(mut fixed) = fixed {
-        // check if this attribute is preceded by a unary operator, e.g. `-a.__sub__(1)`
-        // if it is, the fix has to be handled differently to maintain semantics.
-        let is_in_unary = checker
+        // by the looks of it, we don't need to wrap the expression in parens if
+        // it's the only argument to a call expression.
+        // being in any other kind of expression though, we *will* add parens.
+        // e.g. `print(a.__add__(3))` -> `print(a + 3)` instead of `print((a + 3))`
+        // a multiplication expression example: `x = 2 * a.__add__(3)` -> `x = 2 * (a + 3)`
+        let wrap_in_paren = checker
             .semantic()
             .current_expression_parent()
-            .is_some_and(|parent| matches!(parent, Expr::UnaryOp(ast::ExprUnaryOp { .. })));
-
-        if is_in_unary {
-            let mut tokenizer =
-                SimpleTokenizer::starts_at(value.as_ref().end(), checker.locator().contents())
-                    .skip_trivia();
-
-            let token = tokenizer.next().unwrap();
-            // we're going to start looking for the first immediate Right-Parentheses,
-            // and the first Dot token after that.
-
-            // if our attribute is within parentheses with a unary, e.g. `-(-5).__sub__(1)`
-            // we have to add the fix within the existing parentheses, //     ^
-            // because `--5 - 1` is not the same as `-(-5 - 1)`
-            if token.kind != SimpleTokenKind::RParen {
-                // if we're not in parentheses, we're going to wrap the fix in parentheses
-                // to maintain semantic integrity.
-                // `-a.__sub__(1)` -> `-(a - 1)`
-                fixed = format!("({fixed})");
-            }
+            .is_some_and(|parent| !parent.is_call_expr());
 
-            // find the dot token for this call
-            let dot_start = if token.kind == SimpleTokenKind::Dot {
-                token.start()
-            } else {
-                tokenizer
-                    .find(|token| token.kind == SimpleTokenKind::Dot)
-                    .unwrap()
-                    .start()
-            };
-
-            diagnostic.set_fix(Fix::unsafe_edits(
-                Edit::range_replacement(fixed, value.as_ref().range()),
-                [Edit::deletion(dot_start, call.end())],
-            ));
-        } else {
-            // `(3).__add__(1)` -> `3 + 1`
-            diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
-                fixed,
-                call.range(),
-            )));
+        if wrap_in_paren {
+            fixed = format!("({})", fixed);
         }
+
+        diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
+            fixed,
+            call.range(),
+        )));
     };
 
     checker.diagnostics.push(diagnostic);

diff --git a/...lint/snapshots/ruff_linter__rules__pylint__tests__PLC2801_unnecessary_dunder_call.py.snap b/...lint/snapshots/ruff_linter__rules__pylint__tests__PLC2801_unnecessary_dunder_call.py.snap
@@ -578,6 +578,8 @@ unnecessary_dunder_call.py:35:7: PLC2801 [*] Unnecessary dunder call to `__rsub_
 38 | |     4
 39 | | ))
    | |_^ PLC2801
+40 |   print(2 * a.__add__(3))  # PLC2801
+41 |   x = 2 * a.__add__(3)  # PLC2801
    |
    = help: Use `-` operator
 
@@ -592,14 +594,158 @@ unnecessary_dunder_call.py:35:7: PLC2801 [*] Unnecessary dunder call to `__rsub_
 38    |-    4
 39    |-))
    37 |+    4) - a)
-40 38 | 
-41 39 | 
-42 40 | class Thing:
+40 38 | print(2 * a.__add__(3))  # PLC2801
+41 39 | x = 2 * a.__add__(3)  # PLC2801
+42 40 | x = 2 * -a.__add__(3)  # PLC2801
 
-unnecessary_dunder_call.py:51:16: PLC2801 Unnecessary dunder call to `__getattribute__`. Access attribute directly or use getattr built-in function.
+unnecessary_dunder_call.py:40:11: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
    |
-50 |     def do_thing(self, item):
-51 |         return object.__getattribute__(self, item)  # PLC2801
+38 |     4
+39 | ))
+40 | print(2 * a.__add__(3))  # PLC2801
+   |           ^^^^^^^^^^^^ PLC2801
+41 | x = 2 * a.__add__(3)  # PLC2801
+42 | x = 2 * -a.__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+37 37 |     +
+38 38 |     4
+39 39 | ))
+40    |-print(2 * a.__add__(3))  # PLC2801
+   40 |+print(2 * (a + 3))  # PLC2801
+41 41 | x = 2 * a.__add__(3)  # PLC2801
+42 42 | x = 2 * -a.__add__(3)  # PLC2801
+43 43 | x = a.__add__(3)  # PLC2801
+
+unnecessary_dunder_call.py:41:9: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+39 | ))
+40 | print(2 * a.__add__(3))  # PLC2801
+41 | x = 2 * a.__add__(3)  # PLC2801
+   |         ^^^^^^^^^^^^ PLC2801
+42 | x = 2 * -a.__add__(3)  # PLC2801
+43 | x = a.__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+38 38 |     4
+39 39 | ))
+40 40 | print(2 * a.__add__(3))  # PLC2801
+41    |-x = 2 * a.__add__(3)  # PLC2801
+   41 |+x = 2 * (a + 3)  # PLC2801
+42 42 | x = 2 * -a.__add__(3)  # PLC2801
+43 43 | x = a.__add__(3)  # PLC2801
+44 44 | x = -a.__add__(3)  # PLC2801
+
+unnecessary_dunder_call.py:42:10: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+40 | print(2 * a.__add__(3))  # PLC2801
+41 | x = 2 * a.__add__(3)  # PLC2801
+42 | x = 2 * -a.__add__(3)  # PLC2801
+   |          ^^^^^^^^^^^^ PLC2801
+43 | x = a.__add__(3)  # PLC2801
+44 | x = -a.__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+39 39 | ))
+40 40 | print(2 * a.__add__(3))  # PLC2801
+41 41 | x = 2 * a.__add__(3)  # PLC2801
+42    |-x = 2 * -a.__add__(3)  # PLC2801
+   42 |+x = 2 * -(a + 3)  # PLC2801
+43 43 | x = a.__add__(3)  # PLC2801
+44 44 | x = -a.__add__(3)  # PLC2801
+45 45 | x = (-a).__add__(3)  # PLC2801
+
+unnecessary_dunder_call.py:43:5: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+41 | x = 2 * a.__add__(3)  # PLC2801
+42 | x = 2 * -a.__add__(3)  # PLC2801
+43 | x = a.__add__(3)  # PLC2801
+   |     ^^^^^^^^^^^^ PLC2801
+44 | x = -a.__add__(3)  # PLC2801
+45 | x = (-a).__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+40 40 | print(2 * a.__add__(3))  # PLC2801
+41 41 | x = 2 * a.__add__(3)  # PLC2801
+42 42 | x = 2 * -a.__add__(3)  # PLC2801
+43    |-x = a.__add__(3)  # PLC2801
+   43 |+x = a + 3  # PLC2801
+44 44 | x = -a.__add__(3)  # PLC2801
+45 45 | x = (-a).__add__(3)  # PLC2801
+46 46 | x = -(-a).__add__(3)  # PLC2801
+
+unnecessary_dunder_call.py:44:6: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+42 | x = 2 * -a.__add__(3)  # PLC2801
+43 | x = a.__add__(3)  # PLC2801
+44 | x = -a.__add__(3)  # PLC2801
+   |      ^^^^^^^^^^^^ PLC2801
+45 | x = (-a).__add__(3)  # PLC2801
+46 | x = -(-a).__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+41 41 | x = 2 * a.__add__(3)  # PLC2801
+42 42 | x = 2 * -a.__add__(3)  # PLC2801
+43 43 | x = a.__add__(3)  # PLC2801
+44    |-x = -a.__add__(3)  # PLC2801
+   44 |+x = -(a + 3)  # PLC2801
+45 45 | x = (-a).__add__(3)  # PLC2801
+46 46 | x = -(-a).__add__(3)  # PLC2801
+47 47 | 
+
+unnecessary_dunder_call.py:45:5: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+43 | x = a.__add__(3)  # PLC2801
+44 | x = -a.__add__(3)  # PLC2801
+45 | x = (-a).__add__(3)  # PLC2801
+   |     ^^^^^^^^^^^^^^^ PLC2801
+46 | x = -(-a).__add__(3)  # PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+42 42 | x = 2 * -a.__add__(3)  # PLC2801
+43 43 | x = a.__add__(3)  # PLC2801
+44 44 | x = -a.__add__(3)  # PLC2801
+45    |-x = (-a).__add__(3)  # PLC2801
+   45 |+x = -a + 3  # PLC2801
+46 46 | x = -(-a).__add__(3)  # PLC2801
+47 47 | 
+48 48 | 
+
+unnecessary_dunder_call.py:46:6: PLC2801 [*] Unnecessary dunder call to `__add__`. Use `+` operator.
+   |
+44 | x = -a.__add__(3)  # PLC2801
+45 | x = (-a).__add__(3)  # PLC2801
+46 | x = -(-a).__add__(3)  # PLC2801
+   |      ^^^^^^^^^^^^^^^ PLC2801
+   |
+   = help: Use `+` operator
+
+ℹ Unsafe fix
+43 43 | x = a.__add__(3)  # PLC2801
+44 44 | x = -a.__add__(3)  # PLC2801
+45 45 | x = (-a).__add__(3)  # PLC2801
+46    |-x = -(-a).__add__(3)  # PLC2801
+   46 |+x = -(-a + 3)  # PLC2801
+47 47 | 
+48 48 | 
+49 49 | class Thing:
+
+unnecessary_dunder_call.py:58:16: PLC2801 Unnecessary dunder call to `__getattribute__`. Access attribute directly or use getattr built-in function.
+   |
+57 |     def do_thing(self, item):
+58 |         return object.__getattribute__(self, item)  # PLC2801
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC2801
    |
    = help: Access attribute directly or use getattr built-in function