Skip to content

Conversation

@ericvsmith
Copy link
Member

@ericvsmith ericvsmith commented May 12, 2018

@ericvsmith ericvsmith requested review from ambv and removed request for ambv May 12, 2018 02:22
# 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
Member

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

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
Member

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.

Copy link
Member

@ilevkivskyi ilevkivskyi left a 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.
Copy link
Member

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

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
Member

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

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
Member

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

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
Member

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

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
Member

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

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
Member

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
Member

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
Member

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
Member

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
Member

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
Member Author

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
@miss-islington
Copy link
Contributor

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 pull request May 16, 2018
)

(cherry picked from commit 2a7bacb)

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

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 02:46
miss-islington added a commit that referenced this pull request May 16, 2018
(cherry picked from commit 2a7bacb)

Co-authored-by: Eric V. Smith <ericvsmith@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants