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

gh-115165: Fix Annotated for immutable types #115213

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

dave-shawley
Copy link
Contributor

@dave-shawley dave-shawley commented Feb 9, 2024

This PR widens the list of ignored exceptions when setting the __orig_class__ attribute on the return value from an annotated callable. Only AttributeError was caught yet instances can raise any Exception from __setattr__. After this PR, it will catch any Exception instead. This change was motivated by the uuid.UUID.__setattr__ implementation raises TypeError.

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks!

I think this is safe to backport to the bugfix versions (3.11 and 3.12).

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! This is really great for a first contribution :)

A few small points below:

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/typing.py Show resolved Hide resolved
Add comment justifying `except Exception`
Renamed test_annotated_callable_returning_immutable to
test_instantiate_immutable and moved it near the other instantiation
tests.
Add a test for generic instantiation that trips the same __setattr__
exception in Annotation.
@dave-shawley
Copy link
Contributor Author

@AlexWaygood and @JelleZijlstra -- thanks for the suggestions. I've addressed them locally. Do you prefer "fixup! ..." commits for auto-squashing or independent commits that will be preserved?

@JelleZijlstra
Copy link
Member

It doesn't really matter, we squash the commits on merge. The only thing we generally ask for is to avoid force-pushing to your branch; that makes review harder.

@AlexWaygood
Copy link
Member

@AlexWaygood and @JelleZijlstra -- thanks for the suggestions. I've addressed them locally. Do you prefer "fixup! ..." commits for auto-squashing or independent commits that will be preserved?

All commits will be squash-merged into a single commit by GitHub when we merge your PR into the main branch, so you don't need to worry about keeping a clean history on your PR branch here. We prefer for contributors to avoid force-pushing where possible, so that we can easily see what changed between commits :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@AlexWaygood AlexWaygood enabled auto-merge (squash) February 9, 2024 21:54
@AlexWaygood AlexWaygood merged commit 5643856 into python:main Feb 9, 2024
32 checks passed
@miss-islington-app
Copy link

Thanks @dave-shawley for the PR, and @AlexWaygood for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 9, 2024
…-115213)

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
(cherry picked from commit 5643856)

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

bedevere-app bot commented Feb 9, 2024

GH-115227 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 Feb 9, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 9, 2024
…-115213)

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
(cherry picked from commit 5643856)

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

bedevere-app bot commented Feb 9, 2024

GH-115228 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 9, 2024
AlexWaygood pushed a commit that referenced this pull request Feb 9, 2024
…) (#115227)

gh-115165: Fix `typing.Annotated` for immutable types (GH-115213)

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
(cherry picked from commit 5643856)

Co-authored-by: dave-shawley <[email protected]>
@dave-shawley dave-shawley deleted the gitlab-115165 branch February 9, 2024 22:38
AlexWaygood pushed a commit that referenced this pull request Feb 9, 2024
…) (#115228)

gh-115165: Fix `typing.Annotated` for immutable types (GH-115213)

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
(cherry picked from commit 5643856)

Co-authored-by: dave-shawley <[email protected]>
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
…15213)

The return value from an annotated callable can raise any exception from
__setattr__ for the `__orig_class__` property.
@aminalaee
Copy link
Contributor

@JelleZijlstra @AlexWaygood Can this be backported to older versions too?

@JelleZijlstra
Copy link
Member

Looks like it was backported to 3.11 and 3.12. Older versions only receive security fixes now, which this isn't.

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

Successfully merging this pull request may close these issues.

4 participants