GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-137530: generate an __annotate__ function for dataclasses __init__ by DavidCEllis · Pull Request #137711 · python/cpython Conversation
|
| 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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ah, while this is resolvable for I'm leaning towards making |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Also split out the single large test into multiple smaller tests
There was an error while loading. Please reload this page.
|
Thoughts:
Edit: I realised changing the signature would make |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The ideal solution I would like for this would require an additional feature from Then a solution more like the original version of this PR could work. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for the small suggestions above.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
You'll also need a NEWS entry. Since we're planning to backport to 3.14 to fix the release blocker, I think it's also worth mentioning in What's New as this will be a fairly big change in 3.14.1. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks like the What's New entry all that's left here. @DavidCEllis, do you want to write one? Or I can do it to help get this in. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm happy for you to do it. It's not something I've needed to do before so I don't actually know where it would need to go. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It would go here. For the backport, there'd be a “Notable changes” section at the end, like for 3.13. @JelleZijlstra ... um, are you sure this is worth it? From an outside point of view this looks like a straightforward regression fix. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ping @JelleZijlstra I plan to merge Monday or so if I don't hear back, to give enough time for a backport. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks @DavidCEllis for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, @DavidCEllis and @encukou, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…sses __init__ (pythonGH-137711) (cherry picked from commit 12837c6) Co-authored-by: David Ellis <[email protected]>
|
GH-141352 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry for not responding here earlier, haven't been paying enough attention to GitHub.
Fair enough, my thinking was that this significantly changes the annotations people will see if they introspect dataclasses init functions. But our What's New documents don't seem to even have sections for changes in bugfix releases recently, so maybe it's not worth it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…_init__ (GH-137711) (#141352) (cherry picked from commit 12837c6) Co-authored-by: David Ellis <[email protected]>
They do, at the end -- for example, https://docs.python.org/3.12/whatsnew/3.12.html#notable-changes-in-3-12-4 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.