Skip to content

Commit

Permalink
Handle decorators in class-parenthesis-modifying rules (#5034)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
charliermarsh authored and konstin committed Jun 13, 2023
1 parent bdf6cb1 commit 08dc523
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 91 deletions.
13 changes: 13 additions & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP004.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,19 @@ class A(
...


class A(object, object):
...


@decorator()
class A(object):
...

@decorator() # class A(object):
class A(object):
...


object = A


Expand Down
17 changes: 17 additions & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP039.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ class A \
pass


@decorator()
class A():
pass

@decorator
class A():
pass

# OK
class A:
pass
Expand All @@ -24,3 +32,12 @@ class A(A):

class A(metaclass=type):
pass


@decorator()
class A:
pass

@decorator
class A:
pass
4 changes: 2 additions & 2 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down
69 changes: 23 additions & 46 deletions crates/ruff/src/rules/pyupgrade/rules/useless_object_inheritance.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -26,62 +25,40 @@ impl AlwaysAutofixableViolation for UselessObjectInheritance {
}
}

fn rule(name: &str, bases: &[Expr], scope: &Scope, bindings: &Bindings) -> Option<Diagnostic> {
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);
Expand Down
Loading

0 comments on commit 08dc523

Please sign in to comment.