Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Aug 22, 2025

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.

As noted in the variant docs by @AlexWaygood, we don't fully model this behavior, opting always to create the __class__ cell binding in a new ScopeKind::DunderClassCell around each method definition, without checking if any method in the class body actually refers to __class__ or super.

As such, this PR fixes #18442 but not #19357.

Test Plan

Existing tests, plus the tests from #19783, which now pass without any rule-specific code.

Note that we opted not to alter the behavior of F841 here because flagging __class__ in these cases still seems helpful. See the discussion in #20048 (comment) and in the test comments for more information.

@ntBre ntBre added rule Implementing or modifying a lint rule bug Something isn't working and removed rule Implementing or modifying a lint rule labels Aug 22, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

ntBre and others added 8 commits August 22, 2025 15:56
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]>
Co-authored-by: Mikko Leppänen <[email protected]>
these don't change any existing test results, and also don't seem to help with
the F841 failures in the tests from #19783, but they still seem correct and in
line with our other changes
@ntBre ntBre force-pushed the alex-brent/class-cell branch from 0e8b83c to c1604dd Compare August 22, 2025 20:41
@ntBre
Copy link
Contributor Author

ntBre commented Aug 22, 2025

I pulled in the tests from #19783 (and added @mikeleppane as a co-author, thank you!) and the issue with those rules is now resolved, with one additional change to the semantic model to reuse the cell binding for __class__ in add_binding (c1604dd).

I also tried the tests from #19424, but I think these still need some rule-specific code, so I decided to leave that as a separate PR. The changes here should still help, but they aren't sufficient on their own.

Finally, I applied our class_variables_visible change in a couple more places. Those were some of the last references to "__class__" that I found from grepping.

@ntBre ntBre marked this pull request as ready for review August 22, 2025 21:09
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!! Would you be up for trying to apply the same change to ty as well?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@dscorbett
Copy link

This comment is not true:

# The following three cases should technically be allowed because `__class__`
# is a nonlocal variable in the method scope. However, `__class__` isn't easily
# accessible in other methods, so setting it like this is still likely to be an
# error, and we still emit a diagnostic. See https://github.com/astral-sh/ruff/pull/20048#discussion_r2296252395
class A:
    def set_class(self, cls):
        __class__ = cls

__class__ is a local variable there because it is assigned. It would only be nonlocal with a nonlocal __class__ statement.

@ntBre
Copy link
Contributor Author

ntBre commented Aug 25, 2025

Ah I see, thanks. For some reason I thought this should work even without the explicit nonlocal, but I think this resolves my confusion. This case is now fully resolved, not triggering PLE0117 or F841:

class A:
    def set_class(self, cls):
        nonlocal __class__
        __class__ = cls

and these cases are properly flagged because the local __class__ variable is unused:

class A:
    class B:
        def set_class(self, cls):
            __class__ = cls


class A:
    def foo():
        class B:
            print(__class__)
            def set_class(self, cls):
                __class__ = cls

I'll update the first test case with the actual nonlocal statement and also fix the comment.

@dscorbett
Copy link

In the following example, __class__ is a local variable, which breaks the zero-argument super.

class C:
    def __str__(self):
        if False:
            __class__ = C
        return super().__str__()
print(str(C()))

With this PR, will that be handled properly?

@AlexWaygood
Copy link
Member

With this PR, will that be handled properly?

No, that issue will still be outstanding after this PR has merged. (That doesn't mean that we won't necessarily fix it in the future. But while it's a valid bug report, it's probably low priority for us to fix it since it seems unlikely that a human or LLM would ever write that in real code :-)

@ntBre
Copy link
Contributor Author

ntBre commented Aug 25, 2025

I think #19424 will still be necessary to fix that, if that's the same issue. But the local variable should shadow the nonlocal binding from the cell after this PR, so I think it makes it slightly easier to resolve the other issue too, at least.

@ntBre ntBre merged commit bc7274d into main Aug 26, 2025
35 checks passed
@ntBre ntBre deleted the alex-brent/class-cell branch August 26, 2025 13:49
carljm added a commit to leandrobbraga/ruff that referenced this pull request Aug 27, 2025
* main:
  [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (astral-sh#19647)
  [ty] Optimize TDD atom ordering (astral-sh#20098)
  [`airflow`] Extend `AIR311` and `AIR312` rules (astral-sh#20082)
  [ty] Preserve qualifiers when accessing attributes on unions/intersections (astral-sh#20114)
  [ty] Fix the inferred interface of specialized generic protocols (astral-sh#19866)
  [ty] Infer slightly more precise types for comprehensions (astral-sh#20111)
  [ty] Add more tests for protocols (astral-sh#20095)
  [ty] don't eagerly unpack aliases in user-authored unions (astral-sh#20055)
  [`flake8-use-pathlib`] Update links to the table showing the correspondence between `os` and `pathlib` (astral-sh#20103)
  [`flake8-use-pathlib`] Make `PTH100` fix unsafe because it can change behavior (astral-sh#20100)
  [`flake8-use-pathlib`] Delete unused `Rule::OsSymlink` enabled check (astral-sh#20099)
  [ty] Add search paths info to unresolved import diagnostics (astral-sh#20040)
  [`flake8-logging-format`] Add auto-fix for f-string logging calls (`G004`) (astral-sh#19303)
  Add a `ScopeKind` for the `__class__` cell (astral-sh#20048)
  Fix incorrect D413 links in docstrings convention FAQ (astral-sh#20089)
  [ty] Refactor inlay hints structure to use separate parts (astral-sh#20052)
second-ed pushed a commit to second-ed/ruff that referenced this pull request Sep 9, 2025
Summary
--

This PR aims to resolve (or help to resolve) astral-sh#18442 and astral-sh#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 variant docs by @AlexWaygood, we don't fully model this
behavior, opting always to create the `__class__` cell binding in a new
`ScopeKind::DunderClassCell` around each method definition, without
checking if any method in the class body actually refers to `__class__`
or `super`.

As such, this PR fixes astral-sh#18442 but not astral-sh#19357.

Test Plan
--

Existing tests, plus the tests from astral-sh#19783, which now pass without any
rule-specific code.

Note that we opted not to alter the behavior of F841 here because
flagging `__class__` in these cases still seems helpful. See the
discussion in
astral-sh#20048 (comment) and
in the test comments for more information.

---------

Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Mikko Leppänen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

F841 and PLE0117 don’t understand __class__ bindings

4 participants