-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add a ScopeKind for the __class__ cell
#20048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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
0e8b83c to
c1604dd
Compare
|
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 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 |
AlexWaygood
left a comment
There was a problem hiding this 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?
Co-authored-by: Alex Waygood <[email protected]>
See #20048 (comment) for more details. In short, these bindings will be unused, except by fairly arcane methods of retrieving __class__ and it seems more helpful to warn about them
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Co-authored-by: Alex Waygood <[email protected]>
|
This comment is not true:
|
|
Ah I see, thanks. For some reason I thought this should work even without the explicit class A:
def set_class(self, cls):
nonlocal __class__
__class__ = clsand these cases are properly flagged because the local 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__ = clsI'll update the first test case with the actual |
|
In the following example, class C:
def __str__(self):
if False:
__class__ = C
return super().__str__()
print(str(C()))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 :-) |
|
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. |
* 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)
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]>
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,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 newScopeKind::DunderClassCellaround each method definition, without checking if any method in the class body actually refers to__class__orsuper.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.