From 08dc523a4bb7fafaab347df66b4f8d4126c972ec Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 12 Jun 2023 15:19:59 -0400 Subject: [PATCH] Handle decorators in class-parenthesis-modifying rules (#5034) ## Summary A few of our rules look at the parentheses that follow a class definition (e.g., `class Foo(object):`) and attempt to modify those parentheses. Neither of those rules were behaving properly in the presence of decorators, which were recently added to the statement range. ## Test Plan `cargo test` with a variety of new fixture tests. --- .../test/fixtures/pyupgrade/UP004.py | 13 ++ .../test/fixtures/pyupgrade/UP039.py | 17 +++ crates/ruff/src/checkers/ast/mod.rs | 4 +- .../rules/unnecessary_class_parentheses.rs | 16 ++- .../rules/useless_object_inheritance.rs | 69 +++------ ...ff__rules__pyupgrade__tests__UP004.py.snap | 131 +++++++++++++----- ...ff__rules__pyupgrade__tests__UP039.py.snap | 38 +++++ 7 files changed, 197 insertions(+), 91 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP004.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP004.py index a114f15017054e..723733f9385ede 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP004.py +++ b/crates/ruff/resources/test/fixtures/pyupgrade/UP004.py @@ -134,6 +134,19 @@ class A( ... +class A(object, object): + ... + + +@decorator() +class A(object): + ... + +@decorator() # class A(object): +class A(object): + ... + + object = A diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP039.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP039.py index a4da32bc323dcf..ba44e1deb96de1 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP039.py +++ b/crates/ruff/resources/test/fixtures/pyupgrade/UP039.py @@ -13,6 +13,14 @@ class A \ pass +@decorator() +class A(): + pass + +@decorator +class A(): + pass + # OK class A: pass @@ -24,3 +32,12 @@ class A(A): class A(metaclass=type): pass + + +@decorator() +class A: + pass + +@decorator +class A: + pass diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 0f176b69213c97..e166718bdb09d4 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -727,10 +727,10 @@ where pylint::rules::global_statement(self, name); } if self.enabled(Rule::UselessObjectInheritance) { - pyupgrade::rules::useless_object_inheritance(self, stmt, name, bases, keywords); + pyupgrade::rules::useless_object_inheritance(self, class_def, stmt); } if self.enabled(Rule::UnnecessaryClassParentheses) { - pyupgrade::rules::unnecessary_class_parentheses(self, class_def); + pyupgrade::rules::unnecessary_class_parentheses(self, class_def, stmt); } if self.enabled(Rule::AmbiguousClassName) { diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_class_parentheses.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_class_parentheses.rs index f6e0120a3a3d46..faa8f49fbd1e4b 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_class_parentheses.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_class_parentheses.rs @@ -1,10 +1,11 @@ use std::ops::Add; use ruff_text_size::{TextRange, TextSize}; -use rustpython_parser::ast::{self}; +use rustpython_parser::ast::{self, Stmt}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::identifier_range; use crate::checkers::ast::Checker; use crate::registry::AsRule; @@ -43,12 +44,17 @@ impl AlwaysAutofixableViolation for UnnecessaryClassParentheses { } /// UP039 -pub(crate) fn unnecessary_class_parentheses(checker: &mut Checker, class_def: &ast::StmtClassDef) { +pub(crate) fn unnecessary_class_parentheses( + checker: &mut Checker, + class_def: &ast::StmtClassDef, + stmt: &Stmt, +) { if !class_def.bases.is_empty() || !class_def.keywords.is_empty() { return; } - let contents = checker.locator.slice(class_def.range); + let offset = identifier_range(stmt, checker.locator).start(); + let contents = checker.locator.after(offset); // Find the open and closing parentheses between the class name and the colon, if they exist. let mut depth = 0u32; @@ -85,8 +91,8 @@ pub(crate) fn unnecessary_class_parentheses(checker: &mut Checker, class_def: &a let end = TextSize::try_from(end).unwrap(); // Add initial offset. - let start = class_def.range.start().add(start); - let end = class_def.range.start().add(end); + let start = offset.add(start); + let end = offset.add(end); let mut diagnostic = Diagnostic::new(UnnecessaryClassParentheses, TextRange::new(start, end)); if checker.patch(diagnostic.kind.rule()) { diff --git a/crates/ruff/src/rules/pyupgrade/rules/useless_object_inheritance.rs b/crates/ruff/src/rules/pyupgrade/rules/useless_object_inheritance.rs index 8a3bc3839d1daf..6fa11e3a04a235 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/useless_object_inheritance.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/useless_object_inheritance.rs @@ -1,9 +1,8 @@ -use rustpython_parser::ast::{self, Expr, Keyword, Ranged, Stmt}; +use rustpython_parser::ast::{self, Expr, Ranged, Stmt}; -use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic}; +use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::binding::{Binding, BindingKind, Bindings}; -use ruff_python_semantic::scope::Scope; +use ruff_python_ast::helpers::identifier_range; use crate::autofix::edits::remove_argument; use crate::checkers::ast::Checker; @@ -26,62 +25,40 @@ impl AlwaysAutofixableViolation for UselessObjectInheritance { } } -fn rule(name: &str, bases: &[Expr], scope: &Scope, bindings: &Bindings) -> Option { - for expr in bases { +/// UP004 +pub(crate) fn useless_object_inheritance( + checker: &mut Checker, + class_def: &ast::StmtClassDef, + stmt: &Stmt, +) { + for expr in &class_def.bases { let Expr::Name(ast::ExprName { id, .. }) = expr else { continue; }; if id != "object" { continue; } - if !matches!( - scope - .get(id.as_str()) - .map(|binding_id| &bindings[binding_id]), - None | Some(Binding { - kind: BindingKind::Builtin, - .. - }) - ) { + if !checker.semantic_model().is_builtin("object") { continue; } - return Some(Diagnostic::new( + + let mut diagnostic = Diagnostic::new( UselessObjectInheritance { - name: name.to_string(), + name: class_def.name.to_string(), }, expr.range(), - )); - } - - None -} - -/// UP004 -pub(crate) fn useless_object_inheritance( - checker: &mut Checker, - stmt: &Stmt, - name: &str, - bases: &[Expr], - keywords: &[Keyword], -) { - if let Some(mut diagnostic) = rule( - name, - bases, - checker.semantic_model().scope(), - &checker.semantic_model().bindings, - ) { + ); if checker.patch(diagnostic.kind.rule()) { - let expr_range = diagnostic.range(); - #[allow(deprecated)] - diagnostic.try_set_fix_from_edit(|| { - remove_argument( + diagnostic.try_set_fix(|| { + let edit = remove_argument( checker.locator, - stmt.start(), - expr_range, - bases, - keywords, + identifier_range(stmt, checker.locator).start(), + expr.range(), + &class_def.bases, + &class_def.keywords, true, - ) + )?; + Ok(Fix::automatic(edit)) }); } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP004.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP004.py.snap index f480d6b0df9583..e10667594383c1 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP004.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP004.py.snap @@ -9,7 +9,7 @@ UP004.py:5:9: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 2 2 | ... 3 3 | 4 4 | @@ -29,7 +29,7 @@ UP004.py:10:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 6 6 | ... 7 7 | 8 8 | @@ -51,7 +51,7 @@ UP004.py:16:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 12 12 | ... 13 13 | 14 14 | @@ -75,7 +75,7 @@ UP004.py:24:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 19 19 | ... 20 20 | 21 21 | @@ -99,7 +99,7 @@ UP004.py:31:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 26 26 | ... 27 27 | 28 28 | @@ -122,7 +122,7 @@ UP004.py:37:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 33 33 | ... 34 34 | 35 35 | @@ -146,7 +146,7 @@ UP004.py:45:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 40 40 | ... 41 41 | 42 42 | @@ -171,7 +171,7 @@ UP004.py:53:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 48 48 | ... 49 49 | 50 50 | @@ -196,7 +196,7 @@ UP004.py:61:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 56 56 | ... 57 57 | 58 58 | @@ -221,7 +221,7 @@ UP004.py:69:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 64 64 | ... 65 65 | 66 66 | @@ -243,7 +243,7 @@ UP004.py:75:12: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 72 72 | ... 73 73 | 74 74 | @@ -261,7 +261,7 @@ UP004.py:79:9: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 76 76 | ... 77 77 | 78 78 | @@ -281,7 +281,7 @@ UP004.py:84:5: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 81 81 | 82 82 | 83 83 | class B( @@ -301,7 +301,7 @@ UP004.py:92:5: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 89 89 | 90 90 | class B( 91 91 | A, @@ -320,7 +320,7 @@ UP004.py:98:5: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 95 95 | 96 96 | 97 97 | class B( @@ -340,7 +340,7 @@ UP004.py:108:5: UP004 [*] Class `B` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 105 105 | class B( 106 106 | # Comment on A. 107 107 | A, @@ -349,25 +349,6 @@ UP004.py:108:5: UP004 [*] Class `B` inherits from `object` 110 109 | ... 111 110 | -UP004.py:114:13: UP004 [*] Class `A` inherits from `object` - | -113 | def f(): -114 | class A(object): - | ^^^^^^ UP004 -115 | ... - | - = help: Remove `object` inheritance - -ℹ Suggested fix -111 111 | -112 112 | -113 113 | def f(): -114 |- class A(object): - 114 |+ class A: -115 115 | ... -116 116 | -117 117 | - UP004.py:119:5: UP004 [*] Class `A` inherits from `object` | 118 | class A( @@ -378,7 +359,7 @@ UP004.py:119:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 115 115 | ... 116 116 | 117 117 | @@ -400,7 +381,7 @@ UP004.py:125:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 121 121 | ... 122 122 | 123 123 | @@ -422,7 +403,7 @@ UP004.py:131:5: UP004 [*] Class `A` inherits from `object` | = help: Remove `object` inheritance -ℹ Suggested fix +ℹ Fix 127 127 | ... 128 128 | 129 129 | @@ -435,4 +416,78 @@ UP004.py:131:5: UP004 [*] Class `A` inherits from `object` 135 132 | 136 133 | +UP004.py:137:9: UP004 [*] Class `A` inherits from `object` + | +137 | class A(object, object): + | ^^^^^^ UP004 +138 | ... + | + = help: Remove `object` inheritance + +ℹ Fix +134 134 | ... +135 135 | +136 136 | +137 |-class A(object, object): + 137 |+class A(object): +138 138 | ... +139 139 | +140 140 | + +UP004.py:137:17: UP004 [*] Class `A` inherits from `object` + | +137 | class A(object, object): + | ^^^^^^ UP004 +138 | ... + | + = help: Remove `object` inheritance + +ℹ Fix +134 134 | ... +135 135 | +136 136 | +137 |-class A(object, object): + 137 |+class A(object): +138 138 | ... +139 139 | +140 140 | + +UP004.py:142:9: UP004 [*] Class `A` inherits from `object` + | +141 | @decorator() +142 | class A(object): + | ^^^^^^ UP004 +143 | ... + | + = help: Remove `object` inheritance + +ℹ Fix +139 139 | +140 140 | +141 141 | @decorator() +142 |-class A(object): + 142 |+class A: +143 143 | ... +144 144 | +145 145 | @decorator() # class A(object): + +UP004.py:146:9: UP004 [*] Class `A` inherits from `object` + | +145 | @decorator() # class A(object): +146 | class A(object): + | ^^^^^^ UP004 +147 | ... + | + = help: Remove `object` inheritance + +ℹ Fix +143 143 | ... +144 144 | +145 145 | @decorator() # class A(object): +146 |-class A(object): + 146 |+class A: +147 147 | ... +148 148 | +149 149 | + diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP039.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP039.py.snap index 7cbeae5b2fe264..4d1a0bb04f573c 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP039.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP039.py.snap @@ -56,4 +56,42 @@ UP039.py:12:9: UP039 [*] Unnecessary parentheses after class definition 14 14 | 15 15 | +UP039.py:17:8: UP039 [*] Unnecessary parentheses after class definition + | +16 | @decorator() +17 | class A(): + | ^^ UP039 +18 | pass + | + = help: Remove parentheses + +ℹ Fix +14 14 | +15 15 | +16 16 | @decorator() +17 |-class A(): + 17 |+class A: +18 18 | pass +19 19 | +20 20 | @decorator + +UP039.py:21:8: UP039 [*] Unnecessary parentheses after class definition + | +20 | @decorator +21 | class A(): + | ^^ UP039 +22 | pass + | + = help: Remove parentheses + +ℹ Fix +18 18 | pass +19 19 | +20 20 | @decorator +21 |-class A(): + 21 |+class A: +22 22 | pass +23 23 | +24 24 | # OK +