Fix `Union[..., NoneType]` injection by `get_type_hints` if a `None` default value is used. #482

Daraan · 2024-10-09T09:22:11Z

Fixes #310

This PR reverts injection of Union[..., NoneType] by typing.get_type_hints in Python <3.11 if a function uses a None default value.

Daraan · 2024-10-09T09:25:32Z

src/typing_extensions.py

+            # Values was not modified or original is already Optional
+            if original_value == value or _could_be_inserted_optional(original_value):
+                continue
+            # NoneType was added to value


Alternatively hints[name] = original_value which should be equivalent. I wonder which would be the safer alternative.

Using original_value is incorrect as we may have modified the internals of the hint. For example, get_type_hints() turns List["int"] into List[int].

Thank you for the feedback. Yes, it should have been piped trough _eval_type as well. Can you take a look again?

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra

This PR is incorrect for this example:

>>> def f(x: Union[str, None, "str"] = None): pass
... 
>>> typing_extensions.get_type_hints(f)
{'x': <class 'str'>}

I am not sure this approach is viable.

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-09T11:58:43Z

src/typing_extensions.py

+            # Values was not modified or original is already Optional
+            if original_value == value or _could_be_inserted_optional(original_value):
+                continue
+            # NoneType was added to value


Using original_value is incorrect as we may have modified the internals of the hint. For example, get_type_hints() turns List["int"] into List[int].

Daraan

What are your thoughts on the "viability"? For that one example or in general?

I tried to improve the recreation of the typing.get_type_hints path that is taken before the injection.

Daraan · 2024-10-09T16:49:42Z

src/typing_extensions.py

+            # Values was not modified or original is already Optional
+            if original_value == value or _could_be_inserted_optional(original_value):
+                continue
+            # NoneType was added to value


Thank you for the feedback. Yes, it should have been piped trough _eval_type as well. Can you take a look again?

src/typing_extensions.py

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-09T18:19:09Z

I haven't thought too hard about examples that might break things, but I'm concerned about using == to check whether the two annotations are the same; I don't know if we can rely on equality of annotation objects working reliably for this.

Daraan · 2024-10-10T16:07:34Z

I haven't thought too hard about examples that might break things, but I'm concerned about using == to check whether the two annotations are the same; I don't know if we can rely on equality of annotation objects working reliably for this.

Do you refer to assertEqual here, or also for the ones in typing_extensions.py?

I'll also added hash checks, which are fine. str and repr are fine, too.
assertIs fails in some(to-many) cases depending on the python version.
In essence the problem can be seen here:

import typing
from typing import List
a = List["str"]

print(typing._eval_type(a, None, None) is typing._eval_type(a, None, None))  # False

typing._eval_type uses _GenericAlias.copy_with or in some cases creates a new GenericAlias which do not make use of the caches.

fixed wrong variable and format

JelleZijlstra · 2024-10-21T11:48:10Z

src/test_typing_extensions.py

+                with self.subTest("Check str and repr"):
+                    if skip_reason == "UnionType not preserved in 3.10":
+                        self.skipTest(skip_reason)
+                    self.assertEqual(str(type_hints) + repr(type_hints), str(expected) + repr(expected))


Concatenating these seems a bit odd. In any case, type_hints is a dictionary, so the str() and repr() are the same.

Thanks for pointing this out I was not 100% sure here and kept a lazy version. Changed to only repr.

src/test_typing_extensions.py

src/typing_extensions.py

JelleZijlstra · 2024-10-21T11:53:12Z

src/typing_extensions.py

+            ):
+                continue
+            original_value = original_hints[name]
+            if original_value is None:  # should not happen


Why shouldn't this happen? You can put None in annotations.

At this point a None value should already be converted to NoneType in original_hints. I think these two lines are redundant but to be safe I added the double check.

Added a comment to clarify this.

JelleZijlstra · 2024-10-21T11:57:47Z

src/typing_extensions.py

+            if sys.version_info < (3, 9) and get_origin(original_evaluated) is Union:
+                # Union[str, None, "str"] is not reduced to Union[str, None]
+                original_evaluated = Union[original_evaluated.__args__]
+            # Compare if values differ
+            if original_evaluated != value:
+                hints[name] = original_evaluated


Suggested change

if sys.version_info < (3, 9) and get_origin(original_evaluated) is Union:

# Union[str, None, "str"] is not reduced to Union[str, None]

original_evaluated = Union[original_evaluated.__args__]

# Compare if values differ

if original_evaluated != value:

hints[name] = original_evaluated

hints[name] = original_evaluated

Would this also work?

Yes it would. However there are minor differences involving identities and caching.

typing._eval_type uses _GenericAlias.copy_with or in some cases creates a new GenericAlias which do not make use of the caches.

If a ForwardRef is involved original_evaluated can be a new object as _tp_cache is not queried. Not changing hints["x"] will return an object that matches expected["x"] by identity in more cases.

