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 <Alex.Waygood@Gmail.com>
Co-authored-by: Mikko Leppänen <mleppan23@gmail.com>
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.
This is great!! Would you be up for trying to apply the same change to ty as well?
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
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 <Alex.Waygood@Gmail.com> Co-authored-by: Mikko Leppänen <mleppan23@gmail.com>
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.