Skip to content
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

Merged
merged 12 commits into from May 16, 2018

Conversation

Copy link
Member

@ericvsmith ericvsmith commented May 12, 2018

@ericvsmith ericvsmith requested review from ambv and removed request for ambv May 12, 2018
# 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):
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 13, 2018

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.

Copy link
Member Author

@ericvsmith ericvsmith May 13, 2018

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.

Copy link
Contributor

@ilevkivskyi ilevkivskyi May 13, 2018

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.

ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented on e5fe013 May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check may not work for bare ClassVar, for example, do we want to allow this:

class C:
    x: ClassVar  # this is allowed by PEP 526
    y: int

I think we do, so there should be another option ... or a_type is typing.ClassVar.

ericvsmith
Copy link
Member Author

@ericvsmith ericvsmith commented on e5fe013 May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rely on your expertise here. I'm happy to add this, and I think I should also add it to the regex for string annotations.

ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented on e5fe013 May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just double-checked mypy accepts this code and interprets bare ClassVar as a shorthand for ClassVar[Any].

ericvsmith
Copy link
Member Author

@ericvsmith ericvsmith commented on e5fe013 May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I've relaxed the regex to allow for this, for both ClassVar and InitVar.

Copy link
Contributor

@ilevkivskyi ilevkivskyi left a comment

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.
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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).

Copy link
Member Author

@ericvsmith ericvsmith May 15, 2018

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.

and a_type.__origin__ is typing.ClassVar))



Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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?

Copy link
Member Author

@ericvsmith ericvsmith May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

# looking in sys.modules (again!), and seems like a waste since
# the caller already knows a_module.

# typename is a string type annotation
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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".

Copy link
Member Author

@ericvsmith ericvsmith May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

# 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
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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"?

Copy link
Member Author

@ericvsmith ericvsmith May 15, 2018

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

Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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.

Copy link
Member Author

@ericvsmith ericvsmith May 15, 2018

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.
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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+',
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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.
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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.
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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+',
Copy link
Contributor

@ilevkivskyi ilevkivskyi May 15, 2018

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.

@ericvsmith
Copy link
Author

@ericvsmith ericvsmith commented May 15, 2018

Closing to try and re-trigger Travis

@ericvsmith ericvsmith closed this May 15, 2018
@ericvsmith ericvsmith reopened this May 15, 2018
@ericvsmith ericvsmith merged commit 2a7bacb into python:master May 16, 2018
4 checks passed
@miss-islington
Copy link

@miss-islington miss-islington commented May 16, 2018

Thanks @ericvsmith for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 16, 2018
)

(cherry picked from commit 2a7bacb)

Co-authored-by: Eric V. Smith <ericvsmith@users.noreply.github.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 16, 2018

GH-6889 is a backport of this pull request to the 3.7 branch.

@ericvsmith ericvsmith deleted the bpo-33453-str-annotations branch May 16, 2018
miss-islington added a commit that referenced this issue May 16, 2018
(cherry picked from commit 2a7bacb)

Co-authored-by: Eric V. Smith <ericvsmith@users.noreply.github.com>
lisroach pushed a commit to lisroach/cpython that referenced this issue Sep 10, 2018
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this issue Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants