Repository: python/cpython
Pull request: Fix ClassVar as string fails when getting type hints
Closes python/typing#505
<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
bpo-NNNN: Summary of the changes made
```

Where: bpo-NNNN refers to the issue number in the https://bugs.python.org.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `master`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `master`.

-->


Problem statement:
ClassVar as string fails when getting type hints
I know there are plans to make all type annotations strings eventually.  So I was playing around and discovered this problem.

```
import typing

class Good:
    foo: typing.ClassVar[int] = 7
print(typing.get_type_hints(Good))

class Bad:
    foo: 'typing.ClassVar[int]' = 7
typing.get_type_hints(Bad)
```

```
{'foo': typing.ClassVar[int]}
Traceback (most recent call last):
  File "classvar_problem.py", line 10, in <module>
    typing.get_type_hints(Bad)
  File "/usr/local/homebrew/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/typing.py", line 1511, in get_type_hints
    value = _eval_type(value, base_globals, localns)
  File "/usr/local/homebrew/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/typing.py", line 350, in _eval_type
    return t._eval_type(globalns, localns)
  File "/usr/local/homebrew/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/typing.py", line 246, in _eval_type
    "Forward references must evaluate to types.")
  File "/usr/local/homebrew/Cellar/python3/3.6.3/Frameworks/Python.framework/Versions/3.6/lib/python3.6/typing.py", line 374, in _type_check
    raise TypeError(msg + " Got %.100r." % (arg,))
TypeError: Forward references must evaluate to types. Got typing.ClassVar[int].
```

Hints from issue discussion:
Thanks! This is an interesting edge case. ``ClassVar[int]`` is not valid as a type argument, so currently for example ``List[ClassVar[int]]`` raises ``TypeError``, and we re-use the same internal helper that checks this in ``get_type_hints`` (which is apparently wrong).

We will probably just remove this check soon, so that the errors like this will be reported only in type checkers (like mypy), but not at runtime.

Commit message: Declaring a ClassVar with a type as a string shouldn't fail at runtime. See python/typing#505

Patch to review:
diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py
index 314716cd7de354..eba57a70ba0cd4 100644
--- a/Lib/test/test_typing.py
+++ b/Lib/test/test_typing.py
@@ -1553,14 +1553,6 @@ def foo(a: 'Node[T'):
         with self.assertRaises(SyntaxError):
             get_type_hints(foo)
 
-    def test_type_error(self):
-
-        def foo(a: Tuple['42']):
-            pass
-
-        with self.assertRaises(TypeError):
-            get_type_hints(foo)
-
     def test_name_error(self):
 
         def foo(a: 'Noode[T]'):
@@ -1590,6 +1582,20 @@ def foo(a: 'whatevers') -> {}:
         ith = get_type_hints(C().foo)
         self.assertEqual(ith, {})
 
+    def test_no_type_check_forward_ref_as_string(self):
+        class C:
+            foo: typing.ClassVar[int] = 7
+        class D:
+            foo: ClassVar[int] = 7
+        class E:
+            foo: 'typing.ClassVar[int]' = 7
+        class F:
+            foo: 'ClassVar[int]' = 7
+
+        expected_result = {'foo': typing.ClassVar[int]}
+        for clazz in [C, D, E, F]:
+            self.assertEqual(get_type_hints(clazz), expected_result)
+
     def test_no_type_check_no_bases(self):
         class C:
             def meth(self, x: int): ...
diff --git a/Lib/typing.py b/Lib/typing.py
index 8025dfd932624b..bbcae38761a834 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -486,9 +486,7 @@ def _evaluate(self, globalns, localns):
                 globalns = localns
             elif localns is None:
                 localns = globalns
-            self.__forward_value__ = _type_check(
-                eval(self.__forward_code__, globalns, localns),
-                "Forward references must evaluate to types.")
+            self.__forward_value__ = eval(self.__forward_code__, globalns, localns)
             self.__forward_evaluated__ = True
         return self.__forward_value__
 


Code review comments:
diff_hunk: @@ -486,9 +486,7 @@ def _evaluate(self, globalns, localns):
                 globalns = localns
             elif localns is None:
                 localns = globalns
-            self.__forward_value__ = _type_check(
-                eval(self.__forward_code__, globalns, localns),
-                "Forward references must evaluate to types.")
+            self.__forward_value__ = eval(self.__forward_code__, globalns, localns)
path: Lib/typing.py
text: @user1:
I am not sure just removing the check is the right thing to do. Instead I would rather fix `_type_check`.

We can of course abandon any runtime validation, but this should be a conscious decision and goes beyond the scope of this patch.

@user2:
Well, `get_type_hints()` is not running _type_check on non-forward references so I feel it's a bit weird that we do it for forward references only. Why do we need the additional check for that case? It did successfully evaluate after all, just as if you didn't use a ForwardRef.

@user1:
This is indeed inconsistent, but the proposed solution creates a bigger inconsistency IMO. This code is also called from other places like `List['C']`, with this change arbitrary string will be valid. The actual problem is that currently `_type_check` is used for two different purposes:
* Check whether something is valid as a type
* Check whether something is valid as a type argument

The proper solution is to disentangle these two. Namely, at this particular position we only need to check if it is a valid type, so we can use a less restrictive check. I think the proper solution would be to add an optional argument to `_type_check` to skip the two corresponding checks.