bpo-45524: fix `get_type_hints` with dataclasses `init` generation #29158

sobolevn · 2021-10-22T12:15:32Z

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

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!

Lib/dataclasses.py

AlexWaygood · 2021-10-22T13:22:53Z

Lib/test/test_typing.py

+    def test_dataclass_from_another_module(self):
+        from test import dataclass_textanno
+        from dataclasses import dataclass
+
+        @dataclass
+        class Default(dataclass_textanno.Bar):
+            pass
+
+        @dataclass(init=False)
+        class WithInitFalse(dataclass_textanno.Bar):
+            pass
+
+        @dataclass(init=False)
+        class CustomInit(dataclass_textanno.Bar):
+            def __init__(self, foo: dataclass_textanno.Foo) -> None:
+                pass
+
+        classes = [
+            Default,
+            WithInitFalse,
+            CustomInit,
+            dataclass_textanno.WithFutureInit,
+        ]
+        for klass in classes:
+            with self.subTest(klass=klass):
+                self.assertEqual(
+                    get_type_hints(klass),
+                    {'foo': dataclass_textanno.Foo},
+                )
+                self.assertEqual(get_type_hints(klass.__new__), {})
+                self.assertEqual(
+                    get_type_hints(klass.__init__),
+                    {'foo': dataclass_textanno.Foo, 'return': type(None)},
+                )
+


Bikeshedding point, but: would it be better to put these tests in test_dataclasses? For the similar issue with TypedDict, the new tests were put next to the other TypedDict tests. Moreover, given that this is a very specific issue with the way that the dataclasses module generates __init__ functions, it seems like more of a dataclasses bug than a get_type_hints bug.

I'm fine with any decision from maintainers 👍
Both cases make sense.

Lib/test/test_typing.py

sobolevn · 2021-10-22T14:05:10Z

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 · 2021-10-22T14:35:23Z

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 😆

Lib/test/test_typing.py

sobolevn · 2021-10-26T22:23:33Z

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

AlexWaygood · 2021-10-28T14:59:20Z

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 · 2021-10-29T07:02:40Z

@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 · 2021-10-29T18:09:11Z

@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!

Lib/dataclasses.py

Lib/test/dataclass_textanno2.py

sobolevn · 2021-10-29T18:39:41Z

Thanks! Done 👍

github-actions · 2021-11-29T00:06:17Z

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

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

…taclass

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

sobolevn · 2021-11-29T08:56:03Z

Rebased 🙂

Friendly ping to @ericvsmith and @Fidget-Spinner

ericvsmith · 2022-10-04T17:48:55Z

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

sobolevn requested review from ericvsmith, Fidget-Spinner and gvanrossum as code owners Oct 22, 2021

bedevere-bot added the awaiting review label Oct 22, 2021

the-knights-who-say-ni added the CLA signed label Oct 22, 2021

AlexWaygood requested changes Oct 22, 2021

View changes

bedevere-bot added awaiting core review and removed awaiting review labels Oct 22, 2021

AlexWaygood reviewed Oct 22, 2021

View changes

Lib/test/test_typing.py Outdated Show resolved Hide resolved

gvanrossum reviewed Oct 22, 2021

View changes

Lib/test/test_typing.py Outdated Show resolved Hide resolved

AlexWaygood reviewed Oct 29, 2021

View changes

Lib/dataclasses.py Outdated Show resolved Hide resolved

Lib/test/dataclass_textanno2.py Outdated Show resolved Hide resolved

Lib/test/dataclass_textanno2.py Outdated Show resolved Hide resolved

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

sobolevn and others added 9 commits Nov 29, 2021

bpo-45524: fix get_type_hints with dataclasses __init__ generation

acd9750

Update Lib/dataclasses.py

e057993

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Update Lib/test/test_typing.py

35015d0

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Ever more wild globals manipulation

a60d850

Uncomment tests

304a658

Move tests from test_typing to test_dataclasses

720924e

Manually resolve string types into ForwardRef in __init__ of a da 1�7

97a3496

…taclass

Update Lib/test/dataclass_textanno2.py

8a32156

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Address review

df5fda9

sobolevn force-pushed the issue-45524 branch from 7205490 to df5fda9 Compare Nov 29, 2021

github-actions bot removed the stale Stale PR or inactive for long period of time. label Nov 30, 2021

AlexWaygood mentioned this pull request Jan 12, 2022

bpo-46333: Honor module parameter in ForwardRef #30536

Merged

JelleZijlstra approved these changes Mar 10, 2022

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 10, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

Merge branch 'main' into issue-45524

3ad08f5

sobolevn closed this Aug 28, 2022

sobolevn reopened this Aug 28, 2022

ericvsmith self-assigned this Oct 4, 2022

hauntsaninja added the DO-NOT-MERGE label Jan 7, 2023

bpo-45524: fix `get_type_hints` with dataclasses `init` generation #29158

bpo-45524: fix `get_type_hints` with dataclasses `init` generation #29158

sobolevn commented Oct 22, 2021 •

edited

AlexWaygood left a comment •

edited

AlexWaygood Oct 22, 2021

sobolevn Oct 22, 2021 •

edited

AlexWaygood Oct 22, 2021

sobolevn commented Oct 22, 2021 •

edited

sobolevn commented Oct 22, 2021

sobolevn commented Oct 26, 2021

AlexWaygood commented Oct 28, 2021

sobolevn commented Oct 29, 2021 •

edited

AlexWaygood commented Oct 29, 2021

sobolevn commented Oct 29, 2021

github-actions bot commented Nov 29, 2021

sobolevn commented Nov 29, 2021

ericvsmith commented Oct 4, 2022

bpo-45524: fix get_type_hints with dataclasses __init__ generation #29158

Are you sure you want to change the base?

bpo-45524: fix get_type_hints with dataclasses __init__ generation #29158

Conversation

sobolevn commented Oct 22, 2021 • edited

AlexWaygood left a comment • edited

Choose a reason for hiding this comment

AlexWaygood Oct 22, 2021

Choose a reason for hiding this comment

sobolevn Oct 22, 2021 • edited

Choose a reason for hiding this comment

AlexWaygood Oct 22, 2021

Choose a reason for hiding this comment

sobolevn commented Oct 22, 2021 • edited

sobolevn commented Oct 22, 2021

sobolevn commented Oct 26, 2021

AlexWaygood commented Oct 28, 2021

sobolevn commented Oct 29, 2021 • edited

AlexWaygood commented Oct 29, 2021

sobolevn commented Oct 29, 2021

github-actions bot commented Nov 29, 2021

sobolevn commented Nov 29, 2021

ericvsmith commented Oct 4, 2022

bpo-45524: fix `get_type_hints` with dataclasses `init` generation #29158

bpo-45524: fix `get_type_hints` with dataclasses `init` generation #29158

sobolevn commented Oct 22, 2021 •

edited

AlexWaygood left a comment •

edited

sobolevn Oct 22, 2021 •

edited

sobolevn commented Oct 22, 2021 •

edited

sobolevn commented Oct 29, 2021 •

edited