Skip to content

gh-90562: Update __class__ in function closures for dataclasses with slots=True - #104038

Closed
wookie184 wants to merge 1 commit into
python:mainfrom
wookie184:fix-dataclass-slots-super
Closed

gh-90562: Update __class__ in function closures for dataclasses with slots=True#104038
wookie184 wants to merge 1 commit into
python:mainfrom
wookie184:fix-dataclass-slots-super

Conversation

@wookie184

@wookie184wookie184 commented May 1, 2023

Copy link
Copy Markdown
Contributor

Opened as a draft as I haven't updated docs or added news yet.

This is similar to how attrs does it, which was mentioned in the issue. https://github.com/python-attrs/attrs/blob/7c44cfb3c196f547d351af44e411b43f65819714/src/attr/_make.py#L876-L897.

As well as attrs functionality, this also supports:

  • Functions decorated with decorators made with functools.wraps
  • Properties without getters

There are some cases that are not supported:

  • Functions decorated with decorators that don't use functools.wraps (or more precisely, don't have a __wrapped__ attribute which allows us to access the original function defined in the class).
  • Any other reason that the function is no longer in the class dictionary, any examples of that I can think of are pretty obscure though.

Note that since the __class__ cell is shared between functions, these unsupported methods will work if there is one occurence of a supported way of doing things. This is a bit of a gotcha, but I think if it's documented it should be acceptable.

I made an attempt at getting an idea of what the effect of this on performance will be on existing code

defmake_dataclass():
@dataclass(slots=True)classFoo:
a: intb: intc: intd: inte: intf: intg: intdefh(self): ...
defi(self): ...
defj(self): ...
defk(self): ...
defl(self): ...
defm(self): ...
defn(self): ...
defo(self): ...
defp(self): ...
defq(self): ...
print(timeit.repeat(make_dataclass, number=1000, repeat=3))

Before: [0.9140953719997924, 0.9357239580003807, 0.9163554130000193]
After: [0.9437777250004729, 0.9911788059998798, 0.9630459829995743]
There is a difference, but I think it should be acceptable given it only affects dataclass creation.

@wookie184

Copy link
Copy Markdown
ContributorAuthor

I'll close this, as it can still serve as a POC for the linked issue, but there's nothing further to do on this PR right now.

@ericvsmith

Copy link
Copy Markdown
Member

Thanks, @wookie184. I'm going to use this basic approach in #90562. Although I did that implementation before I noticed this PR, I am going to grab some of your tests and credit you.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting reviewinterpreter-core(Objects, Python, Grammar, and Parser dirs)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@wookie184@ericvsmith@arhadthedev@bedevere-bot