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

dataclass should use inspect.get_annotations instead of examining cls.__dict__ #97799

Closed
larryhastings opened this issue Oct 3, 2022 · 5 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

dataclass needs to examine the annotations of class objects it decorates. Due to __annotations__ being badly designed, it currently pulls the __annotations__ out of the class dict (cls.__dict__.get('__annotations__')). While this works today, this wouldn't work properly for classes defiend in modules using from __future__ import co_annotations if PEP 649 is accepted. It's also not recommended best practices for working with annotations. (Which, admittedly, I wrote.)

Best practices call for using inspect.get_annotations to get the annotations on any object. This does exactly what dataclass wants; it doesn't inherit base class annotations if no child class defines annotations, and it always returns a dict. (Empty, if appropriate.)

dataclass has in the past resisted using inspect.get_annotations because it didn't want to incur the runtime cost of importing inspect. However, as of this time in CPython trunk, inspect is always imported during startup, and dataclass is already importing it anyway. It's time dataclass changed to best practices here.

@larryhastings larryhastings added the type-feature A feature request or enhancement label Oct 3, 2022
@larryhastings larryhastings self-assigned this Oct 3, 2022
@larryhastings
Copy link
Contributor Author

@ericvsmith Tagging you so you see this. Working on a PR.

larryhastings added a commit to larryhastings/cpython that referenced this issue Oct 3, 2022
dataclass used to get the annotations on a class object using
cls.__dict__.get('__annotations__').  Now that it always imports
inspect, it can use inspect.get_annotations, which is modern
best practice for coping with annotations.
larryhastings added a commit that referenced this issue Oct 3, 2022
dataclass used to get the annotations on a class object using
cls.__dict__.get('__annotations__').  Now that it always imports
inspect, it can use inspect.get_annotations, which is modern
best practice for coping with annotations.
@AlexWaygood
Copy link
Member

Closing, as I think this is fixed. (Feel free to reopen if there's more still to do!)

@peku33
Copy link

peku33 commented Jan 10, 2023

Will this fix #83623 ?

@carljm
Copy link
Member

carljm commented Jan 10, 2023

Will this fix #83623 ?

No. inspect.get_annotations() does not eval string annotations by default, and #97800 does not pass eval_str=True. And IMO it shouldn't, because that is likely to trigger cycles or failures to eval in currently-working code. (If you want to discuss that any further, please direct it to #83623, not here.)

@ericvsmith
Copy link
Member

I agree with @carljm. In addition, I don't think there should be changes made to how dataclasses uses annotations until PEP 649 is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

5 participants