GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-82129: Fix `NameError` on `get_type_hints` in `dataclasses` by sobolevn · Pull Request #122232 · python/cpython

Conversation

sobolevn commented Jul 24, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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:

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

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.

Issue: get_type_hints fails if there are un-annotated fields in a dataclass #82129

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sobolevn added 2 commits

July 24, 2024 15:45


          pythongh-82129: Fix NameError on get_type_hints in dataclasses

a96efef


          pythongh-82129: Fix NameError on get_type_hints in dataclasses

0d10812

sobolevn added needs backport to 3.12

only security fixes

needs backport to 3.13 only security fixes labels

Jul 24, 2024

sobolevn requested a review from AlexWaygood

July 24, 2024 12:52

sobolevn requested a review from ericvsmith as a code owner

July 24, 2024 12:52

bedevere-app Bot mentioned this pull request

Jul 24, 2024

get_type_hints fails if there are un-annotated fields in a dataclass #82129

Closed

bedevere-app Bot added the awaiting core review label

Jul 24, 2024

AlexWaygood approved these changes

Jul 24, 2024

View reviewed changes

AlexWaygood left a comment

Copy link

Copy Markdown

Member

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.

neat!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Jul 24, 2024

JelleZijlstra reviewed

Jul 24, 2024

View reviewed changes

Comment thread

Lib/dataclasses.py

		tp = 'typing.Any'
		typing = sys.modules.get('typing')
		if typing:
		tp = typing.Any

JelleZijlstra Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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__.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sobolevn Jul 24, 2024

Copy link

Copy Markdown

Member Author

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.

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

I am already working on that for 3.14+ 😉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sobolevn Jul 24, 2024

Copy link

Copy Markdown

Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood and sobolevn reacted with thumbs up emoji

All reactions

👍 2 reactions

JelleZijlstra Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sobolevn Jul 24, 2024

Copy link

Copy Markdown

Member Author

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.

I agree with @AlexWaygood, this might be a very scary thing to see for annotations without the evaluation 😱

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Jul 24, 2024

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sobolevn Jul 24, 2024

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra requested changes

Jul 24, 2024

View reviewed changes

JelleZijlstra left a comment

Copy link

Copy Markdown

Member

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 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'?
>>>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting changes and removed awaiting merge labels

Jul 24, 2024

bedevere-app Bot commented Jul 24, 2024

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn commented Jul 24, 2024

Copy link

Copy Markdown

Member Author

@JelleZijlstra yeap, I even wrote a test-case for that: https://github.com/python/cpython/pull/122232/files#diff-212e368b34eb9b134f87e765787d6d26b747235a358e13da8922f83861c0d676R4129-R4133

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Address review

7af17e3

sobolevn requested a review from JelleZijlstra

July 24, 2024 13:43

sobolevn mentioned this pull request

Jul 25, 2024

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

Closed

sobolevn commented Jul 25, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn commented Aug 1, 2024

Copy link

Copy Markdown

Member Author

Friendly ping @JelleZijlstra

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra reviewed

Aug 1, 2024

View reviewed changes

JelleZijlstra left a comment

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn, AlexWaygood, and carljm reacted with thumbs up emoji

All reactions

👍 3 reactions

sobolevn commented Aug 1, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn closed this

Aug 1, 2024

Reviewers

JelleZijlstra

JelleZijlstra requested changes

AlexWaygood

AlexWaygood approved these changes

ericvsmith

Awaiting requested review from ericvsmith

ericvsmith is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes needs backport to 3.12

only security fixes

needs backport to 3.13

only security fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

sobolevn commented Jul 24, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Jul 24, 2024

Uh oh!

sobolevn commented Jul 24, 2024

Uh oh!

sobolevn commented Jul 25, 2024

Uh oh!

sobolevn commented Aug 1, 2024

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Aug 1, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sobolevn commented Jul 24, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.