| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
needed for _add_slots
| 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.
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 a problem hiding this comment.
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 a problem hiding this comment.
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.
The possibility to replace only occurs in _add_slots.
|
On doing some other work, I think the logic for STRING is also needed for FORWARDREF. I'm not sure how to resolve VALUE. 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__)){'a': list[ForwardRef('defined_late', is_class=True, owner=<class '__main__.Example'>)], 'return': None}
For Format.FORWARDREF this is resolvable in the same way as STRING. However for VALUE it doesn't quite work out to do the same thing, as the annotations on the class could contain a name that doesn't exist yet - but if init=False that name may not be required for the __init__ annotations. |
Sorry, something went wrong.
|
Ah, while this is resolvable for FORWARDREF - it requires VALUE to fail or get_annotations will use VALUE instead anyway. I'm leaning towards making VALUE fail if any of the class annotations fail with VALUE. I can't currently think of a clean way otherwise to both make FORWARDREF evaluate as far as possible and not leak ForwardRef values into VALUE annotations. |
Sorry, something went wrong.
Also split out the single large test into multiple smaller tests
|
Thoughts:
Edit: I realised changing the signature would make _add_slots simpler and is probably worthwhile. |
Sorry, something went wrong.
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 FORWARDREF annotations as they are without potentially leaking actual ForwardRefs? Footnotes |
Sorry, something went wrong.
|
The ideal solution I would like for this would require an additional feature from annotationlib to provide completely unevaluated references. Then a solution more like the original version of this PR could work. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good, except for the small suggestions above.
Sorry, something went wrong.
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
Sorry, @DavidCEllis and @encukou, I could not cleanly backport this to 3.14 due to a conflict. cherry_picker 12837c63635559873a5abddf511d38456d69617b 3.14 |
Sorry, something went wrong.
…sses __init__ (pythonGH-137711) (cherry picked from commit 12837c6) Co-authored-by: David Ellis <ducksual@gmail.com>
|
GH-141352 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
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.
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.
| Back | FazBrowse Home | New Git URL |
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.