-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
gh-137530: generate an __annotate__ function for dataclasses __init__ #137711
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
base: main
Are you sure you want to change the base?
Conversation
needed for _add_slots
Lib/dataclasses.py
Outdated
elif isinstance(v, annotationlib.ForwardRef): | ||
string_annos[k] = v.evaluate(format=annotationlib.Format.STRING) | ||
else: | ||
string_annos[k] = annotationlib.type_repr(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit unfortunate because we could get better STRING annotations by calling with the STRING format on the original class (and its base classes).
An approach to do this would be to make __init__.__annotate__
delegate to calling get_annotations()
on the class itself, and on any base classes that are contributing fields, with the appropriate format, then processing the result to add "return": None
and filter out any fields that don't correspond to __init__
parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about that, but wasn't certain about the extra complexity, but I've since noticed that if there's a hint that contains a ForwardRef (like list[undefined]
) the result leaks all the ForwardRef details.
I've added a test for that example to demonstrate why this is necessary.
I've taken a slightly different approach, in that I'm gathering all inherited annotations and using them to update the values from the fields. The original source annotation logic this replaces wasn't __init__
specific so I've tried to keep the __annotate__
generator non-specific too.
There are additional commits as this change meant that __annotate__
now (always) has a reference to the original class which broke #135228 - this now also includes an additional test and fix for the issue I brought up there as I already needed to update the fields for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also unintentionally discovered that the __annotate__
functions CPython generates for methods have an incorrect __qualname__
while fixing the __qualname__
for the function generated here.
The possibility to replace only occurs in _add_slots.
On doing some other work, I think the logic for Problem for both: from annotationlib import get_annotations
from dataclasses import dataclass
@dataclass
class Example:
a: list[defined_late]
defined_late = int
print(get_annotations(Example.__init__))
For |
Ah, while this is resolvable for I'm leaning towards making |
Also split out the single large test into multiple smaller tests
Thoughts:
Edit: I realised changing the signature would make |
Use `__class__` as the first argument name so `_update_func_cell_for__class__` can update it.
Ok, I'm going to try to leave this alone for some review. The only functional1 thing I know I'm not completely happy with is this example from the tests2: from dataclasses import dataclass, field
from annotationlib import get_annotations
@dataclass
class F:
not_in_init: list[undefined] = field(init=False, default=None)
in_init: int
annos = annotationlib.get_annotations(F.__init__) # NameError I don't have a good approach for this. I don't think you can use Footnotes |
This is one approach to resolving #137530
It generates a new
__annotate__
function to attach to the__init__
method and removes the in-line annotations that are now unused.It's possible to keep the original source annotations if necessary, but they provide a second possibly incorrect source of the information and they will probably also cause additional issues with #135228 as they may keep the original class around when ForwardRef values are generated.
The generated
__annotate__
will also keep these references and need to be regenerated. I can extend this or follow up to handle these references but I didn't want to do too much in one PR.