That's a minor feature and I am fine with dropping it if you want; for now added a comment to clarify this.

Co-authored-by: Jelle Zijlstra <[email protected]>

+ updated comments

src/test_typing_extensions.py

JelleZijlstra · 2024-10-21T16:56:44Z

src/typing_extensions.py

@@ -1282,7 +1282,7 @@ def _clean_optional(obj, hints, globalns=None, localns=None):
            ):
                continue
            original_value = original_hints[name]
-            if original_value is None:  # should not happen
+            if original_value is None:  # should be NoneType already; check just in case


It shouldn't be NoneType already, since we get this directly from __annotations__.

Excuse me I have argued wrongly. You are right that None would be from __annotations__. In such a case value should be NoneType and _could_be_inserted_optional(NoneType) would have already skipped this iteration (get_origin(NoneType) is not Union).

That's why original_value would still never be None here; updated the comment.

Co-authored-by: Jelle Zijlstra <[email protected]>

Daraan added 3 commits October 9, 2024 11:18

Fix get_type_hints with None default interaction

376ae56

moved section

24c5602

removed empty line

7f000b1

Daraan commented Oct 9, 2024

View reviewed changes

JelleZijlstra requested changes Oct 9, 2024

View reviewed changes

Daraan marked this pull request as draft October 9, 2024 13:07

Daraan added 2 commits October 9, 2024 18:34

Restructured tests and applied suggested changes

8cc4464

+1 test and formatting

881ffee

Daraan marked this pull request as ready for review October 9, 2024 16:46

Daraan commented Oct 9, 2024

View reviewed changes

Daraan added 3 commits October 10, 2024 17:56

Increase test coverage

58082b9

use key access and not values

5c94bd6

str test passes as well

b42e203

removed invalid 3.8 code

ed552e4

Daraan marked this pull request as draft October 10, 2024 17:45

Daraan added 3 commits October 10, 2024 23:34

Extended tests

313ddd8

fixed wrong variable and format

refinement of tests, more tests with aliases

3ba4ee7

Merge 'main' into get_type_hints

6ffcd23

Daraan marked this pull request as ready for review October 11, 2024 15:10

Merge branch 'main' into get_type_hints

403e7ce

JelleZijlstra reviewed Oct 21, 2024

View reviewed changes

Daraan and others added 2 commits October 21, 2024 15:10

Add comment for special case

52c93e9

Co-authored-by: Jelle Zijlstra <[email protected]>

Assure UnionType stays UnionType; check repr only

a1777f3

+ updated comments

JelleZijlstra reviewed Oct 21, 2024

View reviewed changes

src/test_typing_extensions.py Outdated Show resolved Hide resolved

JelleZijlstra reviewed Oct 21, 2024

View reviewed changes

Daraan and others added 3 commits October 21, 2024 18:57

Update src/test_typing_extensions.py

9e59796

Co-authored-by: Jelle Zijlstra <[email protected]>

Merge remote-tracking branch 'upstream/main' into get_type_hints

1761a43

Corrected and clarified case

54b8eb0

Daraan added 2 commits October 22, 2024 16:14

Merge remote-tracking branch 'upstream/main' into get_type_hints

5612f6f

Merge branch 'main' into get_type_hints

91075ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Union[..., NoneType]` injection by `get_type_hints` if a `None` default value is used. #482

Fix `Union[..., NoneType]` injection by `get_type_hints` if a `None` default value is used. #482

Daraan commented Oct 9, 2024

Daraan Oct 9, 2024

JelleZijlstra Oct 9, 2024

Daraan Oct 9, 2024

JelleZijlstra left a comment

JelleZijlstra Oct 9, 2024

Daraan left a comment

Daraan Oct 9, 2024

JelleZijlstra commented Oct 9, 2024

Daraan commented Oct 10, 2024 •

edited

Loading

JelleZijlstra Oct 21, 2024

Daraan Oct 21, 2024 •

edited

Loading

JelleZijlstra Oct 21, 2024

Daraan Oct 21, 2024

JelleZijlstra Oct 21, 2024

Daraan Oct 21, 2024

JelleZijlstra Oct 21, 2024

Daraan Oct 21, 2024 •

edited

Loading

Fix Union[..., NoneType] injection by get_type_hints if a None default value is used. #482

Are you sure you want to change the base?

Fix Union[..., NoneType] injection by get_type_hints if a None default value is used. #482

Conversation

Daraan commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daraan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra commented Oct 9, 2024

Daraan commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Daraan Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daraan Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Fix `Union[..., NoneType]` injection by `get_type_hints` if a `None` default value is used. #482

Fix `Union[..., NoneType]` injection by `get_type_hints` if a `None` default value is used. #482

Daraan commented Oct 10, 2024 •

edited

Loading

Daraan Oct 21, 2024 •

edited

Loading

Daraan Oct 21, 2024 •

edited

Loading