[`pyupgrade`] Apply `UP008` only when the `__class__` cell exists by IDrokin117 · Pull Request #19424 · astral-sh/ruff

IDrokin117 · 2025-07-18T15:33:09Z

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:

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:

super or __class__ names have been loaded within function scope
__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.

github-actions · 2025-07-18T15:43:59Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

IDrokin117 · 2025-07-24T12:56:36Z

@ntBre Could you please review a PR?

ntBre · 2025-07-24T13:04:47Z

Will do! Sorry, I'm a bit behind on my review queue, but this has been on the list.

ntBre

Thanks, this looks great! Sorry again for the delay.

I just had a few small suggestions.

Thanks for the docs links too. I skimmed over a few of those, and I think you've captured everything I understood from them and from the issue too :)

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

crates/ruff_python_ast/src/helpers.rs

ntBre · 2025-08-07T16:04:19Z

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

+    let has_load_super = finder
+        .names
+        .iter()
+        .any(|expr| expr.id.as_str() == "super" && expr.ctx.is_load());
+    let has_load_dunder_class = finder
+        .names
+        .iter()
+        .any(|expr| expr.id.as_str() == "__class__" && expr.ctx.is_load());


Because these are both any, I think we can just check these conditions as the visitor visits and avoid needing to collect a Vec of expressions. We may even be able to terminate the visitor early in that case.

My intention was to make it simple and not necessarily optimized.
I have reworked it, and now it is implemented according to your recommendation, but I think it is more complex to read. I would appreciate any simplification advice.

BTW, it was intentional to pass conditions due to initialization rather than hardcode them in visit_expr. In my opinion, it provides more flexibility for future changes.

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

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

ntBre · 2025-08-13T18:22:36Z

I just came back to this notification right after reviewing #19783. I somehow hadn't connected these two before, but now I'm wondering if they have the same root cause. I think if we're able to work out a neat, general solution for binding the __class__ cell it might help here too.

@AlexWaygood

Summary -- This PR aims to resolve (or help to resolve) #18442 and #19357 by encoding the CPython semantics around the `__class__` cell in our semantic model. Namely, > __class__ is an implicit closure reference created by the compiler if any methods in a class body refer to either __class__ or super. from the Python [docs](https://docs.python.org/3/reference/datamodel.html#creating-the-class-object). As noted in the code comment by @AlexWaygood, we don't fully model this behavior, opting always to create the `__class__` cell binding in a new `ScopeKind::ClassCell` around each method definition, without checking if any method in the class body actually refers to `__class__` or `super`. As such, this PR may only help with #18442 and not #19357. Test Plan -- Existing tests, plus (TODO) tests from #19783 and #19424, which tackled the issues above on a per-rule basis. Co-authored-by: Alex Waygood <[email protected]>

@AlexWaygood

Summary -- This PR aims to resolve (or help to resolve) #18442 and #19357 by encoding the CPython semantics around the `__class__` cell in our semantic model. Namely, > __class__ is an implicit closure reference created by the compiler if any methods in a class body refer to either __class__ or super. from the Python [docs](https://docs.python.org/3/reference/datamodel.html#creating-the-class-object). As noted in the code comment by @AlexWaygood, we don't fully model this behavior, opting always to create the `__class__` cell binding in a new `ScopeKind::ClassCell` around each method definition, without checking if any method in the class body actually refers to `__class__` or `super`. As such, this PR may only help with #18442 and not #19357. Test Plan -- Existing tests, plus (TODO) tests from #19783 and #19424, which tackled the issues above on a per-rule basis. Co-authored-by: Alex Waygood <[email protected]>

ntBre · 2025-08-26T13:54:46Z

We added a new ScopeKind::DunderClassCell (and BindingKind::DunderClassCell) in #20048. I don't believe that will resolve the issue addressed by this PR, but would you mind seeing if it makes anything easier? I was hoping it might help to avoid the new Visitor, but I think we'll still need some rule-specific changes in any case.

Thanks again for your work here :)

IDrokin117 · 2025-08-26T18:05:38Z

@ntBre Good news! Here are my thoughts after reviewing the new feature.
It seems the __class__ binding is being created in any case now, regardless of whether __class__ and super names are present. Am I right? This is going to be simple and good for some rules, but not for this case. Since the __class__ binding pretends to always exist within a method, it doesn't provide any new information. I mean, if a __class__ cell is available, then builtins.super() has to work. But that's not true since __class__ is not always available. I see two options to resolve the issue:

Left the extra visitor to ensure determination of whether __class__ is really injected at runtime based on specific rules.
Simplify the rule and apply it only if super() is encountered, ignoring builtins.super().

