Skip to content

gh-142214: Fix two regressions in dataclasses #142223

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
Merged
JelleZijlstra merged 7 commits into from
Dec 5, 2025
Merged

gh-142214: Fix two regressions in dataclasses #142223

JelleZijlstra merged 7 commits into from
Dec 5, 2025

Conversation

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Dec 3, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Dec 3, 2025
Closed
AlexWaygood
AlexWaygood reviewed Dec 3, 2025
View reviewed changes
sobolevn
sobolevn reviewed Dec 3, 2025
View reviewed changes
@DavidCEllis
Copy link
Contributor

DavidCEllis commented Dec 3, 2025

I am kicking myself on the __annotate__ access. Honestly surprised it didn't trip any existing tests.

It looks like the sqlalchemy field issue is from replacing and subsequently attempting to restore annotations in order to add fields to a dataclass as there's no mechanism for doing so without annotations. The check to fix this is fine, I'm actually surprised I hadn't written this as a comprehension.

Again, sorry for missing these possibilities.

basnijholt
basnijholt approved these changes Dec 3, 2025
View reviewed changes
Copy link

@basnijholt basnijholt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I just locally applied the changes in Lib/dataclasses.py and it fixed all the failing tests on github.com/pipefunc/pipefunc.

@JelleZijlstra JelleZijlstra added the needs backport to 3.14 bugs and security fixes label Dec 3, 2025
@DavidCEllis DavidCEllis mentioned this pull request Dec 3, 2025
Closed
ericvsmith
ericvsmith requested changes Dec 3, 2025
View reviewed changes
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

A small nit, but otherwise looks good to me.

Lib/dataclasses.py
# gh-142214: The annotation may be missing in unusual dynamic cases.
# If so, just skip it.
if k in cls_annotations:
new_annotations[k] = cls_annotations[k]
Copy link
Member

@ericvsmith ericvsmith Dec 3, 2025

Choose a reason for hiding this comment

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

Don't in and [] both do a lookup? Maybe it would be better as:

if ann := cls_annotations.get(k):
    new_annotations[k] = ann

Copy link
Contributor

@DavidCEllis DavidCEllis Dec 3, 2025

Choose a reason for hiding this comment

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

I believe that None is a valid annotation here so this would miss None values?

Copy link
Member Author

@JelleZijlstra JelleZijlstra Dec 3, 2025

Choose a reason for hiding this comment

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

Correct. To save a lookup we could do a try-except KeyError instead. I don't have a preference but I can switch to that if you like.

Copy link
Contributor

@DavidCEllis DavidCEllis Dec 3, 2025

Choose a reason for hiding this comment

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

I think try/except makes sense as in most cases the key should be there. The key not being there should only be in cases where something has modified annotations and removed things after the class has been processed.

Copy link
Member Author

@JelleZijlstra JelleZijlstra Dec 4, 2025

Choose a reason for hiding this comment

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

Done

@bedevere-app
Copy link

bedevere-app bot commented Dec 3, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@hugovk hugovk mentioned this pull request Dec 4, 2025
Closed
@JelleZijlstra
Copy link
Member Author

JelleZijlstra commented Dec 4, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Dec 4, 2025

Thanks for making the requested changes!

@ericvsmith: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from ericvsmith December 4, 2025 19:50
@mweinelt mweinelt mentioned this pull request Dec 5, 2025
Merged
13 tasks
ericvsmith
ericvsmith approved these changes Dec 5, 2025
View reviewed changes
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@JelleZijlstra JelleZijlstra merged commit 53ec7c8 into python:main Dec 5, 2025
51 of 54 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Dec 5, 2025

Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 5, 2025
@JelleZijlstra @miss-islington
pythongh-142214: Fix two regressions in dataclasses (pythonGH-142223)
498e477 
(cherry picked from commit 53ec7c8)

Co-authored-by: Jelle Zijlstra <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Dec 5, 2025

GH-142277 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Dec 5, 2025
@JelleZijlstra JelleZijlstra deleted the fixdatacl branch December 5, 2025 04:06
JelleZijlstra added a commit that referenced this pull request Dec 5, 2025
@miss-islington @JelleZijlstra
[3.14] gh-142214: Fix two regressions in dataclasses (GH-142223) (#14…
654e3c7 
…2277)

gh-142214: Fix two regressions in dataclasses (GH-142223)
(cherry picked from commit 53ec7c8)

Co-authored-by: Jelle Zijlstra <[email protected]>
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@JelleZijlstra @StanFromIreland
pythongh-142214: Fix two regressions in dataclasses (python#142223)
3265f88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn left review comments

@AlexWaygood AlexWaygood AlexWaygood left review comments

@ericvsmith ericvsmith ericvsmith approved these changes

+2 more reviewers

@DavidCEllis DavidCEllis DavidCEllis left review comments

@basnijholt basnijholt basnijholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@JelleZijlstra @DavidCEllis @ericvsmith @sobolevn @basnijholt @AlexWaygood