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-34776: Fix dataclasses to support __future__ "annotations" mode #9518

Merged
merged 8 commits into from
Dec 9, 2019

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Sep 23, 2018

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I wonder if the fix shouldn't apply to all callers of _create_fn()? I know __init__ is the only one that (currently) has annotations, but in general I wish the generated functions looked more like handwritten ones to various introspection tools.

@1st1
Copy link
Member Author

1st1 commented Sep 23, 2018

I wonder if the fix shouldn't apply to all callers of _create_fn()? I know init is the only one that (currently) has annotations, but in general I wish the generated functions looked more like handwritten ones to various introspection tools.

I think there's no harm in doing that. I've updated the PR. It's a bit more invasive now, but the benefit is that we always use the same mechanism (a closure) to inject objects into generated code.

@1st1
Copy link
Member Author

1st1 commented Sep 23, 2018

BTW this looks like a relatively safe change for 3.7.1, but it's Eric's call.

if globals is not None and '__builtins__' not in globals:
globals['__builtins__'] = builtins
if '__builtins__' not in locals:
locals['__builtins__'] = builtins
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually work -- exec() gets __builtins__ only from globals.

Copy link
Member Author

@1st1 1st1 Sep 23, 2018

Choose a reason for hiding this comment

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

Hm, we only need to bound the "__builtins__" name inside the closure as there's generated code that explicitly uses it as "__builtins__.attribute". I can change the name to "_builtins" and everything should be fine too or... am I missing something here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably it's better to rename it to BUILTINS, to make it more obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Guido, I've updated the PR, please take a look.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I don't follow the logic in dataclasses.py well enough to approve this, though I see nothing wrong other than a missing trailing comma. @ericvsmith will have to have the last word.

Lib/dataclasses.py Outdated Show resolved Hide resolved
@ericvsmith
Copy link
Member

I'll try and look at this today. Thanks, @1st1 and @gvanrossum for the code and reviews.

@1st1
Copy link
Member Author

1st1 commented Oct 7, 2018

According to https://discuss.python.org/t/3-7-1-and-3-6-7-release-update/184 there's still couple of days to get this into 3.7.1. @ericvsmith do you have time for a review?

@1st1
Copy link
Member Author

1st1 commented Oct 7, 2018

FWIW we can just merge the 39006fb commit to 3.7 which does the trick in the most minimal way.

@ericvsmith
Copy link
Member

Unfortunately I'm out of town until 2018-10-10 with limited internet access until then. I won't be able to do a decent review until then.

@1st1
Copy link
Member Author

1st1 commented Dec 12, 2018

Hi Eric, Python 3.7.2rc1 has been released but this PR is still not merged. Is it possible to somehow unblock it?

@ericvsmith
Copy link
Member

I apologize for being tied up with a client on end-of-year work. I don't see how this could get in to 3.7.2, but I'm planning on looking at it in January.

@@ -0,0 +1 @@
Fix dataclasses to support __future__ "annotations" mode
Copy link

@Vlad-Shcherbina Vlad-Shcherbina Jan 15, 2019

Choose a reason for hiding this comment

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

Strictly speaking, this problem is not directly related to __future__ annotations.
It's about postponed evaluation of type annotations in general, including forward references in pre-PEP-563 annotations.
Consider rewording along the lines of "Fix dataclasses to support forward references in type annotations".

Here is an example that does not use PEP 563:

from typing import get_type_hints
from dataclasses import dataclass

class T:
    pass

@dataclass()
class C2:
    x: 'T'

print(get_type_hints(C2.__init__))

Before your change:

Traceback (most recent call last):
  File ".\zzz.py", line 11, in <module>
    print(get_type_hints(C2.__init__))
  File "C:\Python37\lib\typing.py", line 1001, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "C:\Python37\lib\typing.py", line 260, in _eval_type
    return t._evaluate(globalns, localns)
  File "C:\Python37\lib\typing.py", line 464, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'T' is not defined

After your change it works as expected:

{'x': <class '__main__.T'>, 'return': <class 'NoneType'>}


exec(txt, globals, locals)
return locals[name]
local_vars = ', '.join(locals.keys())
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: add a comment explaining this.

@ilevkivskyi
Copy link
Member

It looks like https://bugs.python.org/issue37948 will be also fixed by this PR.

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.

FWIW the fix looks good to me. It would be great to add few more comments and a test for plain string annotations and/or https://bugs.python.org/issue37948

@a-recknagel
Copy link

It looks like https://bugs.python.org/issue37948 will be also fixed by this PR.

It won't, I just tested with https://github.com/1st1/cpython/tree/fix_dc_introspect

@ericvsmith
Copy link
Member

It won't, I just tested with https://github.com/1st1/cpython/tree/fix_dc_introspect

@a-recknagel : Could you post your test code and the results? Thanks.

@a-recknagel
Copy link

@ericvsmith Sure. python was built from e38de43, and I ran

>>> import dataclasses
>>> import typing
>>> A = dataclasses.make_dataclass('A', ['var'])
>>> typing.get_type_hints(A)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/arne/pythons/python_dc_fix/lib/python3.8/typing.py", line 972, in get_type_hints
    value = _eval_type(value, base_globals, localns)
  File "/home/arne/pythons/python_dc_fix/lib/python3.8/typing.py", line 259, in _eval_type
    return t._evaluate(globalns, localns)
  File "/home/arne/pythons/python_dc_fix/lib/python3.8/typing.py", line 463, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'typing' is not defined

Which is the same behavior as on the current master.

@ambv ambv merged commit d219cc4 into python:master Dec 9, 2019
@bedevere-bot
Copy link

@ambv: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Thanks @1st1 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@bedevere-bot
Copy link

GH-17531 is a backport of this pull request to the 3.8 branch.

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
ambv pushed a commit that referenced this pull request Dec 9, 2019
ambv pushed a commit that referenced this pull request Dec 9, 2019
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.

10 participants