Skip to content
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

__slots__ = () missing in many base classes? #1144

Closed
ariebovenberg opened this issue Nov 25, 2021 · 5 comments · Fixed by #1147
Closed

__slots__ = () missing in many base classes? #1144

ariebovenberg opened this issue Nov 25, 2021 · 5 comments · Fixed by #1147

Comments

@ariebovenberg
Copy link
Contributor

ariebovenberg commented Nov 25, 2021

related: #416

It seems an empty __slots__ is forgotten in most base classes (like Immutable, Equable, etc.), greatly reducing the advantage (i.e. memory savings) of defining __slots__ in their subclasses (e.g. Result, Maybe). Unless I'm missing something, this is not intended. In that case I will gladly submit a PR fixing this.

Example

>>> from returns.result import Success  # Result defines __slots__
>>> a = Success(4)
>>> a.__dict__  # this should not be present, but it is inherited anyway from base classes without __slots__
{}
>>> object.__setattr__(a, 'foo', 9)  # We can technically set any attribute because there is a `__dict__`
>>> a.foo
9
>>> a.__dict__
{'foo': 9}

Expected behavior of slots classes

>>> class Base:
...  __slots__ = ()  # necessary!
>>> class A(Base):
...  __slots__ = ('foo', )
...
>>> a = A()
>>> a.foo = 4
>>> a.bar = 9
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute 'bar'
>>> a.__dict__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute '__dict__'
@ariebovenberg ariebovenberg changed the title __slots__ == () missing in many base classes? __slots__ = () missing in many base classes? Nov 25, 2021
@sobolevn
Copy link
Member

Yes, my bad. I forgot that I need empty __slots__. PRs are welcome! 👍

@ariebovenberg
Copy link
Contributor Author

ariebovenberg commented Nov 25, 2021

@sobolevn it seems the is_error_handled helper of the pytest plugin patches success/failure containers with an extra _error_handled attribute which is not used outside of tests.

My suggestion would be to add this property to __slots__. How do you feel about adding test-specific behavior to non-test code?

The other alternatives I can think of to avoid this extra __slots__ field are even worse:

  • In the pytest plugin, do a full patch of the classes, substituting them completely with ones with more slots. Not sure if this would even work, but even so it'd be hacky as hell.
  • set __slots__ to include the extra property only if pytest module can be found. Also quite hacky.
  • abandon slots for most containers
  • abandon this pytest plugin helper

@ariebovenberg
Copy link
Contributor Author

ariebovenberg commented Nov 25, 2021

🤔 One other option would be to create a separate (<object id>, <object>): <is error handled> mapping in the test plugin. That'd avoid the extra attribute but we'd have to be careful to delete entries afterwards to avoid leaking memory.

Thoughts?

@sobolevn
Copy link
Member

sobolevn commented Nov 25, 2021

My suggestion would be to add this property to slots. How do you feel about adding test-specific behavior to non-test code?

Or we can create a runtime subclass of a container with this new slot 🤔
Something like type('_Pytest' + SomeType.__name__, (SomeType,), {'__slots__': ...}).

One other option would be to create a separate (<object id>, <object>): <is error handled> mapping in the test plugin

This also seems like a good idea.

@ariebovenberg
Copy link
Contributor Author

I'll try the second approach first, as it should result in 'least surprise' for the user. 🤞 let's see if it works 👀

sobolevn pushed a commit that referenced this issue Nov 28, 2021
…1147)

* add missing __slots__ to containers and their base classes (#1144)

* Fix flake8 issue
@ariebovenberg ariebovenberg mentioned this issue Jan 14, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants