diff --git a/resources/test/fixtures/SPR001.py b/resources/test/fixtures/SPR001.py index a6af5b472b..047d90f30d 100644 --- a/resources/test/fixtures/SPR001.py +++ b/resources/test/fixtures/SPR001.py @@ -20,3 +20,46 @@ class Child(Parent): Child, self, ).method() # wrong + + +class BaseClass: + def f(self): + print("f") + + +def defined_outside(self): + super(MyClass, self).f() # CANNOT use super() + + +class MyClass(BaseClass): + def normal(self): + super(MyClass, self).f() # can use super() + super().f() + + def different_argument(self, other): + super(MyClass, other).f() # CANNOT use super() + + def comprehension_scope(self): + [super(MyClass, self).f() for x in [1]] # CANNOT use super() + + def inner_functions(self): + def outer_argument(): + super(MyClass, self).f() # CANNOT use super() + + def inner_argument(self): + super(MyClass, self).f() # can use super() + super().f() + + outer_argument() + inner_argument(self) + + def inner_class(self): + class InnerClass: + super(MyClass, self).f() # CANNOT use super() + + def method(inner_self): + super(MyClass, self).f() # CANNOT use super() + + InnerClass().method() + + defined_outside = defined_outside diff --git a/src/ast/checks.rs b/src/ast/checks.rs index df424becaa..ca2d263222 100644 --- a/src/ast/checks.rs +++ b/src/ast/checks.rs @@ -3,8 +3,8 @@ use std::collections::BTreeSet; use itertools::izip; use regex::Regex; use rustpython_parser::ast::{ - Arg, Arguments, Cmpop, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Keyword, - Location, Stmt, StmtKind, Unaryop, + Arg, ArgData, Arguments, Cmpop, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, + Keyword, Location, Stmt, StmtKind, Unaryop, }; use crate::ast::operations::SourceCodeLocator; @@ -693,29 +693,79 @@ pub fn check_builtin_shadowing( } } +/// Returns `true` if a call is an argumented `super` invocation. +pub fn is_super_call_with_arguments(func: &Expr, args: &Vec) -> bool { + // Check: is this a `super` call? + if let ExprKind::Name { id, .. } = &func.node { + id == "super" && !args.is_empty() + } else { + false + } +} + // flake8-super /// Check that `super()` has no args pub fn check_super_args( + scope: &Scope, + parents: &[&Stmt], expr: &Expr, func: &Expr, args: &Vec, - locator: &mut SourceCodeLocator, - autofix: &fixer::Mode, ) -> Option { - if let ExprKind::Name { id, .. } = &func.node { - if id == "super" && !args.is_empty() { - let mut check = Check::new( - CheckKind::SuperCallWithParameters, - Range::from_located(expr), - ); - if matches!(autofix, fixer::Mode::Generate | fixer::Mode::Apply) { - if let Some(fix) = fixes::remove_super_arguments(locator, expr) { - check.amend(fix); + if !is_super_call_with_arguments(func, args) { + return None; + } + + // Check: are we in a Function scope? + if !matches!(scope.kind, ScopeKind::Function { .. }) { + return None; + } + + let mut parents = parents.iter().rev(); + + // For a `super` invocation to be unnecessary, the first argument needs to match the enclosing + // class, and the second argument needs to match the first argument to the enclosing function. + if let [first_arg, second_arg] = args.as_slice() { + // Find the enclosing function definition (if any). + if let Some(StmtKind::FunctionDef { + args: parent_args, .. + }) = parents + .find(|stmt| matches!(stmt.node, StmtKind::FunctionDef { .. })) + .map(|stmt| &stmt.node) + { + // Extract the name of the first argument to the enclosing function. + if let Some(ArgData { + arg: parent_arg, .. + }) = parent_args.args.first().map(|expr| &expr.node) + { + // Find the enclosing class definition (if any). + if let Some(StmtKind::ClassDef { + name: parent_name, .. + }) = parents + .find(|stmt| matches!(stmt.node, StmtKind::ClassDef { .. })) + .map(|stmt| &stmt.node) + { + if let ( + ExprKind::Name { + id: first_arg_id, .. + }, + ExprKind::Name { + id: second_arg_id, .. + }, + ) = (&first_arg.node, &second_arg.node) + { + if first_arg_id == parent_name && second_arg_id == parent_arg { + return Some(Check::new( + CheckKind::SuperCallWithParameters, + Range::from_located(expr), + )); + } + } } } - return Some(check); } } + None } diff --git a/src/check_ast.rs b/src/check_ast.rs index 9a70ed140e..ddb9b96479 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -755,10 +755,28 @@ where // flake8-super if self.settings.select.contains(&CheckCode::SPR001) { - if let Some(check) = - checks::check_super_args(expr, func, args, &mut self.locator, self.autofix) - { - self.checks.push(check) + // Only bother going through the super check at all if we're in a `super` call. + // (We check this in `check_super_args` too, so this is just an optimization.) + if checks::is_super_call_with_arguments(func, args) { + let scope = &mut self.scopes + [*(self.scope_stack.last().expect("No current scope found."))]; + let parents: Vec<&Stmt> = self + .parent_stack + .iter() + .map(|index| self.parents[*index]) + .collect(); + if let Some(mut check) = + checks::check_super_args(scope, &parents, expr, func, args) + { + if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) { + if let Some(fix) = + fixes::remove_super_arguments(&mut self.locator, expr) + { + check.amend(fix); + } + } + self.checks.push(check) + } } } diff --git a/src/snapshots/ruff__linter__tests__spr001.snap b/src/snapshots/ruff__linter__tests__spr001.snap index 8f0a29bf54..16afacfaeb 100644 --- a/src/snapshots/ruff__linter__tests__spr001.snap +++ b/src/snapshots/ruff__linter__tests__spr001.snap @@ -50,4 +50,36 @@ expression: checks row: 22 column: 10 applied: false +- kind: SuperCallWithParameters + location: + row: 36 + column: 9 + end_location: + row: 36 + column: 29 + fix: + content: super() + location: + row: 36 + column: 9 + end_location: + row: 36 + column: 29 + applied: false +- kind: SuperCallWithParameters + location: + row: 50 + column: 13 + end_location: + row: 50 + column: 33 + fix: + content: super() + location: + row: 50 + column: 13 + end_location: + row: 50 + column: 33 + applied: false