Add padding to prevent some autofix errors (#7461)

## Summary We should really be doing this anywhere we _replace_ an expression. See: #7455.
astral-sh · Sep 17, 2023 · 64b929b · 64b929b
1 parent 26ae0a6
commit 64b929b
Show file tree

Hide file tree

Showing 17 changed files with 175 additions and 30 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B009_B010.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B009_B010.py
@@ -53,3 +53,6 @@
 setattr(foo, "abc123", None)
 setattr(foo, r"abc123", None)
 setattr(foo.bar, r"baz", None)
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
+assert getattr(func, '_rpc')is True
diff --git a/crates/ruff/resources/test/fixtures/pycodestyle/E713.py b/crates/ruff/resources/test/fixtures/pycodestyle/E713.py
@@ -37,3 +37,4 @@
 assert {"x": not foo} in bar
 assert [42, not foo] in bar
 assert not (re.search(r"^.:\\Users\\[^\\]*\\Downloads\\.*") is None)
+assert not('name' in request)or not request['name']
diff --git a/crates/ruff/resources/test/fixtures/pylint/repeated_isinstance_calls.py b/crates/ruff/resources/test/fixtures/pylint/repeated_isinstance_calls.py
@@ -36,3 +36,8 @@ def isinstances():
     result = isinstance(var[6], int) or isinstance(var[7], int)
     result = isinstance(var[6], (float, int)) or False
     return result
+
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+if(isinstance(self.k, int)) or (isinstance(self.k, float)):
+    ...
diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP003.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP003.py
@@ -10,6 +10,5 @@
 # OK
 y = x.dtype.type(0.0)
 
-# OK
-type = lambda *args, **kwargs: None
-type("")
+# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459841
+assert isinstance(fullname, type("")is not True)
diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP012.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP012.py
@@ -75,3 +75,8 @@
 (f"foo{bar}").encode(encoding="utf-8")
 ("unicode text©").encode("utf-8")
 ("unicode text©").encode(encoding="utf-8")
+
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459882
+def _match_ignore(line):
+	input=stdin and'\n'.encode()or None
diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/getattr_with_constant.rs b/crates/ruff/src/rules/flake8_bugbear/rules/getattr_with_constant.rs
@@ -1,3 +1,4 @@
+use crate::autofix::edits::pad;
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::{self as ast, Constant, Expr};
@@ -80,17 +81,21 @@ pub(crate) fn getattr_with_constant(
     let mut diagnostic = Diagnostic::new(GetAttrWithConstant, expr.range());
     if checker.patch(diagnostic.kind.rule()) {
         diagnostic.set_fix(Fix::suggested(Edit::range_replacement(
-            if matches!(
-                obj,
-                Expr::Name(_) | Expr::Attribute(_) | Expr::Subscript(_) | Expr::Call(_)
-            ) {
-                format!("{}.{}", checker.locator().slice(obj), value)
-            } else {
-                // Defensively parenthesize any other expressions. For example, attribute accesses
-                // on `int` literals must be parenthesized, e.g., `getattr(1, "real")` becomes
-                // `(1).real`. The same is true for named expressions and others.
-                format!("({}).{}", checker.locator().slice(obj), value)
-            },
+            pad(
+                if matches!(
+                    obj,
+                    Expr::Name(_) | Expr::Attribute(_) | Expr::Subscript(_) | Expr::Call(_)
+                ) {
+                    format!("{}.{}", checker.locator().slice(obj), value)
+                } else {
+                    // Defensively parenthesize any other expressions. For example, attribute accesses
+                    // on `int` literals must be parenthesized, e.g., `getattr(1, "real")` becomes
+                    // `(1).real`. The same is true for named expressions and others.
+                    format!("({}).{}", checker.locator().slice(obj), value)
+                },
+                expr.range(),
+                checker.locator(),
+            ),
             expr.range(),
         )));
     }

diff --git a/...rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B009_B009_B010.py.snap b/...rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B009_B009_B010.py.snap
@@ -316,4 +316,19 @@ B009_B010.py:34:1: B009 [*] Do not call `getattr` with a constant attribute valu
 37 37 | 
 38 38 | # Valid setattr usage
 
+B009_B010.py:58:8: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.
+   |
+57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
+58 | assert getattr(func, '_rpc')is True
+   |        ^^^^^^^^^^^^^^^^^^^^^ B009
+   |
+   = help: Replace `getattr` with attribute access
+
+ℹ Suggested fix
+55 55 | setattr(foo.bar, r"baz", None)
+56 56 | 
+57 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
+58    |-assert getattr(func, '_rpc')is True
+   58 |+assert func._rpc is True
+
 
diff --git a/...rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B010_B009_B010.py.snap b/...rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B010_B009_B010.py.snap
@@ -82,6 +82,7 @@ B009_B010.py:53:1: B010 [*] Do not call `setattr` with a constant attribute valu
    53 |+foo.abc123 = None
 54 54 | setattr(foo, r"abc123", None)
 55 55 | setattr(foo.bar, r"baz", None)
+56 56 | 
 
 B009_B010.py:54:1: B010 [*] Do not call `setattr` with a constant attribute value. It is not any safer than normal property access.
    |
@@ -100,13 +101,17 @@ B009_B010.py:54:1: B010 [*] Do not call `setattr` with a constant attribute valu
 54    |-setattr(foo, r"abc123", None)
    54 |+foo.abc123 = None
 55 55 | setattr(foo.bar, r"baz", None)
+56 56 | 
+57 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
 
 B009_B010.py:55:1: B010 [*] Do not call `setattr` with a constant attribute value. It is not any safer than normal property access.
    |
 53 | setattr(foo, "abc123", None)
 54 | setattr(foo, r"abc123", None)
 55 | setattr(foo.bar, r"baz", None)
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B010
+56 | 
+57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
    |
    = help: Replace `setattr` with assignment
 
@@ -116,5 +121,8 @@ B009_B010.py:55:1: B010 [*] Do not call `setattr` with a constant attribute valu
 54 54 | setattr(foo, r"abc123", None)
 55    |-setattr(foo.bar, r"baz", None)
    55 |+foo.bar.baz = None
+56 56 | 
+57 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722458885
+58 58 | assert getattr(func, '_rpc')is True
 
 
diff --git a/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs b/crates/ruff/src/rules/pycodestyle/rules/not_tests.rs
@@ -1,3 +1,4 @@
+use crate::autofix::edits::pad;
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::{self as ast, CmpOp, Expr};
@@ -95,12 +96,16 @@ pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) {
                 let mut diagnostic = Diagnostic::new(NotInTest, unary_op.operand.range());
                 if checker.patch(diagnostic.kind.rule()) {
                     diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
-                        generate_comparison(
-                            left,
-                            &[CmpOp::NotIn],
-                            comparators,
-                            unary_op.into(),
-                            checker.indexer().comment_ranges(),
+                        pad(
+                            generate_comparison(
+                                left,
+                                &[CmpOp::NotIn],
+                                comparators,
+                                unary_op.into(),
+                                checker.indexer().comment_ranges(),
+                                checker.locator(),
+                            ),
+                            unary_op.range(),
                             checker.locator(),
                         ),
                         unary_op.range(),
@@ -114,12 +119,16 @@ pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) {
                 let mut diagnostic = Diagnostic::new(NotIsTest, unary_op.operand.range());
                 if checker.patch(diagnostic.kind.rule()) {
                     diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
-                        generate_comparison(
-                            left,
-                            &[CmpOp::IsNot],
-                            comparators,
-                            unary_op.into(),
-                            checker.indexer().comment_ranges(),
+                        pad(
+                            generate_comparison(
+                                left,
+                                &[CmpOp::IsNot],
+                                comparators,
+                                unary_op.into(),
+                                checker.indexer().comment_ranges(),
+                                checker.locator(),
+                            ),
+                            unary_op.range(),
                             checker.locator(),
                         ),
                         unary_op.range(),

diff --git a/...s/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E713_E713.py.snap b/...s/ruff/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__E713_E713.py.snap
@@ -102,4 +102,20 @@ E713.py:14:9: E713 [*] Test for membership should be `not in`
 16 16 | 
 17 17 | #: Okay
 
+E713.py:40:12: E713 [*] Test for membership should be `not in`
+   |
+38 | assert [42, not foo] in bar
+39 | assert not (re.search(r"^.:\\Users\\[^\\]*\\Downloads\\.*") is None)
+40 | assert not('name' in request)or not request['name']
+   |            ^^^^^^^^^^^^^^^^^ E713
+   |
+   = help: Convert to `not in`
+
+ℹ Fix
+37 37 | assert {"x": not foo} in bar
+38 38 | assert [42, not foo] in bar
+39 39 | assert not (re.search(r"^.:\\Users\\[^\\]*\\Downloads\\.*") is None)
+40    |-assert not('name' in request)or not request['name']
+   40 |+assert 'name' not in request or not request['name']
+
 
diff --git a/crates/ruff/src/rules/pylint/rules/repeated_isinstance_calls.rs b/crates/ruff/src/rules/pylint/rules/repeated_isinstance_calls.rs
@@ -2,6 +2,7 @@ use itertools::Itertools;
 use ruff_python_ast::{self as ast, Arguments, BoolOp, Expr};
 use rustc_hash::{FxHashMap, FxHashSet};
 
+use crate::autofix::edits::pad;
 use crate::autofix::snippet::SourceCodeSnippet;
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
@@ -133,7 +134,10 @@ pub(crate) fn repeated_isinstance_calls(
                 expr.range(),
             );
             if checker.patch(diagnostic.kind.rule()) {
-                diagnostic.set_fix(Fix::automatic(Edit::range_replacement(call, expr.range())));
+                diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
+                    pad(call, expr.range(), checker.locator()),
+                    expr.range(),
+                )));
             }
             checker.diagnostics.push(diagnostic);
         }

diff --git a/...es/pylint/snapshots/ruff__rules__pylint__tests__PLR1701_repeated_isinstance_calls.py.snap b/...es/pylint/snapshots/ruff__rules__pylint__tests__PLR1701_repeated_isinstance_calls.py.snap
@@ -124,4 +124,21 @@ repeated_isinstance_calls.py:30:14: PLR1701 [*] Merge `isinstance` calls: `isins
 32 32 |     # not merged but valid
 33 33 |     result = isinstance(var[5], int) and var[5] * 14 or isinstance(var[5], float) and var[5] * 14.4
 
+repeated_isinstance_calls.py:42:3: PLR1701 [*] Merge `isinstance` calls: `isinstance(self.k, float | int)`
+   |
+41 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+42 | if(isinstance(self.k, int)) or (isinstance(self.k, float)):
+   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
+43 |     ...
+   |
+   = help: Replace with `isinstance(self.k, float | int)`
+
+ℹ Fix
+39 39 | 
+40 40 | 
+41 41 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+42    |-if(isinstance(self.k, int)) or (isinstance(self.k, float)):
+   42 |+if isinstance(self.k, float | int):
+43 43 |     ...
+
 
diff --git a/...uff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__repeated_isinstance_calls.snap b/...uff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__repeated_isinstance_calls.snap
@@ -124,4 +124,21 @@ repeated_isinstance_calls.py:30:14: PLR1701 [*] Merge `isinstance` calls: `isins
 32 32 |     # not merged but valid
 33 33 |     result = isinstance(var[5], int) and var[5] * 14 or isinstance(var[5], float) and var[5] * 14.4
 
+repeated_isinstance_calls.py:42:3: PLR1701 [*] Merge `isinstance` calls: `isinstance(self.k, (float, int))`
+   |
+41 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+42 | if(isinstance(self.k, int)) or (isinstance(self.k, float)):
+   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLR1701
+43 |     ...
+   |
+   = help: Replace with `isinstance(self.k, (float, int))`
+
+ℹ Fix
+39 39 | 
+40 40 | 
+41 41 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722460483
+42    |-if(isinstance(self.k, int)) or (isinstance(self.k, float)):
+   42 |+if isinstance(self.k, (float, int)):
+43 43 |     ...
+
 
diff --git a/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs b/crates/ruff/src/rules/pyupgrade/rules/type_of_primitive.rs
@@ -1,5 +1,6 @@
 use ruff_python_ast::{self as ast, Expr};
 
+use crate::autofix::edits::pad;
 use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_text_size::Ranged;
@@ -76,7 +77,7 @@ pub(crate) fn type_of_primitive(checker: &mut Checker, expr: &Expr, func: &Expr,
         let builtin = primitive.builtin();
         if checker.semantic().is_builtin(&builtin) {
             diagnostic.set_fix(Fix::automatic(Edit::range_replacement(
-                primitive.builtin(),
+                pad(primitive.builtin(), expr.range(), checker.locator()),
                 expr.range(),
             )));
         }

diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_encode_utf8.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_encode_utf8.rs
@@ -5,7 +5,7 @@ use ruff_python_parser::{lexer, AsMode, Tok};
 use ruff_source_file::Locator;
 use ruff_text_size::{Ranged, TextRange};
 
-use crate::autofix::edits::{remove_argument, Parentheses};
+use crate::autofix::edits::{pad, remove_argument, Parentheses};
 use crate::checkers::ast::Checker;
 use crate::registry::Rule;
 
@@ -151,7 +151,10 @@ fn replace_with_bytes_literal(
         prev = range.end();
     }
 
-    Fix::automatic(Edit::range_replacement(replacement, call.range()))
+    Fix::automatic(Edit::range_replacement(
+        pad(replacement, call.range(), locator),
+        call.range(),
+    ))
 }
 
 /// UP012

diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP003.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP003.py.snap
@@ -96,4 +96,19 @@ UP003.py:5:1: UP003 [*] Use `complex` instead of `type(...)`
 7 7 | # OK
 8 8 | type(arg)(" ")
 
+UP003.py:14:29: UP003 [*] Use `str` instead of `type(...)`
+   |
+13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459841
+14 | assert isinstance(fullname, type("")is not True)
+   |                             ^^^^^^^^ UP003
+   |
+   = help: Replace `type(...)` with `str`
+
+ℹ Fix
+11 11 | y = x.dtype.type(0.0)
+12 12 | 
+13 13 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459841
+14    |-assert isinstance(fullname, type("")is not True)
+   14 |+assert isinstance(fullname, str is not True)
+
 
diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP012.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP012.py.snap
@@ -510,6 +510,7 @@ UP012.py:75:1: UP012 [*] Unnecessary UTF-8 `encoding` argument to `encode`
    75 |+(f"foo{bar}").encode()
 76 76 | ("unicode text©").encode("utf-8")
 77 77 | ("unicode text©").encode(encoding="utf-8")
+78 78 | 
 
 UP012.py:76:1: UP012 [*] Unnecessary UTF-8 `encoding` argument to `encode`
    |
@@ -528,6 +529,8 @@ UP012.py:76:1: UP012 [*] Unnecessary UTF-8 `encoding` argument to `encode`
 76    |-("unicode text©").encode("utf-8")
    76 |+("unicode text©").encode()
 77 77 | ("unicode text©").encode(encoding="utf-8")
+78 78 | 
+79 79 | 
 
 UP012.py:77:1: UP012 [*] Unnecessary UTF-8 `encoding` argument to `encode`
    |
@@ -544,5 +547,24 @@ UP012.py:77:1: UP012 [*] Unnecessary UTF-8 `encoding` argument to `encode`
 76 76 | ("unicode text©").encode("utf-8")
 77    |-("unicode text©").encode(encoding="utf-8")
    77 |+("unicode text©").encode()
+78 78 | 
+79 79 | 
+80 80 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459882
+
+UP012.py:82:17: UP012 [*] Unnecessary call to `encode` as UTF-8
+   |
+80 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459882
+81 | def _match_ignore(line):
+82 |     input=stdin and'\n'.encode()or None
+   |                    ^^^^^^^^^^^^^ UP012
+   |
+   = help: Rewrite as bytes literal
+
+ℹ Fix
+79 79 | 
+80 80 | # Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1722459882
+81 81 | def _match_ignore(line):
+82    |-	input=stdin and'\n'.encode()or None
+   82 |+	input=stdin and b'\n' or None