Since I've already implemented tailored __class__ determination, I prefer the first option. What do you think?

ntBre · 2025-08-29T13:34:29Z

Thanks for looking into it and rebasing the PR!

It seems the __class__ binding is being created in any case now, regardless of whether __class__ and super names are present. Am I right?

Yes that's right, we didn't try to detect if __class__ or super was used, so it makes sense that it may not help here.

Left the extra visitor to ensure determination of whether __class__ is really injected at runtime based on specific rules.

Simplify the rule and apply it only if super() is encountered, ignoring builtins.super().

Since I've already implemented tailored __class__ determination, I prefer the first option. What do you think?

Makes sense to me, I'll give this another review soon!

ntBre

Thanks! Just a couple of minor simplification suggestions and one corner case from the docs.

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

Skip UP008 diagnostic for `builtins.super(Class, self)` calls when neither `__class__` nor `super` are referenced locally, preventing incorrect fixes. Issue: astral-sh#19357

`has_local_dunder_class_var_ref` checks if `super` or `__class__` expr.ctx is load and `__class__` var in scope didn't override. Issue: astral-sh#19357

renamed FunctionScopeNameFinder to AnyExpressionFinder moved AnyExpressionFinder to super_call_with_parameters.rs updated test cases and snapshots removed duplicated parents Now AnyExpressionFinder finds any expression by given conditions

ntBre

Thanks!

IDrokin117 marked this pull request as draft July 20, 2025 13:13

IDrokin117 force-pushed the fix/ruff-19357 branch from 01d1724 to a00d546 Compare July 21, 2025 14:48

IDrokin117 marked this pull request as ready for review July 21, 2025 15:01

ntBre self-requested a review July 24, 2025 13:04

ntBre added bug Something isn't working fixes Related to suggested fixes for violations labels Jul 24, 2025

ntBre reviewed Aug 7, 2025

View reviewed changes

IDrokin117 force-pushed the fix/ruff-19357 branch 3 times, most recently from db72ff9 to f1e50af Compare August 9, 2025 20:07

IDrokin117 requested a review from ntBre August 9, 2025 20:08

ntBre mentioned this pull request Aug 22, 2025

Add a ScopeKind for the __class__ cell #20048

Merged

IDrokin117 force-pushed the fix/ruff-19357 branch from f1e50af to db109c8 Compare August 29, 2025 09:32

ntBre reviewed Sep 9, 2025

View reviewed changes

IDrokin117 and others added 4 commits September 9, 2025 19:32

feat(UP008): enhanced diagnostic of builtins.super

9c51fe1

Skip UP008 diagnostic for `builtins.super(Class, self)` calls when neither `__class__` nor `super` are referenced locally, preventing incorrect fixes. Issue: astral-sh#19357

feat(UP008): rework has_local_dunder_class_var_ref func

7c1a748

`has_local_dunder_class_var_ref` checks if `super` or `__class__` expr.ctx is load and `__class__` var in scope didn't override. Issue: astral-sh#19357

feat(up008): update snapshots

9795dc0

IDrokin117 force-pushed the fix/ruff-19357 branch from db109c8 to 27ab45c Compare September 9, 2025 17:58

refactor(UP008): simplify visitor

c394853

IDrokin117 force-pushed the fix/ruff-19357 branch from 27ab45c to c394853 Compare September 9, 2025 18:00

IDrokin117 requested a review from ntBre September 9, 2025 18:09

ntBre approved these changes Sep 9, 2025

View reviewed changes

ntBre changed the title ~~[pyupgrade] Apply UP008 only when the __class__ cell exists (UP008)~~ [pyupgrade] Apply UP008 only when the __class__ cell exists (UP008) Sep 9, 2025

ntBre changed the title ~~[pyupgrade] Apply UP008 only when the __class__ cell exists (UP008)~~ [pyupgrade] Apply UP008 only when the __class__ cell exists Sep 9, 2025

ntBre merged commit 54df73c into astral-sh:main Sep 9, 2025
35 checks passed

BrewTestBot mentioned this pull request Sep 10, 2025

ruff 0.13.0 Homebrew/homebrew-core#237206

Merged

Comments

Conversation

IDrokin117 commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

IDrokin117 commented Jul 24, 2025

Uh oh!

ntBre commented Jul 24, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntBre Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

IDrokin117 Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntBre commented Aug 13, 2025

Uh oh!

ntBre commented Aug 26, 2025

Uh oh!

IDrokin117 commented Aug 26, 2025

Uh oh!

ntBre commented Aug 29, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IDrokin117 commented Jul 18, 2025 •

edited

Loading

github-actions bot commented Jul 18, 2025 •

edited

Loading

`ruff-ecosystem` results