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.

Sign up for GitHub

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

Jump to bottom

gh-118033: Fix __weakref__ not set for generic dataclasses #118099

Merged
merged 6 commits into from
May 9, 2024
Merged

gh-118033: Fix __weakref__ not set for generic dataclasses #118099

merged 6 commits into from
May 9, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Apr 19, 2024 •
edited by bedevere-app bot
Loading

Looks like that classes with __weakrefoffset__ == 0 does not need to add __weakref__ slot to existing super ones.

@sobolevn sobolevn requested a review from ericvsmith as a code owner April 19, 2024 12:04
@bedevere-app bedevere-app bot mentioned this pull request Apr 19, 2024
Closed
@sobolevn
Copy link
Member Author

sobolevn commented Apr 19, 2024 •
edited
Loading

No, this is not correct, we also need to check __dictoffset__ as well. For __dict__ case.

@sobolevn
Copy link
Member Author

Ubuntu failures are not related, there are some network problems right now

carljm
carljm approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

I think this fix is correct; some suggestions and a question.

Lib/dataclasses.py Outdated Show resolved Hide resolved
Lib/dataclasses.py Outdated Show resolved Hide resolved
Lib/dataclasses.py Outdated Show resolved Hide resolved
Lib/dataclasses.py Show resolved Hide resolved
@sobolevn sobolevn requested a review from carljm May 4, 2024 09:05
carljm
carljm approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Sorry I didn't see the notification sooner that you requested another review!

I still think the best version of this might look at __weakrefoffset__ and __dictrefoffset__ always to decide on those two slots, but I agree that's an edge case and can be handled separately; this is still a clear improvement and fixes the original bug.

@sobolevn sobolevn added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels May 9, 2024
@sobolevn sobolevn enabled auto-merge (squash) May 9, 2024 08:09
@sobolevn sobolevn disabled auto-merge May 9, 2024 08:35
@sobolevn sobolevn enabled auto-merge (squash) May 9, 2024 08:35
@sobolevn sobolevn merged commit fa9b9cb into python:main May 9, 2024
33 checks passed
@miss-islington-app
Copy link

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2024
@sobolevn @miss-islington
pythongh-118033: Fix __weakref__ not set for generic dataclasses (p…
9725fb0 
…ythonGH-118099)

(cherry picked from commit fa9b9cb)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 9, 2024
@sobolevn @miss-islington
pythongh-118033: Fix __weakref__ not set for generic dataclasses (p…
cf4a239 
…ythonGH-118099)

(cherry picked from commit fa9b9cb)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@bedevere-app
Copy link

bedevere-app bot commented May 9, 2024

GH-118821 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 9, 2024
@bedevere-app
Copy link

bedevere-app bot commented May 9, 2024

GH-118822 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label May 9, 2024
sobolevn added a commit that referenced this pull request May 9, 2024
@miss-islington @sobolevn
[3.13] gh-118033: Fix __weakref__ not set for generic dataclasses (G…
632682c 
…H-118099) (#118821)

gh-118033: Fix `__weakref__` not set for generic dataclasses (GH-118099)
(cherry picked from commit fa9b9cb)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit that referenced this pull request May 9, 2024
@miss-islington @sobolevn
[3.12] gh-118033: Fix __weakref__ not set for generic dataclasses (G…
e953956 
…H-118099) (#118822)

gh-118033: Fix `__weakref__` not set for generic dataclasses (GH-118099)
(cherry picked from commit fa9b9cb)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@sobolevn sobolevn mentioned this pull request May 9, 2024
Open
@JelleZijlstra JelleZijlstra mentioned this pull request May 28, 2024
Closed
@sobolevn sobolevn mentioned this pull request Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carljm carljm carljm approved these changes

@ericvsmith ericvsmith Awaiting requested review from ericvsmith ericvsmith is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sobolevn @carljm