-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-39942:Fix TypeVar
fails when missing __name__
#19616
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
Conversation
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 opening a pull request @hongweipeng, I tested the changes and it works well, I'm not sure if defaulting to __main__
is the correct behavior thought. If I'm correct both Enums and dataclasses had this issue and they are now just unpickable when __name__
cannot be determined.
if def_mod != 'typing': | ||
self.__module__ = def_mod | ||
try: | ||
def_mod = sys._getframe(1).f_globals.get('__name__', '__main__') # for pickling |
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 don't think it's safe to assume __name__
is __main__
when it's not defined here. I think other classes like enums make the class unpickable when __name__
cannot be determined.
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 '__main__'
is fine. By the way a similar code is used by TypedDict
and NamedTuple
, so you can copy it here. Or maybe it would even make sense to factor this out in an internal helper function doing this:
try:
module = sys._getframe(1).f_globals.get('__name__', '__main__')
except (AttributeError, ValueError):
module = None
@@ -221,6 +221,13 @@ def test_bound_errors(self): | |||
with self.assertRaises(TypeError): | |||
TypeVar('X', str, float, bound=Employee) | |||
|
|||
def test_missing__name__(self): |
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.
def test_missing__name__(self):
# See bpo-39942
code = ("import typing\n"
"T = typing.TypeVar('T')\n"
)
exec(code, {})
would be more like the other tests
This change will also require a NEWS entry (https://devguide.python.org/committing/#what-s-new-and-news-entries), you can add one by using https://blurb-it.herokuapp.com/ |
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.
Looks good, thanks!
Actually, it probably makes sense to backport this to 3.7 and 3.8 since this is technically a bug. Let me try if this still can be done automatically. |
Thanks @hongweipeng for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
Thanks @hongweipeng for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
…19616) https://bugs.python.org/issue39942 (cherry picked from commit a25a04f) Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
GH-19626 is a backport of this pull request to the 3.7 branch. |
…19616) https://bugs.python.org/issue39942 (cherry picked from commit a25a04f) Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
GH-19627 is a backport of this pull request to the 3.8 branch. |
https://bugs.python.org/issue39942 (cherry picked from commit a25a04f) Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
https://bugs.python.org/issue39942 (cherry picked from commit a25a04f) Co-authored-by: HongWeipeng <hongweichen8888@sina.com>
https://bugs.python.org/issue39942