gh-137226: Fix get_type_hints() on generic TypedDict with stringified annotations - #138953
gh-137226: Fix get_type_hints() on generic TypedDict with stringified annotations#138953JelleZijlstra merged 2 commits into
Conversation
Sorry, something went wrong.
…gified annotations This issue appears specifically for TypedDicts because the TypedDict constructor code converts string annotations to ForwardRef objects, and those are not evaluated properly by the get_type_hints() stack because of other shenanigans with type parameters. This issue does not affect normal generic classes because their annotations are not pre-converted to ForwardRefs. The fix attempts to restore the pre- python#137227 behavior in the narrow scenario where the issue manifests. It mostly makes changes only in the paths accessible from get_type_hints(), ensuring that newer APIs (such as evaluate_forward_ref() and annotationlib) are not affected by get_type_hints()'s past odd choices. This PR does not fix python#138949, an older issue I discovered while playing around with this one; we'll need a separate and perhaps more invasive fix for that, but it should wait until after 3.14.0.
This comment was marked as resolved.
This comment was marked as resolved.
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM. Some small nits/questions but nothing blocking; can always merge this and address this later if @hugovk wants it in ASAP!
Sorry, something went wrong.
|
|
||
| def _eval_type(t, globalns, localns, type_params, *, recursive_guard=frozenset(), | ||
| format=None, owner=None, parent_fwdref=None): | ||
| format=None, owner=None, parent_fwdref=None, prefer_fwd_module=False): |
There was a problem hiding this comment.
Could we add a description of this parameter to the docstring?
Should this default to True? Third-party users of this function may be expecting the legacy get_type_hints behaviour? They're obviously asking for breakage if they're using an undocumented, private implementation detail though. So I don't have a strong opinion here.
Sorry, something went wrong.
There was a problem hiding this comment.
I think the False behavior is more sensible so I want to default to it. I know this function gets used in the wild and we shouldn't go out of our way to break such uses, but at the end of the day it's a private function.
Sorry, something went wrong.
|
Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Sorry, something went wrong.
|
Sorry, @JelleZijlstra, I could not cleanly backport this to 3.14 due to a conflict. cherry_picker 6d6aba252301cdf9d5ae3189629e1e43101dd58f 3.14 |
Sorry, something went wrong.
…h stringified annotations (pythonGH-138953) This issue appears specifically for TypedDicts because the TypedDict constructor code converts string annotations to ForwardRef objects, and those are not evaluated properly by the get_type_hints() stack because of other shenanigans with type parameters. This issue does not affect normal generic classes because their annotations are not pre-converted to ForwardRefs. The fix attempts to restore the pre- pythonGH-137227 behavior in the narrow scenario where the issue manifests. It mostly makes changes only in the paths accessible from get_type_hints(), ensuring that newer APIs (such as evaluate_forward_ref() and annotationlib) are not affected by get_type_hints()'s past odd choices. This PR does not fix issue pythonGH-138949, an older issue I discovered while playing around with this one; we'll need a separate and perhaps more invasive fix for that, but it should wait until after 3.14.0. (cherry picked from commit 6d6aba2) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
GH-138989 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
This issue appears specifically for TypedDicts because the TypedDict constructor
code converts string annotations to ForwardRef objects, and those are not evaluated
properly by the get_type_hints() stack because of other shenanigans with type
parameters.
This issue does not affect normal generic classes because their annotations are not
pre-converted to ForwardRefs.
The fix attempts to restore the pre- #137227 behavior in the narrow scenario where
the issue manifests. It mostly makes changes only in the paths accessible from get_type_hints(),
ensuring that newer APIs (such as evaluate_forward_ref() and annotationlib) are not affected
by get_type_hints()'s past odd choices. This PR does not fix issue #138949, an older issue I
discovered while playing around with this one; we'll need a separate and perhaps more
invasive fix for that, but it should wait until after 3.14.0.