Fix ClassVar as string fails when getting type hints #6824

nnja · 2018-05-14T19:31:14Z

the-knights-who-say-ni · 2018-05-14T19:31:17Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

ilevkivskyi · 2018-05-14T19:44:19Z

Lib/typing.py

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


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.

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.

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.

ilevkivskyi · 2018-05-14T19:45:27Z

Could you please also add a short NEWS entry? (You can use blurb to do this.)

gvanrossum

Review declined.

…ssVar scenario.

nnja · 2018-05-15T22:11:03Z

The PR has been updated based on comments by @ilevkivskyi and @ambv, added a NEWS entry, and re-signed the contributor agreement (I believe I had already signed it a few years ago). Please recheck. Thank you.

miss-islington · 2018-05-16T16:27:06Z

Thanks @nnja for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

ambv · 2018-05-16T16:27:16Z

Thanks! ✨ 🍰 ✨

(cherry picked from commit 2d2d3b1) Co-authored-by: Nina Zakharenko <[email protected]>

bedevere-bot · 2018-05-16T16:28:14Z

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

(cherry picked from commit 2d2d3b1) Co-authored-by: Nina Zakharenko <[email protected]>

ilevkivskyi · 2018-05-17T00:25:31Z

Lib/typing.py

@@ -998,7 +1005,7 @@ def get_type_hints(obj, globalns=None, localns=None):
                if value is None:
                    value = type(None)
                if isinstance(value, str):
-                    value = ForwardRef(value)
+                    value = ForwardRef(value, is_argument=True)


Sorry for not having time to review this earlier, this all looks good except for the meaning is exactly the opposite to what I expected :-) There are two options:

Rename is_argument to is_not_argument everywhere (arguably ugly)

Carefully replace True with False and vice versa, and update the corresponding checks

@nnja @ambv Will any of you have time to make a PR?

@ilevkivskyi cc/ @ambv Yes, I'll have time tomorrow evening on my flight home.

Ivan, I don't think the meaning is reversed? As I understand it:

"argument" is what's directly an annotation on a function argument and so on; so it includes non-types

otherwise, it has to be a type (like inside generics)

If you still want to make a change to this, I would prefer renaming to is_annotation which signifies that we're talking about an entire annotations.

Renaming to is_annotation is a possible option, but IMO is still less clear than is_argument with reversed value, since the latter makes simple distinction between these two cases:

List['int'], here 'int' appears as a type argument in another type

'int' here 'int' appears just as a type

In the first case the rules are stricter, e.g. ClassVar can't appear as a type argument.

In view of this, the ideal option would be to both rename it to is_type_argument and reverse False to True, but I don't want to ask for too much :-) I will be happy with either option if there will be a short comment explaining what is_argument/is_annotation/is_type_argument means.

Is the term "type argument" used anywhere in typing? I only heard of them being called "generics".

OK, cool, in this case you're right, let's just reverse True with False and vice versa.

@ambv @ilevkivskyi Should I open a new PR for these changes? What's the needed course of action for the backport? #6912

Yes, please make a PR against master branch. Also it makes sense to do this ASAP, since 3.7rc1 is out in one day.

@ilevkivskyi 👍I can work on this tonight. Will you be available to review?

@ilevkivskyi Done! The updated PR is #7039. Please let me know how it looks.

ilevkivskyi · 2018-05-21T23:36:59Z

Will you be available to review?

Yes, I will be available until midnight PST

terryjreedy · 2018-08-30T18:44:43Z

The CLA label should have been verified before merging this.

ambv · 2018-08-30T18:51:36Z

This was during PyCon sprints. I knew the CLA is signed, bugs.python.org was behind. The confirmation there is manual.

terryjreedy · 2018-08-30T20:54:37Z

Ah. Instantly recorded signings would be great for sprints.

Declaring a ClassVar with a type as a string shouldn't fail at runtim…

d13c741

…e. See python/typing#505

nnja requested review from gvanrossum and ilevkivskyi as code owners May 14, 2018 19:31

bedevere-bot added the awaiting review label May 14, 2018

the-knights-who-say-ni added the CLA not signed label May 14, 2018

ilevkivskyi added skip issue needs backport to 3.7 labels May 14, 2018

ilevkivskyi reviewed May 14, 2018

View reviewed changes

gvanrossum reviewed May 14, 2018

View reviewed changes

Address comments from PR. Add additional unit test to test nested Cla…

b8f0472

…ssVar scenario.

nnja force-pushed the fix_classvar_as_string_failing branch from 7861376 to b8f0472 Compare May 15, 2018 18:58

Add back a test that was accidentally removed

79bfabb

ilevkivskyi removed the CLA not signed label May 15, 2018

the-knights-who-say-ni added the CLA not signed label May 15, 2018

Add NEWS entry

5eb9b73

Update 2018-05-15-18-02-03.bpo-0.pj2Mbb.rst

a053fcd

ambv merged commit 2d2d3b1 into python:master May 16, 2018

bedevere-bot removed the awaiting review label May 16, 2018

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

Fix ClassVar as string fails when getting type hints (pythonGH-6824)

9384427

(cherry picked from commit 2d2d3b1) Co-authored-by: Nina Zakharenko <[email protected]>

bedevere-bot removed the needs backport to 3.7 label May 16, 2018

ambv pushed a commit that referenced this pull request May 16, 2018

Fix ClassVar as string fails when getting type hints (GH-6824) (#6912)

9c17cd3

(cherry picked from commit 2d2d3b1) Co-authored-by: Nina Zakharenko <[email protected]>

ilevkivskyi reviewed May 17, 2018

View reviewed changes

nnja mentioned this pull request May 22, 2018

Reverse the meaning of is_argument when used for type check #7039

Merged

terryjreedy removed the CLA not signed label Aug 30, 2018

the-knights-who-say-ni added the CLA signed label Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ClassVar as string fails when getting type hints #6824

Fix ClassVar as string fails when getting type hints #6824

nnja commented May 14, 2018

the-knights-who-say-ni commented May 14, 2018

ilevkivskyi May 14, 2018

ambv May 14, 2018

ilevkivskyi May 14, 2018

ilevkivskyi commented May 14, 2018

gvanrossum left a comment

nnja commented May 15, 2018

miss-islington commented May 16, 2018

ambv commented May 16, 2018

bedevere-bot commented May 16, 2018

ilevkivskyi May 17, 2018

nnja May 17, 2018

ambv May 17, 2018

ilevkivskyi May 17, 2018 •

edited

Loading

ambv May 17, 2018

ambv May 17, 2018

nnja May 21, 2018

ilevkivskyi May 21, 2018

nnja May 21, 2018

nnja May 22, 2018

ilevkivskyi commented May 21, 2018

terryjreedy commented Aug 30, 2018

ambv commented Aug 30, 2018

terryjreedy commented Aug 30, 2018

Fix ClassVar as string fails when getting type hints #6824

Fix ClassVar as string fails when getting type hints #6824

Conversation

nnja commented May 14, 2018

the-knights-who-say-ni commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented May 14, 2018

gvanrossum left a comment

Choose a reason for hiding this comment

nnja commented May 15, 2018

miss-islington commented May 16, 2018

ambv commented May 16, 2018

bedevere-bot commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented May 21, 2018

terryjreedy commented Aug 30, 2018

ambv commented Aug 30, 2018

terryjreedy commented Aug 30, 2018

ilevkivskyi May 17, 2018 •

edited

Loading