Skip to content

Add __slots__ to all types and nodes #11197

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

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Add __slots__ to all types and nodes #11197

merged 2 commits into from
Oct 1, 2021

Conversation

sobolevn
Copy link
Member

Quick review of changes:

  1. I had to move all class-level values into self. definitions, because class attributes and incompatible with __slots__. This seems like a good idea for me, because there was no reason for them to be class-level. I can even imagine several bugs based on over-use of class-level attributes mutation. It might take more memory in some cases though.
  2. I've added several TODO items to op: str, it seems like Enum would be a better fit there

Closes #11195

@97littleleaf11
Copy link
Collaborator

Personally speaking I like this modification!

@emmatyping
Copy link
Member

Would you be able to provide some benchmarks? You might try running the daemon and use the --perf-stats-file flag and have mypy check itself (or any other larger code base you have around)

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 27, 2021

It might take more memory in some cases though.

__slots__ are ignored in release builds (which are compiled using mypyc), so they shouldn't have any (measurable) performance or memory use impact. I think that their only benefits in most use cases are these:

  1. catch invalid attribute assignments when doing local dev/test without having to run mypy (these would be caught in CI at the latest, though)
  2. makes it impossible to have a mutable initializer.

I think that the main drawbacks are these:

  1. some extra verbosity
  2. there's an extra thing that needs to be updated when adding/deleting an attribute
  3. class-level defaults are not supported
  4. __slots__ is a somewhat obscure feature and some contributors might not know about it

I don't have a strong opinion, but catching invalid attribute assignments is somewhat nice during development and can save some time when debugging (but pretty rarely). What do others think?

@sobolevn
Copy link
Member Author

Since now mypy supports __slots__ check after #10801, I think that this might be very useful during development.

I think that the main drawbacks are these:
some extra verbosity
there's an extra thing that needs to be updated when adding/deleting an attribute

Totally.

class-level defaults are not supported

I consider this as a benefit, actually 🙂
Class level defaults can be tricky: user might accidentally change the default of instances by changing it for a type. This problem is quite common. I am not sure what mypyc does in this case, but it can surely happen during development.

slots is a somewhat obscure feature and some contributors might not know about it

I hope more people will learn about this feature! Moreover, #10801 is now a thing. I personally love it! And @attr.s(slots=True) is my goto-method to create new classes 🙂

I personally think it was more confusing for some types to have __slots__ and for some types to not have them.

@sobolevn
Copy link
Member Author

Would you be able to provide some benchmarks?

@ethanhs do you mean https://github.com/python/mypy/blob/3bdef9fe6d401f3d2e0cacf4964bd315550c3394/mypyc/test-data/run-bench.test or do you have any other specific benchmark in mind?

@97littleleaf11
Copy link
Collaborator

It's easier to understand code structure with __slots__ for new contributors like me.

@emmatyping
Copy link
Member

do you mean https://github.com/python/mypy/blob/3bdef9fe6d401f3d2e0cacf4964bd315550c3394/mypyc/test-data/run-bench.test or do you have any other specific benchmark in mind?

I was thinking you could just time running mypy on itself, but I didn't realize mypyc doesn't use slots, so it probably doesn't matter.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks like nobody is against this, so lets do it!

Using slots elsewhere may not be worth it, though, since mypyc basically gives all compiled classes an implicit __slots__.

@JukkaL JukkaL merged commit 7f89cf8 into python:master Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not all types and nodes have __slots__
4 participants