add slots to base classes, @add_slots takes bases into account #605
Conversation
facebook-github-bot
added
the
CLA Signed
| mock.visited_a() | ||
| object.__setattr__(node, "a_attr", True) | ||
| def visit_Del(self, node: cst.Del) -> None: | ||
| object.__setattr__(node, "target", mock.visited_a()) |
There was a problem hiding this comment.
I had to change to using Del, because Pass didn't have two fields available to reproduce the original test.
due to the slots, arbitrary fields can no longer be set. Existing fields need to be (ab)used.
| def visit_Pass(self, node: cst.Pass) -> None: | ||
| mock.visit_Pass() | ||
| object.__setattr__(node, "visit_Pass", True) | ||
| def visit_If(self, node: cst.If) -> None: |
There was a problem hiding this comment.
I had to change to using If, because Pass didn't have four fields available to reproduce the original test.
due to the slots, arbitrary fields can no longer be set. Existing fields need to be (ab)used.
|
Wow, I did a rough benchmark: from pympler.asizeof import asizeof
from libcst import parse_module
from pathlib import Path
parsed = parse_module(Path('libcst/_typed_visitor.py').read_text())
print(asizeof(parsed))without patched slots: 15_240_816 This cuts memory usage by a factor 3 :) |
libcst/_nodes/base.py
Outdated
| # pyre-ignore[4]: Attribute `__slots__` of class `CSTNode` | ||
| # has type `typing.Tuple[]` but no type is specified. | ||
| # But we know it's str | ||
| __slots__ = () |
There was a problem hiding this comment.
I think you can remove this pyre-ignore by doing
| # pyre-ignore[4]: Attribute `__slots__` of class `CSTNode` | |
| # has type `typing.Tuple[]` but no type is specified. | |
| # But we know it's str | |
| __slots__ = () | |
| __slots__: Sequence[str] = () |
There was a problem hiding this comment.
I needed an additional ClassVar to prevent it from being picked up as a dataclass field
Codecov Report
@@ Coverage Diff @@
## main #605 +/- ##
==========================================
+ Coverage 94.72% 94.76% +0.04%
==========================================
Files 244 245 +1
Lines 24916 25038 +122
==========================================
+ Hits 23602 23728 +126
+ Misses 1314 1310 -4
Continue to review full report at Codecov.
|
Summary
See #574: many slots were missing in base classes, meaning we didn't get the full memory savings or the ability to block arbitrary attribute setting.
Additionally, I found some slots overlapping where
add_slotsdataclasses inherited from each other. I think this isn't even done right in the stdlib...Some changes were needed to the tests now that arbitrary attributes could not be set. Here I was less sure of the desired outcome.
Test Plan
Future violations can be prevented with slotscheck (which I developed).
See here a recent PR where I added it to the returns library.
I'm of course happy to include in dev/CI plan as you indidated