Skip to content

Commit 54df73c

Browse files
IDrokin117Igor Drokin
andauthored
[pyupgrade] Apply UP008 only when the __class__ cell exists (#19424)
## Summary Resolves #19357 Skip UP008 diagnostic for `builtins.super(P, self)` calls when `__class__` is not referenced locally, preventing incorrect fixes. **Note:** I haven't found concrete information about which cases `__class__` will be loaded into the scope. Let me know if anyone has references, it would be useful to enhance the implementation. I did a lot of tests to determine when `__class__` is loaded. Considered sources: 1. [Python doc super](https://docs.python.org/3/library/functions.html#super) 2. [Python doc classes](https://docs.python.org/3/tutorial/classes.html) 3. [pep-3135](https://peps.python.org/pep-3135/#specification) As I understand it, Python will inject at runtime into local scope a `__class__` variable if it detects references to `super` or `__class__`. This allows calling `super()` and passing appropriate parameters. However, the compiler doesn't do the same for `builtins.super`, so we need to somehow introduce `__class__` into the local scope. I figured out `__class__` will be in scope with valid value when two conditions are met: 1. `super` or `__class__` names have been loaded within function scope 4. `__class__` is not overridden. I think my solution isn't elegant, so I would be appreciate a detailed review. ## Test Plan Added 19 test cases, updated snapshots. --------- Co-authored-by: Igor Drokin <drokinii1017@gmail.com>
1 parent d7524ea commit 54df73c

File tree

4 files changed

+620
-41
lines changed

4 files changed

+620
-41
lines changed

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP008.py

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,133 @@ def method2(self):
141141

142142
def method3(self):
143143
super(ExampleWithKeywords, self).some_method() # Should be fixed - no keywords
144+
145+
# See: https://github.com/astral-sh/ruff/issues/19357
146+
# Must be detected
147+
class ParentD:
148+
def f(self):
149+
print("D")
150+
151+
class ChildD1(ParentD):
152+
def f(self):
153+
if False: __class__ # Python injects __class__ into scope
154+
builtins.super(ChildD1, self).f()
155+
156+
class ChildD2(ParentD):
157+
def f(self):
158+
if False: super # Python injects __class__ into scope
159+
builtins.super(ChildD2, self).f()
160+
161+
class ChildD3(ParentD):
162+
def f(self):
163+
builtins.super(ChildD3, self).f()
164+
super # Python injects __class__ into scope
165+
166+
import builtins as builtins_alias
167+
class ChildD4(ParentD):
168+
def f(self):
169+
builtins_alias.super(ChildD4, self).f()
170+
super # Python injects __class__ into scope
171+
172+
class ChildD5(ParentD):
173+
def f(self):
174+
super = 1
175+
super # Python injects __class__ into scope
176+
builtins.super(ChildD5, self).f()
177+
178+
class ChildD6(ParentD):
179+
def f(self):
180+
super: "Any"
181+
__class__ # Python injects __class__ into scope
182+
builtins.super(ChildD6, self).f()
183+
184+
class ChildD7(ParentD):
185+
def f(self):
186+
def x():
187+
__class__ # Python injects __class__ into scope
188+
builtins.super(ChildD7, self).f()
189+
190+
class ChildD8(ParentD):
191+
def f(self):
192+
def x():
193+
super = 1
194+
super # Python injects __class__ into scope
195+
builtins.super(ChildD8, self).f()
196+
197+
class ChildD9(ParentD):
198+
def f(self):
199+
def x():
200+
__class__ = 1
201+
__class__ # Python injects __class__ into scope
202+
builtins.super(ChildD9, self).f()
203+
204+
class ChildD10(ParentD):
205+
def f(self):
206+
def x():
207+
__class__ = 1
208+
super # Python injects __class__ into scope
209+
builtins.super(ChildD10, self).f()
210+
211+
212+
# Must be ignored
213+
class ParentI:
214+
def f(self):
215+
print("I")
216+
217+
class ChildI1(ParentI):
218+
def f(self):
219+
builtins.super(ChildI1, self).f() # no __class__ in the local scope
220+
221+
222+
class ChildI2(ParentI):
223+
def b(self):
224+
x = __class__
225+
if False: super
226+
227+
def f(self):
228+
self.b()
229+
builtins.super(ChildI2, self).f() # no __class__ in the local scope
230+
231+
class ChildI3(ParentI):
232+
def f(self):
233+
if False: super
234+
def x(_):
235+
builtins.super(ChildI3, self).f() # no __class__ in the local scope
236+
x(None)
237+
238+
class ChildI4(ParentI):
239+
def f(self):
240+
super: "str"
241+
builtins.super(ChildI4, self).f() # no __class__ in the local scope
242+
243+
class ChildI5(ParentI):
244+
def f(self):
245+
super = 1
246+
__class__ = 3
247+
builtins.super(ChildI5, self).f() # no __class__ in the local scope
248+
249+
class ChildI6(ParentI):
250+
def f(self):
251+
__class__ = None
252+
__class__
253+
builtins.super(ChildI6, self).f() # no __class__ in the local scope
254+
255+
class ChildI7(ParentI):
256+
def f(self):
257+
__class__ = None
258+
super
259+
builtins.super(ChildI7, self).f()
260+
261+
class ChildI8(ParentI):
262+
def f(self):
263+
__class__: "Any"
264+
super
265+
builtins.super(ChildI8, self).f()
266+
267+
class ChildI9(ParentI):
268+
def f(self):
269+
class A:
270+
def foo(self):
271+
if False: super
272+
if False: __class__
273+
builtins.super(ChildI9, self).f()

crates/ruff_linter/src/rules/pyupgrade/rules/super_call_with_parameters.rs

Lines changed: 78 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use ruff_diagnostics::Applicability;
22
use ruff_macros::{ViolationMetadata, derive_message_formats};
3+
use ruff_python_ast::visitor::{Visitor, walk_expr, walk_stmt};
34
use ruff_python_ast::{self as ast, Expr, Stmt};
5+
use ruff_python_semantic::SemanticModel;
46
use ruff_text_size::{Ranged, TextSize};
57

68
use crate::checkers::ast::Checker;
@@ -94,14 +96,22 @@ pub(crate) fn super_call_with_parameters(checker: &Checker, call: &ast::ExprCall
9496
};
9597

9698
// Find the enclosing function definition (if any).
97-
let Some(Stmt::FunctionDef(ast::StmtFunctionDef {
98-
parameters: parent_parameters,
99-
..
100-
})) = parents.find(|stmt| stmt.is_function_def_stmt())
99+
let Some(
100+
func_stmt @ Stmt::FunctionDef(ast::StmtFunctionDef {
101+
parameters: parent_parameters,
102+
..
103+
}),
104+
) = parents.find(|stmt| stmt.is_function_def_stmt())
101105
else {
102106
return;
103107
};
104108

109+
if is_builtins_super(checker.semantic(), call)
110+
&& !has_local_dunder_class_var_ref(checker.semantic(), func_stmt)
111+
{
112+
return;
113+
}
114+
105115
// Extract the name of the first argument to the enclosing function.
106116
let Some(parent_arg) = parent_parameters.args.first() else {
107117
return;
@@ -193,3 +203,67 @@ pub(crate) fn super_call_with_parameters(checker: &Checker, call: &ast::ExprCall
193203
fn is_super_call_with_arguments(call: &ast::ExprCall, checker: &Checker) -> bool {
194204
checker.semantic().match_builtin_expr(&call.func, "super") && !call.arguments.is_empty()
195205
}
206+
207+
/// Returns `true` if the function contains load references to `__class__` or `super` without
208+
/// local binding.
209+
///
210+
/// This indicates that the function relies on the implicit `__class__` cell variable created by
211+
/// Python when `super()` is called without arguments, making it unsafe to remove `super()` parameters.
212+
fn has_local_dunder_class_var_ref(semantic: &SemanticModel, func_stmt: &Stmt) -> bool {
213+
if semantic.current_scope().has("__class__") {
214+
return false;
215+
}
216+
217+
let mut finder = ClassCellReferenceFinder::new();
218+
finder.visit_stmt(func_stmt);
219+
220+
finder.found()
221+
}
222+
223+
/// Returns `true` if the call is to the built-in `builtins.super` function.
224+
fn is_builtins_super(semantic: &SemanticModel, call: &ast::ExprCall) -> bool {
225+
semantic
226+
.resolve_qualified_name(&call.func)
227+
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["builtins", "super"]))
228+
}
229+
230+
/// A [`Visitor`] that searches for implicit reference to `__class__` cell,
231+
/// excluding nested class definitions.
232+
#[derive(Debug)]
233+
struct ClassCellReferenceFinder {
234+
has_class_cell: bool,
235+
}
236+
237+
impl ClassCellReferenceFinder {
238+
pub(crate) fn new() -> Self {
239+
ClassCellReferenceFinder {
240+
has_class_cell: false,
241+
}
242+
}
243+
pub(crate) fn found(&self) -> bool {
244+
self.has_class_cell
245+
}
246+
}
247+
248+
impl<'a> Visitor<'a> for ClassCellReferenceFinder {
249+
fn visit_stmt(&mut self, stmt: &'a Stmt) {
250+
match stmt {
251+
Stmt::ClassDef(_) => {}
252+
_ => {
253+
if !self.has_class_cell {
254+
walk_stmt(self, stmt);
255+
}
256+
}
257+
}
258+
}
259+
260+
fn visit_expr(&mut self, expr: &'a Expr) {
261+
if expr.as_name_expr().is_some_and(|name| {
262+
matches!(name.id.as_str(), "super" | "__class__") && name.ctx.is_load()
263+
}) {
264+
self.has_class_cell = true;
265+
return;
266+
}
267+
walk_expr(self, expr);
268+
}
269+
}

0 commit comments

Comments
 (0)