-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
bpo-33453: Handle string type annotations in dataclasses. #6768
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
bpo-33453: Handle string type annotations in dataclasses. #6768
Conversation
Lib/dataclasses.py
Outdated
| # for example. So, make a best effort to see if this is a | ||
| # ClassVar or InitVar. | ||
| # For the complete discussion, see https://bugs.python.org/issue33453 | ||
| if isinstance(f.type, str) and f.type.startswith(_CLASSVAR_PREFIXES): |
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.
Can't we instead just duplicate simplified logic from get_type_hints() here? It is essentially just a wrapper around eval() and a little violation of DRY looks better than using this hack.
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've been researching doing just that. I think @ambv's point is that it introduces a performance hit due to calling eval on every field, while the point of string annotations is to remove a performance hit. But I'll let him chime in.
While looking in to this, I get a problem I don't understand. With this code, taken from the typing docs, and without dataclasses:
#from __future__ import annotations
from typing import ClassVar, Dict, get_type_hints
class Starship:
stats: ClassVar[Dict[str, int]] = {}
print(get_type_hints(Starship))
I get this output: {'stats': typing.ClassVar[typing.Dict[str, int]]}.
But, uncomment the __future__ statement and I get a traceback ending in:
File "/Users/eric/local/python/cpython/Lib/typing.py", line 127, in _type_check
raise TypeError(f"{arg} is not valid as type argument")
TypeError: typing.ClassVar[typing.Dict[str, int]] is not valid as type argument
Am I misunderstanding how get_type_hints() works? In this particular case, I would expect the same output with or without the __future__ statement.
In addition to the performance issue, in the following case (without a __future__ statement and without dataclasses), I get an error on get_type_hints() because C is undefined when get_type_hints() is called. This is python/typing#508. Notice that where get_type_hints() is called in this example is exactly where @dataclass is going to run and would need to call the stripped down get_type_hints().
class A:
a: Dict[int, 'C']
print(get_type_hints(A))
class C: pass
-------------
...
File "/Users/eric/local/python/cpython/Lib/typing.py", line 490, in _evaluate
eval(self.__forward_code__, globalns, localns),
File "<string>", line 1, in <module>
NameError: name 'C' is not defined
There's a similar problem with from __future__ import annotations and changing 'C' to C, a problem string annotations was suppose to solve.
As long as this forward reference problem exists, I can't see how we can use even a stripped down get_type_hints().
Crazy idea: maybe look at the string annotation and use everything before the first [ and eval() that? That would work with simple uses of ClassVar[t], but there are plenty of ways that fails, too, like:
cvs = [ClassVar[int]]
class C:
a: cvs[0]
I'm going to move this conversation back to bpo at https://bugs.python.org/issue33453, since this is no longer just about a code review. I'll post something 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.
I am actually not sure the performance penalty will be so dramatic, we can run simple benchmarks to check it. Anyway, I could imagine several other libs that are using type annotations for runtime introspection will be affected by PEP 563 so I think fixing python/typing#508 and python/typing#505 is higher priority. We can then discuss more tomorrow what to do with dataclasses in particular.
so it can be reused in the string annotation case, too.
ilevkivskyi
left a comment
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.
Thanks for fixing this! I have mostly just formatting suggestions.
| def _is_classvar(a_type, typing): | ||
| if typing: | ||
| # This test uses a typing internal class, but it's the best | ||
| # way to test if this is a ClassVar. |
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.
Remove one extra space before "way to ..." (there are two instead of one).
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 like my extra spaces to continue the same paragraph. But I agree they should probably go away. I'll do that systematically in a separate checkout.
Lib/dataclasses.py
Outdated
| and a_type.__origin__ is typing.ClassVar)) | ||
|
|
||
|
|
||
|
|
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.
Maybe two empty lines instead of three?
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.
Fixed
Lib/dataclasses.py
Outdated
| # looking in sys.modules (again!), and seems like a waste since | ||
| # the caller already knows a_module. | ||
|
|
||
| # typename is a string type annotation |
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 think this comment is obsolete, the arg name is now "annotation".
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.
Fixed
Lib/dataclasses.py
Outdated
| # typename is a string type annotation | ||
| # cls is the class that this annotation was found in | ||
| # a_module is the module we want to match | ||
| # a_type is the type in that module we want to match |
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.
Maybe also add a similar comment about "is_type_predicate"?
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.
Fixed
| # cls is the class that this annotation was found in | ||
| # a_module is the module we want to match | ||
| # a_type is the type in that module we want to match | ||
|
|
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 would convert the previous paragraph to docstring.
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'm not a big fan on docstrings for internal functions, so I think I'll leave this as-is.
| 'typing.ClassVar [ str]', | ||
|
|
||
| # Not syntactically valid, but these will | ||
| # be treated as ClassVars. |
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.
Extra space in comment also here.
| # Not syntactically valid, but these will | ||
| # be treated as ClassVars. | ||
| 'typing.ClassVar.[int]', | ||
| 'typing.ClassVar+', |
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 am not an expert in re, but is it possible to adjust regex so that these two will not match? If it is not easy, then ignore this comment.
|
|
||
| def test_initvar(self): | ||
| # These tests assume that both "import dataclasses" and "from | ||
| # dataclasses import *" have been run in this file. |
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.
Extra space here.
| 'dataclasses.InitVar [ str]', | ||
|
|
||
| # Not syntactically valid, but these will | ||
| # be treated as InitVars. |
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.
Extra space also here.
| # Not syntactically valid, but these will | ||
| # be treated as InitVars. | ||
| 'dataclasses.InitVar.[int]', | ||
| 'dataclasses.InitVar+', |
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.
The regex comment above also applies here.
|
Closing to try and re-trigger Travis |
|
Thanks @ericvsmith for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
) (cherry picked from commit 2a7bacb) Co-authored-by: Eric V. Smith <ericvsmith@users.noreply.github.com>
|
GH-6889 is a backport of this pull request to the 3.7 branch. |
https://bugs.python.org/issue33453