Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.2k
gh-82129: FixNameError onget_type_hints indataclasses#122232
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
neat!
| tp = 'typing.Any' | ||
| typing = sys.modules.get('typing') | ||
| if typing: | ||
| tp = typing.Any |
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.
Could we use"__import__('typing').Any"? Sinceget_type_hints() callseval(), that should work regardless of whether the dataclass is defined in a place where typing is imported.
I can provide a more robust solution on 3.14 using__annotate__.
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.
Nikita's current solution feels in keeping with whatdataclasses does elsewhere:
Lines 809 to 815 ine968121
| typing=sys.modules.get('typing') | |
| iftyping: | |
| if (_is_classvar(a_type,typing) | |
| or (isinstance(f.type,str) | |
| and_is_type(f.type,cls,typing,typing.ClassVar, | |
| _is_classvar))): | |
| f._field_type=_FIELD_CLASSVAR |
dataclasses in general takes great pains to avoid importingtyping if it's not already imported. I think these daystyping is actually a faster import thandataclasses, but for now I think it's probably best to stick with the existing general philosophy ofdataclasses there.
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.
My solution would not involve importing typing in the dataclasses module.
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 can provide a more robust solution on 3.14 usingannotate.
I am already working on that for 3.14+ 😉
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.
Could we use "import('typing').Any"?
I feel like this might not be a good idea for older versions. Since right nowdataclasses do not importtyping directly, this might cause performance regressions. This PR can be easily backported.
Plus, we have__annotate__ for the future versions.
Looks like a win-win to me.
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.
Note that I am suggesting to put the__import__ in a string, sotyping would only be imported when the annotations are evaluated.
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.
@AlexWaygood it would look surprising, yes, but it would solve the user-facing issue that got reported.
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 agree with@AlexWaygood, this might be a very scary thing to see for annotations without the evaluation 😱
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.
That's fair, but then I don't think we should go with the current solution either. My example below seems like a reasonable scenario for how this could end up getting used in practice: user code does not importtyping but callsmake_dataclass(), later something like cattrs run and tries to get the type hints for the class.
With your PR, this will work only iftyping happened to have been imported at the time the dataclass was created. But that becomes very fragile: it means that working code can break if you refactor your code to reorder imports, or remove animport typing from an unrelated part of the code.
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.
Ok, let's try both:typing.Any iftyping exists, and"__import__('typing').Any" if it doesn't 👍
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 doesn't necessarily fix the issue.
$ cat dc.py import dataclassesX = dataclasses.make_dataclass("X", ['x'])$ ./python.exe Python 3.14.0a0 (heads/issue-82129:0d1081237a8, Jul 24 2024, 06:15:08) [Clang 15.0.0 (clang-1500.3.9.4)] on darwinType "help", "copyright", "credits" or "license" for more information.>>> import dc>>> import typing>>> typing.get_type_hints(dc.X)Traceback (most recent call last): File "<python-input-2>", line 1, in <module> typing.get_type_hints(dc.X) ~~~~~~~~~~~~~~~~~~~~~^^^^^^ File "/Users/jelle/py/cpython/Lib/typing.py", line 2416, in get_type_hints value = _eval_type(value, base_globals, base_locals, base.__type_params__, format=format, owner=obj) File "/Users/jelle/py/cpython/Lib/typing.py", line 477, in _eval_type return evaluate_forward_ref(t, globals=globalns, locals=localns, type_params=type_params, owner=owner, _recursive_guard=recursive_guard, format=format) File "/Users/jelle/py/cpython/Lib/typing.py", line 1069, in evaluate_forward_ref value = forward_ref.evaluate(globals=globals, locals=locals, type_params=type_params, owner=owner) File "/Users/jelle/py/cpython/Lib/annotationlib.py", line 140, in evaluate value = eval(code, globals=globals, locals=locals) File "<string>", line 1, in <module>NameError: name 'typing' is not defined. Did you forget to import 'typing'?>>>When you're done making the requested changes, leave the comment: |
@JelleZijlstra yeap, I even wrote a test-case for that:https://github.com/python/cpython/pull/122232/files#diff-212e368b34eb9b134f87e765787d6d26b747235a358e13da8922f83861c0d676R4129-R4133 |
Second PR in this chain:#122262 |
Friendly ping@JelleZijlstra |
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.
Changing behavior based on whethertyping happens to have been imported when the dataclass is created still feels fragile, and I think there's a risk it causes new problems if we make this change in a bugfix release.
So I think it may be better to leave this bug unsolved for earlier versions. There are many scenarios whereget_type_hints() could raise NameError (e.g., whenif TYPE_CHECKING is used), and callers have to deal with that. In Python 3.14 we'll have a better foundation for fixing this sort of thing, but in older versions it may be safer to stick with existing behavior.
Ok, I agree: there are no clean ways forward. Thanks for everyone's input! |
Uh oh!
There was an error while loading.Please reload this page.
Ok, here's my attempt at solving this.
When you call
typing.get_type_hintsyou still have to import typing.There's a good chance that it will already be imported by something else in your
sys.modules.We already have this hack in other places:
cpython/Lib/dataclasses.py
Lines 809 to 815 ine968121
So, this solution can be used to hide the problem for older versions of python (down to3.12). While we can actually fully solve with
annotationlibin 3.14+I think that this hack + user-space one:
is good enough for now.