◐ Shell
reader mode source ↗
Skip to content

gh-89687: fix get_type_hints with dataclasses __init__ generation#29158

Closed
sobolevn wants to merge 11 commits into
python:mainfrom
sobolevn:issue-45524
Closed

gh-89687: fix get_type_hints with dataclasses __init__ generation#29158
sobolevn wants to merge 11 commits into
python:mainfrom
sobolevn:issue-45524

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 22, 2021 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

This is a rather brave idea to manipulate globals, but it looks like it works.
It can backfire is some corner cases, though. But, I am not able to think about any. Ideas? 🙂

https://bugs.python.org/issue45524

CC @AlexWaygood @aidan.b.clark

AlexWaygood
AlexWaygood requested changes Oct 22, 2021
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Looks like a good fix -- the test script I linked to in the BPO issue passes fine (as do the tests you've added, obviously). Nice job!

@sobolevn

sobolevn commented Oct 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I found a corner case: when I define a proxy module, say dataclass_textanno2.py, with this content:

from __future__ import annotations

import dataclasses
from test import dataclass_textanno  # We need to be sure that `Foo` is not in scope


class Custom:
    pass


@dataclasses.dataclass
class Child(dataclass_textanno.Bar):
    custom: Custom


@dataclasses.dataclass(init=False)
class WithFutureInit(Child):
    def __init__(self, foo: dataclass_textanno.Foo, custom: Custom) -> None:
        pass

Then, this test does not pass:

def test_dataclass_from_proxy_module(self):
        from test import dataclass_textanno, dataclass_textanno2
        from dataclasses import dataclass

        @dataclass
        class Default(dataclass_textanno2.Child):
            pass

       self.assertEqual(
                    get_type_hints(Defualt.__init__),
                    {'foo': dataclass_textanno.Foo, 'custom': dataclass_textanno2.Custom,  'return': type(None)},
                )

Output:

======================================================================
ERROR: test_dataclass_from_proxy_module (test.test_typing.GetTypeHintTests) (klass=<class 'test.test_typing.GetTypeHintTests.test_dataclass_from_proxy_module.<locals>.Default'>)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/Lib/test/test_typing.py", line 3376, in test_dataclass_from_proxy_module
    get_type_hints(klass.__init__),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/typing.py", line 1852, in get_type_hints
    value = _eval_type(value, globalns, localns)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/typing.py", line 334, in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/typing.py", line 700, in _evaluate
    eval(self.__forward_code__, globalns, localns),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<string>", line 1, in <module>
NameError: name 'Custom' is not defined

----------------------------------------------------------------------
Ran 1 test in 0.044s

FAILED (errors=1)
test test_typing failed
test_typing failed (1 error)

== Tests result: FAILURE ==

1 test failed:
    test_typing

@sobolevn

sobolevn commented Oct 22, 2021

Copy link
Copy Markdown
Member Author

Now it is even more wild. I collect globals from all other mro items especially for __init__ annotations.
This is literally one of the craziest things I've ever done 😆

@sobolevn

sobolevn commented Oct 26, 2021

Copy link
Copy Markdown
Member Author

One more case: same names obviously get overriden by the current logic. I will try something else.

@AlexWaygood

AlexWaygood commented Oct 28, 2021

Copy link
Copy Markdown
Member

One more case: same names obviously get overriden by the current logic. I will try something else.

Not sure I understand. What's a failing test case?

@sobolevn

sobolevn commented Oct 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@AlexWaygood done! Here's the failing test case: https://github.com/python/cpython/pull/29158/files#diff-f0436842533448f8f02a19081d7c9cc8372b6685756caf32c83dfecebbee68e7R3189-R3219

And here's my new solution: https://github.com/python/cpython/pull/29158/files#diff-44ce2dc1c4922b2f5cf7631d8f86cc569a4c25eb003aaecdc2bc22eb9163d5f5R455-R468

I now manually resolve str types into ForwardRef if they are used for __init__.
This looks like the most logical and easy way to solve our problem.

@AlexWaygood

AlexWaygood commented Oct 29, 2021

Copy link
Copy Markdown
Member

@AlexWaygood done! Here's the failing test case: https://github.com/python/cpython/pull/29158/files#diff-f0436842533448f8f02a19081d7c9cc8372b6685756caf32c83dfecebbee68e7R3189-R3219

And here's my new solution: https://github.com/python/cpython/pull/29158/files#diff-44ce2dc1c4922b2f5cf7631d8f86cc569a4c25eb003aaecdc2bc22eb9163d5f5R455-R468

I now manually resolve str types into ForwardRef if they are used for __init__. This looks like the most logical and easy way to solve our problem.

Ahh, I understand -- thanks!

@sobolevn

sobolevn commented Oct 29, 2021

Copy link
Copy Markdown
Member Author

Thanks! Done 👍

@github-actions

github-actions Bot commented Nov 29, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Nov 29, 2021
@sobolevn

sobolevn commented Nov 29, 2021

Copy link
Copy Markdown
Member Author

Rebased 🙂

Friendly ping to @ericvsmith and @Fidget-Spinner

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Nov 30, 2021
@AlexWaygood AlexWaygood mentioned this pull request Jan 12, 2022
Merged
@sobolevn sobolevn closed this Aug 28, 2022
@sobolevn sobolevn reopened this Aug 28, 2022
@ericvsmith

ericvsmith commented Oct 4, 2022

Copy link
Copy Markdown
Member

I'm putting this off until the SC makes a decision on PEP 649.

@ericvsmith ericvsmith self-assigned this Oct 4, 2022
@aidanbclark aidanbclark mannequin mentioned this pull request Aug 16, 2024
Open
@picnixz picnixz changed the title bpo-45524: fix get_type_hints with dataclasses __init__ generation Feb 24, 2025
@Tishka17

Tishka17 commented Jul 28, 2025

Copy link
Copy Markdown

PEP 649 is accepted and implemented in 3.14, so what should be done with this PR?

@sobolevn

sobolevn commented Jul 28, 2025

Copy link
Copy Markdown
Member Author

I lost context of this, please recreate :)

@sobolevn sobolevn closed this Jul 28, 2025
@Tishka17 Tishka17 mentioned this pull request Jul 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@AlexWaygood AlexWaygood AlexWaygood requested changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@ericvsmith ericvsmith Awaiting requested review from ericvsmith ericvsmith is a code owner

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Assignees

@ericvsmith ericvsmith

Labels

awaiting merge DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.