Skip to content

/cpython

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
Merged

bpo-34776: Fix dataclasses to support __future__ "annotations" mode #9518

merged 8 commits into from Dec 9, 2019

Conversation

@1st1
Copy link
Member

1st1 commented Sep 23, 2018 •
edited by bedevere-bot

https://bugs.python.org/issue34776
@1st1
bpo-34776: Fix dataclasses to support __future__ "annotations" mode
Loading status checks…
39006fb
@1st1 1st1 requested a review from ericvsmith Sep 23, 2018
@1st1 1st1 requested a review from gvanrossum Sep 23, 2018
@gvanrossum
gvanrossum reviewed Sep 23, 2018
View changes
Copy link
Member

gvanrossum left a comment

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
Implement Guido's suggestion to always set correct __globals__
Loading status checks…
ebcbf69
@1st1

This comment has been minimized.

Sign in to view
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 added 2 commits Sep 23, 2018
@1st1
Add a comment
Loading status checks…
32427a5
@1st1
nit fix
Loading status checks…
97df9b2
@1st1

This comment has been minimized.

Sign in to view
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.
@gvanrossum
gvanrossum reviewed Sep 23, 2018
View changes
Lib/dataclasses.py Outdated
if globals is not None and '__builtins__' not in globals:
globals['__builtins__'] = builtins
if '__builtins__' not in locals:
locals['__builtins__'] = builtins

This comment has been minimized.

Sign in to view
Copy link
@gvanrossum

gvanrossum Sep 23, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 Sep 23, 2018 •
edited

Author Member

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?

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 Sep 23, 2018

Author Member

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

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 Sep 24, 2018

Author Member

Guido, I've updated the PR, please take a look.
@1st1
rename __builtins__ for clarity
Loading status checks…
563dda4
@gvanrossum
gvanrossum reviewed Sep 24, 2018
View changes
Copy link
Member

gvanrossum left a comment

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
@1st1
Add a trailing comma
Loading status checks…
8400415
@ericvsmith

This comment has been minimized.

Sign in to view
Copy link
Member

ericvsmith commented Sep 24, 2018

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

ericvsmith commented Oct 8, 2018

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

This comment has been minimized.

Sign in to view
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?
@1st1
Merge branch 'master' into fix_dc_introspect
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e38de43
@ericvsmith

This comment has been minimized.

Sign in to view
Copy link
Member

ericvsmith commented Dec 18, 2018

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.
@Vlad-Shcherbina
Vlad-Shcherbina reviewed Jan 15, 2019
View changes
Misc/NEWS.d/next/Library/2018-09-23-14-24-37.bpo-34776.1SrQe3.rst Outdated
@@ -0,0 +1 @@
Fix dataclasses to support __future__ "annotations" mode

This comment has been minimized.

Sign in to view
Copy link
@Vlad-Shcherbina

Vlad-Shcherbina Jan 15, 2019 •
edited

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'>}
@1st1
1st1 reviewed May 2, 2019
View changes
Lib/dataclasses.py

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

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 May 2, 2019

Author Member

Note to self: add a comment explaining this.
@ilevkivskyi

This comment has been minimized.

Sign in to view
Copy link
Contributor

ilevkivskyi commented Sep 2, 2019

It looks like https://bugs.python.org/issue37948 will be also fixed by this PR.
@ilevkivskyi
ilevkivskyi approved these changes Sep 2, 2019
View changes
Copy link
Contributor

ilevkivskyi left a comment

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

This comment has been minimized.

Sign in to view
Copy link

a-recknagel commented Sep 4, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

ericvsmith commented Sep 4, 2019

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

This comment has been minimized.

Sign in to view
Copy link

a-recknagel commented Sep 5, 2019

@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.
@jacebrowning jacebrowning mentioned this pull request Oct 1, 2019
Open
@ambv
Update 2018-09-23-14-24-37.bpo-34776.1SrQe3.rst
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
71ab405
@ambv ambv merged commit d219cc4 into python:master Dec 9, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191209.20 succeeded
Details
bedevere/issue-number Issue number 34776 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 9, 2019

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

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 9, 2019

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

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 9, 2019

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 added a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@1st1 @miss-islington
bpo-34776: Fix dataclasses to support __future__ "annotations" mode (p…
5c0117c 
…ythonGH-9518)

(cherry picked from commit d219cc4)

Co-authored-by: Yury Selivanov <yury@magic.io>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 9, 2019

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

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 9, 2019

GH-17532 is a backport of this pull request to the 3.7 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@1st1 @miss-islington
bpo-34776: Fix dataclasses to support __future__ "annotations" mode (p…
c346808 
…ythonGH-9518)

(cherry picked from commit d219cc4)

Co-authored-by: Yury Selivanov <yury@magic.io>
ambv added a commit that referenced this pull request Dec 9, 2019
@miss-islington @1st1 @ambv
bpo-34776: Fix dataclasses to support __future__ "annotations" mode (G…
Loading status checks…
0d57db2 
…H-9518) (#17531)

(cherry picked from commit d219cc4)

Co-authored-by: Yury Selivanov <yury@magic.io>
ambv added a commit that referenced this pull request Dec 9, 2019
@miss-islington @1st1 @ambv
bpo-34776: Fix dataclasses to support __future__ "annotations" mode (G…
Loading status checks…
66d7a5d 
…H-9518) (#17532)

(cherry picked from commit d219cc4)

Co-authored-by: Yury Selivanov <yury@magic.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vlad-Shcherbina Vlad-Shcherbina

@gvanrossum gvanrossum

@ilevkivskyi ilevkivskyi

@ericvsmith ericvsmith

Assignees

@ericvsmith ericvsmith

Labels
CLA signed
Projects
None yet
Milestone
No milestone
10 participants
@1st1 @ericvsmith @ilevkivskyi @a-recknagel @bedevere-bot @miss-islington @Vlad-Shcherbina @gvanrossum @ambv @the-knights-who-say-ni
You can’t perform that action at this time.