Skip to content

gh-82129: Fix NameError on get_type_hints in dataclasses #122232

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

gh-82129: Fix NameError on get_type_hints in dataclasses #122232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a96efef
gh-82129: Fix `NameError` on `get_type_hints` in `dataclasses`
sobolevn Jul 24, 2024
0d10812
gh-82129: Fix `NameError` on `get_type_hints` in `dataclasses`
sobolevn Jul 24, 2024
7af17e3
Address review
sobolevn Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Lib/dataclasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1530,7 +1530,11 @@ class C(Base):
for item in fields:
if isinstance(item, str):
name = item
tp = 'typing.Any'
typing = sys.modules.get('typing')
if typing:
tp = typing.Any
Copy link
Member

Choose a reason for hiding this comment

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

Could we use "__import__('typing').Any"? Since get_type_hints() calls eval(), that should work regardless of whether the dataclass is defined in a place where typing is imported.

I can provide a more robust solution on 3.14 using __annotate__.

Copy link
Member

Choose a reason for hiding this comment

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

Nikita's current solution feels in keeping with what dataclasses does elsewhere:

cpython/Lib/dataclasses.py

Lines 809 to 815 in e968121

typing = sys.modules.get('typing')
if typing:
if (_is_classvar(a_type, typing)
or (isinstance(f.type, str)
and _is_type(f.type, cls, typing, typing.ClassVar,
_is_classvar))):
f._field_type = _FIELD_CLASSVAR

dataclasses in general takes great pains to avoid importing typing if it's not already imported. I think these days typing is actually a faster import than dataclasses, but for now I think it's probably best to stick with the existing general philosophy of dataclasses there.

Copy link
Member

Choose a reason for hiding this comment

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

My solution would not involve importing typing in the dataclasses module.

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 can provide a more robust solution on 3.14 using annotate.

I am already working on that for 3.14+ 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we use "import('typing').Any"?

I feel like this might not be a good idea for older versions. Since right now dataclasses do not import typing directly, this might cause performance regressions. This PR can be easily backported.

Plus, we have __annotate__ for the future versions.

Looks like a win-win to me.

Copy link
Member

Choose a reason for hiding this comment

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

Note that I am suggesting to put the __import__ in a string, so typing would only be imported when the annotations are evaluated.

Copy link
Member

Choose a reason for hiding this comment

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

@AlexWaygood it would look surprising, yes, but it would solve the user-facing issue that got reported.

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 agree with @AlexWaygood, this might be a very scary thing to see for annotations without the evaluation 😱

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, but then I don't think we should go with the current solution either. My example below seems like a reasonable scenario for how this could end up getting used in practice: user code does not import typing but calls make_dataclass(), later something like cattrs run and tries to get the type hints for the class.

With your PR, this will work only if typing happened to have been imported at the time the dataclass was created. But that becomes very fragile: it means that working code can break if you refactor your code to reorder imports, or remove an import typing from an unrelated part of the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's try both: typing.Any if typing exists, and "__import__('typing').Any" if it doesn't 👍

else:
tp = "__import__('typing').Any"
elif len(item) == 2:
name, tp, = item
elif len(item) == 3:
Expand Down
26 changes: 21 additions & 5 deletions Lib/test/test_dataclasses/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import weakref
import traceback
import unittest
import sys
from unittest.mock import Mock
from typing import ClassVar, Any, List, Union, Tuple, Dict, Generic, TypeVar, Optional, Protocol, DefaultDict
from typing import get_type_hints
Expand All @@ -23,6 +24,7 @@
import dataclasses # Needed for the string "dataclasses.InitVar[int]" to work as an annotation.

from test import support
from test.support import import_helper

# Just any custom exception we can catch.
class CustomError(Exception): pass
Expand Down Expand Up @@ -4108,16 +4110,30 @@ def test_no_types(self):
C = make_dataclass('Point', ['x', 'y', 'z'])
c = C(1, 2, 3)
self.assertEqual(vars(c), {'x': 1, 'y': 2, 'z': 3})
self.assertEqual(C.__annotations__, {'x': 'typing.Any',
'y': 'typing.Any',
'z': 'typing.Any'})
self.assertEqual(C.__annotations__, {'x': typing.Any,
'y': typing.Any,
'z': typing.Any})

C = make_dataclass('Point', ['x', ('y', int), 'z'])
c = C(1, 2, 3)
self.assertEqual(vars(c), {'x': 1, 'y': 2, 'z': 3})
self.assertEqual(C.__annotations__, {'x': 'typing.Any',
self.assertEqual(C.__annotations__, {'x': typing.Any,
'y': int,
'z': 'typing.Any'})
'z': typing.Any})

def test_no_types_no_NameError(self):
C = make_dataclass('Point', ['x'])
self.assertEqual(C.__annotations__, {'x': typing.Any})
self.assertEqual(get_type_hints(C), {'x': typing.Any})

def test_no_types_no_NameError_no_typing_fallback(self):
with import_helper.isolated_modules():
del sys.modules['typing']
C = make_dataclass('Point', ['x'])
self.assertEqual(C.__annotations__,
{'x': "__import__('typing').Any"})
from typing import Any # since we hack our modules
self.assertEqual(get_type_hints(C), {'x': Any})

def test_module_attr(self):
self.assertEqual(ByMakeDataClass.__module__, __name__)
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2024-07-24-15-47-11.gh-issue-82129.7z5g8K.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix name error in :func:`typing.get_type_hints` when calling it in a
dataclass created without an annotation via
:func:`dataclasses.make_dataclass` function.
Loading