Skip to content

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commented Jul 24, 2024

Ok, here's my attempt at solving this.

When you call typing.get_type_hints you still have to import typing.
There's a good chance that it will already be imported by something else in your sys.modules.

We already have this hack in other places:

typing=sys.modules.get('typing')
iftyping:
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

So, this solution can be used to hide the problem for older versions of python (down to 3.12). While we can actually fully solve with annotationlib in 3.14+

I think that this hack + user-space one:

S=make_dataclass('S', ['x']) get_type_hints(S,{'typing': typing})

is good enough for now.

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

neat!

tp='typing.Any'
typing=sys.modules.get('typing')
iftyping:
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:

typing=sys.modules.get('typing')
iftyping:
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
MemberAuthor

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
MemberAuthor

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
MemberAuthor

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
MemberAuthor

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 👍

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

This doesn't necessarily fix the issue.

$ cat dc.py import dataclasses X = dataclasses.make_dataclass("X", ['x']) $ ./python.exe Python 3.14.0a0 (heads/issue-82129:0d1081237a8, Jul 24 2024, 06:15:08) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import dc >>> import typing >>> typing.get_type_hints(dc.X) Traceback (most recent call last): File "<python-input-2>", line 1, in <module> typing.get_type_hints(dc.X) ~~~~~~~~~~~~~~~~~~~~~^^^^^^ File "/Users/jelle/py/cpython/Lib/typing.py", line 2416, in get_type_hints value = _eval_type(value, base_globals, base_locals, base.__type_params__, format=format, owner=obj) File "/Users/jelle/py/cpython/Lib/typing.py", line 477, in _eval_type return evaluate_forward_ref(t, globals=globalns, locals=localns, type_params=type_params, owner=owner, _recursive_guard=recursive_guard, format=format) File "/Users/jelle/py/cpython/Lib/typing.py", line 1069, in evaluate_forward_ref value = forward_ref.evaluate(globals=globals, locals=locals, type_params=type_params, owner=owner) File "/Users/jelle/py/cpython/Lib/annotationlib.py", line 140, in evaluate value = eval(code, globals=globals, locals=locals) File "<string>", line 1, in <module> NameError: name 'typing' is not defined. Did you forget to import 'typing'? >>> 

@bedevere-app
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@sobolevn
Copy link
MemberAuthor

@sobolevn
Copy link
MemberAuthor

Second PR in this chain: #122262
It will allow us to fix this problem for good in 3.14+

@sobolevn
Copy link
MemberAuthor

Friendly ping @JelleZijlstra

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

Changing behavior based on whether typing happens to have been imported when the dataclass is created still feels fragile, and I think there's a risk it causes new problems if we make this change in a bugfix release.

So I think it may be better to leave this bug unsolved for earlier versions. There are many scenarios where get_type_hints() could raise NameError (e.g., when if TYPE_CHECKING is used), and callers have to deal with that. In Python 3.14 we'll have a better foundation for fixing this sort of thing, but in older versions it may be safer to stick with existing behavior.

@sobolevn
Copy link
MemberAuthor

Ok, I agree: there are no clean ways forward. Thanks for everyone's input!

@sobolevnsobolevn closed this Aug 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changesneeds backport to 3.12only security fixesneeds backport to 3.13bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@sobolevn@JelleZijlstra@AlexWaygood