-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-34776: Fix dataclasses to support __future__ "annotations" mode #9518
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
Changes from 4 commits
39006fb
ebcbf69
32427a5
97df9b2
563dda4
8400415
e38de43
71ab405
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -347,20 +347,24 @@ def _create_fn(name, args, body, *, globals=None, locals=None, | |
# __builtins__ may be the "builtins" module or | ||
# the value of its "__dict__", | ||
# so make sure "__builtins__" is the module. | ||
if globals is not None and '__builtins__' not in globals: | ||
globals['__builtins__'] = builtins | ||
if '__builtins__' not in locals: | ||
locals['__builtins__'] = builtins | ||
return_annotation = '' | ||
if return_type is not MISSING: | ||
locals['_return_type'] = return_type | ||
return_annotation = '->_return_type' | ||
args = ','.join(args) | ||
body = '\n'.join(f' {b}' for b in body) | ||
body = '\n'.join(f' {b}' for b in body) | ||
|
||
# Compute the text of the entire function. | ||
txt = f'def {name}({args}){return_annotation}:\n{body}' | ||
txt = f' def {name}({args}){return_annotation}:\n{body}' | ||
|
||
exec(txt, globals, locals) | ||
return locals[name] | ||
local_vars = ', '.join(locals.keys()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to self: add a comment explaining this. |
||
txt = f"def __create_fn__({local_vars}):\n{txt}\n return {name}" | ||
|
||
ns = {} | ||
exec(txt, globals, ns) | ||
return ns['__create_fn__'](**locals) | ||
|
||
|
||
def _field_assign(frozen, name, value, self_name): | ||
|
@@ -448,7 +452,7 @@ def _init_param(f): | |
return f'{f.name}:_type_{f.name}{default}' | ||
|
||
|
||
def _init_fn(fields, frozen, has_post_init, self_name): | ||
def _init_fn(fields, frozen, has_post_init, self_name, globals): | ||
# fields contains both real fields and InitVar pseudo-fields. | ||
|
||
# Make sure we don't have fields without defaults following fields | ||
|
@@ -466,12 +470,15 @@ def _init_fn(fields, frozen, has_post_init, self_name): | |
raise TypeError(f'non-default argument {f.name!r} ' | ||
'follows default argument') | ||
|
||
globals = {'MISSING': MISSING, | ||
'_HAS_DEFAULT_FACTORY': _HAS_DEFAULT_FACTORY} | ||
locals = {f'_type_{f.name}': f.type for f in fields} | ||
locals.update({ | ||
'MISSING': MISSING, | ||
'_HAS_DEFAULT_FACTORY': _HAS_DEFAULT_FACTORY, | ||
}) | ||
|
||
body_lines = [] | ||
for f in fields: | ||
line = _field_init(f, frozen, globals, self_name) | ||
line = _field_init(f, frozen, locals, self_name) | ||
# line is None means that this field doesn't require | ||
# initialization (it's a pseudo-field). Just skip it. | ||
if line: | ||
|
@@ -487,7 +494,6 @@ def _init_fn(fields, frozen, has_post_init, self_name): | |
if not body_lines: | ||
body_lines = ['pass'] | ||
|
||
locals = {f'_type_{f.name}': f.type for f in fields} | ||
return _create_fn('__init__', | ||
[self_name] + [_init_param(f) for f in fields if f.init], | ||
body_lines, | ||
|
@@ -496,19 +502,18 @@ def _init_fn(fields, frozen, has_post_init, self_name): | |
return_type=None) | ||
|
||
|
||
def _repr_fn(fields): | ||
def _repr_fn(fields, globals): | ||
return _create_fn('__repr__', | ||
('self',), | ||
['return self.__class__.__qualname__ + f"(' + | ||
', '.join([f"{f.name}={{self.{f.name}!r}}" | ||
for f in fields]) + | ||
')"']) | ||
')"'], | ||
globals=globals) | ||
|
||
|
||
def _frozen_get_del_attr(cls, fields): | ||
# XXX: globals is modified on the first call to _create_fn, then | ||
# the modified version is used in the second call. Is this okay? | ||
globals = {'cls': cls, | ||
def _frozen_get_del_attr(cls, fields, globals): | ||
locals = {'cls': cls, | ||
'FrozenInstanceError': FrozenInstanceError} | ||
if fields: | ||
fields_str = '(' + ','.join(repr(f.name) for f in fields) + ',)' | ||
|
@@ -520,17 +525,19 @@ def _frozen_get_del_attr(cls, fields): | |
(f'if type(self) is cls or name in {fields_str}:', | ||
' raise FrozenInstanceError(f"cannot assign to field {name!r}")', | ||
f'super(cls, self).__setattr__(name, value)'), | ||
locals=locals, | ||
globals=globals), | ||
_create_fn('__delattr__', | ||
('self', 'name'), | ||
(f'if type(self) is cls or name in {fields_str}:', | ||
' raise FrozenInstanceError(f"cannot delete field {name!r}")', | ||
f'super(cls, self).__delattr__(name)'), | ||
locals=locals, | ||
globals=globals), | ||
) | ||
|
||
|
||
def _cmp_fn(name, op, self_tuple, other_tuple): | ||
def _cmp_fn(name, op, self_tuple, other_tuple, globals): | ||
# Create a comparison function. If the fields in the object are | ||
# named 'x' and 'y', then self_tuple is the string | ||
# '(self.x,self.y)' and other_tuple is the string | ||
|
@@ -540,14 +547,16 @@ def _cmp_fn(name, op, self_tuple, other_tuple): | |
('self', 'other'), | ||
[ 'if other.__class__ is self.__class__:', | ||
f' return {self_tuple}{op}{other_tuple}', | ||
'return NotImplemented']) | ||
'return NotImplemented'], | ||
globals=globals) | ||
|
||
|
||
def _hash_fn(fields): | ||
def _hash_fn(fields, globals): | ||
self_tuple = _tuple_str('self', fields) | ||
return _create_fn('__hash__', | ||
('self',), | ||
[f'return hash({self_tuple})']) | ||
[f'return hash({self_tuple})'], | ||
globals=globals) | ||
|
||
|
||
def _is_classvar(a_type, typing): | ||
|
@@ -719,14 +728,14 @@ def _set_new_attribute(cls, name, value): | |
# take. The common case is to do nothing, so instead of providing a | ||
# function that is a no-op, use None to signify that. | ||
|
||
def _hash_set_none(cls, fields): | ||
def _hash_set_none(cls, fields, globals): | ||
return None | ||
|
||
def _hash_add(cls, fields): | ||
def _hash_add(cls, fields, globals): | ||
flds = [f for f in fields if (f.compare if f.hash is None else f.hash)] | ||
return _hash_fn(flds) | ||
return _hash_fn(flds, globals) | ||
|
||
def _hash_exception(cls, fields): | ||
def _hash_exception(cls, fields, globals): | ||
# Raise an exception. | ||
raise TypeError(f'Cannot overwrite attribute __hash__ ' | ||
f'in class {cls.__name__}') | ||
|
@@ -768,6 +777,16 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
# is defined by the base class, which is found first. | ||
fields = {} | ||
|
||
if cls.__module__ in sys.modules: | ||
globals = sys.modules[cls.__module__].__dict__ | ||
else: | ||
# Theoretically this can happen if someone writes | ||
# a custom string to cls.__module__. In which case | ||
# such dataclass won't be fully introspectable | ||
# (w.r.t. typing.get_type_hints) but will still function | ||
# correctly. | ||
globals = {} | ||
|
||
setattr(cls, _PARAMS, _DataclassParams(init, repr, eq, order, | ||
unsafe_hash, frozen)) | ||
|
||
|
@@ -877,6 +896,7 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
# if possible. | ||
'__dataclass_self__' if 'self' in fields | ||
else 'self', | ||
globals | ||
1st1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
)) | ||
|
||
# Get the fields as a list, and include only real fields. This is | ||
|
@@ -885,7 +905,7 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
|
||
if repr: | ||
flds = [f for f in field_list if f.repr] | ||
_set_new_attribute(cls, '__repr__', _repr_fn(flds)) | ||
_set_new_attribute(cls, '__repr__', _repr_fn(flds, globals)) | ||
|
||
if eq: | ||
# Create _eq__ method. There's no need for a __ne__ method, | ||
|
@@ -895,7 +915,8 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
other_tuple = _tuple_str('other', flds) | ||
_set_new_attribute(cls, '__eq__', | ||
_cmp_fn('__eq__', '==', | ||
self_tuple, other_tuple)) | ||
self_tuple, other_tuple, | ||
globals=globals)) | ||
|
||
if order: | ||
# Create and set the ordering methods. | ||
|
@@ -908,13 +929,14 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
('__ge__', '>='), | ||
]: | ||
if _set_new_attribute(cls, name, | ||
_cmp_fn(name, op, self_tuple, other_tuple)): | ||
_cmp_fn(name, op, self_tuple, other_tuple, | ||
globals=globals)): | ||
raise TypeError(f'Cannot overwrite attribute {name} ' | ||
f'in class {cls.__name__}. Consider using ' | ||
'functools.total_ordering') | ||
|
||
if frozen: | ||
for fn in _frozen_get_del_attr(cls, field_list): | ||
for fn in _frozen_get_del_attr(cls, field_list, globals): | ||
if _set_new_attribute(cls, fn.__name__, fn): | ||
raise TypeError(f'Cannot overwrite attribute {fn.__name__} ' | ||
f'in class {cls.__name__}') | ||
|
@@ -927,7 +949,7 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen): | |
if hash_action: | ||
# No need to call _set_new_attribute here, since by the time | ||
# we're here the overwriting is unconditional. | ||
cls.__hash__ = hash_action(cls, field_list) | ||
cls.__hash__ = hash_action(cls, field_list, globals) | ||
|
||
if not getattr(cls, '__doc__'): | ||
# Create a class doc-string. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
from __future__ import annotations | ||
|
||
import dataclasses | ||
|
||
|
||
class Foo: | ||
pass | ||
|
||
|
||
@dataclasses.dataclass | ||
class Bar: | ||
foo: Foo |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix dataclasses to support __future__ "annotations" mode | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking, this problem is not directly related to 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:
After your change it works as expected:
|
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.
This doesn't actually work --
exec()
gets__builtins__
only from globals.Uh oh!
There was an error while loading. Please reload this page.
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.
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?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.
Probably it's better to rename it to BUILTINS, to make it more obvious.
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.
Guido, I've updated the PR, please take a look.