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

gh-82129: Provide __annotate__ method for dataclasses from make_dataclass #122262

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jul 25, 2024

This is my plan for python3.14+

While #122232 can be backported to older versions and solve their problems, this one is actually a correct way to solve this problem for the future.

This is a WIP, because I think that we should first decide on #122232

@sobolevn
Copy link
Member Author

Test failure:

 ======================================================================
FAIL: test_no_types (test.test_dataclasses.TestMakeDataclass.test_no_types)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_dataclasses\__init__.py", line 4111, in test_no_types
    self.assertEqual(C.__annotations__, {'x': 'typing.Any',
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                         'y': 'typing.Any',
                                         ^^^^^^^^^^^^^^^^^^
                                         'z': 'typing.Any'})
                                         ^^^^^^^^^^^^^^^^^^^
AssertionError: {'x': typing.Any, 'y': typing.Any, 'z': typing.Any} != {'x': 'typing.Any', 'y': 'typing.Any', 'z': 'typing.Any'}
- {'x': typing.Any, 'y': typing.Any, 'z': typing.Any}
+ {'x': 'typing.Any', 'y': 'typing.Any', 'z': 'typing.Any'}
?       +          +       +          +       +          +

is expected, I just don't want to touch tests at this point.

@sobolevn sobolevn changed the title gh-82128: Provide __annotate__ method for dataclasses from make_dataclass gh-82129: Provide __annotate__ method for dataclasses from make_dataclass Jul 25, 2024
@DavidCEllis
Copy link
Contributor

I mentioned it on the issue but with this change typing is imported on class creation because the dataclass call will inspect the annotations and hence trigger the import. It also imports typing even if there are no untyped fields.

Python 3.14.0a0 (heads/main:5592399313, Jul 24 2024, 19:11:33) [MSC v.1938 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> from dataclasses import make_dataclass
>>> print(sys.modules.get("typing"))
None
>>> A = make_dataclass('A', [('a_var', str)])
>>> print(sys.modules.get("typing"))
<module 'typing' from '...\\cpython\\Lib\\typing.py'>

If you want to avoid the typing import this way you need to put some placeholder for dataclass to use and replace the attribute afterwards. Possibly also modifying Field.type to be a property that does the same replacement.

This doesn't happen on the 3.13 version proposed because dataclasses doesn't evaluate the strings so the import doesn't get called by dataclasses when it looks at the annotations.

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

You might be able to get around @DavidCEllis's point by adding some internal-only flag to tell @dataclass to skip introspecting the annotations.

@DavidCEllis
Copy link
Contributor

DavidCEllis commented Jul 25, 2024

Perhaps you can take advantage of dataclasses calling annotate with format.FORWARDREF and return something that looks like a ForwardRef there (or a real ForwardRef if there is a way to make this evaluate correctly if typing has been imported).

If it's called with format.VALUE you could then do the import and return the typing.Any object.

@sobolevn
Copy link
Member Author

Perhaps you can take advantage of dataclasses calling annotate with format.FORWARDREF and return something that looks like a ForwardRef there (or a real ForwardRef if there is a way to make this evaluate correctly if typing has been imported).

Yes, this is my go-to idea right now :)

@sobolevn sobolevn marked this pull request as ready for review September 18, 2024 06:18
@sobolevn
Copy link
Member Author

This PR still does not account for the problem mentioned by @DavidCEllis
It still imports typing when dataclass is created, because dataclasses accesses .__annotations__ inside prepare_class function.

I will try to work around this problem.

@sobolevn
Copy link
Member Author

But, since dataclasses.prepare_class sets __annotations__ in the created class, __annotate__ won't ever be called again, so I am not sure what to do.

cc @JelleZijlstra about my question in #122285

@sobolevn
Copy link
Member Author

sobolevn commented Sep 20, 2024

Now make_dataclass does not import typing on dataclass creation 🎉
And everything else seem to work just fine!

Thanks everyone for your help and ideas! 🤝

@@ -1549,11 +1550,29 @@ class C(Base):
seen.add(name)
annotations[name] = tp

def annotate_method(format):
typing = sys.modules.get("typing")
if typing is None and format == annotationlib.Format.FORWARDREF:
Copy link
Member

Choose a reason for hiding this comment

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

We could also avoid importing typing for the SOURCE format here I think; is that worth it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure we can. I need _convert_to_source, there can be complex annotations that should be formatted properly. I will open a new issue about converting annotations to string with public API though. Right now I don't see a clear way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #124412 for that.

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

…wt3u.rst

Co-authored-by: Carl Meyer <carl@oddbird.net>
MandeepSinghPB08

This comment was marked as spam.

@@ -1527,10 +1527,11 @@ class C(Base):
seen = set()
annotations = {}
defaults = {}
any_marker = object()
Copy link
Member

Choose a reason for hiding this comment

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

I'll reserve a full review until I've had time to read PEP 749, but couldn't the object() used here be a module-level "constant", instead of being allocated every time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants