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

Extra slots for slotted classes with inheritance #714

Closed
Drino opened this issue Nov 5, 2020 · 5 comments
Closed

Extra slots for slotted classes with inheritance #714

Drino opened this issue Nov 5, 2020 · 5 comments
Milestone

Comments

@Drino
Copy link
Contributor

Drino commented Nov 5, 2020

It seems that attrs creates a slot for every attr.ib() defined in class, even if parent class already has a slot for it:

@attr.dataclass(slots=True) 
class C1: 
    a: int 
    b: int 


@attr.dataclass(slots=True) 
class C2(C1): 
    a: int 
    c: int 

@attr.dataclass(slots=True) 
class C3: 
    b: int 
    a: int 
    c: int

import sys
print(sys.getsizeof(C2(1, 2, 3)))  # 72
print(sys.getsizeof(C3(1, 2, 3)))  # 64

print(C1.__slots__)  # ('a', 'b', '__weakref__')
print(C2.__slots__)  # ('a', 'c')
print(C3.__slots__)  # ('b', 'a', 'c', '__weakref__')
print(hasattr(C2(1, 2, 3), '__dict__'))  # False

If parent class is a dict-class, everything works fine, except slots tuple doesn't contain inherited attribs:

@attr.dataclass
class C1:
    a: int
    b: int

@attr.dataclass(slots=True) 
class C2(C1): 
    a: int 
    c: int 

print(sys.getsizeof(C2(1, 2, 3)))  # 64, seems that it's `a`, `c` and `__dict__`
print(C2.__slots__) # ('a', 'c')
print(hasattr(C2(1, 2, 3), '__dict__'))  # True
c = C2(1, 2, 3)
c.missing = 42  # Works fine, seems like C2 here is a __dict__-class

UPD: I've checked the docs on slotted classes. If they are inherited from a __dict__-class they wouldn't be a "normal" slotted class, so this is basically expected behaviour.

@Drino Drino changed the title Extra slots for slotted classes Extra slots for slotted classes with inheritance Nov 5, 2020
@hynek hynek modified the milestones: 20.3.0, 20.4.0 Nov 5, 2020
@Drino
Copy link
Contributor Author

Drino commented Nov 13, 2020

Trying to fix this in #718

It came to me as a surprise, that slots=True creates slots only for own attributes when inherited from __dict__ attr-class (there is even a test for it). Is it actually desired behaviour?

@hynek
Copy link
Member

hynek commented Nov 14, 2020

IIRC you don’t save mem by slotting inherited arguments but @Tinche might bring some light.

Unrelatedly, am I missing something or didn’t you add a test that your change actually does what it’s supposed to do?

@Drino
Copy link
Contributor Author

Drino commented Nov 14, 2020

You wouldn't save mem (as you are inheriting slots-class from dict-class, this it'll have a __dict__) but would get a faster attribute access. But slotting only self-attribs sounds like a reasonable decision.

For the test - I've altered test named test_inheritance_from_slots so a slotted child there re-defines a field of its slotted parent. I'll create a separate one.

@Drino
Copy link
Contributor Author

Drino commented Nov 14, 2020

I've actually missed a broken compatibility in case of inheriting a slotted class from a dict-class that inherits from a slotted class and overrides member_descriptor for slot:

@attr.s(slots=True)
class Parent:
    x = attr.ib()


@attr.s
class Child(Parent):
    @cached_property  # So it can be deleted and re-used
    def x(self): 
        ...

@attr.s(slots=True)
class Grandchild(Child):
     # Currently attrs adds `x` to `__slots__`, so `Grandchild.x` is a `member_descriptor`, but after my PR it would be a `cached_property`
    x = attr.ib()

Though I don't think that any sane person is actually doing things like that, I've fixed it in the PR.

@hynek
Copy link
Member

hynek commented Jan 27, 2021

Eh since #718 has been merged, I consider this fixed?

@hynek hynek closed this as completed Jan 27, 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

No branches or pull requests

2 participants